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
