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

login
register
mail settings
Submitter Andrew Cooper
Date Aug. 30, 2017, 6:02 p.m.
Message ID <500b696f-4dfd-a101-900b-229e34d6b6f1@citrix.com>
Download mbox | patch
Permalink /patch/328227/
State New
Headers show

Comments

Andrew Cooper - Aug. 30, 2017, 6:02 p.m.
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
Wei Liu - Aug. 31, 2017, 9:14 a.m.
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.

Patch

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