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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

> > +    if ( (ret = cpupool_move_domain(cd, d->cpupool)) )
> > +        return ret;
>
> You can join both ifs into a single one, since both return ret.

Sure.

> > +
> > +    for ( i = 0; i < cd->max_vcpus; i++ )
> > +    {
> > +        mfn_t vcpu_info_mfn;
> > +
> > +        if ( !d->vcpu[i] || cd->vcpu[i] )
> > +            continue;
> > +
> > +        if ( !vcpu_create(cd, i) )
> > +            return -EINVAL;
> > +
> > +        /*
> > +         * Map in a page for the vcpu_info if the guest uses one to the 
> > exact
> > +         * same spot.
> > +         */
> > +        vcpu_info_mfn = d->vcpu[i]->vcpu_info_mfn;
> > +        if ( !mfn_eq(vcpu_info_mfn, INVALID_MFN) )
> > +        {
> > +            struct page_info *page;
> > +            mfn_t new_mfn;
> > +            gfn_t gfn = mfn_to_gfn(d, vcpu_info_mfn);
> > +            unsigned long gfn_l = gfn_x(gfn);
> > +
> > +            if ( !(page = alloc_domheap_page(cd, 0)) )
> > +                return -ENOMEM;
> > +
> > +            new_mfn = page_to_mfn(page);
> > +            set_gpfn_from_mfn(mfn_x(new_mfn), gfn_l);
> > +
> > +            if ( !(ret = p2m->set_entry(p2m, gfn, new_mfn, PAGE_ORDER_4K,
> > +                                        p2m_ram_rw, p2m->default_access, 
> > -1)) )
> > +                return ret;
> > +
> > +            if ( !(ret = map_vcpu_info(cd->vcpu[i], gfn_l,
> > +                                       d->vcpu[i]->vcpu_info_offset)) )
> > +                return ret;
>
> I think you also need to copy the contents from the parent into those
> vcpu_info areas, or else you might discard pending event channels
> contained in the evtchn_* fields? (and the masked channels if any).
>
> The runtime area should be handled in a similar way AFAICT (albeit
> there's no need to copy the parent's data in that case), see
> VCPUOP_register_runstate_memory_area.

Will do.


> > +static int populate_special_pages(struct domain *cd)
> > +{
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(cd);
> > +    static const unsigned int params[] =
> > +    {
> > +        HVM_PARAM_STORE_PFN,
> > +        HVM_PARAM_IOREQ_PFN,
> > +        HVM_PARAM_BUFIOREQ_PFN,
> > +        HVM_PARAM_CONSOLE_PFN
> > +    };
> > +    unsigned int i;
> > +
> > +    for ( i=0; i<4; i++ )
>
> Nit: can you please add some spaces around the operators?

Sure.

>
> > +    {
> > +        uint64_t value = 0;
> > +        mfn_t new_mfn;
> > +        struct page_info *page;
> > +
> > +        if ( hvm_get_param(cd, params[i], &value) || !value )
> > +            continue;
> > +
> > +        if ( !(page = alloc_domheap_page(cd, 0)) )
> > +            return -ENOMEM;
> > +
> > +        new_mfn = page_to_mfn(page);
> > +        set_gpfn_from_mfn(mfn_x(new_mfn), value);
> > +
> > +        return p2m->set_entry(p2m, _gfn(value), new_mfn, PAGE_ORDER_4K,
> > +                              p2m_ram_rw, p2m->default_access, -1);
>
> I think you also need to copy the contents from the parent page here.

The toolstack simply clears these pages during restore so I'm not sure
(see 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxc/xc_sr_restore_x86_hvm.c;h=3f78248f32fec239db77b0e483b0195211e6b974;hb=HEAD#l61).
I don't see why you would have to clear the pages first if they get
overwritten by saved versions later. Or these pages are expected to be
torn-down by the save/restore aware guests?

> > +static int fork(struct domain *d, struct domain *cd)
> > +{
> > +    int rc = -EBUSY;
> > +
> > +    if ( !cd->controller_pause_count )
> > +        return rc;
> > +
> > +    /*
> > +     * We only want to get and pause the parent once, not each time this
> > +     * operation is restarted due to preemption.
> > +     */
> > +    if ( !cd->parent_paused )
> > +    {
> > +        if ( !get_domain(d) )
> > +        {
> > +            ASSERT_UNREACHABLE();
> > +            return -EBUSY;
> > +        }
> > +
> > +        domain_pause(d);
> > +        cd->parent_paused = true;
> > +        cd->max_pages = d->max_pages;
> > +        cd->max_vcpus = d->max_vcpus;
> > +    }
> > +
> > +    /* this is preemptible so it's the first to get done */
> > +    if ( (rc = fork_hap_allocation(cd, d)) )
> > +        goto done;
> > +
> > +    if ( (rc = bring_up_vcpus(cd, d)) )
> > +        goto done;
> > +
> > +    if ( (rc = hvm_copy_context_and_params(cd, d)) )
> > +        goto done;
> > +
> > +    if ( (rc = populate_special_pages(cd)) )
> > +        goto done;
> > +
> > +    fork_tsc(cd, d);
>
> I think you need to copy the contents of the shared info page from the
> parent into the child, or else you are discarding any pending event
> channels. You should also map such shared info page into the same gfn
> as the parent.
>

I'll look into it, thanks!

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel