[PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[Andrew Cooper]

The xen event channel for the ioreq server must be targeted at the appropriate
vcpu, so the correct one can be unpaused when IO is completed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>

---

The cosmetic tweaks were not completely cosmetic.
---
 xen/arch/x86/hvm/hvm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index b03ee4e..4b7792d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -638,7 +638,7 @@ static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
     {
         struct domain *d = s->domain;
 
-        rc = alloc_unbound_xen_event_channel(v->domain, 0, s->domid, NULL);
+        rc = alloc_unbound_xen_event_channel(v->domain, v->vcpu_id, s->domid, NULL);
         if ( rc < 0 )
             goto fail3;
 
-- 
1.7.10.4

Re: [PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[Jan Beulich]

>>> On 04.02.15 at 13:35, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -638,7 +638,7 @@ static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
>      {
>          struct domain *d = s->domain;
>  
> -        rc = alloc_unbound_xen_event_channel(v->domain, 0, s->domid, NULL);
> +        rc = alloc_unbound_xen_event_channel(v->domain, v->vcpu_id, s->domid, NULL);

But this sits in a conditional checking v->vcpu_id == 0. I.e. I don't
see what difference this makes.

Jan

Re: [PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Jan Beulich
> Sent: 04 February 2015 12:46
> To: Andrew Cooper
> Cc: Xen-devel
> Subject: Re: [Xen-devel] [PATCH] x86/hvm: Fix HVM guest regression
> introduced by c58ba78c84
> 
> >>> On 04.02.15 at 13:35, <andrew.cooper3@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -638,7 +638,7 @@ static int hvm_ioreq_server_add_vcpu(struct
> hvm_ioreq_server *s,
> >      {
> >          struct domain *d = s->domain;
> >
> > -        rc = alloc_unbound_xen_event_channel(v->domain, 0, s->domid,
> NULL);
> > +        rc = alloc_unbound_xen_event_channel(v->domain, v->vcpu_id, s-
> >domid, NULL);
> 
> But this sits in a conditional checking v->vcpu_id == 0. I.e. I don't
> see what difference this makes.

True, but without the context it did appear to be wrong. Perhaps best to go with vcpu_id for clarity?

  Paul

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

Re: [PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[Andrew Cooper]

On 04/02/15 12:45, Jan Beulich wrote:
>>>> On 04.02.15 at 13:35, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -638,7 +638,7 @@ static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
>>      {
>>          struct domain *d = s->domain;
>>  
>> -        rc = alloc_unbound_xen_event_channel(v->domain, 0, s->domid, NULL);
>> +        rc = alloc_unbound_xen_event_channel(v->domain, v->vcpu_id, s->domid, NULL);
> But this sits in a conditional checking v->vcpu_id == 0. I.e. I don't
> see what difference this makes.
>
> Jan
>

Hmm so it does.

Something between d0b2caa..c58ba78 has broken HVM guests to point at
which HVMloader doesn't reliably function.

Back to debugging...

~Andrew

Re: [PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[Jan Beulich]

>>> On 04.02.15 at 13:56, <andrew.cooper3@citrix.com> wrote:
> Something between d0b2caa..c58ba78 has broken HVM guests to point at
> which HVMloader doesn't reliably function.

And no crash (with register state dumped) or any other hint as to
what's going wrong?

Jan

Re: [PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[Andrew Cooper]

On 04/02/15 13:00, Jan Beulich wrote:
>>>> On 04.02.15 at 13:56, <andrew.cooper3@citrix.com> wrote:
>> Something between d0b2caa..c58ba78 has broken HVM guests to point at
>> which HVMloader doesn't reliably function.
> And no crash (with register state dumped) or any other hint as to
> what's going wrong?

PV guests are all fine, and mixed PV/HVM tests have the PV side complete
successfully.

~Andrew

Re: [PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[Don Slutz]

On 02/04/15 08:02, Andrew Cooper wrote:
> On 04/02/15 13:00, Jan Beulich wrote:
>>>>> On 04.02.15 at 13:56, <andrew.cooper3@citrix.com> wrote:
>>> Something between d0b2caa..c58ba78 has broken HVM guests to point at
>>> which HVMloader doesn't reliably function.
>> And no crash (with register state dumped) or any other hint as to
>> what's going wrong?
> 
> PV guests are all fine, and mixed PV/HVM tests have the PV side complete
> successfully.
> 

With the info provided, it looks a lot like I have found:

(copied here, dropped duplication):


-------- Forwarded Message --------
Subject: Re: [Qemu-devel] [PATCH v5 2/2] Xen: Use the ioreq-server API
when available
Date: Wed, 28 Jan 2015 19:57:42 -0500
From: Don Slutz <dslutz@verizon.com>
To: Don Slutz <dslutz@verizon.com>, Paul Durrant
<paul.durrant@citrix.com>, qemu-devel@nongnu.org, Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
CC: Peter Maydell <peter.maydell@linaro.org>, Olaf Hering
<olaf@aepfle.de>, Alexey Kardashevskiy <aik@ozlabs.ru>, Stefan Weil
<sw@weilnetz.de>, Michael Tokarev <mjt@tls.msk.ru>, Alexander Graf
<agraf@suse.de>, Gerd Hoffmann <kraxel@redhat.com>, Stefan Hajnoczi
<stefanha@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>



On 01/28/15 19:05, Don Slutz wrote:
> On 01/28/15 14:32, Don Slutz wrote:
>> On 12/05/14 05:50, Paul Durrant wrote:
>>> The ioreq-server API added to Xen 4.5 offers better security than
>>> the existing Xen/QEMU interface because the shared pages that are
>>> used to pass emulation request/results back and forth are removed
>>> from the guest's memory space before any requests are serviced.
>>> This prevents the guest from mapping these pages (they are in a
>>> well known location) and attempting to attack QEMU by synthesizing
>>> its own request structures. Hence, this patch modifies configure
>>> to detect whether the API is available, and adds the necessary
>>> code to use the API if it is.
>>
>> This patch (which is now on xenbits qemu staging) is causing me
>> issues.
>>
>
> I have found the key.
>
> The following will reproduce my issue:
>
> 1) xl create -p <config>
> 2) read one of HVM_PARAM_IOREQ_PFN, HVM_PARAM_BUFIOREQ_PFN, or
>    HVM_PARAM_BUFIOREQ_EVTCHN
> 3) xl unpause new guest
>
> The guest will hang in hvmloader.
>

...

Using QEMU upstream master (or xenbits qemu staging), you do not have a
default ioreq server.  And so hvm_select_ioreq_server() returns NULL for
hvmloader's iorequest to:

CPU4  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 ]

(I added this xentrace to figure out what is happening, and I have
a lot of data about it, if any one wants it.)

To get a guest hang instead of calling hvm_complete_assist_req()
for some of hvmloader's pci_read() calls, you can do the following:


1) xl create -p <config>
2) read one of HVM_PARAM_IOREQ_PFN, HVM_PARAM_BUFIOREQ_PFN, or
   HVM_PARAM_BUFIOREQ_EVTCHN
3) xl unpause new guest

The guest will hang in hvmloader.

The read of HVM_PARAM_IOREQ_PFN will cause a default ioreq server to
be created and directed to the QEMU upsteam that is not a default
ioreq server.  This read also creates the extra event channels that
I see.

I see that hvmop_create_ioreq_server() prevents you from creating
an is_default ioreq_server, so QEMU is not able to do.

Not sure where we go from here.

   -Don Slutz


Using the debug key e, you can see extra unbound event channels.


I had proposed the 1 line fix:

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index bad410e..7ac4b45 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -993,7 +993,7 @@ static int hvm_create_ioreq_server(struct domain *d,
domid_t domid,
     spin_lock(&d->arch.hvm_domain.ioreq_server.lock);

     rc = -EEXIST;
-    if ( is_default && d->arch.hvm_domain.default_ioreq_server != NULL )
+    if ( is_default && !list_empty(&d->arch.hvm_domain.ioreq_server.list) )
         goto fail2;

     rc = hvm_ioreq_server_init(s, d, domid, is_default, handle_bufioreq,


But no idea if this is right or way off base.

    -Don Slutz



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

Re: [PATCH] x86/hvm: Fix HVM guest regression introduced by c58ba78c84

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Andrew Cooper
> Sent: 04 February 2015 12:35
> To: Xen-devel
> Cc: Andrew Cooper; Jan Beulich
> Subject: [Xen-devel] [PATCH] x86/hvm: Fix HVM guest regression introduced
> by c58ba78c84
> 
> The xen event channel for the ioreq server must be targeted at the
> appropriate
> vcpu, so the correct one can be unpaused when IO is completed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> 
> ---
> 
> The cosmetic tweaks were not completely cosmetic.

No, they weren't were they.

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  xen/arch/x86/hvm/hvm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index b03ee4e..4b7792d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -638,7 +638,7 @@ static int hvm_ioreq_server_add_vcpu(struct
> hvm_ioreq_server *s,
>      {
>          struct domain *d = s->domain;
> 
> -        rc = alloc_unbound_xen_event_channel(v->domain, 0, s->domid,
> NULL);
> +        rc = alloc_unbound_xen_event_channel(v->domain, v->vcpu_id, s-
> >domid, NULL);
>          if ( rc < 0 )
>              goto fail3;
> 
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel