Re: [PATCH v2 1/3] x86/mem_sharing: option to enforce fork starting with empty p2m

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.03.2022 16:03, Tamas K Lengyel wrote:
> Add option to the fork memop to enforce a start with an empty p2m.
> Pre-populating special pages to the fork tend to be necessary only when 
> setting
> up forks to be fully functional with a toolstack or if the fork makes use of
> them in some way. For short-lived forks these pages are optional and starting
> with an empty p2m has advantages both in terms of reset performance as well as
> easier reasoning about the state of the fork after creation.

I'm afraid I don't consider this enough of an explanation: Why would these
page be optional? Where does the apriori knowledge come from that the guest
wouldn't manage to access the vCPU info pages or the APIC access one?

> --- a/xen/arch/x86/include/asm/hvm/domain.h
> +++ b/xen/arch/x86/include/asm/hvm/domain.h
> @@ -31,7 +31,9 @@
>  #ifdef CONFIG_MEM_SHARING
>  struct mem_sharing_domain
>  {
> -    bool enabled, block_interrupts;
> +    bool enabled;
> +    bool block_interrupts;
> +    bool empty_p2m;

While the name of the field is perhaps fine as is, it would be helpful to
have a comment here clarifying that this is only about the guest's initial
and reset state; this specifically does not indicate the p2m has to remain
empty (aiui).

> @@ -1856,7 +1860,13 @@ static int fork(struct domain *cd, struct domain *d)
>      if ( (rc = bring_up_vcpus(cd, d)) )
>          goto done;
>  
> -    rc = copy_settings(cd, d);
> +    if ( !(rc = copy_settings(cd, d, empty_p2m)) )
> +    {
> +        cd->arch.hvm.mem_sharing.block_interrupts = block_interrupts;
> +
> +        if ( (cd->arch.hvm.mem_sharing.empty_p2m = empty_p2m) )

Is there a reason you don't do the assignment earlier, thus avoiding the
need to pass around the extra function argument?

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -543,10 +543,10 @@ struct xen_mem_sharing_op {
>          } debug;
>          struct mem_sharing_op_fork {      /* OP_FORK */
>              domid_t parent_domain;        /* IN: parent's domain id */
> -/* Only makes sense for short-lived forks */
> +/* These flags only makes sense for short-lived forks */

Nit: s/makes/make/.

Jan