Re: [PATCH v4 5/9] x86/mem-sharing: copy GADDR based shared guest areas

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.09.2023 14:57, Tamas K Lengyel wrote:
> On Thu, Sep 28, 2023 at 7:08 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>> On Thu, Sep 28, 2023 at 12:11:02PM +0200, Jan Beulich wrote:
>>> On 28.09.2023 11:51, Roger Pau Monné wrote:
>>>> On Thu, Sep 28, 2023 at 09:16:20AM +0200, Jan Beulich wrote:
>>>>> +        /*
>>>>> +         * 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.
>>
>> Yes, it's already an issue in the current code.  I wonder whether
>> logic in the guest or Xen could malfunctions due to the fact that
>> map_vcpu_info() unconditionally sets evtchn_upcall_pending and injects
>> an event channel upcall, but the later call to copy_domain_page()
>> might unset evtchn_upcall_pending while the vector is already injected.
> 
> Sorry but I really don't follow the discussion here. My understanding
> was that map_vcpu_info, as its name suggests, maps the page. We use it
> to map a new page into that position in case the fork hasn't set it up
> yet but the parent has one. Then we follow with the copy from the
> parent so the page content is matching. If there is already a
> vcpu_info page in the fork, we just do the copy.
> 
> Now, if map_vcpu_info does more then mapping, then I don't know what
> it does, why it does it, and what happens if we skip it when the fork
> is reset for example. Is the suggestion to call it map_vcpu_info every
> time the page content is reset (ie after the copy)?

The vCPU info area (already prior to this series) and the two other areas
can be updated by the hypervisor at any time. Once one such area is
registered within a certain page, if another such area happens to live in
the same page, copying the entire page again would overwrite all updates
that might already have been made for the first area. IOW copying ought
to - imo - happen exactly once, when the new page is allocated.

As to map_vcpu_info() - just look at the function: It writes to the newly
registered area. Even if the function name says just "map", that's an
integral part of the operation. We can't just map it, but leave the area
untouched.

Jan