[PATCH 0/3] x86/mm: merge ptwr and mmio_ro page fault handlers

[PATCH 0/3] x86/mm: merge ptwr and mmio_ro page fault handlers

[Wei Liu]

Wei Liu (3):
  x86/mm: introduce trace point for mmio_ro emulation
  x86/mm: don't wrap x86_emulate_ctxt in ptwr_emulate_ctxt
  x86/mm: merge ptwr and mmio_ro page fault handlers

 xen/arch/x86/mm.c                | 325 +++++++++++++++++++--------------------
 xen/arch/x86/traps.c             |  20 +--
 xen/include/asm-x86/mm.h         |   6 +-
 xen/include/asm-x86/perfc_defn.h |   1 +
 4 files changed, 171 insertions(+), 181 deletions(-)

-- 
2.11.0


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

[PATCH 1/3] x86/mm: introduce trace point for mmio_ro emulation

[Wei Liu]

Using ptrw_emulation trace point is wrong.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c                | 2 +-
 xen/include/asm-x86/perfc_defn.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1f23470cef..ed80df02fa 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5397,7 +5397,7 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
 
         /* Fallthrough */
     case X86EMUL_RETRY:
-        perfc_incr(ptwr_emulations);
+        perfc_incr(mmio_ro_emulations);
         return EXCRET_fault_fixed;
     }
 
diff --git a/xen/include/asm-x86/perfc_defn.h b/xen/include/asm-x86/perfc_defn.h
index aac9331843..6db8746906 100644
--- a/xen/include/asm-x86/perfc_defn.h
+++ b/xen/include/asm-x86/perfc_defn.h
@@ -30,6 +30,7 @@ PERFCOUNTER(copy_user_faults,       "copy_user faults")
 
 PERFCOUNTER(map_domain_page_count,  "map_domain_page count")
 PERFCOUNTER(ptwr_emulations,        "writable pt emulations")
+PERFCOUNTER(mmio_ro_emulations,     "mmio ro emulations")
 
 PERFCOUNTER(exception_fixed,        "pre-exception fixed")
 
-- 
2.11.0


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

[PATCH 2/3] x86/mm: don't wrap x86_emulate_ctxt in ptwr_emulate_ctxt

[Wei Liu]

Rewrite the code so that it has the same structure as
mmio_ro_emualte_ctxt. The new code doesn't contain x86_emulate_ctxt
anymore but a pointer to the x86_emulate_ctxt; x86_emulate_ctxt now
also points to ptwr_emulate_ctxt via its data pointer.

This patch will help unify mmio_ro and ptwr code paths later.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ed80df02fa..5b840cc603 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4956,9 +4956,9 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
  */
 
 struct ptwr_emulate_ctxt {
-    struct x86_emulate_ctxt ctxt;
     unsigned long cr2;
     l1_pgentry_t  pte;
+    struct x86_emulate_ctxt *ctxt;
 };
 
 static int ptwr_emulated_read(
@@ -5018,7 +5018,7 @@ static int ptwr_emulated_update(
         {
             x86_emul_pagefault(0, /* Read fault. */
                                addr + sizeof(paddr_t) - rc,
-                               &ptwr_ctxt->ctxt);
+                               ptwr_ctxt->ctxt);
             return X86EMUL_EXCEPTION;
         }
         /* Mask out bits provided by caller. */
@@ -5133,9 +5133,7 @@ static int ptwr_emulated_write(
 
     memcpy(&val, p_data, bytes);
 
-    return ptwr_emulated_update(
-        offset, 0, val, bytes, 0,
-        container_of(ctxt, struct ptwr_emulate_ctxt, ctxt));
+    return ptwr_emulated_update(offset, 0, val, bytes, 0, ctxt->data);
 }
 
 static int ptwr_emulated_cmpxchg(
@@ -5158,9 +5156,7 @@ static int ptwr_emulated_cmpxchg(
     memcpy(&old, p_old, bytes);
     memcpy(&new, p_new, bytes);
 
-    return ptwr_emulated_update(
-        offset, old, new, bytes, 1,
-        container_of(ctxt, struct ptwr_emulate_ctxt, ctxt));
+    return ptwr_emulated_update(offset, old, new, bytes, 1, ctxt->data);
 }
 
 static const struct x86_emulate_ops ptwr_emulate_ops = {
@@ -5179,14 +5175,14 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr,
     struct domain *d = v->domain;
     struct page_info *page;
     l1_pgentry_t      pte;
-    struct ptwr_emulate_ctxt ptwr_ctxt = {
-        .ctxt = {
-            .regs = regs,
-            .vendor = d->arch.cpuid->x86_vendor,
-            .addr_size = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
-            .sp_size   = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
-            .lma       = !is_pv_32bit_domain(d),
-        },
+    struct ptwr_emulate_ctxt ptwr_ctxt;
+    struct x86_emulate_ctxt ctxt = {
+       .regs = regs,
+       .vendor = d->arch.cpuid->x86_vendor,
+       .addr_size = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
+       .sp_size   = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
+       .lma       = !is_pv_32bit_domain(d),
+       .data      = &ptwr_ctxt,
     };
     int rc;
 
@@ -5213,10 +5209,13 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr,
         goto bail;
     }
 
-    ptwr_ctxt.cr2 = addr;
-    ptwr_ctxt.pte = pte;
+    ptwr_ctxt = (struct ptwr_emulate_ctxt) {
+        .cr2 = addr,
+        .pte = pte,
+        .ctxt = &ctxt
+    };
 
-    rc = x86_emulate(&ptwr_ctxt.ctxt, &ptwr_emulate_ops);
+    rc = x86_emulate(&ctxt, &ptwr_emulate_ops);
 
     page_unlock(page);
     put_page(page);
@@ -5231,18 +5230,18 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr,
          * emulation bug, or a guest playing with the instruction stream under
          * Xen's feet.
          */
-        if ( ptwr_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
-             ptwr_ctxt.ctxt.event.vector == TRAP_page_fault )
-            pv_inject_event(&ptwr_ctxt.ctxt.event);
+        if ( ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
+             ctxt.event.vector == TRAP_page_fault )
+            pv_inject_event(&ctxt.event);
         else
             gdprintk(XENLOG_WARNING,
                      "Unexpected event (type %u, vector %#x) from emulation\n",
-                     ptwr_ctxt.ctxt.event.type, ptwr_ctxt.ctxt.event.vector);
+                     ctxt.event.type, ctxt.event.vector);
 
         /* Fallthrough */
     case X86EMUL_OKAY:
 
-        if ( ptwr_ctxt.ctxt.retire.singlestep )
+        if ( ctxt.retire.singlestep )
             pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
 
         /* Fallthrough */
-- 
2.11.0


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

[PATCH 3/3] x86/mm: merge ptwr and mmio_ro page fault handlers

[Wei Liu]

Provide a unified entry to avoid going through pte look-up, decode and
emulation cycle more than necessary. The path taken is determined by
the faulting address.

Note that the order of checks is changed in the new function, but the
order of the checks is performed shouldn't matter.

The sole caller is changed to use the new function.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

xtf is happy with this change. Let me know if more tests can be done.
---
 xen/arch/x86/mm.c        | 316 +++++++++++++++++++++++------------------------
 xen/arch/x86/traps.c     |  20 +--
 xen/include/asm-x86/mm.h |   6 +-
 3 files changed, 166 insertions(+), 176 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5b840cc603..3879d4a54d 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5168,92 +5168,6 @@ static const struct x86_emulate_ops ptwr_emulate_ops = {
     .cpuid      = pv_emul_cpuid,
 };
 
-/* Write page fault handler: check if guest is trying to modify a PTE. */
-int ptwr_do_page_fault(struct vcpu *v, unsigned long addr,
-                       struct cpu_user_regs *regs)
-{
-    struct domain *d = v->domain;
-    struct page_info *page;
-    l1_pgentry_t      pte;
-    struct ptwr_emulate_ctxt ptwr_ctxt;
-    struct x86_emulate_ctxt ctxt = {
-       .regs = regs,
-       .vendor = d->arch.cpuid->x86_vendor,
-       .addr_size = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
-       .sp_size   = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG,
-       .lma       = !is_pv_32bit_domain(d),
-       .data      = &ptwr_ctxt,
-    };
-    int rc;
-
-    /* Attempt to read the PTE that maps the VA being accessed. */
-    guest_get_eff_l1e(addr, &pte);
-
-    /* We are looking only for read-only mappings of p.t. pages. */
-    if ( ((l1e_get_flags(pte) & (_PAGE_PRESENT|_PAGE_RW)) != _PAGE_PRESENT) ||
-         rangeset_contains_singleton(mmio_ro_ranges, l1e_get_pfn(pte)) ||
-         !get_page_from_mfn(l1e_get_mfn(pte), d) )
-        goto bail;
-
-    page = l1e_get_page(pte);
-    if ( !page_lock(page) )
-    {
-        put_page(page);
-        goto bail;
-    }
-
-    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
-    {
-        page_unlock(page);
-        put_page(page);
-        goto bail;
-    }
-
-    ptwr_ctxt = (struct ptwr_emulate_ctxt) {
-        .cr2 = addr,
-        .pte = pte,
-        .ctxt = &ctxt
-    };
-
-    rc = x86_emulate(&ctxt, &ptwr_emulate_ops);
-
-    page_unlock(page);
-    put_page(page);
-
-    switch ( rc )
-    {
-    case X86EMUL_EXCEPTION:
-        /*
-         * This emulation only covers writes to pagetables which are marked
-         * read-only by Xen.  We tolerate #PF (in case a concurrent pagetable
-         * update has succeeded on a different vcpu).  Anything else is an
-         * emulation bug, or a guest playing with the instruction stream under
-         * Xen's feet.
-         */
-        if ( ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
-             ctxt.event.vector == TRAP_page_fault )
-            pv_inject_event(&ctxt.event);
-        else
-            gdprintk(XENLOG_WARNING,
-                     "Unexpected event (type %u, vector %#x) from emulation\n",
-                     ctxt.event.type, ctxt.event.vector);
-
-        /* Fallthrough */
-    case X86EMUL_OKAY:
-
-        if ( ctxt.retire.singlestep )
-            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-
-        /* Fallthrough */
-    case X86EMUL_RETRY:
-        perfc_incr(ptwr_emulations);
-        return EXCRET_fault_fixed;
-    }
-
- bail:
-    return 0;
-}
-
 /*************************
  * fault handling for read-only MMIO pages
  */
@@ -5326,83 +5240,6 @@ static const struct x86_emulate_ops mmcfg_intercept_ops = {
     .cpuid      = pv_emul_cpuid,
 };
 
-/* Check if guest is trying to modify a r/o MMIO page. */
-int mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
-                          struct cpu_user_regs *regs)
-{
-    l1_pgentry_t pte;
-    unsigned long mfn;
-    unsigned int addr_size = is_pv_32bit_vcpu(v) ? 32 : BITS_PER_LONG;
-    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = addr };
-    struct x86_emulate_ctxt ctxt = {
-        .regs = regs,
-        .vendor = v->domain->arch.cpuid->x86_vendor,
-        .addr_size = addr_size,
-        .sp_size = addr_size,
-        .lma = !is_pv_32bit_vcpu(v),
-        .data = &mmio_ro_ctxt,
-    };
-    int rc;
-
-    /* Attempt to read the PTE that maps the VA being accessed. */
-    guest_get_eff_l1e(addr, &pte);
-
-    /* We are looking only for read-only mappings of MMIO pages. */
-    if ( ((l1e_get_flags(pte) & (_PAGE_PRESENT|_PAGE_RW)) != _PAGE_PRESENT) )
-        return 0;
-
-    mfn = l1e_get_pfn(pte);
-    if ( mfn_valid(_mfn(mfn)) )
-    {
-        struct page_info *page = mfn_to_page(_mfn(mfn));
-        struct domain *owner = page_get_owner_and_reference(page);
-
-        if ( owner )
-            put_page(page);
-        if ( owner != dom_io )
-            return 0;
-    }
-
-    if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn) )
-        return 0;
-
-    if ( pci_ro_mmcfg_decode(mfn, &mmio_ro_ctxt.seg, &mmio_ro_ctxt.bdf) )
-        rc = x86_emulate(&ctxt, &mmcfg_intercept_ops);
-    else
-        rc = x86_emulate(&ctxt, &mmio_ro_emulate_ops);
-
-    switch ( rc )
-    {
-    case X86EMUL_EXCEPTION:
-        /*
-         * This emulation only covers writes to MMCFG space or read-only MFNs.
-         * We tolerate #PF (from hitting an adjacent page or a successful
-         * concurrent pagetable update).  Anything else is an emulation bug,
-         * or a guest playing with the instruction stream under Xen's feet.
-         */
-        if ( ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
-             ctxt.event.vector == TRAP_page_fault )
-            pv_inject_event(&ctxt.event);
-        else
-            gdprintk(XENLOG_WARNING,
-                     "Unexpected event (type %u, vector %#x) from emulation\n",
-                     ctxt.event.type, ctxt.event.vector);
-
-        /* Fallthrough */
-    case X86EMUL_OKAY:
-
-        if ( ctxt.retire.singlestep )
-            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-
-        /* Fallthrough */
-    case X86EMUL_RETRY:
-        perfc_incr(mmio_ro_emulations);
-        return EXCRET_fault_fixed;
-    }
-
-    return 0;
-}
-
 void *alloc_xen_pagetable(void)
 {
     if ( system_state != SYS_STATE_early_boot )
@@ -6434,6 +6271,159 @@ void write_32bit_pse_identmap(uint32_t *l2)
                  _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);
 }
 
+/* Check if guest is trying to modify a r/o MMIO page. */
+static int mmio_ro_do_page_fault(struct x86_emulate_ctxt *ctxt,
+                                 unsigned long addr, l1_pgentry_t pte)
+{
+    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = addr };
+    mfn_t mfn = l1e_get_mfn(pte);
+    int rc;
+
+    if ( mfn_valid(mfn) )
+    {
+        struct page_info *page = mfn_to_page(mfn);
+        struct domain *owner = page_get_owner_and_reference(page);
+
+        if ( owner )
+            put_page(page);
+        if ( owner != dom_io )
+            goto bail;
+    }
+
+    ctxt->data = &mmio_ro_ctxt;
+    if ( pci_ro_mmcfg_decode(mfn_x(mfn), &mmio_ro_ctxt.seg, &mmio_ro_ctxt.bdf) )
+        rc = x86_emulate(ctxt, &mmcfg_intercept_ops);
+    else
+        rc = x86_emulate(ctxt, &mmio_ro_emulate_ops);
+
+    switch ( rc )
+    {
+    case X86EMUL_EXCEPTION:
+        /*
+         * This emulation only covers writes to MMCFG space or read-only MFNs.
+         * We tolerate #PF (from hitting an adjacent page or a successful
+         * concurrent pagetable update).  Anything else is an emulation bug,
+         * or a guest playing with the instruction stream under Xen's feet.
+         */
+        if ( ctxt->event.type == X86_EVENTTYPE_HW_EXCEPTION &&
+             ctxt->event.vector == TRAP_page_fault )
+            pv_inject_event(&ctxt->event);
+        else
+            gdprintk(XENLOG_WARNING,
+                     "Unexpected event (type %u, vector %#x) from emulation\n",
+                     ctxt->event.type, ctxt->event.vector);
+
+        /* Fallthrough */
+    case X86EMUL_OKAY:
+
+        if ( ctxt->retire.singlestep )
+            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+
+        /* Fallthrough */
+    case X86EMUL_RETRY:
+        perfc_incr(mmio_ro_emulations);
+        return EXCRET_fault_fixed;
+    }
+
+ bail:
+    return 0;
+}
+
+/* Write page fault handler: check if guest is trying to modify a PTE. */
+static int ptwr_do_page_fault(struct x86_emulate_ctxt *ctxt, struct domain *d,
+                              unsigned long addr, l1_pgentry_t pte)
+{
+    struct ptwr_emulate_ctxt ptwr_ctxt = {
+        .cr2 = addr,
+        .pte = pte,
+        .ctxt = ctxt
+    };
+    struct page_info *page;
+    int rc;
+
+    if ( !get_page_from_mfn(l1e_get_mfn(pte), d) )
+        goto bail;
+
+    page = l1e_get_page(pte);
+    if ( !page_lock(page) )
+    {
+        put_page(page);
+        goto bail;
+    }
+
+    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
+    {
+        page_unlock(page);
+        put_page(page);
+        goto bail;
+    }
+
+    ctxt->data = &ptwr_ctxt;
+    rc = x86_emulate(ctxt, &ptwr_emulate_ops);
+
+    page_unlock(page);
+    put_page(page);
+
+    switch ( rc )
+    {
+    case X86EMUL_EXCEPTION:
+        /*
+         * This emulation only covers writes to pagetables which are marked
+         * read-only by Xen.  We tolerate #PF (in case a concurrent pagetable
+         * update has succeeded on a different vcpu).  Anything else is an
+         * emulation bug, or a guest playing with the instruction stream under
+         * Xen's feet.
+         */
+        if ( ctxt->event.type == X86_EVENTTYPE_HW_EXCEPTION &&
+             ctxt->event.vector == TRAP_page_fault )
+            pv_inject_event(&ctxt->event);
+        else
+            gdprintk(XENLOG_WARNING,
+                     "Unexpected event (type %u, vector %#x) from emulation\n",
+                     ctxt->event.type, ctxt->event.vector);
+
+        /* Fallthrough */
+    case X86EMUL_OKAY:
+
+        if ( ctxt->retire.singlestep )
+            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+
+        /* Fallthrough */
+    case X86EMUL_RETRY:
+        perfc_incr(ptwr_emulations);
+        return EXCRET_fault_fixed;
+    }
+
+ bail:
+    return 0;
+}
+
+int ptwr_or_mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
+                                  struct cpu_user_regs *regs)
+{
+    l1_pgentry_t pte;
+    struct domain *d = v->domain;
+    unsigned int addr_size = is_pv_32bit_vcpu(v) ? 32 : BITS_PER_LONG;
+    struct x86_emulate_ctxt ctxt = {
+        .regs      = regs,
+        .vendor    = d->arch.cpuid->x86_vendor,
+        .addr_size = addr_size,
+        .sp_size   = addr_size,
+        .lma       = !is_pv_32bit_vcpu(v),
+    };
+
+    /* Attempt to read the PTE that maps the VA being accessed. */
+    guest_get_eff_l1e(addr, &pte);
+
+    if ( ((l1e_get_flags(pte) & (_PAGE_PRESENT | _PAGE_RW)) != _PAGE_PRESENT) )
+        return 0;
+
+    if ( rangeset_contains_singleton(mmio_ro_ranges, l1e_get_pfn(pte)) )
+        return mmio_ro_do_page_fault(&ctxt, addr, pte);
+    else
+        return ptwr_do_page_fault(&ctxt, d, addr, pte);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index f525fa28d3..6fba64cacd 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1308,16 +1308,18 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
          !(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
          (regs->error_code & PFEC_write_access) )
     {
-        if ( VM_ASSIST(d, writable_pagetables) &&
-             /* Do not check if access-protection fault since the page may
-                legitimately be not present in shadow page tables */
-             (paging_mode_enabled(d) ||
-              (regs->error_code & PFEC_page_present)) &&
-             ptwr_do_page_fault(v, addr, regs) )
-            return EXCRET_fault_fixed;
+        bool ptwr, mmio_ro;
+
+        ptwr = VM_ASSIST(d, writable_pagetables) &&
+               /* Do not check if access-protection fault since the page may
+                  legitimately be not present in shadow page tables */
+               (paging_mode_enabled(d) ||
+                (regs->error_code & PFEC_page_present));
+
+        mmio_ro = is_hardware_domain(d) &&
+                  (regs->error_code & PFEC_page_present);
 
-        if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
-             mmio_ro_do_page_fault(v, addr, regs) )
+        if ( (ptwr || mmio_ro) && ptwr_or_mmio_ro_do_page_fault(v, addr, regs) )
             return EXCRET_fault_fixed;
     }
 
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index ec7ce3c8c8..85adafdfa2 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -511,10 +511,8 @@ extern int mmcfg_intercept_write(enum x86_segment seg,
 int pv_emul_cpuid(uint32_t leaf, uint32_t subleaf,
                   struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt);
 
-int  ptwr_do_page_fault(struct vcpu *, unsigned long,
-                        struct cpu_user_regs *);
-int  mmio_ro_do_page_fault(struct vcpu *, unsigned long,
-                           struct cpu_user_regs *);
+int ptwr_or_mmio_ro_do_page_fault(struct vcpu *, unsigned long,
+                                  struct cpu_user_regs *);
 
 int audit_adjust_pgtables(struct domain *d, int dir, int noisy);
 
-- 
2.11.0


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

Re: [PATCH 2/3] x86/mm: don't wrap x86_emulate_ctxt in ptwr_emulate_ctxt

[Andrew Cooper]

On 30/08/17 18:11, Wei Liu wrote:
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index ed80df02fa..5b840cc603 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4956,9 +4956,9 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>   */
>  
>  struct ptwr_emulate_ctxt {
> -    struct x86_emulate_ctxt ctxt;
>      unsigned long cr2;
>      l1_pgentry_t  pte;
> +    struct x86_emulate_ctxt *ctxt;

You can do away with this pointer entirely if you modify
ptwr_emulated_update() to take the full x86_emulate_ctxt.  Locally, you
can just declare

struct ptwr_emulate_ctxt *ptwr_ctxt = ctxt->data;

Otherwise, LGTM.

~Andrew

>  };
>  
>  static int ptwr_emulated_read(
>


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

Re: [PATCH 3/3] x86/mm: merge ptwr and mmio_ro page fault handlers

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

On 30/08/17 18:11, Wei Liu wrote:
> Provide a unified entry to avoid going through pte look-up, decode and
> emulation cycle more than necessary. The path taken is determined by
> the faulting address.
>
> Note that the order of checks is changed in the new function, but the
> order of the checks is performed shouldn't matter.
>
> The sole caller is changed to use the new function.
>
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> xtf is happy with this change. Let me know if more tests can be done.

Booting dom0 is probably the best easy test going.

> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index ec7ce3c8c8..85adafdfa2 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -511,10 +511,8 @@ extern int mmcfg_intercept_write(enum x86_segment seg,
>  int pv_emul_cpuid(uint32_t leaf, uint32_t subleaf,
>                    struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt);
>  
> -int  ptwr_do_page_fault(struct vcpu *, unsigned long,
> -                        struct cpu_user_regs *);
> -int  mmio_ro_do_page_fault(struct vcpu *, unsigned long,
> -                           struct cpu_user_regs *);
> +int ptwr_or_mmio_ro_do_page_fault(struct vcpu *, unsigned long,
> +                                  struct cpu_user_regs *);

pv_ro_page_fault() ?

>  
>  int audit_adjust_pgtables(struct domain *d, int dir, int noisy);
>  

I was thinking something more like this extra delta (only compile
tested) which drops rather more code.

~Andrew


[-- Attachment #2: 0001-extra.patch --]
[-- Type: text/x-patch, Size: 6601 bytes --]

>From 5c9d0935e2079935aaf980d972a00d8d5e8197e3 Mon Sep 17 00:00:00 2001
From: Andrew Cooper <andrew.cooper3@citrix.com>
Date: Wed, 30 Aug 2017 18:50:47 +0100
Subject: [PATCH] extra

---
 xen/arch/x86/mm.c | 129 ++++++++++++++++++++----------------------------------
 1 file changed, 47 insertions(+), 82 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 3879d4a..753a775 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6277,7 +6277,6 @@ static int mmio_ro_do_page_fault(struct x86_emulate_ctxt *ctxt,
 {
     struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = addr };
     mfn_t mfn = l1e_get_mfn(pte);
-    int rc;
 
     if ( mfn_valid(mfn) )
     {
@@ -6287,46 +6286,14 @@ static int mmio_ro_do_page_fault(struct x86_emulate_ctxt *ctxt,
         if ( owner )
             put_page(page);
         if ( owner != dom_io )
-            goto bail;
+            return X86EMUL_UNHANDLEABLE;
     }
 
     ctxt->data = &mmio_ro_ctxt;
     if ( pci_ro_mmcfg_decode(mfn_x(mfn), &mmio_ro_ctxt.seg, &mmio_ro_ctxt.bdf) )
-        rc = x86_emulate(ctxt, &mmcfg_intercept_ops);
+        return x86_emulate(ctxt, &mmcfg_intercept_ops);
     else
-        rc = x86_emulate(ctxt, &mmio_ro_emulate_ops);
-
-    switch ( rc )
-    {
-    case X86EMUL_EXCEPTION:
-        /*
-         * This emulation only covers writes to MMCFG space or read-only MFNs.
-         * We tolerate #PF (from hitting an adjacent page or a successful
-         * concurrent pagetable update).  Anything else is an emulation bug,
-         * or a guest playing with the instruction stream under Xen's feet.
-         */
-        if ( ctxt->event.type == X86_EVENTTYPE_HW_EXCEPTION &&
-             ctxt->event.vector == TRAP_page_fault )
-            pv_inject_event(&ctxt->event);
-        else
-            gdprintk(XENLOG_WARNING,
-                     "Unexpected event (type %u, vector %#x) from emulation\n",
-                     ctxt->event.type, ctxt->event.vector);
-
-        /* Fallthrough */
-    case X86EMUL_OKAY:
-
-        if ( ctxt->retire.singlestep )
-            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-
-        /* Fallthrough */
-    case X86EMUL_RETRY:
-        perfc_incr(mmio_ro_emulations);
-        return EXCRET_fault_fixed;
-    }
-
- bail:
-    return 0;
+        return x86_emulate(ctxt, &mmio_ro_emulate_ops);
 }
 
 /* Write page fault handler: check if guest is trying to modify a PTE. */
@@ -6336,66 +6303,31 @@ static int ptwr_do_page_fault(struct x86_emulate_ctxt *ctxt, struct domain *d,
     struct ptwr_emulate_ctxt ptwr_ctxt = {
         .cr2 = addr,
         .pte = pte,
-        .ctxt = ctxt
+        .ctxt = ctxt,
     };
     struct page_info *page;
-    int rc;
+    int rc = X86EMUL_UNHANDLEABLE;
 
     if ( !get_page_from_mfn(l1e_get_mfn(pte), d) )
-        goto bail;
+        goto out;
 
     page = l1e_get_page(pte);
     if ( !page_lock(page) )
-    {
-        put_page(page);
-        goto bail;
-    }
+        goto put;
 
     if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
-    {
-        page_unlock(page);
-        put_page(page);
-        goto bail;
-    }
+        goto unlock;
 
     ctxt->data = &ptwr_ctxt;
     rc = x86_emulate(ctxt, &ptwr_emulate_ops);
 
+ unlock:
     page_unlock(page);
+ put:
     put_page(page);
+ out:
 
-    switch ( rc )
-    {
-    case X86EMUL_EXCEPTION:
-        /*
-         * This emulation only covers writes to pagetables which are marked
-         * read-only by Xen.  We tolerate #PF (in case a concurrent pagetable
-         * update has succeeded on a different vcpu).  Anything else is an
-         * emulation bug, or a guest playing with the instruction stream under
-         * Xen's feet.
-         */
-        if ( ctxt->event.type == X86_EVENTTYPE_HW_EXCEPTION &&
-             ctxt->event.vector == TRAP_page_fault )
-            pv_inject_event(&ctxt->event);
-        else
-            gdprintk(XENLOG_WARNING,
-                     "Unexpected event (type %u, vector %#x) from emulation\n",
-                     ctxt->event.type, ctxt->event.vector);
-
-        /* Fallthrough */
-    case X86EMUL_OKAY:
-
-        if ( ctxt->retire.singlestep )
-            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
-
-        /* Fallthrough */
-    case X86EMUL_RETRY:
-        perfc_incr(ptwr_emulations);
-        return EXCRET_fault_fixed;
-    }
-
- bail:
-    return 0;
+    return rc;
 }
 
 int ptwr_or_mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
@@ -6404,6 +6336,7 @@ int ptwr_or_mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
     l1_pgentry_t pte;
     struct domain *d = v->domain;
     unsigned int addr_size = is_pv_32bit_vcpu(v) ? 32 : BITS_PER_LONG;
+    int rc;
     struct x86_emulate_ctxt ctxt = {
         .regs      = regs,
         .vendor    = d->arch.cpuid->x86_vendor,
@@ -6415,13 +6348,45 @@ int ptwr_or_mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
     /* Attempt to read the PTE that maps the VA being accessed. */
     guest_get_eff_l1e(addr, &pte);
 
+    /* We are looking only for read-only mappings. */
     if ( ((l1e_get_flags(pte) & (_PAGE_PRESENT | _PAGE_RW)) != _PAGE_PRESENT) )
         return 0;
 
     if ( rangeset_contains_singleton(mmio_ro_ranges, l1e_get_pfn(pte)) )
-        return mmio_ro_do_page_fault(&ctxt, addr, pte);
+        rc = mmio_ro_do_page_fault(&ctxt, addr, pte);
     else
-        return ptwr_do_page_fault(&ctxt, d, addr, pte);
+        rc = ptwr_do_page_fault(&ctxt, d, addr, pte);
+
+    switch ( rc )
+    {
+    case X86EMUL_EXCEPTION:
+        /*
+         * This emulation covers writes to:
+         *  - L1 pagetables.
+         *  - MMCFG space or read-only MFNs.
+         * We tolerate #PF (from hitting an adjacent page or a successful
+         * concurrent pagetable update).  Anything else is an emulation bug,
+         * or a guest playing with the instruction stream under Xen's feet.
+         */
+        if ( ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
+             ctxt.event.vector == TRAP_page_fault )
+            pv_inject_event(&ctxt.event);
+        else
+            gdprintk(XENLOG_WARNING,
+                     "Unexpected event (type %u, vector %#x) from emulation\n",
+                     ctxt.event.type, ctxt.event.vector);
+
+        /* Fallthrough */
+    case X86EMUL_OKAY:
+        if ( ctxt.retire.singlestep )
+            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+
+        /* Fallthrough */
+    case X86EMUL_RETRY:
+        return EXCRET_fault_fixed;
+    }
+
+    return 0;
 }
 
 /*
-- 
2.1.4


[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] x86/mm: introduce trace point for mmio_ro emulation

[Jan Beulich]

>>> On 30.08.17 at 19:11, <wei.liu2@citrix.com> wrote:
> Using ptrw_emulation trace point is wrong.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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



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

Re: [PATCH 3/3] x86/mm: merge ptwr and mmio_ro page fault handlers

[Wei Liu]

On Wed, Aug 30, 2017 at 07:02:52PM +0100, Andrew Cooper wrote:
> On 30/08/17 18:11, Wei Liu wrote:
> > Provide a unified entry to avoid going through pte look-up, decode and
> > emulation cycle more than necessary. The path taken is determined by
> > the faulting address.
> >
> > Note that the order of checks is changed in the new function, but the
> > order of the checks is performed shouldn't matter.
> >
> > The sole caller is changed to use the new function.
> >
> > No functional change.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> > xtf is happy with this change. Let me know if more tests can be done.
> 
> Booting dom0 is probably the best easy test going.
> 
> > diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> > index ec7ce3c8c8..85adafdfa2 100644
> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -511,10 +511,8 @@ extern int mmcfg_intercept_write(enum x86_segment seg,
> >  int pv_emul_cpuid(uint32_t leaf, uint32_t subleaf,
> >                    struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt);
> >  
> > -int  ptwr_do_page_fault(struct vcpu *, unsigned long,
> > -                        struct cpu_user_regs *);
> > -int  mmio_ro_do_page_fault(struct vcpu *, unsigned long,
> > -                           struct cpu_user_regs *);
> > +int ptwr_or_mmio_ro_do_page_fault(struct vcpu *, unsigned long,
> > +                                  struct cpu_user_regs *);
> 
> pv_ro_page_fault() ?
> 
> >  
> >  int audit_adjust_pgtables(struct domain *d, int dir, int noisy);
> >  
> 
> I was thinking something more like this extra delta (only compile
> tested) which drops rather more code.

Your diff deleted two perfc_incr. I will modify it to restore that
behaviour.

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