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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan