diff mbox series

[v2,04/17] pc: route all memory devices through the machine hotplug handler

Message ID 20180511131953.12905-5-david@redhat.com
State New
Headers show
Series MemoryDevice: use multi stage hotplug handlers | expand

Commit Message

David Hildenbrand May 11, 2018, 1:19 p.m. UTC 
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(+)

Comments

Paolo Bonzini May 12, 2018, 2:47 p.m. UTC | #1 
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
David Hildenbrand May 12, 2018, 4:45 p.m. UTC | #2 
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
>
David Hildenbrand May 14, 2018, 9:12 a.m. UTC | #3 
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 mbox series

Patch

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;
 }