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

[Oleksandr]

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