[Qemu-devel] [PATCH] pci: fix info pci with host bridge.

[Qemu-devel] [PATCH] pci: fix info pci with host bridge.

[Isaku Yamahata]

This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5.
pci host bridge doesn't have header type of bridge.
The check should be by header type, instead of pci class device.

Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index e91d2e6..eb2043e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1273,7 +1273,7 @@ static QObject *pci_get_devices_list(PCIBus *bus, int bus_num);
 
 static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num)
 {
-    int class;
+    uint8_t type;
     QObject *obj;
 
     obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d,"                                       "'class_info': %p, 'id': %p, 'regions': %p,"
@@ -1289,8 +1289,8 @@ static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num)
         qdict_put(qdict, "irq", qint_from_int(dev->config[PCI_INTERRUPT_LINE]));
     }
 
-    class = pci_get_word(dev->config + PCI_CLASS_DEVICE);
-    if (class == PCI_CLASS_BRIDGE_HOST || class == PCI_CLASS_BRIDGE_PCI) {
+    type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
+    if (type == PCI_HEADER_TYPE_BRIDGE) {
         QDict *qdict;
         QObject *pci_bridge;
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH] pci: fix info pci with host bridge.

[Michael S. Tsirkin]

On Mon, Feb 08, 2010 at 03:40:38PM +0900, Isaku Yamahata wrote:
> This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5.
> pci host bridge doesn't have header type of bridge.
> The check should be by header type, instead of pci class device.
> 
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Re: 525e05147d5a3bdc08caa422d108c1ef71b584b5
this commit put hard-coded constant in pci.c.
It would have been better to post it on list for review
instead of direct commit.

> ---
>  hw/pci.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index e91d2e6..eb2043e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1273,7 +1273,7 @@ static QObject *pci_get_devices_list(PCIBus *bus, int bus_num);
>  
>  static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num)
>  {
> -    int class;
> +    uint8_t type;
>      QObject *obj;
>  
>      obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d,"                                       "'class_info': %p, 'id': %p, 'regions': %p,"
> @@ -1289,8 +1289,8 @@ static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num)
>          qdict_put(qdict, "irq", qint_from_int(dev->config[PCI_INTERRUPT_LINE]));
>      }
>  
> -    class = pci_get_word(dev->config + PCI_CLASS_DEVICE);
> -    if (class == PCI_CLASS_BRIDGE_HOST || class == PCI_CLASS_BRIDGE_PCI) {
> +    type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> +    if (type == PCI_HEADER_TYPE_BRIDGE) {
>          QDict *qdict;
>          QObject *pci_bridge;
>  
> -- 
> 1.6.6.1

[Qemu-devel] Re: [PATCH] pci: fix info pci with host bridge.

[Michael S. Tsirkin]

On Mon, Feb 08, 2010 at 12:10:52PM +0200, Michael S. Tsirkin wrote:
> On Mon, Feb 08, 2010 at 03:40:38PM +0900, Isaku Yamahata wrote:
> > This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5.
> > pci host bridge doesn't have header type of bridge.
> > The check should be by header type, instead of pci class device.
> > 
> > Cc: Blue Swirl <blauwirbel@gmail.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> Re: 525e05147d5a3bdc08caa422d108c1ef71b584b5
> this commit put hard-coded constant in pci.c.
> It would have been better to post it on list for review
> instead of direct commit.

Heh, I looked at the reverse patch for some reason,
it didn't put in constants, it removed them :)

> > ---
> >  hw/pci.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index e91d2e6..eb2043e 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1273,7 +1273,7 @@ static QObject *pci_get_devices_list(PCIBus *bus, int bus_num);
> >  
> >  static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num)
> >  {
> > -    int class;
> > +    uint8_t type;
> >      QObject *obj;
> >  
> >      obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d,"                                       "'class_info': %p, 'id': %p, 'regions': %p,"
> > @@ -1289,8 +1289,8 @@ static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num)
> >          qdict_put(qdict, "irq", qint_from_int(dev->config[PCI_INTERRUPT_LINE]));
> >      }
> >  
> > -    class = pci_get_word(dev->config + PCI_CLASS_DEVICE);
> > -    if (class == PCI_CLASS_BRIDGE_HOST || class == PCI_CLASS_BRIDGE_PCI) {
> > +    type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> > +    if (type == PCI_HEADER_TYPE_BRIDGE) {
> >          QDict *qdict;
> >          QObject *pci_bridge;
> >  
> > -- 
> > 1.6.6.1

[Qemu-devel] Re: [PATCH] pci: fix info pci with host bridge.

[Michael S. Tsirkin]

On Mon, Feb 08, 2010 at 03:40:38PM +0900, Isaku Yamahata wrote:
> This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5.
> pci host bridge doesn't have header type of bridge.
> The check should be by header type, instead of pci class device.
> 
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

So the effect of this will be that info pci won't report
host bridge, right? IOW, it kind of reverts
525e05147d5a3bdc08caa422d108c1ef71b584b5, or am I
missing something?

> ---
>  hw/pci.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index e91d2e6..eb2043e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1273,7 +1273,7 @@ static QObject *pci_get_devices_list(PCIBus *bus, int bus_num);
>  
>  static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num)
>  {
> -    int class;
> +    uint8_t type;
>      QObject *obj;
>  
>      obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d,"                                       "'class_info': %p, 'id': %p, 'regions': %p,"
> @@ -1289,8 +1289,8 @@ static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num)
>          qdict_put(qdict, "irq", qint_from_int(dev->config[PCI_INTERRUPT_LINE]));
>      }
>  
> -    class = pci_get_word(dev->config + PCI_CLASS_DEVICE);
> -    if (class == PCI_CLASS_BRIDGE_HOST || class == PCI_CLASS_BRIDGE_PCI) {
> +    type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> +    if (type == PCI_HEADER_TYPE_BRIDGE) {
>          QDict *qdict;
>          QObject *pci_bridge;
>  
> -- 
> 1.6.6.1

[Qemu-devel] Re: [PATCH] pci: fix info pci with host bridge.

[Blue Swirl]

On Mon, Feb 8, 2010 at 12:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Feb 08, 2010 at 03:40:38PM +0900, Isaku Yamahata wrote:
>> This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5.
>> pci host bridge doesn't have header type of bridge.
>> The check should be by header type, instead of pci class device.
>>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>
> So the effect of this will be that info pci won't report
> host bridge, right? IOW, it kind of reverts
> 525e05147d5a3bdc08caa422d108c1ef71b584b5, or am I
> missing something?

Yes, it breaks info pci. PBM/APB does not use PCI_HEADER_TYPE_BRIDGE.

[Qemu-devel] Re: [PATCH] pci: fix info pci with host bridge.

[Isaku Yamahata]

On Mon, Feb 08, 2010 at 07:23:34PM +0200, Blue Swirl wrote:
> On Mon, Feb 8, 2010 at 12:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Feb 08, 2010 at 03:40:38PM +0900, Isaku Yamahata wrote:
> >> This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5.
> >> pci host bridge doesn't have header type of bridge.
> >> The check should be by header type, instead of pci class device.
> >>
> >> Cc: Blue Swirl <blauwirbel@gmail.com>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> >
> > So the effect of this will be that info pci won't report
> > host bridge, right? IOW, it kind of reverts
> > 525e05147d5a3bdc08caa422d108c1ef71b584b5, or am I
> > missing something?
> 
> Yes, it breaks info pci. PBM/APB does not use PCI_HEADER_TYPE_BRIDGE.

Devices of pci host bridge class (!= PCI-PCI bridge) don't have
header type PCI_HEADER_TYPE_BRIDGE (= 1), but PCI_HEADER_TYPE_NORMAL (= 0).
In fact, i440fx pci host bridge is of PCI_HEADER_TYPE_NORMAL.
You can see it in piix_pci.c.
Registers of offset 0x10-0x3F in configuration space are used
differently depending on header type.
For example, PCI_HEADER_TYPE_NORMAL device don't have primary bus register,
secondary bus register and so on. Those registers are used as BAR2.
It doesn't make senses to show BAR2 as bus numbers.

Having said that, I'm confused. So I downloaded
"UltraSPARC IIi User's Manual", 805-0087.pdf.
Page 301, table 19-12, section 19l.3.1 says that its header type is 0x0.
(= PCI_HEADER_TYPE_Normal).

cited from table 19-12.

offset
0x10-0x27       Base Address
0x28-0x2F       Reserved
0x30-0x34       Expansion ROM
0x34-0x3b       Reserved
0x3e            MIN_GNT
0x3f            MAX_LAT
...

So it doesn't make sense to access those registers as
primary bus number, etc...
However the manual also says that those shaded registers aren't implemented.
Maybe it would make sense to use those unused/unimplemented registers
for registers of header type 1 in PBM emulation.
This is what you want to do, Right?

Probably what you want in pci_info_device() is something like
"if (type == PCI_HEADER_TYPE_BRIDGE ||
    (vendorid == PCI_VENDOR_ID_SUN && deviceid == PCI_DEVICE_ID_SUN_SABRE))"

This is ugly, so introducing device specific info callback?
-- 
yamahata

[Qemu-devel] Re: [PATCH] pci: fix info pci with host bridge.

[Blue Swirl]

On Tue, Feb 9, 2010 at 5:02 AM, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> On Mon, Feb 08, 2010 at 07:23:34PM +0200, Blue Swirl wrote:
>> On Mon, Feb 8, 2010 at 12:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Feb 08, 2010 at 03:40:38PM +0900, Isaku Yamahata wrote:
>> >> This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5.
>> >> pci host bridge doesn't have header type of bridge.
>> >> The check should be by header type, instead of pci class device.
>> >>
>> >> Cc: Blue Swirl <blauwirbel@gmail.com>
>> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> >> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> >
>> > So the effect of this will be that info pci won't report
>> > host bridge, right? IOW, it kind of reverts
>> > 525e05147d5a3bdc08caa422d108c1ef71b584b5, or am I
>> > missing something?
>>
>> Yes, it breaks info pci. PBM/APB does not use PCI_HEADER_TYPE_BRIDGE.
>
> Devices of pci host bridge class (!= PCI-PCI bridge) don't have
> header type PCI_HEADER_TYPE_BRIDGE (= 1), but PCI_HEADER_TYPE_NORMAL (= 0).
> In fact, i440fx pci host bridge is of PCI_HEADER_TYPE_NORMAL.
> You can see it in piix_pci.c.
> Registers of offset 0x10-0x3F in configuration space are used
> differently depending on header type.
> For example, PCI_HEADER_TYPE_NORMAL device don't have primary bus register,
> secondary bus register and so on. Those registers are used as BAR2.
> It doesn't make senses to show BAR2 as bus numbers.
>
> Having said that, I'm confused. So I downloaded
> "UltraSPARC IIi User's Manual", 805-0087.pdf.
> Page 301, table 19-12, section 19l.3.1 says that its header type is 0x0.
> (= PCI_HEADER_TYPE_Normal).
>
> cited from table 19-12.
>
> offset
> 0x10-0x27       Base Address
> 0x28-0x2F       Reserved
> 0x30-0x34       Expansion ROM
> 0x34-0x3b       Reserved
> 0x3e            MIN_GNT
> 0x3f            MAX_LAT
> ...
>
> So it doesn't make sense to access those registers as
> primary bus number, etc...
> However the manual also says that those shaded registers aren't implemented.
> Maybe it would make sense to use those unused/unimplemented registers
> for registers of header type 1 in PBM emulation.
> This is what you want to do, Right?
>
> Probably what you want in pci_info_device() is something like
> "if (type == PCI_HEADER_TYPE_BRIDGE ||
>    (vendorid == PCI_VENDOR_ID_SUN && deviceid == PCI_DEVICE_ID_SUN_SABRE))"
>
> This is ugly, so introducing device specific info callback?

Or maybe header should be of normal type and the secondary/subordinate
registers should not be used like they would be with a PCI-PCI bridge.
In that case maybe the bus number checks are not correct, or perhaps
OpenBIOS PCI code is wrong.