Re: [Qemu-devel] [PATCH v2 36/36] qdev: HotplugHandler: add support for unplugging BUS-less devices

[Andreas Färber]

Am 01.10.2014 um 14:43 schrieb Igor Mammedov:
> On Wed, 1 Oct 2014 16:22:23 +0530
> Bharata B Rao <address@hidden> wrote:
> 
>> On Wed, Oct 01, 2014 at 11:57:37AM +0200, Igor Mammedov wrote:
>>> On Wed, 1 Oct 2014 14:27:19 +0530
>>> Bharata B Rao <address@hidden> wrote:
>>>
>>>> On Fri, Sep 26, 2014 at 09:28:41AM +0000, Igor Mammedov wrote:
>>>>> Signed-off-by: Igor Mammedov <address@hidden>
>>>>> ---
>>>>> v2:
>>>>>   merged in Paolo's suggestion to reduce indentation in patch
>>>>> ---
>>>>>  hw/core/qdev.c | 61
>>>>> ++++++++++++++++++++++++++++++++-------------------------- 1
>>>>> file changed, 34 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>> index bc45a59..215effb 100644
>>>>> --- a/hw/core/qdev.c
>>>>> +++ b/hw/core/qdev.c
>>>>> @@ -223,9 +223,28 @@ void
>>>>> qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>>>> dev->alias_required_for_version = required_for_version; }
>>>>>
>>>>> +static HotplugHandler *qdev_get_hotplug_handler(DeviceState
>>>>> *dev) +{
>>>>> +    HotplugHandler *hotplug_ctrl = NULL;
>>>>> +
>>>>> +    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
>>>>> +        hotplug_ctrl = dev->parent_bus->hotplug_handler;
>>>>> +    } else if (object_dynamic_cast(qdev_get_machine(),
>>>>> TYPE_MACHINE)) {
>>>>> +        MachineState *machine = MACHINE(qdev_get_machine());
>>>>> +        MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>>> +
>>>>> +        if (mc->get_hotplug_handler) {
>>>>> +            hotplug_ctrl = mc->get_hotplug_handler(machine,
>>>>> dev);
>>>>> +        }
>>>>> +    }
>>>>> +    return hotplug_ctrl;
>>>>> +}
>>>>> +
>>>>>  void qdev_unplug(DeviceState *dev, Error **errp)
>>>>>  {
>>>>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>>>> +    HotplugHandler *hotplug_ctrl;
>>>>> +    HotplugHandlerClass *hdc;
>>>>>
>>>>>      if (dev->parent_bus
>>>>> && !qbus_is_hotpluggable(dev->parent_bus)) { error_set(errp,
>>>>> QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>>>>
>>>> Unlike x86 CPU that sits on ICC bus, PowerPC CPU doesn't sit on
>>>> any bus. I was under the impression that this patch helps in
>>>> unplugging such devices. However I do see some problems.
>>> Indeed, this patch goes only half way for supporting unplug of
>>> bussless devices (i.e. it only fetches unplug handler without
>>> addressing device lookup issue)
>>>
>>>>
>>>> While trying to unplug a PowerPC CPU that has earlier been added
>>>> using device_add, qdev-monitor.c:qmp_device_del() fails to
>>>> find/locate the device since the device isn't on any bus and
>>>> hence qdev_unplug() won't be called for such a device. I thought
>>>> I could make "main_system_bus" as the parent bus of this CPU
>>>> device but that wouldn't help during unplugging. With that
>>>> qmp_device_del() can find the device, but qdev_uplug() fails at
>>>> the above if condition since "main_system_bus" isn't hotpluggable.
>>>>
>>>> So how should we deal with such devices which don't have any
>>>> parent bus ?
>>> There is no need to invent non existing BUS. We should fix lookup
>>> issue instead of it. Doing something along this lines:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04574.html
>>>
>>> Above of cause assumes that device is placed in 'peripherals' tree.
>>> Does above patch fixes issue?
>>
>> Yes it does fix the issue. Thanks.
> Thanks for confirming, I'll try to rewrite above path to a more
> acceptable form and post it as follow up to this series, hopefully
> tomorrow.
> 
> Andreas,
>  is it acceptable or should I respin fixedup 36/36 instead?

As I started looking into this series I'd appreciate not respinning the
full series if not necessary. :) Whether you do a 37/36 v2 or a 36/36 v3
is up to you guys.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg