Patchwork [04/11] qdev: pcibus_dev_info

login
register
mail settings
Submitter Nathan Baum
Date Dec. 26, 2009, 9:19 p.m.
Message ID <1261862362-2530-5-git-send-email-nathan@parenthephobia.org.uk>
Download mbox | patch
Permalink /patch/41814/
State New
Headers show

Comments

Nathan Baum - Dec. 26, 2009, 9:19 p.m.
This returns a QObject detailing the PCI-specific data about the device.

Signed-off-by: Nathan Baum <nathan@parenthephobia.org.uk>
---
 hw/pci.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)
Luiz Capitulino - Dec. 29, 2009, 5:08 p.m.
On Sat, 26 Dec 2009 21:19:15 +0000
Nathan Baum <nathan@parenthephobia.org.uk> wrote:

> +static QObject *pcibus_dev_info(Monitor *mon, DeviceState *dev)
> +{
> +    PCIDevice *d = (PCIDevice *)dev;
> +    const pci_class_desc *desc;
> +    PCIIORegion *r;
> +    int i, class;
> +    QObject *retval;
> +    QList *regions;
> +    
> +    retval = qobject_from_jsonf("{ 'addr': { 'bus' : %d, 'slot' : %d, 'func': %d }, "
> +                                "  'device': { 'vendor': %d, 'id': %d }, "
> +                                "  'subsystem': { 'vendor': %d, 'id': %d } "
> +                                "}",

 You can have 'address' here.

> +    class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> +    desc = pci_class_descriptions;
> +    while (desc->desc && class != desc->class)
> +        desc++;
> +    if (desc->desc) {
> +        qdict_put(qobject_to_qdict(retval), "class", qstring_from_str(desc->desc));
> +    } else {
> +        qdict_put(qobject_to_qdict(retval), "class", qint_from_int(class));
> +    }

 I think it's not good to return different data types for the same key, as
it will make clients more complex, what you can do here is to make 'class'
a QDict with members 'desc' and 'number'.
Nathan Baum - Dec. 29, 2009, 8:02 p.m.
On Tue, 2009-12-29 at 15:08 -0200, Luiz Capitulino wrote:

> > +    class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> > +    desc = pci_class_descriptions;
> > +    while (desc->desc && class != desc->class)
> > +        desc++;
> > +    if (desc->desc) {
> > +        qdict_put(qobject_to_qdict(retval), "class", qstring_from_str(desc->desc));
> > +    } else {
> > +        qdict_put(qobject_to_qdict(retval), "class", qint_from_int(class));
> > +    }
> 
>  I think it's not good to return different data types for the same key, as
> it will make clients more complex, what you can do here is to make 'class'
> a QDict with members 'desc' and 'number'.

That's reasonable. It also means we can tell clients the class number
even when the description is known, which might be useful.
Luiz Capitulino - Dec. 31, 2009, 12:55 p.m.
On Sat, 26 Dec 2009 21:19:15 +0000
Nathan Baum <nathan@parenthephobia.org.uk> wrote:

> This returns a QObject detailing the PCI-specific data about the device.

 Back to this again, turns out I'm converting pci_info() and I think we
should agree on some standards for PCI keys.

> +static QObject *pcibus_dev_info(Monitor *mon, DeviceState *dev)
> +{
> +    PCIDevice *d = (PCIDevice *)dev;
> +    const pci_class_desc *desc;
> +    PCIIORegion *r;
> +    int i, class;
> +    QObject *retval;
> +    QList *regions;
> +    
> +    retval = qobject_from_jsonf("{ 'addr': { 'bus' : %d, 'slot' : %d, 'func': %d }, "
> +                                "  'device': { 'vendor': %d, 'id': %d }, "
> +                                "  'subsystem': { 'vendor': %d, 'id': %d } "
> +                                "}",

 I'm using 'function' instead of 'func' and I have a dict called 'id' with keys
'device' and 'vendor'. Maybe in this case you could call it 'device_info' as
you have subsystem as well.

 These are only general suggestions, it's not always easy to choose them.

> +    class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> +    desc = pci_class_descriptions;
> +    while (desc->desc && class != desc->class)
> +        desc++;
> +    if (desc->desc) {
> +        qdict_put(qobject_to_qdict(retval), "class", qstring_from_str(desc->desc));
> +    } else {
> +        qdict_put(qobject_to_qdict(retval), "class", qint_from_int(class));
> +    }

 We can share code here, I've added a key called 'class_info' for this, it's
a dict with exact the same info ('desc' and 'class').

> +    
> +    regions = qlist_new();
> +    qdict_put(qobject_to_qdict(retval), "regions", regions);
> +
> +    for (i = 0; i < PCI_NUM_REGIONS; i++) {
> +        r = &d->io_regions[i];
> +        if (!r->size)
> +            continue;
> +        qlist_append_obj(regions,
> +                         qobject_from_jsonf("{'type':%s,'addr':%d,'size':%d}",
> +                                            r->type & PCI_BASE_ADDRESS_SPACE_IO ? "i/o" : "mem",
> +                                            (int) r->addr,
> +                                            (int) r->size));

 You should never cast to int, you should use PRId64 like this example:

obj = qobject_from_jsonf("{ 'BAR': %d, 'type': 'io', "
			"'address': %" PRId64 ", "
			"'current': %" PRId64 " }",
			i, r->addr, r->addr + r->size - 1);

 Also, I'm using 'io' and 'memory' for the 'type' key, usually I prefer the
full word if it's something simple.

 /me remember to write a 'dict best' practices doc.

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 9722fce..8688d8a 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -27,6 +27,8 @@ 
 #include "net.h"
 #include "sysemu.h"
 #include "loader.h"
+#include "qjson.h"
+#include "qint.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -1585,6 +1587,52 @@  static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
     }
 }
 
+static QObject *pcibus_dev_info(Monitor *mon, DeviceState *dev)
+{
+    PCIDevice *d = (PCIDevice *)dev;
+    const pci_class_desc *desc;
+    PCIIORegion *r;
+    int i, class;
+    QObject *retval;
+    QList *regions;
+    
+    retval = qobject_from_jsonf("{ 'addr': { 'bus' : %d, 'slot' : %d, 'func': %d }, "
+                                "  'device': { 'vendor': %d, 'id': %d }, "
+                                "  'subsystem': { 'vendor': %d, 'id': %d } "
+                                "}",
+                                d->config[PCI_SECONDARY_BUS],
+                                PCI_SLOT(d->devfn),
+                                PCI_FUNC(d->devfn),
+                                pci_get_word(d->config + PCI_VENDOR_ID),
+                                pci_get_word(d->config + PCI_DEVICE_ID),
+                                pci_get_word(d->config + PCI_SUBSYSTEM_VENDOR_ID),
+                                pci_get_word(d->config + PCI_SUBSYSTEM_ID));
+    class = pci_get_word(d->config + PCI_CLASS_DEVICE);
+    desc = pci_class_descriptions;
+    while (desc->desc && class != desc->class)
+        desc++;
+    if (desc->desc) {
+        qdict_put(qobject_to_qdict(retval), "class", qstring_from_str(desc->desc));
+    } else {
+        qdict_put(qobject_to_qdict(retval), "class", qint_from_int(class));
+    }
+    
+    regions = qlist_new();
+    qdict_put(qobject_to_qdict(retval), "regions", regions);
+
+    for (i = 0; i < PCI_NUM_REGIONS; i++) {
+        r = &d->io_regions[i];
+        if (!r->size)
+            continue;
+        qlist_append_obj(regions,
+                         qobject_from_jsonf("{'type':%s,'addr':%d,'size':%d}",
+                                            r->type & PCI_BASE_ADDRESS_SPACE_IO ? "i/o" : "mem",
+                                            (int) r->addr,
+                                            (int) r->size));
+    }
+    return retval;
+}
+
 static PCIDeviceInfo bridge_info = {
     .qdev.name    = "pci-bridge",
     .qdev.size    = sizeof(PCIBridge),