On Thu, Jan 26, 2023 at 3:14 AM Jan Beulich <[email protected]> wrote: > > On 25.01.2023 16:34, Tamas K Lengyel wrote: > > On Tue, Jan 24, 2023 at 6:19 AM Jan Beulich <[email protected]> wrote: > >> > >> On 23.01.2023 19:32, Tamas K Lengyel wrote: > >>> On Mon, Jan 23, 2023 at 11:24 AM Jan Beulich <[email protected]> wrote: > >>>> On 23.01.2023 17:09, Tamas K Lengyel wrote: > >>>>> On Mon, Jan 23, 2023 at 9:55 AM Jan Beulich <[email protected]> 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
