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(+)

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

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
> 


-- 

Thanks,

David / dhildenb

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
> 


-- 

Thanks,

David / dhildenb