[PATCH v15 05/10] x86/shadow: make clean-dirty-bitmap post-processing preemptable

[Jan Beulich <jbeulich@xxxxxxxx>]

What this currently amounts to is blowing the domain's tables. To make
that call preemptable and to record state in a race-free manner, it
needs to be moved inside the paging-locked region. Since the
corresponding HAP function wants to acquire the P2M lock, the hook
invocation itself cannot be moved. Call the shadow function directly,
and prevent the hook from being invoked in that case.

To prevent "extra" accounting from being done more than once in
paging_log_dirty_op(), the mapping of the top level table also becomes
conditional (upon the subsequent loop doing nothing anyway).

Further note that this now also avoids sh{,adow}_clean_dirty_bitmap()
needlessly being called when no paging mode was enabled at all for the
domain. (The function continues to be called even when log-dirty mode
isn't enabled.)

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
The hook functions were/are slightly mis-named, as they don't clean
anything. However, to keep names similar (and hence possible to grep
for) the new directly called function is still named in the same manner.

Of course we could purge the hook altogether, calling the HAP function
directly as well. If so, question would be whether to do so right here
or in a public follow-up (perhaps the latter).

In the course of doing this work I've noticed that libxenguest uses
XEN_DOMCTL_SHADOW_OP_CLEAN even on the last iteration. Wouldn't it
suffice to invoke the non-cleaning (peek) operation there instead,
reducing the overhead of shadow's post-processing?
---
v15: Fix !SHADOW_PAGING build.
v14: New.

--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -234,7 +234,7 @@ struct paging_domain {
         union {
             struct {
                 unsigned long done:PADDR_BITS - PAGE_SHIFT;
-                unsigned long i4:PAGETABLE_ORDER;
+                unsigned long i4:PAGETABLE_ORDER + 1;
                 unsigned long i3:PAGETABLE_ORDER;
             } log_dirty;
         };
--- a/xen/arch/x86/include/asm/shadow.h
+++ b/xen/arch/x86/include/asm/shadow.h
@@ -56,6 +56,9 @@ int shadow_domain_init(struct domain *d)
  * paging_vcpu_init() in paging.c */
 void shadow_vcpu_init(struct vcpu *v);
 
+/* Post-processing necessary after the log-dirty bitmap was cleaned. */
+bool shadow_clean_dirty_bitmap(struct domain *d);
+
 #ifdef CONFIG_SHADOW_PAGING
 
 /* Enable an arbitrary shadow mode.  Call once at domain creation. */
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -473,8 +473,9 @@ static int paging_log_dirty_op(struct do
         goto out;
     }
 
-    l4 = paging_map_log_dirty_bitmap(d);
     i4 = d->arch.paging.preempt.log_dirty.i4;
+    if ( i4 < LOGDIRTY_NODE_ENTRIES )
+        l4 = paging_map_log_dirty_bitmap(d);
     i3 = d->arch.paging.preempt.log_dirty.i3;
     pages = d->arch.paging.preempt.log_dirty.done;
 
@@ -542,6 +543,19 @@ static int paging_log_dirty_op(struct do
     if ( l4 )
         unmap_domain_page(l4);
 
+    /*
+     * In the shadow case post-processing may take long and hence needs to be
+     * preemptable. Since continuation state is protected by the paging lock,
+     * we need to call its function (and record state) before dropping that
+     * lock. The hook invocation further down therefore will be skipped.
+     */
+    if ( !rv && clean && paging_mode_shadow(d) &&
+         !shadow_clean_dirty_bitmap(d) )
+    {
+        d->arch.paging.preempt.log_dirty.i4 = LOGDIRTY_NODE_ENTRIES;
+        rv = -ERESTART;
+    }
+
     if ( !rv )
     {
         d->arch.paging.preempt.dom = NULL;
@@ -569,10 +583,12 @@ static int paging_log_dirty_op(struct do
 
     if ( pages < sc->pages )
         sc->pages = pages;
-    if ( clean )
+    if ( clean && paging_mode_hap(d) )
     {
-        /* We need to further call clean_dirty_bitmap() functions of specific
-         * paging modes (shadow or hap).  Safe because the domain is paused. */
+        /*
+         * We need to further call HAP's clean_dirty_bitmap() function (shadow
+         * was dealt with above).  Safe because the domain is paused.
+         */
         d->arch.paging.log_dirty.ops->clean(d);
     }
     domain_unpause(d);
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -71,7 +71,6 @@ DEFINE_PER_CPU(uint32_t,trace_shadow_pat
 
 static int cf_check sh_enable_log_dirty(struct domain *d);
 static int cf_check sh_disable_log_dirty(struct domain *d);
-static void cf_check sh_clean_dirty_bitmap(struct domain *d);
 
 static void cf_check shadow_update_paging_modes(struct vcpu *v);
 
@@ -82,7 +81,7 @@ int shadow_domain_init(struct domain *d)
     static const struct log_dirty_ops sh_ops = {
         .enable  = sh_enable_log_dirty,
         .disable = sh_disable_log_dirty,
-        .clean   = sh_clean_dirty_bitmap,
+        .clean   = ZERO_BLOCK_PTR,
     };
 
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.pinned_shadows);
@@ -2606,14 +2605,18 @@ static int cf_check sh_disable_log_dirty
 /* This function is called when we CLEAN log dirty bitmap. See
  * paging_log_dirty_op() for details.
  */
-static void cf_check sh_clean_dirty_bitmap(struct domain *d)
+bool shadow_clean_dirty_bitmap(struct domain *d)
 {
-    paging_lock(d);
+    bool preempted = false;
+
+    ASSERT(paging_locked_by_me(d));
+
     /* Need to revoke write access to the domain's pages again.
      * In future, we'll have a less heavy-handed approach to this,
      * but for now, we just unshadow everything except Xen. */
-    shadow_blow_tables(d, NULL);
-    paging_unlock(d);
+    shadow_blow_tables(d, &preempted);
+
+    return !preempted;
 }
 
 /**************************************************************************/
--- a/xen/arch/x86/mm/shadow/none.c
+++ b/xen/arch/x86/mm/shadow/none.c
@@ -7,11 +7,6 @@ static int cf_check _toggle_log_dirty(st
     return -EOPNOTSUPP;
 }
 
-static void cf_check _clean_dirty_bitmap(struct domain *d)
-{
-    ASSERT(is_pv_domain(d));
-}
-
 static void cf_check _update_paging_modes(struct vcpu *v)
 {
     ASSERT_UNREACHABLE();
@@ -23,7 +18,7 @@ int shadow_domain_init(struct domain *d)
     static const struct log_dirty_ops sh_none_ops = {
         .enable  = _toggle_log_dirty,
         .disable = _toggle_log_dirty,
-        .clean   = _clean_dirty_bitmap,
+        .clean   = ZERO_BLOCK_PTR,
     };
 
     paging_log_dirty_init(d, &sh_none_ops);