Re: [qemu-s390x] [PATCH v4 06/14] spapr: prepare for multi stage hotplug handlers

[David Hildenbrand]

On 05.06.2018 03:08, David Gibson wrote:
> On Thu, May 17, 2018 at 10:15:19AM +0200, David Hildenbrand 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).
>>
>> Also, add code to pass control to the final stage hotplug handler at the
>> parent bus.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 43 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index ebf30dd60b..b7c5c95f7a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler 
>> *hotplug_dev,
>>  {
>>      MachineState *ms = MACHINE(hotplug_dev);
>>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>> +    Error *local_err = NULL;
>>  
>> +    /* final stage hotplug handler */
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>          int node;
>>  
>>          if (!smc->dr_lmb_enabled) {
>> -            error_setg(errp, "Memory hotplug not supported for this 
>> machine");
>> -            return;
>> +            error_setg(&local_err,
>> +                       "Memory hotplug not supported for this machine");
>> +            goto out;
>>          }
>> -        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
>> errp);
>> -        if (*errp) {
>> -            return;
>> +        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
>> +                                        &local_err);
>> +        if (local_err) {
>> +            goto out;
>>          }
>>          if (node < 0 || node >= MAX_NODES) {
>> -            error_setg(errp, "Invaild node %d", node);
>> -            return;
>> +            error_setg(&local_err, "Invaild node %d", node);
>> +            goto out;
>>          }
>>  
>> -        spapr_memory_plug(hotplug_dev, dev, node, errp);
>> +        spapr_memory_plug(hotplug_dev, dev, node, &local_err);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>> -        spapr_core_plug(hotplug_dev, dev, errp);
>> +        spapr_core_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);
>> +    }
>> +out:
>> +    error_propagate(errp, local_err);
>> +}
>> +
>> +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>> +                                        DeviceState *dev, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +
>> +    /* final stage hotplug handler */
>> +    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> 
> As I think Igor said on the equivalent PC patch, I don't quite get
> this.  Isn't this already handled by the generic hotplug code picking
> up the bus's hotplug handler if the machine doesn't supply one?

See my reply to patch nr 4.

What we do is, we install the machine hotplug handler as an
"intermediate" hotplug handler.

E.g. if we have a VIRTIO based MemoryDevice, we have to initialize the
MemoryDevice specific stuff in the machine hotplug handler, but then
pass the device onto the last stage hotplug handler (which will
eventually attach it to a bus and notify the guest).

For PC_DIMM, the machine hotplug handler is already the last stage
hotplug handler. Everything is fine. If it is not a PC_DIMM, we have to
pass it on to the right last stage hotplug handler (e.g. using the bus).

So the generic hotplug code will alway pick up the machine hotplug
handler for MemoryDevices first, do the MemoryDevice specific stuff and
then pass on control to the "actual" hotplug handler.

Please note that this handling only applies to device types we "route"
through the machine hotplug handler, what we will initially only do for
MemoryDevices.

Just like for PC, I factored out the introduction of the local error
variable and will add a better description to the patch descriptions on
how this plays together.

-- 

Thanks,

David / dhildenb