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

[PATCH v2 0/3] 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.

All important change is in patch 3.

Andrew Cooper (3):
  x86/paging: Enforce PG_external == PG_translate == PG_refcounts
  x86/shadow: Drop all emulation for PV vcpus
  x86/traps: Adjust paged-guest handling in the PV pagefault path

 xen/arch/x86/mm/paging.c        |  13 +++--
 xen/arch/x86/mm/shadow/common.c | 112 +++++++---------------------------------
 xen/arch/x86/mm/shadow/multi.c  |  21 ++------
 xen/arch/x86/traps.c            |  16 ++----
 xen/include/asm-x86/paging.h    |   6 +++
 5 files changed, 42 insertions(+), 126 deletions(-)

-- 
2.1.4


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

[PATCH 1/3] 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.
 * Drop the or'ing of PG_{HAP,SH}_enable.  The underlying functions already do
   this.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>

v2:
 * Drop references to logdirty
---
 xen/arch/x86/mm/paging.c        | 13 +++++++++----
 xen/arch/x86/mm/shadow/common.c |  3 +--
 xen/include/asm-x86/paging.h    |  3 +++
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 853a035..4437611 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -846,19 +846,24 @@ void paging_final_teardown(struct domain *d)
  * creation. */
 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 acb35c2..79bb12e 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..cc29f9b 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))
-- 
2.1.4


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

[PATCH 2/3] 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>
Acked-by: Tim Deegan <tim@xen.org>
---
 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 79bb12e..0ba4153 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 d25ead1..336d24f 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3378,12 +3378,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(
@@ -3443,12 +3438,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 */
     /*
@@ -3489,12 +3479,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 3/3] x86/traps: Adjust paged-guest handling in 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.  Therefore, the first call to
paging_fault() is dead, and dropped.

Logdirty mode is now the only paging mode we should ever find a PV guest with,
so add a new predicate and assertion to this fact.

Drop the final reference to paging_mode_external().  It is more accurately now
only for logdirty guests.

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 +++++-----------
 xen/include/asm-x86/paging.h |  3 +++
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 013e633..f76aec2 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1799,14 +1799,9 @@ 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;
-    }
+    /* Logdirty mode is the only expected paging mode for PV guests. */
+    if ( paging_mode_enabled(d) )
+        ASSERT(paging_mode_only_log_dirty(d));
 
     if ( !(regs->error_code & PFEC_page_present) &&
           (pagefault_by_memadd(addr, regs)) )
@@ -1838,9 +1833,8 @@ 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. */
-    if ( paging_mode_enabled(d) && !paging_mode_external(d) )
+    /* Logdirty guests call back into the paging code to update shadows. */
+    if ( paging_mode_log_dirty(d) )
     {
         int ret = paging_fault(addr, regs);
         if ( ret == EXCRET_fault_fixed )
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index cc29f9b..2243aa1 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -69,6 +69,9 @@
 #define paging_mode_translate(_d) (!!((_d)->arch.paging.mode & PG_translate))
 #define paging_mode_external(_d)  (!!((_d)->arch.paging.mode & PG_external))
 
+#define paging_mode_only_log_dirty(_d)                  \
+    (((_d)->arch.paging.mode & PG_MASK) == PG_log_dirty)
+
 /* flags used for paging debug */
 #define PAGING_DEBUG_LOGDIRTY 0
 
-- 
2.1.4


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

Re: [PATCH 3/3] x86/traps: Adjust paged-guest handling in the PV pagefault path

[Tim Deegan]

At 16:54 +0000 on 13 Dec (1481648074), 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.  Therefore, the first call to
> paging_fault() is dead, and dropped.
> 
> Logdirty mode is now the only paging mode we should ever find a PV guest with,
> so add a new predicate and assertion to this fact.
> 
> Drop the final reference to paging_mode_external().  It is more accurately now
> only for logdirty guests.
> 
> 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/3] x86/traps: Adjust paged-guest handling in the PV pagefault path

[Jan Beulich]

>>> On 13.12.16 at 17:54, <andrew.cooper3@citrix.com> 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.  Therefore, the first call to
> paging_fault() is dead, and dropped.
> 
> Logdirty mode is now the only paging mode we should ever find a PV guest with,
> so add a new predicate and assertion to this fact.
> 
> Drop the final reference to paging_mode_external().  It is more accurately now
> only for logdirty guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


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