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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 )").

>>>> @@ -1573,6 +1648,22 @@ int map_guest_area(struct vcpu *v, paddr
>>>>     */
>>>>    void unmap_guest_area(struct vcpu *v, struct guest_area *area)
>>>>    {
>>>> +    struct domain *d = v->domain;
>>>> +    void *map;
>>>> +    struct page_info *pg;
>>>
>>> AFAIU, the assumption is the vCPU should be paused here.
>>
>> Yes, as the comment ahead of the function (introduced by an earlier
>> patch) says.
> 
> Ah, I missed that. Thanks!
> 
>>
>>> Should we add an ASSERT()?
>>
>> I was going from unmap_vcpu_info(), which had the same requirement,
>> while also only recording it by way of a comment. I certainly could
>> add an ASSERT(), but besides this being questionable as to the rules
>> set forth in ./CODING_STYLE I also view assertions of "paused" state
>> as being of limited use - the entity in question may become unpaused
>> on the clock cycle after the check was done. 
> 
> That's correct. However, that race you mention is unlikely to happen 
> *every* time. So there are a very high chance the ASSERT() will hit if 
> miscalled.
> 
>> (But yes, such are no
>> different from e.g. the fair number of spin_is_locked() checks we
>> have scattered around, which don't really provide guarantees either.)
> And that's fine to not provide the full guarantee. You are still 
> probably going to catch 95% (if not more) of the callers that forgot to 
> call it with the spin lock held.
> 
> At least for me, those ASSERTs() were super helpful during development 
> in more than a few cases.

Okay, I'll add one then, but in the earlier patch, matching the comment
that's introduced there.

Jan