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

[David Hildenbrand]

On 18.01.19 15:37, Igor Mammedov wrote:
> On Fri, 18 Jan 2019 13:53:14 +0100
> David Hildenbrand <address@hidden> wrote:
> 
>> 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
> I'd prefer override for concrete devices only and the rest PCI devices
> handled as they used to. That also should simplify hotplug handlers you
> are adding here.

Yes, will do for the next round as long as gates to inclusion hell don't
open up again :)

Thanks!


-- 

Thanks,

David / dhildenb