Re: [PATCH RFC 07/10] domain: map/unmap GADDR based shared guest areas

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.12.2022 19:27, Julien Grall wrote:
> On 29/11/2022 14:02, Jan Beulich wrote:
>> On 29.11.2022 09:40, Julien Grall wrote:
>>> On 28/11/2022 10:01, Jan Beulich wrote:
>>>> On 24.11.2022 22:29, Julien Grall wrote:
>>>>> On 19/10/2022 09:43, Jan Beulich wrote:
>>>>>> --- a/xen/common/domain.c
>>>>>> +++ b/xen/common/domain.c
>>>>>> @@ -1563,7 +1563,82 @@ int map_guest_area(struct vcpu *v, paddr
>>>>>>                        struct guest_area *area,
>>>>>>                        void (*populate)(void *dst, struct vcpu *v))
>>>>>>     {
>>>>>> -    return -EOPNOTSUPP;
>>>>>> +    struct domain *currd = v->domain;
>>>>>> +    void *map = NULL;
>>>>>> +    struct page_info *pg = NULL;
>>>>>> +    int rc = 0;
>>>>>> +
>>>>>> +    if ( gaddr )
>>>>>
>>>>> 0 is technically a valid (guest) physical address on Arm.
>>>>
>>>> I guess it is everywhere; it certainly also is on x86. While perhaps a
>>>> little unfortunate in ordering, the public header changes coming only
>>>> in the following patches was the best way I could think of to split
>>>> this work into reasonable size pieces. There the special meaning of 0
>>>> is clearly documented. And I don't really see it as a meaningful
>>>> limitation to not allow guests to register such areas at address zero.
>>> I would expect an OS to allocate the region using the generic physical
>>> allocator. This allocator may decide that '0' is a valid address and
>>> return it.
>>>
>>> So I think your approach could potentially complicate the OS
>>> implementation. I think it would be better to use an all Fs value as
>>> this cannot be valid for this hypercall (we require at least 4-byte
>>> alignment).
>>
>> Valid point, yet my avoiding of an all-Fs value was specifically with
>> compat callers in mind - the values would be different for these and
>> native ones, which would make the check more clumsy (otherwise it
>> could simply be "if ( ~gaddr )").
> 
> Ah I forgot about compat. How about converting the 32-bit Fs to a 64-bit 
> Fs in the compat code?
> 
> This will avoid to add restriction in the hypercall interface just 
> because of compat.

Possible, but ugly: Since we're using the uint64_t field of the interface
structure, no translation is presently necessary for
VCPUOP_register_vcpu_time_phys_area (we do have the layer for
VCPUOP_register_runstate_phys_area, because of the size of the pointed
to area being different). Hence what you ask for would mean adding compat
code somewhere. In which case we could as well make the above check
itself depend on whether we're dealing with a compat domain. Which is
what I've named "clumsy" above; still that would seem better to me than
to add actual compat translation code.

Then again, looking at the last patch, switching to what you suggest
would (largely) address one of the RFC remarks there as well. So I guess
I'll make the change to that if() plus associated adjustments elsewhere
(in particular in the last patch there is a similar check which needs to
remain in sync).

Jan