On Wed, Apr 13, 2022 at 09:41:51AM -0400, Tamas K Lengyel wrote:
> Allow specify distinct parts of the fork VM to be reset. This is useful when a
> fuzzing operation involves mapping in only a handful of pages that are known
> ahead of time. Throwing these pages away just to be re-copied immediately is
> expensive, thus allowing to specify partial resets can speed things up.
>
> Also allow resetting to be initiated from vm_event responses as an
> optiomization.
>
> Signed-off-by: Tamas K Lengyel <tamas.leng...@intel.com>
Reviewed-by: Roger Pau Monné <roger....@citrix.com>
> ---
> v4: No change
> v3: Rebase on simpler approach after dropping empty_p2m feature
> v2: address review comments and add more sanity checking
> ---
> tools/include/xenctrl.h | 3 ++-
> tools/libs/ctrl/xc_memshr.c | 7 ++++++-
> xen/arch/x86/include/asm/mem_sharing.h | 9 +++++++++
> xen/arch/x86/mm/mem_sharing.c | 24 +++++++++++++++++++-----
> xen/common/vm_event.c | 15 +++++++++++++++
> xen/include/public/memory.h | 4 +++-
> xen/include/public/vm_event.h | 8 ++++++++
> 7 files changed, 62 insertions(+), 8 deletions(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 95bd5eca67..1b089a2c02 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2290,7 +2290,8 @@ int xc_memshr_fork(xc_interface *xch,
> *
> * With VMs that have a lot of memory this call may block for a long time.
> */
> -int xc_memshr_fork_reset(xc_interface *xch, uint32_t forked_domain);
> +int xc_memshr_fork_reset(xc_interface *xch, uint32_t forked_domain,
> + bool reset_state, bool reset_memory);
>
> /* Debug calls: return the number of pages referencing the shared frame
> backing
> * the input argument. Should be one or greater.
> diff --git a/tools/libs/ctrl/xc_memshr.c b/tools/libs/ctrl/xc_memshr.c
> index a6cfd7dccf..a0d0b894e2 100644
> --- a/tools/libs/ctrl/xc_memshr.c
> +++ b/tools/libs/ctrl/xc_memshr.c
> @@ -257,12 +257,17 @@ int xc_memshr_fork(xc_interface *xch, uint32_t pdomid,
> uint32_t domid,
> return xc_memshr_memop(xch, domid, &mso);
> }
>
> -int xc_memshr_fork_reset(xc_interface *xch, uint32_t domid)
> +int xc_memshr_fork_reset(xc_interface *xch, uint32_t domid, bool reset_state,
> + bool reset_memory)
> {
> xen_mem_sharing_op_t mso;
>
> memset(&mso, 0, sizeof(mso));
> mso.op = XENMEM_sharing_op_fork_reset;
> + if ( reset_state )
> + mso.u.fork.flags |= XENMEM_FORK_RESET_STATE;
> + if ( reset_memory )
> + mso.u.fork.flags |= XENMEM_FORK_RESET_MEMORY;
IMO would be clearer to init mso fields at definition.
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 84cf52636b..d26a6699fc 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,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));
Might be appropriate to destroy the domain in case fork reset fails?
ASSERT will only help in debug builds.
> + }
> +#endif
>
> /*
> * 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 a1a0f0233a..f8d26fb77d 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 */
> /* Only makes sense for short-lived forks */
> #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
> /* Only makes sense for short-lived forks */
> #define XENMEM_FORK_BLOCK_INTERRUPTS (1u << 1)
Should you add:
/* Only for OP_FORK_RESET. */
> +#define XENMEM_FORK_RESET_STATE (1u << 2)
> +#define XENMEM_FORK_RESET_MEMORY (1u << 3)
Thanks, Roger.
