[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
On Mon, Apr 25, 2022 at 10:12 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote: > > 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> Thank you! > > --- > > 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. Not sure what you mean exactly, mso = { ... }; ? I think the logic is pretty clear as-is and I don't have any preference for one style vs the other. > > > 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. No, I would prefer not destroying the domain here. If it ever becomes necessary the right way would be to introduce a new monitor event to signal an error and let the listener decide what to do. At the moment I don't see that being necessary as there are no known scenarios where we would be able to setup a fork but fail to reset is. > > > + } > > +#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) I think the flag names are really descriptive already that these apply to the FORK_RESET case but I would have no objection to that comment being added at commit. Thanks again, Tamas
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |