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 11:48 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 26.01.2023 16:41, Tamas K Lengyel wrote:
> > 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.
>
> I'm sorry, but what does "please do" mean when I asked for improvement
> suggestions? I continue to think the comment is quite clear as is, so
> if anything needs adjusting, I'd need to know pretty precisely what it
> is that needs adding and/or re-writing. I can't, after all, guess what
> your misunderstanding resulted from.

I meant please do add the extra information you just spelled out in your previous email. "Map the page into the guest. We pass the entire range to map_guest_area so that it can check that no cross-page mapping is taking place (in which case it will fail). If no such issue is present the page is being mapped and made accessible to the guest."

If that's not what the function is doing, please explain clearly what it will do.

Tamas