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é <[email protected]> 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
