Re: [PATCH 2/2] x86/mem_sharing: Add extra variable to track fork progress

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Feb 15, 2023 at 6:03 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 14.02.2023 06:07, Tamas K Lengyel wrote:
> > When VM forking is initiated a VM is not supposed to try to perform mem_sharing
> > yet as the fork process hasn't completed all required steps. However, the vCPU
> > bring-up paths trigger guest memory accesses that can lead to such premature
> > sharing ops. However, the gating check to see whether a VM is a fork is set
> > already (ie. the domain has a parent).
>
> I find this confusing, and I had to read the patch to understand what's
> meant. Since there are a total of three VMs involved here (the one
> driving the fork, the one being forked, and the new clone), "a VM" is
> insufficient. The sentence further reads as if that VM is performing
> sharing operations on itself, which according to my understanding is
> impossible.
>
> Furthermore "the vCPU bringup paths" is also not specific enough imo.
> The forked VM as well as the new clone are both paused if I'm not
> mistaken, so neither can be in the process of bringing up any vCPU-s.
> I assume you refer to bring_up_vcpus(), but I'm afraid I view this
> function as misnamed. vCPU-s are being _created_ there, not brought up.
> Furthermore there are no guest memory accesses from underneath
> bring_up_vcpus(), so with those accesses you may be referring to
> copy_settings() instead? Which in turn - I'm sorry for my ignorance -
> raises the question why (implicit) hypervisor side accesses to guest
> memory might trigger sharing: So far I was working from the assumption
> that it's only control tool requests which do. Otoh you talk of
> "sharing ops", which suggests it may indeed be requests coming from
> the control tool. Yet that's also what invoked the request to fork,
> so shouldn't it coordinate with itself and avoid issuing sharing ops

> prematurely?

I went back to double-check and here is the memory access during vcpu_create:

(XEN) Xen call trace:
(XEN)    [<ffff82d0402ebf38>] R mem_sharing.c#mem_sharing_gfn_alloc+0x5c/0x5e
(XEN)    [<ffff82d0402ecb12>] F mem_sharing.c#add_to_physmap+0x175/0x233
(XEN)    [<ffff82d0402ee81d>] F mem_sharing_fork_page+0x4ee/0x51e
(XEN)    [<ffff82d0402f244f>] F p2m_get_gfn_type_access+0x119/0x1a7
(XEN)    [<ffff82d0402e67ef>] F hap.c#hap_update_paging_modes+0xbe/0x2ee
(XEN)    [<ffff82d0402942a0>] F vmx_create_vmcs+0x981/0xb71
(XEN)    [<ffff82d040298884>] F vmx.c#vmx_vcpu_initialise+0x64/0x1a0
(XEN)    [<ffff82d0402acc59>] F hvm_vcpu_initialise+0x97/0x19e
(XEN)    [<ffff82d0403168db>] F arch_vcpu_create+0xf3/0x1db
(XEN)    [<ffff82d040206c69>] F vcpu_create+0x211/0x35d
(XEN)    [<ffff82d0402f00b7>] F mem_sharing_memop+0x16a9/0x1869
(XEN)    [<ffff82d0403317c5>] F subarch_memory_op+0x298/0x2c4
(XEN)    [<ffff82d04032ca26>] F arch_memory_op+0xa9f/0xaa4
(XEN)    [<ffff82d040221e66>] F do_memory_op+0x2150/0x2297
(XEN)    [<ffff82d04030bcfb>] F pv_do_multicall_call+0x22c/0x4c7
(XEN)    [<ffff82d040319727>] F arch_do_multicall_call+0x23/0x2c
(XEN)    [<ffff82d04022231f>] F do_multicall+0xd3/0x417
(XEN)    [<ffff82d04030c0e4>] F pv_hypercall+0xea/0x3a0
(XEN)    [<ffff82d040201292>] F lstar_enter+0x122/0x130

Any time any page is requested in a fork that's not present it gets populated from the parent (if the parent has it). What I was worried about is nominate_page being called on the fork but I was mistaken, it was called on the parent before add_to_physmap is called on the fork - which is fine and the expected behavior.

We can drop this patch, sorry for the noise.

Tamas