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

[Julien Grall <julien@xen.org>]

Hi Jan,

On 17/10/2022 14:46, Jan Beulich wrote:
> On 11.10.2022 15:33, Julien Grall wrote:
>> On 11/10/2022 14:28, Jan Beulich wrote:
>>> On 11.10.2022 15:01, Julien Grall wrote:
>>>> On 11/10/2022 12:59, Jan Beulich wrote:
>>>>> On 16.07.2022 16:56, Oleksandr Tyshchenko wrote:
>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>
>>>>>> 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 a grant table page is a xenheap page.
>>>>>>
>>>>>> To cover 64-bit/40-bit IPA on Arm64/Arm32 we need the space
>>>>>> to hold 52-bit/28-bit + extra bit value respectively. In order
>>>>>> to not grow the size of struct page_info borrow the required
>>>>>> amount of bits from type_info's count portion which current
>>>>>> context won't suffer (currently only 1 bit is used on Arm).
>>>>>
>>>>> I'm afraid this isn't true: There's no requirement for a guest to pass
>>>>> all different GFNs to VCPUOP_register_vcpu_info, yet map_vcpu_info()
>>>>> tries to obtain a reference for every vCPU.
>>>>
>>>> AFAIU, this would be a reference of the **count_info** not **type_info**
>>>> (which BTW will never be incremented on Arm because we have no type
>>>> support).
>>>
>>> I should have said "obtain a writable type reference".
>>
>> Thanks for the clarification.
>>
>>>
>>>> The commit message is only referring to the 'type_info's count'. So...
>>>>
>>>>> With my adding of GFN
>>>>> (really gaddr) based registration of the runstate area (already
>>>>> looking towards 4.18) the maximum possible count is to further grow.
>>>>
>>>> ... I am not sure which problem you are referring too.
>>>
>>> Wow - a mere stub (but not inline) function to make the build happy.
>>> Then why is the description talking about one bit that's needed on
>>> Arm?
>>
>> Because share_xen_page_with_guest() will always set the type info's
>> count to 1.
>>
>> TBH I don't exactly know why we set it. I always assumed this was a
>> requirement for the common code but never checked.
> 
> So my first thought was that this type-ref handling all being no-ops
> would be an issue with gnttab v2, but besides that not being security
> supported on Arm the code also passes SHARE_rw (for a reason that
> escapes me) when sharing the status pages.

Probably because grant-table v2 was never tested on Arm.

> 
> It does however mean that Dom0 can map the trace buffers r/w (unless
> there's some special code in Arm preventing that), despite them being
> shared with SHARE_ro. Not a big problem considering all the power Dom0
> has, but still against the intentions.

We don't use the refcounting but still use the flag PGT_writable_page to 
indicate whether the mapping is writeable or read-only.

The code to map the trace buffers will look at the flag and decide the 
attribute in the P2M.

Cheers,

-- 
Julien Grall