[PATCH v15 06/10] x86/mm: make more of log-dirty mode enable/disable preemptable

[Jan Beulich <jbeulich@xxxxxxxx>]

So far only part of disable was preemptable. Shadow code, however, will
need to purge all earlier shadows, which may take quite a bit of time.
In the general (paging) layer arrange for the hook functions to possibly
indicate the need for resuming. In shadow code leverage "enable" already
calling shadow_blow_tables(), simply making that call preemptable, while
adding a call to that function for "disable".

In order for resume state to be properly guarded, the paging lock now
needs acquiring in paging_log_dirty_{en,dis}able(). HAP's need to
acquire the P2M lock makes it necessary to allow the hook functions to
drop the paging lock.

Note that when requesting resuming, the hook functions must not have
changed the paging (shadow) mode yet; the actual mode change part is
expected to be performed quickly enough to not require preemption. This
is why "blow" (done ahead of mode switching) is preferred over
"detach-old" (occurring with mode already updated). While this may mean
more purging being done now for "disable", starting from a clean state
afterwards may be better overall anyway.

Note further that HAP code is, except for the locking adjustments,
(intended to be) largely unaffected, for not having a need for
preemption checking.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
Locking isn't really nice here, but I see no better way given HAP's need
to acquire the P2M lock in its hook functions. Things may end up a little
better if doing away with the hooks and handling HAP and shadow (in part)
separately in paging.c, and then building upon HAP not needing any
preemption checking. Yet whether purging the hooks right here is okay is
unclear (see also "x86/shadow: make clean-dirty-bitmap post-processing
preemptable", where the question also arises).

See also the comments in "x86/shadow: 1-bit-disable doesn't need to
detach old tables". Specifically avoiding shadow_blow_tables() here
would mean pinned shadows wouldn't be needlessly unpinned.
---
v15: Add missing domain_unpause() to paging_log_dirty_enable()'s HAP
     path. Re-base.
v14: (Re)check and update preemption state under paging lock. Adjust
     locking accordingly. Re-base.
v13: New.

--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -199,6 +199,11 @@ struct log_dirty_domain {
 
     /* functions which are paging mode specific */
     const struct log_dirty_ops {
+        /*
+         * enable() and disable() will be called with the paging lock held.
+         * They may drop the lock (but not drop and re-acquire it), but then
+         * need to indicate so by returning a positive value.
+         */
         int        (*enable  )(struct domain *d);
         int        (*disable )(struct domain *d);
         void       (*clean   )(struct domain *d);
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -184,7 +184,7 @@ static int cf_check hap_enable_log_dirty
         return -EBUSY;
 
     /* turn on PG_log_dirty bit in paging mode */
-    paging_lock(d);
+    ASSERT(paging_locked_by_me(d));
     d->arch.paging.mode |= PG_log_dirty;
     paging_unlock(d);
 
@@ -198,12 +198,12 @@ static int cf_check hap_enable_log_dirty
     p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
     guest_flush_tlb_mask(d, d->dirty_cpumask);
 
-    return 0;
+    return 1;
 }
 
 static int cf_check hap_disable_log_dirty(struct domain *d)
 {
-    paging_lock(d);
+    ASSERT(paging_locked_by_me(d));
     d->arch.paging.mode &= ~PG_log_dirty;
     paging_unlock(d);
 
@@ -215,7 +215,7 @@ static int cf_check hap_disable_log_dirt
      * normal mode, or via hardware-assisted log-dirty.
      */
     p2m_change_entry_type_global(d, p2m_ram_logdirty, p2m_ram_rw);
-    return 0;
+    return 1;
 }
 
 static void cf_check hap_clean_dirty_bitmap(struct domain *d)
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -104,13 +104,10 @@ static int paging_free_log_dirty_bitmap(
     mfn_t *l4, *l3, *l2;
     int i4, i3, i2;
 
-    paging_lock(d);
+    ASSERT(paging_locked_by_me(d));
 
     if ( mfn_eq(d->arch.paging.log_dirty.top, INVALID_MFN) )
-    {
-        paging_unlock(d);
         return 0;
-    }
 
     if ( !d->arch.paging.preempt.dom )
     {
@@ -121,10 +118,7 @@ static int paging_free_log_dirty_bitmap(
     }
     else if ( d->arch.paging.preempt.dom != current->domain ||
               d->arch.paging.preempt.op != XEN_DOMCTL_SHADOW_OP_OFF )
-    {
-        paging_unlock(d);
         return -EBUSY;
-    }
 
     l4 = map_domain_page(d->arch.paging.log_dirty.top);
     i4 = d->arch.paging.preempt.log_dirty.i4;
@@ -196,12 +190,11 @@ static int paging_free_log_dirty_bitmap(
         d->arch.paging.preempt.op = XEN_DOMCTL_SHADOW_OP_OFF;
     }
 
-    paging_unlock(d);
-
     return rc;
 }
 
-static int paging_log_dirty_enable(struct domain *d)
+static int paging_log_dirty_enable(struct domain *d, unsigned int op,
+                                   bool resuming)
 {
     int ret;
 
@@ -217,9 +210,42 @@ static int paging_log_dirty_enable(struc
     if ( paging_mode_log_dirty(d) )
         return -EINVAL;
 
-    domain_pause(d);
+    if ( !resuming )
+        domain_pause(d);
+
+    paging_lock(d);
+
+    if ( d->arch.paging.preempt.dom &&
+         (d->arch.paging.preempt.dom != current->domain ||
+          d->arch.paging.preempt.op != op) )
+    {
+        paging_unlock(d);
+        if ( !resuming )
+            domain_unpause(d);
+        return -EBUSY;
+    }
+
     ret = d->arch.paging.log_dirty.ops->enable(d);
-    domain_unpause(d);
+
+    if ( ret > 0 )
+    {
+        /* Paging lock dropped by hook. */
+        domain_unpause(d);
+        ret = 0;
+    }
+    else if ( ret != -ERESTART )
+    {
+        d->arch.paging.preempt.dom = NULL;
+        paging_unlock(d);
+        domain_unpause(d);
+    }
+    else
+    {
+        ASSERT(!paging_mode_log_dirty(d));
+        d->arch.paging.preempt.dom = current->domain;
+        d->arch.paging.preempt.op = op;
+        paging_unlock(d);
+    }
 
     return ret;
 }
@@ -229,21 +255,49 @@ static int paging_log_dirty_disable(stru
     int ret = 1;
 
     if ( !resuming )
-    {
         domain_pause(d);
-        /* Safe because the domain is paused. */
-        if ( paging_mode_log_dirty(d) )
+
+    paging_lock(d);
+
+    if ( d->arch.paging.preempt.dom &&
+         (d->arch.paging.preempt.dom != current->domain ||
+          d->arch.paging.preempt.op != XEN_DOMCTL_SHADOW_OP_OFF) )
+    {
+        paging_unlock(d);
+        if ( !resuming )
+            domain_unpause(d);
+        return -EBUSY;
+    }
+
+    /* Safe because the domain is paused. */
+    if ( paging_mode_log_dirty(d) )
+    {
+        ret = d->arch.paging.log_dirty.ops->disable(d);
+        if ( ret > 0 ) /* Paging lock dropped by hook? */
+        {
+            paging_lock(d);
+            ret = 0;
+        }
+        else if ( ret != -ERESTART )
+            d->arch.paging.preempt.dom = NULL;
+        else
         {
-            ret = d->arch.paging.log_dirty.ops->disable(d);
-            ASSERT(ret <= 0);
+            ASSERT(paging_mode_log_dirty(d));
+            d->arch.paging.preempt.dom = current->domain;
+            d->arch.paging.preempt.op = XEN_DOMCTL_SHADOW_OP_OFF;
+            paging_unlock(d);
+            return ret;
         }
+
+        ASSERT(ret < 0 || !paging_mode_log_dirty(d));
     }
 
     ret = paging_free_log_dirty_bitmap(d, ret);
-    if ( ret == -ERESTART )
-        return ret;
 
-    domain_unpause(d);
+    paging_unlock(d);
+
+    if ( ret != -ERESTART )
+        domain_unpause(d);
 
     return ret;
 }
@@ -720,7 +774,7 @@ int paging_domctl(struct domain *d, stru
             break;
         fallthrough;
     case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY:
-        return paging_log_dirty_enable(d);
+        return paging_log_dirty_enable(d, sc->op, resuming);
 
     case XEN_DOMCTL_SHADOW_OP_OFF:
         if ( (rc = paging_log_dirty_disable(d, resuming)) != 0 )
@@ -812,7 +866,9 @@ int paging_teardown(struct domain *d)
 
 #if PG_log_dirty
     /* clean up log dirty resources. */
+    paging_lock(d);
     rc = paging_free_log_dirty_bitmap(d, 0);
+    paging_unlock(d);
     if ( rc == -ERESTART )
         return rc;
 #endif
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2567,13 +2567,22 @@ static int cf_check sh_enable_log_dirty(
 {
     int ret;
 
-    paging_lock(d);
+    ASSERT(paging_locked_by_me(d));
+
     if ( shadow_mode_enabled(d) )
     {
-        /* This domain already has some shadows: need to clear them out
+        bool preempted = false;
+
+        /*
+         * This domain already has some shadows: need to clear them out
          * of the way to make sure that all references to guest memory are
-         * properly write-protected */
-        shadow_blow_tables(d, NULL);
+         * properly write-protected.
+         *
+         * Furthermore see sh_disable_log_dirty() below.
+         */
+        shadow_blow_tables(d, &preempted);
+        if ( preempted )
+            return -ERESTART;
     }
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_LINUX_L3_TOPLEVEL)
@@ -2585,7 +2594,6 @@ static int cf_check sh_enable_log_dirty(
 #endif
 
     ret = shadow_one_bit_enable(d, PG_log_dirty);
-    paging_unlock(d);
 
     return ret;
 }
@@ -2593,13 +2601,18 @@ static int cf_check sh_enable_log_dirty(
 /* shadow specfic code which is called in paging_log_dirty_disable() */
 static int cf_check sh_disable_log_dirty(struct domain *d)
 {
-    int ret;
+    bool preempted = false;
 
-    paging_lock(d);
-    ret = shadow_one_bit_disable(d, PG_log_dirty);
-    paging_unlock(d);
+    ASSERT(paging_locked_by_me(d));
 
-    return ret;
+    /*
+     * Limit the amount of work to do from sh_detach_old_tables() (called from
+     * shadow_one_bit_disable() via sh_new_mode() -> sh_update_paging_modes()),
+     * such that it doesn't also need to deal with preemption checks.
+     */
+    shadow_blow_tables(d, &preempted);
+
+    return preempted ? -ERESTART : shadow_one_bit_disable(d, PG_log_dirty);
 }
 
 /* This function is called when we CLEAN log dirty bitmap. See