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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2021 21:57, Oleksandr wrote:
> 
> On 13.09.21 09:17, Jan Beulich wrote:
> 
> Hi Jan
> 
>> On 10.09.2021 09:52, Jan Beulich wrote:
>>> On 10.09.2021 01:04, Oleksandr Tyshchenko wrote:
>>>> @@ -731,11 +733,19 @@ static void p2m_put_l3_page(const lpae_t pte)
>>>>        */
>>>>       if ( p2m_is_foreign(pte.p2m.type) )
>>>>       {
>>>> -        mfn_t mfn = lpae_get_mfn(pte);
>>>> -
>>>>           ASSERT(mfn_valid(mfn));
>>>>           put_page(mfn_to_page(mfn));
>>>>       }
>>>> +
>>>> +#ifdef CONFIG_GRANT_TABLE
>>>> +    /*
>>>> +     * Check whether we deal with grant table page. As the grant table 
>>>> page
>>>> +     * is xen_heap page and its entry has known p2m type, detect it and 
>>>> mark
>>>> +     * the stored GFN as invalid.
>>>> +     */
>>>> +    if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
>>>> +        page_set_frame_gfn(mfn_to_page(mfn), INVALID_GFN);
>>>> +#endif
>>> I take it the write done is benign for other Xen heap pages? I suppose
>>> this would want making very explicit, as such an assumption is easy to
>>> go stale by entirely unrelated changes.
>>>
>>> I also wonder whether you really mean to include p2m_ram_ro pages here
>>> as well.
>> Actually I've meanwhile realized I should put my question here quite
>> differently: Aren't you effectively introducing an M2P here for Arm,
>> except that you artificially limit it to the Xen heap pages needed for
>> the grant table?
> 
> Difficult to say, it might indeed look a bit close to M2P, so the answer 
> to your question is more yes than no. But, I didn't have a plan to 
> introduce M2P on Arm. It turned out that stashing GFN into page_info (as 
> was suggested) avoided huge lookups, so we have got MFN->GFN in the end. 
> The purpose of this patch was just to fix a potential issue with 
> remapping grant-table frame on architecture without the M2P (Arm).

I understand this is the immediate goal. I wonder though if it's helpful
to make this a special case when it can (I think) easily be made general.
But of course there may be (perhaps Arm-specific) aspects which I'm
simply unaware of.

Jan