[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [V10 PATCH 11/23] PVH xen: support invalid op emulation for PVH



On 08/08/13 02:49, Mukesh Rathor wrote:
On Wed, 7 Aug 2013 12:29:13 +0100
George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:

On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
<mukesh.rathor@xxxxxxxxxx> wrote:
This patch supports invalid op emulation for PVH by calling
appropriate copy macros and and HVM function to inject PF.

Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
  xen/arch/x86/traps.c        |   17 ++++++++++++++---
  xen/include/asm-x86/traps.h |    1 +
  2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 378ef0a..a3ca70b 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -459,6 +459,11 @@ static void instruction_done(
      struct cpu_user_regs *regs, unsigned long eip, unsigned int
bpmatch) {
      regs->eip = eip;
+
+    /* PVH fixme: debug trap below */
+    if ( is_pvh_vcpu(current) )
+        return;
What exactly does this comment mean?  Do you mean, "FIXME: Make debug
trapping work for PVH guests"?  (i.e., this functionality will be
implemented later?)
Correct, future work. Look at what the db trap is doing and make
it work for PVH if it doesn't already.

+
      regs->eflags &= ~X86_EFLAGS_RF;
      if ( bpmatch || (regs->eflags & X86_EFLAGS_TF) )
      {
@@ -913,7 +918,7 @@ static int emulate_invalid_rdtscp(struct
cpu_user_regs *regs) return EXCRET_fault_fixed;
  }

-static int emulate_forced_invalid_op(struct cpu_user_regs *regs)
+int emulate_forced_invalid_op(struct cpu_user_regs *regs)
Why make this non-static?  No one is using this in this patch.  If a
later patch needs it, you should make it non-static there, so we can
decide at that point if making it non-static is merited or not.
Sigh! Originally, it was that way, but then to keep that patch from
getting too big, it got moved here after few versions. We are making
emulation available for outside the PV, ie, to PVH.

As far as I'm concerned, the size of the patch itself is immaterial; the only important question, regarding how to break down patches (just like in breaking down functions), is how easy or difficult it is to understand the whole thing.

Now it's typically the case that long patches are hard to understand, and that breaking them down into smaller chunks makes them easier to read. But a division like this, where you've moved some random hunk into a different patch with which it has no logical relation, makes the series *harder* to understand, not easier.

Additionally, as the series evolves, it makes it difficult to keep all of the dependencies straight. Suppose you changed your approach for that future patch so that you didn't need this public anymore. You, and all the reviewers, could easily forget about the dependency, since it's in a separate patch which may have already been classified as "OK".

It's like taking a function named foo() and breaking it down into foo_1() and foo_2(). You're making the function shorter, but achieving the opposite of what having short functions is supposed to achieve. :-)


+    if ( (rc = raw_copy_from_guest(sig, (char *)eip,
sizeof(sig))) != 0 ) {
          propagate_page_fault(eip + sizeof(sig) - rc, 0);
          return EXCRET_fault_fixed;
@@ -931,7 +936,7 @@ static int emulate_forced_invalid_op(struct
cpu_user_regs *regs) eip += sizeof(sig);

      /* We only emulate CPUID. */
-    if ( ( rc = copy_from_user(instr, (char *)eip,
sizeof(instr))) != 0 )
+    if ( ( rc = raw_copy_from_guest(instr, (char *)eip,
sizeof(instr))) != 0 ) {
          propagate_page_fault(eip + sizeof(instr) - rc, 0);
          return EXCRET_fault_fixed;
@@ -1076,6 +1081,12 @@ void propagate_page_fault(unsigned long
addr, u16 error_code) struct vcpu *v = current;
      struct trap_bounce *tb = &v->arch.pv_vcpu.trap_bounce;

+    if ( is_pvh_vcpu(v) )
+    {
+        hvm_inject_page_fault(error_code, addr);
+        return;
+    }
Would it make more sense to rename this function
"pv_inject_page_fault", and then make a macro to switch between the
two?
I don't think so, propagate_page_fault seems generic enough.

What I meant was something similar to what I suggested for patch 10 -- make propagate_page_fault() truly generic, by making it check what mode is running and calling either pv_inject_page_fault() or hvm_inject_page_fault() as appropriate.

 -George

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.