[PATCH 0/5] x86: Remove vestigal PV Autotranslate pieces

[PATCH 0/5] x86: Remove vestigal PV Autotranslate pieces

[Andrew Cooper]

Autotranslate PV domains haven't been able to be built for two releases of
Xen, and noone has noticed.  The shadow emulation code for such domains has
never functioned correctly for guests running in a mode different to Xen.

This isn't as much cleanup as I intended to do, but it turns out that I pulled
a little too hard on a thread, and everything fell to pieces.  This reduced
series has had moderate testing within XenServer, and everything still appears
to be fine.

Toolstack and PVH folk: There is also toolstack side cleanup which can be
done, but the concept of a translated PV guest is also used for PVHv1.  I have
some extra deletion to contribute to whomever rips PVHv1 out of the domain
builder.

Andrew Cooper (5):
  x86/traps: Drop paging_mode_external() handling from the PV pagefault path
  x86/shadow: Tweak some initialisation in sh_page_fault()
  x86/paging: Enforce PG_external == PG_translate == PG_refcounts
  x86/shadow: Drop all emulation for PV vcpus
  x86/shadow: Misc minor cleanup

 xen/arch/x86/mm/paging.c        |  19 ++++---
 xen/arch/x86/mm/shadow/common.c | 112 +++++++---------------------------------
 xen/arch/x86/mm/shadow/multi.c  |  46 ++++-------------
 xen/arch/x86/traps.c            |  16 ++----
 xen/include/asm-x86/paging.h    |   9 +++-
 5 files changed, 55 insertions(+), 147 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/5] x86/traps: Drop paging_mode_external() handling from the PV pagefault path

[Andrew Cooper]

PV guests necessarily can't be external, as Xen must steal address space from
them.  Pagefaults for HVM guests are handled by {vmx,svm}_vmexit_handler() and
don't enter the PV fixup_page_fault() path.

This paging_fault() callsite is therefore dead code, so drop it.

Clarify the comment at the other paging_fault() callsite to indicate that it
covers the logdirty case only.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/traps.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 013e633..b20faa4 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1799,15 +1799,6 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
     if ( in_irq() || !(regs->eflags & X86_EFLAGS_IF) )
         return 0;
 
-    /* Faults from external-mode guests are handled by shadow/hap */
-    if ( paging_mode_external(d) && guest_mode(regs) )
-    {
-        int ret = paging_fault(addr, regs);
-        if ( ret == EXCRET_fault_fixed )
-            trace_trap_two_addr(TRC_PV_PAGING_FIXUP, regs->eip, addr);
-        return ret;
-    }
-
     if ( !(regs->error_code & PFEC_page_present) &&
           (pagefault_by_memadd(addr, regs)) )
         return handle_memadd_fault(addr, regs);
@@ -1838,8 +1829,11 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
             return EXCRET_fault_fixed;
     }
 
-    /* For non-external shadowed guests, we fix up both their own 
-     * pagefaults and Xen's, since they share the pagetables. */
+    /*
+     * For non-external shadowed guests (i.e. PV guests with logdirty
+     * active), we fix up both their own pagefaults and Xen's, since
+     * they share the pagetables.
+     */
     if ( paging_mode_enabled(d) && !paging_mode_external(d) )
     {
         int ret = paging_fault(addr, regs);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/5] x86/shadow: Tweak some initialisation in sh_page_fault()

[Andrew Cooper]

sh_page_fault() is a complicated function.  It aids clarity for the reader if
constant data is declared as such.

Declare struct npfec access and fetch_type_t ft as const, which requires
initialising them during declaration.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/mm/shadow/multi.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index f494f7b..67c98b9 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2860,15 +2860,17 @@ static int sh_page_fault(struct vcpu *v,
     struct sh_emulate_ctxt emul_ctxt;
     const struct x86_emulate_ops *emul_ops;
     int r;
-    fetch_type_t ft = 0;
     p2m_type_t p2mt;
     uint32_t rc;
     int version;
-    struct npfec access = {
+    const struct npfec access = {
          .read_access = 1,
+         .write_access = !!(regs->error_code & PFEC_write_access),
          .gla_valid = 1,
          .kind = npfec_kind_with_gla
     };
+    const fetch_type_t ft =
+        access.write_access ? ft_demand_write : ft_demand_read;
 #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION
     int fast_emul = 0;
 #endif
@@ -2878,9 +2880,6 @@ static int sh_page_fault(struct vcpu *v,
 
     perfc_incr(shadow_fault);
 
-    if ( regs->error_code & PFEC_write_access )
-        access.write_access = 1;
-
 #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION
     /* If faulting frame is successfully emulated in last shadow fault
      * it's highly likely to reach same emulation action for this frame.
@@ -3050,10 +3049,6 @@ static int sh_page_fault(struct vcpu *v,
         goto propagate;
     }
 
-    /* What kind of access are we dealing with? */
-    ft = ((regs->error_code & PFEC_write_access)
-          ? ft_demand_write : ft_demand_read);
-
     /* What mfn is the guest trying to access? */
     gfn = guest_l1e_get_gfn(gw.l1e);
     gmfn = get_gfn(d, gfn, &p2mt);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/5] x86/paging: Enforce PG_external == PG_translate == PG_refcounts

[Andrew Cooper]

Setting PG_refcounts but not PG_translate is not useful.

While adjusting this, make a few other improvements.

 * Have paging_enable() unilaterally reject any unknown modes.
 * Correct the function description.  paging_enable() is also used to enable
   logdirty during runtime.
 * Drop the or'ing of PG_{HAP,SH}_enable.  The underlying functions already do
   this.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/mm/paging.c        | 19 +++++++++++++------
 xen/arch/x86/mm/shadow/common.c |  3 +--
 xen/include/asm-x86/paging.h    |  9 +++++++--
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 853a035..9548222 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -842,23 +842,30 @@ void paging_final_teardown(struct domain *d)
     p2m_final_teardown(d);
 }
 
-/* Enable an arbitrary paging-assistance mode.  Call once at domain
- * creation. */
+/*
+ * Enable an arbitrary paging-assistance mode.  Call once at domain
+ * creation, and during runtime for logdirty mode.
+ */
 int paging_enable(struct domain *d, u32 mode)
 {
-    switch ( mode & (PG_external | PG_translate) )
+    /* Unrecognised paging mode? */
+    if ( mode & ~PG_MASK )
+        return -EINVAL;
+
+    /* All of external|translate|refcounts, or none. */
+    switch ( mode & (PG_external | PG_translate | PG_refcounts) )
     {
     case 0:
-    case PG_external | PG_translate:
+    case PG_external | PG_translate | PG_refcounts:
         break;
     default:
         return -EINVAL;
     }
 
     if ( hap_enabled(d) )
-        return hap_enable(d, mode | PG_HAP_enable);
+        return hap_enable(d, mode);
     else
-        return shadow_enable(d, mode | PG_SH_enable);
+        return shadow_enable(d, mode);
 }
 
 /* Called from the guest to indicate that a process is being torn down
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 39be564..84a87f3 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3144,8 +3144,7 @@ int shadow_enable(struct domain *d, u32 mode)
     domain_pause(d);
 
     /* Sanity check the arguments */
-    if ( shadow_mode_enabled(d) ||
-         ((mode & PG_translate) && !(mode & PG_refcounts)) )
+    if ( shadow_mode_enabled(d) )
     {
         rv = -EINVAL;
         goto out_unlocked;
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index f83ed8b..1535659 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -57,6 +57,9 @@
  * requires VT or similar mechanisms */
 #define PG_external    (XEN_DOMCTL_SHADOW_ENABLE_EXTERNAL << PG_mode_shift)
 
+/* All paging modes. */
+#define PG_MASK (PG_refcounts | PG_log_dirty | PG_translate | PG_external)
+
 #define paging_mode_enabled(_d)   (!!(_d)->arch.paging.mode)
 #define paging_mode_shadow(_d)    (!!((_d)->arch.paging.mode & PG_SH_enable))
 #define paging_mode_hap(_d)       (!!((_d)->arch.paging.mode & PG_HAP_enable))
@@ -212,8 +215,10 @@ int paging_teardown(struct domain *d);
 /* Call once all of the references to the domain have gone away */
 void paging_final_teardown(struct domain *d);
 
-/* Enable an arbitrary paging-assistance mode.  Call once at domain
- * creation. */
+/*
+ * Enable an arbitrary paging-assistance mode.  Call once at domain
+ * creation, and during runtime for logdirty mode.
+ */
 int paging_enable(struct domain *d, u32 mode);
 
 #define paging_get_hostmode(v)		((v)->arch.paging.mode)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/5] x86/shadow: Drop all emulation for PV vcpus

[Andrew Cooper]

Emulation is only performed for paging_mode_refcount() domains, which in
practice means HVM domains only.

Drop the PV emulation code.  As it always set addr_side and sp_size to
BITS_PER_LONG, it can't have worked correctly for PV guests running in a
different mode to Xen.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/mm/shadow/common.c | 111 +++++++---------------------------------
 xen/arch/x86/mm/shadow/multi.c  |  21 ++------
 2 files changed, 22 insertions(+), 110 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 84a87f3..2525a57 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -318,75 +318,6 @@ static const struct x86_emulate_ops hvm_shadow_emulator_ops = {
     .cpuid      = hvmemul_cpuid,
 };
 
-static int
-pv_emulate_read(enum x86_segment seg,
-                unsigned long offset,
-                void *p_data,
-                unsigned int bytes,
-                struct x86_emulate_ctxt *ctxt)
-{
-    unsigned int rc;
-
-    if ( !is_x86_user_segment(seg) )
-        return X86EMUL_UNHANDLEABLE;
-
-    if ( (rc = copy_from_user(p_data, (void *)offset, bytes)) != 0 )
-    {
-        x86_emul_pagefault(0, offset + bytes - rc, ctxt); /* Read fault. */
-        return X86EMUL_EXCEPTION;
-    }
-
-    return X86EMUL_OKAY;
-}
-
-static int
-pv_emulate_write(enum x86_segment seg,
-                 unsigned long offset,
-                 void *p_data,
-                 unsigned int bytes,
-                 struct x86_emulate_ctxt *ctxt)
-{
-    struct sh_emulate_ctxt *sh_ctxt =
-        container_of(ctxt, struct sh_emulate_ctxt, ctxt);
-    struct vcpu *v = current;
-    if ( !is_x86_user_segment(seg) )
-        return X86EMUL_UNHANDLEABLE;
-    return v->arch.paging.mode->shadow.x86_emulate_write(
-        v, offset, p_data, bytes, sh_ctxt);
-}
-
-static int
-pv_emulate_cmpxchg(enum x86_segment seg,
-                   unsigned long offset,
-                   void *p_old,
-                   void *p_new,
-                   unsigned int bytes,
-                   struct x86_emulate_ctxt *ctxt)
-{
-    struct sh_emulate_ctxt *sh_ctxt =
-        container_of(ctxt, struct sh_emulate_ctxt, ctxt);
-    unsigned long old, new;
-    struct vcpu *v = current;
-
-    if ( !is_x86_user_segment(seg) || bytes > sizeof(long) )
-        return X86EMUL_UNHANDLEABLE;
-
-    old = new = 0;
-    memcpy(&old, p_old, bytes);
-    memcpy(&new, p_new, bytes);
-
-    return v->arch.paging.mode->shadow.x86_emulate_cmpxchg(
-               v, offset, old, new, bytes, sh_ctxt);
-}
-
-static const struct x86_emulate_ops pv_shadow_emulator_ops = {
-    .read       = pv_emulate_read,
-    .insn_fetch = pv_emulate_read,
-    .write      = pv_emulate_write,
-    .cmpxchg    = pv_emulate_cmpxchg,
-    .cpuid      = pv_emul_cpuid,
-};
-
 const struct x86_emulate_ops *shadow_init_emulation(
     struct sh_emulate_ctxt *sh_ctxt, struct cpu_user_regs *regs)
 {
@@ -394,17 +325,13 @@ const struct x86_emulate_ops *shadow_init_emulation(
     struct vcpu *v = current;
     unsigned long addr;
 
+    ASSERT(has_hvm_container_vcpu(v));
+
     memset(sh_ctxt, 0, sizeof(*sh_ctxt));
 
     sh_ctxt->ctxt.regs = regs;
     sh_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
 
-    if ( is_pv_vcpu(v) )
-    {
-        sh_ctxt->ctxt.addr_size = sh_ctxt->ctxt.sp_size = BITS_PER_LONG;
-        return &pv_shadow_emulator_ops;
-    }
-
     /* Segment cache initialisation. Primed with CS. */
     creg = hvm_get_seg_reg(x86_seg_cs, sh_ctxt);
 
@@ -441,24 +368,24 @@ void shadow_continue_emulation(struct sh_emulate_ctxt *sh_ctxt,
     struct vcpu *v = current;
     unsigned long addr, diff;
 
-    /* We don't refetch the segment bases, because we don't emulate
-     * writes to segment registers */
+    ASSERT(has_hvm_container_vcpu(v));
 
-    if ( is_hvm_vcpu(v) )
-    {
-        diff = regs->eip - sh_ctxt->insn_buf_eip;
-        if ( diff > sh_ctxt->insn_buf_bytes )
-        {
-            /* Prefetch more bytes. */
-            sh_ctxt->insn_buf_bytes =
-                (!hvm_translate_linear_addr(
-                    x86_seg_cs, regs->eip, sizeof(sh_ctxt->insn_buf),
-                    hvm_access_insn_fetch, sh_ctxt, &addr) &&
-                 !hvm_fetch_from_guest_linear(
-                     sh_ctxt->insn_buf, addr, sizeof(sh_ctxt->insn_buf), 0, NULL))
-                ? sizeof(sh_ctxt->insn_buf) : 0;
-            sh_ctxt->insn_buf_eip = regs->eip;
-        }
+    /*
+     * We don't refetch the segment bases, because we don't emulate
+     * writes to segment registers
+     */
+    diff = regs->eip - sh_ctxt->insn_buf_eip;
+    if ( diff > sh_ctxt->insn_buf_bytes )
+    {
+        /* Prefetch more bytes. */
+        sh_ctxt->insn_buf_bytes =
+            (!hvm_translate_linear_addr(
+                x86_seg_cs, regs->eip, sizeof(sh_ctxt->insn_buf),
+                hvm_access_insn_fetch, sh_ctxt, &addr) &&
+             !hvm_fetch_from_guest_linear(
+                 sh_ctxt->insn_buf, addr, sizeof(sh_ctxt->insn_buf), 0, NULL))
+            ? sizeof(sh_ctxt->insn_buf) : 0;
+        sh_ctxt->insn_buf_eip = regs->eip;
     }
 }
 
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 67c98b9..713f23d 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3382,12 +3382,7 @@ static int sh_page_fault(struct vcpu *v,
               (((emul_ctxt.ctxt.event.vector == TRAP_gp_fault) ||
                 (emul_ctxt.ctxt.event.vector == TRAP_stack_error)) &&
                emul_ctxt.ctxt.event.error_code == 0)) )
-        {
-            if ( has_hvm_container_domain(d) )
-                hvm_inject_event(&emul_ctxt.ctxt.event);
-            else
-                pv_inject_event(&emul_ctxt.ctxt.event);
-        }
+            hvm_inject_event(&emul_ctxt.ctxt.event);
         else
         {
             SHADOW_PRINTK(
@@ -3447,12 +3442,7 @@ static int sh_page_fault(struct vcpu *v,
 #endif
 
     if ( emul_ctxt.ctxt.retire.singlestep )
-    {
-        if ( has_hvm_container_domain(d) )
-            hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-        else
-            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-    }
+        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
 
 #if GUEST_PAGING_LEVELS == 3 /* PAE guest */
     /*
@@ -3493,12 +3483,7 @@ static int sh_page_fault(struct vcpu *v,
                 TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED);
 
                 if ( emul_ctxt.ctxt.retire.singlestep )
-                {
-                    if ( has_hvm_container_domain(d) )
-                        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-                    else
-                        pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-                }
+                    hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
 
                 break; /* Don't emulate again if we failed! */
             }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 5/5] x86/shadow: Misc minor cleanup

[Andrew Cooper]

 * Move the #ifdefary inside sh_audit_gw() to avoid needing the else clause.
 * The walk_t parameter is only ever read, so make it const.
 * Use mfn_eq() rather than opencoding it.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/mm/shadow/multi.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 713f23d..336d24f 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -321,11 +321,11 @@ gw_remove_write_accesses(struct vcpu *v, unsigned long va, walk_t *gw)
     return rc;
 }
 
-#if SHADOW_AUDIT & SHADOW_AUDIT_ENTRIES
 /* Lightweight audit: pass all the shadows associated with this guest walk
  * through the audit mechanisms */
-static void sh_audit_gw(struct vcpu *v, walk_t *gw)
+static void sh_audit_gw(struct vcpu *v, const walk_t *gw)
 {
+#if SHADOW_AUDIT & SHADOW_AUDIT_ENTRIES
     struct domain *d = v->domain;
     mfn_t smfn;
 
@@ -362,13 +362,9 @@ static void sh_audit_gw(struct vcpu *v, walk_t *gw)
               && mfn_valid(
               (smfn = get_fl1_shadow_status(d, guest_l2e_get_gfn(gw->l2e)))) )
         (void) sh_audit_fl1_table(v, smfn, INVALID_MFN);
+#endif /* SHADOW_AUDIT & SHADOW_AUDIT_ENTRIES */
 }
 
-#else
-#define sh_audit_gw(_v, _gw) do {} while(0)
-#endif /* audit code */
-
-
 /*
  * Write a new value into the guest pagetable, and update the shadows
  * appropriately.  Returns 0 if we page-faulted, 1 for success.
@@ -3309,7 +3305,7 @@ static int sh_page_fault(struct vcpu *v,
                 }
             }
 #else /* 32 or 64 */
-            used = (mfn_x(pagetable_get_mfn(tmp->arch.guest_table)) == mfn_x(gmfn));
+            used = mfn_eq(pagetable_get_mfn(tmp->arch.guest_table), gmfn);
 #endif
             if ( used )
                 break;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/traps: Drop paging_mode_external() handling from the PV pagefault path

[Jan Beulich]

>>> On 12.12.16 at 11:43, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1799,15 +1799,6 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
>      if ( in_irq() || !(regs->eflags & X86_EFLAGS_IF) )
>          return 0;
>  
> -    /* Faults from external-mode guests are handled by shadow/hap */
> -    if ( paging_mode_external(d) && guest_mode(regs) )
> -    {
> -        int ret = paging_fault(addr, regs);
> -        if ( ret == EXCRET_fault_fixed )
> -            trace_trap_two_addr(TRC_PV_PAGING_FIXUP, regs->eip, addr);
> -        return ret;
> -    }

Perhaps worthwhile leaving an ASSERT() with the inverted if()
condition here?

Anyway, with or without that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/5] x86/shadow: Tweak some initialisation in sh_page_fault()

[Jan Beulich]

>>> On 12.12.16 at 11:43, <andrew.cooper3@citrix.com> wrote:
> sh_page_fault() is a complicated function.  It aids clarity for the reader if
> constant data is declared as such.
> 
> Declare struct npfec access and fetch_type_t ft as const, which requires
> initialising them during declaration.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/traps: Drop paging_mode_external() handling from the PV pagefault path

[Tim Deegan]

At 10:43 +0000 on 12 Dec (1481539421), Andrew Cooper wrote:
> PV guests necessarily can't be external, as Xen must steal address space from
> them.  Pagefaults for HVM guests are handled by {vmx,svm}_vmexit_handler() and
> don't enter the PV fixup_page_fault() path.
> 
> This paging_fault() callsite is therefore dead code, so drop it.
> 
> Clarify the comment at the other paging_fault() callsite to indicate that it
> covers the logdirty case only.
> 
> No functional change.

IMO this is a change, just not on any supported config.

> -    /* For non-external shadowed guests, we fix up both their own 
> -     * pagefaults and Xen's, since they share the pagetables. */
> +    /*
> +     * For non-external shadowed guests (i.e. PV guests with logdirty
> +     * active), we fix up both their own pagefaults and Xen's, since
> +     * they share the pagetables.
> +     */
>      if ( paging_mode_enabled(d) && !paging_mode_external(d) )

Here we can drop the check of !paging_mode_external(d), or maybe turn
it into an assertion somewhere.

With that,

Acked-by: Tim Deegan <tim@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/5] x86/shadow: Tweak some initialisation in sh_page_fault()

[Tim Deegan]

At 10:43 +0000 on 12 Dec (1481539422), Andrew Cooper wrote:
> sh_page_fault() is a complicated function.  It aids clarity for the reader if
> constant data is declared as such.
> 
> Declare struct npfec access and fetch_type_t ft as const, which requires
> initialising them during declaration.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Tim Deegan <tim@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/paging: Enforce PG_external == PG_translate == PG_refcounts

[Tim Deegan]

At 10:43 +0000 on 12 Dec (1481539423), Andrew Cooper wrote:
> Setting PG_refcounts but not PG_translate is not useful.
> 
> While adjusting this, make a few other improvements.
> 
>  * Have paging_enable() unilaterally reject any unknown modes.
>  * Correct the function description.  paging_enable() is also used to enable
>    logdirty during runtime.

Don't run-time log-dirty changes go through paging_log_dirty_{en,dis}able()?

Apart from that, Acked-by: Tim Deegan <tim@xen.org>.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/traps: Drop paging_mode_external() handling from the PV pagefault path

[Andrew Cooper]

On 12/12/16 11:27, Tim Deegan wrote:
> At 10:43 +0000 on 12 Dec (1481539421), Andrew Cooper wrote:
>> PV guests necessarily can't be external, as Xen must steal address space from
>> them.  Pagefaults for HVM guests are handled by {vmx,svm}_vmexit_handler() and
>> don't enter the PV fixup_page_fault() path.
>>
>> This paging_fault() callsite is therefore dead code, so drop it.
>>
>> Clarify the comment at the other paging_fault() callsite to indicate that it
>> covers the logdirty case only.
>>
>> No functional change.
> IMO this is a change, just not on any supported config.

Really?  I'd agree if the content of the clause was reachable (and we
didn't support the configuration required to make it reachable), but my
argument here is that it is genuinely dead code and cannot be reached.

The only way to make paging_mode_external(d) true would cause Xen to be
using a different path to service the pagefault.

This particular piece of code makes me wonder whether HVM guests used to
use the PV pagefault path, but if they ever did, they don't any more.

>
>> -    /* For non-external shadowed guests, we fix up both their own 
>> -     * pagefaults and Xen's, since they share the pagetables. */
>> +    /*
>> +     * For non-external shadowed guests (i.e. PV guests with logdirty
>> +     * active), we fix up both their own pagefaults and Xen's, since
>> +     * they share the pagetables.
>> +     */
>>      if ( paging_mode_enabled(d) && !paging_mode_external(d) )
> Here we can drop the check of !paging_mode_external(d), or maybe turn
> it into an assertion somewhere.
>
> With that,
>
> Acked-by: Tim Deegan <tim@xen.org>

Seeing as both you and Jan have asked, I will see about addition an
assertion.  However, it will have to be in patch 3 for bisectability,
when we rule out the use of PV refcount guests.  Are you ok with that?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/5] x86/shadow: Drop all emulation for PV vcpus

[Tim Deegan]

At 10:43 +0000 on 12 Dec (1481539424), Andrew Cooper wrote:
> Emulation is only performed for paging_mode_refcount() domains, which in
> practice means HVM domains only.
> 
> Drop the PV emulation code.  As it always set addr_side and sp_size to
> BITS_PER_LONG, it can't have worked correctly for PV guests running in a
> different mode to Xen.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Tim Deegan <tim@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/5] x86/shadow: Misc minor cleanup

[Tim Deegan]

At 10:43 +0000 on 12 Dec (1481539425), Andrew Cooper wrote:
>  * Move the #ifdefary inside sh_audit_gw() to avoid needing the else clause.
>  * The walk_t parameter is only ever read, so make it const.
>  * Use mfn_eq() rather than opencoding it.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Tim Deegan <tim@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/traps: Drop paging_mode_external() handling from the PV pagefault path

[Tim Deegan]

At 11:45 +0000 on 12 Dec (1481543145), Andrew Cooper wrote:
> On 12/12/16 11:27, Tim Deegan wrote:
> > At 10:43 +0000 on 12 Dec (1481539421), Andrew Cooper wrote:
> >> PV guests necessarily can't be external, as Xen must steal address space from
> >> them.  Pagefaults for HVM guests are handled by {vmx,svm}_vmexit_handler() and
> >> don't enter the PV fixup_page_fault() path.
> >>
> >> This paging_fault() callsite is therefore dead code, so drop it.
> >>
> >> Clarify the comment at the other paging_fault() callsite to indicate that it
> >> covers the logdirty case only.
> >>
> >> No functional change.
> > IMO this is a change, just not on any supported config.
> 
> Really?  I'd agree if the content of the clause was reachable (and we
> didn't support the configuration required to make it reachable), but my
> argument here is that it is genuinely dead code and cannot be reached.

Ah, you're right - I was thinking of refcounts mode.  External mode
guests can't get here.  In which case...

> >> +    /*
> >> +     * For non-external shadowed guests (i.e. PV guests with logdirty
> >> +     * active), we fix up both their own pagefaults and Xen's, since
> >> +     * they share the pagetables.
> >> +     */
> >>      if ( paging_mode_enabled(d) && !paging_mode_external(d) )
> > Here we can drop the check of !paging_mode_external(d), or maybe turn
> > it into an assertion somewhere.
> >
> > With that,
> >
> > Acked-by: Tim Deegan <tim@xen.org>
> 
> Seeing as both you and Jan have asked, I will see about addition an
> assertion.  However, it will have to be in patch 3 for bisectability,
> when we rule out the use of PV refcount guests.  Are you ok with that?

...I don't follow you here -- since external-mode guests can't reach this
code we can surely switch from test to assert.

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/paging: Enforce PG_external == PG_translate == PG_refcounts

[Andrew Cooper]

On 12/12/16 11:43, Tim Deegan wrote:
> At 10:43 +0000 on 12 Dec (1481539423), Andrew Cooper wrote:
>> Setting PG_refcounts but not PG_translate is not useful.
>>
>> While adjusting this, make a few other improvements.
>>
>>  * Have paging_enable() unilaterally reject any unknown modes.
>>  * Correct the function description.  paging_enable() is also used to enable
>>    logdirty during runtime.
> Don't run-time log-dirty changes go through paging_log_dirty_{en,dis}able()?

You are completely right.  Sorry for that.  I failed to observe that
logdirty gets pulled out before XEN_DOMCTL_SHADOW_OP_ENABLE is
propagated into shadow_domctl()

I will drop the comment adjustments.

>
> Apart from that, Acked-by: Tim Deegan <tim@xen.org>.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/paging: Enforce PG_external == PG_translate == PG_refcounts

[Jan Beulich]

>>> On 12.12.16 at 12:59, <andrew.cooper3@citrix.com> wrote:
> On 12/12/16 11:43, Tim Deegan wrote:
>> At 10:43 +0000 on 12 Dec (1481539423), Andrew Cooper wrote:
>>> Setting PG_refcounts but not PG_translate is not useful.
>>>
>>> While adjusting this, make a few other improvements.
>>>
>>>  * Have paging_enable() unilaterally reject any unknown modes.
>>>  * Correct the function description.  paging_enable() is also used to enable
>>>    logdirty during runtime.
>> Don't run-time log-dirty changes go through paging_log_dirty_{en,dis}able()?
> 
> You are completely right.  Sorry for that.  I failed to observe that
> logdirty gets pulled out before XEN_DOMCTL_SHADOW_OP_ENABLE is
> propagated into shadow_domctl()
> 
> I will drop the comment adjustments.

With that feel free to also add
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/traps: Drop paging_mode_external() handling from the PV pagefault path

[Andrew Cooper]

On 12/12/16 11:57, Tim Deegan wrote:
> At 11:45 +0000 on 12 Dec (1481543145), Andrew Cooper wrote:
>> On 12/12/16 11:27, Tim Deegan wrote:
>>> At 10:43 +0000 on 12 Dec (1481539421), Andrew Cooper wrote:
>>>> PV guests necessarily can't be external, as Xen must steal address space from
>>>> them.  Pagefaults for HVM guests are handled by {vmx,svm}_vmexit_handler() and
>>>> don't enter the PV fixup_page_fault() path.
>>>>
>>>> This paging_fault() callsite is therefore dead code, so drop it.
>>>>
>>>> Clarify the comment at the other paging_fault() callsite to indicate that it
>>>> covers the logdirty case only.
>>>>
>>>> No functional change.
>>> IMO this is a change, just not on any supported config.
>> Really?  I'd agree if the content of the clause was reachable (and we
>> didn't support the configuration required to make it reachable), but my
>> argument here is that it is genuinely dead code and cannot be reached.
> Ah, you're right - I was thinking of refcounts mode.  External mode
> guests can't get here.  In which case...
>
>>>> +    /*
>>>> +     * For non-external shadowed guests (i.e. PV guests with logdirty
>>>> +     * active), we fix up both their own pagefaults and Xen's, since
>>>> +     * they share the pagetables.
>>>> +     */
>>>>      if ( paging_mode_enabled(d) && !paging_mode_external(d) )
>>> Here we can drop the check of !paging_mode_external(d), or maybe turn
>>> it into an assertion somewhere.
>>>
>>> With that,
>>>
>>> Acked-by: Tim Deegan <tim@xen.org>
>> Seeing as both you and Jan have asked, I will see about addition an
>> assertion.  However, it will have to be in patch 3 for bisectability,
>> when we rule out the use of PV refcount guests.  Are you ok with that?
> ...I don't follow you here -- since external-mode guests can't reach this
> code we can surely switch from test to assert.

My plan was to add something like this:

if ( paging_mode_enabled(d) )
    ASSERT(paging_mode_only_logdirty(d));

for a suitable new predicate.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/traps: Drop paging_mode_external() handling from the PV pagefault path

[Tim Deegan]

At 13:05 +0000 on 12 Dec (1481547944), Andrew Cooper wrote:
> On 12/12/16 11:57, Tim Deegan wrote:
> > At 11:45 +0000 on 12 Dec (1481543145), Andrew Cooper wrote:
> >> On 12/12/16 11:27, Tim Deegan wrote:
> >>> At 10:43 +0000 on 12 Dec (1481539421), Andrew Cooper wrote:
> >>>> PV guests necessarily can't be external, as Xen must steal address space from
> >>>> them.  Pagefaults for HVM guests are handled by {vmx,svm}_vmexit_handler() and
> >>>> don't enter the PV fixup_page_fault() path.
> >>>>
> >>>> This paging_fault() callsite is therefore dead code, so drop it.
> >>>>
> >>>> Clarify the comment at the other paging_fault() callsite to indicate that it
> >>>> covers the logdirty case only.
> >>>>
> >>>> No functional change.
> >>> IMO this is a change, just not on any supported config.
> >> Really?  I'd agree if the content of the clause was reachable (and we
> >> didn't support the configuration required to make it reachable), but my
> >> argument here is that it is genuinely dead code and cannot be reached.
> > Ah, you're right - I was thinking of refcounts mode.  External mode
> > guests can't get here.  In which case...
> >
> >>>> +    /*
> >>>> +     * For non-external shadowed guests (i.e. PV guests with logdirty
> >>>> +     * active), we fix up both their own pagefaults and Xen's, since
> >>>> +     * they share the pagetables.
> >>>> +     */
> >>>>      if ( paging_mode_enabled(d) && !paging_mode_external(d) )
> >>> Here we can drop the check of !paging_mode_external(d), or maybe turn
> >>> it into an assertion somewhere.
> >>>
> >>> With that,
> >>>
> >>> Acked-by: Tim Deegan <tim@xen.org>
> >> Seeing as both you and Jan have asked, I will see about addition an
> >> assertion.  However, it will have to be in patch 3 for bisectability,
> >> when we rule out the use of PV refcount guests.  Are you ok with that?
> > ...I don't follow you here -- since external-mode guests can't reach this
> > code we can surely switch from test to assert.
> 
> My plan was to add something like this:
> 
> if ( paging_mode_enabled(d) )
>     ASSERT(paging_mode_only_logdirty(d));
> 
> for a suitable new predicate.

That sounds fine.  Order it whatever way is most convenient, so long
as this !paging_mode_external(d) disappears at some point. :)

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel