diff mbox

[v4,12/31] x86/mm: split out readonly MMIO emulation code

Message ID 20170817144456.18989-13-wei.liu2@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Liu Aug. 17, 2017, 2:44 p.m. UTC
Move the code to pv/emul-mmio-op.c. Fix coding style issues while
moving.

Note that mmio_ro_emulated_write is needed by both PV and HVM, so it
is left in x86/mm.c.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c              | 129 --------------------------------
 xen/arch/x86/pv/Makefile       |   1 +
 xen/arch/x86/pv/emul-mmio-op.c | 166 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+), 129 deletions(-)
 create mode 100644 xen/arch/x86/pv/emul-mmio-op.c

Comments

Jan Beulich Aug. 24, 2017, 3:16 p.m. UTC | #1
>>> On 17.08.17 at 16:44, <wei.liu2@citrix.com> wrote:
> Move the code to pv/emul-mmio-op.c. Fix coding style issues while
> moving.
> 
> Note that mmio_ro_emulated_write is needed by both PV and HVM, so it
> is left in x86/mm.c.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/mm.c              | 129 --------------------------------
>  xen/arch/x86/pv/Makefile       |   1 +
>  xen/arch/x86/pv/emul-mmio-op.c | 166 +++++++++++++++++++++++++++++++++++++++++

Again I think just mmio.c would do. Other comments on earlier
patches apply here as well.

Jan
Andrew Cooper Aug. 24, 2017, 3:25 p.m. UTC | #2
On 24/08/17 16:16, Jan Beulich wrote:
>>>> On 17.08.17 at 16:44, <wei.liu2@citrix.com> wrote:
>> Move the code to pv/emul-mmio-op.c. Fix coding style issues while
>> moving.
>>
>> Note that mmio_ro_emulated_write is needed by both PV and HVM, so it
>> is left in x86/mm.c.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  xen/arch/x86/mm.c              | 129 --------------------------------
>>  xen/arch/x86/pv/Makefile       |   1 +
>>  xen/arch/x86/pv/emul-mmio-op.c | 166 +++++++++++++++++++++++++++++++++++++++++
> Again I think just mmio.c would do. Other comments on earlier
> patches apply here as well.

I think it would be wise to merge the ptwr and mmio handling.  At the
moment, we invoke a full lookup pte/decode/try-to-emulate cycle twice in
the #PF handler for PV guests before handing the fault back to the guest.

The correct ops and context can be determined by inspecting the l1e
under %cr2 before calling into any emulation code.

Simplifying this logic before moving it would be the better option.

~Andrew
Wei Liu Aug. 30, 2017, 2:35 p.m. UTC | #3
On Thu, Aug 24, 2017 at 04:25:23PM +0100, Andrew Cooper wrote:
> On 24/08/17 16:16, Jan Beulich wrote:
> >>>> On 17.08.17 at 16:44, <wei.liu2@citrix.com> wrote:
> >> Move the code to pv/emul-mmio-op.c. Fix coding style issues while
> >> moving.
> >>
> >> Note that mmio_ro_emulated_write is needed by both PV and HVM, so it
> >> is left in x86/mm.c.
> >>
> >> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >> ---
> >>  xen/arch/x86/mm.c              | 129 --------------------------------
> >>  xen/arch/x86/pv/Makefile       |   1 +
> >>  xen/arch/x86/pv/emul-mmio-op.c | 166 +++++++++++++++++++++++++++++++++++++++++
> > Again I think just mmio.c would do. Other comments on earlier
> > patches apply here as well.
> 
> I think it would be wise to merge the ptwr and mmio handling.  At the
> moment, we invoke a full lookup pte/decode/try-to-emulate cycle twice in
> the #PF handler for PV guests before handing the fault back to the guest.
> 
> The correct ops and context can be determined by inspecting the l1e
> under %cr2 before calling into any emulation code.

I will see what I can do. The predicates in traps.c look terribly
complicated.
diff mbox

Patch

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 3c0aa52f38..a42720c8d1 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4785,11 +4785,6 @@  long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     return 0;
 }
 
-
-/*************************
- * fault handling for read-only MMIO pages
- */
-
 int mmio_ro_emulated_write(
     enum x86_segment seg,
     unsigned long offset,
@@ -4811,130 +4806,6 @@  int mmio_ro_emulated_write(
     return X86EMUL_OKAY;
 }
 
-static const struct x86_emulate_ops mmio_ro_emulate_ops = {
-    .read       = x86emul_unhandleable_rw,
-    .insn_fetch = pv_emul_ptwr_read,
-    .write      = mmio_ro_emulated_write,
-    .validate   = pv_emul_is_mem_write,
-    .cpuid      = pv_emul_cpuid,
-};
-
-int mmcfg_intercept_write(
-    enum x86_segment seg,
-    unsigned long offset,
-    void *p_data,
-    unsigned int bytes,
-    struct x86_emulate_ctxt *ctxt)
-{
-    struct mmio_ro_emulate_ctxt *mmio_ctxt = ctxt->data;
-
-    /*
-     * Only allow naturally-aligned stores no wider than 4 bytes to the
-     * original %cr2 address.
-     */
-    if ( ((bytes | offset) & (bytes - 1)) || bytes > 4 || !bytes ||
-         offset != mmio_ctxt->cr2 )
-    {
-        gdprintk(XENLOG_WARNING, "bad write (cr2=%lx, addr=%lx, bytes=%u)\n",
-                mmio_ctxt->cr2, offset, bytes);
-        return X86EMUL_UNHANDLEABLE;
-    }
-
-    offset &= 0xfff;
-    if ( pci_conf_write_intercept(mmio_ctxt->seg, mmio_ctxt->bdf,
-                                  offset, bytes, p_data) >= 0 )
-        pci_mmcfg_write(mmio_ctxt->seg, PCI_BUS(mmio_ctxt->bdf),
-                        PCI_DEVFN2(mmio_ctxt->bdf), offset, bytes,
-                        *(uint32_t *)p_data);
-
-    return X86EMUL_OKAY;
-}
-
-static const struct x86_emulate_ops mmcfg_intercept_ops = {
-    .read       = x86emul_unhandleable_rw,
-    .insn_fetch = pv_emul_ptwr_read,
-    .write      = mmcfg_intercept_write,
-    .validate   = pv_emul_is_mem_write,
-    .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. */
-    pv_get_guest_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);
-        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(ptwr_emulations);
-        return EXCRET_fault_fixed;
-    }
-
-    return 0;
-}
-
 void *alloc_xen_pagetable(void)
 {
     if ( system_state != SYS_STATE_early_boot )
diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
index cbd890c5f2..016b1b6e8f 100644
--- a/xen/arch/x86/pv/Makefile
+++ b/xen/arch/x86/pv/Makefile
@@ -3,6 +3,7 @@  obj-y += domain.o
 obj-y += emulate.o
 obj-y += emul-gate-op.o
 obj-y += emul-inv-op.o
+obj-y += emul-mmio-op.o
 obj-y += emul-priv-op.o
 obj-y += emul-ptwr-op.o
 obj-y += hypercall.o
diff --git a/xen/arch/x86/pv/emul-mmio-op.c b/xen/arch/x86/pv/emul-mmio-op.c
new file mode 100644
index 0000000000..ee5c684777
--- /dev/null
+++ b/xen/arch/x86/pv/emul-mmio-op.c
@@ -0,0 +1,166 @@ 
+/******************************************************************************
+ * arch/x86/emul-mmio-op.c
+ *
+ * Readonly MMIO emulation for PV guests
+ *
+ * Copyright (c) 2002-2005 K A Fraser
+ * Copyright (c) 2004 Christian Limpach
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/rangeset.h>
+#include <xen/sched.h>
+
+#include <asm/domain.h>
+#include <asm/mm.h>
+#include <asm/pci.h>
+#include <asm/pv/mm.h>
+
+#include "emulate.h"
+
+/*************************
+ * fault handling for read-only MMIO pages
+ */
+
+static const struct x86_emulate_ops mmio_ro_emulate_ops = {
+    .read       = x86emul_unhandleable_rw,
+    .insn_fetch = pv_emul_ptwr_read,
+    .write      = mmio_ro_emulated_write,
+    .validate   = pv_emul_is_mem_write,
+    .cpuid      = pv_emul_cpuid,
+};
+
+int mmcfg_intercept_write(enum x86_segment seg, unsigned long offset,
+                          void *p_data, unsigned int bytes,
+                          struct x86_emulate_ctxt *ctxt)
+{
+    struct mmio_ro_emulate_ctxt *mmio_ctxt = ctxt->data;
+
+    /*
+     * Only allow naturally-aligned stores no wider than 4 bytes to the
+     * original %cr2 address.
+     */
+    if ( ((bytes | offset) & (bytes - 1)) || bytes > 4 || !bytes ||
+         offset != mmio_ctxt->cr2 )
+    {
+        gdprintk(XENLOG_WARNING, "bad write (cr2=%lx, addr=%lx, bytes=%u)\n",
+                mmio_ctxt->cr2, offset, bytes);
+        return X86EMUL_UNHANDLEABLE;
+    }
+
+    offset &= 0xfff;
+    if ( pci_conf_write_intercept(mmio_ctxt->seg, mmio_ctxt->bdf,
+                                  offset, bytes, p_data) >= 0 )
+        pci_mmcfg_write(mmio_ctxt->seg, PCI_BUS(mmio_ctxt->bdf),
+                        PCI_DEVFN2(mmio_ctxt->bdf), offset, bytes,
+                        *(uint32_t *)p_data);
+
+    return X86EMUL_OKAY;
+}
+
+static const struct x86_emulate_ops mmcfg_intercept_ops = {
+    .read       = x86emul_unhandleable_rw,
+    .insn_fetch = pv_emul_ptwr_read,
+    .write      = mmcfg_intercept_write,
+    .validate   = pv_emul_is_mem_write,
+    .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. */
+    pv_get_guest_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);
+        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(ptwr_emulations);
+        return EXCRET_fault_fixed;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */