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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Mar 30, 2022, 6:31 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:

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.

I don't see why that would be an improvement in any way. I also don't want to make VM_EVENT flags tied to the XENMEM ones as they are separate interfaces. I rather just drop the changes to the XENMEM interface then do that.

> --- 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?

What do you mean by rejected? There is no feasible "rejection" that could be done in this path other then skipping it.

> --- 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.

Ack.

Tamas