* [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function
@ 2015-01-27 19:06 Wei Liu
2015-01-27 19:34 ` Andrew Cooper
2015-01-28 9:22 ` Paul Durrant
0 siblings, 2 replies; 7+ messages in thread
From: Wei Liu @ 2015-01-27 19:06 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function
2015-01-27 19:06 [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function Wei Liu
@ 2015-01-27 19:34 ` Andrew Cooper
2015-01-28 9:26 ` Jan Beulich
2015-01-28 9:22 ` Paul Durrant
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2015-01-27 19:34 UTC (permalink / raw)
To: Wei Liu, xen-devel; +Cc: Paul Durrant, Jan Beulich
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 )
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function
2015-01-27 19:06 [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function Wei Liu
2015-01-27 19:34 ` Andrew Cooper
@ 2015-01-28 9:22 ` Paul Durrant
2015-01-29 0:27 ` Don Slutz
1 sibling, 1 reply; 7+ messages in thread
From: Paul Durrant @ 2015-01-28 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
> -----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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function
2015-01-27 19:34 ` Andrew Cooper
@ 2015-01-28 9:26 ` Jan Beulich
2015-01-28 20:03 ` Don Slutz
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2015-01-28 9:26 UTC (permalink / raw)
To: Andrew Cooper, Wei Liu; +Cc: xen-devel, Paul Durrant
>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function
2015-01-28 9:26 ` Jan Beulich
@ 2015-01-28 20:03 ` Don Slutz
0 siblings, 0 replies; 7+ messages in thread
From: Don Slutz @ 2015-01-28 20:03 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Wei Liu; +Cc: xen-devel, Paul Durrant
...
> (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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function
2015-01-28 9:22 ` Paul Durrant
@ 2015-01-29 0:27 ` Don Slutz
2015-01-29 10:52 ` Wei Liu
0 siblings, 1 reply; 7+ messages in thread
From: Don Slutz @ 2015-01-29 0:27 UTC (permalink / raw)
To: Paul Durrant, Wei Liu, xen-devel; +Cc: Andrew Cooper, Jan Beulich
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function
2015-01-29 0:27 ` Don Slutz
@ 2015-01-29 10:52 ` Wei Liu
0 siblings, 0 replies; 7+ messages in thread
From: Wei Liu @ 2015-01-29 10:52 UTC (permalink / raw)
To: Don Slutz; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, xen-devel
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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-01-29 10:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 19:06 [PATCH] ioreq-server: handle IOREQ_TYPE_PCI_CONFIG in assist function Wei Liu
2015-01-27 19:34 ` Andrew Cooper
2015-01-28 9:26 ` Jan Beulich
2015-01-28 20:03 ` Don Slutz
2015-01-28 9:22 ` Paul Durrant
2015-01-29 0:27 ` Don Slutz
2015-01-29 10:52 ` Wei Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.