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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 6392 bytes --]

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@oracle.com> 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.


>From 47a5554201f0bc1778e5bcbde8c39088707f727f Mon Sep 17 00:00:00 2001
From: Mukesh Rathor <mukesh.rathor@oracle.com>
Date: Mon, 3 Feb 2014 11:45:52 -0500
Subject: [PATCH] pvh: Fix regression caused by 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.

CC: Yang Zhang <yang.z.zhang@Intel.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d2ba435..563b02f 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1394,13 +1394,14 @@ 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,
      * 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 ( ioreq && ioreq->state != STATE_IOREQ_NONE )
         return;
     /*
      * a softirq may interrupt us between a virtual vmentry is
-- 
1.7.7.6



>From d76fc0d2f59ac65bd692adfa5f215da9ecf85d6a Mon Sep 17 00:00:00 2001
From: Mukesh Rathor <mukesh.rathor@oracle.com>
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@Intel.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 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

> 
> Jan
> 

[-- Attachment #2: 0001-pvh-Fix-regression-caused-by-assumption-that-HVM-pat.patch --]
[-- Type: text/plain, Size: 2151 bytes --]

>From 47a5554201f0bc1778e5bcbde8c39088707f727f Mon Sep 17 00:00:00 2001
From: Mukesh Rathor <mukesh.rathor@oracle.com>
Date: Mon, 3 Feb 2014 11:45:52 -0500
Subject: [PATCH] pvh: Fix regression caused by 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.

CC: Yang Zhang <yang.z.zhang@Intel.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d2ba435..563b02f 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1394,13 +1394,14 @@ 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,
      * 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 ( ioreq && ioreq->state != STATE_IOREQ_NONE )
         return;
     /*
      * a softirq may interrupt us between a virtual vmentry is
-- 
1.7.7.6


[-- Attachment #3: 0001-pvh-Fix-regression-due-to-assumption-that-HVM-paths-.patch --]
[-- Type: text/plain, Size: 2755 bytes --]

>From d76fc0d2f59ac65bd692adfa5f215da9ecf85d6a Mon Sep 17 00:00:00 2001
From: Mukesh Rathor <mukesh.rathor@oracle.com>
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@Intel.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 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


[-- Attachment #4: Type: text/plain, Size: 126 bytes --]

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