On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index a21c781452..bfa6082f13 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1892,15 +1892,19 @@ 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;
>
> domain_pause(d);
I would assert that at least one of reset_sate or reset_memory is set
here, as callers already do the checks.
>
> + if ( !reset_memory )
> + goto state;
I don't like using labels and goto like this as I think it makes the
code harder to follow, and so more likely to introduce bugs. I would
rather place the memory reset parts inside of an if ( reset_memory ) {
... }, but that's my taste.
> +
> /* need recursive lock because we will free pages */
> spin_lock_recursive(&d->page_alloc_lock);
> page_list_for_each_safe(page, tmp, &d->page_list)
> @@ -1933,7 +1937,9 @@ static int mem_sharing_fork_reset(struct domain *d)
> }
> spin_unlock_recursive(&d->page_alloc_lock);
>
> - rc = copy_settings(d, pd, d->arch.hvm.mem_sharing.skip_special_pages);
> + state:
> + if ( reset_state )
> + rc = copy_settings(d, pd,
> d->arch.hvm.mem_sharing.skip_special_pages);
>
> domain_unpause(d);
>
> @@ -2239,15 +2245,21 @@ int
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>
> case XENMEM_sharing_op_fork_reset:
> {
> + bool reset_state = mso.u.fork.flags & XENMEM_FORK_RESET_STATE;
> + bool reset_memory = mso.u.fork.flags & XENMEM_FORK_RESET_MEMORY;
> +
> rc = -EINVAL;
> - if ( mso.u.fork.pad || mso.u.fork.flags )
> + if ( mso.u.fork.pad || (!reset_state && !reset_memory) )
> + goto out;
> + if ( mso.u.fork.flags &
> + ~(XENMEM_FORK_RESET_STATE | XENMEM_FORK_RESET_MEMORY) )
> goto out;
>
> rc = -ENOSYS;
> if ( !d->parent )
> goto out;
>
> - rc = mem_sharing_fork_reset(d);
> + rc = mem_sharing_fork_reset(d, reset_state, reset_memory);
> break;
> }
>
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 84cf52636b..a7b192be0d 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -28,6 +28,11 @@
> #include <asm/p2m.h>
> #include <asm/monitor.h>
> #include <asm/vm_event.h>
> +
> +#ifdef CONFIG_MEM_SHARING
> +#include <asm/mem_sharing.h>
> +#endif
> +
> #include <xsm/xsm.h>
> #include <public/hvm/params.h>
>
> @@ -394,6 +399,15 @@ 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
> + do {
> + 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 )
> + mem_sharing_fork_reset(d, reset_state, reset_mem);
You seem to drop the error code returned by mem_sharing_fork_reset.
> + } while(0);
> +#endif
I think you can avoid the do {} while(0); just using the braces will
allow you to define local variables in the inner block.
> /*
> * Check emulation flags in the arch-specific handler only, as it
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 208d8dcbd9..30ce23c5a7 100644
> --- 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 */
> domid_t parent_domain; /* IN: parent's domain id */
> /* These flags only makes sense for short-lived forks */
> #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
> #define XENMEM_FORK_BLOCK_INTERRUPTS (1u << 1)
> #define XENMEM_FORK_SKIP_SPECIAL_PAGES (1u << 2)
> +#define XENMEM_FORK_RESET_STATE (1u << 3)
> +#define XENMEM_FORK_RESET_MEMORY (1u << 4)
For backward compatibility purposes should the flags be added
backwards, ie:
#define XENMEM_FORK_KEEP_STATE (1u << 3)
#define XENMEM_FORK_KEEP_MEMORY (1u << 4)
So that existing callers of XENMEM_sharing_op_fork_reset will continue
working as expected?
Or we don't care about that as the interface is protected with
__XEN_TOOLS__?
Thanks, Roger.
