[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 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@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. >>> >>> 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |