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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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).
> > > +    {
> > > +        unsigned long gfn = PFN_DOWN(gaddr);
> > This could be gfn_t for adding some type safety.
> Indeed I did consider doing so, but the resulting code would imo be
> less legible. But this difference perhaps isn't significant enough
> for me to object to changing, in case you (or others) think the
> type safety is really a meaningful gain here.
In general, my preference is to always use the typesafe version because 
it reduces the number of "unsigned long".
[...]

> > The first function will set v->is_running to false (see
> > vcpu_context_saved()). So I think the "area" could be touched even afte
> > vcpu_pause() is returned.
> >
> > Therefore, I think we will need _update_runstate_area() (or
> > update_runstate_area()) to be called first.
> ... I don't see a need for adjustment. The corresponding
>
>      _update_runstate_area(prev);
>
> sits quite a bit earlier in context_switch(). (Arm code is quite a bit
> different, but this particular aspect looks to apply there as well.)
You are right. Sorry I misread the code.

> > > @@ -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.
Cheers,

--
Julien Grall