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

[Igor Mammedov]

On Wed, 01 Oct 2014 14:46:03 +0200
Andreas Färber <address@hidden> wrote:

> 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.
I'll add 37/36 to fix lookup issue.


> 
> Thanks,
> Andreas
> 

Thanks,
Igor