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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> --- a/xen/arch/x86/include/asm/hvm/domain.h
> +++ b/xen/arch/x86/include/asm/hvm/domain.h
> @@ -33,6 +33,14 @@ struct mem_sharing_domain
>  {
>      bool enabled, block_interrupts;
>  
> +    /*
> +     * We need to avoid trying to nominate pages for forking until the
> +     * fork operation is completely finished. As parts of fork creation
> +     * is restartable we mark here if the process started to skip the
> +     * non-restartable portion.
> +     */
> +    bool fork_started;

"non-restartable portion" reads incorrect to me: The issue there is with
that portion being one-time initialization. I think this should also be
said this way. Furthermore (nit) it wants to be either "parts" and "are"
or "part" and "is", and I think a comma after "started" would help
readability (or else it can be read as "the process started to skip
<whatever>"). Finally maybe better "... this flag indicates that ..."
(in particular because we definitely do not mark here, but in fork()).

> @@ -1905,7 +1906,7 @@ static int fork(struct domain *cd, struct domain *d)
>          *cd->arch.cpuid = *d->arch.cpuid;
>          *cd->arch.msr = *d->arch.msr;
>          cd->vmtrace_size = d->vmtrace_size;
> -        cd->parent = d;
> +        msd->fork_started = 1;

"true" please (and "false" below).

Jan