[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



 


Rackspace

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