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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jan 26, 2023 at 3:14 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 25.01.2023 16:34, Tamas K Lengyel wrote:
> > On Tue, Jan 24, 2023 at 6:19 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 23.01.2023 19:32, Tamas K Lengyel wrote:
> >>> On Mon, Jan 23, 2023 at 11:24 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>> On 23.01.2023 17:09, Tamas K Lengyel wrote:
> >>>>> On Mon, Jan 23, 2023 at 9:55 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>>>>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>>>>> @@ -1653,6 +1653,65 @@ 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;
> >>>>>> +
> >>>>>> +        /*
> >>>>>> +         * Simply specify the entire range up to the end of the
> > page.
> >>>>> All the
> >>>>>> +         * function uses it for is a check for not crossing page
> >>>>> boundaries.
> >>>>>> +         */
> >>>>>> +        offset = PAGE_OFFSET(d_area->map);
> >>>>>> +        ret = map_guest_area(cd_vcpu, gfn_to_gaddr(gfn) + offset,
> >>>>>> +                             PAGE_SIZE - offset, cd_area, NULL);
> >>>>>> +        if ( ret )
> >>>>>> +            return ret;
> >>>>>> +    }
> >>>>>> +    else
> >>>>>> +        cd_mfn = page_to_mfn(cd_area->pg);
> >>>>>
> >>>>> Everything to this point seems to be non mem-sharing/forking related.
> >>> Could
> >>>>> these live somewhere else? There must be some other place where
> >>> allocating
> >>>>> these areas happens already for non-fork VMs so it would make sense to
> >>> just
> >>>>> refactor that code to be callable from here.
> >>>>
> >>>> It is the "copy" aspect with makes this mem-sharing (or really fork)
> >>>> specific. Plus in the end this is no different from what you have
> >>>> there right now for copying the vCPU info area. In the final patch
> >>>> that other code gets removed by re-using the code here.
> >>>
> >>> Yes, the copy part is fork-specific. Arguably if there was a way to do
> > the
> >>> allocation of the page for vcpu_info I would prefer that being
> > elsewhere,
> >>> but while the only requirement is allocate-page and copy from parent
> > I'm OK
> >>> with that logic being in here because it's really straight forward. But
> > now
> >>> you also do extra sanity checks here which are harder to comprehend in
> > this
> >>> context alone.
> >>
> >> What sanity checks are you talking about (also below, where you claim
> >> map_guest_area() would be used only to sanity check)?
> >
> > Did I misread your comment above "All the function uses it for is a check
> > for not crossing page boundaries"? That sounds to me like a simple sanity
> > check, unclear why it matters though and why only for forks.
>
> The comment is about the function's use of the range it is being passed.
> It doesn't say in any way that the function is doing only sanity checking.
> If the comment wording is ambiguous or unclear, I'm happy to take
> improvement suggestions.

Yes, please do, it definitely was confusing while reviewing the patch.

Thanks,

Tamas