| Message ID | 20130418104156.7a5349f0@nial.usersys.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 18 Apr 2013 10:41:56 +0200 Igor Mammedov <imammedo@redhat.com> wrote: [...] > > - if (!bus) { > > - bus = sysbus_get_default(); > > - } > > - > I've checked all direct childs of TYPE_DEVICE and they all set k->bus_type, > with only one exception of TYPE_CPU. So it should be safe to remove fallback > from qdev_device_add POV. > However TYPE_CPU breaks assumption that device always has parent_bus set > to not NULL in qdev_unplug() and qdev_print() Err, qdev_print() is safe since it's called on bus children only, so it has parent_bus. > > It would be better to add something like this: > // untested > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 4eb0134..45009ba 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) > { > DeviceClass *dc = DEVICE_GET_CLASS(dev); > > - if (!dev->parent_bus->allow_hotplug) { > + if (dev->parent_bus && !dev->parent_bus->allow_hotplug) { > error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); > return; > } > diff --git a/qdev-monitor.c b/qdev-monitor.c > index 9a78ccf..2476e4e 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState *dev, > int indent) qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent); > class = object_class_get_parent(class); > } while (class != object_class_by_name(TYPE_DEVICE)); > - bus_print_dev(dev->parent_bus, mon, dev, indent); > + if (dev->parent_bus) { > + bus_print_dev(dev->parent_bus, mon, dev, indent); > + } > QLIST_FOREACH(child, &dev->child_bus, sibling) { > qbus_print(mon, child, indent); > } > > > /* create device, set properties */ > > qdev = DEVICE(object_new(driver)); > > - qdev_set_parent_bus(qdev, bus); > > + > > + if (bus) { > > + qdev_set_parent_bus(qdev, bus); > > + } > > > > id = qemu_opts_id(opts); > > if (id) { > >
Hi Igor,
When I use the config below,an error occurs.Is there anything wrong?
Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 -chardev spicevmc,id=charchannel0,name=vdagent -device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 -drive file=/home/img/win7.qed,if=virtio,index=0,format=qed -monitor stdio -vga qxl -vnc :1
Error output:
-device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: Bus 'virtio-serial0.0' is full
-device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: Bus 'virtio-serial0.0' not found
Any feedback are appliciated.
-----Original Message-----
From: qemu-devel-bounces+libaiqing=huawei.com@nongnu.org [mailto:qemu-devel-bounces+libaiqing=huawei.com@nongnu.org] On Behalf Of Igor Mammedov
Sent: Thursday, April 18, 2013 5:02 PM
To: Igor Mammedov
Cc: ehabkost@redhat.com; qemu-devel@nongnu.org; armbru@redhat.com; anthony@codemonkey.ws; pbonzini@redhat.com; lcapitulino@redhat.com; Andreas Färber
Subject: Re: [Qemu-devel] [PATCH] qdev: Fix device_add bus assumptions
On Thu, 18 Apr 2013 10:41:56 +0200
Igor Mammedov <imammedo@redhat.com> wrote:
[...]
> > - if (!bus) {
> > - bus = sysbus_get_default();
> > - }
> > -
> I've checked all direct childs of TYPE_DEVICE and they all set k->bus_type,
> with only one exception of TYPE_CPU. So it should be safe to remove fallback
> from qdev_device_add POV.
> However TYPE_CPU breaks assumption that device always has parent_bus set
> to not NULL in qdev_unplug() and qdev_print()
Err, qdev_print() is safe since it's called on bus children only, so it has
parent_bus.
>
> It would be better to add something like this:
> // untested
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4eb0134..45009ba 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> {
> DeviceClass *dc = DEVICE_GET_CLASS(dev);
>
> - if (!dev->parent_bus->allow_hotplug) {
> + if (dev->parent_bus && !dev->parent_bus->allow_hotplug) {
> error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> return;
> }
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 9a78ccf..2476e4e 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState *dev,
> int indent) qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
> class = object_class_get_parent(class);
> } while (class != object_class_by_name(TYPE_DEVICE));
> - bus_print_dev(dev->parent_bus, mon, dev, indent);
> + if (dev->parent_bus) {
> + bus_print_dev(dev->parent_bus, mon, dev, indent);
> + }
> QLIST_FOREACH(child, &dev->child_bus, sibling) {
> qbus_print(mon, child, indent);
> }
>
> > /* create device, set properties */
> > qdev = DEVICE(object_new(driver));
> > - qdev_set_parent_bus(qdev, bus);
> > +
> > + if (bus) {
> > + qdev_set_parent_bus(qdev, bus);
> > + }
> >
> > id = qemu_opts_id(opts);
> > if (id) {
>
>
Hi, Am 22.04.2013 13:51, schrieb Libaiqing: > When I use the config below,an error occurs.Is there anything wrong? > > Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 -chardev spicevmc,id=charchannel0,name=vdagent -device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 -drive file=/home/img/win7.qed,if=virtio,index=0,format=qed -monitor stdio -vga qxl -vnc :1 > > Error output: > -device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: Bus 'virtio-serial0.0' is full > -device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: Bus 'virtio-serial0.0' not found > > Any feedback are appliciated. This does not sound related to this patch at all... Instead it sounds as if the virtio refactorings had some effect not only on virtio-net but also on virtio-serial. Fred? Andreas > -----Original Message----- > From: qemu-devel-bounces+libaiqing=huawei.com@nongnu.org [mailto:qemu-devel-bounces+libaiqing=huawei.com@nongnu.org] On Behalf Of Igor Mammedov > Sent: Thursday, April 18, 2013 5:02 PM > To: Igor Mammedov > Cc: ehabkost@redhat.com; qemu-devel@nongnu.org; armbru@redhat.com; anthony@codemonkey.ws; pbonzini@redhat.com; lcapitulino@redhat.com; Andreas Färber > Subject: Re: [Qemu-devel] [PATCH] qdev: Fix device_add bus assumptions > > On Thu, 18 Apr 2013 10:41:56 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > [...] >>> - if (!bus) { >>> - bus = sysbus_get_default(); >>> - } >>> - >> I've checked all direct childs of TYPE_DEVICE and they all set k->bus_type, >> with only one exception of TYPE_CPU. So it should be safe to remove fallback >> from qdev_device_add POV. >> However TYPE_CPU breaks assumption that device always has parent_bus set >> to not NULL in qdev_unplug() and qdev_print() > Err, qdev_print() is safe since it's called on bus children only, so it has > parent_bus. > >> >> It would be better to add something like this: >> // untested >> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 4eb0134..45009ba 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) >> { >> DeviceClass *dc = DEVICE_GET_CLASS(dev); >> >> - if (!dev->parent_bus->allow_hotplug) { >> + if (dev->parent_bus && !dev->parent_bus->allow_hotplug) { >> error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); >> return; >> } >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 9a78ccf..2476e4e 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState *dev, >> int indent) qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent); >> class = object_class_get_parent(class); >> } while (class != object_class_by_name(TYPE_DEVICE)); >> - bus_print_dev(dev->parent_bus, mon, dev, indent); >> + if (dev->parent_bus) { >> + bus_print_dev(dev->parent_bus, mon, dev, indent); >> + } >> QLIST_FOREACH(child, &dev->child_bus, sibling) { >> qbus_print(mon, child, indent); >> } >> >>> /* create device, set properties */ >>> qdev = DEVICE(object_new(driver)); >>> - qdev_set_parent_bus(qdev, bus); >>> + >>> + if (bus) { >>> + qdev_set_parent_bus(qdev, bus); >>> + } >>> >>> id = qemu_opts_id(opts); >>> if (id) { >> >> > >
Am 18.04.2013 10:41, schrieb Igor Mammedov: > On Tue, 16 Apr 2013 03:50:21 +0200 > Andreas Färber <afaerber@suse.de> wrote: > >> Drop an unreachable fallback bus assignment to SysBus. >> >> If no ,bus= is specified, only search busses recursively for bus type if >> the DeviceClass has a bus_type specified. Handle resulting NULL cases. >> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> qdev-monitor.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index 9a78ccf..73d7946 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -18,6 +18,7 @@ >> */ >> >> #include "hw/qdev.h" >> +#include "hw/sysbus.h" >> #include "monitor/monitor.h" >> #include "monitor/qdev.h" >> #include "qmp-commands.h" >> @@ -415,7 +416,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> DeviceClass *k; >> const char *driver, *path, *id; >> DeviceState *qdev; >> - BusState *bus; >> + BusState *bus = NULL; >> >> driver = qemu_opt_get(opts, "driver"); >> if (!driver) { >> @@ -453,7 +454,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> driver, object_get_typename(OBJECT(bus))); >> return NULL; >> } >> - } else { >> + } else if (k->bus_type != NULL) { >> bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type); >> if (!bus) { >> qerror_report(QERR_NO_BUS_FOR_DEVICE, >> @@ -461,18 +462,17 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> return NULL; >> } >> } >> - if (qdev_hotplug && !bus->allow_hotplug) { >> + if (qdev_hotplug && bus && !bus->allow_hotplug) { >> qerror_report(QERR_BUS_NO_HOTPLUG, bus->name); >> return NULL; >> } >> >> - if (!bus) { >> - bus = sysbus_get_default(); >> - } >> - > I've checked all direct childs of TYPE_DEVICE and they all set k->bus_type, > with only one exception of TYPE_CPU. > So it should be safe to remove fallback > from qdev_device_add POV. Sorry, don't understand what exactly you are suggesting? > However TYPE_CPU breaks assumption that device always has parent_bus set to not > NULL in qdev_unplug() and qdev_print() > > It would be better to add something like this: > // untested > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 4eb0134..45009ba 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) > { > DeviceClass *dc = DEVICE_GET_CLASS(dev); > > - if (!dev->parent_bus->allow_hotplug) { > + if (dev->parent_bus && !dev->parent_bus->allow_hotplug) { > error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); > return; > } This part looks good, can you resend as a proper patch? Even if we can't test the no-parent_bus unplug path ATM, it shouldn't hurt to fix assumptions about it. Andreas > diff --git a/qdev-monitor.c b/qdev-monitor.c > index 9a78ccf..2476e4e 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent) > qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent); > class = object_class_get_parent(class); > } while (class != object_class_by_name(TYPE_DEVICE)); > - bus_print_dev(dev->parent_bus, mon, dev, indent); > + if (dev->parent_bus) { > + bus_print_dev(dev->parent_bus, mon, dev, indent); > + } > QLIST_FOREACH(child, &dev->child_bus, sibling) { > qbus_print(mon, child, indent); > } > >> /* create device, set properties */ >> qdev = DEVICE(object_new(driver)); >> - qdev_set_parent_bus(qdev, bus); >> + >> + if (bus) { >> + qdev_set_parent_bus(qdev, bus); >> + } >> >> id = qemu_opts_id(opts); >> if (id) { > >
On 22/04/2013 15:27, Andreas Färber wrote: > Hi, > > Am 22.04.2013 13:51, schrieb Libaiqing: >> When I use the config below,an error occurs.Is there anything wrong? >> >> Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 -chardev spicevmc,id=charchannel0,name=vdagent -device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 -drive file=/home/img/win7.qed,if=virtio,index=0,format=qed -monitor stdio -vga qxl -vnc :1 >> >> Error output: >> -device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: Bus 'virtio-serial0.0' is full >> -device virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: Bus 'virtio-serial0.0' not found >> >> Any feedback are appliciated. > This does not sound related to this patch at all... > > Instead it sounds as if the virtio refactorings had some effect not only > on virtio-net but also on virtio-serial. Fred? > > Andreas Hi, Yes, sounds like the same issue as virtio-net: bus: pci.0 type PCI dev: virtio-serial-pci, id "virtio-serial0" ioeventfd = off vectors = 2 class = 0x780 indirect_desc = on event_idx = on max_ports = 31 addr = 04.0 romfile = <null> rombar = 1 multifunction = off command_serr_enable = on class Class 0780, addr 00:04.0, pci id 1af4:1003 (sub 1af4:0003) bar 0: i/o at 0xc040 [0xc05f] bar 1: mem at 0xfebf1000 [0xfebf1fff] bus: virtio-serial0.0 type virtio-pci-bus dev: virtio-serial-device, id "" max_ports = 31 bus: virtio-serial-bus.0 type virtio-serial-bus The autogenerated bus name "deviceid.n" (virtio-serial0.0) became a virtio-bus... virtio-serial-bus.0 is the right bus to connect virtserialport. Any idea how to fix that? Sorry for that, Fred > >> -----Original Message----- >> From: qemu-devel-bounces+libaiqing=huawei.com@nongnu.org [mailto:qemu-devel-bounces+libaiqing=huawei.com@nongnu.org] On Behalf Of Igor Mammedov >> Sent: Thursday, April 18, 2013 5:02 PM >> To: Igor Mammedov >> Cc: ehabkost@redhat.com; qemu-devel@nongnu.org; armbru@redhat.com; anthony@codemonkey.ws; pbonzini@redhat.com; lcapitulino@redhat.com; Andreas Färber >> Subject: Re: [Qemu-devel] [PATCH] qdev: Fix device_add bus assumptions >> >> On Thu, 18 Apr 2013 10:41:56 +0200 >> Igor Mammedov <imammedo@redhat.com> wrote: >> >> [...] >>>> - if (!bus) { >>>> - bus = sysbus_get_default(); >>>> - } >>>> - >>> I've checked all direct childs of TYPE_DEVICE and they all set k->bus_type, >>> with only one exception of TYPE_CPU. So it should be safe to remove fallback >>> from qdev_device_add POV. >>> However TYPE_CPU breaks assumption that device always has parent_bus set >>> to not NULL in qdev_unplug() and qdev_print() >> Err, qdev_print() is safe since it's called on bus children only, so it has >> parent_bus. >> >>> It would be better to add something like this: >>> // untested >>> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>> index 4eb0134..45009ba 100644 >>> --- a/hw/core/qdev.c >>> +++ b/hw/core/qdev.c >>> @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) >>> { >>> DeviceClass *dc = DEVICE_GET_CLASS(dev); >>> >>> - if (!dev->parent_bus->allow_hotplug) { >>> + if (dev->parent_bus && !dev->parent_bus->allow_hotplug) { >>> error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); >>> return; >>> } >>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>> index 9a78ccf..2476e4e 100644 >>> --- a/qdev-monitor.c >>> +++ b/qdev-monitor.c >>> @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState *dev, >>> int indent) qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent); >>> class = object_class_get_parent(class); >>> } while (class != object_class_by_name(TYPE_DEVICE)); >>> - bus_print_dev(dev->parent_bus, mon, dev, indent); >>> + if (dev->parent_bus) { >>> + bus_print_dev(dev->parent_bus, mon, dev, indent); >>> + } >>> QLIST_FOREACH(child, &dev->child_bus, sibling) { >>> qbus_print(mon, child, indent); >>> } >>> >>>> /* create device, set properties */ >>>> qdev = DEVICE(object_new(driver)); >>>> - qdev_set_parent_bus(qdev, bus); >>>> + >>>> + if (bus) { >>>> + qdev_set_parent_bus(qdev, bus); >>>> + } >>>> >>>> id = qemu_opts_id(opts); >>>> if (id) { >>> >> >
Hi Fred, Am 22.04.2013 15:54, schrieb KONRAD Frédéric: > On 22/04/2013 15:27, Andreas Färber wrote: >> Am 22.04.2013 13:51, schrieb Libaiqing: >>> When I use the config below,an error occurs.Is there anything wrong? >>> >>> Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c >>> -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 >>> -chardev spicevmc,id=charchannel0,name=vdagent -device >>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 >>> -drive file=/home/img/win7.qed,if=virtio,index=0,format=qed -monitor >>> stdio -vga qxl -vnc :1 >>> >>> Error output: >>> -device >>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: >>> Bus 'virtio-serial0.0' is full >>> -device >>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: >>> Bus 'virtio-serial0.0' not found >>> >>> Any feedback are appliciated. >> This does not sound related to this patch at all... >> >> Instead it sounds as if the virtio refactorings had some effect not only >> on virtio-net but also on virtio-serial. Fred? > > Yes, sounds like the same issue as virtio-net: > > bus: pci.0 > type PCI > dev: virtio-serial-pci, id "virtio-serial0" > ioeventfd = off > vectors = 2 > class = 0x780 > indirect_desc = on > event_idx = on > max_ports = 31 > addr = 04.0 > romfile = <null> > rombar = 1 > multifunction = off > command_serr_enable = on > class Class 0780, addr 00:04.0, pci id 1af4:1003 (sub 1af4:0003) > bar 0: i/o at 0xc040 [0xc05f] > bar 1: mem at 0xfebf1000 [0xfebf1fff] > bus: virtio-serial0.0 > type virtio-pci-bus > dev: virtio-serial-device, id "" > max_ports = 31 > bus: virtio-serial-bus.0 > type virtio-serial-bus > > The autogenerated bus name "deviceid.n" (virtio-serial0.0) became a > virtio-bus... > > virtio-serial-bus.0 is the right bus to connect virtserialport. > > Any idea how to fix that? The only idea I can come up with right now is to overwrite the bus name on realize/qdev-init of the containing (virtio-serial-pci) device. Whether we want that is another question... :) It would fix command line backwards compatibility but would be inconsistent then; I guess the former is more important here. Regards, Andreas > > Sorry for that, > Fred > >> >>> -----Original Message----- >>> From: qemu-devel-bounces+libaiqing=huawei.com@nongnu.org >>> [mailto:qemu-devel-bounces+libaiqing=huawei.com@nongnu.org] On Behalf >>> Of Igor Mammedov >>> Sent: Thursday, April 18, 2013 5:02 PM >>> To: Igor Mammedov >>> Cc: ehabkost@redhat.com; qemu-devel@nongnu.org; armbru@redhat.com; >>> anthony@codemonkey.ws; pbonzini@redhat.com; lcapitulino@redhat.com; >>> Andreas Färber >>> Subject: Re: [Qemu-devel] [PATCH] qdev: Fix device_add bus assumptions >>> >>> On Thu, 18 Apr 2013 10:41:56 +0200 >>> Igor Mammedov <imammedo@redhat.com> wrote: >>> >>> [...] >>>>> - if (!bus) { >>>>> - bus = sysbus_get_default(); >>>>> - } >>>>> - >>>> I've checked all direct childs of TYPE_DEVICE and they all set >>>> k->bus_type, >>>> with only one exception of TYPE_CPU. So it should be safe to remove >>>> fallback >>>> from qdev_device_add POV. >>>> However TYPE_CPU breaks assumption that device always has >>>> parent_bus set >>>> to not NULL in qdev_unplug() and qdev_print() >>> Err, qdev_print() is safe since it's called on bus children only, so >>> it has >>> parent_bus. >>> >>>> It would be better to add something like this: >>>> // untested >>>> >>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>>> index 4eb0134..45009ba 100644 >>>> --- a/hw/core/qdev.c >>>> +++ b/hw/core/qdev.c >>>> @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) >>>> { >>>> DeviceClass *dc = DEVICE_GET_CLASS(dev); >>>> - if (!dev->parent_bus->allow_hotplug) { >>>> + if (dev->parent_bus && !dev->parent_bus->allow_hotplug) { >>>> error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); >>>> return; >>>> } >>>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>>> index 9a78ccf..2476e4e 100644 >>>> --- a/qdev-monitor.c >>>> +++ b/qdev-monitor.c >>>> @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState >>>> *dev, >>>> int indent) qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, >>>> indent); >>>> class = object_class_get_parent(class); >>>> } while (class != object_class_by_name(TYPE_DEVICE)); >>>> - bus_print_dev(dev->parent_bus, mon, dev, indent); >>>> + if (dev->parent_bus) { >>>> + bus_print_dev(dev->parent_bus, mon, dev, indent); >>>> + } >>>> QLIST_FOREACH(child, &dev->child_bus, sibling) { >>>> qbus_print(mon, child, indent); >>>> } >>>> >>>>> /* create device, set properties */ >>>>> qdev = DEVICE(object_new(driver)); >>>>> - qdev_set_parent_bus(qdev, bus); >>>>> + >>>>> + if (bus) { >>>>> + qdev_set_parent_bus(qdev, bus); >>>>> + } >>>>> id = qemu_opts_id(opts); >>>>> if (id) { >>>> >>> >> >
On 22/04/2013 16:02, Andreas Färber wrote: > Hi Fred, > > Am 22.04.2013 15:54, schrieb KONRAD Frédéric: >> On 22/04/2013 15:27, Andreas Färber wrote: >>> Am 22.04.2013 13:51, schrieb Libaiqing: >>>> When I use the config below,an error occurs.Is there anything wrong? >>>> >>>> Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c >>>> -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 >>>> -chardev spicevmc,id=charchannel0,name=vdagent -device >>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 >>>> -drive file=/home/img/win7.qed,if=virtio,index=0,format=qed -monitor >>>> stdio -vga qxl -vnc :1 >>>> >>>> Error output: >>>> -device >>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: >>>> Bus 'virtio-serial0.0' is full >>>> -device >>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: >>>> Bus 'virtio-serial0.0' not found >>>> >>>> Any feedback are appliciated. >>> This does not sound related to this patch at all... >>> >>> Instead it sounds as if the virtio refactorings had some effect not only >>> on virtio-net but also on virtio-serial. Fred? >> Yes, sounds like the same issue as virtio-net: >> >> bus: pci.0 >> type PCI >> dev: virtio-serial-pci, id "virtio-serial0" >> ioeventfd = off >> vectors = 2 >> class = 0x780 >> indirect_desc = on >> event_idx = on >> max_ports = 31 >> addr = 04.0 >> romfile = <null> >> rombar = 1 >> multifunction = off >> command_serr_enable = on >> class Class 0780, addr 00:04.0, pci id 1af4:1003 (sub 1af4:0003) >> bar 0: i/o at 0xc040 [0xc05f] >> bar 1: mem at 0xfebf1000 [0xfebf1fff] >> bus: virtio-serial0.0 >> type virtio-pci-bus >> dev: virtio-serial-device, id "" >> max_ports = 31 >> bus: virtio-serial-bus.0 >> type virtio-serial-bus >> >> The autogenerated bus name "deviceid.n" (virtio-serial0.0) became a >> virtio-bus... >> >> virtio-serial-bus.0 is the right bus to connect virtserialport. >> >> Any idea how to fix that? > The only idea I can come up with right now is to overwrite the bus name > on realize/qdev-init of the containing (virtio-serial-pci) device. > > Whether we want that is another question... :) It would fix command line > backwards compatibility but would be inconsistent then; I guess the > former is more important here. > > Regards, > Andreas I'm not sure that only overwriting the bus name will fix the issue. virtio-serial-device's bus still won't have the right name? Here with the command line, we expect virtio-serial-pci,id=id0 to create a virtio-serial-bus called id0.0 is that right? Thanks, Fred > >> Sorry for that, >> Fred >> >>>> -----Original Message----- >>>> From: qemu-devel-bounces+libaiqing=huawei.com@nongnu.org >>>> [mailto:qemu-devel-bounces+libaiqing=huawei.com@nongnu.org] On Behalf >>>> Of Igor Mammedov >>>> Sent: Thursday, April 18, 2013 5:02 PM >>>> To: Igor Mammedov >>>> Cc: ehabkost@redhat.com; qemu-devel@nongnu.org; armbru@redhat.com; >>>> anthony@codemonkey.ws; pbonzini@redhat.com; lcapitulino@redhat.com; >>>> Andreas Färber >>>> Subject: Re: [Qemu-devel] [PATCH] qdev: Fix device_add bus assumptions >>>> >>>> On Thu, 18 Apr 2013 10:41:56 +0200 >>>> Igor Mammedov <imammedo@redhat.com> wrote: >>>> >>>> [...] >>>>>> - if (!bus) { >>>>>> - bus = sysbus_get_default(); >>>>>> - } >>>>>> - >>>>> I've checked all direct childs of TYPE_DEVICE and they all set >>>>> k->bus_type, >>>>> with only one exception of TYPE_CPU. So it should be safe to remove >>>>> fallback >>>>> from qdev_device_add POV. >>>>> However TYPE_CPU breaks assumption that device always has >>>>> parent_bus set >>>>> to not NULL in qdev_unplug() and qdev_print() >>>> Err, qdev_print() is safe since it's called on bus children only, so >>>> it has >>>> parent_bus. >>>> >>>>> It would be better to add something like this: >>>>> // untested >>>>> >>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>>>> index 4eb0134..45009ba 100644 >>>>> --- a/hw/core/qdev.c >>>>> +++ b/hw/core/qdev.c >>>>> @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) >>>>> { >>>>> DeviceClass *dc = DEVICE_GET_CLASS(dev); >>>>> - if (!dev->parent_bus->allow_hotplug) { >>>>> + if (dev->parent_bus && !dev->parent_bus->allow_hotplug) { >>>>> error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); >>>>> return; >>>>> } >>>>> diff --git a/qdev-monitor.c b/qdev-monitor.c >>>>> index 9a78ccf..2476e4e 100644 >>>>> --- a/qdev-monitor.c >>>>> +++ b/qdev-monitor.c >>>>> @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState >>>>> *dev, >>>>> int indent) qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, >>>>> indent); >>>>> class = object_class_get_parent(class); >>>>> } while (class != object_class_by_name(TYPE_DEVICE)); >>>>> - bus_print_dev(dev->parent_bus, mon, dev, indent); >>>>> + if (dev->parent_bus) { >>>>> + bus_print_dev(dev->parent_bus, mon, dev, indent); >>>>> + } >>>>> QLIST_FOREACH(child, &dev->child_bus, sibling) { >>>>> qbus_print(mon, child, indent); >>>>> } >>>>> >>>>>> /* create device, set properties */ >>>>>> qdev = DEVICE(object_new(driver)); >>>>>> - qdev_set_parent_bus(qdev, bus); >>>>>> + >>>>>> + if (bus) { >>>>>> + qdev_set_parent_bus(qdev, bus); >>>>>> + } >>>>>> id = qemu_opts_id(opts); >>>>>> if (id) { >
Am 22.04.2013 16:15, schrieb KONRAD Frédéric: > On 22/04/2013 16:02, Andreas Färber wrote: >> Hi Fred, >> >> Am 22.04.2013 15:54, schrieb KONRAD Frédéric: >>> On 22/04/2013 15:27, Andreas Färber wrote: >>>> Am 22.04.2013 13:51, schrieb Libaiqing: >>>>> When I use the config below,an error occurs.Is there anything >>>>> wrong? >>>>> >>>>> Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c >>>>> -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 >>>>> -chardev spicevmc,id=charchannel0,name=vdagent -device >>>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 >>>>> >>>>> -drive file=/home/img/win7.qed,if=virtio,index=0,format=qed -monitor >>>>> stdio -vga qxl -vnc :1 >>>>> >>>>> Error output: >>>>> -device >>>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: >>>>> >>>>> Bus 'virtio-serial0.0' is full >>>>> -device >>>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: >>>>> >>>>> Bus 'virtio-serial0.0' not found >>>>> >>>>> Any feedback are appliciated. >>>> This does not sound related to this patch at all... >>>> >>>> Instead it sounds as if the virtio refactorings had some effect not >>>> only >>>> on virtio-net but also on virtio-serial. Fred? >>> Yes, sounds like the same issue as virtio-net: >>> >>> bus: pci.0 >>> type PCI >>> dev: virtio-serial-pci, id "virtio-serial0" >>> ioeventfd = off >>> vectors = 2 >>> class = 0x780 >>> indirect_desc = on >>> event_idx = on >>> max_ports = 31 >>> addr = 04.0 >>> romfile = <null> >>> rombar = 1 >>> multifunction = off >>> command_serr_enable = on >>> class Class 0780, addr 00:04.0, pci id 1af4:1003 (sub >>> 1af4:0003) >>> bar 0: i/o at 0xc040 [0xc05f] >>> bar 1: mem at 0xfebf1000 [0xfebf1fff] >>> bus: virtio-serial0.0 >>> type virtio-pci-bus >>> dev: virtio-serial-device, id "" >>> max_ports = 31 >>> bus: virtio-serial-bus.0 >>> type virtio-serial-bus >>> >>> The autogenerated bus name "deviceid.n" (virtio-serial0.0) became a >>> virtio-bus... >>> >>> virtio-serial-bus.0 is the right bus to connect virtserialport. >>> >>> Any idea how to fix that? >> The only idea I can come up with right now is to overwrite the bus name >> on realize/qdev-init of the containing (virtio-serial-pci) device. >> >> Whether we want that is another question... :) It would fix command line >> backwards compatibility but would be inconsistent then; I guess the >> former is more important here. >> >> Regards, >> Andreas > I'm not sure that only overwriting the bus name will fix the issue. > > virtio-serial-device's bus still won't have the right name? I was talking of virtio-serial-pci overwriting virtio-serial-device's bus with its own id.0 after it's been created by virtio-serial-device with NULL argument! Assuming it gets created in instance_init and can't just access its parent's id in its own realize function to have it correct from the start. Andreas > Here with the command line, we expect virtio-serial-pci,id=id0 to create > a virtio-serial-bus called id0.0 is that right? > > Thanks, > Fred
On 22/04/2013 16:22, Andreas Färber wrote: > Am 22.04.2013 16:15, schrieb KONRAD Frédéric: >> On 22/04/2013 16:02, Andreas Färber wrote: >>> Hi Fred, >>> >>> Am 22.04.2013 15:54, schrieb KONRAD Frédéric: >>>> On 22/04/2013 15:27, Andreas Färber wrote: >>>>> Am 22.04.2013 13:51, schrieb Libaiqing: >>>>>> When I use the config below,an error occurs.Is there anything >>>>>> wrong? >>>>>> >>>>>> Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c >>>>>> -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 >>>>>> -chardev spicevmc,id=charchannel0,name=vdagent -device >>>>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 >>>>>> >>>>>> -drive file=/home/img/win7.qed,if=virtio,index=0,format=qed -monitor >>>>>> stdio -vga qxl -vnc :1 >>>>>> >>>>>> Error output: >>>>>> -device >>>>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: >>>>>> >>>>>> Bus 'virtio-serial0.0' is full >>>>>> -device >>>>>> virtserialport,bus=virtio-serial0.0,chardev=charchannel0,id=channel0,name=com.redhat.spice.0: >>>>>> >>>>>> Bus 'virtio-serial0.0' not found >>>>>> >>>>>> Any feedback are appliciated. >>>>> This does not sound related to this patch at all... >>>>> >>>>> Instead it sounds as if the virtio refactorings had some effect not >>>>> only >>>>> on virtio-net but also on virtio-serial. Fred? >>>> Yes, sounds like the same issue as virtio-net: >>>> >>>> bus: pci.0 >>>> type PCI >>>> dev: virtio-serial-pci, id "virtio-serial0" >>>> ioeventfd = off >>>> vectors = 2 >>>> class = 0x780 >>>> indirect_desc = on >>>> event_idx = on >>>> max_ports = 31 >>>> addr = 04.0 >>>> romfile = <null> >>>> rombar = 1 >>>> multifunction = off >>>> command_serr_enable = on >>>> class Class 0780, addr 00:04.0, pci id 1af4:1003 (sub >>>> 1af4:0003) >>>> bar 0: i/o at 0xc040 [0xc05f] >>>> bar 1: mem at 0xfebf1000 [0xfebf1fff] >>>> bus: virtio-serial0.0 >>>> type virtio-pci-bus >>>> dev: virtio-serial-device, id "" >>>> max_ports = 31 >>>> bus: virtio-serial-bus.0 >>>> type virtio-serial-bus >>>> >>>> The autogenerated bus name "deviceid.n" (virtio-serial0.0) became a >>>> virtio-bus... >>>> >>>> virtio-serial-bus.0 is the right bus to connect virtserialport. >>>> >>>> Any idea how to fix that? >>> The only idea I can come up with right now is to overwrite the bus name >>> on realize/qdev-init of the containing (virtio-serial-pci) device. >>> >>> Whether we want that is another question... :) It would fix command line >>> backwards compatibility but would be inconsistent then; I guess the >>> former is more important here. >>> >>> Regards, >>> Andreas >> I'm not sure that only overwriting the bus name will fix the issue. >> >> virtio-serial-device's bus still won't have the right name? > I was talking of virtio-serial-pci overwriting virtio-serial-device's > bus with its own id.0 after it's been created by virtio-serial-device > with NULL argument! Assuming it gets created in instance_init and can't > just access its parent's id in its own realize function to have it > correct from the start. > > Andreas Ok, I'll take a look. Thanks for help :), Fred > >> Here with the command line, we expect virtio-serial-pci,id=id0 to create >> a virtio-serial-bus called id0.0 is that right? >> >> Thanks, >> Fred
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 4eb0134..45009ba 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) { DeviceClass *dc = DEVICE_GET_CLASS(dev); - if (!dev->parent_bus->allow_hotplug) { + if (dev->parent_bus && !dev->parent_bus->allow_hotplug) { error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name); return; } diff --git a/qdev-monitor.c b/qdev-monitor.c index 9a78ccf..2476e4e 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent) qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent); class = object_class_get_parent(class); } while (class != object_class_by_name(TYPE_DEVICE)); - bus_print_dev(dev->parent_bus, mon, dev, indent); + if (dev->parent_bus) { + bus_print_dev(dev->parent_bus, mon, dev, indent); + } QLIST_FOREACH(child, &dev->child_bus, sibling) { qbus_print(mon, child, indent); }