[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH v4 18/24] qdev: hotplug: provide do_unplug handler

From: David Hildenbrand
Subject: Re: [Qemu-ppc] [PATCH v4 18/24] qdev: hotplug: provide do_unplug handler
Date: Fri, 5 Oct 2018 09:40:50 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

On 04/10/2018 17:59, Igor Mammedov wrote:
> On Wed, 3 Oct 2018 19:21:25 +0200
> David Hildenbrand <address@hidden> wrote:
>> On 03/10/2018 08:29, David Gibson wrote:
>>> On Wed, Sep 26, 2018 at 11:42:13AM +0200, David Hildenbrand wrote:  
>>>> The unplug and unplug_request handlers are special: They are not
>>>> executed when unrealizing a device, but rather trigger the removal of a
>>>> device from device_del() via object_unparent() - to effectively
>>>> unrealize a device.
>>>> If such a device has a child bus and another device attached to
>>>> that bus (e.g. how virtio devices are created with their proxy device),
>>>> we will not get a call to the unplug handler. As we want to support
>>>> hotplug handlers (and especially also some unplug logic to undo resource
>>>> assignment) for such devices, we cannot simply call the unplug handler
>>>> when unrealizing - it has a different semantic ("trigger removal").
>>>> To handle this scenario, we need a do_unplug handler, that will be
>>>> executed for all devices with a hotplug handler.
>>>> While at it, introduce hotplug_fn_nofail and fix a spelling mistake in
>>>> a comment.
>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>> ---
>>>>  hw/core/hotplug.c    | 10 ++++++++++
>>>>  hw/core/qdev.c       |  6 ++++++
>>>>  include/hw/hotplug.h | 26 ++++++++++++++++++++++++--
>>>>  3 files changed, 40 insertions(+), 2 deletions(-)
>>>> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
>>>> index 2253072d0e..e7a68d5160 100644
>>>> --- a/hw/core/hotplug.c
>>>> +++ b/hw/core/hotplug.c
>>>> @@ -45,6 +45,16 @@ void hotplug_handler_post_plug(HotplugHandler 
>>>> *plug_handler,
>>>>      }
>>>>  }
>>>> +void hotplug_handler_do_unplug(HotplugHandler *plug_handler,
>>>> +                                 DeviceState *plugged_dev)  
>>> Hrm.  I really dislike things named "do_X".  The "do" rarely adds any
>>> useful meaning.  And when there's also something called just plain
>>> "X", it's *always* unclear how they relate to each other.
>>> That's doubly true when it's a general interface like this, rather
>>> than just some local functions.
>> Yes, the naming is not the best, but I didn't want to rename all unplug
>> handlers before we have an agreement on how to proceed. My concept would
>> be that
>> 1. unplug() is renamed to trigger_unplug(). unplug_request() remains.
>> 2. do_unplug() is renamed to pre_unplug() (just like pre_plug())
>> 3. we might have in addition unplug() after realize(false) - just like
>> plug()
>> trigger_unplug() would perform checks and result in object_unparent().
>> From there, all cleanup/unplugging would be performed via the unrealize
>> chain, just like we have for realize() now. That would allow to properly
>> unplug complete device hierarchies.
>> But Igor rather wants one hotplug handler chain, and no dedicated
>> hotplug handlers for other devices in the device hierarchy.
> Because what we are dealing here with (virtio-pmem) is not separate
> devices hierarchy, it's one complex device and if we are to avoid
> layering violation, we should operate internal devices via that outer
> device which would orchestrate given to it resources internally as it
> sees it fit.
> It's similar with be spapr cpu core, where internal threads do
> not have their own handlers they are plugged as the integral part
> of the core.
> What I'm strongly opposed is using separate hotplug handlers for
> internal devices of a composite one.
> I'm more lenient in cases of where the hotplug handler of a composite
> device access it's internals directly if creating interfaces to
> manage internal devices is big over-engineering, since all
> hotplug flow is explicitly described within one handler and
> I don't need to hunt around to figure out how device is wired up.
> It's still not right wrt not violating abstraction layers and
> might break if internal details change, but usually hotplug
> handler is target unique and tightly coupled code of manged
> and managing pieces (like the case of spapr cpu core) so it
> works so far. For some generic handler I'd vote for following
> all the rules.
> In this series approach, handlers are used if they are separate
> devices without explicit connection which I find totally broken
> (and tried to explain in this thread, probably not well enough).

Thanks for the extended explanation.

Let's see if I can make it work. I guess virtio devices are really
special (and turning other devices without proxys into memory devices
would be much easier).



David / dhildenb

reply via email to

[Prev in Thread] Current Thread [Next in Thread]