Re: [Qemu-devel] [RFC,PATCH 04/11] qdev: pcibus_dev_info

Re: [Qemu-devel] [RFC,PATCH 04/11] qdev: pcibus_dev_info

[Markus Armbruster]

Nathan Baum <nathan@parenthephobia.org.uk> writes:

> 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(-)
>
> 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 }, "

'bus' doesn't really belong here, it's a property of the bus, not the
device.  That's why device property "addr" is just device, function.

If we want to have it here anyway, we should have 'domain' as well.
Even though we don't support PCI domains, yet.

> +                                "  '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));

You need to print pcibus_t values with %FMT_PCIBUS, just like
pcibus_dev_print().  Casting to int so you can use %d can truncate the
value.

> +    }
> +    return retval;
> +}
> +
>  static PCIDeviceInfo bridge_info = {
>      .qdev.name    = "pci-bridge",
>      .qdev.size    = sizeof(PCIBridge),

Re: [Qemu-devel] [RFC,PATCH 08/11] qdev: Add usb_bus_dev_info

[Markus Armbruster]

Nathan Baum <nathan@parenthephobia.org.uk> writes:

> Returns a QObject with information about a USB device.
>
> Signed-off-by: Nathan Baum <nathan@parenthephobia.org.uk>
> ---
>  hw/usb-bus.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
> index 54027df..6d02807 100644
> --- a/hw/usb-bus.c
> +++ b/hw/usb-bus.c
> @@ -3,6 +3,7 @@
>  #include "qdev.h"
>  #include "sysemu.h"
>  #include "monitor.h"
> +#include "qjson.h"
>  
>  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
>  
> @@ -232,6 +233,18 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
>                     dev->attached ? ", attached" : "");
>  }
>  
> +static QObject *usb_bus_dev_info(Monitor *mon, DeviceState *qdev)
> +{
> +    USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
> +    USBBus *bus = usb_bus_from_device(dev);
> +    return qobject_from_jsonf("{'busnr': %d, 'addr':%d, 'speed': %s, 'desc': %s, 'attached': %i}",
> +                              bus->busnr,

As for PCI, 'busnr' belongs to the bus, not the device.

Hmm, we don't have the infrastructure to return bus information, yet.
"info qtree" hardcodes printing of name and type.  Gerd, what do you
think?

> +                              dev->addr,
> +                              usb_speed(dev->speed),
> +                              dev->product_desc,
> +                              dev->attached);
> +}
> +
>  void usb_info(Monitor *mon)
>  {
>      USBBus *bus;

Re: [Qemu-devel] [RFC,PATCH 08/11] qdev: Add usb_bus_dev_info

[Nathan Baum]

On Fri, 2010-01-15 at 19:14 +0100, Markus Armbruster wrote:
> Nathan Baum <nathan@parenthephobia.org.uk> writes:
> 
> > Returns a QObject with information about a USB device.
> >
> > Signed-off-by: Nathan Baum <nathan@parenthephobia.org.uk>
> > ---
> >  hw/usb-bus.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/usb-bus.c b/hw/usb-bus.c
> > index 54027df..6d02807 100644
> > --- a/hw/usb-bus.c
> > +++ b/hw/usb-bus.c
> > @@ -3,6 +3,7 @@
> >  #include "qdev.h"
> >  #include "sysemu.h"
> >  #include "monitor.h"
> > +#include "qjson.h"
> >  
> >  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
> >  
> > @@ -232,6 +233,18 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
> >                     dev->attached ? ", attached" : "");
> >  }
> >  
> > +static QObject *usb_bus_dev_info(Monitor *mon, DeviceState *qdev)
> > +{
> > +    USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
> > +    USBBus *bus = usb_bus_from_device(dev);
> > +    return qobject_from_jsonf("{'busnr': %d, 'addr':%d, 'speed': %s, 'desc': %s, 'attached': %i}",
> > +                              bus->busnr,
> 
> As for PCI, 'busnr' belongs to the bus, not the device.

Hmm. In cases like this, is it appropriate to modify the output of the
existing "info qtree" when it is modified to use the QObject data?

Would it be sensible to go the (probably small amount of) effort to
change the print functions to print exactly they do now, and put changes
to their output in different patches so they can easily be dropped if
necessary?

> Hmm, we don't have the infrastructure to return bus information, yet.
> "info qtree" hardcodes printing of name and type.  Gerd, what do you
> think?
> 
> > +                              dev->addr,
> > +                              usb_speed(dev->speed),
> > +                              dev->product_desc,
> > +                              dev->attached);
> > +}
> > +
> >  void usb_info(Monitor *mon)
> >  {
> >      USBBus *bus;

Re: [Qemu-devel] [RFC,PATCH 08/11] qdev: Add usb_bus_dev_info

[Markus Armbruster]

Nathan Baum <nathan@parenthephobia.org.uk> writes:

> On Fri, 2010-01-15 at 19:14 +0100, Markus Armbruster wrote:
>> Nathan Baum <nathan@parenthephobia.org.uk> writes:
>> 
>> > Returns a QObject with information about a USB device.
>> >
>> > Signed-off-by: Nathan Baum <nathan@parenthephobia.org.uk>
>> > ---
>> >  hw/usb-bus.c |   13 +++++++++++++
>> >  1 files changed, 13 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>> > index 54027df..6d02807 100644
>> > --- a/hw/usb-bus.c
>> > +++ b/hw/usb-bus.c
>> > @@ -3,6 +3,7 @@
>> >  #include "qdev.h"
>> >  #include "sysemu.h"
>> >  #include "monitor.h"
>> > +#include "qjson.h"
>> >  
>> >  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
>> >  
>> > @@ -232,6 +233,18 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
>> >                     dev->attached ? ", attached" : "");
>> >  }
>> >  
>> > +static QObject *usb_bus_dev_info(Monitor *mon, DeviceState *qdev)
>> > +{
>> > +    USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
>> > +    USBBus *bus = usb_bus_from_device(dev);
>> > +    return qobject_from_jsonf("{'busnr': %d, 'addr':%d, 'speed': %s, 'desc': %s, 'attached': %i}",
>> > +                              bus->busnr,
>> 
>> As for PCI, 'busnr' belongs to the bus, not the device.
>
> Hmm. In cases like this, is it appropriate to modify the output of the
> existing "info qtree" when it is modified to use the QObject data?
>
> Would it be sensible to go the (probably small amount of) effort to
> change the print functions to print exactly they do now, and put changes
> to their output in different patches so they can easily be dropped if
> necessary?

We might want to change info qtree output anyway if we show bus
information separately there...

>> Hmm, we don't have the infrastructure to return bus information, yet.
>> "info qtree" hardcodes printing of name and type.  Gerd, what do you
>> think?

... as I implied here.

Regardless, I think we should first decide what data we want to transmit
over QMP, and how to structure it, then figure out if and how to change
its human readable presentation.

[...]

Re: [Qemu-devel] [RFC,PATCH 08/11] qdev: Add usb_bus_dev_info

[Gerd Hoffmann]

On 01/18/10 11:15, Markus Armbruster wrote:
> Nathan Baum<nathan@parenthephobia.org.uk>  writes:
>
>>>> +static QObject *usb_bus_dev_info(Monitor *mon, DeviceState *qdev)
>>>> +{
>>>> +    USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
>>>> +    USBBus *bus = usb_bus_from_device(dev);
>>>> +    return qobject_from_jsonf("{'busnr': %d, 'addr':%d, 'speed': %s, 'desc': %s, 'attached': %i}",
>>>> +                              bus->busnr,
>>>
>>> As for PCI, 'busnr' belongs to the bus, not the device.

You want be able to figure which bus the device is attached to.

I think you actually can because the command returns the device tree 
converted into a qobject tree, correct?

Note: busnr is *not* fixed, it can be changed by the guest (maybe not 
for the primary, but surely for any secondary by writing to a pci bridge 
register).

>> Hmm. In cases like this, is it appropriate to modify the output of the
>> existing "info qtree" when it is modified to use the QObject data?
 >>
>> Would it be sensible to go the (probably small amount of) effort to
>> change the print functions to print exactly they do now, and put changes
>> to their output in different patches so they can easily be dropped if
>> necessary?
>
> We might want to change info qtree output anyway if we show bus
> information separately there...

Indeed.

>>> Hmm, we don't have the infrastructure to return bus information, yet.
>>> "info qtree" hardcodes printing of name and type.  Gerd, what do you
>>> think?

Adding a ->print_bus() function (or whatever the qmp aequivalent will 
be) to BusInfo is fine with me.

> Regardless, I think we should first decide what data we want to transmit
> over QMP, and how to structure it, then figure out if and how to change
> its human readable presentation.

Sounds like a plan ;)

cheers,
   Gerd

Re: [Qemu-devel] [RFC,PATCH 08/11] qdev: Add usb_bus_dev_info

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On 01/18/10 11:15, Markus Armbruster wrote:
>> Nathan Baum<nathan@parenthephobia.org.uk>  writes:
>>
>>>>> +static QObject *usb_bus_dev_info(Monitor *mon, DeviceState *qdev)
>>>>> +{
>>>>> +    USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
>>>>> +    USBBus *bus = usb_bus_from_device(dev);
>>>>> +    return qobject_from_jsonf("{'busnr': %d, 'addr':%d, 'speed': %s, 'desc': %s, 'attached': %i}",
>>>>> +                              bus->busnr,
>>>>
>>>> As for PCI, 'busnr' belongs to the bus, not the device.
>
> You want be able to figure which bus the device is attached to.
>
> I think you actually can because the command returns the device tree
> converted into a qobject tree, correct?

Correct.

[...]

Re: [Qemu-devel] [RFC,PATCH 06/11] qdev: sysbus_dev_info

[Markus Armbruster]

Nathan Baum <nathan@parenthephobia.org.uk> writes:

> Returns information about the system bus as a QObject.
>
> Signed-off-by: Nathan Baum <nathan@parenthephobia.org.uk>
> ---
>  hw/sysbus.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/hw/sysbus.c b/hw/sysbus.c
> index 1f7f138..2092d9f 100644
> --- a/hw/sysbus.c
> +++ b/hw/sysbus.c
> @@ -20,6 +20,7 @@
>  #include "sysbus.h"
>  #include "sysemu.h"
>  #include "monitor.h"
> +#include "qjson.h"
>  
>  static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>  
> @@ -170,3 +171,20 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>                         indent, "", s->mmio[i].addr, s->mmio[i].size);
>      }
>  }
> +
> +static QObject *sysbus_dev_info(Monitor *mon, DeviceState *dev)
> +{
> +    SysBusDevice *s = sysbus_from_qdev(dev);
> +    QDict *dict = qdict_new();
> +    QList *list = qlist_new();
> +    int i;
> +    
> +    for (i = 0; i < s->num_mmio; i++) {
> +        qlist_append_obj(list, qobject_from_jsonf("{ 'addr': " TARGET_FMT_plx ", 'size': " TARGET_FMT_plx " }",
> +                                                  s->mmio[i].addr, s->mmio[i].size));
> +    }
> +    
> +    qdict_put(dict, "mmio", list);
> +    return (QObject *) dict;

I think Luiz wants you to use QOBJECT() here.

> +    
> +}

Re: [Qemu-devel] [RFC, PATCH 10/11] qdev: Add do_info_qbus and friends.

[Markus Armbruster]

Nathan Baum <nathan@parenthephobia.org.uk> writes:

> Places information about a bus and the devices on into a QObject.
>
> Signed-off-by: Nathan Baum <nathan@parenthephobia.org.uk>
> ---
>  hw/qdev.c |   73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 73 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index b6bd4ae..f5d68c6 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -30,6 +30,8 @@
>  #include "sysemu.h"
>  #include "monitor.h"
>  #include "qerror.h"
> +#include "qint.h"
> +#include "qbool.h"
>  
>  static int qdev_hotplug = 0;
>  
> @@ -654,6 +656,77 @@ void qbus_free(BusState *bus)
>      }
>  }
>  
> +static void do_info_qbus(Monitor *mon, BusState *bus, QObject **ret_data);
> +
> +static void do_info_qdev_props(Monitor *mon, DeviceState *dev, Property *props, const char *prefix,

Long line, please break; there's plenty of space left in the next one :)

> +                               QDict *qdict)
> +{
> +    char name[64];
> +    char value[64];
> +
> +    if (!props)
> +        return;
> +    while (props->name) {
> +        if (props->info->print) {
> +            props->info->print(dev, props, value, sizeof(value));
> +            snprintf(name, sizeof(name), "%s-%s", prefix, props->name);

Funny naming convention PREFIX-PROPNAME, because do_info_qdev_props()
below puts properties directly in the qdict.

Maybe it would be cleaner to have them in their own dictionary "props".

However, because there are both device properties and bus properties
(really: device properties common to all devices on this bus), their
names can clash.  Device properties take precedence (see
qdev_prop_find()).  Hmm, qdev_printf() prints even overridden bus
properties, not sure that's appropriate.  Gerd?

> +            qdict_put(qdict, name, qstring_from_str(value));
> +        }
> +        props++;
> +    }
> +}
> +
> +static void do_info_qdev(Monitor *mon, DeviceState *dev, QObject **ret_data)
> +{
> +    BusState *child;
> +    QDict *qdict;
> +    QList *children;
> +
> +    qdict = qdict_new();
> +    qdict_put(qdict, "name", qstring_from_str(dev->info->name));
> +    if (dev->id)
> +        qdict_put(qdict, "id", qstring_from_str(dev->id));
> +    qdict_put(qdict, "gpio-in", qint_from_int(dev->num_gpio_in));
> +    qdict_put(qdict, "gpio-out", qint_from_int(dev->num_gpio_out));
> +    do_info_qdev_props(mon, dev, dev->info->props, "dev", qdict);
> +    do_info_qdev_props(mon, dev, dev->parent_bus->info->props, "bus", qdict);
> +    children = qlist_new();
> +    QLIST_FOREACH(child, &dev->child_bus, sibling) {
> +        QObject *data = NULL;
> +        do_info_qbus(mon, child, &data);
> +        if (data)
> +            qlist_append_obj(children, data);
> +    }
> +    if (!qlist_empty(children))
> +        qdict_put(qdict, "children", children);
> +    if (dev->parent_bus->info->info_dev) {
> +        qdict_put_obj(qdict, "info", dev->parent_bus->info->info_dev(mon, dev));
> +    }
> +    *ret_data = (QObject *) qdict;

Use QOBJECT().

> +}
> +
> +static void do_info_qbus(Monitor *mon, BusState *bus, QObject **ret_data)
> +{
> +    struct DeviceState *dev;
> +    QDict *qdict;
> +    QList *children;
> +
> +    qdict = qdict_new();
> +    qdict_put(qdict, "bus", qstring_from_str(bus->name));
> +    qdict_put(qdict, "type", qstring_from_str(bus->info->name));
> +    qdict_put(qdict, "allow_hotplug", qbool_from_int(bus->allow_hotplug));
> +    children = qlist_new();
> +    qdict_put(qdict, "children", children);
> +    QLIST_FOREACH(dev, &bus->children, sibling) {
> +        QObject *data = NULL;
> +        do_info_qdev(mon, dev, &data);
> +        if (data)
> +            qlist_append_obj(children, data);
> +    }
> +    
> +    *ret_data = (QObject *) qdict;

Use QOBJECT().

> +}
> +
>  #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__)
>  static void qbus_print(Monitor *mon, BusState *bus, int indent);

Re: [Qemu-devel] [RFC, PATCH 10/11] qdev: Add do_info_qbus and friends.

[Gerd Hoffmann]

> However, because there are both device properties and bus properties
> (really: device properties common to all devices on this bus), their
> names can clash.  Device properties take precedence (see
> qdev_prop_find()).  Hmm, qdev_printf() prints even overridden bus
> properties, not sure that's appropriate.  Gerd?

IMHO they must not clash.  This isn't enforced in any way though.

cheers,
   Gerd

Re: [Qemu-devel] [RFC, PATCH 10/11] qdev: Add do_info_qbus and friends.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>> However, because there are both device properties and bus properties
>> (really: device properties common to all devices on this bus), their
>> names can clash.  Device properties take precedence (see
>> qdev_prop_find()).  Hmm, qdev_printf() prints even overridden bus
>> properties, not sure that's appropriate.  Gerd?
>
> IMHO they must not clash.  This isn't enforced in any way though.

If they must not clash, then it makes no sense to invent a fancy prefix
to cope with clashes, I think.

Alternatively, we could declare devices overriding properties inherited
from the bus a feature.  Does it make any sense to show the shadowed
properties then?

Re: [Qemu-devel] [RFC, PATCH 10/11] qdev: Add do_info_qbus and friends.

[Gerd Hoffmann]

On 01/18/10 13:34, Markus Armbruster wrote:
>>> However, because there are both device properties and bus properties
>>> (really: device properties common to all devices on this bus), their
>>> names can clash.  Device properties take precedence (see
>>> qdev_prop_find()).  Hmm, qdev_printf() prints even overridden bus
>>> properties, not sure that's appropriate.  Gerd?
>>
>> IMHO they must not clash.  This isn't enforced in any way though.
>
> If they must not clash, then it makes no sense to invent a fancy prefix
> to cope with clashes, I think.

I've added the bus- and dev- prefixes to make clear where the properties 
come from (BusInfo or DeviceInfo) for informational purposes, not to 
avoid clashes.  We could just drop that ...

> Alternatively, we could declare devices overriding properties inherited
> from the bus a feature.

No, this is just asking for trouble IMHO.

cheers,
   Gerd

Re: [Qemu-devel] [RFC,PATCH 00/11] Half-convert info qtree to QMP.

[Markus Armbruster]

Nathan Baum <nathan@parenthephobia.org.uk> writes:

> Hullo.
>
> This series of patches partially converts info qtree to QMP.
>
> I've gone halfway: one can use query-qtree in QMP.
>
> I haven't converted the old monitor function other than to rename it;
> do_info_qtree_print just ignores the QObject it is passed and prints
> the qtree the old-fashioned way.
>
>  hw/isa-bus.c |   19 +++++++++++++
>  hw/pci.c     |   50 +++++++++++++++++++++++++++++++++++
>  hw/qdev.c    |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/qdev.h    |    5 +++-
>  hw/sysbus.c  |   20 ++++++++++++++
>  hw/usb-bus.c |   15 ++++++++++
>  monitor.c    |    3 +-
>  7 files changed, 191 insertions(+), 3 deletions(-)

Patch 2 adds a static function, patch 3 puts it to use.  Not bisectable
with -Werror.  Merge the two.  Same for patch 4+5, 6+7, 8+9, 10+11.

Completing the job shouldn't be hard, just change the print_dev methods
to format the value returned by the info_dev method instead of getting
the information out of the device.

Thanks!