[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.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@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). Thank you for the clarification. I will go with 1. > > Jan > -- Regards, Oleksandr Tyshchenko
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |