xen/arch/arm/include/asm/grant_table.h | 58 ++++++++++------------------------ xen/arch/arm/include/asm/mm.h | 38 ++++++++++++++++++++-- xen/arch/arm/mm.c | 24 ++++++++++++-- xen/arch/arm/p2m.c | 7 ++-- xen/arch/x86/include/asm/grant_table.h | 5 --- xen/arch/x86/include/asm/mm.h | 3 ++ xen/common/grant_table.c | 9 ------ xen/common/page_alloc.c | 7 ++++ 8 files changed, 88 insertions(+), 63 deletions(-)
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. This patch is based on
the assumption that grant table page is the xenheap page.
To cover 64-bit/40-bit IPA on Arm64/Arm32 we need the space
to hold 52-bit/28-bit + extra bit value respectively. In order
to not grow the size of struct page_info borrow the required
amount of bits from type_info's count portion which current
context won't suffer (currently only 1 bit is used on Arm).
Please note, to minimize code changes and avoid introducing
an extra #ifdef-s to the header, we keep the same amount of
bits on both subarches, although the count portion on Arm64
could be wider, so we waste some bits here.
Introduce corresponding PGT_* constructs and access macros.
Update existing gnttab macros to deal with GFN value according
to new location. Also update the use of count portion on Arm
in share_xen_page_with_guest().
While at it, extend this simplified M2P-like approach for any
xenheap pages which are proccessed in xenmem_add_to_physmap_one()
except foreign ones. Update the code to set GFN portion after
establishing new mapping for the xenheap page in said function
and to clean GFN portion when putting a reference on that page
in p2m_put_l3_page().
And for everything to work correctly introduce arch-specific
initialization pattern PGT_TYPE_INFO_INIT_PATTERN to be ORed
with type_info field in alloc_xenheap_pages(). The pattern's
purpose on Arm is to clear the GFN portion before use, on x86
it is just a stub.
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 globally.
Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Dear @RISC-V maintainers, please note in current patch I drop arch
specific helpers gnttab_init(destroy)_arch helpers as unneeded for
both Arm and x86. Please let me know if you are going to reuse them
in the nearest future and I will retain them.
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/
https://lore.kernel.org/xen-devel/1631652245-30746-1-git-send-email-olekstysh@gmail.com/
https://lore.kernel.org/xen-devel/1632425551-18910-1-git-send-email-olekstysh@gmail.com/
https://lore.kernel.org/xen-devel/1638563610-4419-1-git-send-email-olekstysh@gmail.com/
Changes RFC1 -> RFC2:
- update patch description
- add/update comments in code
- clarify check in p2m_put_l3_page()
- introduce arch_alloc_xenheap_page() and arch_free_xenheap_page()
and drop page_arch_init()
- add ASSERT to gnttab_shared_page() and gnttab_status_page()
- rework changes to Arm's struct page_info: do not split type_info,
allocate GFN portion by reducing count portion, create corresponding
PGT_* construct, etc
- update page_get_frame_gfn() and page_set_frame_gfn()
- update the use of count portion on Arm
- drop the leading underscore in the macro parameter names
Changes RFC2 -> RFC3:
- update patch description
- drop PGT_count_base and MASK_INSR() in share_xen_page_with_guest()
- update alloc_xenheap_page() and free_xenheap_page() for SEPARATE_XENHEAP
case (Arm32)
- provide an extra bit for GFN portion, to get PGT_INVALID_FRAME_GFN
one bit more than the maximum number of physical address bits on Arm32
Changes RFC3 -> V4:
- rebase on Jan's "gnttab: remove guest_physmap_remove_page() call
from gnttab_map_frame()"
- finally resolve locking question by recent Julien's suggestion,
so drop the RFC tag
- update comments in Arm's mm.h/p2m.c to not mention grant table
- convert page_set(get)_frame_gfn to static inline func and
rename them to page_set(get)_xenheap_gfn()
- rename PGT_INVALID_FRAME_GFN to PGT_INVALID_XENHEAP_GFN
- add ASSERT(is_xen_heap_page(...)) in page_set(get)_frame_gfn
- remove BUG_ON() in arch_free_xenheap_page
- remove local type_info in share_xen_page_with_guest()
- remove an extra argument p2m in p2m_put_l3_page()
- remove #ifdef CONFIG_GRANT_TABLE in p2m_put_l3_page()
- also cover real-only pages by using p2m_is_ram instead of a check
against p2m_ram_rw in p2m_put_l3_page() and use "else if" construct
- call arch_free_xenheap_page() before clearing the PGC_xen_heap in
free_xenheap_pages()
- remove ASSERT() in gnttab_shared(status)_page and use simpler
virt_to_page
- remove local pg_ in gnttab_shared(status)_gfn
- update patch description to reflect recent changes
Changes V4 -> V5:
- rebase on latest staging
- update patch description
- drop arch_alloc(free)_xenheap_page macro and use arch-specific
initialization pattern to clear GFN portion before use
- add const to struct page_info *p in page_get_xenheap_gfn
- fix a breakage on Arm32
---
xen/arch/arm/include/asm/grant_table.h | 58 ++++++++++------------------------
xen/arch/arm/include/asm/mm.h | 38 ++++++++++++++++++++--
xen/arch/arm/mm.c | 24 ++++++++++++--
xen/arch/arm/p2m.c | 7 ++--
xen/arch/x86/include/asm/grant_table.h | 5 ---
xen/arch/x86/include/asm/mm.h | 3 ++
xen/common/grant_table.c | 9 ------
xen/common/page_alloc.c | 7 ++++
8 files changed, 88 insertions(+), 63 deletions(-)
diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
index d31a4d6..d6fda31 100644
--- a/xen/arch/arm/include/asm/grant_table.h
+++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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, mfn) \
({ \
- int rc_ = 0; \
gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \
- if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \
- (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn, \
- 0)) == 0 ) \
- ((st) ? (gt)->arch.status_gfn \
- : (gt)->arch.shared_gfn)[idx] = (gfn); \
- rc_; \
+ (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn, gfn)) \
+ ? guest_physmap_remove_page((gt)->domain, ogfn, mfn, 0) \
+ : 0; \
})
#define gnttab_get_frame_gfn(gt, st, idx) ({ \
@@ -88,11 +54,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) ({ \
+ virt_to_page((t)->shared_raw[i]); \
+})
+
+#define gnttab_status_page(t, i) ({ \
+ virt_to_page((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) ({ \
+ page_get_xenheap_gfn(gnttab_shared_page(t, i)); \
+})
+
+#define gnttab_status_gfn(d, t, i) ({ \
+ page_get_xenheap_gfn(gnttab_status_page(t, i)); \
+})
#define gnttab_need_iommu_mapping(d) \
(is_domain_direct_mapped(d) && is_iommu_enabled(d))
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 424aaf2..b99044c 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -98,9 +98,22 @@ struct page_info
#define PGT_writable_page PG_mask(1, 1) /* has writable mappings? */
#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)
+ /* 2-bit count of uses of this frame as its current type. */
+#define PGT_count_mask PG_mask(3, 3)
+
+/*
+ * Stored in bits [28:0] or [60:0] GFN if page is xenheap page.
+ */
+#define PGT_gfn_width PG_shift(3)
+#define PGT_gfn_mask ((1UL<<PGT_gfn_width)-1)
+
+#define PGT_INVALID_XENHEAP_GFN _gfn(PGT_gfn_mask)
+
+/*
+ * An arch-specific initialization pattern is needed for the type_info field
+ * as it's GFN portion can contain the valid GFN if page is xenheap page.
+ */
+#define PGT_TYPE_INFO_INIT_PATTERN gfn_x(PGT_INVALID_XENHEAP_GFN)
/* Cleared when the owning guest 'frees' this page. */
#define _PGC_allocated PG_shift(1)
@@ -358,6 +371,25 @@ void clear_and_clean_page(struct page_info *page);
unsigned int arch_get_dma_bitsize(void);
+static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
+{
+ gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
+
+ ASSERT(is_xen_heap_page(p));
+
+ return gfn_eq(gfn_, PGT_INVALID_XENHEAP_GFN) ? INVALID_GFN : gfn_;
+}
+
+static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
+{
+ gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN : gfn;
+
+ ASSERT(is_xen_heap_page(p));
+
+ p->u.inuse.type_info &= ~PGT_gfn_mask;
+ p->u.inuse.type_info |= gfn_x(gfn_);
+}
+
#endif /* __ARCH_ARM_MM__ */
/*
* Local variables:
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index eea926d..4f4cab3 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1382,8 +1382,10 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
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;
+ page->u.inuse.type_info &= ~(PGT_type_mask | PGT_count_mask);
+ page->u.inuse.type_info |= (flags == SHARE_ro ? PGT_none
+ : PGT_writable_page) |
+ MASK_INSR(1, PGT_count_mask);
page_set_owner(page, d);
smp_wmb(); /* install valid domain ptr before updating refcnt. */
@@ -1487,7 +1489,23 @@ int xenmem_add_to_physmap_one(
}
/* Map at new location. */
- rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
+ if ( !p2m_is_ram(t) || !is_xen_heap_mfn(mfn) )
+ rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
+ else
+ {
+ struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+ p2m_write_lock(p2m);
+ if ( gfn_eq(page_get_xenheap_gfn(mfn_to_page(mfn)), INVALID_GFN) )
+ {
+ rc = p2m_set_entry(p2m, gfn, 1, mfn, t, p2m->default_access);
+ if ( !rc )
+ page_set_xenheap_gfn(mfn_to_page(mfn), gfn);
+ }
+ else
+ rc = -EBUSY;
+ p2m_write_unlock(p2m);
+ }
/*
* For XENMAPSPACE_gmfn_foreign if we failed to add the mapping, we need
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8b20b43..fd8aff9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -720,6 +720,8 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
*/
static void p2m_put_l3_page(const lpae_t pte)
{
+ mfn_t mfn = lpae_get_mfn(pte);
+
ASSERT(p2m_is_valid(pte));
/*
@@ -731,11 +733,12 @@ 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));
}
+ /* Detect the xenheap page and mark the stored GFN as invalid. */
+ else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
+ page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);
}
/* Free lpae sub-tree behind an entry */
diff --git a/xen/arch/x86/include/asm/grant_table.h b/xen/arch/x86/include/asm/grant_table.h
index a8a2143..5c23cec 100644
--- a/xen/arch/x86/include/asm/grant_table.h
+++ b/xen/arch/x86/include/asm/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, mfn) \
(gfn_eq(gfn, INVALID_GFN) \
? guest_physmap_remove_page((gt)->domain, \
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index 5dbcee8..b6b0dce 100644
--- 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
+
/* Are the 'type mask' bits identical? */
#define PGT_type_equal(x, y) (!(((x) ^ (y)) & PGT_type_mask))
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 0262f2c..01d7a29 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -94,8 +94,6 @@ struct grant_table {
/* Domain to which this struct grant_table belongs. */
struct domain *domain;
-
- struct grant_table_arch arch;
};
unsigned int __read_mostly opt_max_grant_frames = 64;
@@ -1997,14 +1995,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:
@@ -3911,8 +3904,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 38eea87..ec7d803 100644
--- 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);
}
--
2.7.4
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.
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().
Further, did you check that when the value is 0 the compiler would
fully eliminate the new code in both flavors of the function?
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 ...
Jan
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
On 06.01.2022 17:30, Oleksandr wrote: > In case you don't have any other objections shall I re-push v5.1 with > proposed adjustments now? I'd suggest you wait for feedback by others. After all there may also be opposition to what I have said (in which case you'd go back and forth). Jan
On 06.01.22 18:50, Jan Beulich wrote: Hi Jan > On 06.01.2022 17:30, Oleksandr wrote: >> In case you don't have any other objections shall I re-push v5.1 with >> proposed adjustments now? > I'd suggest you wait for feedback by others. After all there may also > be opposition to what I have said (in which case you'd go back and > forth). You are right. > > Jan > -- Regards, Oleksandr Tyshchenko
Hi, Sorry for the late reply. On 06/01/2022 16:30, Oleksandr wrote: So I agree with Jan that the name should be adjusted if it stays where it is. That said, I would actually prefer the adjustment in alloc_heap_pages(). It is one less assignment per page and I don't expect any issue with setting the bits to INVALID_GFN everywhere in the future on Arm. Note that you would also need to update acquire_staticmem_pages(). Cheers, -- Julien Grall
On 07.02.22 19:41, Julien Grall wrote: > Hi, Hi Julien > > Sorry for the late reply. np > > On 06/01/2022 16:30, Oleksandr wrote: > > So I agree with Jan that the name should be adjusted if it stays where > it is. > > That said, I would actually prefer the adjustment in > alloc_heap_pages(). It is one less assignment per page and I don't > expect any issue with setting the bits to INVALID_GFN everywhere in > the future on Arm. Sorry I lost the context. To clarify, are you speaking about what I proposed at [1]? If yes, then ... > > Note that you would also need to update acquire_staticmem_pages(). ... yes, will do. [1] https://lore.kernel.org/xen-devel/b4832284-9bfc-d600-14b1-1784f53e5d9f@gmail.com/ > > > Cheers, > -- Regards, Oleksandr Tyshchenko
On 07/02/2022 17:58, Oleksandr Tyshchenko wrote: > > On 07.02.22 19:41, Julien Grall wrote: >> On 06/01/2022 16:30, Oleksandr wrote: >> >> So I agree with Jan that the name should be adjusted if it stays where >> it is. >> >> That said, I would actually prefer the adjustment in >> alloc_heap_pages(). It is one less assignment per page and I don't >> expect any issue with setting the bits to INVALID_GFN everywhere in >> the future on Arm. > > > Sorry I lost the context. To clarify, are you speaking about what I > proposed at [1]? That's correct. Cheers, -- Julien Grall
On 07.02.22 19:59, Julien Grall wrote: Hi Julien > > > On 07/02/2022 17:58, Oleksandr Tyshchenko wrote: >> >> On 07.02.22 19:41, Julien Grall wrote: >>> On 06/01/2022 16:30, Oleksandr wrote: >>> >>> So I agree with Jan that the name should be adjusted if it stays where >>> it is. >>> >>> That said, I would actually prefer the adjustment in >>> alloc_heap_pages(). It is one less assignment per page and I don't >>> expect any issue with setting the bits to INVALID_GFN everywhere in >>> the future on Arm. >> >> >> Sorry I lost the context. To clarify, are you speaking about what I >> proposed at [1]? > > That's correct. Thank you for the clarification. > > Cheers, > -- Regards, Oleksandr Tyshchenko
Hi Oleksandr,
On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
> 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. This patch is based on
> the assumption that grant table page is the xenheap page.
I would write "grant table pages are xenheap pages" or "a grant table
page is a xenheap page".
[...]
> diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
> index d31a4d6..d6fda31 100644
> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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, mfn) \
> ({ \
> - int rc_ = 0; \
> gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \
> - if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \
> - (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn, \
> - 0)) == 0 ) \
> - ((st) ? (gt)->arch.status_gfn \
> - : (gt)->arch.shared_gfn)[idx] = (gfn); \
> - rc_; \
> + (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn, gfn)) \
> + ? guest_physmap_remove_page((gt)->domain, ogfn, mfn, 0) \
> + : 0; \
Given that we are implementing something similar to an M2P, I was
expecting the implementation to be pretty much the same as the x86 helper.
Would you be able to outline why it is different?
> })
>
> #define gnttab_get_frame_gfn(gt, st, idx) ({ \
> @@ -88,11 +54,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) ({ \
> + virt_to_page((t)->shared_raw[i]); \
> +})
This can be simplified to:
#define gnttab_shared_page(t, i) virt_to_page((t)->shared_raw[i])
> +
> +#define gnttab_status_page(t, i) ({ \
> + virt_to_page((t)->status[i]); \
> +})
Same here.
>
> -#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) ({ \
> + page_get_xenheap_gfn(gnttab_shared_page(t, i)); \
> +})
Same here
> +
> +#define gnttab_status_gfn(d, t, i) ({ \
> + page_get_xenheap_gfn(gnttab_status_page(t, i)); \
> +})
Same here.
>
> #define gnttab_need_iommu_mapping(d) \
> (is_domain_direct_mapped(d) && is_iommu_enabled(d))
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 424aaf2..b99044c 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -98,9 +98,22 @@ struct page_info
> #define PGT_writable_page PG_mask(1, 1) /* has writable mappings? */
> #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)
> + /* 2-bit count of uses of this frame as its current type. */
> +#define PGT_count_mask PG_mask(3, 3)
> +
> +/*
> + * Stored in bits [28:0] or [60:0] GFN if page is xenheap page.
This comment would be easier to understand if you add resp. (arm32) and
(arm64) after each range.
> + */
> +#define PGT_gfn_width PG_shift(3)
> +#define PGT_gfn_mask ((1UL<<PGT_gfn_width)-1)
> +
> +#define PGT_INVALID_XENHEAP_GFN _gfn(PGT_gfn_mask)
> +
> +/*
> + * An arch-specific initialization pattern is needed for the type_info field
> + * as it's GFN portion can contain the valid GFN if page is xenheap page.
> + */
> +#define PGT_TYPE_INFO_INIT_PATTERN gfn_x(PGT_INVALID_XENHEAP_GFN)
>
> /* Cleared when the owning guest 'frees' this page. */
> #define _PGC_allocated PG_shift(1)
> @@ -358,6 +371,25 @@ void clear_and_clean_page(struct page_info *page);
>
> unsigned int arch_get_dma_bitsize(void);
>
> +static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
> +{
> + gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
> +
> + ASSERT(is_xen_heap_page(p));
> +
> + return gfn_eq(gfn_, PGT_INVALID_XENHEAP_GFN) ? INVALID_GFN : gfn_;
> +}
> +
> +static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
> +{
> + gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN : gfn;
> +
> + ASSERT(is_xen_heap_page(p));
> +
> + p->u.inuse.type_info &= ~PGT_gfn_mask;
> + p->u.inuse.type_info |= gfn_x(gfn_);
> +}
This is not going to be atomic. So can you outline which locking
mechanism should be used to protect access (set/get) to the GFN?
I will do another review of the patch once I know what we locking should
protect the accesses.
Cheers,
--
Julien Grall
On 07.02.22 19:15, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien
>
>
> On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
>> 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. This patch is based on
>> the assumption that grant table page is the xenheap page.
>
> I would write "grant table pages are xenheap pages" or "a grant table
> page is a xenheap page".
ok, will do
>
> [...]
>
>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>> b/xen/arch/arm/include/asm/grant_table.h
>> index d31a4d6..d6fda31 100644
>> --- a/xen/arch/arm/include/asm/grant_table.h
>> +++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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,
>> mfn) \
>> ({ \
>> - int rc_ =
>> 0; \
>> gfn_t ogfn = gnttab_get_frame_gfn(gt, st,
>> idx); \
>> - if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn)
>> || \
>> - (rc_ = guest_physmap_remove_page((gt)->domain, ogfn,
>> mfn, \
>> - 0)) == 0
>> ) \
>> - ((st) ?
>> (gt)->arch.status_gfn \
>> - : (gt)->arch.shared_gfn)[idx] =
>> (gfn); \
>> - rc_; \
>> + (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn,
>> gfn)) \
>> + ? guest_physmap_remove_page((gt)->domain, ogfn, mfn,
>> 0) \
>> + :
>> 0; \
>
> Given that we are implementing something similar to an M2P, I was
> expecting the implementation to be pretty much the same as the x86
> helper.
>
> Would you be able to outline why it is different?
Being honest, I didn't think about it so far. But, I agree with the
question.
It feels to me that Arm variant can now behave as x86 one (as
xenmem_add_to_physmap_one() now checks for the prior mapping), I mean to
use INVALID_GFN as an indication to remove a page.
What do you think?
>
>
>> })
>> #define gnttab_get_frame_gfn(gt, st, idx)
>> ({ \
>> @@ -88,11 +54,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)
>> ({ \
>> + virt_to_page((t)->shared_raw[i]); \
>> +})
>
> This can be simplified to:
>
> #define gnttab_shared_page(t, i) virt_to_page((t)->shared_raw[i])
agree, will do
>
>> +
>> +#define gnttab_status_page(t, i)
>> ({ \
>> + virt_to_page((t)->status[i]); \
>> +})
>
> Same here.
ok
>
>> -#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)
>> ({ \
>> + page_get_xenheap_gfn(gnttab_shared_page(t,
>> i)); \
>> +})
>
> Same here
ok
>
>> +
>> +#define gnttab_status_gfn(d, t, i)
>> ({ \
>> + page_get_xenheap_gfn(gnttab_status_page(t,
>> i)); \
>> +})
>
> Same here.
ok
>
>> #define gnttab_need_iommu_mapping(d) \
>> (is_domain_direct_mapped(d) && is_iommu_enabled(d))
>> diff --git a/xen/arch/arm/include/asm/mm.h
>> b/xen/arch/arm/include/asm/mm.h
>> index 424aaf2..b99044c 100644
>> --- a/xen/arch/arm/include/asm/mm.h
>> +++ b/xen/arch/arm/include/asm/mm.h
>> @@ -98,9 +98,22 @@ struct page_info
>> #define PGT_writable_page PG_mask(1, 1) /* has writable
>> mappings? */
>> #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)
>> + /* 2-bit count of uses of this frame as its current type. */
>> +#define PGT_count_mask PG_mask(3, 3)
>> +
>> +/*
>> + * Stored in bits [28:0] or [60:0] GFN if page is xenheap page.
>
> This comment would be easier to understand if you add resp. (arm32)
> and (arm64) after each range.
agree, will do
>
>
>> + */
>> +#define PGT_gfn_width PG_shift(3)
>> +#define PGT_gfn_mask ((1UL<<PGT_gfn_width)-1)
>> +
>> +#define PGT_INVALID_XENHEAP_GFN _gfn(PGT_gfn_mask)
>> +
>> +/*
>> + * An arch-specific initialization pattern is needed for the
>> type_info field
>> + * as it's GFN portion can contain the valid GFN if page is xenheap
>> page.
>> + */
>> +#define PGT_TYPE_INFO_INIT_PATTERN gfn_x(PGT_INVALID_XENHEAP_GFN)
>> /* Cleared when the owning guest 'frees' this page. */
>> #define _PGC_allocated PG_shift(1)
>> @@ -358,6 +371,25 @@ void clear_and_clean_page(struct page_info *page);
>> unsigned int arch_get_dma_bitsize(void);
>> +static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
>> +{
>> + gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
>> +
>> + ASSERT(is_xen_heap_page(p));
>> +
>> + return gfn_eq(gfn_, PGT_INVALID_XENHEAP_GFN) ? INVALID_GFN : gfn_;
>> +}
>> +
>> +static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
>> +{
>> + gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN
>> : gfn;
>> +
>> + ASSERT(is_xen_heap_page(p));
>> +
>> + p->u.inuse.type_info &= ~PGT_gfn_mask;
>> + p->u.inuse.type_info |= gfn_x(gfn_);
>> +}
>
> This is not going to be atomic. So can you outline which locking
> mechanism should be used to protect access (set/get) to the GFN?
I think, the P2M lock.
>
>
> I will do another review of the patch once I know what we locking
> should protect the accesses.
>
> Cheers,
>
--
Regards,
Oleksandr Tyshchenko
Hi,
On 07/02/2022 19:56, Oleksandr Tyshchenko wrote:
>
> On 07.02.22 19:15, Julien Grall wrote:
>> Hi Oleksandr,
>> On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
>>> 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. This patch is based on
>>> the assumption that grant table page is the xenheap page.
>>
>> I would write "grant table pages are xenheap pages" or "a grant table
>> page is a xenheap page".
>>
>> [...]
>>
>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>> b/xen/arch/arm/include/asm/grant_table.h
>>> index d31a4d6..d6fda31 100644
>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>> +++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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,
>>> mfn) \
>>> ({ \
>>> - int rc_ =
>>> 0; \
>>> gfn_t ogfn = gnttab_get_frame_gfn(gt, st,
>>> idx); \
>>> - if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn)
>>> || \
>>> - (rc_ = guest_physmap_remove_page((gt)->domain, ogfn,
>>> mfn, \
>>> - 0)) == 0
>>> ) \
>>> - ((st) ?
>>> (gt)->arch.status_gfn \
>>> - : (gt)->arch.shared_gfn)[idx] =
>>> (gfn); \
>>> - rc_; \
>>> + (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn,
>>> gfn)) \
>>> + ? guest_physmap_remove_page((gt)->domain, ogfn, mfn,
>>> 0) \
>>> + :
>>> 0; \
>>
>> Given that we are implementing something similar to an M2P, I was
>> expecting the implementation to be pretty much the same as the x86
>> helper.
>>
>> Would you be able to outline why it is different?
>
> Being honest, I didn't think about it so far. But, I agree with the
> question.
>
> It feels to me that Arm variant can now behave as x86 one (as
> xenmem_add_to_physmap_one() now checks for the prior mapping), I mean to
> use INVALID_GFN as an indication to remove a page.
>
> What do you think?
I will defer that to Jan.
Jan, IIRC you were the one introducing the call to
guest_physmap_remove_page(). Do you remember why the difference between
x86 and Arm were necessary?
[...]
>>
>>
>>> + */
>>> +#define PGT_gfn_width PG_shift(3)
>>> +#define PGT_gfn_mask ((1UL<<PGT_gfn_width)-1)
>>> +
>>> +#define PGT_INVALID_XENHEAP_GFN _gfn(PGT_gfn_mask)
>>> +
>>> +/*
>>> + * An arch-specific initialization pattern is needed for the
>>> type_info field
>>> + * as it's GFN portion can contain the valid GFN if page is xenheap
>>> page.
>>> + */
>>> +#define PGT_TYPE_INFO_INIT_PATTERN gfn_x(PGT_INVALID_XENHEAP_GFN)
>>> /* Cleared when the owning guest 'frees' this page. */
>>> #define _PGC_allocated PG_shift(1)
>>> @@ -358,6 +371,25 @@ void clear_and_clean_page(struct page_info *page);
>>> unsigned int arch_get_dma_bitsize(void);
>>> +static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
>>> +{
>>> + gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
>>> +
>>> + ASSERT(is_xen_heap_page(p));
>>> +
>>> + return gfn_eq(gfn_, PGT_INVALID_XENHEAP_GFN) ? INVALID_GFN : gfn_;
>>> +}
>>> +
>>> +static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
>>> +{
>>> + gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN
>>> : gfn;
>>> +
>>> + ASSERT(is_xen_heap_page(p));
>>> +
>>> + p->u.inuse.type_info &= ~PGT_gfn_mask;
>>> + p->u.inuse.type_info |= gfn_x(gfn_);
>>> +}
>>
>> This is not going to be atomic. So can you outline which locking
>> mechanism should be used to protect access (set/get) to the GFN?
>
>
> I think, the P2M lock.
Ok. So, looking at the code, most of calls to page_get_xenheap_gfn() are
not protected with the p2m_lock().
(Jan please confirm) If I am not mistaken, on x86, a read to the M2P is
not always protected. But they have code within the P2M lock to check
any difference (see p2m_remove_page()). I think we would need the same,
so we don't end up to introduce a behavior similar to what XSA-387 has
fixed on x86.
In addition to that, if p2m_get_xenheap_gfn() is going to be called
locklessly. Then we need to make sure the update to type_info are
atomic. This means:
- p2m_get_xenheap_gfn() should use READ_ONCE().
- p2m_set_xenheap_gfn() should use WRITE_ONCE(). We might even need to
use cmpxchg() if there are other update to type_info that are not
protected. I will let you have a look.
Cheers,
--
Julien Grall
On 08.02.2022 12:58, Julien Grall wrote:
> On 07/02/2022 19:56, Oleksandr Tyshchenko wrote:
>> On 07.02.22 19:15, Julien Grall wrote:
>>> Hi Oleksandr,
>>> On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
>>>> 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. This patch is based on
>>>> the assumption that grant table page is the xenheap page.
>>>
>>> I would write "grant table pages are xenheap pages" or "a grant table
>>> page is a xenheap page".
>>>
>>> [...]
>>>
>>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>>> b/xen/arch/arm/include/asm/grant_table.h
>>>> index d31a4d6..d6fda31 100644
>>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>>> +++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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,
>>>> mfn) \
>>>> ({ \
>>>> - int rc_ =
>>>> 0; \
>>>> gfn_t ogfn = gnttab_get_frame_gfn(gt, st,
>>>> idx); \
>>>> - if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn)
>>>> || \
>>>> - (rc_ = guest_physmap_remove_page((gt)->domain, ogfn,
>>>> mfn, \
>>>> - 0)) == 0
>>>> ) \
>>>> - ((st) ?
>>>> (gt)->arch.status_gfn \
>>>> - : (gt)->arch.shared_gfn)[idx] =
>>>> (gfn); \
>>>> - rc_; \
>>>> + (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn,
>>>> gfn)) \
>>>> + ? guest_physmap_remove_page((gt)->domain, ogfn, mfn,
>>>> 0) \
>>>> + :
>>>> 0; \
>>>
>>> Given that we are implementing something similar to an M2P, I was
>>> expecting the implementation to be pretty much the same as the x86
>>> helper.
>>>
>>> Would you be able to outline why it is different?
>>
>> Being honest, I didn't think about it so far. But, I agree with the
>> question.
>>
>> It feels to me that Arm variant can now behave as x86 one (as
>> xenmem_add_to_physmap_one() now checks for the prior mapping), I mean to
>> use INVALID_GFN as an indication to remove a page.
>>
>> What do you think?
>
> I will defer that to Jan.
>
> Jan, IIRC you were the one introducing the call to
> guest_physmap_remove_page(). Do you remember why the difference between
> x86 and Arm were necessary?
The code was different before, and Arm's behavior was also different.
Hence the two functions couldn't be quite similar. If Arm behavior is
now converging with x86'es, the functions becoming more similar is
not entirely unexpected.
>>>> @@ -358,6 +371,25 @@ void clear_and_clean_page(struct page_info *page);
>>>> unsigned int arch_get_dma_bitsize(void);
>>>> +static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
>>>> +{
>>>> + gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
>>>> +
>>>> + ASSERT(is_xen_heap_page(p));
>>>> +
>>>> + return gfn_eq(gfn_, PGT_INVALID_XENHEAP_GFN) ? INVALID_GFN : gfn_;
>>>> +}
>>>> +
>>>> +static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
>>>> +{
>>>> + gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN
>>>> : gfn;
>>>> +
>>>> + ASSERT(is_xen_heap_page(p));
>>>> +
>>>> + p->u.inuse.type_info &= ~PGT_gfn_mask;
>>>> + p->u.inuse.type_info |= gfn_x(gfn_);
>>>> +}
>>>
>>> This is not going to be atomic. So can you outline which locking
>>> mechanism should be used to protect access (set/get) to the GFN?
>>
>>
>> I think, the P2M lock.
>
> Ok. So, looking at the code, most of calls to page_get_xenheap_gfn() are
> not protected with the p2m_lock().
>
> (Jan please confirm) If I am not mistaken, on x86, a read to the M2P is
> not always protected. But they have code within the P2M lock to check
> any difference (see p2m_remove_page()). I think we would need the same,
> so we don't end up to introduce a behavior similar to what XSA-387 has
> fixed on x86.
Yes, this matches my understanding.
Jan
On 08.02.22 14:47, Jan Beulich wrote:
Hi Julien, Jan
> On 08.02.2022 12:58, Julien Grall wrote:
>> On 07/02/2022 19:56, Oleksandr Tyshchenko wrote:
>>> On 07.02.22 19:15, Julien Grall wrote:
>>>> Hi Oleksandr,
>>>> On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
>>>>> 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. This patch is based on
>>>>> the assumption that grant table page is the xenheap page.
>>>> I would write "grant table pages are xenheap pages" or "a grant table
>>>> page is a xenheap page".
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>>>> b/xen/arch/arm/include/asm/grant_table.h
>>>>> index d31a4d6..d6fda31 100644
>>>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>>>> +++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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,
>>>>> mfn) \
>>>>> ({ \
>>>>> - int rc_ =
>>>>> 0; \
>>>>> gfn_t ogfn = gnttab_get_frame_gfn(gt, st,
>>>>> idx); \
>>>>> - if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn)
>>>>> || \
>>>>> - (rc_ = guest_physmap_remove_page((gt)->domain, ogfn,
>>>>> mfn, \
>>>>> - 0)) == 0
>>>>> ) \
>>>>> - ((st) ?
>>>>> (gt)->arch.status_gfn \
>>>>> - : (gt)->arch.shared_gfn)[idx] =
>>>>> (gfn); \
>>>>> - rc_; \
>>>>> + (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn,
>>>>> gfn)) \
>>>>> + ? guest_physmap_remove_page((gt)->domain, ogfn, mfn,
>>>>> 0) \
>>>>> + :
>>>>> 0; \
>>>> Given that we are implementing something similar to an M2P, I was
>>>> expecting the implementation to be pretty much the same as the x86
>>>> helper.
>>>>
>>>> Would you be able to outline why it is different?
>>> Being honest, I didn't think about it so far. But, I agree with the
>>> question.
>>>
>>> It feels to me that Arm variant can now behave as x86 one (as
>>> xenmem_add_to_physmap_one() now checks for the prior mapping), I mean to
>>> use INVALID_GFN as an indication to remove a page.
>>>
>>> What do you think?
>> I will defer that to Jan.
>>
>> Jan, IIRC you were the one introducing the call to
>> guest_physmap_remove_page(). Do you remember why the difference between
>> x86 and Arm were necessary?
> The code was different before, and Arm's behavior was also different.
> Hence the two functions couldn't be quite similar. If Arm behavior is
> now converging with x86'es, the functions becoming more similar is
> not entirely unexpected.
If Arm's gnttab_set_frame_gfn appears to be the similar to x86's one,
what would be the next step?
I thought of the following options:
1. Leave per-arch helpers as they are
2. Move helper to the common grant_table.h
3. Remove the helpers, call guest_physmap_remove_page() directly from
the common grant_table.c
>
>>>>> @@ -358,6 +371,25 @@ void clear_and_clean_page(struct page_info *page);
>>>>> unsigned int arch_get_dma_bitsize(void);
>>>>> +static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
>>>>> +{
>>>>> + gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
>>>>> +
>>>>> + ASSERT(is_xen_heap_page(p));
>>>>> +
>>>>> + return gfn_eq(gfn_, PGT_INVALID_XENHEAP_GFN) ? INVALID_GFN : gfn_;
>>>>> +}
>>>>> +
>>>>> +static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
>>>>> +{
>>>>> + gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN
>>>>> : gfn;
>>>>> +
>>>>> + ASSERT(is_xen_heap_page(p));
>>>>> +
>>>>> + p->u.inuse.type_info &= ~PGT_gfn_mask;
>>>>> + p->u.inuse.type_info |= gfn_x(gfn_);
>>>>> +}
>>>> This is not going to be atomic. So can you outline which locking
>>>> mechanism should be used to protect access (set/get) to the GFN?
>>>
>>> I think, the P2M lock.
>> Ok. So, looking at the code, most of calls to page_get_xenheap_gfn() are
>> not protected with the p2m_lock().
>>
>> (Jan please confirm) If I am not mistaken, on x86, a read to the M2P is
>> not always protected. But they have code within the P2M lock to check
>> any difference (see p2m_remove_page()). I think we would need the same,
>> so we don't end up to introduce a behavior similar to what XSA-387 has
>> fixed on x86.
> Yes, this matches my understanding.
>
> Jan
>
--
Regards,
Oleksandr Tyshchenko
On 09.02.2022 11:08, Oleksandr Tyshchenko wrote:
>
> On 08.02.22 14:47, Jan Beulich wrote:
>
>
> Hi Julien, Jan
>
>
>> On 08.02.2022 12:58, Julien Grall wrote:
>>> On 07/02/2022 19:56, Oleksandr Tyshchenko wrote:
>>>> On 07.02.22 19:15, Julien Grall wrote:
>>>>> Hi Oleksandr,
>>>>> On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
>>>>>> 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. This patch is based on
>>>>>> the assumption that grant table page is the xenheap page.
>>>>> I would write "grant table pages are xenheap pages" or "a grant table
>>>>> page is a xenheap page".
>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>>>>> b/xen/arch/arm/include/asm/grant_table.h
>>>>>> index d31a4d6..d6fda31 100644
>>>>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>>>>> +++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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,
>>>>>> mfn) \
>>>>>> ({ \
>>>>>> - int rc_ =
>>>>>> 0; \
>>>>>> gfn_t ogfn = gnttab_get_frame_gfn(gt, st,
>>>>>> idx); \
>>>>>> - if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn)
>>>>>> || \
>>>>>> - (rc_ = guest_physmap_remove_page((gt)->domain, ogfn,
>>>>>> mfn, \
>>>>>> - 0)) == 0
>>>>>> ) \
>>>>>> - ((st) ?
>>>>>> (gt)->arch.status_gfn \
>>>>>> - : (gt)->arch.shared_gfn)[idx] =
>>>>>> (gfn); \
>>>>>> - rc_; \
>>>>>> + (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn,
>>>>>> gfn)) \
>>>>>> + ? guest_physmap_remove_page((gt)->domain, ogfn, mfn,
>>>>>> 0) \
>>>>>> + :
>>>>>> 0; \
>>>>> Given that we are implementing something similar to an M2P, I was
>>>>> expecting the implementation to be pretty much the same as the x86
>>>>> helper.
>>>>>
>>>>> Would you be able to outline why it is different?
>>>> Being honest, I didn't think about it so far. But, I agree with the
>>>> question.
>>>>
>>>> It feels to me that Arm variant can now behave as x86 one (as
>>>> xenmem_add_to_physmap_one() now checks for the prior mapping), I mean to
>>>> use INVALID_GFN as an indication to remove a page.
>>>>
>>>> What do you think?
>>> I will defer that to Jan.
>>>
>>> Jan, IIRC you were the one introducing the call to
>>> guest_physmap_remove_page(). Do you remember why the difference between
>>> x86 and Arm were necessary?
>> The code was different before, and Arm's behavior was also different.
>> Hence the two functions couldn't be quite similar. If Arm behavior is
>> now converging with x86'es, the functions becoming more similar is
>> not entirely unexpected.
>
> If Arm's gnttab_set_frame_gfn appears to be the similar to x86's one,
> what would be the next step?
>
> I thought of the following options:
>
> 1. Leave per-arch helpers as they are
> 2. Move helper to the common grant_table.h
> 3. Remove the helpers, call guest_physmap_remove_page() directly from
> the common grant_table.c
Well, "similar" is not enough for 2 or 3, but if they end up identical,
then I guess 3 is the way to go unless we foresee e.g. RISC-V wanting
something different. But this would be a separate change, so the
similarity level of the two implementations can actually be easily
seen during review (or later when doing archaeology).
Jan
On 09.02.22 13:04, Jan Beulich wrote:
Hi Jan
> On 09.02.2022 11:08, Oleksandr Tyshchenko wrote:
>> On 08.02.22 14:47, Jan Beulich wrote:
>>
>>
>> Hi Julien, Jan
>>
>>
>>> On 08.02.2022 12:58, Julien Grall wrote:
>>>> On 07/02/2022 19:56, Oleksandr Tyshchenko wrote:
>>>>> On 07.02.22 19:15, Julien Grall wrote:
>>>>>> Hi Oleksandr,
>>>>>> On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
>>>>>>> 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. This patch is based on
>>>>>>> the assumption that grant table page is the xenheap page.
>>>>>> I would write "grant table pages are xenheap pages" or "a grant table
>>>>>> page is a xenheap page".
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>>>>>> b/xen/arch/arm/include/asm/grant_table.h
>>>>>>> index d31a4d6..d6fda31 100644
>>>>>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>>>>>> +++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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,
>>>>>>> mfn) \
>>>>>>> ({ \
>>>>>>> - int rc_ =
>>>>>>> 0; \
>>>>>>> gfn_t ogfn = gnttab_get_frame_gfn(gt, st,
>>>>>>> idx); \
>>>>>>> - if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn)
>>>>>>> || \
>>>>>>> - (rc_ = guest_physmap_remove_page((gt)->domain, ogfn,
>>>>>>> mfn, \
>>>>>>> - 0)) == 0
>>>>>>> ) \
>>>>>>> - ((st) ?
>>>>>>> (gt)->arch.status_gfn \
>>>>>>> - : (gt)->arch.shared_gfn)[idx] =
>>>>>>> (gfn); \
>>>>>>> - rc_; \
>>>>>>> + (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn,
>>>>>>> gfn)) \
>>>>>>> + ? guest_physmap_remove_page((gt)->domain, ogfn, mfn,
>>>>>>> 0) \
>>>>>>> + :
>>>>>>> 0; \
>>>>>> Given that we are implementing something similar to an M2P, I was
>>>>>> expecting the implementation to be pretty much the same as the x86
>>>>>> helper.
>>>>>>
>>>>>> Would you be able to outline why it is different?
>>>>> Being honest, I didn't think about it so far. But, I agree with the
>>>>> question.
>>>>>
>>>>> It feels to me that Arm variant can now behave as x86 one (as
>>>>> xenmem_add_to_physmap_one() now checks for the prior mapping), I mean to
>>>>> use INVALID_GFN as an indication to remove a page.
>>>>>
>>>>> What do you think?
>>>> I will defer that to Jan.
>>>>
>>>> Jan, IIRC you were the one introducing the call to
>>>> guest_physmap_remove_page(). Do you remember why the difference between
>>>> x86 and Arm were necessary?
>>> The code was different before, and Arm's behavior was also different.
>>> Hence the two functions couldn't be quite similar. If Arm behavior is
>>> now converging with x86'es, the functions becoming more similar is
>>> not entirely unexpected.
>> If Arm's gnttab_set_frame_gfn appears to be the similar to x86's one,
>> what would be the next step?
>>
>> I thought of the following options:
>>
>> 1. Leave per-arch helpers as they are
>> 2. Move helper to the common grant_table.h
>> 3. Remove the helpers, call guest_physmap_remove_page() directly from
>> the common grant_table.c
> Well, "similar" is not enough for 2 or 3, but if they end up identical,
> then I guess 3 is the way to go unless we foresee e.g. RISC-V wanting
> something different. But this would be a separate change, so the
> similarity level of the two implementations can actually be easily
> seen during review (or later when doing archaeology).
Thank you for the clarification. I will go with 1.
>
> Jan
>
--
Regards,
Oleksandr Tyshchenko
On 08.02.22 13:58, Julien Grall wrote:
> Hi,
Hi Julien
>
> On 07/02/2022 19:56, Oleksandr Tyshchenko wrote:
>>
>> On 07.02.22 19:15, Julien Grall wrote:
>>> Hi Oleksandr,
>>> On 05/01/2022 23:11, Oleksandr Tyshchenko wrote:
>>>> 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. This patch is based on
>>>> the assumption that grant table page is the xenheap page.
>>>
>>> I would write "grant table pages are xenheap pages" or "a grant table
>>> page is a xenheap page".
>>>
>>> [...]
>>>
>>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>>> b/xen/arch/arm/include/asm/grant_table.h
>>>> index d31a4d6..d6fda31 100644
>>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>>> +++ b/xen/arch/arm/include/asm/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,41 +41,12 @@ 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,
>>>> mfn) \
>>>> ({ \
>>>> - int rc_ =
>>>> 0; \
>>>> gfn_t ogfn = gnttab_get_frame_gfn(gt, st,
>>>> idx); \
>>>> - if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn)
>>>> || \
>>>> - (rc_ = guest_physmap_remove_page((gt)->domain, ogfn,
>>>> mfn, \
>>>> - 0)) == 0
>>>> ) \
>>>> - ((st) ?
>>>> (gt)->arch.status_gfn \
>>>> - : (gt)->arch.shared_gfn)[idx] =
>>>> (gfn); \
>>>> - rc_; \
>>>> + (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn,
>>>> gfn)) \
>>>> + ? guest_physmap_remove_page((gt)->domain, ogfn, mfn,
>>>> 0) \
>>>> + :
>>>> 0; \
>>>
>>> Given that we are implementing something similar to an M2P, I was
>>> expecting the implementation to be pretty much the same as the x86
>>> helper.
>>>
>>> Would you be able to outline why it is different?
>>
>> Being honest, I didn't think about it so far. But, I agree with the
>> question.
>>
>> It feels to me that Arm variant can now behave as x86 one (as
>> xenmem_add_to_physmap_one() now checks for the prior mapping), I mean to
>> use INVALID_GFN as an indication to remove a page.
>>
>> What do you think?
>
> I will defer that to Jan.
I will comment on this in a separate mail where Jan already answered.
>
>
> Jan, IIRC you were the one introducing the call to
> guest_physmap_remove_page(). Do you remember why the difference
> between x86 and Arm were necessary?
>
> [...]
>
>>>
>>>
>>>> + */
>>>> +#define PGT_gfn_width PG_shift(3)
>>>> +#define PGT_gfn_mask ((1UL<<PGT_gfn_width)-1)
>>>> +
>>>> +#define PGT_INVALID_XENHEAP_GFN _gfn(PGT_gfn_mask)
>>>> +
>>>> +/*
>>>> + * An arch-specific initialization pattern is needed for the
>>>> type_info field
>>>> + * as it's GFN portion can contain the valid GFN if page is xenheap
>>>> page.
>>>> + */
>>>> +#define PGT_TYPE_INFO_INIT_PATTERN gfn_x(PGT_INVALID_XENHEAP_GFN)
>>>> /* Cleared when the owning guest 'frees' this page. */
>>>> #define _PGC_allocated PG_shift(1)
>>>> @@ -358,6 +371,25 @@ void clear_and_clean_page(struct page_info
>>>> *page);
>>>> unsigned int arch_get_dma_bitsize(void);
>>>> +static inline gfn_t page_get_xenheap_gfn(const struct page_info
>>>> *p)
>>>> +{
>>>> + gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
>>>> +
>>>> + ASSERT(is_xen_heap_page(p));
>>>> +
>>>> + return gfn_eq(gfn_, PGT_INVALID_XENHEAP_GFN) ? INVALID_GFN :
>>>> gfn_;
>>>> +}
>>>> +
>>>> +static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t
>>>> gfn)
>>>> +{
>>>> + gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN
>>>> : gfn;
>>>> +
>>>> + ASSERT(is_xen_heap_page(p));
>>>> +
>>>> + p->u.inuse.type_info &= ~PGT_gfn_mask;
>>>> + p->u.inuse.type_info |= gfn_x(gfn_);
>>>> +}
>>>
>>> This is not going to be atomic. So can you outline which locking
>>> mechanism should be used to protect access (set/get) to the GFN?
>>
>>
>> I think, the P2M lock.
>
> Ok. So, looking at the code, most of calls to page_get_xenheap_gfn()
> are not protected with the p2m_lock().
Yes.
Thank you for the suggestions below, I feel I need some clarifications ...
>
>
> (Jan please confirm) If I am not mistaken, on x86, a read to the M2P
> is not always protected. But they have code within the P2M lock to
> check any difference (see p2m_remove_page()). I think we would need
> the same, so we don't end up to introduce a behavior similar to what
> XSA-387 has fixed on x86.
... OK, I assume you are speaking about the check in the loop that was
added by the following commit:
c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e "x86/p2m: don't assert that the
passed in MFN matches for a remove"
Also, I assume we need that check in the same place on Arm (with P2M
lock held), which, I think, could be p2m_remove_mapping().
I ported the check from x86 code, but this is not a verbatim copy due to
the difference in local P2M helpers/macro between arches, also I have
skipped a part of that check "|| t == p2m_mmio_direct" which was added
by one of the follow-up commit:
753cb68e653002e89fdcd1c80e52905fdbfb78cb "x86/p2m: guard (in particular)
identity mapping entries"
since I have no idea whether we need the same on Arm.
Below the diff I have locally:
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5646343..90d7563 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1315,11 +1315,32 @@ static inline int p2m_remove_mapping(struct
domain *d,
mfn_t mfn)
{
struct p2m_domain *p2m = p2m_get_hostp2m(d);
+ unsigned long i;
int rc;
p2m_write_lock(p2m);
+ for ( i = 0; i < nr; )
+ {
+ unsigned int cur_order;
+ bool valid;
+ mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
NULL, NULL,
+ &cur_order, &valid);
+
+ if ( valid &&
+ (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) )
+ {
+ rc = -EILSEQ;
+ goto out;
+ }
+
+ i += (1UL << cur_order) -
+ ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
+ }
+
rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
p2m_invalid, p2m_access_rwx);
+
+out:
p2m_write_unlock(p2m);
return rc;
Could you please clarify, is it close to what you had in mind? If yes, I
am wondering, don't we need this check to be only executed for xenheap
pages (and, probably, which P2M's entry type in RAM?) rather than for
all pages?
>
>
> In addition to that, if p2m_get_xenheap_gfn() is going to be called
> locklessly. Then we need to make sure the update to type_info are
> atomic. This means:
> - p2m_get_xenheap_gfn() should use READ_ONCE().
> - p2m_set_xenheap_gfn() should use WRITE_ONCE(). We might even need
> to use cmpxchg() if there are other update to type_info that are not
> protected. I will let you have a look.
... OK, I didn't find READ_ONCE/WRITE_ONCE in Xen. I am wondering, can
we use ACCESS_ONCE instead?
Below the diff I have locally:
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 9e093a6..b18acb7 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -373,7 +373,7 @@ unsigned int arch_get_dma_bitsize(void);
static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
{
- gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
+ gfn_t gfn_ = _gfn(ACCESS_ONCE(p->u.inuse.type_info) & PGT_gfn_mask);
ASSERT(is_xen_heap_page(p));
@@ -383,11 +383,14 @@ static inline gfn_t page_get_xenheap_gfn(const
struct page_info *p)
static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
{
gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN : gfn;
+ unsigned long type_info;
ASSERT(is_xen_heap_page(p));
- p->u.inuse.type_info &= ~PGT_gfn_mask;
- p->u.inuse.type_info |= gfn_x(gfn_);
+ type_info = ACCESS_ONCE(p->u.inuse.type_info);
+ type_info &= ~PGT_gfn_mask;
+ type_info |= gfn_x(gfn_);
+ ACCESS_ONCE(p->u.inuse.type_info) = type_info;
}
#endif /* __ARCH_ARM_MM__ */
It is going to be a non-protected write to GFN portion of type_info.
But, at that time the page is not used yet, so I think this is harmless.
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 50334a0..97cf0d8 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1024,7 +1024,7 @@ static struct page_info *alloc_heap_pages(
&tlbflush_timestamp);
/* Initialise fields which have other uses for free pages. */
- pg[i].u.inuse.type_info = 0;
+ pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
page_set_owner(&pg[i], NULL);
}
>
>
> Cheers,
>
--
Regards,
Oleksandr Tyshchenko
On 08/02/2022 19:50, Oleksandr Tyshchenko wrote:
>
> On 08.02.22 13:58, Julien Grall wrote:
>> Hi,
>
> Hi Julien
Hi,
>>
>>
>> (Jan please confirm) If I am not mistaken, on x86, a read to the M2P
>> is not always protected. But they have code within the P2M lock to
>> check any difference (see p2m_remove_page()). I think we would need
>> the same, so we don't end up to introduce a behavior similar to what
>> XSA-387 has fixed on x86.
>
>
> ... OK, I assume you are speaking about the check in the loop that was
> added by the following commit:
> c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e "x86/p2m: don't assert that the
> passed in MFN matches for a remove"
Yes, this is the one I Have in mind.
> Also, I assume we need that check in the same place on Arm (with P2M
> lock held), which, I think, could be p2m_remove_mapping().
I believe so. Can you do some testing to check this would not break
other types of mapping? (Booting a guest and using PV device should be
enough).
>
> I ported the check from x86 code, but this is not a verbatim copy due to
> the difference in local P2M helpers/macro between arches, also I have
> skipped a part of that check "|| t == p2m_mmio_direct" which was added
> by one of the follow-up commit:
> 753cb68e653002e89fdcd1c80e52905fdbfb78cb "x86/p2m: guard (in particular)
> identity mapping entries"
> since I have no idea whether we need the same on Arm.
I am not entirely sure. For now, I would drop it so long the behavior
stay the same (i.e. it will go ahead with removing the mappings).t.
> Below the diff I have locally:
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 5646343..90d7563 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1315,11 +1315,32 @@ static inline int p2m_remove_mapping(struct
> domain *d,
> mfn_t mfn)
> {
> struct p2m_domain *p2m = p2m_get_hostp2m(d);
> + unsigned long i;
> int rc;
>
> p2m_write_lock(p2m);
> + for ( i = 0; i < nr; )
> + {
> + unsigned int cur_order;
> + bool valid;
> + mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
> NULL, NULL,
> + &cur_order, &valid); > +
> + if ( valid &&
valid is a copy of the LPAE bit valid. This may be 0 if Xen decided to
clear it (i.e when emulating set/way). Yet the mapping itself is
considered valid from Xen PoV.
So you want to replace with a different check (see below).
> + (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) )
> + {
> + rc = -EILSEQ;
> + goto out;
> + }
> +
> + i += (1UL << cur_order) -
> + ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
> + }
> +
> rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
> p2m_invalid, p2m_access_rwx);
> +
> +out:
> p2m_write_unlock(p2m);
>
> return rc;
>
>
> Could you please clarify, is it close to what you had in mind? If yes, I
> am wondering, don't we need this check to be only executed for xenheap
> pages (and, probably, which P2M's entry type in RAM?) rather than for
> all pages?
From my understanding, for the purpose of this work, we only strictly
need to check that for xenheap pages.
But I think it would be a good opportunity to harden the P2M code. At
the moment, on Arm, you can remove any mappings you want (even with the
wrong helpers). This lead us to a few issues when mapping were overriden
silently (in particular when building dom0).
So I would say we should enforce it for every RAM mapping. Stefano,
Bertrand, what do you think?
Note that, I would like to see this change in a separate commit. It will
be easier to review.
>
>
>>
>>
>> In addition to that, if p2m_get_xenheap_gfn() is going to be called
>> locklessly. Then we need to make sure the update to type_info are
>> atomic. This means:
>> - p2m_get_xenheap_gfn() should use READ_ONCE().
>> - p2m_set_xenheap_gfn() should use WRITE_ONCE(). We might even need
>> to use cmpxchg() if there are other update to type_info that are not
>> protected. I will let you have a look.
>
>
> ... OK, I didn't find READ_ONCE/WRITE_ONCE in Xen. I am wondering, can
> we use ACCESS_ONCE instead?
Yes. Sorry, I keep forgetting we don't have READ_ONCE/WRITE_ONCE in Xen.
>
> Below the diff I have locally:
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 9e093a6..b18acb7 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -373,7 +373,7 @@ unsigned int arch_get_dma_bitsize(void);
>
> static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
> {
> - gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
> + gfn_t gfn_ = _gfn(ACCESS_ONCE(p->u.inuse.type_info) & PGT_gfn_mask);
>
> ASSERT(is_xen_heap_page(p));
>
> @@ -383,11 +383,14 @@ static inline gfn_t page_get_xenheap_gfn(const
> struct page_info *p)
> static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t gfn)
> {
> gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? PGT_INVALID_XENHEAP_GFN : gfn;
> + unsigned long type_info;
>
> ASSERT(is_xen_heap_page(p));
>
> - p->u.inuse.type_info &= ~PGT_gfn_mask;
> - p->u.inuse.type_info |= gfn_x(gfn_);
> + type_info = ACCESS_ONCE(p->u.inuse.type_info);
> + type_info &= ~PGT_gfn_mask;
> + type_info |= gfn_x(gfn_);
> + ACCESS_ONCE(p->u.inuse.type_info) = type_info;
> }
>
> #endif /* __ARCH_ARM_MM__ */
>
>
> It is going to be a non-protected write to GFN portion of type_info.
Well no. You are using a Read-Modify-Write operation on type_info. This
is not atomic and will overwrite any change (if any) done on other part
of the type_info.
If I am mistaken, there are two other places where type_info is
modified. One is...
> But, at that time the page is not used yet, so I think this is harmless.
>
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 50334a0..97cf0d8 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1024,7 +1024,7 @@ static struct page_info *alloc_heap_pages(
> &tlbflush_timestamp);
>
> /* Initialise fields which have other uses for free pages. */
> - pg[i].u.inuse.type_info = 0;
> + pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
> page_set_owner(&pg[i], NULL);
>
> }
... this one. I agree the page is not accessible at this time. So
page_set_xenheap_gfn() should not be used.
The other one is in share_xen_page_with_guest() which I think is still
fine because the caller page_set_xenheap_gfn() would need to acquire a
reference on the page. This is only possible after the count_info is
updated in share_xen_page_with_guest() *and* there a barrier between the
type_info and count_info.
I think this behavior should be documented on top of type_info (along
with the locking). This would be helpful if type_info gain more use in
the future.
Cheers,
--
Julien Grall
On 10.02.22 11:46, Julien Grall wrote:
>
>
> On 08/02/2022 19:50, Oleksandr Tyshchenko wrote:
>>
>> On 08.02.22 13:58, Julien Grall wrote:
>>> Hi,
>>
>> Hi Julien
>
> Hi,
Hi Julien
Thank you for the clarifications!
>
>>>
>>>
>>> (Jan please confirm) If I am not mistaken, on x86, a read to the M2P
>>> is not always protected. But they have code within the P2M lock to
>>> check any difference (see p2m_remove_page()). I think we would need
>>> the same, so we don't end up to introduce a behavior similar to what
>>> XSA-387 has fixed on x86.
>>
>>
>> ... OK, I assume you are speaking about the check in the loop that was
>> added by the following commit:
>> c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e "x86/p2m: don't assert that the
>> passed in MFN matches for a remove"
>
> Yes, this is the one I Have in mind.
good
>
>> Also, I assume we need that check in the same place on Arm (with P2M
>> lock held), which, I think, could be p2m_remove_mapping().
>
> I believe so.
good
> Can you do some testing to check this would not break other types of
> mapping? (Booting a guest and using PV device should be enough).
Sure, already checked and will check more thoroughly before submitting.
>
>
>>
>> I ported the check from x86 code, but this is not a verbatim copy due to
>> the difference in local P2M helpers/macro between arches, also I have
>> skipped a part of that check "|| t == p2m_mmio_direct" which was added
>> by one of the follow-up commit:
>> 753cb68e653002e89fdcd1c80e52905fdbfb78cb "x86/p2m: guard (in particular)
>> identity mapping entries"
>> since I have no idea whether we need the same on Arm.
>
> I am not entirely sure. For now, I would drop it so long the behavior
> stay the same (i.e. it will go ahead with removing the mappings).t.
ok, will drop.
>
>
>> Below the diff I have locally:
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 5646343..90d7563 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1315,11 +1315,32 @@ static inline int p2m_remove_mapping(struct
>> domain *d,
>> mfn_t mfn)
>> {
>> struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> + unsigned long i;
>> int rc;
>>
>> p2m_write_lock(p2m);
>> + for ( i = 0; i < nr; )
>> + {
>> + unsigned int cur_order;
>> + bool valid;
>> + mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
>> NULL, NULL,
>> + &cur_order, &valid); > +
>> + if ( valid &&
>
> valid is a copy of the LPAE bit valid. This may be 0 if Xen decided to
> clear it (i.e when emulating set/way). Yet the mapping itself is
> considered valid from Xen PoV.
>
> So you want to replace with a different check (see below).
Hmm, I got it, so ...
>
>
>> + (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i),
>> mfn_return)) )
>> + {
>> + rc = -EILSEQ;
>> + goto out;
>> + }
>> +
>> + i += (1UL << cur_order) -
>> + ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
>> + }
>> +
>> rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
>> p2m_invalid, p2m_access_rwx);
>> +
>> +out:
>> p2m_write_unlock(p2m);
>>
>> return rc;
>>
>>
>> Could you please clarify, is it close to what you had in mind? If yes, I
>> am wondering, don't we need this check to be only executed for xenheap
>> pages (and, probably, which P2M's entry type in RAM?) rather than for
>> all pages?
>
> From my understanding, for the purpose of this work, we only strictly
> need to check that for xenheap pages.
... yes, but ...
>
>
> But I think it would be a good opportunity to harden the P2M code. At
> the moment, on Arm, you can remove any mappings you want (even with
> the wrong helpers). This lead us to a few issues when mapping were
> overriden silently (in particular when building dom0).
> So I would say we should enforce it for every RAM mapping.
... I think this makes sense, so the proper check in that case, I
assume, should contain p2m_is_any_ram() macro:
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5646343..2b82c49 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1315,11 +1315,32 @@ static inline int p2m_remove_mapping(struct
domain *d,
mfn_t mfn)
{
struct p2m_domain *p2m = p2m_get_hostp2m(d);
+ unsigned long i;
int rc;
p2m_write_lock(p2m);
+ for ( i = 0; i < nr; )
+ {
+ unsigned int cur_order;
+ p2m_type_t t;
+ mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
&t, NULL,
+ &cur_order, NULL);
+
+ if ( p2m_is_any_ram(t) &&
+ (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) )
+ {
+ rc = -EILSEQ;
+ goto out;
+ }
+
+ i += (1UL << cur_order) -
+ ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
+ }
+
rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
p2m_invalid, p2m_access_rwx);
+
+out:
p2m_write_unlock(p2m);
return rc;
(END)
> Stefano, Bertrand, what do you think?
>
> Note that, I would like to see this change in a separate commit. It
> will be easier to review.
ok, I will introduce this check by a separate patch.
>
>
>>
>>
>>>
>>>
>>> In addition to that, if p2m_get_xenheap_gfn() is going to be called
>>> locklessly. Then we need to make sure the update to type_info are
>>> atomic. This means:
>>> - p2m_get_xenheap_gfn() should use READ_ONCE().
>>> - p2m_set_xenheap_gfn() should use WRITE_ONCE(). We might even need
>>> to use cmpxchg() if there are other update to type_info that are not
>>> protected. I will let you have a look.
>>
>>
>> ... OK, I didn't find READ_ONCE/WRITE_ONCE in Xen. I am wondering, can
>> we use ACCESS_ONCE instead?
>
> Yes. Sorry, I keep forgetting we don't have READ_ONCE/WRITE_ONCE in Xen.
ok
>
>>
>> Below the diff I have locally:
>>
>> diff --git a/xen/arch/arm/include/asm/mm.h
>> b/xen/arch/arm/include/asm/mm.h
>> index 9e093a6..b18acb7 100644
>> --- a/xen/arch/arm/include/asm/mm.h
>> +++ b/xen/arch/arm/include/asm/mm.h
>> @@ -373,7 +373,7 @@ unsigned int arch_get_dma_bitsize(void);
>>
>> static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
>> {
>> - gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
>> + gfn_t gfn_ = _gfn(ACCESS_ONCE(p->u.inuse.type_info) &
>> PGT_gfn_mask);
>>
>> ASSERT(is_xen_heap_page(p));
>>
>> @@ -383,11 +383,14 @@ static inline gfn_t page_get_xenheap_gfn(const
>> struct page_info *p)
>> static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t
>> gfn)
>> {
>> gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ?
>> PGT_INVALID_XENHEAP_GFN : gfn;
>> + unsigned long type_info;
>>
>> ASSERT(is_xen_heap_page(p));
>>
>> - p->u.inuse.type_info &= ~PGT_gfn_mask;
>> - p->u.inuse.type_info |= gfn_x(gfn_);
>> + type_info = ACCESS_ONCE(p->u.inuse.type_info);
>> + type_info &= ~PGT_gfn_mask;
>> + type_info |= gfn_x(gfn_);
>> + ACCESS_ONCE(p->u.inuse.type_info) = type_info;
>> }
>>
>> #endif /* __ARCH_ARM_MM__ */
>>
>>
>> It is going to be a non-protected write to GFN portion of type_info.
>
> Well no. You are using a Read-Modify-Write operation on type_info.
> This is not atomic and will overwrite any change (if any) done on
> other part of the type_info.
I am confused a bit, to which my comment your comment above belongs (to
the diff for page_set_xenheap_gfn() above or to sentence right after it)?
The "It is going to be a non-protected write to GFN portion of
type_info." sentence is related to the diff for alloc_heap_pages()
below. Sorry if I didn't separate the comments properly.
>
>
> If I am mistaken, there are two other places where type_info is
> modified. One is...
>
>
>> But, at that time the page is not used yet, so I think this is harmless.
>>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 50334a0..97cf0d8 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1024,7 +1024,7 @@ static struct page_info *alloc_heap_pages(
>> &tlbflush_timestamp);
>>
>> /* Initialise fields which have other uses for free
>> pages. */
>> - pg[i].u.inuse.type_info = 0;
>> + pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
>> page_set_owner(&pg[i], NULL);
>>
>> }
>
> ... this one. I agree the page is not accessible at this time. So
> page_set_xenheap_gfn() should not be used.
yes
>
>
> The other one is in share_xen_page_with_guest() which I think is still
> fine because the caller page_set_xenheap_gfn() would need to acquire a
> reference on the page. This is only possible after the count_info is
> updated in share_xen_page_with_guest() *and* there a barrier between
> the type_info and count_info.
>
> I think this behavior should be documented on top of type_info (along
> with the locking). This would be helpful if type_info gain more use in
> the future.
agree, will do.
>
>
> Cheers,
>
--
Regards,
Oleksandr Tyshchenko
Hi Oleksandr,
On 10/02/2022 16:55, Oleksandr wrote:
>
> On 10.02.22 11:46, Julien Grall wrote:
>>
>>
>> On 08/02/2022 19:50, Oleksandr Tyshchenko wrote:
>>>
>>> On 08.02.22 13:58, Julien Grall wrote:
>>> Below the diff I have locally:
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 5646343..90d7563 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -1315,11 +1315,32 @@ static inline int p2m_remove_mapping(struct
>>> domain *d,
>>> mfn_t mfn)
>>> {
>>> struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>> + unsigned long i;
>>> int rc;
>>>
>>> p2m_write_lock(p2m);
>>> + for ( i = 0; i < nr; )
>>> + {
>>> + unsigned int cur_order;
>>> + bool valid;
>>> + mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
>>> NULL, NULL,
>>> + &cur_order, &valid); > +
>>> + if ( valid &&
>>
>> valid is a copy of the LPAE bit valid. This may be 0 if Xen decided to
>> clear it (i.e when emulating set/way). Yet the mapping itself is
>> considered valid from Xen PoV.
>>
>> So you want to replace with a different check (see below).
>
>
> Hmm, I got it, so ...
>
>
>>
>>
>>> + (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i),
>>> mfn_return)) )
>>> + {
>>> + rc = -EILSEQ;
>>> + goto out;
>>> + }
>>> +
>>> + i += (1UL << cur_order) -
>>> + ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
>>> + }
>>> +
>>> rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
>>> p2m_invalid, p2m_access_rwx);
>>> +
>>> +out:
>>> p2m_write_unlock(p2m);
>>>
>>> return rc;
>>>
>>>
>>> Could you please clarify, is it close to what you had in mind? If yes, I
>>> am wondering, don't we need this check to be only executed for xenheap
>>> pages (and, probably, which P2M's entry type in RAM?) rather than for
>>> all pages?
>>
>> From my understanding, for the purpose of this work, we only strictly
>> need to check that for xenheap pages.
>
>
> ... yes, but ...
>
>
>>
>>
>> But I think it would be a good opportunity to harden the P2M code. At
>> the moment, on Arm, you can remove any mappings you want (even with
>> the wrong helpers). This lead us to a few issues when mapping were
>> overriden silently (in particular when building dom0).
>> So I would say we should enforce it for every RAM mapping.
>
>
> ... I think this makes sense, so the proper check in that case, I
> assume, should contain p2m_is_any_ram() macro:
Correct, p2m_is_any_ram() looks the macro we want to use here.
>> Note that, I would like to see this change in a separate commit. It
>> will be easier to review.
>
>
> ok, I will introduce this check by a separate patch.
Thank you!
[...]
>>> It is going to be a non-protected write to GFN portion of type_info.
>>
>> Well no. You are using a Read-Modify-Write operation on type_info.
>> This is not atomic and will overwrite any change (if any) done on
>> other part of the type_info.
>
>
> I am confused a bit, to which my comment your comment above belongs (to
> the diff for page_set_xenheap_gfn() above or to sentence right after it)?
> The "It is going to be a non-protected write to GFN portion of
> type_info." sentence is related to the diff for alloc_heap_pages()
> below. Sorry if I didn't separate the comments properly.
Ok. So it will be a write operation, but I still don't understand why
you think it is just the GFN portion.
The code is using "...type_info = PGT_TYPE_INFO_INITIALIZER". So the
full 64-bit (assuming arm64) will be modified.
In general, the GFN takes 60 of the 64-bits. So any time you need to
modify the GFN (or the count_info), you will end up to modify the entire
of type_info (and vice-versa). If this is becoming we problem (e.g. the
count_info is actively used) we will need to use cmpxchg() to modify the
GFN portion.
Cheers,
--
Julien Grall
On 13.02.22 18:06, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien
>
> On 10/02/2022 16:55, Oleksandr wrote:
>>
>> On 10.02.22 11:46, Julien Grall wrote:
>>>
>>>
>>> On 08/02/2022 19:50, Oleksandr Tyshchenko wrote:
>>>>
>>>> On 08.02.22 13:58, Julien Grall wrote:
>>>> Below the diff I have locally:
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index 5646343..90d7563 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -1315,11 +1315,32 @@ static inline int p2m_remove_mapping(struct
>>>> domain *d,
>>>> mfn_t mfn)
>>>> {
>>>> struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>> + unsigned long i;
>>>> int rc;
>>>>
>>>> p2m_write_lock(p2m);
>>>> + for ( i = 0; i < nr; )
>>>> + {
>>>> + unsigned int cur_order;
>>>> + bool valid;
>>>> + mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
>>>> NULL, NULL,
>>>> + &cur_order, &valid); > +
>>>> + if ( valid &&
>>>
>>> valid is a copy of the LPAE bit valid. This may be 0 if Xen decided
>>> to clear it (i.e when emulating set/way). Yet the mapping itself is
>>> considered valid from Xen PoV.
>>>
>>> So you want to replace with a different check (see below).
>>
>>
>> Hmm, I got it, so ...
>>
>>
>>>
>>>
>>>> + (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i),
>>>> mfn_return)) )
>>>> + {
>>>> + rc = -EILSEQ;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + i += (1UL << cur_order) -
>>>> + ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
>>>> + }
>>>> +
>>>> rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
>>>> p2m_invalid, p2m_access_rwx);
>>>> +
>>>> +out:
>>>> p2m_write_unlock(p2m);
>>>>
>>>> return rc;
>>>>
>>>>
>>>> Could you please clarify, is it close to what you had in mind? If
>>>> yes, I
>>>> am wondering, don't we need this check to be only executed for xenheap
>>>> pages (and, probably, which P2M's entry type in RAM?) rather than for
>>>> all pages?
>>>
>>> From my understanding, for the purpose of this work, we only
>>> strictly need to check that for xenheap pages.
>>
>>
>> ... yes, but ...
>>
>>
>>>
>>>
>>> But I think it would be a good opportunity to harden the P2M code.
>>> At the moment, on Arm, you can remove any mappings you want (even
>>> with the wrong helpers). This lead us to a few issues when mapping
>>> were overriden silently (in particular when building dom0).
>>> So I would say we should enforce it for every RAM mapping.
>>
>>
>> ... I think this makes sense, so the proper check in that case, I
>> assume, should contain p2m_is_any_ram() macro:
>
>
> Correct, p2m_is_any_ram() looks the macro we want to use here.
Good, thank you for the clarification! FYI, I have already re-checked
with p2m_is_any_ram(). DomU with PV devices (display, sound, net) and
Virtio (block) boots without any issues, the reboot and destroy also
work. To be clear, all backends in my environment reside in DomD.
>
>>> Note that, I would like to see this change in a separate commit. It
>>> will be easier to review.
>>
>>
>> ok, I will introduce this check by a separate patch.
>
> Thank you!
>
> [...]
>
>>>> It is going to be a non-protected write to GFN portion of type_info.
>>>
>>> Well no. You are using a Read-Modify-Write operation on type_info.
>>> This is not atomic and will overwrite any change (if any) done on
>>> other part of the type_info.
>>
>>
>> I am confused a bit, to which my comment your comment above belongs
>> (to the diff for page_set_xenheap_gfn() above or to sentence right
>> after it)?
>> The "It is going to be a non-protected write to GFN portion of
>> type_info." sentence is related to the diff for alloc_heap_pages()
>> below. Sorry if I didn't separate the comments properly.
>
> Ok. So it will be a write operation, but I still don't understand why
> you think it is just the GFN portion.
>
> The code is using "...type_info = PGT_TYPE_INFO_INITIALIZER". So the
> full 64-bit (assuming arm64) will be modified.
You are right, I wasn't precise, sorry.
>
>
> In general, the GFN takes 60 of the 64-bits. So any time you need to
> modify the GFN (or the count_info), you will end up to modify the
> entire of type_info (and vice-versa). If this is becoming we problem
> (e.g. the count_info is actively used) we will need to use cmpxchg()
> to modify the GFN portion.
I got it, as I understood from your explanation about
share_xen_page_with_guest() at [1] this is not a problem yet within
current code base.
[1]
https://lore.kernel.org/xen-devel/a104d3ea-170e-8175-ac04-abfcebb4ae29@xen.org/
>
>
> Cheers,
>
--
Regards,
Oleksandr Tyshchenko
© 2016 - 2024 Red Hat, Inc.