From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH] pvh: Fix regression caused by assumption that HVM paths MUST use io-backend device. Date: Tue, 4 Feb 2014 11:42:58 -0500 Message-ID: <20140204164258.GB7443@phenom.dumpdata.com> References: <1391447001-19100-1-git-send-email-konrad.wilk@oracle.com> <1391447001-19100-2-git-send-email-konrad.wilk@oracle.com> <52F0B8F30200007800118E81@nat28.tlf.novell.com> <20140204144833.GE3853@phenom.dumpdata.com> <52F10F2402000078001190B7@nat28.tlf.novell.com> <20140204153258.GA6847@phenom.dumpdata.com> <52F119780200007800119172@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="oyUTqETQ0mS9luUI" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WAj6c-00054G-W7 for xen-devel@lists.xenproject.org; Tue, 04 Feb 2014 16:44:15 +0000 Content-Disposition: inline In-Reply-To: <52F119780200007800119172@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: george.dunlap@eu.citrix.com, Konrad Rzeszutek Wilk , jun.nakajima@Intel.com, yang.z.zhang@Intel.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org --oyUTqETQ0mS9luUI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Feb 04, 2014 at 03:46:48PM +0000, Jan Beulich wrote: > >>> On 04.02.14 at 16:32, Konrad Rzeszutek Wilk 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 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: [] nvmx_switch_guest+0x4d/0x903 [] 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 CC: Jun Nakajima Signed-off-by: Mukesh Rathor Signed-off-by: Konrad Rzeszutek Wilk --- 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 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: [] nvmx_switch_guest+0x4d/0x903 [] 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 CC: Jun Nakajima Suggested-by: Jan Beulich Signed-off-by: Mukesh Rathor Signed-off-by: Konrad Rzeszutek Wilk --- 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 > --oyUTqETQ0mS9luUI Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0001-pvh-Fix-regression-caused-by-assumption-that-HVM-pat.patch" >>From 47a5554201f0bc1778e5bcbde8c39088707f727f Mon Sep 17 00:00:00 2001 From: Mukesh Rathor 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: [] nvmx_switch_guest+0x4d/0x903 [] 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 CC: Jun Nakajima Signed-off-by: Mukesh Rathor Signed-off-by: Konrad Rzeszutek Wilk --- 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 --oyUTqETQ0mS9luUI Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0001-pvh-Fix-regression-due-to-assumption-that-HVM-paths-.patch" >>From d76fc0d2f59ac65bd692adfa5f215da9ecf85d6a Mon Sep 17 00:00:00 2001 From: Mukesh Rathor 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: [] nvmx_switch_guest+0x4d/0x903 [] 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 CC: Jun Nakajima Suggested-by: Jan Beulich Signed-off-by: Mukesh Rathor Signed-off-by: Konrad Rzeszutek Wilk --- 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 --oyUTqETQ0mS9luUI Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --oyUTqETQ0mS9luUI--