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

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 13.02.22 18:06, Julien Grall wrote:
> Hi Oleksandr,


Hi Julien



>
> On 10/02/2022 16:55, Oleksandr wrote:
>>
>> On 10.02.22 11:46, Julien Grall wrote:
>>>
>>>
>>> On 08/02/2022 19:50, Oleksandr Tyshchenko wrote:
>>>>
>>>> On 08.02.22 13:58, Julien Grall wrote:
>>>> Below the diff I have locally:
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index 5646343..90d7563 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -1315,11 +1315,32 @@ static inline int p2m_remove_mapping(struct
>>>> domain *d,
>>>>                                         mfn_t mfn)
>>>>    {
>>>>        struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>> +    unsigned long i;
>>>>        int rc;
>>>>
>>>>        p2m_write_lock(p2m);
>>>> +    for ( i = 0; i < nr; )
>>>> +    {
>>>> +        unsigned int cur_order;
>>>> +        bool valid;
>>>> +        mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
>>>> NULL, NULL,
>>>> +                                         &cur_order, &valid); > +
>>>> +        if ( valid &&
>>>
>>> valid is a copy of the LPAE bit valid. This may be 0 if Xen decided 
>>> to clear it (i.e when emulating set/way). Yet the mapping itself is 
>>> considered valid from Xen PoV.
>>>
>>> So you want to replace with a different check (see below).
>>
>>
>> Hmm, I got it, so ...
>>
>>
>>>
>>>
>>>> +             (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), 
>>>> mfn_return)) )
>>>> +        {
>>>> +            rc = -EILSEQ;
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        i += (1UL << cur_order) -
>>>> +             ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
>>>> +    }
>>>> +
>>>>        rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
>>>>                           p2m_invalid, p2m_access_rwx);
>>>> +
>>>> +out:
>>>>        p2m_write_unlock(p2m);
>>>>
>>>>        return rc;
>>>>
>>>>
>>>> Could you please clarify, is it close to what you had in mind? If 
>>>> yes, I
>>>> am wondering, don't we need this check to be only executed for xenheap
>>>> pages (and, probably, which P2M's entry type in RAM?) rather than for
>>>> all pages?
>>>
>>> From my understanding, for the purpose of this work, we only 
>>> strictly need to check that for xenheap pages.
>>
>>
>>   ... yes, but ...
>>
>>
>>>
>>>
>>> But I think it would be a good opportunity to harden the P2M code. 
>>> At the moment, on Arm, you can remove any mappings you want (even 
>>> with the wrong helpers). This lead us to a few issues when mapping 
>>> were overriden silently (in particular when building dom0).
>>> So I would say we should enforce it for every RAM mapping. 
>>
>>
>> ... I think this makes sense, so the proper check in that case, I 
>> assume, should contain p2m_is_any_ram() macro:
>
>
> Correct, p2m_is_any_ram() looks the macro we want to use here.


Good, thank you for the clarification! FYI, I have already re-checked 
with p2m_is_any_ram(). DomU with PV devices (display, sound, net) and 
Virtio (block) boots without any issues, the reboot and destroy also 
work. To be clear, all backends in my environment reside in DomD.


>
>>> Note that, I would like to see this change in a separate commit. It 
>>> will be easier to review.
>>
>>
>> ok, I will introduce this check by a separate patch.
>
> Thank you!
>
> [...]
>
>>>> It is going to be a non-protected write to GFN portion of type_info.
>>>
>>> Well no. You are using a Read-Modify-Write operation on type_info. 
>>> This is not atomic and will overwrite any change (if any) done on 
>>> other part of the type_info.
>>
>>
>> I am confused a bit, to which my comment your comment above belongs 
>> (to the diff for page_set_xenheap_gfn() above or to sentence right 
>> after it)?
>> The "It is going to be a non-protected write to GFN portion of 
>> type_info." sentence is related to the diff for alloc_heap_pages() 
>> below. Sorry if I didn't separate the comments properly.
>
> Ok. So it will be a write operation, but I still don't understand why 
> you think it is just the GFN portion.
>
> The code is using "...type_info = PGT_TYPE_INFO_INITIALIZER". So the 
> full 64-bit (assuming arm64) will be modified.


You are right, I wasn't precise, sorry.


>
>
> In general, the GFN takes 60 of the 64-bits. So any time you need to 
> modify the GFN (or the count_info), you will end up to modify the 
> entire of type_info (and vice-versa). If this is becoming we problem 
> (e.g. the count_info is actively used) we will need to use cmpxchg() 
> to modify the GFN portion.


I got it, as I understood from your explanation about 
share_xen_page_with_guest() at [1] this is not a problem yet within 
current code base.


[1] 
https://lore.kernel.org/xen-devel/a104d3ea-170e-8175-ac04-abfcebb4ae29@xxxxxxx/


>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko