[PATCH] Xen 4.4-rc3 regression with PVH compared to Xen 4.4-rc2.

[PATCH] Xen 4.4-rc3 regression with PVH compared to Xen 4.4-rc2.

[Konrad Rzeszutek Wilk]

I am hereby requesting an Xen 4.4 exemption for this bug-fix.

The PVH feature is considered experimental, but it would be good to have
it working out of the box without crashing the hypervisor.

Sadly that is not the case as 09bb434748af9bfe3f7fca4b6eef721a7d5042a4
"Nested VMX: prohibit virtual vmentry/vmexit during IO emulation"
casues an NULL pointer dereference when starting a guest with 'pvh=1'
in the guest config.

There are two ways of fixing this:
a). Add an '!xen_pvh_domain()' or '!xen_pvh_vcpu(current)' in the path, or
b). Check for ioreq() being NULL. This is actually done in other places
    in the hypervisor - so I choose to piggyback on that.

Thank you!


 xen/arch/x86/hvm/vmx/vvmx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


Konrad Rzeszutek Wilk (1):
      pvh: Fix regression caused by assumption that HVM paths MUST use io-backend device.

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

[Konrad Rzeszutek Wilk]

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: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d2ba435..2f516c9 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1400,7 +1400,7 @@ void nvmx_switch_guest(void)
      * 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 ( get_ioreq(v) && get_ioreq(v)->state != STATE_IOREQ_NONE )
         return;
     /*
      * a softirq may interrupt us between a virtual vmentry is
-- 
1.7.7.6

Re: [PATCH] Xen 4.4-rc3 regression with PVH compared to Xen 4.4-rc2.

[Mukesh Rathor]

On Mon,  3 Feb 2014 12:03:20 -0500
Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:

> I am hereby requesting an Xen 4.4 exemption for this bug-fix.
> 
> The PVH feature is considered experimental, but it would be good to
> have it working out of the box without crashing the hypervisor.
> 
> Sadly that is not the case as 09bb434748af9bfe3f7fca4b6eef721a7d5042a4
> "Nested VMX: prohibit virtual vmentry/vmexit during IO emulation"
> casues an NULL pointer dereference when starting a guest with 'pvh=1'
> in the guest config.
> 
> There are two ways of fixing this:
> a). Add an '!xen_pvh_domain()' or '!xen_pvh_vcpu(current)' in the
> path, or b). Check for ioreq() being NULL. This is actually done in
> other places in the hypervisor - so I choose to piggyback on that.
> 

I was about to send this patch on friday:

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



when I realized even after the above fix it is still crashing  for
me... debugging right now. JFYI.

thanks
Mukesh

Re: [PATCH] Xen 4.4-rc3 regression with PVH compared to Xen 4.4-rc2.

[Konrad Rzeszutek Wilk]

On Mon, Feb 03, 2014 at 11:26:05AM -0800, Mukesh Rathor wrote:
> On Mon,  3 Feb 2014 12:03:20 -0500
> Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> 
> > I am hereby requesting an Xen 4.4 exemption for this bug-fix.
> > 
> > The PVH feature is considered experimental, but it would be good to
> > have it working out of the box without crashing the hypervisor.
> > 
> > Sadly that is not the case as 09bb434748af9bfe3f7fca4b6eef721a7d5042a4
> > "Nested VMX: prohibit virtual vmentry/vmexit during IO emulation"
> > casues an NULL pointer dereference when starting a guest with 'pvh=1'
> > in the guest config.
> > 
> > There are two ways of fixing this:
> > a). Add an '!xen_pvh_domain()' or '!xen_pvh_vcpu(current)' in the
> > path, or b). Check for ioreq() being NULL. This is actually done in
> > other places in the hypervisor - so I choose to piggyback on that.
> > 
> 
> I was about to send this patch on friday:
> 
> 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
> 
> 
> 
> when I realized even after the above fix it is still crashing  for
> me... debugging right now. JFYI.

Are you doing it on a 'virgin' 4.4-rc3 or with your extra patches?

Also adding Roger so that he does not have to debug this crash.
> 
> thanks
> Mukesh
> 

Re: [PATCH] Xen 4.4-rc3 regression with PVH compared to Xen 4.4-rc2.

[Mukesh Rathor]

On Mon, 3 Feb 2014 14:53:58 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Mon, Feb 03, 2014 at 11:26:05AM -0800, Mukesh Rathor wrote:
> > On Mon,  3 Feb 2014 12:03:20 -0500
> > Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > 
> > > I am hereby requesting an Xen 4.4 exemption for this bug-fix.
> > > 
> > > The PVH feature is considered experimental, but it would be good
> > > to have it working out of the box without crashing the hypervisor.
> > > 
> > > Sadly that is not the case as
> > > 09bb434748af9bfe3f7fca4b6eef721a7d5042a4 "Nested VMX: prohibit
> > > virtual vmentry/vmexit during IO emulation" casues an NULL
> > > pointer dereference when starting a guest with 'pvh=1' in the
> > > guest config.
> > > 
> > > There are two ways of fixing this:
> > > a). Add an '!xen_pvh_domain()' or '!xen_pvh_vcpu(current)' in the
> > > path, or b). Check for ioreq() being NULL. This is actually done
> > > in other places in the hypervisor - so I choose to piggyback on
> > > that.
> > > 
> > 
> > I was about to send this patch on friday:
> > 
> > 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
> > 
> > 
> > 
> > when I realized even after the above fix it is still crashing  for
> > me... debugging right now. JFYI.
> 
> Are you doing it on a 'virgin' 4.4-rc3 or with your extra patches?

Actually, it's with my extra dom0 patches.

Mukesh

Re: [PATCH] Xen 4.4-rc3 regression with PVH compared to Xen 4.4-rc2.

[Mukesh Rathor]

On Mon, 3 Feb 2014 11:26:05 -0800
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Mon,  3 Feb 2014 12:03:20 -0500
> Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> 
> > I am hereby requesting an Xen 4.4 exemption for this bug-fix.
> > 
> > The PVH feature is considered experimental, but it would be good to
> > have it working out of the box without crashing the hypervisor.
> > 
> > Sadly that is not the case as
> > 09bb434748af9bfe3f7fca4b6eef721a7d5042a4 "Nested VMX: prohibit
> > virtual vmentry/vmexit during IO emulation" casues an NULL pointer
> > dereference when starting a guest with 'pvh=1' in the guest config.
> > 
> > There are two ways of fixing this:
> > a). Add an '!xen_pvh_domain()' or '!xen_pvh_vcpu(current)' in the
> > path, or b). Check for ioreq() being NULL. This is actually done in
> > other places in the hypervisor - so I choose to piggyback on that.
> > 
> 
> I was about to send this patch on friday:
> 
> 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
> 
> 
> 
> when I realized even after the above fix it is still crashing  for
> me... debugging right now. JFYI.

Ok, the crash in nvmx_switch_guest() even after the above fix is a 
different issue for which I am making a patch. So, this patch should
be applied, however, rather than calling get_ioreq() twice as in 
Konrad's patch, I recommend my code above which calls it once. It's 
inlined now, and prob would be optimized, but the function could
change in many ways in future.

thanks
mukesh

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

[Jan Beulich]

>>> On 03.02.14 at 18:03, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1400,7 +1400,7 @@ void nvmx_switch_guest(void)
>       * 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 ( get_ioreq(v) && get_ioreq(v)->state != STATE_IOREQ_NONE )

As Mukesh pointed out, calling get_ioreq() twice is inefficient.

But to me it's not clear whether a PVH vCPU getting here is wrong
in the first place, i.e. I would think the above condition should be
|| rather than && (after all, even if nested HVM one day became
supported for PVH, there not being an ioreq would still seem to be
a clear indication of no further work to be done here).

Of course, if done that way, the corresponding comment would
benefit from being extended accordingly.

Jan

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

[Konrad Rzeszutek Wilk]

On Tue, Feb 04, 2014 at 08:54:59AM +0000, Jan Beulich wrote:
> >>> On 03.02.14 at 18:03, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -1400,7 +1400,7 @@ void nvmx_switch_guest(void)
> >       * 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 ( get_ioreq(v) && get_ioreq(v)->state != STATE_IOREQ_NONE )
> 
> As Mukesh pointed out, calling get_ioreq() twice is inefficient.
> 
> But to me it's not clear whether a PVH vCPU getting here is wrong
> in the first place, i.e. I would think the above condition should be
> || rather than && (after all, even if nested HVM one day became

I presume you mean like this:

	if ( !get_ioreq(v) || get_ioreq(v)->state != STATE_IOREQ_NONE )
		return;

If the Intel maintainers are OK with that I can do it that (and only
do one get_ioreq(v) call) and expand the comment.

Or just take the simple route and squash Mukesh's patch in mine and
revist this later - as I would prefer to make the minimal amount of
changes to any code in during rc3.


> supported for PVH, there not being an ioreq would still seem to be
> a clear indication of no further work to be done here).
> 
> Of course, if done that way, the corresponding comment would
> benefit from being extended accordingly.
> 
> Jan
> 
> 

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

[Jan Beulich]

>>> On 04.02.14 at 15:48, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, Feb 04, 2014 at 08:54:59AM +0000, Jan Beulich wrote:
>> >>> On 03.02.14 at 18:03, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
>> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> > @@ -1400,7 +1400,7 @@ void nvmx_switch_guest(void)
>> >       * 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 ( get_ioreq(v) && get_ioreq(v)->state != STATE_IOREQ_NONE )
>> 
>> As Mukesh pointed out, calling get_ioreq() twice is inefficient.
>> 
>> But to me it's not clear whether a PVH vCPU getting here is wrong
>> in the first place, i.e. I would think the above condition should be
>> || rather than && (after all, even if nested HVM one day became
> 
> I presume you mean like this:
> 
> 	if ( !get_ioreq(v) || get_ioreq(v)->state != STATE_IOREQ_NONE )
> 		return;
> 
> If the Intel maintainers are OK with that I can do it that (and only
> do one get_ioreq(v) call) and expand the comment.
> 
> Or just take the simple route and squash Mukesh's patch in mine and
> revist this later - as I would prefer to make the minimal amount of
> changes to any code in during rc3.

Wasn't it that Mukesh's patch simply was yours with the two
get_ioreq()s folded by using a local variable?

Jan

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

[Konrad Rzeszutek Wilk]

On Tue, Feb 04, 2014 at 03:02:44PM +0000, Jan Beulich wrote:
> >>> On 04.02.14 at 15:48, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Tue, Feb 04, 2014 at 08:54:59AM +0000, Jan Beulich wrote:
> >> >>> On 03.02.14 at 18:03, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> >> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> >> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> >> > @@ -1400,7 +1400,7 @@ void nvmx_switch_guest(void)
> >> >       * 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 ( get_ioreq(v) && get_ioreq(v)->state != STATE_IOREQ_NONE )
> >> 
> >> As Mukesh pointed out, calling get_ioreq() twice is inefficient.
> >> 
> >> But to me it's not clear whether a PVH vCPU getting here is wrong
> >> in the first place, i.e. I would think the above condition should be
> >> || rather than && (after all, even if nested HVM one day became
> > 
> > I presume you mean like this:
> > 
> > 	if ( !get_ioreq(v) || get_ioreq(v)->state != STATE_IOREQ_NONE )
> > 		return;
> > 
> > If the Intel maintainers are OK with that I can do it that (and only
> > do one get_ioreq(v) call) and expand the comment.
> > 
> > Or just take the simple route and squash Mukesh's patch in mine and
> > revist this later - as I would prefer to make the minimal amount of
> > changes to any code in during rc3.
> 
> 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

>From 6864786da87c4d67998a8ef46a3d289ac85074a9 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 |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d2ba435..143b7a2 100644
--- 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);
     /*
      * 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 )
         return;
     /*
      * a softirq may interrupt us between a virtual vmentry is
-- 
1.7.7.6

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

[Jan Beulich]

>>> 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?

Jan

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

[Konrad Rzeszutek Wilk]

[-- 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

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

[George Dunlap]

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

But they do different things -- one does "ioreq && ioreq->state..." and 
the other does "!ioreq || ioreq->state...".  The first one is incorrect, 
AFAICT.

  -George

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

[Jan Beulich]

>>> On 05.02.14 at 15:35, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/04/2014 04:42 PM, Konrad Rzeszutek Wilk wrote:
>> 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.
> 
> But they do different things -- one does "ioreq && ioreq->state..." and 
> the other does "!ioreq || ioreq->state...".  The first one is incorrect, 
> AFAICT.

Not outright incorrect perhaps, but sub-optimal. That was the subject
of the discussion so far, and we're mainly waiting for the two copied
VMX guys to confirm that the second patch is correct.

Jan

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

[Konrad Rzeszutek Wilk]

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@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.
> 
> 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.
> 
>  -George
> 
> 

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

[Zhang, Yang Z]

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

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.

Best regards,
Yang

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

[Konrad Rzeszutek Wilk]

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


> 
> Best regards,
> Yang
> 

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

[George Dunlap]

On 02/07/2014 03:41 PM, Konrad Rzeszutek Wilk wrote:
> 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@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.
>> 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@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>

I forget if I release acked this yet, but just in case:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.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

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

[Zhang, Yang Z]

Konrad Rzeszutek Wilk wrote on 2014-02-07:
> 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@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.
>>>> 
>> 
>> 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?
> 

Sure.

Best regards,
Yang

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

[George Dunlap]

On Tue, Feb 11, 2014 at 12:17 AM, Zhang, Yang Z <yang.z.zhang@intel.com> wrote:
> Konrad Rzeszutek Wilk wrote on 2014-02-07:
>> 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@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.
>>>>>
>>>
>>> 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?
>>
>
> Sure.

Konrad / Jan: Ping?

If this fix looks reasonable it would be nice to get this in before RC4.

 -George

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

[Jan Beulich]

>>> On 13.02.14 at 16:38, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> Konrad / Jan: Ping?
> 
> If this fix looks reasonable it would be nice to get this in before RC4.

It would have long been committed if the VMX maintainers finally
gave their okay. Even privately pinging them didn't seem to help...

Jan

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

[George Dunlap]

On 02/13/2014 04:03 PM, Jan Beulich wrote:
>>>> On 13.02.14 at 16:38, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>> Konrad / Jan: Ping?
>>
>> If this fix looks reasonable it would be nice to get this in before RC4.
> It would have long been committed if the VMX maintainers finally
> gave their okay. Even privately pinging them didn't seem to help...

Oh, right, I forgot that Yang isn't a maintainer.

Well it looks like we have two options:
* Commit it without a maintainer's ack
* Revert the patch that caused the regression to PVH.

(Or maybe Yang can chase the maintainers internally.)

  -George

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

[Jan Beulich]

>>> On 13.02.14 at 17:08, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/13/2014 04:03 PM, Jan Beulich wrote:
>>>>> On 13.02.14 at 16:38, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>>> Konrad / Jan: Ping?
>>>
>>> If this fix looks reasonable it would be nice to get this in before RC4.
>> It would have long been committed if the VMX maintainers finally
>> gave their okay. Even privately pinging them didn't seem to help...
> 
> Oh, right, I forgot that Yang isn't a maintainer.
> 
> Well it looks like we have two options:
> * Commit it without a maintainer's ack

Just committed it on the basis that I too can ack this. I would just
have preferred for the more specific maintainers to have given
their input/okay...

Jan

> * Revert the patch that caused the regression to PVH.
> 
> (Or maybe Yang can chase the maintainers internally.)
> 
>   -George