| Message ID | 20180517081527.14410-5-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | MemoryDevice: use multi stage hotplug handlers | expand |
On Thu, 17 May 2018 10:15:17 +0200 David Hildenbrand <david@redhat.com> wrote: > For multi stage hotplug handlers, we'll have to do some error handling > in some hotplug functions, so let's use a local error variable (except > for unplug requests). I'd split out introducing local error into separate patch so patch would do a single thing. > Also, add code to pass control to the final stage hotplug handler at the > parent bus. But I don't agree with generic "} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {" forwarding, it's done by 3/14 for generic case and in case of special device that needs bus handler called from machine one, I'd suggest to do forwarding explicitly for that device only like we do with acpi_dev. > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 8 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index d768930d02..510076e156 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > - pc_cpu_pre_plug(hotplug_dev, dev, errp); > + pc_cpu_pre_plug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, > + &local_err); > } > + error_propagate(errp, local_err); > } > > static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > - pc_dimm_plug(hotplug_dev, dev, errp); > + pc_dimm_plug(hotplug_dev, dev, &local_err); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > - pc_cpu_plug(hotplug_dev, dev, errp); > + pc_cpu_plug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > } > + error_propagate(errp, local_err); > } > > static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, > @@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, > pc_dimm_unplug_request(hotplug_dev, dev, errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > pc_cpu_unplug_request_cb(hotplug_dev, dev, errp); > - } else { > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev, > + errp); > + } else if (!dev->parent_bus) { > error_setg(errp, "acpi: device unplug request for not supported device" > " type: %s", object_get_typename(OBJECT(dev))); > } > @@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, > static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > - pc_dimm_unplug(hotplug_dev, dev, errp); > + pc_dimm_unplug(hotplug_dev, dev, &local_err); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > - pc_cpu_unplug_cb(hotplug_dev, dev, errp); > - } else { > - error_setg(errp, "acpi: device unplug for not supported device" > + pc_cpu_unplug_cb(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, > + &local_err); > + } else if (!dev->parent_bus) { > + error_setg(&local_err, "acpi: device unplug for not supported device" > " type: %s", object_get_typename(OBJECT(dev))); > } > + error_propagate(errp, local_err); > } > > static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
On 30.05.2018 15:08, Igor Mammedov wrote: > On Thu, 17 May 2018 10:15:17 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> For multi stage hotplug handlers, we'll have to do some error handling >> in some hotplug functions, so let's use a local error variable (except >> for unplug requests). > I'd split out introducing local error into separate patch > so patch would do a single thing. > >> Also, add code to pass control to the final stage hotplug handler at the >> parent bus. > But I don't agree with generic > "} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {" > forwarding, it's done by 3/14 for generic case and in case of > special device that needs bus handler called from machine one, > I'd suggest to do forwarding explicitly for that device only > like we do with acpi_dev. I decided to do it that way because it is generic and results in nicer recovery handling (e.g. in case pc_dimm plug fails, we can simply rollback all (for now MemoryDevice) previous plug operations). IMHO, the resulting code is easier to read. From this handling it is clear that "if we reach the hotplug handler, and it is not some special device plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug handler if any exists" > > >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++-------- >> 1 file changed, 31 insertions(+), 8 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index d768930d02..510076e156 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, >> static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp) >> { >> + Error *local_err = NULL; >> + >> + /* final stage hotplug handler */ >> if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { >> - pc_cpu_pre_plug(hotplug_dev, dev, errp); >> + pc_cpu_pre_plug(hotplug_dev, dev, &local_err); >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { >> + hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, >> + &local_err); >> } >> + error_propagate(errp, local_err); >> } >> >> static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp) >> { >> + Error *local_err = NULL; >> + >> + /* final stage hotplug handler */ >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { >> - pc_dimm_plug(hotplug_dev, dev, errp); >> + pc_dimm_plug(hotplug_dev, dev, &local_err); >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { >> - pc_cpu_plug(hotplug_dev, dev, errp); >> + pc_cpu_plug(hotplug_dev, dev, &local_err); >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { >> + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); >> } >> + error_propagate(errp, local_err); >> } >> >> static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, >> @@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, >> pc_dimm_unplug_request(hotplug_dev, dev, errp); >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { >> pc_cpu_unplug_request_cb(hotplug_dev, dev, errp); >> - } else { >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { >> + hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev, >> + errp); >> + } else if (!dev->parent_bus) { >> error_setg(errp, "acpi: device unplug request for not supported device" >> " type: %s", object_get_typename(OBJECT(dev))); >> } >> @@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, >> static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp) >> { >> + Error *local_err = NULL; >> + >> + /* final stage hotplug handler */ >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { >> - pc_dimm_unplug(hotplug_dev, dev, errp); >> + pc_dimm_unplug(hotplug_dev, dev, &local_err); >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { >> - pc_cpu_unplug_cb(hotplug_dev, dev, errp); >> - } else { >> - error_setg(errp, "acpi: device unplug for not supported device" >> + pc_cpu_unplug_cb(hotplug_dev, dev, &local_err); >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { >> + hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, >> + &local_err); >> + } else if (!dev->parent_bus) { >> + error_setg(&local_err, "acpi: device unplug for not supported device" >> " type: %s", object_get_typename(OBJECT(dev))); >> } >> + error_propagate(errp, local_err); >> } >> >> static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, >
On Wed, 30 May 2018 16:13:32 +0200 David Hildenbrand <david@redhat.com> wrote: > On 30.05.2018 15:08, Igor Mammedov wrote: > > On Thu, 17 May 2018 10:15:17 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> For multi stage hotplug handlers, we'll have to do some error handling > >> in some hotplug functions, so let's use a local error variable (except > >> for unplug requests). > > I'd split out introducing local error into separate patch > > so patch would do a single thing. > > > >> Also, add code to pass control to the final stage hotplug handler at the > >> parent bus. > > But I don't agree with generic > > "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {" > > forwarding, it's done by 3/14 for generic case and in case of > > special device that needs bus handler called from machine one, > > I'd suggest to do forwarding explicitly for that device only > > like we do with acpi_dev. > > I decided to do it that way because it is generic and results in nicer > recovery handling (e.g. in case pc_dimm plug fails, we can simply > rollback all (for now MemoryDevice) previous plug operations). rollback should be managed by the caller of pc_dimm plug directly, so it's not relevant here. > IMHO, the resulting code is easier to read. > > From this handling it is clear that > "if we reach the hotplug handler, and it is not some special device > plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug > handler if any exists" I strongly disagree with that it's easier to deal with. You are basically duplicating already generalized code from qdev_get_hotplug_handler() back into boards. If a device doesn't have to be handled by machine handler, than qdev_get_hotplug_handler() must return its bus handler if any directly. So branch in question that your are copying is a dead one, pls drop it. > > > > > > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++-------- > >> 1 file changed, 31 insertions(+), 8 deletions(-) > >> > >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >> index d768930d02..510076e156 100644 > >> --- a/hw/i386/pc.c > >> +++ b/hw/i386/pc.c > >> @@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, > >> static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > >> DeviceState *dev, Error **errp) > >> { > >> + Error *local_err = NULL; > >> + > >> + /* final stage hotplug handler */ > >> if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > >> - pc_cpu_pre_plug(hotplug_dev, dev, errp); > >> + pc_cpu_pre_plug(hotplug_dev, dev, &local_err); > >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > >> + hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, > >> + &local_err); > >> } > >> + error_propagate(errp, local_err); > >> } > >> > >> static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev, > >> DeviceState *dev, Error **errp) > >> { > >> + Error *local_err = NULL; > >> + > >> + /* final stage hotplug handler */ > >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > >> - pc_dimm_plug(hotplug_dev, dev, errp); > >> + pc_dimm_plug(hotplug_dev, dev, &local_err); > >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > >> - pc_cpu_plug(hotplug_dev, dev, errp); > >> + pc_cpu_plug(hotplug_dev, dev, &local_err); > >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > >> + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > >> } > >> + error_propagate(errp, local_err); > >> } > >> > >> static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, > >> @@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, > >> pc_dimm_unplug_request(hotplug_dev, dev, errp); > >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > >> pc_cpu_unplug_request_cb(hotplug_dev, dev, errp); > >> - } else { > >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > >> + hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev, > >> + errp); > >> + } else if (!dev->parent_bus) { > >> error_setg(errp, "acpi: device unplug request for not supported device" > >> " type: %s", object_get_typename(OBJECT(dev))); > >> } > >> @@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, > >> static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev, > >> DeviceState *dev, Error **errp) > >> { > >> + Error *local_err = NULL; > >> + > >> + /* final stage hotplug handler */ > >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > >> - pc_dimm_unplug(hotplug_dev, dev, errp); > >> + pc_dimm_unplug(hotplug_dev, dev, &local_err); > >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > >> - pc_cpu_unplug_cb(hotplug_dev, dev, errp); > >> - } else { > >> - error_setg(errp, "acpi: device unplug for not supported device" > >> + pc_cpu_unplug_cb(hotplug_dev, dev, &local_err); > >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > >> + hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, > >> + &local_err); > >> + } else if (!dev->parent_bus) { > >> + error_setg(&local_err, "acpi: device unplug for not supported device" > >> " type: %s", object_get_typename(OBJECT(dev))); > >> } > >> + error_propagate(errp, local_err); > >> } > >> > >> static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, > > > >
On 31.05.2018 16:13, Igor Mammedov wrote: > On Wed, 30 May 2018 16:13:32 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 30.05.2018 15:08, Igor Mammedov wrote: >>> On Thu, 17 May 2018 10:15:17 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> For multi stage hotplug handlers, we'll have to do some error handling >>>> in some hotplug functions, so let's use a local error variable (except >>>> for unplug requests). >>> I'd split out introducing local error into separate patch >>> so patch would do a single thing. I can do that if it makes review easier. >>> >>>> Also, add code to pass control to the final stage hotplug handler at the >>>> parent bus. >>> But I don't agree with generic >>> "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {" >>> forwarding, it's done by 3/14 for generic case and in case of >>> special device that needs bus handler called from machine one, >>> I'd suggest to do forwarding explicitly for that device only >>> like we do with acpi_dev. >> >> I decided to do it that way because it is generic and results in nicer >> recovery handling (e.g. in case pc_dimm plug fails, we can simply >> rollback all (for now MemoryDevice) previous plug operations). > rollback should be managed by the caller of pc_dimm plug > directly, so it's not relevant here. > >> IMHO, the resulting code is easier to read. >> >> From this handling it is clear that >> "if we reach the hotplug handler, and it is not some special device >> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug >> handler if any exists" > I strongly disagree with that it's easier to deal with. > You are basically duplicating already generalized code > from qdev_get_hotplug_handler() back into boards. > > If a device doesn't have to be handled by machine handler, > than qdev_get_hotplug_handler() must return its bus handler > if any directly. So branch in question that your are copying > is a dead one, pls drop it. We forward selected (pc_get_hotpug_handler()) devices to the right hotplug handler. Nothing wrong about that. I don't agree with "basically duplicating already generalized code" wrong. We have to forward at some place. Your idea simply places that code at some other place. But I think we have to get the general idea sorted out first. What you have in mind (e.g. plug): if (TYPE_MEMORY_DEVICE) { memory_device_plug(); if (local_err) { goto out; } if (TYPE_PC_DIMM) { pc_dimm_plug(hotplug_dev, dev, &local_err); } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); } if (local_err) { memory_device_unplug() goto out; } } else if (TYPE_CPU) ... What I have in mind (and implemented in this series): if (TYPE_MEMORY_DEVICE) { memory_device_plug(); } /* possibly other interfaces */ if (local_err) { error_handling(); return; } if (TYPE_PC_DIMM) { pc_dimm_plug(hotplug_dev, dev, &local_err); } ... } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); } if (local_err) { if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { memory_device_unplug() } /* possibly other interfaces */ } ... I claim that my variant is more generic because: - it easily supports multiple interfaces (like MemoryDevice) per Device that need a hotplug handler call - there is only one call to hotplug_handler_plug() in case we add similar handling for another device Apart from that they do exactly the same thing.
On Mon, 4 Jun 2018 13:27:01 +0200 David Hildenbrand <david@redhat.com> wrote: > On 31.05.2018 16:13, Igor Mammedov wrote: > > On Wed, 30 May 2018 16:13:32 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 30.05.2018 15:08, Igor Mammedov wrote: > >>> On Thu, 17 May 2018 10:15:17 +0200 > >>> David Hildenbrand <david@redhat.com> wrote: > >>> > >>>> For multi stage hotplug handlers, we'll have to do some error handling > >>>> in some hotplug functions, so let's use a local error variable (except > >>>> for unplug requests). > >>> I'd split out introducing local error into separate patch > >>> so patch would do a single thing. > > I can do that if it makes review easier. > > >>> > >>>> Also, add code to pass control to the final stage hotplug handler at the > >>>> parent bus. > >>> But I don't agree with generic > >>> "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {" > >>> forwarding, it's done by 3/14 for generic case and in case of > >>> special device that needs bus handler called from machine one, > >>> I'd suggest to do forwarding explicitly for that device only > >>> like we do with acpi_dev. > >> > >> I decided to do it that way because it is generic and results in nicer > >> recovery handling (e.g. in case pc_dimm plug fails, we can simply > >> rollback all (for now MemoryDevice) previous plug operations). > > rollback should be managed by the caller of pc_dimm plug > > directly, so it's not relevant here. > > > >> IMHO, the resulting code is easier to read. > >> > >> From this handling it is clear that > >> "if we reach the hotplug handler, and it is not some special device > >> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug > >> handler if any exists" > > I strongly disagree with that it's easier to deal with. > > You are basically duplicating already generalized code > > from qdev_get_hotplug_handler() back into boards. > > > > If a device doesn't have to be handled by machine handler, > > than qdev_get_hotplug_handler() must return its bus handler > > if any directly. So branch in question that your are copying > > is a dead one, pls drop it. > > We forward selected (pc_get_hotpug_handler()) devices to the > right hotplug handler. Nothing wrong about that. I don't agree > with "basically duplicating already generalized code" wrong. > We have to forward at some place. Your idea simply places that > code at some other place. > > > But I think we have to get the general idea sorted out first. > > What you have in mind (e.g. plug): > > if (TYPE_MEMORY_DEVICE) { > memory_device_plug(); > if (local_err) { > goto out; > } > > if (TYPE_PC_DIMM) { > pc_dimm_plug(hotplug_dev, dev, &local_err); > } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > } > if (local_err) { > memory_device_unplug() > goto out; > } > } else if (TYPE_CPU) > ... > > > What I have in mind (and implemented in this series): > > > if (TYPE_MEMORY_DEVICE) { > memory_device_plug(); > } > /* possibly other interfaces */ > if (local_err) { > error_handling(); > return; > } > > if (TYPE_PC_DIMM) { > pc_dimm_plug(hotplug_dev, dev, &local_err); > } ... > } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > } I don't like this implicit wiring, where reader of this part of code has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs request forwarding. I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code, something like this: if (TYPE_PC_DIMM) { pc_dimm_plug() /* do here additional concrete machine specific things */ } else if (TYPE_VIRTIO_MEM) { virtio_mem_plug() <- do forwarding in there /* and do here additional concrete machine specific things */ } else if (TYPE_CPU) { cpu_plug() /* do here additional concrete machine specific things */ } > if (local_err) { > if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { > memory_device_unplug() > } > /* possibly other interfaces */ > } > ... > > > I claim that my variant is more generic because: > - it easily supports multiple interfaces (like MemoryDevice) > per Device that need a hotplug handler call > - there is only one call to hotplug_handler_plug() in case we > add similar handling for another device As someone said "one more layer of indirection would solve problem". But then one would have a clue how it works after a while (including author of the feature). I don't think we have a problem here and need generalization. > > Apart from that they do exactly the same thing. >
On 07.06.2018 15:44, Igor Mammedov wrote: > On Mon, 4 Jun 2018 13:27:01 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 31.05.2018 16:13, Igor Mammedov wrote: >>> On Wed, 30 May 2018 16:13:32 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> On 30.05.2018 15:08, Igor Mammedov wrote: >>>>> On Thu, 17 May 2018 10:15:17 +0200 >>>>> David Hildenbrand <david@redhat.com> wrote: >>>>> >>>>>> For multi stage hotplug handlers, we'll have to do some error handling >>>>>> in some hotplug functions, so let's use a local error variable (except >>>>>> for unplug requests). >>>>> I'd split out introducing local error into separate patch >>>>> so patch would do a single thing. >> >> I can do that if it makes review easier. >> >>>>> >>>>>> Also, add code to pass control to the final stage hotplug handler at the >>>>>> parent bus. >>>>> But I don't agree with generic >>>>> "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {" >>>>> forwarding, it's done by 3/14 for generic case and in case of >>>>> special device that needs bus handler called from machine one, >>>>> I'd suggest to do forwarding explicitly for that device only >>>>> like we do with acpi_dev. >>>> >>>> I decided to do it that way because it is generic and results in nicer >>>> recovery handling (e.g. in case pc_dimm plug fails, we can simply >>>> rollback all (for now MemoryDevice) previous plug operations). >>> rollback should be managed by the caller of pc_dimm plug >>> directly, so it's not relevant here. >>> >>>> IMHO, the resulting code is easier to read. >>>> >>>> From this handling it is clear that >>>> "if we reach the hotplug handler, and it is not some special device >>>> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug >>>> handler if any exists" >>> I strongly disagree with that it's easier to deal with. >>> You are basically duplicating already generalized code >>> from qdev_get_hotplug_handler() back into boards. >>> >>> If a device doesn't have to be handled by machine handler, >>> than qdev_get_hotplug_handler() must return its bus handler >>> if any directly. So branch in question that your are copying >>> is a dead one, pls drop it. >> >> We forward selected (pc_get_hotpug_handler()) devices to the >> right hotplug handler. Nothing wrong about that. I don't agree >> with "basically duplicating already generalized code" wrong. >> We have to forward at some place. Your idea simply places that >> code at some other place. >> >> >> But I think we have to get the general idea sorted out first. >> >> What you have in mind (e.g. plug): >> >> if (TYPE_MEMORY_DEVICE) { >> memory_device_plug(); >> if (local_err) { >> goto out; >> } >> >> if (TYPE_PC_DIMM) { >> pc_dimm_plug(hotplug_dev, dev, &local_err); >> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { >> hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); >> } >> if (local_err) { >> memory_device_unplug() >> goto out; >> } >> } else if (TYPE_CPU) >> ... >> >> >> What I have in mind (and implemented in this series): >> >> >> if (TYPE_MEMORY_DEVICE) { >> memory_device_plug(); >> } >> /* possibly other interfaces */ >> if (local_err) { >> error_handling(); >> return; >> } >> >> if (TYPE_PC_DIMM) { >> pc_dimm_plug(hotplug_dev, dev, &local_err); >> } ... >> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { >> hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); >> } > I don't like this implicit wiring, where reader of this part of code > has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs > request forwarding. > I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code, > something like this: > > if (TYPE_PC_DIMM) { > pc_dimm_plug() > /* do here additional concrete machine specific things */ > } else if (TYPE_VIRTIO_MEM) { > virtio_mem_plug() <- do forwarding in there > /* and do here additional concrete machine specific things */ > } else if (TYPE_CPU) { > cpu_plug() > /* do here additional concrete machine specific things */ > } That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code. > >> if (local_err) { >> if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { >> memory_device_unplug() >> } >> /* possibly other interfaces */ >> } >> ... >> >> >> I claim that my variant is more generic because: >> - it easily supports multiple interfaces (like MemoryDevice) >> per Device that need a hotplug handler call >> - there is only one call to hotplug_handler_plug() in case we >> add similar handling for another device > As someone said "one more layer of indirection would solve problem". > But then one would have a clue how it works after a while (including > author of the feature). > I don't think we have a problem here and need generalization. > >> >> Apart from that they do exactly the same thing. >> >
On Thu, 7 Jun 2018 16:00:54 +0200 David Hildenbrand <david@redhat.com> wrote: > On 07.06.2018 15:44, Igor Mammedov wrote: > > On Mon, 4 Jun 2018 13:27:01 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 31.05.2018 16:13, Igor Mammedov wrote: > >>> On Wed, 30 May 2018 16:13:32 +0200 > >>> David Hildenbrand <david@redhat.com> wrote: > >>> > >>>> On 30.05.2018 15:08, Igor Mammedov wrote: > >>>>> On Thu, 17 May 2018 10:15:17 +0200 > >>>>> David Hildenbrand <david@redhat.com> wrote: > >>>>> > >>>>>> For multi stage hotplug handlers, we'll have to do some error handling > >>>>>> in some hotplug functions, so let's use a local error variable (except > >>>>>> for unplug requests). > >>>>> I'd split out introducing local error into separate patch > >>>>> so patch would do a single thing. > >> > >> I can do that if it makes review easier. > >> > >>>>> > >>>>>> Also, add code to pass control to the final stage hotplug handler at the > >>>>>> parent bus. > >>>>> But I don't agree with generic > >>>>> "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {" > >>>>> forwarding, it's done by 3/14 for generic case and in case of > >>>>> special device that needs bus handler called from machine one, > >>>>> I'd suggest to do forwarding explicitly for that device only > >>>>> like we do with acpi_dev. > >>>> > >>>> I decided to do it that way because it is generic and results in nicer > >>>> recovery handling (e.g. in case pc_dimm plug fails, we can simply > >>>> rollback all (for now MemoryDevice) previous plug operations). > >>> rollback should be managed by the caller of pc_dimm plug > >>> directly, so it's not relevant here. > >>> > >>>> IMHO, the resulting code is easier to read. > >>>> > >>>> From this handling it is clear that > >>>> "if we reach the hotplug handler, and it is not some special device > >>>> plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug > >>>> handler if any exists" > >>> I strongly disagree with that it's easier to deal with. > >>> You are basically duplicating already generalized code > >>> from qdev_get_hotplug_handler() back into boards. > >>> > >>> If a device doesn't have to be handled by machine handler, > >>> than qdev_get_hotplug_handler() must return its bus handler > >>> if any directly. So branch in question that your are copying > >>> is a dead one, pls drop it. > >> > >> We forward selected (pc_get_hotpug_handler()) devices to the > >> right hotplug handler. Nothing wrong about that. I don't agree > >> with "basically duplicating already generalized code" wrong. > >> We have to forward at some place. Your idea simply places that > >> code at some other place. > >> > >> > >> But I think we have to get the general idea sorted out first. > >> > >> What you have in mind (e.g. plug): > >> > >> if (TYPE_MEMORY_DEVICE) { > >> memory_device_plug(); > >> if (local_err) { > >> goto out; > >> } > >> > >> if (TYPE_PC_DIMM) { > >> pc_dimm_plug(hotplug_dev, dev, &local_err); > >> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > >> hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > >> } > >> if (local_err) { > >> memory_device_unplug() > >> goto out; > >> } > >> } else if (TYPE_CPU) > >> ... > >> > >> > >> What I have in mind (and implemented in this series): > >> > >> > >> if (TYPE_MEMORY_DEVICE) { > >> memory_device_plug(); > >> } > >> /* possibly other interfaces */ > >> if (local_err) { > >> error_handling(); > >> return; > >> } > >> > >> if (TYPE_PC_DIMM) { > >> pc_dimm_plug(hotplug_dev, dev, &local_err); > >> } ... > >> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > >> hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > >> } > > I don't like this implicit wiring, where reader of this part of code > > has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs > > request forwarding. > > I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code, > > something like this: > > > > if (TYPE_PC_DIMM) { > > pc_dimm_plug() > > /* do here additional concrete machine specific things */ > > } else if (TYPE_VIRTIO_MEM) { > > virtio_mem_plug() <- do forwarding in there > > /* and do here additional concrete machine specific things */ > > } else if (TYPE_CPU) { > > cpu_plug() > > /* do here additional concrete machine specific things */ > > } > > That will result in a lot of duplicate code - for every machine we > support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - > virtio-mem and virtio-pmem could most probably share the code. maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain. > > > > >> if (local_err) { > >> if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { > >> memory_device_unplug() > >> } > >> /* possibly other interfaces */ > >> } > >> ... > >> > >> > >> I claim that my variant is more generic because: > >> - it easily supports multiple interfaces (like MemoryDevice) > >> per Device that need a hotplug handler call > >> - there is only one call to hotplug_handler_plug() in case we > >> add similar handling for another device > > As someone said "one more layer of indirection would solve problem". > > But then one would have a clue how it works after a while (including > > author of the feature). > > I don't think we have a problem here and need generalization. > > > >> > >> Apart from that they do exactly the same thing. > >> > > > >
>>> if (TYPE_PC_DIMM) { >>> pc_dimm_plug() >>> /* do here additional concrete machine specific things */ >>> } else if (TYPE_VIRTIO_MEM) { >>> virtio_mem_plug() <- do forwarding in there >>> /* and do here additional concrete machine specific things */ >>> } else if (TYPE_CPU) { >>> cpu_plug() >>> /* do here additional concrete machine specific things */ >>> } >> >> That will result in a lot of duplicate code - for every machine we >> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - >> virtio-mem and virtio-pmem could most probably share the code. > maybe or maybe not, depending on if pmem endups as memory device or > separate controller. And even with duplication, machine code would > be easy to follow just down one explicit call chain. Not 100% convinced but I am now going into that direction.
On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: > > >>> if (TYPE_PC_DIMM) { > >>> pc_dimm_plug() > >>> /* do here additional concrete machine specific things */ > >>> } else if (TYPE_VIRTIO_MEM) { > >>> virtio_mem_plug() <- do forwarding in there > >>> /* and do here additional concrete machine specific things */ > >>> } else if (TYPE_CPU) { > >>> cpu_plug() > >>> /* do here additional concrete machine specific things */ > >>> } > >> > >> That will result in a lot of duplicate code - for every machine we > >> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - > >> virtio-mem and virtio-pmem could most probably share the code. > > maybe or maybe not, depending on if pmem endups as memory device or > > separate controller. And even with duplication, machine code would > > be easy to follow just down one explicit call chain. > > Not 100% convinced but I am now going into that direction. Can this go into DeviceClass? Failover has the same need to allocate/free resources for vfio without a full realize/unrealize. > -- > > Thanks, > > David / dhildenb
On 08.06.2018 14:55, Michael S. Tsirkin wrote: > On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: >> >>>>> if (TYPE_PC_DIMM) { >>>>> pc_dimm_plug() >>>>> /* do here additional concrete machine specific things */ >>>>> } else if (TYPE_VIRTIO_MEM) { >>>>> virtio_mem_plug() <- do forwarding in there >>>>> /* and do here additional concrete machine specific things */ >>>>> } else if (TYPE_CPU) { >>>>> cpu_plug() >>>>> /* do here additional concrete machine specific things */ >>>>> } >>>> >>>> That will result in a lot of duplicate code - for every machine we >>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - >>>> virtio-mem and virtio-pmem could most probably share the code. >>> maybe or maybe not, depending on if pmem endups as memory device or >>> separate controller. And even with duplication, machine code would >>> be easy to follow just down one explicit call chain. >> >> Not 100% convinced but I am now going into that direction. > > Can this go into DeviceClass? Failover has the same need to > allocate/free resources for vfio without a full realize/unrealize. > Conceptually, I would have called here something like virtio_mem_plug() ... Which would end up calling memory_device_plug() and triggering the target hotplug handler. I assume this can also be done from a device class callback. So we would need a total of 3 callbacks for a) pre_plug b) plug c) unplug In addition, unplug requests might be necessary, so d) unplug_request >> -- >> >> Thanks, >> >> David / dhildenb
On Fri, Jun 08, 2018 at 03:07:53PM +0200, David Hildenbrand wrote: > On 08.06.2018 14:55, Michael S. Tsirkin wrote: > > On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: > >> > >>>>> if (TYPE_PC_DIMM) { > >>>>> pc_dimm_plug() > >>>>> /* do here additional concrete machine specific things */ > >>>>> } else if (TYPE_VIRTIO_MEM) { > >>>>> virtio_mem_plug() <- do forwarding in there > >>>>> /* and do here additional concrete machine specific things */ > >>>>> } else if (TYPE_CPU) { > >>>>> cpu_plug() > >>>>> /* do here additional concrete machine specific things */ > >>>>> } > >>>> > >>>> That will result in a lot of duplicate code - for every machine we > >>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - > >>>> virtio-mem and virtio-pmem could most probably share the code. > >>> maybe or maybe not, depending on if pmem endups as memory device or > >>> separate controller. And even with duplication, machine code would > >>> be easy to follow just down one explicit call chain. > >> > >> Not 100% convinced but I am now going into that direction. > > > > Can this go into DeviceClass? Failover has the same need to > > allocate/free resources for vfio without a full realize/unrealize. > > > > Conceptually, I would have called here something like > > virtio_mem_plug() ... > > Which would end up calling memory_device_plug() and triggering the > target hotplug handler. > > I assume this can also be done from a device class callback. > > So we would need a total of 3 callbacks for > > a) pre_plug > b) plug > c) unplug > > In addition, unplug requests might be necessary, so > > d) unplug_request Right - basically HotplugHandlerClass. > >> -- > >> > >> Thanks, > >> > >> David / dhildenb > > > -- > > Thanks, > > David / dhildenb
On 08.06.2018 17:12, Michael S. Tsirkin wrote: > On Fri, Jun 08, 2018 at 03:07:53PM +0200, David Hildenbrand wrote: >> On 08.06.2018 14:55, Michael S. Tsirkin wrote: >>> On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: >>>> >>>>>>> if (TYPE_PC_DIMM) { >>>>>>> pc_dimm_plug() >>>>>>> /* do here additional concrete machine specific things */ >>>>>>> } else if (TYPE_VIRTIO_MEM) { >>>>>>> virtio_mem_plug() <- do forwarding in there >>>>>>> /* and do here additional concrete machine specific things */ >>>>>>> } else if (TYPE_CPU) { >>>>>>> cpu_plug() >>>>>>> /* do here additional concrete machine specific things */ >>>>>>> } >>>>>> >>>>>> That will result in a lot of duplicate code - for every machine we >>>>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - >>>>>> virtio-mem and virtio-pmem could most probably share the code. >>>>> maybe or maybe not, depending on if pmem endups as memory device or >>>>> separate controller. And even with duplication, machine code would >>>>> be easy to follow just down one explicit call chain. >>>> >>>> Not 100% convinced but I am now going into that direction. >>> >>> Can this go into DeviceClass? Failover has the same need to >>> allocate/free resources for vfio without a full realize/unrealize. >>> >> >> Conceptually, I would have called here something like >> >> virtio_mem_plug() ... >> >> Which would end up calling memory_device_plug() and triggering the >> target hotplug handler. >> >> I assume this can also be done from a device class callback. >> >> So we would need a total of 3 callbacks for >> >> a) pre_plug >> b) plug >> c) unplug >> >> In addition, unplug requests might be necessary, so >> >> d) unplug_request > > > Right - basically HotplugHandlerClass. Looking into this idea: What I would have right now (conceptually) if (TYPE_PC_DIMM) { pc_dimm_plug(machine); } else if (TYPE_CPU) { cpu_plug(machine); } else if (TYPE_VIRTIO_MEM) { virtio_mem_plug(machine); } Instead you want something like: if (TYPE_PC_DIMM) { pc_dimm_plug(machine); } else if (TYPE_CPU) { cpu_plug(machine); // igor requested an explicit list here, we could also check for // DEVICE_CLASS(device)->plug and make it generic } else if (TYPE_VIRTIO_MEM) { DEVICE_CLASS(device)->plug(machine); // call bus hotplug handler if necessary, or let the previous call // handle it? } We cannot pass the machine directly (due to board.h and user-only), instead we would have to pass it as hotplug handler. Then, the device class code would however make assumptions that always a machine is passed. Any opinions? >>>> -- >>>> >>>> Thanks, >>>> >>>> David / dhildenb >> >> >> -- >> >> Thanks, >> >> David / dhildenb
On Wed, 13 Jun 2018 12:58:46 +0200 David Hildenbrand <david@redhat.com> wrote: > On 08.06.2018 17:12, Michael S. Tsirkin wrote: > > On Fri, Jun 08, 2018 at 03:07:53PM +0200, David Hildenbrand wrote: > >> On 08.06.2018 14:55, Michael S. Tsirkin wrote: > >>> On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: > >>>> > >>>>>>> if (TYPE_PC_DIMM) { > >>>>>>> pc_dimm_plug() > >>>>>>> /* do here additional concrete machine specific things */ > >>>>>>> } else if (TYPE_VIRTIO_MEM) { > >>>>>>> virtio_mem_plug() <- do forwarding in there > >>>>>>> /* and do here additional concrete machine specific things */ > >>>>>>> } else if (TYPE_CPU) { > >>>>>>> cpu_plug() > >>>>>>> /* do here additional concrete machine specific things */ > >>>>>>> } > >>>>>> > >>>>>> That will result in a lot of duplicate code - for every machine we > >>>>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - > >>>>>> virtio-mem and virtio-pmem could most probably share the code. > >>>>> maybe or maybe not, depending on if pmem endups as memory device or > >>>>> separate controller. And even with duplication, machine code would > >>>>> be easy to follow just down one explicit call chain. > >>>> > >>>> Not 100% convinced but I am now going into that direction. > >>> > >>> Can this go into DeviceClass? Failover has the same need to > >>> allocate/free resources for vfio without a full realize/unrealize. > >>> > >> > >> Conceptually, I would have called here something like > >> > >> virtio_mem_plug() ... > >> > >> Which would end up calling memory_device_plug() and triggering the > >> target hotplug handler. > >> > >> I assume this can also be done from a device class callback. > >> > >> So we would need a total of 3 callbacks for > >> > >> a) pre_plug > >> b) plug > >> c) unplug > >> > >> In addition, unplug requests might be necessary, so > >> > >> d) unplug_request > > > > > > Right - basically HotplugHandlerClass. > > Looking into this idea: > > What I would have right now (conceptually) > > if (TYPE_PC_DIMM) { > pc_dimm_plug(machine); > } else if (TYPE_CPU) { > cpu_plug(machine); > } else if (TYPE_VIRTIO_MEM) { > virtio_mem_plug(machine); > } > > Instead you want something like: > > if (TYPE_PC_DIMM) { > pc_dimm_plug(machine); > } else if (TYPE_CPU) { > cpu_plug(machine); > // igor requested an explicit list here, we could also check for > // DEVICE_CLASS(device)->plug and make it generic > } else if (TYPE_VIRTIO_MEM) { > DEVICE_CLASS(device)->plug(machine); > // call bus hotplug handler if necessary, or let the previous call > // handle it? not exactly this, I suggested following: [ ... specific to machine_foo wiring ...] virtio_mem_plug() { [... some machine specific wiring ...] bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) [... some more machine specific wiring ...] } [ ... specific to machine_foo wiring ...] i.e. device itself doesn't participate in attaching to external entities, those entities (machine or bus controller virtio device is attached to) do wiring on their own within their own domain. > } > > We cannot pass the machine directly (due to board.h and user-only), > instead we would have to pass it as hotplug handler. Then, the device > class code would however make assumptions that always a machine is passed. > > Any opinions? > > > > >>>> -- > >>>> > >>>> Thanks, > >>>> > >>>> David / dhildenb > >> > >> > >> -- > >> > >> Thanks, > >> > >> David / dhildenb > >
On 13.06.2018 17:48, Igor Mammedov wrote: > On Wed, 13 Jun 2018 12:58:46 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 08.06.2018 17:12, Michael S. Tsirkin wrote: >>> On Fri, Jun 08, 2018 at 03:07:53PM +0200, David Hildenbrand wrote: >>>> On 08.06.2018 14:55, Michael S. Tsirkin wrote: >>>>> On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: >>>>>> >>>>>>>>> if (TYPE_PC_DIMM) { >>>>>>>>> pc_dimm_plug() >>>>>>>>> /* do here additional concrete machine specific things */ >>>>>>>>> } else if (TYPE_VIRTIO_MEM) { >>>>>>>>> virtio_mem_plug() <- do forwarding in there >>>>>>>>> /* and do here additional concrete machine specific things */ >>>>>>>>> } else if (TYPE_CPU) { >>>>>>>>> cpu_plug() >>>>>>>>> /* do here additional concrete machine specific things */ >>>>>>>>> } >>>>>>>> >>>>>>>> That will result in a lot of duplicate code - for every machine we >>>>>>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - >>>>>>>> virtio-mem and virtio-pmem could most probably share the code. >>>>>>> maybe or maybe not, depending on if pmem endups as memory device or >>>>>>> separate controller. And even with duplication, machine code would >>>>>>> be easy to follow just down one explicit call chain. >>>>>> >>>>>> Not 100% convinced but I am now going into that direction. >>>>> >>>>> Can this go into DeviceClass? Failover has the same need to >>>>> allocate/free resources for vfio without a full realize/unrealize. >>>>> >>>> >>>> Conceptually, I would have called here something like >>>> >>>> virtio_mem_plug() ... >>>> >>>> Which would end up calling memory_device_plug() and triggering the >>>> target hotplug handler. >>>> >>>> I assume this can also be done from a device class callback. >>>> >>>> So we would need a total of 3 callbacks for >>>> >>>> a) pre_plug >>>> b) plug >>>> c) unplug >>>> >>>> In addition, unplug requests might be necessary, so >>>> >>>> d) unplug_request >>> >>> >>> Right - basically HotplugHandlerClass. >> >> Looking into this idea: >> >> What I would have right now (conceptually) >> >> if (TYPE_PC_DIMM) { >> pc_dimm_plug(machine); >> } else if (TYPE_CPU) { >> cpu_plug(machine); >> } else if (TYPE_VIRTIO_MEM) { >> virtio_mem_plug(machine); >> } >> >> Instead you want something like: >> >> if (TYPE_PC_DIMM) { >> pc_dimm_plug(machine); >> } else if (TYPE_CPU) { >> cpu_plug(machine); >> // igor requested an explicit list here, we could also check for >> // DEVICE_CLASS(device)->plug and make it generic >> } else if (TYPE_VIRTIO_MEM) { >> DEVICE_CLASS(device)->plug(machine); >> // call bus hotplug handler if necessary, or let the previous call >> // handle it? > not exactly this, I suggested following: > > [ ... specific to machine_foo wiring ...] > > virtio_mem_plug() { > [... some machine specific wiring ...] > > bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() > bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) > > [... some more machine specific wiring ...] > } > > [ ... specific to machine_foo wiring ...] > > i.e. device itself doesn't participate in attaching to external entities, > those entities (machine or bus controller virtio device is attached to) > do wiring on their own within their own domain. I am fine with this, but Michael asked if this ("virtio_mem_plug()") could go via new DeviceClass functions. Michael, would that also work for your use case?
On Wed, Jun 13, 2018 at 05:51:01PM +0200, David Hildenbrand wrote: > On 13.06.2018 17:48, Igor Mammedov wrote: > > On Wed, 13 Jun 2018 12:58:46 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 08.06.2018 17:12, Michael S. Tsirkin wrote: > >>> On Fri, Jun 08, 2018 at 03:07:53PM +0200, David Hildenbrand wrote: > >>>> On 08.06.2018 14:55, Michael S. Tsirkin wrote: > >>>>> On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: > >>>>>> > >>>>>>>>> if (TYPE_PC_DIMM) { > >>>>>>>>> pc_dimm_plug() > >>>>>>>>> /* do here additional concrete machine specific things */ > >>>>>>>>> } else if (TYPE_VIRTIO_MEM) { > >>>>>>>>> virtio_mem_plug() <- do forwarding in there > >>>>>>>>> /* and do here additional concrete machine specific things */ > >>>>>>>>> } else if (TYPE_CPU) { > >>>>>>>>> cpu_plug() > >>>>>>>>> /* do here additional concrete machine specific things */ > >>>>>>>>> } > >>>>>>>> > >>>>>>>> That will result in a lot of duplicate code - for every machine we > >>>>>>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - > >>>>>>>> virtio-mem and virtio-pmem could most probably share the code. > >>>>>>> maybe or maybe not, depending on if pmem endups as memory device or > >>>>>>> separate controller. And even with duplication, machine code would > >>>>>>> be easy to follow just down one explicit call chain. > >>>>>> > >>>>>> Not 100% convinced but I am now going into that direction. > >>>>> > >>>>> Can this go into DeviceClass? Failover has the same need to > >>>>> allocate/free resources for vfio without a full realize/unrealize. > >>>>> > >>>> > >>>> Conceptually, I would have called here something like > >>>> > >>>> virtio_mem_plug() ... > >>>> > >>>> Which would end up calling memory_device_plug() and triggering the > >>>> target hotplug handler. > >>>> > >>>> I assume this can also be done from a device class callback. > >>>> > >>>> So we would need a total of 3 callbacks for > >>>> > >>>> a) pre_plug > >>>> b) plug > >>>> c) unplug > >>>> > >>>> In addition, unplug requests might be necessary, so > >>>> > >>>> d) unplug_request > >>> > >>> > >>> Right - basically HotplugHandlerClass. > >> > >> Looking into this idea: > >> > >> What I would have right now (conceptually) > >> > >> if (TYPE_PC_DIMM) { > >> pc_dimm_plug(machine); > >> } else if (TYPE_CPU) { > >> cpu_plug(machine); > >> } else if (TYPE_VIRTIO_MEM) { > >> virtio_mem_plug(machine); > >> } > >> > >> Instead you want something like: > >> > >> if (TYPE_PC_DIMM) { > >> pc_dimm_plug(machine); > >> } else if (TYPE_CPU) { > >> cpu_plug(machine); > >> // igor requested an explicit list here, we could also check for > >> // DEVICE_CLASS(device)->plug and make it generic > >> } else if (TYPE_VIRTIO_MEM) { > >> DEVICE_CLASS(device)->plug(machine); > >> // call bus hotplug handler if necessary, or let the previous call > >> // handle it? > > not exactly this, I suggested following: > > > > [ ... specific to machine_foo wiring ...] > > > > virtio_mem_plug() { > > [... some machine specific wiring ...] > > > > bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() > > bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) > > > > [... some more machine specific wiring ...] > > } > > > > [ ... specific to machine_foo wiring ...] > > > > i.e. device itself doesn't participate in attaching to external entities, > > those entities (machine or bus controller virtio device is attached to) > > do wiring on their own within their own domain. > > I am fine with this, but Michael asked if this ("virtio_mem_plug()") > could go via new DeviceClass functions. Michael, would that also work > for your use case? It's not virtio specifically, I'm interested in how this will work for PCI generally. Right now we call do_pci_register_device which immediately makes it guest visible. > > -- > > Thanks, > > David / dhildenb
>>> [ ... specific to machine_foo wiring ...] >>> >>> virtio_mem_plug() { >>> [... some machine specific wiring ...] >>> >>> bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() >>> bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) >>> >>> [... some more machine specific wiring ...] >>> } >>> >>> [ ... specific to machine_foo wiring ...] >>> >>> i.e. device itself doesn't participate in attaching to external entities, >>> those entities (machine or bus controller virtio device is attached to) >>> do wiring on their own within their own domain. >> >> I am fine with this, but Michael asked if this ("virtio_mem_plug()") >> could go via new DeviceClass functions. Michael, would that also work >> for your use case? > > It's not virtio specifically, I'm interested in how this will work for > PCI generally. Right now we call do_pci_register_device which > immediately makes it guest visible. So you're telling me that a PCI device exposes itself to the system in pci_qdev_realize() instead of letting a hotplug handler handle that? My assumption is that the PCI bridge hotplug handler handles this. What am I missing? I can see that e.g. for a virtio device the realize call chain is pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize -> virtio_XXX_realize() If any realization in pci_qdev_realize() fails, we do a do_pci_unregister_device(). So if it is true what you're saying than we're already exposing partially realized (and possibly unrealized again) devices via PCI. I *guess* because we're holding the iothread mutex this is okay and actually not visible. And we only seem to be sending events in the PCI bridge hotplug handlers, so my assumption is that this is fine. > > >> >> -- >> >> Thanks, >> >> David / dhildenb
On Wed, Jun 13, 2018 at 09:37:54PM +0200, David Hildenbrand wrote: > >>> [ ... specific to machine_foo wiring ...] > >>> > >>> virtio_mem_plug() { > >>> [... some machine specific wiring ...] > >>> > >>> bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() > >>> bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) > >>> > >>> [... some more machine specific wiring ...] > >>> } > >>> > >>> [ ... specific to machine_foo wiring ...] > >>> > >>> i.e. device itself doesn't participate in attaching to external entities, > >>> those entities (machine or bus controller virtio device is attached to) > >>> do wiring on their own within their own domain. > >> > >> I am fine with this, but Michael asked if this ("virtio_mem_plug()") > >> could go via new DeviceClass functions. Michael, would that also work > >> for your use case? > > > > It's not virtio specifically, I'm interested in how this will work for > > PCI generally. Right now we call do_pci_register_device which > > immediately makes it guest visible. > > So you're telling me that a PCI device exposes itself to the system in > pci_qdev_realize() instead of letting a hotplug handler handle that? My > assumption is that the PCI bridge hotplug handler handles this. Well given how things work in qemu that's not exactly the case. See below. > What am > I missing? > > I can see that e.g. for a virtio device the realize call chain is > > pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize -> > virtio_XXX_realize() > > If any realization in pci_qdev_realize() fails, we do a > do_pci_unregister_device(). > > So if it is true what you're saying than we're already exposing > partially realized (and possibly unrealized again) devices via PCI. I > *guess* because we're holding the iothread mutex this is okay and > actually not visible. For now but we need ability to have separate new commands for realize and plug, so we will drop the mutex. > And we only seem to be sending events in the PCI > bridge hotplug handlers, so my assumption is that this is fine. For core PCI, it's mostly just this line: bus->devices[devfn] = pci_dev; which makes it accessible to pci config cycles. But failover also cares about vfio, which seems to set up e.g. irqfs on realize. > > > > > >> > >> -- > >> > >> Thanks, > >> > >> David / dhildenb > > > -- > > Thanks, > > David / dhildenb
On 14.06.2018 00:05, Michael S. Tsirkin wrote: > On Wed, Jun 13, 2018 at 09:37:54PM +0200, David Hildenbrand wrote: >>>>> [ ... specific to machine_foo wiring ...] >>>>> >>>>> virtio_mem_plug() { >>>>> [... some machine specific wiring ...] >>>>> >>>>> bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() >>>>> bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) >>>>> >>>>> [... some more machine specific wiring ...] >>>>> } >>>>> >>>>> [ ... specific to machine_foo wiring ...] >>>>> >>>>> i.e. device itself doesn't participate in attaching to external entities, >>>>> those entities (machine or bus controller virtio device is attached to) >>>>> do wiring on their own within their own domain. >>>> >>>> I am fine with this, but Michael asked if this ("virtio_mem_plug()") >>>> could go via new DeviceClass functions. Michael, would that also work >>>> for your use case? >>> >>> It's not virtio specifically, I'm interested in how this will work for >>> PCI generally. Right now we call do_pci_register_device which >>> immediately makes it guest visible. >> >> So you're telling me that a PCI device exposes itself to the system in >> pci_qdev_realize() instead of letting a hotplug handler handle that? My >> assumption is that the PCI bridge hotplug handler handles this. > > Well given how things work in qemu that's not exactly > the case. See below. > >> What am >> I missing? >> >> I can see that e.g. for a virtio device the realize call chain is >> >> pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize -> >> virtio_XXX_realize() >> >> If any realization in pci_qdev_realize() fails, we do a >> do_pci_unregister_device(). >> >> So if it is true what you're saying than we're already exposing >> partially realized (and possibly unrealized again) devices via PCI. I >> *guess* because we're holding the iothread mutex this is okay and >> actually not visible. > > For now but we need ability to have separate new commands for > realize and plug, so we will drop the mutex. If you want to actually drop the mutex, I assume that quite some rework will be necessary (not only for this specific PCI hotplug handler case), most probably also in other code parts (to) - all of the hotplug/realize code seems to rely on the mutex being locked and not being dropped temporarily. > >> And we only seem to be sending events in the PCI >> bridge hotplug handlers, so my assumption is that this is fine. > > For core PCI, it's mostly just this line: > > bus->devices[devfn] = pci_dev; This should go into the hotplug handler if I am not wrong. From what I learned from Igor, this is a layer violation. Resource assignment should happen during pre_plug / plug. But then you might need a different way to "reserve" a function (if there could be races then with the lock temporarily dropped). > > which makes it accessible to pci config cycles. > > But failover also cares about vfio, which seems to set up > e.g. irqfs on realize.
On Thu, 14 Jun 2018 08:14:05 +0200 David Hildenbrand <david@redhat.com> wrote: > On 14.06.2018 00:05, Michael S. Tsirkin wrote: > > On Wed, Jun 13, 2018 at 09:37:54PM +0200, David Hildenbrand wrote: > >>>>> [ ... specific to machine_foo wiring ...] > >>>>> > >>>>> virtio_mem_plug() { > >>>>> [... some machine specific wiring ...] > >>>>> > >>>>> bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() > >>>>> bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) > >>>>> > >>>>> [... some more machine specific wiring ...] > >>>>> } > >>>>> > >>>>> [ ... specific to machine_foo wiring ...] > >>>>> > >>>>> i.e. device itself doesn't participate in attaching to external entities, > >>>>> those entities (machine or bus controller virtio device is attached to) > >>>>> do wiring on their own within their own domain. > >>>> > >>>> I am fine with this, but Michael asked if this ("virtio_mem_plug()") > >>>> could go via new DeviceClass functions. Michael, would that also work > >>>> for your use case? > >>> > >>> It's not virtio specifically, I'm interested in how this will work for > >>> PCI generally. Right now we call do_pci_register_device which > >>> immediately makes it guest visible. > >> > >> So you're telling me that a PCI device exposes itself to the system in > >> pci_qdev_realize() instead of letting a hotplug handler handle that? My > >> assumption is that the PCI bridge hotplug handler handles this. > > > > Well given how things work in qemu that's not exactly > > the case. See below. > > > >> What am > >> I missing? > >> > >> I can see that e.g. for a virtio device the realize call chain is > >> > >> pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize -> > >> virtio_XXX_realize() > >> > >> If any realization in pci_qdev_realize() fails, we do a > >> do_pci_unregister_device(). > >> > >> So if it is true what you're saying than we're already exposing > >> partially realized (and possibly unrealized again) devices via PCI. I > >> *guess* because we're holding the iothread mutex this is okay and > >> actually not visible. > > > > For now but we need ability to have separate new commands for > > realize and plug, so we will drop the mutex. > > If you want to actually drop the mutex, I assume that quite some rework > will be necessary (not only for this specific PCI hotplug handler case), > most probably also in other code parts (to) - all of the hotplug/realize > code seems to rely on the mutex being locked and not being dropped > temporarily. yep, all monitor actions and reactions from guest via mmio are now protected by global lock that guaranties not parallel action could executed at the same time. So it's save for now and dropping global lock would require some refactoring (probably a lot). > > > >> And we only seem to be sending events in the PCI > >> bridge hotplug handlers, so my assumption is that this is fine. > > > > For core PCI, it's mostly just this line: > > > > bus->devices[devfn] = pci_dev; > > This should go into the hotplug handler if I am not wrong. From what I > learned from Igor, this is a layer violation. Resource assignment should > happen during pre_plug / plug. But then you might need a different way > to "reserve" a function (if there could be races then with the lock > temporarily dropped). I agree, but it's a separate refactoring and it isn't pre-requisite for virtio-mem work, so it shouldn't hold it. > > which makes it accessible to pci config cycles. > > > > But failover also cares about vfio, which seems to set up > > e.g. irqfs on realize. Do we have a thread for failover somewhere on the list that discusses ideas and requirements for it, where we can discuss it? Otherwise this discussion will get buried in this unrelated thread.
On Wed, 13 Jun 2018 17:51:01 +0200 David Hildenbrand <david@redhat.com> wrote: > On 13.06.2018 17:48, Igor Mammedov wrote: > > On Wed, 13 Jun 2018 12:58:46 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 08.06.2018 17:12, Michael S. Tsirkin wrote: > >>> On Fri, Jun 08, 2018 at 03:07:53PM +0200, David Hildenbrand wrote: > >>>> On 08.06.2018 14:55, Michael S. Tsirkin wrote: > >>>>> On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: > >>>>>> > >>>>>>>>> if (TYPE_PC_DIMM) { > >>>>>>>>> pc_dimm_plug() > >>>>>>>>> /* do here additional concrete machine specific things */ > >>>>>>>>> } else if (TYPE_VIRTIO_MEM) { > >>>>>>>>> virtio_mem_plug() <- do forwarding in there > >>>>>>>>> /* and do here additional concrete machine specific things */ > >>>>>>>>> } else if (TYPE_CPU) { > >>>>>>>>> cpu_plug() > >>>>>>>>> /* do here additional concrete machine specific things */ > >>>>>>>>> } > >>>>>>>> > >>>>>>>> That will result in a lot of duplicate code - for every machine we > >>>>>>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - > >>>>>>>> virtio-mem and virtio-pmem could most probably share the code. > >>>>>>> maybe or maybe not, depending on if pmem endups as memory device or > >>>>>>> separate controller. And even with duplication, machine code would > >>>>>>> be easy to follow just down one explicit call chain. > >>>>>> > >>>>>> Not 100% convinced but I am now going into that direction. > >>>>> > >>>>> Can this go into DeviceClass? Failover has the same need to > >>>>> allocate/free resources for vfio without a full realize/unrealize. > >>>>> > >>>> > >>>> Conceptually, I would have called here something like > >>>> > >>>> virtio_mem_plug() ... > >>>> > >>>> Which would end up calling memory_device_plug() and triggering the > >>>> target hotplug handler. > >>>> > >>>> I assume this can also be done from a device class callback. > >>>> > >>>> So we would need a total of 3 callbacks for > >>>> > >>>> a) pre_plug > >>>> b) plug > >>>> c) unplug > >>>> > >>>> In addition, unplug requests might be necessary, so > >>>> > >>>> d) unplug_request > >>> > >>> > >>> Right - basically HotplugHandlerClass. > >> > >> Looking into this idea: > >> > >> What I would have right now (conceptually) > >> > >> if (TYPE_PC_DIMM) { > >> pc_dimm_plug(machine); > >> } else if (TYPE_CPU) { > >> cpu_plug(machine); > >> } else if (TYPE_VIRTIO_MEM) { > >> virtio_mem_plug(machine); > >> } > >> > >> Instead you want something like: > >> > >> if (TYPE_PC_DIMM) { > >> pc_dimm_plug(machine); > >> } else if (TYPE_CPU) { > >> cpu_plug(machine); > >> // igor requested an explicit list here, we could also check for > >> // DEVICE_CLASS(device)->plug and make it generic > >> } else if (TYPE_VIRTIO_MEM) { > >> DEVICE_CLASS(device)->plug(machine); > >> // call bus hotplug handler if necessary, or let the previous call > >> // handle it? > > not exactly this, I suggested following: > > > > [ ... specific to machine_foo wiring ...] > > > > virtio_mem_plug() { > > [... some machine specific wiring ...] > > > > bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() > > bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) > > > > [... some more machine specific wiring ...] > > } > > > > [ ... specific to machine_foo wiring ...] > > > > i.e. device itself doesn't participate in attaching to external entities, > > those entities (machine or bus controller virtio device is attached to) > > do wiring on their own within their own domain. > > I am fine with this, but Michael asked if this ("virtio_mem_plug()") > could go via new DeviceClass functions. Michael, would that also work > for your use case? We can discuss DeviceClass functions when patches for failover surface and if it's really need.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d768930d02..510076e156 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + Error *local_err = NULL; + + /* final stage hotplug handler */ if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { - pc_cpu_pre_plug(hotplug_dev, dev, errp); + pc_cpu_pre_plug(hotplug_dev, dev, &local_err); + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, + &local_err); } + error_propagate(errp, local_err); } static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + Error *local_err = NULL; + + /* final stage hotplug handler */ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { - pc_dimm_plug(hotplug_dev, dev, errp); + pc_dimm_plug(hotplug_dev, dev, &local_err); } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { - pc_cpu_plug(hotplug_dev, dev, errp); + pc_cpu_plug(hotplug_dev, dev, &local_err); + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); } + error_propagate(errp, local_err); } static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, @@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, pc_dimm_unplug_request(hotplug_dev, dev, errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { pc_cpu_unplug_request_cb(hotplug_dev, dev, errp); - } else { + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev, + errp); + } else if (!dev->parent_bus) { error_setg(errp, "acpi: device unplug request for not supported device" " type: %s", object_get_typename(OBJECT(dev))); } @@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + Error *local_err = NULL; + + /* final stage hotplug handler */ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { - pc_dimm_unplug(hotplug_dev, dev, errp); + pc_dimm_unplug(hotplug_dev, dev, &local_err); } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { - pc_cpu_unplug_cb(hotplug_dev, dev, errp); - } else { - error_setg(errp, "acpi: device unplug for not supported device" + pc_cpu_unplug_cb(hotplug_dev, dev, &local_err); + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, + &local_err); + } else if (!dev->parent_bus) { + error_setg(&local_err, "acpi: device unplug for not supported device" " type: %s", object_get_typename(OBJECT(dev))); } + error_propagate(errp, local_err); } static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests). Also, add code to pass control to the final stage hotplug handler at the parent bus. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-)