| Message ID | bc13e07cdb651afc2c8a97dde1be9c2158160307.1649857162.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] x86/mem_sharing: make fork_reset more configurable | expand |
On Wed, Apr 13, 2022 at 9:43 AM Tamas K Lengyel <tamas.lengyel@intel.com> 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 > optimization. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> Patch ping. Could I get a Reviewed-by if there are no objections? Thanks, Tamas
On 22.04.2022 16:07, Tamas K Lengyel wrote: > On Wed, Apr 13, 2022 at 9:43 AM Tamas K Lengyel <tamas.lengyel@intel.com> 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 >> optimization. >> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > > Patch ping. Could I get a Reviewed-by if there are no objections? Hmm, this is a little difficult. I'd be willing to give an ack, but that's meaningless for most of the code here. Besides a stylistic issue I did point out which I'm not happy with, I'm afraid I'm not good enough at mem-sharing and forking. Therefore I wouldn't want to offer an R-b. Considering the VM event interaction, maybe the BitDefender guys could take a stab? Of course you'd then still need a tool stack side ack. Jan
On Mon, Apr 25, 2022, 3:49 AM Jan Beulich <jbeulich@suse.com> wrote: > On 22.04.2022 16:07, Tamas K Lengyel wrote: > > On Wed, Apr 13, 2022 at 9:43 AM Tamas K Lengyel <tamas.lengyel@intel.com> > 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 > >> optimization. > >> > >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > > > > Patch ping. Could I get a Reviewed-by if there are no objections? > > Hmm, this is a little difficult. I'd be willing to give an ack, but that's > meaningless for most of the code here. Besides a stylistic issue I did > point out which I'm not happy with, I'm afraid I'm not good enough at > mem-sharing and forking. Therefore I wouldn't want to offer an R-b. > Considering the VM event interaction, maybe the BitDefender guys could > take a stab? > > Of course you'd then still need a tool stack side ack. > So my take is that noone cares about mem_sharing, which is fine, its an obscure experiment subsystem. But the only path I see as maintainer to get anything in-tree is if I hand the task of writing the patch to a coworker who then sends it in so that I can ack it. This is clearly disfunctional and is to the detriment of the project overall. We need to get some rules in place to avoid situations like this that clearly lead to no development and no improvement and a huge incentive to forgot about upstreaming. With no substantive objections but no acks a maintainer should be able to get changes in-tree. That's part of what I would consider maintaining a codebase to be! Anyway, to be realistic I don't expect that option to materialize so I'm very close to just stop all contributions to the project. It's dishartening. Tamas
On 25.04.2022 13:29, Tamas K Lengyel wrote: > On Mon, Apr 25, 2022, 3:49 AM Jan Beulich <jbeulich@suse.com> wrote: > >> On 22.04.2022 16:07, Tamas K Lengyel wrote: >>> On Wed, Apr 13, 2022 at 9:43 AM Tamas K Lengyel <tamas.lengyel@intel.com> >> 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 >>>> optimization. >>>> >>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> >>> >>> Patch ping. Could I get a Reviewed-by if there are no objections? >> >> Hmm, this is a little difficult. I'd be willing to give an ack, but that's >> meaningless for most of the code here. Besides a stylistic issue I did >> point out which I'm not happy with, I'm afraid I'm not good enough at >> mem-sharing and forking. Therefore I wouldn't want to offer an R-b. >> Considering the VM event interaction, maybe the BitDefender guys could >> take a stab? >> >> Of course you'd then still need a tool stack side ack. >> > > So my take is that noone cares about mem_sharing, which is fine, its an > obscure experiment subsystem. But the only path I see as maintainer to get > anything in-tree is if I hand the task of writing the patch to a coworker > who then sends it in so that I can ack it. This is clearly disfunctional > and is to the detriment of the project overall. We need to get some rules > in place to avoid situations like this that clearly lead to no development > and no improvement and a huge incentive to forgot about upstreaming. With > no substantive objections but no acks a maintainer should be able to get > changes in-tree. That's part of what I would consider maintaining a > codebase to be! I certainly understand your frustration, the more that I'm similarly affected with a much larger pile of patches. The check-in policy (see ./MAINTAINERS) is - I'm tempted to say "unfortunately" - quite clear about there being a need for a 2nd party to be involved. In this case though I've pointed out a possible route to unblock these two patches - let's give Petre and Alexandru at least a few days to possibly react to the ping. Apart from this I can only suggest to put this on the agenda of the next Community Call; I'm afraid I won't myself, as I've had this topic there already way too often. Jan
On Mon, Apr 25, 2022 at 12:29 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: > > > On Mon, Apr 25, 2022, 3:49 AM Jan Beulich <jbeulich@suse.com> wrote: > >> On 22.04.2022 16:07, Tamas K Lengyel wrote: >> > On Wed, Apr 13, 2022 at 9:43 AM Tamas K Lengyel < >> tamas.lengyel@intel.com> 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 >> >> optimization. >> >> >> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> >> > >> > Patch ping. Could I get a Reviewed-by if there are no objections? >> >> Hmm, this is a little difficult. I'd be willing to give an ack, but that's >> meaningless for most of the code here. Besides a stylistic issue I did >> point out which I'm not happy with, I'm afraid I'm not good enough at >> mem-sharing and forking. Therefore I wouldn't want to offer an R-b. >> Considering the VM event interaction, maybe the BitDefender guys could >> take a stab? >> >> Of course you'd then still need a tool stack side ack. >> > > So my take is that noone cares about mem_sharing, which is fine, its an > obscure experiment subsystem. > My take is slightly different; it's more that the project is large enough that it's difficult to se where the needs are. If Roger or Andy or I or Wei or anyone see a thread with you & Jan going back and forth, it's natural for us to assume that you & Jan have it in hand, and there's no need for us to read through the thread. Jan dislikes asking specific people for a review, but many of the rest of us have sort of gotten in the habit of doing so, as a way to solve the "visibility" issue. The only other way I can think of to solve the problem is to have a robot try to assign tasks to people -- a method that has received skepticism, and would also require a non-negligible amount of tooling to be written. > But the only path I see as maintainer to get anything in-tree is if I hand > the task of writing the patch to a coworker who then sends it in so that I > can ack it. This is clearly disfunctional and is to the detriment of the > project overall. We need to get some rules in place to avoid situations > like this that clearly lead to no development and no improvement and a huge > incentive to forgot about upstreaming. With no substantive objections but > no acks a maintainer should be able to get changes in-tree. That's part of > what I would consider maintaining a codebase to be! > Another possibility would be to ask your colleague actually do a Reviewed-by. The first time or two they might not be "of suitable stature in the community", but I don't think it should take long to establish such a stature, if they were doing the review in earnest. I do agree that it seems like in this situation, the bar seems too high for you to get your own code checked in. I'd be open to the argument that we should change the text of the check-in policy in MAINTAINERS to allow maintainer modifications with only an Acked-by. > Anyway, to be realistic I don't expect that option to materialize so I'm > very close to just stop all contributions to the project. It's dishartening. > I can understand why you'd be disheartened if you thought you just couldn't get any code checked in even as maintainer. However, there are lots of escalation paths open to you: you could email the community manager (me); you could make a wider appeal on IRC for reviewers; you could raise the general issue at the community call; you could send a patch proposing changes to the check-in procedure described in MAINTAINERS. -George >
On Mon, Apr 25, 2022 at 8:53 AM George Dunlap <dunlapg@umich.edu> wrote: > > > > On Mon, Apr 25, 2022 at 12:29 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: >> >> >> >> On Mon, Apr 25, 2022, 3:49 AM Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 22.04.2022 16:07, Tamas K Lengyel wrote: >>> > On Wed, Apr 13, 2022 at 9:43 AM Tamas K Lengyel <tamas.lengyel@intel.com> 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 >>> >> optimization. >>> >> >>> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> >>> > >>> > Patch ping. Could I get a Reviewed-by if there are no objections? >>> >>> Hmm, this is a little difficult. I'd be willing to give an ack, but that's >>> meaningless for most of the code here. Besides a stylistic issue I did >>> point out which I'm not happy with, I'm afraid I'm not good enough at >>> mem-sharing and forking. Therefore I wouldn't want to offer an R-b. >>> Considering the VM event interaction, maybe the BitDefender guys could >>> take a stab? >>> >>> Of course you'd then still need a tool stack side ack. >> >> >> So my take is that noone cares about mem_sharing, which is fine, its an obscure experiment subsystem. > > > My take is slightly different; it's more that the project is large enough that it's difficult to se where the needs are. If Roger or Andy or I or Wei or anyone see a thread with you & Jan going back and forth, it's natural for us to assume that you & Jan have it in hand, and there's no need for us to read through the thread. Jan dislikes asking specific people for a review, but many of the rest of us have sort of gotten in the habit of doing so, as a way to solve the "visibility" issue. The only other way I can think of to solve the problem is to have a robot try to assign tasks to people -- a method that has received skepticism, and would also require a non-negligible amount of tooling to be written. What's the point of the MAINTAINER's file and being automatically CC-d if you then still separately have to ping the same people by name? It's fine not to give an a-b/r-b if you still see discussion on some parts of the patch, but like on this one, where the tools changes are trivial - why would you wait? To be frank I long consider the tools-side part of Xen unmaintained with only the most trivial stuff ever having a chance to make it in. VM forking has effectively 0 toolstack side support in-tree because I never got any feedback from tools maintainers after sending the patches in for months. I would consider the toolstack side stuff in this patch for example trivial, but again, no tools maintainers ever look at it so I was actually considering dropping it from the patch completely since I really only need the vm_event interface. Again, that would be dropping an otherwise potentially useful interface purely due to the dysfunction of the project's maintenance. > >> >> But the only path I see as maintainer to get anything in-tree is if I hand the task of writing the patch to a coworker who then sends it in so that I can ack it. This is clearly disfunctional and is to the detriment of the project overall. We need to get some rules in place to avoid situations like this that clearly lead to no development and no improvement and a huge incentive to forgot about upstreaming. With no substantive objections but no acks a maintainer should be able to get changes in-tree. That's part of what I would consider maintaining a codebase to be! > > > Another possibility would be to ask your colleague actually do a Reviewed-by. The first time or two they might not be "of suitable stature in the community", but I don't think it should take long to establish such a stature, if they were doing the review in earnest. Sure, but clearly still more effort then it should be just to work around the system. > > I do agree that it seems like in this situation, the bar seems too high for you to get your own code checked in. I'd be open to the argument that we should change the text of the check-in policy in MAINTAINERS to allow maintainer modifications with only an Acked-by. Happy to hear! I think such a change would reduce the overhead on reviewing patches like this that clearly have no effect on anything else. > >> >> Anyway, to be realistic I don't expect that option to materialize so I'm very close to just stop all contributions to the project. It's dishartening. > > > > I can understand why you'd be disheartened if you thought you just couldn't get any code checked in even as maintainer. However, there are lots of escalation paths open to you: you could email the community manager (me); you could make a wider appeal on IRC for reviewers; you could raise the general issue at the community call; you could send a patch proposing changes to the check-in procedure described in MAINTAINERS. Fair point. But when your main job is not working on Xen and you have a couple weeks in-between other stuff to try to get some improvements in, it's not really viable to have to go reform the whole project. That's just the reality. If we can reduce the bar on getting code upstream in situations like this then I would be happy to continue working on the project but otherwise I don't see how this is worth anyone's time. Tamas
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@intel.com> Reviewed-by: Roger Pau Monné <roger.pau@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.
On Mon, Apr 25, 2022 at 10:12 AM Roger Pau Monné <roger.pau@citrix.com> 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@intel.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> 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
On Mon, Apr 25, 2022 at 11:24:37AM -0400, Tamas K Lengyel wrote: > On Mon, Apr 25, 2022 at 10:12 AM Roger Pau Monné <roger.pau@citrix.com> 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@intel.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > 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. IMO it's clearer to initialize the fields at declaration using mso = { ... } because then you avoid the memset. > > > > > 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. My concern for raising this was what would happen on non-debug builds if mem_sharing_fork_reset() failed, and hence my request to crash the domain. I would have used something like: if ( (reset_state || reset_mem) && mem_sharing_fork_reset(d, reset_state, reset_mem) ) { ASSERT_UNREACHABLE(); domain_crash(d); break; } But if you and other vm_event maintainers are fine with the current approach and don't think it's a problem that's OK with me. Thanks, Roger.
Hi, On 26/04/2022 09:17, Roger Pau Monné wrote: > On Mon, Apr 25, 2022 at 11:24:37AM -0400, Tamas K Lengyel wrote: >> On Mon, Apr 25, 2022 at 10:12 AM Roger Pau Monné <roger.pau@citrix.com> wrote: >>>> 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. > > My concern for raising this was what would happen on non-debug > builds if mem_sharing_fork_reset() failed, and hence my request to > crash the domain. I would have used something like: > > if ( (reset_state || reset_mem) && > mem_sharing_fork_reset(d, reset_state, reset_mem) ) > { > ASSERT_UNREACHABLE(); > domain_crash(d); > break; > } > > But if you and other vm_event maintainers are fine with the current > approach and don't think it's a problem that's OK with me. The current approach is actually not correct. On production build, ASSERT() will turn to NOP. IOW mem_sharing_fork_reset() *will* not be called. So the call needs to move outside of the ASSERT() and use a construct similar to what you suggested: if ( .... && mem_sharing_fork_reset(...) ) { ASSERT_UNREACHABLE(); break; } Cheers,
On Tue, Apr 26, 2022, 4:33 AM Julien Grall <julien@xen.org> wrote: > Hi, > > On 26/04/2022 09:17, Roger Pau Monné wrote: > > On Mon, Apr 25, 2022 at 11:24:37AM -0400, Tamas K Lengyel wrote: > >> On Mon, Apr 25, 2022 at 10:12 AM Roger Pau Monné <roger.pau@citrix.com> > wrote: > >>>> 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. > > > > My concern for raising this was what would happen on non-debug > > builds if mem_sharing_fork_reset() failed, and hence my request to > > crash the domain. I would have used something like: > > > > if ( (reset_state || reset_mem) && > > mem_sharing_fork_reset(d, reset_state, reset_mem) ) > > { > > ASSERT_UNREACHABLE(); > > domain_crash(d); > > break; > > } > > > > But if you and other vm_event maintainers are fine with the current > > approach and don't think it's a problem that's OK with me. > > The current approach is actually not correct. On production build, > ASSERT() will turn to NOP. IOW mem_sharing_fork_reset() *will* not be > called. > > So the call needs to move outside of the ASSERT() and use a construct > similar to what you suggested: > > if ( .... && mem_sharing_fork_reset(...) ) > { > ASSERT_UNREACHABLE(); > break; > } > Ah, good call. Thanks! Tamas >
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; return xc_memshr_memop(xch, domid, &mso); } diff --git a/xen/arch/x86/include/asm/mem_sharing.h b/xen/arch/x86/include/asm/mem_sharing.h index cf7a12f4d2..2c00069bc9 100644 --- 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); + /* * If called by a foreign domain, possible errors are * -EBUSY -> ring full @@ -148,6 +151,12 @@ static inline int mem_sharing_fork_page(struct domain *d, gfn_t gfn, bool lock) return -EOPNOTSUPP; } +static inline int mem_sharing_fork_reset(struct domain *d, bool reset_state, + bool reset_memory) +{ + return -EOPNOTSUPP; +} + #endif #endif /* __MEM_SHARING_H__ */ diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index 15e6a7ed81..2f447d94ab 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -1879,15 +1879,21 @@ static int fork(struct domain *cd, struct domain *d) * 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); + domain_pause(d); + if ( !reset_memory ) + goto state; + /* 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) @@ -1920,7 +1926,9 @@ static int mem_sharing_fork_reset(struct domain *d) } spin_unlock_recursive(&d->page_alloc_lock); - rc = copy_settings(d, pd); + state: + if ( reset_state ) + rc = copy_settings(d, pd); domain_unpause(d); @@ -2227,15 +2235,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..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)); + } +#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) +#define XENMEM_FORK_RESET_STATE (1u << 2) +#define XENMEM_FORK_RESET_MEMORY (1u << 3) uint16_t flags; /* IN: optional settings */ uint32_t pad; /* Must be set to 0 */ } fork; diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index bb003d21d0..81c2ee28cc 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -127,6 +127,14 @@ * Reset the vmtrace buffer (if vmtrace is enabled) */ #define VM_EVENT_FLAG_RESET_VMTRACE (1 << 13) +/* + * Reset the VM state (if VM is fork) + */ +#define VM_EVENT_FLAG_RESET_FORK_STATE (1 << 14) +/* + * Remove unshared entried from physmap (if VM is fork) + */ +#define VM_EVENT_FLAG_RESET_FORK_MEMORY (1 << 15) /* * Reasons for the vm event request
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@intel.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(-)