* [Qemu-devel] [PATCH] pci: introduce get_dev_dict callback.
@ 2010-02-10 6:59 Isaku Yamahata
2010-02-10 9:45 ` [Qemu-devel] " Michael S. Tsirkin
0 siblings, 1 reply; 3+ messages in thread
From: Isaku Yamahata @ 2010-02-10 6:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Blue Swirl, Michael S. Tsirkin
This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5
by introducing device specific get_dev_dict callback.
pci host bridge doesn't always have header type of bridge.
Especially PBM which is emulated doesn't conform to PCI spec
at the moment. So by introducing get_dev_dict, allow each pci device
to return specific infos.
This patch also makes it easier to enhance info pci command in the future.
For example returning pcie stuff.
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/dec_pci.c | 1 +
hw/pci.c | 74 ++++++++++++++++++++++++++++++---------------------------
hw/pci.h | 3 ++
4 files changed, 44 insertions(+), 35 deletions(-)
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 46d5b0e..693f99e 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_dev_dict = pci_bridge_get_dev_dict,
.header_type = PCI_HEADER_TYPE_BRIDGE,
};
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index 8d059f1..824fafc 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -91,6 +91,7 @@ static PCIDeviceInfo dec_21154_pci_host_info = {
.qdev.name = "dec-21154",
.qdev.size = sizeof(PCIDevice),
.init = dec_21154_pci_host_init,
+ .get_dev_dict = pci_bridge_get_dev_dict,
.header_type = PCI_HEADER_TYPE_BRIDGE,
};
diff --git a/hw/pci.c b/hw/pci.c
index 9ad63dd..9510aee 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1271,10 +1271,43 @@ static QObject *pci_get_regions_list(const PCIDevice *dev)
static QObject *pci_get_devices_list(PCIBus *bus, int bus_num);
+void pci_bridge_get_dev_dict(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;
obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d," "'class_info': %p, 'id': %p, 'regions': %p,"
" 'qdev_id': %s }",
@@ -1283,45 +1316,15 @@ 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);
+ if (info /* not all pci device aren't converted qdev yet */ &&
+ info->get_dev_dict) {
+ info->get_dev_dict(dev, bus, qdict);
}
return obj;
@@ -1880,6 +1883,7 @@ static PCIDeviceInfo bridge_info = {
.init = pci_bridge_initfn,
.exit = pci_bridge_exitfn,
.config_write = pci_bridge_write_config,
+ .get_dev_dict = pci_bridge_get_dev_dict,
.header_type = PCI_HEADER_TYPE_BRIDGE,
.qdev.props = (Property[]) {
DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
diff --git a/hw/pci.h b/hw/pci.h
index 8b511d2..43ddced 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 PCIGetDevDictFunc(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_dev_dict(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;
+ PCIGetDevDictFunc *get_dev_dict;
/* pci config header type */
uint8_t header_type;
--
1.6.6.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Qemu-devel] Re: [PATCH] pci: introduce get_dev_dict callback.
2010-02-10 6:59 [Qemu-devel] [PATCH] pci: introduce get_dev_dict callback Isaku Yamahata
@ 2010-02-10 9:45 ` Michael S. Tsirkin
2010-02-10 15:30 ` Isaku Yamahata
0 siblings, 1 reply; 3+ messages in thread
From: Michael S. Tsirkin @ 2010-02-10 9:45 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: Blue Swirl, qemu-devel
On Wed, Feb 10, 2010 at 03:59:30PM +0900, Isaku Yamahata wrote:
> This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5
Could you please clarify what exactly is the bug,
and how this patch fixes it?
> by introducing device specific get_dev_dict callback.
> pci host bridge doesn't always have header type of bridge.
>
> Especially PBM which is emulated doesn't conform to PCI spec
> at the moment. So by introducing get_dev_dict, allow each pci device
> to return specific infos.
> This patch also makes it easier to enhance info pci command in the future.
> For example returning pcie stuff.
>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
If the idea is to allow quirk mode, how about we call the hook
get_info_quirk, and only use it for non-spec compliant devices?
The point being making it clear that most devices should not use it.
> ---
> hw/apb_pci.c | 1 +
> hw/dec_pci.c | 1 +
> hw/pci.c | 74 ++++++++++++++++++++++++++++++---------------------------
> hw/pci.h | 3 ++
> 4 files changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 46d5b0e..693f99e 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_dev_dict = pci_bridge_get_dev_dict,
> .header_type = PCI_HEADER_TYPE_BRIDGE,
> };
>
> diff --git a/hw/dec_pci.c b/hw/dec_pci.c
> index 8d059f1..824fafc 100644
> --- a/hw/dec_pci.c
> +++ b/hw/dec_pci.c
> @@ -91,6 +91,7 @@ static PCIDeviceInfo dec_21154_pci_host_info = {
> .qdev.name = "dec-21154",
> .qdev.size = sizeof(PCIDevice),
> .init = dec_21154_pci_host_init,
> + .get_dev_dict = pci_bridge_get_dev_dict,
> .header_type = PCI_HEADER_TYPE_BRIDGE,
> };
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 9ad63dd..9510aee 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1271,10 +1271,43 @@ static QObject *pci_get_regions_list(const PCIDevice *dev)
>
> static QObject *pci_get_devices_list(PCIBus *bus, int bus_num);
>
> +void pci_bridge_get_dev_dict(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;
>
> obj = qobject_from_jsonf("{ 'bus': %d, 'slot': %d, 'function': %d," "'class_info': %p, 'id': %p, 'regions': %p,"
> " 'qdev_id': %s }",
> @@ -1283,45 +1316,15 @@ 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);
> + if (info /* not all pci device aren't converted qdev yet */ &&
-> not all pci device are converted to qdev yet
So - will this break info pci for these devices?
> + info->get_dev_dict) {
> + info->get_dev_dict(dev, bus, qdict);
> }
>
> return obj;
> @@ -1880,6 +1883,7 @@ static PCIDeviceInfo bridge_info = {
> .init = pci_bridge_initfn,
> .exit = pci_bridge_exitfn,
> .config_write = pci_bridge_write_config,
> + .get_dev_dict = pci_bridge_get_dev_dict,
> .header_type = PCI_HEADER_TYPE_BRIDGE,
> .qdev.props = (Property[]) {
> DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
> diff --git a/hw/pci.h b/hw/pci.h
> index 8b511d2..43ddced 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 PCIGetDevDictFunc(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_dev_dict(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;
> + PCIGetDevDictFunc *get_dev_dict;
>
> /* pci config header type */
> uint8_t header_type;
> --
> 1.6.6.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Qemu-devel] Re: [PATCH] pci: introduce get_dev_dict callback.
2010-02-10 9:45 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-02-10 15:30 ` Isaku Yamahata
0 siblings, 0 replies; 3+ messages in thread
From: Isaku Yamahata @ 2010-02-10 15:30 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Blue Swirl, qemu-devel
On Wed, Feb 10, 2010 at 11:45:48AM +0200, Michael S. Tsirkin wrote:
> On Wed, Feb 10, 2010 at 03:59:30PM +0900, Isaku Yamahata wrote:
> > This patch fixes 525e05147d5a3bdc08caa422d108c1ef71b584b5
>
> Could you please clarify what exactly is the bug,
> and how this patch fixes it?
How about the following?
The change set of 525e05147d5a3bdc08caa422d108c1ef71b584b5
wrongly assumes that pci host bridge class device has
header type of pci-pci bridge. But this isn't true. In fact i440fx
pci host bridge has header type of normal device.
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.
> > by introducing device specific get_dev_dict callback.
> > pci host bridge doesn't always have header type of bridge.
> >
> > Especially PBM which is emulated doesn't conform to PCI spec
> > at the moment. So by introducing get_dev_dict, allow each pci device
> > to return specific infos.
> > This patch also makes it easier to enhance info pci command in the future.
> > For example returning pcie stuff.
> >
> > Cc: Blue Swirl <blauwirbel@gmail.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>
> If the idea is to allow quirk mode, how about we call the hook
> get_info_quirk, and only use it for non-spec compliant devices?
> The point being making it clear that most devices should not use it.
Ok. I'll respin the patch with get_info_quirk.
--
yamahata
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-02-10 15:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-10 6:59 [Qemu-devel] [PATCH] pci: introduce get_dev_dict callback Isaku Yamahata
2010-02-10 9:45 ` [Qemu-devel] " Michael S. Tsirkin
2010-02-10 15:30 ` Isaku Yamahata
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).