| Message ID | 20100208064038.GC22624@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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?
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.
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;
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(-)