[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH V5] xen/gnttab: Store frame GFN in struct page_info on Arm
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@xxxxxxxx> >>>> >>>> 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |