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
