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

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

[Wei Liu]

Address Andrew's comments and rebased on top of staging.

Wei Liu (2):
  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        | 302 +++++++++++++++++++++--------------------------
 xen/arch/x86/traps.c     |  20 ++--
 xen/include/asm-x86/mm.h |   5 +-
 3 files changed, 145 insertions(+), 182 deletions(-)

-- 
2.11.0


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

[PATCH v2 1/2] 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. x86_emulate_ctxt now 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>
---
v2: do away with pointer in ptwr_emulate_ctxt
---
 xen/arch/x86/mm.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f4b9747aba..3306088255 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4964,7 +4964,6 @@ 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;
 };
@@ -4995,7 +4994,7 @@ static int ptwr_emulated_update(
     paddr_t val,
     unsigned int bytes,
     unsigned int do_cmpxchg,
-    struct ptwr_emulate_ctxt *ptwr_ctxt)
+    struct x86_emulate_ctxt *ctxt)
 {
     unsigned long mfn;
     unsigned long unaligned_addr = addr;
@@ -5003,6 +5002,7 @@ static int ptwr_emulated_update(
     l1_pgentry_t pte, ol1e, nl1e, *pl1e;
     struct vcpu *v = current;
     struct domain *d = v->domain;
+    struct ptwr_emulate_ctxt *ptwr_ctxt = ctxt->data;
     int ret;
 
     /* Only allow naturally-aligned stores within the original %cr2 page. */
@@ -5026,7 +5026,7 @@ static int ptwr_emulated_update(
         {
             x86_emul_pagefault(0, /* Read fault. */
                                addr + sizeof(paddr_t) - rc,
-                               &ptwr_ctxt->ctxt);
+                               ctxt);
             return X86EMUL_EXCEPTION;
         }
         /* Mask out bits provided by caller. */
@@ -5141,9 +5141,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);
 }
 
 static int ptwr_emulated_cmpxchg(
@@ -5166,9 +5164,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);
 }
 
 static const struct x86_emulate_ops ptwr_emulate_ops = {
@@ -5187,14 +5183,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;
 
@@ -5221,10 +5217,12 @@ 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,
+    };
 
-    rc = x86_emulate(&ptwr_ctxt.ctxt, &ptwr_emulate_ops);
+    rc = x86_emulate(&ctxt, &ptwr_emulate_ops);
 
     page_unlock(page);
     put_page(page);
@@ -5239,18 +5237,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 v2 2/2] 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>

v2: rebase and (sorta) merge Andrew's changes

Dom0 boots fine.  XTF is happy with this change. Let me know if more
tests can be done.
---
 xen/arch/x86/mm.c        | 290 +++++++++++++++++++++--------------------------
 xen/arch/x86/traps.c     |  20 ++--
 xen/include/asm-x86/mm.h |   5 +-
 3 files changed, 140 insertions(+), 175 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 3306088255..429442aa1d 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5176,91 +5176,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. */
-    pte = guest_get_eff_l1e(addr);
-
-    /* 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,
-    };
-
-    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
  */
@@ -5333,83 +5248,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. */
-    pte = guest_get_eff_l1e(addr);
-
-    /* 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 )
@@ -6441,6 +6279,134 @@ 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);
+
+    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 )
+            return X86EMUL_UNHANDLEABLE;
+    }
+
+    ctxt->data = &mmio_ro_ctxt;
+    if ( pci_ro_mmcfg_decode(mfn_x(mfn), &mmio_ro_ctxt.seg, &mmio_ro_ctxt.bdf) )
+        return x86_emulate(ctxt, &mmcfg_intercept_ops);
+    else
+        return x86_emulate(ctxt, &mmio_ro_emulate_ops);
+}
+
+/* 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,
+    };
+    struct page_info *page;
+    int rc = X86EMUL_UNHANDLEABLE;
+
+    if ( !get_page_from_mfn(l1e_get_mfn(pte), d) )
+        goto out;
+
+    page = l1e_get_page(pte);
+    if ( !page_lock(page) )
+    {
+        put_page(page);
+        goto out;
+    }
+
+    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
+    {
+        page_unlock(page);
+        put_page(page);
+        goto out;
+    }
+
+    ctxt->data = &ptwr_ctxt;
+    rc = x86_emulate(ctxt, &ptwr_emulate_ops);
+
+    page_unlock(page);
+    put_page(page);
+
+ out:
+    return rc;
+}
+
+int pv_ro_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),
+    };
+    int rc;
+    bool mmio_ro;
+
+    /* Attempt to read the PTE that maps the VA being accessed. */
+    pte = guest_get_eff_l1e(addr);
+
+    /* We are only looking for read-only mappings */
+    if ( ((l1e_get_flags(pte) & (_PAGE_PRESENT | _PAGE_RW)) != _PAGE_PRESENT) )
+        return 0;
+
+    mmio_ro = rangeset_contains_singleton(mmio_ro_ranges, l1e_get_pfn(pte));
+    if ( mmio_ro )
+        rc = mmio_ro_do_page_fault(&ctxt, addr, pte);
+    else
+        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:
+        if ( mmio_ro )
+            perfc_incr(mmio_ro_emulations);
+        else
+            perfc_incr(ptwr_emulations);
+        return EXCRET_fault_fixed;
+    }
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index f525fa28d3..8cdd3c892a 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) && pv_ro_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 4c03a33e79..2dc4898bed 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -511,10 +511,7 @@ 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 pv_ro_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 v2 1/2] x86/mm: don't wrap x86_emulate_ctxt in ptwr_emulate_ctxt

[Andrew Cooper]

On 31/08/17 12:22, Wei Liu wrote:
> Rewrite the code so that it has the same structure as
> mmio_ro_emualte_ctxt. x86_emulate_ctxt now 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>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

[Jan Beulich]

>>> On 31.08.17 at 13:22, <wei.liu2@citrix.com> wrote:
> @@ -5221,10 +5217,12 @@ 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,
> +    };

I don't see the need for this re-write.

Jan


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

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

[Jan Beulich]

>>> On 31.08.17 at 13:22, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5176,91 +5176,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. */
> -    pte = guest_get_eff_l1e(addr);
> -
> -    /* 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,
> -    };
> -
> -    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
>   */

Note how you move the remains of the function above below this
comment, which isn't really correct.

> @@ -6441,6 +6279,134 @@ 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);
> +
> +    if ( mfn_valid(mfn) )
> +    {
> +        struct page_info *page = mfn_to_page(mfn);
> +        struct domain *owner = page_get_owner_and_reference(page);

Please add const as you move this.

> +        if ( owner )
> +            put_page(page);
> +        if ( owner != dom_io )
> +            return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    ctxt->data = &mmio_ro_ctxt;
> +    if ( pci_ro_mmcfg_decode(mfn_x(mfn), &mmio_ro_ctxt.seg, &mmio_ro_ctxt.bdf) )
> +        return x86_emulate(ctxt, &mmcfg_intercept_ops);
> +    else
> +        return x86_emulate(ctxt, &mmio_ro_emulate_ops);
> +}
> +
> +/* 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,
> +    };
> +    struct page_info *page;
> +    int rc = X86EMUL_UNHANDLEABLE;
> +
> +    if ( !get_page_from_mfn(l1e_get_mfn(pte), d) )
> +        goto out;
> +
> +    page = l1e_get_page(pte);
> +    if ( !page_lock(page) )
> +    {
> +        put_page(page);
> +        goto out;
> +    }
> +
> +    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
> +    {
> +        page_unlock(page);
> +        put_page(page);
> +        goto out;
> +    }
> +
> +    ctxt->data = &ptwr_ctxt;
> +    rc = x86_emulate(ctxt, &ptwr_emulate_ops);
> +
> +    page_unlock(page);
> +    put_page(page);
> +
> + out:
> +    return rc;
> +}

Could you please avoid goto when the exit path is trivial?

> +int pv_ro_page_fault(struct vcpu *v, unsigned long addr,

Since you alter this and the caller anyway, the first parameter
could as well become const struct domain *, simplifying ...

> +                     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),

... both is_pv_32bit_vcpu() accesses here (which internally use
v->domain). In fact I wonder whether it wouldn't yield more
consistent code if you didn't pass in the domain at all, as this
must strictly be current->domain, or invoking x86_emulate() and
various other functions is bogus. You'd then latch this into currd
here.

Furthermore I think this second use could become "addr_size > 32".

Jan

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

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

[Wei Liu]

On Fri, Sep 01, 2017 at 03:38:49AM -0600, Jan Beulich wrote:
> >  /*************************
> >   * fault handling for read-only MMIO pages
> >   */
> 
> Note how you move the remains of the function above below this
> comment, which isn't really correct.

I can place the new function where the old was.
> 
[..]
> > +int pv_ro_page_fault(struct vcpu *v, unsigned long addr,
> 
> Since you alter this and the caller anyway, the first parameter
> could as well become const struct domain *, simplifying ...
> 
> > +                     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),
> 
> ... both is_pv_32bit_vcpu() accesses here (which internally use
> v->domain). In fact I wonder whether it wouldn't yield more
> consistent code if you didn't pass in the domain at all, as this
> must strictly be current->domain, or invoking x86_emulate() and
> various other functions is bogus. You'd then latch this into currd
> here.
> 
> Furthermore I think this second use could become "addr_size > 32".
> 

Sorry, second use of what?

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

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

[Wei Liu]

On Fri, Sep 01, 2017 at 02:46:27PM +0100, Wei Liu wrote:
> On Fri, Sep 01, 2017 at 03:38:49AM -0600, Jan Beulich wrote:
> > >  /*************************
> > >   * fault handling for read-only MMIO pages
> > >   */
> > 
> > Note how you move the remains of the function above below this
> > comment, which isn't really correct.
> 
> I can place the new function where the old was.
> > 
> [..]
> > > +int pv_ro_page_fault(struct vcpu *v, unsigned long addr,
> > 
> > Since you alter this and the caller anyway, the first parameter
> > could as well become const struct domain *, simplifying ...
> > 
> > > +                     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),
> > 
> > ... both is_pv_32bit_vcpu() accesses here (which internally use
> > v->domain). In fact I wonder whether it wouldn't yield more
> > consistent code if you didn't pass in the domain at all, as this
> > must strictly be current->domain, or invoking x86_emulate() and
> > various other functions is bogus. You'd then latch this into currd
> > here.
> > 
> > Furthermore I think this second use could become "addr_size > 32".
> > 
> 
> Sorry, second use of what?

Oh, you mean

   .lma = addr_size > 32

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

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

[Jan Beulich]

>>> On 01.09.17 at 15:50, <wei.liu2@citrix.com> wrote:
> On Fri, Sep 01, 2017 at 02:46:27PM +0100, Wei Liu wrote:
>> On Fri, Sep 01, 2017 at 03:38:49AM -0600, Jan Beulich wrote:
>> > > +                     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),
>> > 
>> > ... both is_pv_32bit_vcpu() accesses here (which internally use
>> > v->domain). In fact I wonder whether it wouldn't yield more
>> > consistent code if you didn't pass in the domain at all, as this
>> > must strictly be current->domain, or invoking x86_emulate() and
>> > various other functions is bogus. You'd then latch this into currd
>> > here.
>> > 
>> > Furthermore I think this second use could become "addr_size > 32".
>> > 
>> 
>> Sorry, second use of what?
> 
> Oh, you mean
> 
>    .lma = addr_size > 32

Yes.

Jan


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