From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Rework Arm implementation to store grant table frame GFN
in struct page_info directly instead of keeping it in
standalone status/shared arrays.

To cover 64-bit/40-bit IPA on Arm64/Arm32 we need the new
field to hold 52-bit/28-bit respectively. In order to not
grow the size of struct page_info reduce the type_info field
which current context won't suffer. Add new frame_gfn field
on top of the freed bits and introduce access macros. Update
existing macros to deal with GFN value according to new
location.

Also update P2M code on Arm to clean said GFN when putting
a reference on the grant table page in p2m_put_l3_page().

This patch is intended to fix the potential issue on Arm
which might happen when remapping grant-table frame.
A guest (or the toolstack) will unmap the grant-table frame
using XENMEM_remove_physmap. This is a generic hypercall,
so on x86, we are relying on the fact the M2P entry will
be cleared on removal. For architecture without the M2P,
the GFN would still be present in the grant frame/status
array. So on the next call to map the page, we will end up to
request the P2M to remove whatever mapping was the given GFN.
This could well be another mapping.

Besides that, this patch simplifies arch code on Arm by
removing arrays and corresponding management code and
as the result gnttab_init_arch/gnttab_destroy_arch helpers
and struct grant_table_arch become useless and can be dropped.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
You can find the related discussions at:
https://lore.kernel.org/xen-devel/93d0df14-2c8a-c2e3-8c51-54412190171c@xen.org/
https://lore.kernel.org/xen-devel/1628890077-12545-1-git-send-email-olekstysh@gmail.com/

! Please note, there is still unresolved locking question here for which
I failed to find a suitable solution !

According to the internal conversation:
Now the GFN field in the struct page_info is accessed from
gnttab_set_frame_gfn() in the grant table code and from page_set_frame_gfn()
in the P2M code (the former uses the latter).

We need to prevent the concurrent access to this field. But, we cannot grab
the grant lock from the P2M code because we will introduce a lock inversion.
The page_set_frame_gfn() will be called from the P2M code with the p2m lock held
and then acquire the grant table lock. The gnttab_map_frame() will do the inverse.
---
 xen/arch/arm/p2m.c                | 18 ++++++++++---
 xen/common/grant_table.c          |  9 -------
 xen/common/page_alloc.c           |  1 +
 xen/include/asm-arm/grant_table.h | 53 +++++++++++++--------------------------
 xen/include/asm-arm/mm.h          | 27 ++++++++++++++++----
 xen/include/asm-x86/grant_table.h |  5 ----
 xen/include/asm-x86/mm.h          |  2 ++
 7 files changed, 56 insertions(+), 59 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index eff9a10..7bef210 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -718,8 +718,10 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
  * TODO: Handle superpages, for now we only take special references for leaf
  * pages (specifically foreign ones, which can't be super mapped today).
  */
-static void p2m_put_l3_page(const lpae_t pte)
+static void p2m_put_l3_page(struct p2m_domain *p2m, const lpae_t pte)
 {
+    mfn_t mfn = lpae_get_mfn(pte);
+
     ASSERT(p2m_is_valid(pte));
 
     /*
@@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
      */
     if ( p2m_is_foreign(pte.p2m.type) )
     {
-        mfn_t mfn = lpae_get_mfn(pte);
-
         ASSERT(mfn_valid(mfn));
         put_page(mfn_to_page(mfn));
     }
+
+#ifdef CONFIG_GRANT_TABLE
+    /*
+     * Check whether we deal with grant table page. As the grant table page
+     * is xen_heap page and its entry has known p2m type, detect it and mark
+     * the stored GFN as invalid.
+     */
+    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
+        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
+#endif
 }
 
 /* Free lpae sub-tree behind an entry */
@@ -768,7 +778,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
         p2m->stats.mappings[level]--;
         /* Nothing to do if the entry is a super-page. */
         if ( level == 3 )
-            p2m_put_l3_page(entry);
+            p2m_put_l3_page(p2m, entry);
         return;
     }
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index b1930e2..9cc3550 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -93,8 +93,6 @@ struct grant_table {
 
     /* Domain to which this struct grant_table belongs. */
     const struct domain *domain;
-
-    struct grant_table_arch arch;
 };
 
 unsigned int __read_mostly opt_max_grant_frames = 64;
@@ -1981,14 +1979,9 @@ int grant_table_init(struct domain *d, int max_grant_frames,
 
     grant_write_lock(gt);
 
-    ret = gnttab_init_arch(gt);
-    if ( ret )
-        goto unlock;
-
     /* gnttab_grow_table() allocates a min number of frames, so 0 is okay. */
     ret = gnttab_grow_table(d, 0);
 
- unlock:
     grant_write_unlock(gt);
 
  out:
@@ -3895,8 +3888,6 @@ grant_table_destroy(
     if ( t == NULL )
         return;
 
-    gnttab_destroy_arch(t);
-
     for ( i = 0; i < nr_grant_frames(t); i++ )
         free_xenheap_page(t->shared_raw[i]);
     xfree(t->shared_raw);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 958ba0c..f7428f9 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1021,6 +1021,7 @@ static struct page_info *alloc_heap_pages(
 
         /* Initialise fields which have other uses for free pages. */
         pg[i].u.inuse.type_info = 0;
+        page_arch_init(&pg[i]);
         page_set_owner(&pg[i], NULL);
 
     }
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 0ce77f9..77dfa8e 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -11,11 +11,6 @@
 #define INITIAL_NR_GRANT_FRAMES 1U
 #define GNTTAB_MAX_VERSION 1
 
-struct grant_table_arch {
-    gfn_t *shared_gfn;
-    gfn_t *status_gfn;
-};
-
 static inline void gnttab_clear_flags(struct domain *d,
                                       unsigned int mask, uint16_t *addr)
 {
@@ -46,35 +41,11 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
 #define gnttab_dom0_frames()                                             \
     min_t(unsigned int, opt_max_grant_frames, PFN_DOWN(_etext - _stext))
 
-#define gnttab_init_arch(gt)                                             \
-({                                                                       \
-    unsigned int ngf_ = (gt)->max_grant_frames;                          \
-    unsigned int nsf_ = grant_to_status_frames(ngf_);                    \
-                                                                         \
-    (gt)->arch.shared_gfn = xmalloc_array(gfn_t, ngf_);                  \
-    (gt)->arch.status_gfn = xmalloc_array(gfn_t, nsf_);                  \
-    if ( (gt)->arch.shared_gfn && (gt)->arch.status_gfn )                \
-    {                                                                    \
-        while ( ngf_-- )                                                 \
-            (gt)->arch.shared_gfn[ngf_] = INVALID_GFN;                   \
-        while ( nsf_-- )                                                 \
-            (gt)->arch.status_gfn[nsf_] = INVALID_GFN;                   \
-    }                                                                    \
-    else                                                                 \
-        gnttab_destroy_arch(gt);                                         \
-    (gt)->arch.shared_gfn ? 0 : -ENOMEM;                                 \
-})
-
-#define gnttab_destroy_arch(gt)                                          \
-    do {                                                                 \
-        XFREE((gt)->arch.shared_gfn);                                    \
-        XFREE((gt)->arch.status_gfn);                                    \
-    } while ( 0 )
-
 #define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
     do {                                                                 \
-        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
-            (gfn);                                                       \
+        struct page_info *pg_ = (st) ? gnttab_status_page(gt, idx)       \
+                                     : gnttab_shared_page(gt, idx);      \
+        page_set_frame_gfn(pg_, gfn);                                    \
     } while ( 0 )
 
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
@@ -82,11 +53,21 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
         : gnttab_shared_gfn(NULL, gt, idx);                              \
 })
 
-#define gnttab_shared_gfn(d, t, i)                                       \
-    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
+#define gnttab_shared_page(t, i)    \
+    mfn_to_page(_mfn(__virt_to_mfn((t)->shared_raw[i])))
+
+#define gnttab_status_page(t, i)    \
+    mfn_to_page(_mfn(__virt_to_mfn((t)->status[i])))
 
-#define gnttab_status_gfn(d, t, i)                                       \
-    (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
+#define gnttab_shared_gfn(d, t, i) ({                                    \
+    struct page_info *pg_ = gnttab_shared_page(t, i);                    \
+    page_get_frame_gfn(pg_);                                             \
+})
+
+#define gnttab_status_gfn(d, t, i) ({                                    \
+    struct page_info *pg_ = gnttab_status_page(t, i);                    \
+    page_get_frame_gfn(pg_);                                             \
+})
 
 #define gnttab_need_iommu_mapping(d)                    \
     (is_domain_direct_mapped(d) && is_iommu_enabled(d))
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index ded74d2..dea4c49 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -39,7 +39,15 @@ struct page_info
         /* Page is in use: ((count_info & PGC_count_mask) != 0). */
         struct {
             /* Type reference count and various PGT_xxx flags and fields. */
-            unsigned long type_info;
+            unsigned long type_info:4;
+
+            /*
+             * Stored GFN if page is used for grant table frame
+             * (bits 59(27)-0).
+             */
+#define PGT_FRAME_GFN_WIDTH      (BITS_PER_LONG - 4)
+#define PGT_INVALID_FRAME_GFN    _gfn((1UL << PGT_FRAME_GFN_WIDTH) - 1)
+            unsigned long frame_gfn:PGT_FRAME_GFN_WIDTH;
         } inuse;
         /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
         union {
@@ -94,12 +102,12 @@ struct page_info
 #define PG_shift(idx)   (BITS_PER_LONG - (idx))
 #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
 
-#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
-#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
-#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
+#define PGT_none          (0UL << 3)  /* no special uses of this page   */
+#define PGT_writable_page (1UL << 3)  /* has writable mappings?         */
+#define PGT_type_mask     (1UL << 3)
 
  /* Count of uses of this frame as its current type. */
-#define PGT_count_width   PG_shift(2)
+#define PGT_count_width   1
 #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
 
  /* Cleared when the owning guest 'frees' this page. */
@@ -163,6 +171,15 @@ extern unsigned long xenheap_base_pdx;
 
 #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
 
+#define page_get_frame_gfn(_p) ({                               \
+    gfn_t gfn_ = _gfn((_p)->u.inuse.frame_gfn);                 \
+    gfn_eq(gfn_, PGT_INVALID_FRAME_GFN) ? INVALID_GFN : gfn_;   \
+})
+
+#define page_set_frame_gfn(_p, _gfn) ((_p)->u.inuse.frame_gfn = gfn_x(_gfn))
+
+#define page_arch_init(_p)   page_set_frame_gfn(_p, INVALID_GFN)
+
 #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
 /* PDX of the first page in the frame table. */
 extern unsigned long frametable_base_pdx;
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 84e3296..0eb018f 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -14,9 +14,6 @@
 
 #define INITIAL_NR_GRANT_FRAMES 1U
 
-struct grant_table_arch {
-};
-
 static inline int create_grant_host_mapping(uint64_t addr, mfn_t frame,
                                             unsigned int flags,
                                             unsigned int cache_flags)
@@ -35,8 +32,6 @@ static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame,
     return replace_grant_pv_mapping(addr, frame, new_addr, flags);
 }
 
-#define gnttab_init_arch(gt) 0
-#define gnttab_destroy_arch(gt) do {} while ( 0 )
 #define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
     mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index cb90527..d2ea39b 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -327,6 +327,8 @@ struct page_info
 
 #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
 
+#define page_arch_init(_p)    do {} while ( 0 )
+
 #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
 extern unsigned long max_page;
 extern unsigned long total_pages;
-- 
2.7.4

On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>       */
>      if ( p2m_is_foreign(pte.p2m.type) )
>      {
> -        mfn_t mfn = lpae_get_mfn(pte);
> -
>          ASSERT(mfn_valid(mfn));
>          put_page(mfn_to_page(mfn));
>      }
> +
> +#ifdef CONFIG_GRANT_TABLE
> +    /*
> +     * Check whether we deal with grant table page. As the grant table page
> +     * is xen_heap page and its entry has known p2m type, detect it and mark
> +     * the stored GFN as invalid.
> +     */
> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
> +#endif

I take it the write done is benign for other Xen heap pages? I suppose
this would want making very explicit, as such an assumption is easy to
go stale by entirely unrelated changes.

I also wonder whether you really mean to include p2m_ram_ro pages here
as well.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1021,6 +1021,7 @@ static struct page_info *alloc_heap_pages(
>  
>          /* Initialise fields which have other uses for free pages. */
>          pg[i].u.inuse.type_info = 0;
> +        page_arch_init(&pg[i]);

u.type_info's count portion also gets checked upon freeing a page.
Don't you want to have an arch-custom check for your new use of the
field as well? Or do you consider it not a problem (bug) if a page
was freed which still has a GFN set on it?

> @@ -82,11 +53,21 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>          : gnttab_shared_gfn(NULL, gt, idx);                              \
>  })
>  
> -#define gnttab_shared_gfn(d, t, i)                                       \
> -    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
> +#define gnttab_shared_page(t, i)    \
> +    mfn_to_page(_mfn(__virt_to_mfn((t)->shared_raw[i])))
> +
> +#define gnttab_status_page(t, i)    \
> +    mfn_to_page(_mfn(__virt_to_mfn((t)->status[i])))

I wonder whether you wouldn't want to at least ASSERT() here that
the virtual address you start from is actually non-NULL.

> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -39,7 +39,15 @@ struct page_info
>          /* Page is in use: ((count_info & PGC_count_mask) != 0). */
>          struct {
>              /* Type reference count and various PGT_xxx flags and fields. */
> -            unsigned long type_info;
> +            unsigned long type_info:4;
> +
> +            /*
> +             * Stored GFN if page is used for grant table frame
> +             * (bits 59(27)-0).

Are these bit numbers correct? I thought Arm, like x86, counted bits
from low to high (unlike PPC, for example)?

> +             */
> +#define PGT_FRAME_GFN_WIDTH      (BITS_PER_LONG - 4)
> +#define PGT_INVALID_FRAME_GFN    _gfn((1UL << PGT_FRAME_GFN_WIDTH) - 1)
> +            unsigned long frame_gfn:PGT_FRAME_GFN_WIDTH;
>          } inuse;
>          /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
>          union {
> @@ -94,12 +102,12 @@ struct page_info
>  #define PG_shift(idx)   (BITS_PER_LONG - (idx))
>  #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
>  
> -#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
> -#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
> -#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
> +#define PGT_none          (0UL << 3)  /* no special uses of this page   */
> +#define PGT_writable_page (1UL << 3)  /* has writable mappings?         */
> +#define PGT_type_mask     (1UL << 3)
>  
>   /* Count of uses of this frame as its current type. */
> -#define PGT_count_width   PG_shift(2)
> +#define PGT_count_width   1
>  #define PGT_count_mask    ((1UL<<PGT_count_width)-1)

Leaving just a single bit seems pretty risky to me, and I can't see
why you do so. You need 52 bits on Arm64. Which means you have 12
bits left. Why not use them in full? Even on Arm32 you have 3 bits
available for the count afaict.

Also there's a disconnect here: If anyone wanted to change the number
of bits used for the various fields, each such change should require
an adjustment in as few independent places as possible. That is, there
shouldn't be multiple uses of literal 4 further up, and there also
shouldn't be a hidden connection between that 4 and the PGT_* values
here. I think you'd better express the GFN as such a PGT_* construct
as well. And you better would keep using PG_mask() and PG_shift().

> @@ -163,6 +171,15 @@ extern unsigned long xenheap_base_pdx;
>  
>  #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
>  
> +#define page_get_frame_gfn(_p) ({                               \
> +    gfn_t gfn_ = _gfn((_p)->u.inuse.frame_gfn);                 \
> +    gfn_eq(gfn_, PGT_INVALID_FRAME_GFN) ? INVALID_GFN : gfn_;   \
> +})
> +
> +#define page_set_frame_gfn(_p, _gfn) ((_p)->u.inuse.frame_gfn = gfn_x(_gfn))
> +
> +#define page_arch_init(_p)   page_set_frame_gfn(_p, INVALID_GFN)

What's the purpose of the leading underscore in the macro parameter
names? They're not in line with the C standard's designation of sub-
namespaces for identifiers. (At least for the x86 counterpart please
read this as a request to drop the underscore there.)

Jan

On 10.09.2021 09:52, Jan Beulich wrote:
> On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
>> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>>       */
>>      if ( p2m_is_foreign(pte.p2m.type) )
>>      {
>> -        mfn_t mfn = lpae_get_mfn(pte);
>> -
>>          ASSERT(mfn_valid(mfn));
>>          put_page(mfn_to_page(mfn));
>>      }
>> +
>> +#ifdef CONFIG_GRANT_TABLE
>> +    /*
>> +     * Check whether we deal with grant table page. As the grant table page
>> +     * is xen_heap page and its entry has known p2m type, detect it and mark
>> +     * the stored GFN as invalid.
>> +     */
>> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
>> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
>> +#endif
> 
> I take it the write done is benign for other Xen heap pages? I suppose
> this would want making very explicit, as such an assumption is easy to
> go stale by entirely unrelated changes.
> 
> I also wonder whether you really mean to include p2m_ram_ro pages here
> as well.

Actually I've meanwhile realized I should put my question here quite
differently: Aren't you effectively introducing an M2P here for Arm,
except that you artificially limit it to the Xen heap pages needed for
the grant table?

Jan

On 13.09.21 09:17, Jan Beulich wrote:

Hi Jan

> On 10.09.2021 09:52, Jan Beulich wrote:
>> On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
>>> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>>>        */
>>>       if ( p2m_is_foreign(pte.p2m.type) )
>>>       {
>>> -        mfn_t mfn = lpae_get_mfn(pte);
>>> -
>>>           ASSERT(mfn_valid(mfn));
>>>           put_page(mfn_to_page(mfn));
>>>       }
>>> +
>>> +#ifdef CONFIG_GRANT_TABLE
>>> +    /*
>>> +     * Check whether we deal with grant table page. As the grant table page
>>> +     * is xen_heap page and its entry has known p2m type, detect it and mark
>>> +     * the stored GFN as invalid.
>>> +     */
>>> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
>>> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
>>> +#endif
>> I take it the write done is benign for other Xen heap pages? I suppose
>> this would want making very explicit, as such an assumption is easy to
>> go stale by entirely unrelated changes.
>>
>> I also wonder whether you really mean to include p2m_ram_ro pages here
>> as well.
> Actually I've meanwhile realized I should put my question here quite
> differently: Aren't you effectively introducing an M2P here for Arm,
> except that you artificially limit it to the Xen heap pages needed for
> the grant table?

Difficult to say, it might indeed look a bit close to M2P, so the answer 
to your question is more yes than no. But, I didn't have a plan to 
introduce M2P on Arm. It turned out that stashing GFN into page_info (as 
was suggested) avoided huge lookups, so we have got MFN->GFN in the end. 
The purpose of this patch was just to fix a potential issue with 
remapping grant-table frame on architecture without the M2P (Arm).


>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko

On 13.09.2021 21:57, Oleksandr wrote:
> 
> On 13.09.21 09:17, Jan Beulich wrote:
> 
> Hi Jan
> 
>> On 10.09.2021 09:52, Jan Beulich wrote:
>>> On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
>>>> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>>>>        */
>>>>       if ( p2m_is_foreign(pte.p2m.type) )
>>>>       {
>>>> -        mfn_t mfn = lpae_get_mfn(pte);
>>>> -
>>>>           ASSERT(mfn_valid(mfn));
>>>>           put_page(mfn_to_page(mfn));
>>>>       }
>>>> +
>>>> +#ifdef CONFIG_GRANT_TABLE
>>>> +    /*
>>>> +     * Check whether we deal with grant table page. As the grant table page
>>>> +     * is xen_heap page and its entry has known p2m type, detect it and mark
>>>> +     * the stored GFN as invalid.
>>>> +     */
>>>> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
>>>> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
>>>> +#endif
>>> I take it the write done is benign for other Xen heap pages? I suppose
>>> this would want making very explicit, as such an assumption is easy to
>>> go stale by entirely unrelated changes.
>>>
>>> I also wonder whether you really mean to include p2m_ram_ro pages here
>>> as well.
>> Actually I've meanwhile realized I should put my question here quite
>> differently: Aren't you effectively introducing an M2P here for Arm,
>> except that you artificially limit it to the Xen heap pages needed for
>> the grant table?
> 
> Difficult to say, it might indeed look a bit close to M2P, so the answer 
> to your question is more yes than no. But, I didn't have a plan to 
> introduce M2P on Arm. It turned out that stashing GFN into page_info (as 
> was suggested) avoided huge lookups, so we have got MFN->GFN in the end. 
> The purpose of this patch was just to fix a potential issue with 
> remapping grant-table frame on architecture without the M2P (Arm).

I understand this is the immediate goal. I wonder though if it's helpful
to make this a special case when it can (I think) easily be made general.
But of course there may be (perhaps Arm-specific) aspects which I'm
simply unaware of.

Jan

On 10.09.21 10:52, Jan Beulich wrote:


Hi Jan

> On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
>> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>>        */
>>       if ( p2m_is_foreign(pte.p2m.type) )
>>       {
>> -        mfn_t mfn = lpae_get_mfn(pte);
>> -
>>           ASSERT(mfn_valid(mfn));
>>           put_page(mfn_to_page(mfn));
>>       }
>> +
>> +#ifdef CONFIG_GRANT_TABLE
>> +    /*
>> +     * Check whether we deal with grant table page. As the grant table page
>> +     * is xen_heap page and its entry has known p2m type, detect it and mark
>> +     * the stored GFN as invalid.
>> +     */
>> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
>> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
>> +#endif
> I take it the write done is benign for other Xen heap pages? I suppose
> this would want making very explicit, as such an assumption is easy to
> go stale by entirely unrelated changes.

Yes, I don't see what bad could happen if we would clear this field for 
non grant table pages. Except grant table pages I could detect only one 
page passed this check here which is, I assume, shared_info page. All 
pages have this field initialized with INVALID_GFN. But *only* grant 
table pages have this field in use. So, I think, no one will suffer. I 
will add a comment. Or you meant something more than just a comment?


>
> I also wonder whether you really mean to include p2m_ram_ro pages here
> as well.

You are right, only p2m_ram_rw is our target.


>
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1021,6 +1021,7 @@ static struct page_info *alloc_heap_pages(
>>   
>>           /* Initialise fields which have other uses for free pages. */
>>           pg[i].u.inuse.type_info = 0;
>> +        page_arch_init(&pg[i]);
> u.type_info's count portion also gets checked upon freeing a page.
> Don't you want to have an arch-custom check for your new use of the
> field as well? Or do you consider it not a problem (bug) if a page
> was freed which still has a GFN set on it?

Hmm, I didn't think about it. Good point. I can't say for sure, but I 
don't think it would be a normal behavior.
The valid GFN field for the page to be freed would mean that something 
doesn't work as expected (the corresponding page table entry wasn't 
freed, or it was, but
we couldn't detect the grant table page, etc). I will probably add 
corresponding page_arch_free() which triggers BUG if GFN is still valid.

But, there is a moment with that. The u.type_info's count portion is 
checked in free_domheap_pages(). So, I think we need to have arch-custom 
check in code path for xenheap_page.

Something like that:

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 958ba0c..bb359ca 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2204,7 +2204,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;
+        arch_init_xenheap_page(&pg[i]);
+    }

      return page_to_virt(pg);
  }
@@ -2222,7 +2225,10 @@ void free_xenheap_pages(void *v, unsigned int order)
      pg = virt_to_page(v);

      for ( i = 0; i < (1u << order); i++ )
+    {
          pg[i].count_info &= ~PGC_xen_heap;
+        arch_free_xenheap_page(&pg[i]);
+    }

      free_heap_pages(pg, order, true);
  }


Where on Arm, these are:

+#define arch_init_xenheap_page(_p)   page_set_frame_gfn(_p, INVALID_GFN)
+#define arch_free_xenheap_page(_p) \
+    BUG_ON(!gfn_eq(page_get_frame_gfn(_p), INVALID_GFN))

And just stubs on x86.

Please let me know if you think otherwise.


>
>> @@ -82,11 +53,21 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>           : gnttab_shared_gfn(NULL, gt, idx);                              \
>>   })
>>   
>> -#define gnttab_shared_gfn(d, t, i)                                       \
>> -    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
>> +#define gnttab_shared_page(t, i)    \
>> +    mfn_to_page(_mfn(__virt_to_mfn((t)->shared_raw[i])))
>> +
>> +#define gnttab_status_page(t, i)    \
>> +    mfn_to_page(_mfn(__virt_to_mfn((t)->status[i])))
> I wonder whether you wouldn't want to at least ASSERT() here that
> the virtual address you start from is actually non-NULL.

Agree, will add.


>
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -39,7 +39,15 @@ struct page_info
>>           /* Page is in use: ((count_info & PGC_count_mask) != 0). */
>>           struct {
>>               /* Type reference count and various PGT_xxx flags and fields. */
>> -            unsigned long type_info;
>> +            unsigned long type_info:4;
>> +
>> +            /*
>> +             * Stored GFN if page is used for grant table frame
>> +             * (bits 59(27)-0).
> Are these bit numbers correct? I thought Arm, like x86, counted bits
> from low to high (unlike PPC, for example)?

I think, these are correct.  Yes, Little Endian is used.


>
>> +             */
>> +#define PGT_FRAME_GFN_WIDTH      (BITS_PER_LONG - 4)
>> +#define PGT_INVALID_FRAME_GFN    _gfn((1UL << PGT_FRAME_GFN_WIDTH) - 1)
>> +            unsigned long frame_gfn:PGT_FRAME_GFN_WIDTH;
>>           } inuse;
>>           /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
>>           union {
>> @@ -94,12 +102,12 @@ struct page_info
>>   #define PG_shift(idx)   (BITS_PER_LONG - (idx))
>>   #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
>>   
>> -#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
>> -#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
>> -#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
>> +#define PGT_none          (0UL << 3)  /* no special uses of this page   */
>> +#define PGT_writable_page (1UL << 3)  /* has writable mappings?         */
>> +#define PGT_type_mask     (1UL << 3)
>>   
>>    /* Count of uses of this frame as its current type. */
>> -#define PGT_count_width   PG_shift(2)
>> +#define PGT_count_width   1
>>   #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
> Leaving just a single bit seems pretty risky to me, and I can't see
> why you do so. You need 52 bits on Arm64. Which means you have 12
> bits left. Why not use them in full? Even on Arm32 you have 3 bits
> available for the count afaict.

Only 1 bit in the type_info field is really used on Arm, but anyway ...


>
> Also there's a disconnect here: If anyone wanted to change the number
> of bits used for the various fields, each such change should require
> an adjustment in as few independent places as possible. That is, there
> shouldn't be multiple uses of literal 4 further up, and there also
> shouldn't be a hidden connection between that 4 and the PGT_* values
> here. I think you'd better express the GFN as such a PGT_* construct
> as well. And you better would keep using PG_mask() and PG_shift().

... I think, this indeed makes sense. If got your request correctly, we 
can avoid disconnect here
and reserve 3-bit field for count for both Arm32 and Arm64. The struct 
page_info's type_info field remains untouched.


diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index ded74d2..8b73e1c 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -99,8 +99,14 @@ struct page_info
  #define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 
63.                 */

   /* Count of uses of this frame as its current type. */
-#define PGT_count_width   PG_shift(2)
-#define PGT_count_mask    ((1UL<<PGT_count_width)-1)
+#define PGT_count_base    PG_shift(4)
+#define PGT_count_mask    PG_mask(7, 4)
+
+/* Stored in bits [27:0] or [59:0] GFN if page is used for grant table 
frame */
+#define PGT_gfn_width     PG_shift(4)
+#define PGT_gfn_mask      ((1UL<<PGT_gfn_width)-1)
+
+#define PGT_INVALID_FRAME_GFN   _gfn(PGT_gfn_mask)

   /* Cleared when the owning guest 'frees' this page. */
  #define _PGC_allocated    PG_shift(1)
@@ -163,6 +169,22 @@ extern unsigned long xenheap_base_pdx;

  #define maddr_get_owner(ma) (page_get_owner(maddr_to_page((ma))))

+#define page_get_frame_gfn(_p) ({                               \
+    gfn_t gfn_ = _gfn((_p)->u.inuse.type_info & PGT_gfn_mask);  \
+    gfn_eq(gfn_, PGT_INVALID_FRAME_GFN) ? INVALID_GFN : gfn_;   \
+})
+
+#define page_set_frame_gfn(_p, _gfn) ({                         \
+    gfn_t gfn_ = gfn_eq(_gfn, INVALID_GFN) ?                    \
+                 PGT_INVALID_FRAME_GFN : _gfn;                  \
+    (_p)->u.inuse.type_info &= ~PGT_gfn_mask;                   \
+    (_p)->u.inuse.type_info |= gfn_x(gfn_);                     \
+})

[snip]

Is it close to what you keep in mind?


The single use of type_info on Arm needs updating:

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0e07335..f306a93 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1376,14 +1376,18 @@ unsigned long domain_get_maximum_gpfn(struct 
domain *d)
  void share_xen_page_with_guest(struct page_info *page, struct domain *d,
                                 enum XENSHARE_flags flags)
  {
+    unsigned long type_info;
+
      if ( page_get_owner(page) == d )
          return;

      spin_lock(&d->page_alloc_lock);

      /* The incremented type count pins as writable or read-only. */
-    page->u.inuse.type_info =
-        (flags == SHARE_ro ? PGT_none : PGT_writable_page) | 1;
+    type_info = page->u.inuse.type_info & ~(PGT_type_mask | 
PGT_count_mask);
+    page->u.inuse.type_info = type_info |
+        (flags == SHARE_ro ? PGT_none : PGT_writable_page) |
+        (1UL << PGT_count_base);

      page_set_owner(page, d);
      smp_wmb(); /* install valid domain ptr before updating refcnt. */


>
>> @@ -163,6 +171,15 @@ extern unsigned long xenheap_base_pdx;
>>   
>>   #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
>>   
>> +#define page_get_frame_gfn(_p) ({                               \
>> +    gfn_t gfn_ = _gfn((_p)->u.inuse.frame_gfn);                 \
>> +    gfn_eq(gfn_, PGT_INVALID_FRAME_GFN) ? INVALID_GFN : gfn_;   \
>> +})
>> +
>> +#define page_set_frame_gfn(_p, _gfn) ((_p)->u.inuse.frame_gfn = gfn_x(_gfn))
>> +
>> +#define page_arch_init(_p)   page_set_frame_gfn(_p, INVALID_GFN)
> What's the purpose of the leading underscore in the macro parameter
> names? They're not in line with the C standard's designation of sub-
> namespaces for identifiers. (At least for the x86 counterpart please
> read this as a request to drop the underscore there.)

ok


Thank you.


-- 
Regards,

Oleksandr Tyshchenko

On 13.09.2021 19:09, Oleksandr wrote:
> On 10.09.21 10:52, Jan Beulich wrote:
>> On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
>>> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>>>        */
>>>       if ( p2m_is_foreign(pte.p2m.type) )
>>>       {
>>> -        mfn_t mfn = lpae_get_mfn(pte);
>>> -
>>>           ASSERT(mfn_valid(mfn));
>>>           put_page(mfn_to_page(mfn));
>>>       }
>>> +
>>> +#ifdef CONFIG_GRANT_TABLE
>>> +    /*
>>> +     * Check whether we deal with grant table page. As the grant table page
>>> +     * is xen_heap page and its entry has known p2m type, detect it and mark
>>> +     * the stored GFN as invalid.
>>> +     */
>>> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
>>> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
>>> +#endif
>> I take it the write done is benign for other Xen heap pages? I suppose
>> this would want making very explicit, as such an assumption is easy to
>> go stale by entirely unrelated changes.
> 
> Yes, I don't see what bad could happen if we would clear this field for 
> non grant table pages. Except grant table pages I could detect only one 
> page passed this check here which is, I assume, shared_info page. All 
> pages have this field initialized with INVALID_GFN. But *only* grant 
> table pages have this field in use. So, I think, no one will suffer. I 
> will add a comment. Or you meant something more than just a comment?

The answer here goes hand in hand with the pending question of whether
you wouldn't better generally introduce recording of the GFN (and hence
effectively an M2P): The less special casing here, the better imo. The
more special casing here, the more justification / explanation is imo
needed in the comment.

>>> --- a/xen/include/asm-arm/mm.h
>>> +++ b/xen/include/asm-arm/mm.h
>>> @@ -39,7 +39,15 @@ struct page_info
>>>           /* Page is in use: ((count_info & PGC_count_mask) != 0). */
>>>           struct {
>>>               /* Type reference count and various PGT_xxx flags and fields. */
>>> -            unsigned long type_info;
>>> +            unsigned long type_info:4;
>>> +
>>> +            /*
>>> +             * Stored GFN if page is used for grant table frame
>>> +             * (bits 59(27)-0).
>> Are these bit numbers correct? I thought Arm, like x86, counted bits
>> from low to high (unlike PPC, for example)?
> 
> I think, these are correct.  Yes, Little Endian is used.

Well, the low 4 bits are used by type_info here. Therefore I'm having
trouble seeing what the 0 refers to.

>>> @@ -94,12 +102,12 @@ struct page_info
>>>   #define PG_shift(idx)   (BITS_PER_LONG - (idx))
>>>   #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
>>>   
>>> -#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
>>> -#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
>>> -#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
>>> +#define PGT_none          (0UL << 3)  /* no special uses of this page   */
>>> +#define PGT_writable_page (1UL << 3)  /* has writable mappings?         */
>>> +#define PGT_type_mask     (1UL << 3)
>>>   
>>>    /* Count of uses of this frame as its current type. */
>>> -#define PGT_count_width   PG_shift(2)
>>> +#define PGT_count_width   1
>>>   #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
>> Leaving just a single bit seems pretty risky to me, and I can't see
>> why you do so. You need 52 bits on Arm64. Which means you have 12
>> bits left. Why not use them in full? Even on Arm32 you have 3 bits
>> available for the count afaict.
> 
> Only 1 bit in the type_info field is really used on Arm, but anyway ...
> 
> 
>>
>> Also there's a disconnect here: If anyone wanted to change the number
>> of bits used for the various fields, each such change should require
>> an adjustment in as few independent places as possible. That is, there
>> shouldn't be multiple uses of literal 4 further up, and there also
>> shouldn't be a hidden connection between that 4 and the PGT_* values
>> here. I think you'd better express the GFN as such a PGT_* construct
>> as well. And you better would keep using PG_mask() and PG_shift().
> 
> ... I think, this indeed makes sense. If got your request correctly, we 
> can avoid disconnect here
> and reserve 3-bit field for count for both Arm32 and Arm64. The struct 
> page_info's type_info field remains untouched.
> 
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index ded74d2..8b73e1c 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -99,8 +99,14 @@ struct page_info
>   #define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 
> 63.                 */
> 
>    /* Count of uses of this frame as its current type. */
> -#define PGT_count_width   PG_shift(2)
> -#define PGT_count_mask    ((1UL<<PGT_count_width)-1)
> +#define PGT_count_base    PG_shift(4)
> +#define PGT_count_mask    PG_mask(7, 4)
> +
> +/* Stored in bits [27:0] or [59:0] GFN if page is used for grant table 
> frame */
> +#define PGT_gfn_width     PG_shift(4)
> +#define PGT_gfn_mask      ((1UL<<PGT_gfn_width)-1)
> +
> +#define PGT_INVALID_FRAME_GFN   _gfn(PGT_gfn_mask)
> 
>    /* Cleared when the owning guest 'frees' this page. */
>   #define _PGC_allocated    PG_shift(1)
> @@ -163,6 +169,22 @@ extern unsigned long xenheap_base_pdx;
> 
>   #define maddr_get_owner(ma) (page_get_owner(maddr_to_page((ma))))
> 
> +#define page_get_frame_gfn(_p) ({                               \
> +    gfn_t gfn_ = _gfn((_p)->u.inuse.type_info & PGT_gfn_mask);  \
> +    gfn_eq(gfn_, PGT_INVALID_FRAME_GFN) ? INVALID_GFN : gfn_;   \
> +})
> +
> +#define page_set_frame_gfn(_p, _gfn) ({                         \
> +    gfn_t gfn_ = gfn_eq(_gfn, INVALID_GFN) ?                    \
> +                 PGT_INVALID_FRAME_GFN : _gfn;                  \
> +    (_p)->u.inuse.type_info &= ~PGT_gfn_mask;                   \
> +    (_p)->u.inuse.type_info |= gfn_x(gfn_);                     \
> +})
> 
> [snip]
> 
> Is it close to what you keep in mind?

Yes. Just to be sure - did you check that moving the count up
from starting at bit 0 is compatible with all present uses? At
least on x86 I think we have uses where it is assumed to occupy
the bottom so many bits (and the same also for PGC_count_mask).

Jan

On 14.09.21 11:31, Jan Beulich wrote:

Hi Jan

> On 13.09.2021 19:09, Oleksandr wrote:
>> On 10.09.21 10:52, Jan Beulich wrote:
>>> On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
>>>> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>>>>         */
>>>>        if ( p2m_is_foreign(pte.p2m.type) )
>>>>        {
>>>> -        mfn_t mfn = lpae_get_mfn(pte);
>>>> -
>>>>            ASSERT(mfn_valid(mfn));
>>>>            put_page(mfn_to_page(mfn));
>>>>        }
>>>> +
>>>> +#ifdef CONFIG_GRANT_TABLE
>>>> +    /*
>>>> +     * Check whether we deal with grant table page. As the grant table page
>>>> +     * is xen_heap page and its entry has known p2m type, detect it and mark
>>>> +     * the stored GFN as invalid.
>>>> +     */
>>>> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
>>>> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
>>>> +#endif
>>> I take it the write done is benign for other Xen heap pages? I suppose
>>> this would want making very explicit, as such an assumption is easy to
>>> go stale by entirely unrelated changes.
>> Yes, I don't see what bad could happen if we would clear this field for
>> non grant table pages. Except grant table pages I could detect only one
>> page passed this check here which is, I assume, shared_info page. All
>> pages have this field initialized with INVALID_GFN. But *only* grant
>> table pages have this field in use. So, I think, no one will suffer. I
>> will add a comment. Or you meant something more than just a comment?
> The answer here goes hand in hand with the pending question of whether
> you wouldn't better generally introduce recording of the GFN (and hence
> effectively an M2P): The less special casing here, the better imo. The
> more special casing here, the more justification / explanation is imo
> needed in the comment.

I got your point. I think, I am not the right person to judge whether 
the Xen on Arm needs / wants complete M2P support or not (this is a 
question to the maintainers and the community),
but I can't recollect a strong desire or critical need for implementing 
it every time such discussion arose and IIRC only arguments against it. 
Also this activity is not in my list of priorities to do on Xen on Arm. 
But, I agree that having M2P would prevent this (with grant table) and 
similar situations. So, I will add more justification / explanation for 
the current patch.


>
>>>> --- a/xen/include/asm-arm/mm.h
>>>> +++ b/xen/include/asm-arm/mm.h
>>>> @@ -39,7 +39,15 @@ struct page_info
>>>>            /* Page is in use: ((count_info & PGC_count_mask) != 0). */
>>>>            struct {
>>>>                /* Type reference count and various PGT_xxx flags and fields. */
>>>> -            unsigned long type_info;
>>>> +            unsigned long type_info:4;
>>>> +
>>>> +            /*
>>>> +             * Stored GFN if page is used for grant table frame
>>>> +             * (bits 59(27)-0).
>>> Are these bit numbers correct? I thought Arm, like x86, counted bits
>>> from low to high (unlike PPC, for example)?
>> I think, these are correct.  Yes, Little Endian is used.
> Well, the low 4 bits are used by type_info here. Therefore I'm having
> trouble seeing what the 0 refers to.

Now I see that comment is not precise. Probably I should have written 
that "60-bit(28-bit) field is used to store the GFN ..."


>
>>>> @@ -94,12 +102,12 @@ struct page_info
>>>>    #define PG_shift(idx)   (BITS_PER_LONG - (idx))
>>>>    #define PG_mask(x, idx) (x ## UL << PG_shift(idx))
>>>>    
>>>> -#define PGT_none          PG_mask(0, 1)  /* no special uses of this page   */
>>>> -#define PGT_writable_page PG_mask(1, 1)  /* has writable mappings?         */
>>>> -#define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or 63.                 */
>>>> +#define PGT_none          (0UL << 3)  /* no special uses of this page   */
>>>> +#define PGT_writable_page (1UL << 3)  /* has writable mappings?         */
>>>> +#define PGT_type_mask     (1UL << 3)
>>>>    
>>>>     /* Count of uses of this frame as its current type. */
>>>> -#define PGT_count_width   PG_shift(2)
>>>> +#define PGT_count_width   1
>>>>    #define PGT_count_mask    ((1UL<<PGT_count_width)-1)
>>> Leaving just a single bit seems pretty risky to me, and I can't see
>>> why you do so. You need 52 bits on Arm64. Which means you have 12
>>> bits left. Why not use them in full? Even on Arm32 you have 3 bits
>>> available for the count afaict.
>> Only 1 bit in the type_info field is really used on Arm, but anyway ...
>>
>>
>>> Also there's a disconnect here: If anyone wanted to change the number
>>> of bits used for the various fields, each such change should require
>>> an adjustment in as few independent places as possible. That is, there
>>> shouldn't be multiple uses of literal 4 further up, and there also
>>> shouldn't be a hidden connection between that 4 and the PGT_* values
>>> here. I think you'd better express the GFN as such a PGT_* construct
>>> as well. And you better would keep using PG_mask() and PG_shift().
>> ... I think, this indeed makes sense. If got your request correctly, we
>> can avoid disconnect here
>> and reserve 3-bit field for count for both Arm32 and Arm64. The struct
>> page_info's type_info field remains untouched.
>>
>>
>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>> index ded74d2..8b73e1c 100644
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -99,8 +99,14 @@ struct page_info
>>    #define PGT_type_mask     PG_mask(1, 1)  /* Bits 31 or
>> 63.                 */
>>
>>     /* Count of uses of this frame as its current type. */
>> -#define PGT_count_width   PG_shift(2)
>> -#define PGT_count_mask    ((1UL<<PGT_count_width)-1)
>> +#define PGT_count_base    PG_shift(4)
>> +#define PGT_count_mask    PG_mask(7, 4)
>> +
>> +/* Stored in bits [27:0] or [59:0] GFN if page is used for grant table
>> frame */
>> +#define PGT_gfn_width     PG_shift(4)
>> +#define PGT_gfn_mask      ((1UL<<PGT_gfn_width)-1)
>> +
>> +#define PGT_INVALID_FRAME_GFN   _gfn(PGT_gfn_mask)
>>
>>     /* Cleared when the owning guest 'frees' this page. */
>>    #define _PGC_allocated    PG_shift(1)
>> @@ -163,6 +169,22 @@ extern unsigned long xenheap_base_pdx;
>>
>>    #define maddr_get_owner(ma) (page_get_owner(maddr_to_page((ma))))
>>
>> +#define page_get_frame_gfn(_p) ({                               \
>> +    gfn_t gfn_ = _gfn((_p)->u.inuse.type_info & PGT_gfn_mask);  \
>> +    gfn_eq(gfn_, PGT_INVALID_FRAME_GFN) ? INVALID_GFN : gfn_;   \
>> +})
>> +
>> +#define page_set_frame_gfn(_p, _gfn) ({                         \
>> +    gfn_t gfn_ = gfn_eq(_gfn, INVALID_GFN) ?                    \
>> +                 PGT_INVALID_FRAME_GFN : _gfn;                  \
>> +    (_p)->u.inuse.type_info &= ~PGT_gfn_mask;                   \
>> +    (_p)->u.inuse.type_info |= gfn_x(gfn_);                     \
>> +})
>>
>> [snip]
>>
>> Is it close to what you keep in mind?
> Yes. Just to be sure - did you check that moving the count up
> from starting at bit 0 is compatible with all present uses? At
> least on x86 I think we have uses where it is assumed to occupy
> the bottom so many bits (and the same also for PGC_count_mask).

ok, thank you. Yes, I checked that (on Arm). I could find only one place 
which would need updating, which was the share_xen_page_with_guest().


>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko