Re: [Xen-devel] [PATCH v12 1/3] xen/mem_sharing: VM forking

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 26, 2020 at 08:07:09AM +0100, Jan Beulich wrote:
> On 25.03.2020 16:47, Roger Pau Monné wrote:
> > On Mon, Mar 23, 2020 at 10:04:35AM -0700, Tamas K Lengyel wrote:
> >> +static int copy_vcpu_settings(struct domain *cd, struct domain *d)
> >> +{
> >> +    unsigned int i;
> >> +    struct p2m_domain *p2m = p2m_get_hostp2m(cd);
> >> +    int ret = -EINVAL;
> >> +
> >> +    for ( i = 0; i < cd->max_vcpus; i++ )
> >> +    {
> >> +        const struct vcpu *d_vcpu = d->vcpu[i];
> >> +        struct vcpu *cd_vcpu = cd->vcpu[i];
> >> +        struct vcpu_runstate_info runstate;
> >> +        mfn_t vcpu_info_mfn;
> >> +
> >> +        if ( !d_vcpu || !cd_vcpu )
> >> +            continue;
> >> +
> >> +        /*
> >> +         * Copy & map in the vcpu_info page if the guest uses one
> >> +         */
> >> +        vcpu_info_mfn = d_vcpu->vcpu_info_mfn;
> >> +        if ( !mfn_eq(vcpu_info_mfn, INVALID_MFN) )
> >> +        {
> >> +            mfn_t new_vcpu_info_mfn = cd_vcpu->vcpu_info_mfn;
> >> +
> >> +            /*
> >> +             * Allocate & map the page for it if it hasn't been already
> >> +             */
> >> +            if ( mfn_eq(new_vcpu_info_mfn, INVALID_MFN) )
> >> +            {
> >> +                gfn_t gfn = mfn_to_gfn(d, vcpu_info_mfn);
> >> +                unsigned long gfn_l = gfn_x(gfn);
> >> +                struct page_info *page;
> >> +
> >> +                if ( !(page = alloc_domheap_page(cd, 0)) )
> >> +                    return -ENOMEM;
> >> +
> >> +                new_vcpu_info_mfn = page_to_mfn(page);
> >> +                set_gpfn_from_mfn(mfn_x(new_vcpu_info_mfn), gfn_l);
> >> +
> >> +                ret = p2m->set_entry(p2m, gfn, new_vcpu_info_mfn, 
> >> PAGE_ORDER_4K,
> >> +                                     p2m_ram_rw, p2m->default_access, -1);
> >> +                if ( ret )
> >> +                    return ret;
> >> +
> >> +                ret = map_vcpu_info(cd_vcpu, gfn_l,
> >> +                                    d_vcpu->vcpu_info_offset);
> >> +                if ( ret )
> >> +                    return ret;
> >> +            }
> >> +
> >> +            copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn);
> >> +        }
> >> +
> >> +        /*
> >> +         * Setup the vCPU runstate area
> >> +         */
> >> +        if ( guest_handle_is_null(runstate_guest(cd_vcpu)) )
> > 
> > Maybe I'm confused, but isn't this the other way around and you need
> > to check? If the parent runstate is not null copy it to the fork,
> > ie:
> > 
> > if ( !guest_handle_is_null(runstate_guest(d_vcpu)) )
> > {
> >     ...
> > 
> >> +        {
> >> +            runstate_guest(cd_vcpu) = runstate_guest(d_vcpu);
> >> +            vcpu_runstate_get(cd_vcpu, &runstate);
> >> +            __copy_to_guest(runstate_guest(cd_vcpu), &runstate, 1);
> > 
> > You should check the return code I think.
> 
> I don't think so - this is a best effort operation just like e.g.
> in the handling of VCPUOP_register_runstate_memory_area.

I think printing a debug message might be helpful, not so much as for
the importance of failing to copy the runstate area, but it could
signal that something went wrong, anyway I don't have such a strong
opinion.

Just to confirm, __copy_to_guest will cause the forked domain memory
to be populated and the whole page to be copied over right? (and will
also cause the page tables to be added to the fork physmap in write
mode to set the accessed/dirty bits)

Thanks, Roger.