[PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function

[PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function

[Wei Liu]

QEMU stubdom will read PCI config space when enumerating PCI devices.
Xen should return ~0 when there is no suitable ioreq server to dispatch
the request.

Without this patch, QEMU stubdom will fail to start because hvmloader
fails following assertion:

118         ASSERT((devfn != PCI_ISA_DEVFN) ||
119                ((vendor_id == 0x8086) && (device_id == 0x7000)));

because vendor_id and device_id are 0.

This fixes a regression for QEMU stubdom. It should be backported to 4.5
as well.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c7984d1..c826ac5 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2577,6 +2577,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
     {
     case IOREQ_TYPE_COPY:
     case IOREQ_TYPE_PIO:
+    case IOREQ_TYPE_PCI_CONFIG:
         if ( p->dir == IOREQ_READ )
         {
             if ( !p->data_is_ptr )
-- 
1.9.1

Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function

[Andrew Cooper]

On 27/01/15 19:06, Wei Liu wrote:
> QEMU stubdom will read PCI config space when enumerating PCI devices.
> Xen should return ~0 when there is no suitable ioreq server to dispatch
> the request.
>
> Without this patch, QEMU stubdom will fail to start because hvmloader
> fails following assertion:
>
> 118         ASSERT((devfn != PCI_ISA_DEVFN) ||
> 119                ((vendor_id == 0x8086) && (device_id == 0x7000)));
>
> because vendor_id and device_id are 0.
>
> This fixes a regression for QEMU stubdom. It should be backported to 4.5
> as well.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

The patch is clearly a good bugfix, but I am not sure the commit message
is accurate.

A Qemu stubdom is a PV guest, not an HVM one, so will not be triggering
this path in Xen.  It is HVMLoader which scans the PCI bus.

I presume, given the description, that in the case that a Qemu stubdom
is used (as opposed to a dom0 qemu), it is not registered as the default
ioreq server, causing Xen to complete the config cycles (and incorrectly
return 0 instead of ~0)?

~Andrew

> ---
>  xen/arch/x86/hvm/hvm.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c7984d1..c826ac5 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2577,6 +2577,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>      {
>      case IOREQ_TYPE_COPY:
>      case IOREQ_TYPE_PIO:
> +    case IOREQ_TYPE_PCI_CONFIG:
>          if ( p->dir == IOREQ_READ )
>          {
>              if ( !p->data_is_ptr )

Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function

[Jan Beulich]

>>> On 27.01.15 at 20:34, <andrew.cooper3@citrix.com> wrote:
> On 27/01/15 19:06, Wei Liu wrote:
>> QEMU stubdom will read PCI config space when enumerating PCI devices.
>> Xen should return ~0 when there is no suitable ioreq server to dispatch
>> the request.
>>
>> Without this patch, QEMU stubdom will fail to start because hvmloader
>> fails following assertion:
>>
>> 118         ASSERT((devfn != PCI_ISA_DEVFN) ||
>> 119                ((vendor_id == 0x8086) && (device_id == 0x7000)));
>>
>> because vendor_id and device_id are 0.
>>
>> This fixes a regression for QEMU stubdom. It should be backported to 4.5
>> as well.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Paul Durrant <paul.durrant@citrix.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> The patch is clearly a good bugfix, but I am not sure the commit message
> is accurate.

Nor am I convinced that this is the only place to fix. hvmloader
assuming to get back ~0 is contrary to various other code also
taking 0 in vendor or device IDs as a sign to skip the device.

(Looking at the surrounding code I'm also puzzled by all the
pci_write?()-s in that bus scanning loop: Most if not all of them
should be done by the BIOS, not hvmloader. But of course that's
the case for the BAR setup further down too.)

Jan

Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function

[Don Slutz]

...
> (Looking at the surrounding code I'm also puzzled by all the
> pci_write?()-s in that bus scanning loop: Most if not all of them
> should be done by the BIOS, not hvmloader. But of course that's
> the case for the BAR setup further down too.)
> 

I do not know the history, but seabios (which would
normally do this) has the statements (in qemu_platform_setup()):

    if (runningOnXen()) {
...
   return
}

    // Initialize pci


    pci_setup();

Which expects that hvmloader has done the pci_setup().

   -Don Slutz


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

Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function

[Paul Durrant]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 27 January 2015 19:06
> To: xen-devel@lists.xen.org
> Cc: Wei Liu; Jan Beulich; Paul Durrant; Andrew Cooper
> Subject: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist
> function
> 
> QEMU stubdom will read PCI config space when enumerating PCI devices.
> Xen should return ~0 when there is no suitable ioreq server to dispatch
> the request.
> 
> Without this patch, QEMU stubdom will fail to start because hvmloader
> fails following assertion:
> 
> 118         ASSERT((devfn != PCI_ISA_DEVFN) ||
> 119                ((vendor_id == 0x8086) && (device_id == 0x7000)));
> 
> because vendor_id and device_id are 0.
> 
> This fixes a regression for QEMU stubdom. It should be backported to 4.5
> as well.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

No, you should not need this. If you look at the code closely you'll see that hvm_complete_assist_req() is only called if hvm_select_ioreq_server() returns NULL, and the switch from IOREQ_TYPE_PIO to IOREQ_TYPE_PCI_CONFIG is only made if a non-default ioreq server matches the current cf8. Hence we could actually ASSERT(p->type !- IOREQ_TYPE_PCI_CONFIG) at the top of hvm_complete_assist_req().

  Paul

> ---
>  xen/arch/x86/hvm/hvm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c7984d1..c826ac5 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2577,6 +2577,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>      {
>      case IOREQ_TYPE_COPY:
>      case IOREQ_TYPE_PIO:
> +    case IOREQ_TYPE_PCI_CONFIG:
>          if ( p->dir == IOREQ_READ )
>          {
>              if ( !p->data_is_ptr )
> --
> 1.9.1

Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function

[Don Slutz]

On 01/28/15 04:22, Paul Durrant wrote:
>> -----Original Message-----
>> From: Wei Liu [mailto:wei.liu2@citrix.com]
>> Sent: 27 January 2015 19:06
>> To: xen-devel@lists.xen.org
>> Cc: Wei Liu; Jan Beulich; Paul Durrant; Andrew Cooper
>> Subject: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist
>> function
>>
>> QEMU stubdom will read PCI config space when enumerating PCI devices.
>> Xen should return ~0 when there is no suitable ioreq server to dispatch
>> the request.
>>
>> Without this patch, QEMU stubdom will fail to start because hvmloader
>> fails following assertion:
>>
>> 118         ASSERT((devfn != PCI_ISA_DEVFN) ||
>> 119                ((vendor_id == 0x8086) && (device_id == 0x7000)));
>>
>> because vendor_id and device_id are 0.
>>
>> This fixes a regression for QEMU stubdom. It should be backported to 4.5
>> as well.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Paul Durrant <paul.durrant@citrix.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> No, you should not need this. If you look at the code closely you'll see that
> hvm_complete_assist_req() is only called if hvm_select_ioreq_server()
returns NULL,
> and the switch from IOREQ_TYPE_PIO to IOREQ_TYPE_PCI_CONFIG is only
made if a
> non-default ioreq server matches the current cf8. Hence we could
> actually ASSERT(p->type !- IOREQ_TYPE_PCI_CONFIG) at the top
> of hvm_complete_assist_req().
> 


I will agree that the ASSERT looks good to me with s/!-/!=/.

I have no idea how Wei Liu makes it to this code via QEMU stubdom
(last I knew QEMU stubdom needs QEMU traditional not QEMU upstream.
And the only way to get IOREQ_TYPE_PCI_CONFIG is with QEMU upstream).


I have found out how hvmloader ends up using hvm_complete_assist_req()
when not expected.  Not sure how this is related to Wei Liu's issue.

   -Don Slutz



>   Paul
> 
>> ---
>>  xen/arch/x86/hvm/hvm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index c7984d1..c826ac5 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2577,6 +2577,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>>      {
>>      case IOREQ_TYPE_COPY:
>>      case IOREQ_TYPE_PIO:
>> +    case IOREQ_TYPE_PCI_CONFIG:
>>          if ( p->dir == IOREQ_READ )
>>          {
>>              if ( !p->data_is_ptr )
>> --
>> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function

[Wei Liu]

On Wed, Jan 28, 2015 at 07:27:41PM -0500, Don Slutz wrote:
> On 01/28/15 04:22, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Wei Liu [mailto:wei.liu2@citrix.com]
> >> Sent: 27 January 2015 19:06
> >> To: xen-devel@lists.xen.org
> >> Cc: Wei Liu; Jan Beulich; Paul Durrant; Andrew Cooper
> >> Subject: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist
> >> function
> >>
> >> QEMU stubdom will read PCI config space when enumerating PCI devices.
> >> Xen should return ~0 when there is no suitable ioreq server to dispatch
> >> the request.
> >>
> >> Without this patch, QEMU stubdom will fail to start because hvmloader
> >> fails following assertion:
> >>
> >> 118         ASSERT((devfn != PCI_ISA_DEVFN) ||
> >> 119                ((vendor_id == 0x8086) && (device_id == 0x7000)));
> >>
> >> because vendor_id and device_id are 0.
> >>
> >> This fixes a regression for QEMU stubdom. It should be backported to 4.5
> >> as well.
> >>
> >> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> Cc: Paul Durrant <paul.durrant@citrix.com>
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > 
> > No, you should not need this. If you look at the code closely you'll see that
> > hvm_complete_assist_req() is only called if hvm_select_ioreq_server()
> returns NULL,
> > and the switch from IOREQ_TYPE_PIO to IOREQ_TYPE_PCI_CONFIG is only
> made if a
> > non-default ioreq server matches the current cf8. Hence we could
> > actually ASSERT(p->type !- IOREQ_TYPE_PCI_CONFIG) at the top
> > of hvm_complete_assist_req().
> > 
> 
> 
> I will agree that the ASSERT looks good to me with s/!-/!=/.
> 
> I have no idea how Wei Liu makes it to this code via QEMU stubdom
> (last I knew QEMU stubdom needs QEMU traditional not QEMU upstream.
> And the only way to get IOREQ_TYPE_PCI_CONFIG is with QEMU upstream).
> 
> 
> I have found out how hvmloader ends up using hvm_complete_assist_req()
> when not expected.  Not sure how this is related to Wei Liu's issue.
> 

This patch is now useless because the problem I discovered is a race
between stubdom and guest. It will take some time to fix.

Wei.

>    -Don Slutz
> 
> 
> 
> >   Paul
> > 
> >> ---
> >>  xen/arch/x86/hvm/hvm.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> >> index c7984d1..c826ac5 100644
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -2577,6 +2577,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
> >>      {
> >>      case IOREQ_TYPE_COPY:
> >>      case IOREQ_TYPE_PIO:
> >> +    case IOREQ_TYPE_PCI_CONFIG:
> >>          if ( p->dir == IOREQ_READ )
> >>          {
> >>              if ( !p->data_is_ptr )
> >> --
> >> 1.9.1
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> >