Re: [PATCH V5] xen/gnttab: Store frame GFN in struct page_info on Arm

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

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