Re: [Qemu-devel] [PATCH] qdev: Fix device_add bus assumptions

["KONRAD Frédéric" <fred.konrad@greensocs.com>]

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) {
>