Re: [Qemu-ppc] [PATCH RFC 09/10] pc: Support for PCI based memory devices

[David Hildenbrand]

On 18.01.19 11:20, Igor Mammedov wrote:
> On Wed, 16 Jan 2019 12:35:22 +0100
> David Hildenbrand <address@hidden> wrote:
> 
>> Override the PCI device hotplug handler to properly handle the
>> memory device part from the machine hotplug handler and forward to the
>> actual PCI bus hotplug handler.
>>
>> As PCI hotplug has not been properly factored out into hotplug handlers,
>> most magic is performed in the (un)realize functions. Also some buses don't
>                                                              ^^^^^^^^ pls, be 
> more specific

Last time I checked there were some TYPE_PCI_HOST_BRIDGE devices in QEMU
that would not have a hotplug handler. I *think* they are all unrelated
to x86/pc. But as I was not sure if I am missing something, I introduced
a simple error check.

> 
>> have a PCI hotplug handler at all yet (not sure if they are actually used
>> on x86, but just to be sure).
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/i386/pc.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 89 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index fd0cb29ba9..62f83859fa 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -75,6 +75,7 @@
>>  #include "hw/usb.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
>> @@ -2224,6 +2225,84 @@ static void pc_cpu_pre_plug(HotplugHandler 
>> *hotplug_dev,
>>      numa_cpu_pre_plug(cpu_slot, dev, errp);
>>  }
>>  
>> +static void pc_pci_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> +                            Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +        if (!hotplug_dev2) {
>> +            /*
>> +             * Without a bus hotplug handler, we cannot control the 
>> plug/unplug
>> +             * order. Disallow memory devices on such buses.
>> +             */
>> +            error_setg(errp, "Memory devices not supported on this bus.");
>> +            return;
>> +        }
>> +        memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev),
>> +                               NULL, errp);
>> +    }
>> +
>> +    if (hotplug_dev2) {
>> +        hotplug_handler_pre_plug(hotplug_dev2, dev, errp);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>> +        }
>> +    }
>> +}
>> +
>> +static void pc_pci_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> +                        Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +        memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>> +    }
>> +
>> +    if (hotplug_dev2) {
>> +        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
>> +    }
>> +
>> +    if (local_err) {
>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>> +        }
>> +    }
>> +    error_propagate(errp, local_err);
>> +}
>> +
>> +static void pc_pci_unplug_request(HotplugHandler *hotplug_dev, DeviceState 
>> *dev,
>> +                                  Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +
>> +    if (hotplug_dev2) {
>> +        hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
>> +    }
>> +}
>> +
>> +static void pc_pci_device_unplug(HotplugHandler *hotplug_dev, DeviceState 
>> *dev,
>> +                                 Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (hotplug_dev2) {
>> +        hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
>> +    }
>> +
>> +    if (!local_err) {
>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>> +        }
>> +    }
>> +    error_propagate(errp, local_err);
>> +}
>> +
>>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>                                            DeviceState *dev, Error **errp)
>>  {
>> @@ -2231,6 +2310,8 @@ static void 
>> pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>          pc_memory_pre_plug(hotplug_dev, dev, errp);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>          pc_cpu_pre_plug(hotplug_dev, dev, errp);
>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> +        pc_pci_pre_plug(hotplug_dev, dev, errp);
>>      }
>>  }
>>  
>> @@ -2241,6 +2322,8 @@ static void pc_machine_device_plug_cb(HotplugHandler 
>> *hotplug_dev,
>>          pc_memory_plug(hotplug_dev, dev, errp);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>          pc_cpu_plug(hotplug_dev, dev, errp);
>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> +        pc_pci_plug(hotplug_dev, dev, errp);
>>      }
>>  }
>>  
>> @@ -2251,6 +2334,8 @@ static void 
>> pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>          pc_memory_unplug_request(hotplug_dev, dev, errp);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>          pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> +        pc_pci_unplug_request(hotplug_dev, dev, errp);
>>      } else {
>>          error_setg(errp, "acpi: device unplug request for not supported 
>> device"
>>                     " type: %s", object_get_typename(OBJECT(dev)));
>> @@ -2264,6 +2349,8 @@ static void pc_machine_device_unplug_cb(HotplugHandler 
>> *hotplug_dev,
>>          pc_memory_unplug(hotplug_dev, dev, errp);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>          pc_cpu_unplug_cb(hotplug_dev, dev, errp);
>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> +        pc_pci_device_unplug(hotplug_dev, dev, errp);
>>      } else {
>>          error_setg(errp, "acpi: device unplug for not supported device"
>>                     " type: %s", object_get_typename(OBJECT(dev)));
>> @@ -2274,7 +2361,8 @@ static HotplugHandler 
>> *pc_get_hotpug_handler(MachineState *machine,
>>                                               DeviceState *dev)
>>  {
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
>> -        object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> +        object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
>> +        object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> why all PCI devices instead of virtio-pmem-pci one?
> 

Last time I tried to include "hw/virtio/virtio-pci.h" in pc.c it
resulted in an inclusion error madness. But this should be easy to fix I
guess. (the basic idea of this patch is the important bit)

And I just tried to include and it seems to result in no errors right now.

>>          return HOTPLUG_HANDLER(machine);
>>      }
>>  
> 


-- 

Thanks,

David / dhildenb