[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
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@xxxxxxxxxx> 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, -- Julien Grall
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |