| Message ID | 517EB04E.1060204@greensocs.com |
|---|---|
| State | New |
| Headers | show |
Am 29.04.2013 19:39, schrieb KONRAD Frédéric: > On 29/04/2013 18:32, Paolo Bonzini wrote: >> Il 29/04/2013 18:28, KONRAD Frédéric ha scritto: >>>> Could this be simply a qdev property? >>> Yes, that can be a good idea. >>> >>> What about adding a qdev property bus_name and using it in qbus_realize? >>> >>> Like this: >>> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>> index 4eb0134..c5d5407 100644 >>> --- a/hw/core/qdev.c >>> +++ b/hw/core/qdev.c >>> @@ -421,6 +421,13 @@ static void qbus_realize(BusState *bus, DeviceState >>> *parent, const char *name) >>> >>> if (name) { >>> bus->name = g_strdup(name); >>> + } else if (bus->parent && bus->parent->bus_name) { >>> + /* parent device has bus_name -> use it for bus name */ >>> + len = strlen(bus->parent->bus_name) + 16; >>> + buf = g_malloc(len); >>> + snprintf(buf, len, "%s.%d", bus->parent->bus_name, >>> + bus->parent->num_child_bus); >>> + bus->name = buf; >>> } else if (bus->parent && bus->parent->id) { >>> /* parent device has id -> use it for bus name */ >>> len = strlen(bus->parent->id) + 16; >>> >>> If so, change to scsi_bus_new is not needed and the two new functions >>> are >>> not needed. >>> >>> Is that making sense? >> Ah, that's a bit more extreme. :) >> >> I think I like it, but I need more input. >> >> Paolo > Well, just for virtio-scsi-pci, something like that: > > --- > hw/core/qdev.c | 14 ++++++++++++++ > hw/virtio/virtio-pci.c | 9 +++++++++ > include/hw/qdev-core.h | 4 ++++ > 3 files changed, 27 insertions(+) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 4eb0134..3aa0082 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -342,6 +342,13 @@ BusState *qdev_get_child_bus(DeviceState *dev, > const char *name) > return NULL; > } > > +void qdev_set_bus_name(DeviceState *dev, const char *bus_name) device_... for new functions please. :) > +{ Might be called multiple times, so better insert: if (dev->bus_name) { g_free(dev->bus_name); dev->bus_name = NULL; } > + if (bus_name) { > + dev->bus_name = g_strdup(bus_name); > + } > +} > + > int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, > qbus_walkerfn *busfn, void *opaque) > { > @@ -421,6 +428,13 @@ static void qbus_realize(BusState *bus, DeviceState > *parent, const char *name) > > if (name) { > bus->name = g_strdup(name); > + } else if (bus->parent && bus->parent->bus_name) { > + /* parent device has bus_name -> use it for bus name */ This seems backwards to me. qdev had made sure to have a 1:n relationship between device and bus, whereas you are making the name template 1:1 here. Don't you rather want a qbus_set_name() mechanism operating on the bus? > + len = strlen(bus->parent->bus_name) + 16; Why 15 decimal digits? Is there any constant we could reuse? > + buf = g_malloc(len); > + snprintf(buf, len, "%s.%d", bus->parent->bus_name, > + bus->parent->num_child_bus); > + bus->name = buf; > } else if (bus->parent && bus->parent->id) { > /* parent device has id -> use it for bus name */ > len = strlen(bus->parent->id) + 16; > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 070df44..8d5d146 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1106,11 +1106,20 @@ static int > virtio_scsi_pci_init_pci(VirtIOPCIProxy *vpci_dev) > VirtIOSCSIPCI *dev = VIRTIO_SCSI_PCI(vpci_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); > + DeviceState *proxy = DEVICE(dev); > > if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { > vpci_dev->nvectors = vs->conf.num_queues + 3; > } > > + /* > + * For command line compatibility, this set the virtio-scsi-device bus "sets" > + * name as before. > + */ > + if (proxy->id) { > + qdev_set_bus_name(vdev, proxy->id); > + } > + > qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); > if (qdev_init(vdev) < 0) { > return -1; > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index cf83d54..332e11f 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -125,6 +125,7 @@ struct DeviceState { > int num_child_bus; > int instance_id_alias; > int alias_required_for_version; > + const char *bus_name; > }; > > #define TYPE_BUS "bus" > @@ -224,6 +225,9 @@ BusState *qdev_get_child_bus(DeviceState *dev, const > char *name); > void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n); > void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n); > > +/* Set the bus_name property. */ > +void qdev_set_bus_name(DeviceState *dev, const char *bus_name); > + > BusState *qdev_get_parent_bus(DeviceState *dev); > > /*** BUS API. ***/ Andreas
On 29/04/2013 19:55, Andreas Färber wrote: > Am 29.04.2013 19:39, schrieb KONRAD Frédéric: >> On 29/04/2013 18:32, Paolo Bonzini wrote: >>> Il 29/04/2013 18:28, KONRAD Frédéric ha scritto: >>>>> Could this be simply a qdev property? >>>> Yes, that can be a good idea. >>>> >>>> What about adding a qdev property bus_name and using it in qbus_realize? >>>> >>>> Like this: >>>> >>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>>> index 4eb0134..c5d5407 100644 >>>> --- a/hw/core/qdev.c >>>> +++ b/hw/core/qdev.c >>>> @@ -421,6 +421,13 @@ static void qbus_realize(BusState *bus, DeviceState >>>> *parent, const char *name) >>>> >>>> if (name) { >>>> bus->name = g_strdup(name); >>>> + } else if (bus->parent && bus->parent->bus_name) { >>>> + /* parent device has bus_name -> use it for bus name */ >>>> + len = strlen(bus->parent->bus_name) + 16; >>>> + buf = g_malloc(len); >>>> + snprintf(buf, len, "%s.%d", bus->parent->bus_name, >>>> + bus->parent->num_child_bus); >>>> + bus->name = buf; >>>> } else if (bus->parent && bus->parent->id) { >>>> /* parent device has id -> use it for bus name */ >>>> len = strlen(bus->parent->id) + 16; >>>> >>>> If so, change to scsi_bus_new is not needed and the two new functions >>>> are >>>> not needed. >>>> >>>> Is that making sense? >>> Ah, that's a bit more extreme. :) >>> >>> I think I like it, but I need more input. >>> >>> Paolo >> Well, just for virtio-scsi-pci, something like that: >> >> --- >> hw/core/qdev.c | 14 ++++++++++++++ >> hw/virtio/virtio-pci.c | 9 +++++++++ >> include/hw/qdev-core.h | 4 ++++ >> 3 files changed, 27 insertions(+) >> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 4eb0134..3aa0082 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -342,6 +342,13 @@ BusState *qdev_get_child_bus(DeviceState *dev, >> const char *name) >> return NULL; >> } >> >> +void qdev_set_bus_name(DeviceState *dev, const char *bus_name) > device_... for new functions please. :) > >> +{ > > Might be called multiple times, so better insert: > > if (dev->bus_name) { > g_free(dev->bus_name); > dev->bus_name = NULL; > } > >> + if (bus_name) { >> + dev->bus_name = g_strdup(bus_name); >> + } >> +} >> + >> int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, >> qbus_walkerfn *busfn, void *opaque) >> { >> @@ -421,6 +428,13 @@ static void qbus_realize(BusState *bus, DeviceState >> *parent, const char *name) >> >> if (name) { >> bus->name = g_strdup(name); >> + } else if (bus->parent && bus->parent->bus_name) { >> + /* parent device has bus_name -> use it for bus name */ > This seems backwards to me. qdev had made sure to have a 1:n > relationship between device and bus, whereas you are making the name > template 1:1 here. Don't you rather want a qbus_set_name() mechanism > operating on the bus? What do you mean with the relationship 1:n / 1:1 I don't understand? Note: I did that quickly to ask Paolo if it was worth doing like that or like the original series. > >> + len = strlen(bus->parent->bus_name) + 16; > Why 15 decimal digits? Is there any constant we could reuse? > >> + buf = g_malloc(len); >> + snprintf(buf, len, "%s.%d", bus->parent->bus_name, >> + bus->parent->num_child_bus); >> + bus->name = buf; >> } else if (bus->parent && bus->parent->id) { >> /* parent device has id -> use it for bus name */ >> len = strlen(bus->parent->id) + 16; >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index 070df44..8d5d146 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -1106,11 +1106,20 @@ static int >> virtio_scsi_pci_init_pci(VirtIOPCIProxy *vpci_dev) >> VirtIOSCSIPCI *dev = VIRTIO_SCSI_PCI(vpci_dev); >> DeviceState *vdev = DEVICE(&dev->vdev); >> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); >> + DeviceState *proxy = DEVICE(dev); >> >> if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { >> vpci_dev->nvectors = vs->conf.num_queues + 3; >> } >> >> + /* >> + * For command line compatibility, this set the virtio-scsi-device bus > "sets" > >> + * name as before. >> + */ >> + if (proxy->id) { >> + qdev_set_bus_name(vdev, proxy->id); >> + } >> + >> qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); >> if (qdev_init(vdev) < 0) { >> return -1; >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index cf83d54..332e11f 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -125,6 +125,7 @@ struct DeviceState { >> int num_child_bus; >> int instance_id_alias; >> int alias_required_for_version; >> + const char *bus_name; >> }; >> >> #define TYPE_BUS "bus" >> @@ -224,6 +225,9 @@ BusState *qdev_get_child_bus(DeviceState *dev, const >> char *name); >> void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n); >> void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n); >> >> +/* Set the bus_name property. */ >> +void qdev_set_bus_name(DeviceState *dev, const char *bus_name); >> + >> BusState *qdev_get_parent_bus(DeviceState *dev); >> >> /*** BUS API. ***/ > Andreas >
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 4eb0134..3aa0082 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -342,6 +342,13 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name) return NULL; } +void qdev_set_bus_name(DeviceState *dev, const char *bus_name) +{ + if (bus_name) { + dev->bus_name = g_strdup(bus_name); + } +} + int qbus_walk_children(BusState *bus, qdev_walkerfn *devfn, qbus_walkerfn *busfn, void *opaque) { @@ -421,6 +428,13 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name) if (name) { bus->name = g_strdup(name); + } else if (bus->parent && bus->parent->bus_name) { + /* parent device has bus_name -> use it for bus name */ + len = strlen(bus->parent->bus_name) + 16; + buf = g_malloc(len); + snprintf(buf, len, "%s.%d", bus->parent->bus_name, + bus->parent->num_child_bus); + bus->name = buf; } else if (bus->parent && bus->parent->id) { /* parent device has id -> use it for bus name */ len = strlen(bus->parent->id) + 16; diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 070df44..8d5d146 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1106,11 +1106,20 @@ static int virtio_scsi_pci_init_pci(VirtIOPCIProxy *vpci_dev) VirtIOSCSIPCI *dev = VIRTIO_SCSI_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); + DeviceState *proxy = DEVICE(dev); if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { vpci_dev->nvectors = vs->conf.num_queues + 3; } + /* + * For command line compatibility, this set the virtio-scsi-device bus + * name as before. + */ + if (proxy->id) { + qdev_set_bus_name(vdev, proxy->id); + } + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); if (qdev_init(vdev) < 0) { return -1; diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index cf83d54..332e11f 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -125,6 +125,7 @@ struct DeviceState { int num_child_bus; int instance_id_alias; int alias_required_for_version; + const char *bus_name; }; #define TYPE_BUS "bus" @@ -224,6 +225,9 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name); void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n); void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n); +/* Set the bus_name property. */ +void qdev_set_bus_name(DeviceState *dev, const char *bus_name); + BusState *qdev_get_parent_bus(DeviceState *dev);