On 28.09.2023 11:51, Roger Pau Monné wrote:
> On Thu, Sep 28, 2023 at 09:16:20AM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -1641,6 +1641,68 @@ 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;
>> + }
>
> I'm still unsure why map_guest_area() shouldn't be able to deal with a
> forked child needing the page to be mapped. What happens when a
> forked child executes the hypercall to map such areas against not yet
> populated gfns?
>
> Shouldn't map_guest_area() be capable of handling those calls and
> populating on demand?
Funny you should use this wording: P2M_ALLOC will deal with populating
PoD slots, yes, but aiui forks don't fill their p2m with all PoD slots,
but rather leave them empty. Yet again I need to leave it to Tamas to
confirm or prove me wrong.
>> + else if ( p2mt != p2m_ram_rw )
>> + return -EBUSY;
>> +
>> + /*
>> + * Map the area into the guest. For simplicity specify the entire
>> range
>> + * up to the end of the page: All the function uses it for is to
>> check
>> + * that the range doesn't cross page boundaries. Having the area
>> mapped
>> + * in the original domain implies that it fits there and therefore
>> will
>> + * also fit in the clone.
>> + */
>> + 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);
>> +
>> + copy_domain_page(cd_mfn, d_mfn);
>
> I think the page copy should be done only once, when the page is
> populated on the child p2m. Otherwise areas smaller than a page size
> (like vpcu_time_info_t) that share the same page will get multiple
> copies of the same data for no reason.
I think you're right, but this would then be another issue in the original
code that I merely didn't spot (and it's not just "copy for no reason",
we'd actually corrupt what was put there before). IOW the copying needs to
move ahead of map_guest_area() (or yet more precisely after the error
checking for p2m->set_entry()), and in the original code it would have
needed to live ahead of map_vcpu_info(). Once again I'd like Tamas to
confirm (or otherwise) before making that change, though.
Jan
