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

[Jan Beulich <jbeulich@xxxxxxxx>]

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