Re: [PATCH v2 3/3] x86/mem_sharing: make fork_reset more configurable

[Jan Beulich <jbeulich@suse.com>]

On 29.03.2022 16:03, Tamas K Lengyel wrote:
> --- a/xen/arch/x86/include/asm/mem_sharing.h
> +++ b/xen/arch/x86/include/asm/mem_sharing.h
> @@ -85,6 +85,9 @@ static inline bool mem_sharing_is_fork(const struct domain *d)
>  int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
>                            bool unsharing);
>  
> +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> +                           bool reset_memory);

Please avoid passing multiple booleans, even more so when you already
derive them from a single "flags" value. This would likely be easiest
if you re-used the VM_EVENT_FLAG_RESET_FORK_* values also for
XENMEM_FORK_RESET_*, with suitable BUILD_BUG_ON() put in place to
prove they're the same.

> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1890,15 +1890,24 @@ static int fork(struct domain *cd, struct domain *d, uint16_t flags)
>   * footprints the hypercall continuation should be implemented (or if this
>   * feature needs to be become "stable").
>   */
> -static int mem_sharing_fork_reset(struct domain *d)
> +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> +                           bool reset_memory)
>  {
> -    int rc;
> +    int rc = 0;
>      struct domain *pd = d->parent;
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      struct page_info *page, *tmp;
>  
> +    ASSERT(reset_state || reset_memory);
> +
> +    if ( !d->arch.hvm.mem_sharing.fork_complete )
> +        return -ENOSYS;

Either EOPNOTSUPP (in case you think this operation could make sense
to implement for incomplete forks) or e.g. EINVAL, but please not
ENOSYS.

> @@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>              if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
>                  p2m_mem_paging_resume(d, &rsp);
>  #endif
> +#ifdef CONFIG_MEM_SHARING
> +            if ( mem_sharing_is_fork(d) )
> +            {
> +                bool reset_state = rsp.flags & VM_EVENT_FLAG_RESET_FORK_STATE;
> +                bool reset_mem = rsp.flags & VM_EVENT_FLAG_RESET_FORK_MEMORY;
> +
> +                if ( reset_state || reset_mem )
> +                    ASSERT(!mem_sharing_fork_reset(d, reset_state, reset_mem));
> +            }
> +#endif

Should the two flags be rejected in the "else" case, rather than
being silently ignored?

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
>                  uint32_t gref;     /* IN: gref to debug         */
>              } u;
>          } debug;
> -        struct mem_sharing_op_fork {      /* OP_FORK */
> +        struct mem_sharing_op_fork {      /* OP_FORK/_RESET */

I consider the notation in the comment misleading - I would read it to
mean OP_FORK and OP_RESET, supported by the earlier
OP_SHARE/ADD_PHYSMAP. Commonly we write OP_FORK{,_RESET} in such cases.

Jan