Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas

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

On Wed, Sep 27, 2023 at 07:43:26AM -0400, Tamas K Lengyel wrote:
> On Wed, Sep 27, 2023 at 7:08 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Wed, May 03, 2023 at 05:56:46PM +0200, Jan Beulich wrote:
> > > 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>
> >
> > Given the very limited and specific usage of the current Xen forking
> > code, do we really need to bother about copying such areas?
> >
> > IOW: I doubt that not updating the runstate/time areas will make any
> > difference to the forking code ATM.
> 
> The current implementation of forking allows for fully functional VM
> forks to be setup, including launching the dm for them. The toolstack
> side hasn't been merged for that into Xen but that doesn't mean it's
> not available or used already. So any internal changes to Xen that
> create guest-states that could potentially be interacted with and
> relied on by a guest should get properly wired in for forking. So I'm
> happy Jan took the initiative here for wiring this up, even if the
> use-case seems niche.

But there are still some bits missing in Xen, seeing the comment in
copy_vcpu_settings().  If we don't copy the timers already then I'm
unsure whether copying the runstate/time shared areas is very
relevant.

> >
> > > ---
> > > v3: Extend comment.
> > >
> > > --- a/xen/arch/x86/mm/mem_sharing.c
> > > +++ b/xen/arch/x86/mm/mem_sharing.c
> > > @@ -1641,6 +1641,68 @@ static void copy_vcpu_nonreg_state(struc
> > >      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)
> > > +{
> > > +    mfn_t d_mfn, cd_mfn;
> > > +
> > > +    if ( !d_area->pg )
> > > +        return 0;
> > > +
> > > +    d_mfn = page_to_mfn(d_area->pg);
> > > +
> > > +    /* Allocate & map a page for the area if it hasn't been already. */
> > > +    if ( !cd_area->pg )
> > > +    {
> > > +        gfn_t gfn = mfn_to_gfn(d, d_mfn);
> > > +        struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
> > > +        p2m_type_t p2mt;
> > > +        p2m_access_t p2ma;
> > > +        unsigned int offset;
> > > +        int ret;
> > > +
> > > +        cd_mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL, NULL);
> > > +        if ( mfn_eq(cd_mfn, INVALID_MFN) )
> > > +        {
> > > +            struct page_info *pg = alloc_domheap_page(cd_vcpu->domain, 0);
> > > +
> > > +            if ( !pg )
> > > +                return -ENOMEM;
> > > +
> > > +            cd_mfn = page_to_mfn(pg);
> > > +            set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> > > +
> > > +            ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K, p2m_ram_rw,
> > > +                                 p2m->default_access, -1);
> > > +            if ( ret )
> > > +                return ret;
> > > +        }
> > > +        else if ( p2mt != p2m_ram_rw )
> > > +            return -EBUSY;
> >
> > Shouldn't the populate of the underlying gfn in the fork case
> > be done by map_guest_area itself?
> 
> Would seem more logical, I agree, but then the call should be
> map_guest_area first, which conditionally calls down into a
> mem_sharing_copy_guest_area if the domain is a fork.
> 
> >
> > What if a forked guest attempts to register a new runstate/time info
> > against an address not yet populated?
> 
> Unpopulated memory will get forked from the parent for all access
> paths currently in existence, including access to a forked VMs memory
> from dom0/dm and the various internal Xen access paths. If the memory
> range is not mapped in the parent registering on that range ought to
> fail by I assume existing checks that validate that the memory is
> present during registration.

What I'm trying to say is that map_guest_area() already has to deal
with forked guests, and hence the populating of the gfn shouldn't be
needed as map_guest_area() must know how to deal with such guest
anyway.

Thanks, Roger.