>> # hw/mem/pc-dimm.c
>> mhp_pc_dimm_assigned_slot(int slot) "%d"
>> mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64
>> +# hw/mem/memory-device.c
>> +memory_device_unassign_address(uint64_t addr) "0x%"PRIx64
> maybe split out tracing into a separate patch?
Can do, although I think this is overkill.
>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 562712def2..abdd38a6b5 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3621,6 +3621,11 @@ static void spapr_machine_device_plug(HotplugHandler
>> *hotplug_dev,
>> hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev,
>> &local_err);
>> }
>> out:
>> + if (local_err) {
>> + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> + memory_device_unplug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev));
>> + }
>> + }
> we shouldn't call unplug here.
> I'd suggest spapr_memory_plug() and/or memory_device_plug() to do
> error handling and rollback on it's own, it shouldn't complicate generic
> machines' hotplug handlers.
Please note that this is the generic way to handle multi stage hotplug
handlers. (see the more detailed comment in reply to patch 4 if I
remember correctly)
>From my point of view, this is the right thing to do.
>
>> error_propagate(errp, local_err);
>> }
>>
>> @@ -3638,7 +3643,16 @@ static void
>> spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>> hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
>> &local_err);
>> }
>> - error_propagate(errp, local_err);
>> +
>> + if (local_err) {
> this check probably not needed, error_propagate()
> bails out of local_err is NULL
With the current code (returning), this is needed.
>
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> + /* first stage hotplug handler */
> I'd drop this comment, it looks not necessary and even a bit confusing to me.
I'll drop all the comments of this form.
>
>> + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> + memory_device_unplug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev));
>> + }
>> }
>>
>> static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
>> index 3a4e9edc92..b8365959e7 100644
>> --- a/include/hw/mem/memory-device.h
>> +++ b/include/hw/mem/memory-device.h
>> @@ -58,6 +58,6 @@ uint64_t memory_device_get_free_addr(MachineState *ms,
>> const uint64_t *hint,
>> Error **errp);
>> void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
>> uint64_t addr);
>> -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
>> +void memory_device_unplug(MachineState *ms, MemoryDeviceState *md);
>>
>> #endif
>
--
Thanks,
David / dhildenb
