Re: [PATCH V5] xen/gnttab: Store frame GFN in struct page_info on Arm

[Oleksandr <olekstysh@xxxxxxxxx>]

On 06.01.22 16:20, Jan Beulich wrote:


Hi Jan

> On 06.01.2022 00:11, Oleksandr Tyshchenko wrote:
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -57,6 +57,9 @@
> >   #define PGT_count_width   PG_shift(8)
> >   #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
> >   
> > +/* No arch-specific initialization pattern is needed for the type_info field. */
> > +#define PGT_TYPE_INFO_INIT_PATTERN   0
> I think this should be inside an #ifndef in page_alloc.c.

ok, will do.

I hope you meant the following:

#ifndef PGT_TYPE_INFO_INIT_PATTERN
#define PGT_TYPE_INFO_INIT_PATTERN 0
#endif


> Also the name suggests usage for all kinds of pages, as you did
> outline on the v4 thread. Yet ...
>
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2159,6 +2159,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
> >   void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
> >   {
> >       struct page_info *pg;
> > +    unsigned int i;
> >   
> >       ASSERT(!in_irq());
> >   
> > @@ -2167,6 +2168,9 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
> >       if ( unlikely(pg == NULL) )
> >           return NULL;
> >   
> > +    for ( i = 0; i < (1u << order); i++ )
> > +        pg[i].u.inuse.type_info |= PGT_TYPE_INFO_INIT_PATTERN;
> > +
> >       return page_to_virt(pg);
> >   }
> >   
> > @@ -2214,7 +2218,10 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
> >           return NULL;
> >   
> >       for ( i = 0; i < (1u << order); i++ )
> > +    {
> >           pg[i].count_info |= PGC_xen_heap;
> > +        pg[i].u.inuse.type_info |= PGT_TYPE_INFO_INIT_PATTERN;
> > +    }
> >   
> >       return page_to_virt(pg);
> >   }
> ... you now only use it in alloc_xenheap_pages().
Yes, I decided to go with your initial suggestion to OR the pattern here.


> Further, did you check that when the value is 0 the compiler would
> fully eliminate the new code in both flavors of the function?

No, I didn't. But I have just rechecked on Arm64 
(!CONFIG_SEPARATE_XENHEAP) and Arm32 (CONFIG_SEPARATE_XENHEAP).
If I remove PGT_TYPE_INFO_INIT_PATTERN definition from Arm's header I 
don't see any assembler output generated for following expression in 
both cases:
pg[i].u.inuse.type_info |= PGT_TYPE_INFO_INIT_PATTERN;
where PGT_TYPE_INFO_INIT_PATTERN is 0


> Finally, leaving aside the aspect of where the value should be used,
> and also leaving aside the fact that the T in PGT is redundant with
> TYPE_INFO, I think something like PGT_TYPE_INFO_INITIALIZER would be
> better than having "pattern" in the name. But this may just be me ...
I am perfectly fine with new name.


In case you don't have any other objections shall I re-push v5.1 with 
proposed adjustments now?

Thank you.


> Jan
--
Regards,

Oleksandr Tyshchenko