diff mbox

[v4,04/27] x86: move PV invalid op emulation code

Message ID 20170621085731.xrmxq3lao4hd4od3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Liu June 21, 2017, 8:57 a.m. UTC
On Wed, Jun 21, 2017 at 12:15:46AM -0600, Jan Beulich wrote:
> >>> On 20.06.17 at 18:25, <wei.liu2@citrix.com> wrote:
> > On Tue, Jun 20, 2017 at 10:21:27AM -0600, Jan Beulich wrote:
> >> >>> On 08.06.17 at 19:11, <wei.liu2@citrix.com> wrote:
> >> > @@ -1053,8 +982,8 @@ void do_invalid_op(struct cpu_user_regs *regs)
> >> >  
> >> >      if ( likely(guest_mode(regs)) )
> >> >      {
> >> > -        if ( !emulate_invalid_rdtscp(regs) &&
> >> > -             !emulate_forced_invalid_op(regs) )
> >> > +        if ( !pv_emulate_invalid_rdtscp(regs) &&
> >> > +             !pv_emulate_forced_invalid_op(regs) )
> >> 
> >> I wonder if the first couldn't be called by the second, making it
> >> unnecessary to export both. Or maybe have a wrapper
> >> pv_emulate_invalid_op() around both.
> >> 
> > 
> > Do you want me to refactor and move code in the same patch? Wouldn't
> > that make it hard for you to review?
> 
> Why - especially in the wrapper variant you'd move both functions
> unchanged (perhaps even with the names left as they are), and
> merely add the wrapper (and of course use it in the code fragment
> above). That'll make review rather simple, as you'll still be able to
> state that you left both existing functions unchanged.

OK

---8<---
From 50dfe1fe116c28a3953f0b72acc7b1dee4136e2b Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Mon, 5 Jun 2017 13:07:16 +0100
Subject: [PATCH] x86: move PV invalid op emulation code

Move the code to pv/emul-inv-op.c. Both functions are unchanged.
Provide pv_emulate_invalid_op and use it in traps.c.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/pv/Makefile       |   1 +
 xen/arch/x86/pv/emul-inv-op.c  | 128 +++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/traps.c           |  74 +-----------------------
 xen/include/asm-x86/pv/traps.h |   2 +
 4 files changed, 132 insertions(+), 73 deletions(-)
 create mode 100644 xen/arch/x86/pv/emul-inv-op.c

Comments

Jan Beulich June 21, 2017, 9:09 a.m. UTC | #1
>>> On 21.06.17 at 10:57, <wei.liu2@citrix.com> wrote:
> +int pv_emulate_invalid_op(struct cpu_user_regs *regs)
> +{
> +    return !emulate_invalid_rdtscp(regs) && !emulate_forced_invalid_op(regs);
> +}

This way you want to make the function return bool. Alternatively
you would want to preserve the EXCRET_* return value here, and
handle it accordingly in the caller.

Jan
Wei Liu June 21, 2017, 9:14 a.m. UTC | #2
On Wed, Jun 21, 2017 at 03:09:41AM -0600, Jan Beulich wrote:
> >>> On 21.06.17 at 10:57, <wei.liu2@citrix.com> wrote:
> > +int pv_emulate_invalid_op(struct cpu_user_regs *regs)
> > +{
> > +    return !emulate_invalid_rdtscp(regs) && !emulate_forced_invalid_op(regs);
> > +}
> 
> This way you want to make the function return bool. Alternatively
> you would want to preserve the EXCRET_* return value here, and
> handle it accordingly in the caller.
> 

I will just make it return bool. Do you want me to send another version?
Jan Beulich June 21, 2017, 9:26 a.m. UTC | #3
>>> On 21.06.17 at 11:14, <wei.liu2@citrix.com> wrote:
> On Wed, Jun 21, 2017 at 03:09:41AM -0600, Jan Beulich wrote:
>> >>> On 21.06.17 at 10:57, <wei.liu2@citrix.com> wrote:
>> > +int pv_emulate_invalid_op(struct cpu_user_regs *regs)
>> > +{
>> > +    return !emulate_invalid_rdtscp(regs) && !emulate_forced_invalid_op(regs);
>> > +}
>> 
>> This way you want to make the function return bool. Alternatively
>> you would want to preserve the EXCRET_* return value here, and
>> handle it accordingly in the caller.
>> 
> 
> I will just make it return bool. Do you want me to send another version?

No need to I think - feel free to add my ack.

Jan
Wei Liu June 21, 2017, 9:29 a.m. UTC | #4
On Wed, Jun 21, 2017 at 03:26:12AM -0600, Jan Beulich wrote:
> >>> On 21.06.17 at 11:14, <wei.liu2@citrix.com> wrote:
> > On Wed, Jun 21, 2017 at 03:09:41AM -0600, Jan Beulich wrote:
> >> >>> On 21.06.17 at 10:57, <wei.liu2@citrix.com> wrote:
> >> > +int pv_emulate_invalid_op(struct cpu_user_regs *regs)
> >> > +{
> >> > +    return !emulate_invalid_rdtscp(regs) && !emulate_forced_invalid_op(regs);
> >> > +}
> >> 
> >> This way you want to make the function return bool. Alternatively
> >> you would want to preserve the EXCRET_* return value here, and
> >> handle it accordingly in the caller.
> >> 
> > 
> > I will just make it return bool. Do you want me to send another version?
> 
> No need to I think - feel free to add my ack.
> 

Cool. Thank you.
diff mbox

Patch

diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
index 1f6fbd3f5c..42ca64dc9e 100644
--- a/xen/arch/x86/pv/Makefile
+++ b/xen/arch/x86/pv/Makefile
@@ -5,5 +5,6 @@  obj-bin-y += dom0_build.init.o
 obj-y += domain.o
 obj-y += emulate.o
 obj-y += emul-gate-op.o
+obj-y += emul-inv-op.o
 obj-y += emul-priv-op.o
 obj-bin-y += gpr_switch.o
diff --git a/xen/arch/x86/pv/emul-inv-op.c b/xen/arch/x86/pv/emul-inv-op.c
new file mode 100644
index 0000000000..a1c56da171
--- /dev/null
+++ b/xen/arch/x86/pv/emul-inv-op.c
@@ -0,0 +1,128 @@ 
+/******************************************************************************
+ * arch/x86/pv/emul-inv-op.c
+ *
+ * Emulate invalid op for PV guests
+ *
+ * Modifications to Linux original are copyright (c) 2002-2004, K A Fraser
+ *
+ * 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/errno.h>
+#include <xen/event.h>
+#include <xen/guest_access.h>
+#include <xen/iocap.h>
+#include <xen/spinlock.h>
+#include <xen/trace.h>
+
+#include <asm/apic.h>
+#include <asm/debugreg.h>
+#include <asm/hpet.h>
+#include <asm/hypercall.h>
+#include <asm/mc146818rtc.h>
+#include <asm/p2m.h>
+#include <asm/pv/traps.h>
+#include <asm/shared.h>
+#include <asm/traps.h>
+#include <asm/x86_emulate.h>
+
+#include <xsm/xsm.h>
+
+#include "emulate.h"
+
+static int emulate_invalid_rdtscp(struct cpu_user_regs *regs)
+{
+    char opcode[3];
+    unsigned long eip, rc;
+    struct vcpu *v = current;
+
+    eip = regs->rip;
+    if ( (rc = copy_from_user(opcode, (char *)eip, sizeof(opcode))) != 0 )
+    {
+        pv_inject_page_fault(0, eip + sizeof(opcode) - rc);
+        return EXCRET_fault_fixed;
+    }
+    if ( memcmp(opcode, "\xf\x1\xf9", sizeof(opcode)) )
+        return 0;
+    eip += sizeof(opcode);
+    pv_soft_rdtsc(v, regs, 1);
+    pv_emul_instruction_done(regs, eip);
+    return EXCRET_fault_fixed;
+}
+
+static int emulate_forced_invalid_op(struct cpu_user_regs *regs)
+{
+    char sig[5], instr[2];
+    unsigned long eip, rc;
+    struct cpuid_leaf res;
+
+    eip = regs->rip;
+
+    /* Check for forced emulation signature: ud2 ; .ascii "xen". */
+    if ( (rc = copy_from_user(sig, (char *)eip, sizeof(sig))) != 0 )
+    {
+        pv_inject_page_fault(0, eip + sizeof(sig) - rc);
+        return EXCRET_fault_fixed;
+    }
+    if ( memcmp(sig, "\xf\xbxen", sizeof(sig)) )
+        return 0;
+    eip += sizeof(sig);
+
+    /* We only emulate CPUID. */
+    if ( ( rc = copy_from_user(instr, (char *)eip, sizeof(instr))) != 0 )
+    {
+        pv_inject_page_fault(0, eip + sizeof(instr) - rc);
+        return EXCRET_fault_fixed;
+    }
+    if ( memcmp(instr, "\xf\xa2", sizeof(instr)) )
+        return 0;
+
+    /* If cpuid faulting is enabled and CPL>0 inject a #GP in place of #UD. */
+    if ( current->arch.cpuid_faulting && !guest_kernel_mode(current, regs) )
+    {
+        regs->rip = eip;
+        pv_inject_hw_exception(TRAP_gp_fault, regs->error_code);
+        return EXCRET_fault_fixed;
+    }
+
+    eip += sizeof(instr);
+
+    guest_cpuid(current, regs->eax, regs->ecx, &res);
+
+    regs->rax = res.a;
+    regs->rbx = res.b;
+    regs->rcx = res.c;
+    regs->rdx = res.d;
+
+    pv_emul_instruction_done(regs, eip);
+
+    trace_trap_one_addr(TRC_PV_FORCED_INVALID_OP, regs->rip);
+
+    return EXCRET_fault_fixed;
+}
+
+int pv_emulate_invalid_op(struct cpu_user_regs *regs)
+{
+    return !emulate_invalid_rdtscp(regs) && !emulate_forced_invalid_op(regs);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 7b781f17db..88dfd464e7 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -968,77 +968,6 @@  void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
     }
 }
 
-static int emulate_invalid_rdtscp(struct cpu_user_regs *regs)
-{
-    char opcode[3];
-    unsigned long eip, rc;
-    struct vcpu *v = current;
-
-    eip = regs->rip;
-    if ( (rc = copy_from_user(opcode, (char *)eip, sizeof(opcode))) != 0 )
-    {
-        pv_inject_page_fault(0, eip + sizeof(opcode) - rc);
-        return EXCRET_fault_fixed;
-    }
-    if ( memcmp(opcode, "\xf\x1\xf9", sizeof(opcode)) )
-        return 0;
-    eip += sizeof(opcode);
-    pv_soft_rdtsc(v, regs, 1);
-    pv_emul_instruction_done(regs, eip);
-    return EXCRET_fault_fixed;
-}
-
-static int emulate_forced_invalid_op(struct cpu_user_regs *regs)
-{
-    char sig[5], instr[2];
-    unsigned long eip, rc;
-    struct cpuid_leaf res;
-
-    eip = regs->rip;
-
-    /* Check for forced emulation signature: ud2 ; .ascii "xen". */
-    if ( (rc = copy_from_user(sig, (char *)eip, sizeof(sig))) != 0 )
-    {
-        pv_inject_page_fault(0, eip + sizeof(sig) - rc);
-        return EXCRET_fault_fixed;
-    }
-    if ( memcmp(sig, "\xf\xbxen", sizeof(sig)) )
-        return 0;
-    eip += sizeof(sig);
-
-    /* We only emulate CPUID. */
-    if ( ( rc = copy_from_user(instr, (char *)eip, sizeof(instr))) != 0 )
-    {
-        pv_inject_page_fault(0, eip + sizeof(instr) - rc);
-        return EXCRET_fault_fixed;
-    }
-    if ( memcmp(instr, "\xf\xa2", sizeof(instr)) )
-        return 0;
-
-    /* If cpuid faulting is enabled and CPL>0 inject a #GP in place of #UD. */
-    if ( current->arch.cpuid_faulting && !guest_kernel_mode(current, regs) )
-    {
-        regs->rip = eip;
-        pv_inject_hw_exception(TRAP_gp_fault, regs->error_code);
-        return EXCRET_fault_fixed;
-    }
-
-    eip += sizeof(instr);
-
-    guest_cpuid(current, regs->eax, regs->ecx, &res);
-
-    regs->rax = res.a;
-    regs->rbx = res.b;
-    regs->rcx = res.c;
-    regs->rdx = res.d;
-
-    pv_emul_instruction_done(regs, eip);
-
-    trace_trap_one_addr(TRC_PV_FORCED_INVALID_OP, regs->rip);
-
-    return EXCRET_fault_fixed;
-}
-
 void do_invalid_op(struct cpu_user_regs *regs)
 {
     const struct bug_frame *bug = NULL;
@@ -1053,8 +982,7 @@  void do_invalid_op(struct cpu_user_regs *regs)
 
     if ( likely(guest_mode(regs)) )
     {
-        if ( !emulate_invalid_rdtscp(regs) &&
-             !emulate_forced_invalid_op(regs) )
+        if ( pv_emulate_invalid_op(regs) )
             pv_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         return;
     }
diff --git a/xen/include/asm-x86/pv/traps.h b/xen/include/asm-x86/pv/traps.h
index b1b6b1d0ad..458028a94b 100644
--- a/xen/include/asm-x86/pv/traps.h
+++ b/xen/include/asm-x86/pv/traps.h
@@ -27,11 +27,13 @@ 
 
 int pv_emulate_privileged_op(struct cpu_user_regs *regs);
 void pv_emulate_gate_op(struct cpu_user_regs *regs);
+int pv_emulate_invalid_op(struct cpu_user_regs *regs);
 
 #else  /* !CONFIG_PV */
 
 static inline int pv_emulate_privileged_op(struct cpu_user_regs *regs) { return 0; }
 static inline void pv_emulate_gate_op(struct cpu_user_regs *regs) {}
+static int pv_emulate_invalid_op(struct cpu_user_regs *regs) { return 0; }
 
 #endif /* CONFIG_PV */