Message ID | 20180511131953.12905-5-david@redhat.com |
---|---|
State | New |
Headers | show |
Series | MemoryDevice: use multi stage hotplug handlers | expand |
On 11/05/2018 15:19, David Hildenbrand wrote: > + if (dev->parent_bus) { > + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { > + return HOTPLUG_HANDLER(machine); > + } > + } > + How do you get here with a MemoryDevice that has !dev->parent_bus? Thanks, Paolo
On 12.05.2018 16:47, Paolo Bonzini wrote: > On 11/05/2018 15:19, David Hildenbrand wrote: >> + if (dev->parent_bus) { >> + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { >> + return HOTPLUG_HANDLER(machine); >> + } >> + } >> + > > How do you get here with a MemoryDevice that has !dev->parent_bus? > Excellent question :) This is for now (for pc and spapr) a theoretical case, but I included it to make all hotplug handler look alike and also show for other potential device (interfaces) how it should be handled. I'll give you the s390x example I had in mind: s390x cannot hotplug dimms. dimms, however are busless devices that implement the MemoryDevice interface. If we would simply always indicate this way that we have a hotplug handler, e.g. the check in qdev_device_add() would not trigger: ... if (bus) { qdev_set_parent_bus(dev, bus); } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) { /* No bus, no machine hotplug handler --> device is not hotpluggable */ error_setg(&err, "Device '%s' can not be hotplugged on this machine", driver); goto err_del_dev; } ... So the rational is "if its a busless device and I (the machine) am not able to fully plug it, I must also not partially plug it." However, right now I am not sure (due to qdev_hotplug) if this is enough. > Thanks, > > Paolo >
On 12.05.2018 16:47, Paolo Bonzini wrote: > On 11/05/2018 15:19, David Hildenbrand wrote: >> + if (dev->parent_bus) { >> + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { >> + return HOTPLUG_HANDLER(machine); >> + } >> + } >> + > > How do you get here with a MemoryDevice that has !dev->parent_bus? > Thinking about it, I'll drop this check and instead split up CONFIG_MEM_HOTPLUG into CONFIG_MEM_DEVICE and CONFIG_DIMM Thanks! > Thanks, > > Paolo >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 39153fa083..7a5558ebea 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -74,6 +74,7 @@ #include "hw/nmi.h" #include "hw/i386/intel_iommu.h" #include "hw/net/ne2000-isa.h" +#include "hw/mem/memory-device.h" /* debug PC/ISA interrupts */ //#define DEBUG_IRQ @@ -2079,6 +2080,12 @@ static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, return HOTPLUG_HANDLER(machine); } + if (dev->parent_bus) { + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) { + return HOTPLUG_HANDLER(machine); + } + } + return NULL; }
Necessary to hotplug them cleanly later. We can only support memory devices that are not DIMMs if we have a parent bus. Otherwise we might miss "Device '%s' can not be hotplugged on this machine" cases. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/i386/pc.c | 7 +++++++ 1 file changed, 7 insertions(+)