[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 1/2] x86/mem_sharing: make fork_reset more configurable


  • To: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Mon, 25 Apr 2022 16:11:40 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=h4ikID8eT+vgIa8f/6lN1MNLyj10nnkMkVbOfXLaKxg=; b=mDNO5Ud5JacsR6Mrme8+UFs0pknEpS68dHloJq3k+zcSvkZzZzGp6I+DpLUeXPidMATF5osH5u5xJWWoF+ow2SOgIuOVcJktJRTjnWX3CBm3YQWQrrOJcMJJCgzgPPc3tKC+KhLPd6qC6dbDQNhn6MoFWO2ssUnsk9/VCgbg1tuNXg6CMbXH3pjRYQRH9QZUzu27sWseoesF5orZUOUrMfJ6aJrDkjbyb8wZw9vqfqjsyfPXYO55+OviSsaJ2Xzpkez65vSaDfodzqzKnP5QYuSlOUJ5PXlDvaz5WZtdttsxGSXHxAvI6jxX65AnO62GbNrNpXADjqN1fUaX2cJFdg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=cYL2yx1FYPRgfgMRSWubP/kSlzVUfq+rrVk98rhJtCBiS6vcMD44rHIDwaj357vbGPlbz9qmNo/62P4b+W9kbCGvVvvy/vHOA3ObvqSFpYOIqj5Q29b03CIIVujCRhIfQTLF+wnlELUZATkiM4xe3BmxpXaD7o9PbXkHvcZ/e1Rt4oV0vwFDLV9iiuIbYP1S3vNGz8xF6UGM/qUuArgp3xGdqQXnVzXchzNwddhhOse7d1VmsHD2NSg4tacSQaV72uVy6je/RjahbwKCvvhM5Ol+ObuiuagGWlNtlaLPRNUQwkpdmDT4ph4wHxK+774aOl6cncCSEIsXjn8Q7Xa4aw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>, Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 25 Apr 2022 14:12:05 +0000
  • Ironport-data: A9a23:olxYDapBOO8i8QDDGi8PB5TWGQleBmKQZBIvgKrLsJaIsI4StFCzt garIBmFbvncYGKgKYh0PI+x8BwFv8XWyt8xSlZorX8yQSkQ+JuZCYyVIHmrMnLJJKUvbq7GA +byyDXkBJppJpMJjk71atANlVEliefQAOCU5NfsYkidfyc9IMsaoU8lyrZRbrJA24DjWVvR4 46q+qUzBXf+s9JKGjNMg068gEsHUMTa4Fv0aXRnOJinFHeH/5UkJMp3yZOZdhMUcaENdgKOf M7RzanRw4/s10xF5uVJMFrMWhZirrb6ZWBig5fNMkSoqkAqSicais7XOBeAAKv+Zvrgc91Zk b1wWZKMpQgBAaHGkcc+fBBiPwpENI9G04D4DXflvpnGp6HGWyOEL/RGKmgTZNRd1sMpRGZE+ LofNSwHaQ2Fi6Su2rWnR+Jwh8Mlas72IIcYvXImxjbcZRokacmbH+OWupkFg3Fp2Zkm8fX2P qL1bRJ1axvNeVtXM0o/A5Mihua4wHL4dlW0rXrL9PBpuTmPlGSd1pCyF9jxQoOpGflQxEihj 2LN0VX7LVIjYYn3JT2ttyjEavX0tT/yXYsJUrm18PF7jVm7x2oPBRlQXly+ydGph0j7V99BJ kg8/is1sbN05EGtVsP6XRCzvDiDpBF0c8VUO/037keK0KW8ywSWHG8fVRZadccr8sQxQFQC1 FWEgtfoDjxHq6CORDSW8bL8hTGvPSkYK0cSaClCShEKi/H4u506hB/LStdlEYa2g8fzFDW2x CqFxAAijrAaluYX1KG2+1/WjjbqrZ/MJiY85x7eX2asxgl4eIKoaYGu5VXBq/1HKe6xVkGAp nMNn8GU8cgEDI2BmSKARukABvei4PPtDdHHqVtmHp1k+zHz/XemJNlU+Gsnex4vNdsYczj0Z kOVoRlW+JJYIHqta+lwfp61DMMpi6PnELwJS8zpUzaHWbApHCfvwc2kTRT4M7zF+KT0rZwCB A==
  • Ironport-hdrordr: A9a23:jrAE/62lgr75uUerC98tswqjBSByeYIsimQD101hICG9Lfb0qy n+pp4mPEHP4wr5OEtOpTlPAtjkfZr5z+8M3WB3B8bYYOCGghrQEGgG1+ffKlLbexEWmtQttp uINpIOcuEYbmIK8voSgjPIdOrIqePvmM7IuQ6d9QYKcegDUdAd0+4TMHf+LqQZfnglOXJvf6 Dsm/av6gDQMEg/X4CePD0oTuLDr9rEmNbPZgMHPQcu7E2rgSmz4LD3PhCE1lNGOgk/iosKwC zgqUjU96+ju/a0xlv10HLS1Y1fnJ/ExsFYDMKBp8AJInHHixquZq5mR7qe1QpF6N2H2RIPqp 3hsh0gN8N85zf4eXy0mwLk303a3DMn+xbZuCulqEqmhfa8aCMxCsJHi44cWADe8VAcsNZ117 8O936FtrJMZCmw0xjV1pztbVVHh0C0qX0tnao4lHpES7YTb7dXsMg24F5VKpEdByj3gbpXXN WGNPuspcq+TGnqL0ww5gJUsZ+RtzUIb1q7q3E5y4KoO2M8pgE686MarPZv60vouqhNDqWs3N 60Q5iApIs+MPP+UpgNdNvpYfHHfVAlEii8Rl57HzzcZdI6EkOIjaLLy5MIw8zvUKA07fIJ6e b8uRVjxCQPR34=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.lengyel@xxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.