| Message ID | 20100212023134.GC11930@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 12, 2010 at 11:31:34AM +0900, Isaku Yamahata wrote: > This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5 > by introducing device specific get_info_quirk callback. > It wrongly assumes that pci host bridge class device has > header type of pci-pci bridge. But this isn't always true. > In fact i440fx pci host bridge has header type of normal device, > hence it breaks i440fx and other pci host bridges. > The right fix is that header type should be checked, instead of device class. > > The change set's purpose is to show PBM pci host bridge > info which doesn't conform to PCI specification. So, PBM has header type PCI_HEADER_TYPE_NORMAL but all config space is in bridge format? > So introduce get_info_quirk callback and use it for PBM. > > Cc: Blue Swirl <blauwirbel@gmail.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/apb_pci.c | 1 + > hw/pci.c | 79 ++++++++++++++++++++++++++++++++------------------------- > hw/pci.h | 3 ++ > 3 files changed, 48 insertions(+), 35 deletions(-) > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > index 46d5b0e..a557469 100644 > --- a/hw/apb_pci.c > +++ b/hw/apb_pci.c > @@ -479,6 +479,7 @@ static PCIDeviceInfo pbm_pci_host_info = { > .qdev.name = "pbm", > .qdev.size = sizeof(PCIDevice), > .init = pbm_pci_host_init, > + .get_info_quirk = pci_bridge_get_info, > .header_type = PCI_HEADER_TYPE_BRIDGE, > }; > > diff --git a/hw/pci.c b/hw/pci.c > index 9ad63dd..1e2bde8 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1271,10 +1271,44 @@ static QObject *pci_get_regions_list(const PCIDevice *dev) > > static QObject *pci_get_devices_list(PCIBus *bus, int bus_num); > > +void pci_bridge_get_info(PCIDevice *dev, PCIBus *bus, QDict *qdict) > +{ > + QObject *pci_bridge; > + > + pci_bridge = qobject_from_jsonf("{ 'bus': " > + "{ 'number': %d, 'secondary': %d, 'subordinate': %d }, " > + "'io_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "}, " > + "'memory_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "}, " > + "'prefetchable_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "} }", > + dev->config[PCI_PRIMARY_BUS], dev->config[PCI_SECONDARY_BUS], > + dev->config[PCI_SUBORDINATE_BUS], > + pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_IO), > + pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_IO), > + pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_MEMORY), > + pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_MEMORY), > + pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_MEMORY | > + PCI_BASE_ADDRESS_MEM_PREFETCH), > + pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_MEMORY | > + PCI_BASE_ADDRESS_MEM_PREFETCH)); > + > + if (dev->config[PCI_SECONDARY_BUS] != 0) { > + PCIBus *child_bus = pci_find_bus(bus, dev->config[PCI_SECONDARY_BUS]); > + if (child_bus) { > + qdict_put_obj(qobject_to_qdict(pci_bridge), "devices", > + pci_get_devices_list(child_bus, > + dev->config[PCI_SECONDARY_BUS])); > + } > + } > + > + qdict_put_obj(qdict, "pci_bridge", pci_bridge); > +} > + > static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num) > { > - int class; > + PCIDeviceInfo *info = DO_UPCAST(PCIDeviceInfo, qdev, dev->qdev.info); > QObject *obj; > + QDict *qdict; > + uint8_t type; > > obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d," "'class_info': %p, 'id': %p, 'regions': %p," > " 'qdev_id': %s }", > @@ -1283,45 +1317,20 @@ static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num) > pci_get_dev_class(dev), pci_get_dev_id(dev), > pci_get_regions_list(dev), > dev->qdev.id ? dev->qdev.id : ""); > + qdict = qobject_to_qdict(obj); > > if (dev->config[PCI_INTERRUPT_PIN] != 0) { > - QDict *qdict = qobject_to_qdict(obj); > 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) { > - QDict *qdict; > - QObject *pci_bridge; > - > - pci_bridge = qobject_from_jsonf("{ 'bus': " > - "{ 'number': %d, 'secondary': %d, 'subordinate': %d }, " > - "'io_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "}, " > - "'memory_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "}, " > - "'prefetchable_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "} }", > - dev->config[PCI_PRIMARY_BUS], dev->config[PCI_SECONDARY_BUS], > - dev->config[PCI_SUBORDINATE_BUS], > - pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_IO), > - pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_IO), > - pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_MEMORY), > - pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_MEMORY), > - pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_MEMORY | > - PCI_BASE_ADDRESS_MEM_PREFETCH), > - pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_MEMORY | > - PCI_BASE_ADDRESS_MEM_PREFETCH)); > - > - if (dev->config[PCI_SECONDARY_BUS] != 0) { > - PCIBus *child_bus = pci_find_bus(bus, dev->config[PCI_SECONDARY_BUS]); > - > - if (child_bus) { > - qdict = qobject_to_qdict(pci_bridge); > - qdict_put_obj(qdict, "devices", > - pci_get_devices_list(child_bus, > - dev->config[PCI_SECONDARY_BUS])); > - } > - } > - qdict = qobject_to_qdict(obj); > - qdict_put_obj(qdict, "pci_bridge", pci_bridge); > + type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION; > + if (type == PCI_HEADER_TYPE_BRIDGE) { > + pci_bridge_get_info(dev, bus, qdict); > + } > + > + if (info /* not all pci device aren't converted qdev yet */ && > + info->get_info_quirk) { > + info->get_info_quirk(dev, bus, qdict); > } this should be: > if (info /* not all pci device aren't converted qdev yet */ && > info->get_info_quirk) { > info->get_info_quirk(dev, bus, qdict); > } else if type == PCI_HEADER_TYPE_BRIDGE) { > pci_bridge_get_info(dev, bus, qdict); } > > return obj; > diff --git a/hw/pci.h b/hw/pci.h > index 8b511d2..5b87acd 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -80,6 +80,7 @@ typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev, > typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num, > pcibus_t addr, pcibus_t size, int type); > typedef int PCIUnregisterFunc(PCIDevice *pci_dev); > +typedef void PCIGetInfoQuirkFunc(PCIDevice *pci_dev, PCIBus *bus, QDict *qdict); > > typedef struct PCIIORegion { > pcibus_t addr; /* current PCI mapping address. -1 means not mapped */ > @@ -240,6 +241,7 @@ void do_pci_info(Monitor *mon, QObject **ret_data); > PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, > pci_map_irq_fn map_irq, const char *name); > PCIDevice *pci_bridge_get_device(PCIBus *bus); > +void pci_bridge_get_info(PCIDevice *dev, PCIBus *bus, QDict *qdict); > > static inline void > pci_set_byte(uint8_t *config, uint8_t val) > @@ -314,6 +316,7 @@ typedef struct { > PCIUnregisterFunc *exit; > PCIConfigReadFunc *config_read; > PCIConfigWriteFunc *config_write; > + PCIGetInfoQuirkFunc *get_info_quirk; > > /* pci config header type */ > uint8_t header_type; > -- > 1.6.6.1
On Fri, Feb 12, 2010 at 02:54:59PM +0200, Michael S. Tsirkin wrote: > On Fri, Feb 12, 2010 at 11:31:34AM +0900, Isaku Yamahata wrote: > > This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5 > > by introducing device specific get_info_quirk callback. > > It wrongly assumes that pci host bridge class device has > > header type of pci-pci bridge. But this isn't always true. > > In fact i440fx pci host bridge has header type of normal device, > > hence it breaks i440fx and other pci host bridges. > > The right fix is that header type should be checked, instead of device class. > > > > The change set's purpose is to show PBM pci host bridge > > info which doesn't conform to PCI specification. > > So, PBM has header type PCI_HEADER_TYPE_NORMAL > but all config space is in bridge format? Some of registers is in bridge format. To be honest, I don't know whether all or some. Blue seems to want bus numbers registers at least. http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00519.html
On Fri, Feb 12, 2010 at 3:34 PM, Isaku Yamahata <yamahata@valinux.co.jp> wrote: > On Fri, Feb 12, 2010 at 02:54:59PM +0200, Michael S. Tsirkin wrote: >> On Fri, Feb 12, 2010 at 11:31:34AM +0900, Isaku Yamahata wrote: >> > This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5 >> > by introducing device specific get_info_quirk callback. >> > It wrongly assumes that pci host bridge class device has >> > header type of pci-pci bridge. But this isn't always true. >> > In fact i440fx pci host bridge has header type of normal device, >> > hence it breaks i440fx and other pci host bridges. >> > The right fix is that header type should be checked, instead of device class. >> > >> > The change set's purpose is to show PBM pci host bridge >> > info which doesn't conform to PCI specification. >> >> So, PBM has header type PCI_HEADER_TYPE_NORMAL >> but all config space is in bridge format? > > Some of registers is in bridge format. > To be honest, I don't know whether all or some. > Blue seems to want bus numbers registers at least. > > http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00519.html I'm not so sure anymore, I'll have to check this in the weekend. Perhaps there was some other problem with bus numbering or OpenBIOS PCI programming which resulted in the need to program PRIMARY/ SECONDARY/SUBORDINATE bus values for host bridge. Does it make any sense to use anything other than bus 0 for PCI bus attached to host bridge?
On Fri, Feb 12, 2010 at 10:10 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > On Fri, Feb 12, 2010 at 3:34 PM, Isaku Yamahata <yamahata@valinux.co.jp> wrote: >> On Fri, Feb 12, 2010 at 02:54:59PM +0200, Michael S. Tsirkin wrote: >>> On Fri, Feb 12, 2010 at 11:31:34AM +0900, Isaku Yamahata wrote: >>> > This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5 >>> > by introducing device specific get_info_quirk callback. >>> > It wrongly assumes that pci host bridge class device has >>> > header type of pci-pci bridge. But this isn't always true. >>> > In fact i440fx pci host bridge has header type of normal device, >>> > hence it breaks i440fx and other pci host bridges. >>> > The right fix is that header type should be checked, instead of device class. >>> > >>> > The change set's purpose is to show PBM pci host bridge >>> > info which doesn't conform to PCI specification. >>> >>> So, PBM has header type PCI_HEADER_TYPE_NORMAL >>> but all config space is in bridge format? >> >> Some of registers is in bridge format. >> To be honest, I don't know whether all or some. >> Blue seems to want bus numbers registers at least. >> >> http://lists.gnu.org/archive/html/qemu-devel/2010-02/msg00519.html > > I'm not so sure anymore, I'll have to check this in the weekend. > Perhaps there was some other problem with bus numbering or OpenBIOS > PCI programming which resulted in the need to program PRIMARY/ > SECONDARY/SUBORDINATE bus values for host bridge. Does it make any > sense to use anything other than bus 0 for PCI bus attached to host > bridge? I found the bug in my new PCI probing code, 525e05147d5a3bdc08caa422d108c1ef71b584b5 was not correct after all. There is no need to configure host brige bus values. So I'll apply Isaku's earlier fix that removes the bogus host bridge data from 'info pci'.
diff --git a/hw/apb_pci.c b/hw/apb_pci.c index 46d5b0e..a557469 100644 --- a/hw/apb_pci.c +++ b/hw/apb_pci.c @@ -479,6 +479,7 @@ static PCIDeviceInfo pbm_pci_host_info = { .qdev.name = "pbm", .qdev.size = sizeof(PCIDevice), .init = pbm_pci_host_init, + .get_info_quirk = pci_bridge_get_info, .header_type = PCI_HEADER_TYPE_BRIDGE, }; diff --git a/hw/pci.c b/hw/pci.c index 9ad63dd..1e2bde8 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -1271,10 +1271,44 @@ static QObject *pci_get_regions_list(const PCIDevice *dev) static QObject *pci_get_devices_list(PCIBus *bus, int bus_num); +void pci_bridge_get_info(PCIDevice *dev, PCIBus *bus, QDict *qdict) +{ + QObject *pci_bridge; + + pci_bridge = qobject_from_jsonf("{ 'bus': " + "{ 'number': %d, 'secondary': %d, 'subordinate': %d }, " + "'io_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "}, " + "'memory_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "}, " + "'prefetchable_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "} }", + dev->config[PCI_PRIMARY_BUS], dev->config[PCI_SECONDARY_BUS], + dev->config[PCI_SUBORDINATE_BUS], + pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_IO), + pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_IO), + pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_MEMORY), + pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_MEMORY), + pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_MEMORY | + PCI_BASE_ADDRESS_MEM_PREFETCH), + pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_MEMORY | + PCI_BASE_ADDRESS_MEM_PREFETCH)); + + if (dev->config[PCI_SECONDARY_BUS] != 0) { + PCIBus *child_bus = pci_find_bus(bus, dev->config[PCI_SECONDARY_BUS]); + if (child_bus) { + qdict_put_obj(qobject_to_qdict(pci_bridge), "devices", + pci_get_devices_list(child_bus, + dev->config[PCI_SECONDARY_BUS])); + } + } + + qdict_put_obj(qdict, "pci_bridge", pci_bridge); +} + static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num) { - int class; + PCIDeviceInfo *info = DO_UPCAST(PCIDeviceInfo, qdev, dev->qdev.info); QObject *obj; + QDict *qdict; + uint8_t type; obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d," "'class_info': %p, 'id': %p, 'regions': %p," " 'qdev_id': %s }", @@ -1283,45 +1317,20 @@ static QObject *pci_get_dev_dict(PCIDevice *dev, PCIBus *bus, int bus_num) pci_get_dev_class(dev), pci_get_dev_id(dev), pci_get_regions_list(dev), dev->qdev.id ? dev->qdev.id : ""); + qdict = qobject_to_qdict(obj); if (dev->config[PCI_INTERRUPT_PIN] != 0) { - QDict *qdict = qobject_to_qdict(obj); 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) { - QDict *qdict; - QObject *pci_bridge; - - pci_bridge = qobject_from_jsonf("{ 'bus': " - "{ 'number': %d, 'secondary': %d, 'subordinate': %d }, " - "'io_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "}, " - "'memory_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "}, " - "'prefetchable_range': { 'base': %" PRId64 ", 'limit': %" PRId64 "} }", - dev->config[PCI_PRIMARY_BUS], dev->config[PCI_SECONDARY_BUS], - dev->config[PCI_SUBORDINATE_BUS], - pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_IO), - pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_IO), - pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_MEMORY), - pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_MEMORY), - pci_bridge_get_base(dev, PCI_BASE_ADDRESS_SPACE_MEMORY | - PCI_BASE_ADDRESS_MEM_PREFETCH), - pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_SPACE_MEMORY | - PCI_BASE_ADDRESS_MEM_PREFETCH)); - - if (dev->config[PCI_SECONDARY_BUS] != 0) { - PCIBus *child_bus = pci_find_bus(bus, dev->config[PCI_SECONDARY_BUS]); - - if (child_bus) { - qdict = qobject_to_qdict(pci_bridge); - qdict_put_obj(qdict, "devices", - pci_get_devices_list(child_bus, - dev->config[PCI_SECONDARY_BUS])); - } - } - qdict = qobject_to_qdict(obj); - qdict_put_obj(qdict, "pci_bridge", pci_bridge); + type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION; + if (type == PCI_HEADER_TYPE_BRIDGE) { + pci_bridge_get_info(dev, bus, qdict); + } + + if (info /* not all pci device aren't converted qdev yet */ && + info->get_info_quirk) { + info->get_info_quirk(dev, bus, qdict); } return obj; diff --git a/hw/pci.h b/hw/pci.h index 8b511d2..5b87acd 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -80,6 +80,7 @@ typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev, typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num, pcibus_t addr, pcibus_t size, int type); typedef int PCIUnregisterFunc(PCIDevice *pci_dev); +typedef void PCIGetInfoQuirkFunc(PCIDevice *pci_dev, PCIBus *bus, QDict *qdict); typedef struct PCIIORegion { pcibus_t addr; /* current PCI mapping address. -1 means not mapped */ @@ -240,6 +241,7 @@ void do_pci_info(Monitor *mon, QObject **ret_data); PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, pci_map_irq_fn map_irq, const char *name); PCIDevice *pci_bridge_get_device(PCIBus *bus); +void pci_bridge_get_info(PCIDevice *dev, PCIBus *bus, QDict *qdict); static inline void pci_set_byte(uint8_t *config, uint8_t val) @@ -314,6 +316,7 @@ typedef struct { PCIUnregisterFunc *exit; PCIConfigReadFunc *config_read; PCIConfigWriteFunc *config_write; + PCIGetInfoQuirkFunc *get_info_quirk; /* pci config header type */ uint8_t header_type;
This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5 by introducing device specific get_info_quirk callback. It wrongly assumes that pci host bridge class device has header type of pci-pci bridge. But this isn't always true. In fact i440fx pci host bridge has header type of normal device, hence it breaks i440fx and other pci host bridges. The right fix is that header type should be checked, instead of device class. The change set's purpose is to show PBM pci host bridge info which doesn't conform to PCI specification. So introduce get_info_quirk callback and use it for PBM. Cc: Blue Swirl <blauwirbel@gmail.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/apb_pci.c | 1 + hw/pci.c | 79 ++++++++++++++++++++++++++++++++------------------------- hw/pci.h | 3 ++ 3 files changed, 48 insertions(+), 35 deletions(-)