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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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.
> > > > > @@ -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.
Thanks! I still owe you a review for the rest of the series.

Cheers,

--
Julien Grall