Re: [Xen-devel] [PATCH] pvh: Fix regression caused by assumption that HVM paths MUST use io-backend device.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Feb 07, 2014 at 02:28:07AM +0000, Zhang, Yang Z wrote:
> Konrad Rzeszutek Wilk wrote on 2014-02-05:
> > On Wed, Feb 05, 2014 at 02:35:51PM +0000, George Dunlap wrote:
> >> On 02/04/2014 04:42 PM, Konrad Rzeszutek Wilk wrote:
> >>> On Tue, Feb 04, 2014 at 03:46:48PM +0000, Jan Beulich wrote:
> >>>>>>> On 04.02.14 at 16:32, Konrad Rzeszutek Wilk
> > <konrad.wilk@xxxxxxxxxx> wrote:
> >>>>> On Tue, Feb 04, 2014 at 03:02:44PM +0000, Jan Beulich wrote:
> >>>>>> Wasn't it that Mukesh's patch simply was yours with the two
> >>>>>> get_ioreq()s folded by using a local variable?
> >>>>> Yes. As so
> >>>> Thanks. Except that ...
> >>>> 
> >>>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> >>>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> >>>>> @@ -1394,13 +1394,13 @@ void nvmx_switch_guest(void)
> >>>>>      struct vcpu *v = current;
> >>>>>      struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
> >>>>>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> >>>>> -
> >>>>> +    ioreq_t *p = get_ioreq(v);
> >>>> ... you don't want to drop the blank line, and naming the new
> >>>> variable "ioreq" would seem preferable.
> >>>> 
> >>>>>      /*
> >>>>>       * a pending IO emualtion may still no finished. In this case,
> >>>>>       * no virtual vmswith is allowed. Or else, the following IO
> >>>>>       * emulation will handled in a wrong VCPU context.
> >>>>>       */
> >>>>> -    if ( get_ioreq(v)->state != STATE_IOREQ_NONE )
> >>>>> +    if ( p && p->state != STATE_IOREQ_NONE )
> >>>> And, as said before, I'd think "!p ||" instead of "p &&" would be
> >>>> the right thing here. Yang, Jun?
> >>> I have two patches - one the simpler one that is pretty
> >>> straightfoward and the one you suggested. Either one fixes PVH
> >>> guests. I also did bootup tests with HVM guests to make sure they worked.
> >>> 
> >>> Attached and inline.
> >> 
> 
> Sorry for the late response. I just back from Chinese new year holiday.
> 
> >> But they do different things -- one does "ioreq && ioreq->state..."
> > 
> > Correct.
> >> and the other does "!ioreq || ioreq->state...".  The first one is
> >> incorrect, AFAICT.
> > 
> > Both of them fix the hypervisor blowing up with any PVH guest.
> 
> Both of fixings are right to me.
> The only concern is that what we want to do here:
> "ioreq && ioreq->state..." will only allow the VCPU that supporting IO 
> request emulation mechanism to continue nested check which current means HVM 
> VCPU.
> And "!ioreq || ioreq->state..." will check the VCPU that doesn't support the 
> IO request emulation mechanism only which current means PVH VCPU.
> 
> The purpose of my original patch only wants to allow the HVM VCPU that 
> doesn't has pending IO request to continue nested check. Not use it to 
> distinguish whether it is HVM or PVH. So here I prefer to only allow HVM VCPU 
> goes to here as Jan mentioned before that non-HVM domain should never call 
> nested related function at all unless it also supports nested.

So it sounds like the #2 patch is preferable by you.

Can I stick Acked-by on it?


From d76fc0d2f59ac65bd692adfa5f215da9ecf85d6a Mon Sep 17 00:00:00 2001
From: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
Date: Mon, 3 Feb 2014 11:45:52 -0500
Subject: [PATCH] pvh: Fix regression due to assumption that HVM paths MUST
 use io-backend device.

The commit 09bb434748af9bfe3f7fca4b6eef721a7d5042a4
"Nested VMX: prohibit virtual vmentry/vmexit during IO emulation"
assumes that the HVM paths are only taken by HVM guests. With the PVH
enabled that is no longer the case - which means that we do not have
to have the IO-backend device (QEMU) enabled.

As such, that patch can crash the hypervisor:

Xen call trace:
    [<ffff82d0801ddd9a>] nvmx_switch_guest+0x4d/0x903
    [<ffff82d0801de95b>] vmx_asm_vmexit_handler+0x4b/0xc0

Pagetable walk from 000000000000001e:
  L4[0x000] = 0000000000000000 ffffffffffffffff

****************************************
Panic on CPU 7:
FATAL PAGE FAULT
[error_code=0000]
Faulting linear address: 000000000000001e
****************************************

as we do not have an io based backend. In the case that the
PVH guest does run an HVM guest inside it - we need to do
further work to suport this - and for now the check will
bail us out.

We also fix spelling mistakes and the sentence structure.

CC: Yang Zhang <yang.z.zhang@xxxxxxxxx>
CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
Suggested-by: Jan Beulich <JBeulich@xxxxxxxx>
Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
 xen/arch/x86/hvm/vmx/vvmx.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d2ba435..71522cf 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1394,13 +1394,17 @@ void nvmx_switch_guest(void)
     struct vcpu *v = current;
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
     struct cpu_user_regs *regs = guest_cpu_user_regs();
+    ioreq_t *ioreq = get_ioreq(v);
 
     /*
-     * a pending IO emualtion may still no finished. In this case,
+     * A pending IO emulation may still be not finished. In this case,
      * no virtual vmswith is allowed. Or else, the following IO
-     * emulation will handled in a wrong VCPU context.
+     * emulation will be handled in a wrong VCPU context. If there are
+     * no IO backends - PVH guest by itself or a PVH guest with an HVM guest
+     * running inside - we don't want to continue as this setup is not
+     * implemented nor supported as of right now.
      */
-    if ( get_ioreq(v)->state != STATE_IOREQ_NONE )
+    if ( !ioreq || ioreq->state != STATE_IOREQ_NONE )
         return;
     /*
      * a softirq may interrupt us between a virtual vmentry is
-- 
1.7.7.6


> 
> Best regards,
> Yang
> 

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