Re: [PATCH v5 06/10] x86/mem-sharing: copy GADDR based shared guest areas

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Oct 03, 2023 at 09:53:11AM -0400, Tamas K Lengyel wrote:
> On Mon, Oct 2, 2023 at 11:13 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
> >
> > From: Jan Beulich <jbeulich@suse.com>
> >
> > In preparation of the introduction of new vCPU operations allowing to
> > register the respective areas (one of the two is x86-specific) by
> > guest-physical address, add the necessary fork handling (with the
> > backing function yet to be filled in).
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v4:
> >  - Rely on map_guest_area() to populate the child p2m if necessary.
> > ---
> >  xen/arch/x86/mm/mem_sharing.c | 31 +++++++++++++++++++++++++++++++
> >  xen/common/domain.c           |  7 +++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index 5f8f1fb4d871..99cf001fd70f 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1641,6 +1641,24 @@ static void copy_vcpu_nonreg_state(struct vcpu *d_vcpu, struct vcpu *cd_vcpu)
> >      hvm_set_nonreg_state(cd_vcpu, &nrs);
> >  }
> >
> > +static int copy_guest_area(struct guest_area *cd_area,
> > +                           const struct guest_area *d_area,
> > +                           struct vcpu *cd_vcpu,
> > +                           const struct domain *d)
> > +{
> > +    unsigned int offset;
> > +
> > +    /* Check if no area to map, or already mapped. */
> > +    if ( !d_area->pg || cd_area->pg )
> > +        return 0;
> > +
> > +    offset = PAGE_OFFSET(d_area->map);
> > +    return map_guest_area(cd_vcpu, gfn_to_gaddr(
> > +                                       mfn_to_gfn(d, page_to_mfn(d_area->pg))) +
> > +                                   offset,
> > +                          PAGE_SIZE - offset, cd_area, NULL);
> > +}
> > +
> >  static int copy_vpmu(struct vcpu *d_vcpu, struct vcpu *cd_vcpu)
> >  {
> >      struct vpmu_struct *d_vpmu = vcpu_vpmu(d_vcpu);
> > @@ -1709,6 +1727,16 @@ static int copy_vcpu_settings(struct domain *cd, const struct domain *d)
> >                  return ret;
> >          }
> >
> > +        /* Same for the (physically registered) runstate and time info areas. */
> > +        ret = copy_guest_area(&cd_vcpu->runstate_guest_area,
> > +                              &d_vcpu->runstate_guest_area, cd_vcpu, d);
> > +        if ( ret )
> > +            return ret;
> > +        ret = copy_guest_area(&cd_vcpu->arch.time_guest_area,
> > +                              &d_vcpu->arch.time_guest_area, cd_vcpu, d);
> > +        if ( ret )
> > +            return ret;
> > +
> >          ret = copy_vpmu(d_vcpu, cd_vcpu);
> >          if ( ret )
> >              return ret;
> > @@ -1950,7 +1978,10 @@ int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> >
> >   state:
> >      if ( reset_state )
> > +    {
> >          rc = copy_settings(d, pd);
> > +        /* TBD: What to do here with -ERESTART? */
> 
> There is no situation where we get an -ERESTART here currently. Is
> map_guest_area expected to run into situations where it fails with
> that rc?

Yes, there's a spin_trylock() call that will result in
map_guest_area() returning -ERESTART.

> If yes we might need a lock in place so we can block until it
> can succeed.

I'm not sure whether returning -ERESTART can actually happen in
map_guest_area() for the fork case: the child domain is still paused
at this point, so there can't be concurrent guest hypercalls that
would also cause the domain hypercall_deadlock_mutex to be acquired.

The comment was added by Jan, so I cannot be certain about the
intention, neither I would like to misinterpret his words.  My
understanding is that future uses of copy_settings() might indeed need
to report -ERESTART, and that it would need to be propagated for
proper hypercall continuations at some point.

Thanks, Roger.