[RFCv2,9/9] pc: Support for virtio-pmem-pci

Override the device hotplug handler to properly handle the memory device
part via virtio-pmem-pci callbacks 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 PCI host
buses don't have a PCI hotplug handler at all yet, just to be sure that
we alway have a hotplug handler on x86, add a simple error check.

Unlocking virtio-pmem will unlock virtio-pmem-pci.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 default-configs/i386-softmmu.mak |  1 +
 hw/i386/pc.c                     | 70 +++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 1 deletion(-)

On Wed, 23 Jan 2019 20:55:27 +0100
David Hildenbrand <david@redhat.com> wrote:

> Override the device hotplug handler to properly handle the memory device
> part via virtio-pmem-pci callbacks 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 PCI host
> buses don't have a PCI hotplug handler at all yet, just to be sure that
> we alway have a hotplug handler on x86, add a simple error check.
> 
> Unlocking virtio-pmem will unlock virtio-pmem-pci.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  default-configs/i386-softmmu.mak |  1 +
>  hw/i386/pc.c                     | 70 +++++++++++++++++++++++++++++++-
>  2 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 64c998c4c8..3f63e95a55 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -52,6 +52,7 @@ CONFIG_APIC=y
>  CONFIG_IOAPIC=y
>  CONFIG_PVPANIC=y
>  CONFIG_MEM_DEVICE=y
> +CONFIG_VIRTIO_PMEM=y
>  CONFIG_DIMM=y
>  CONFIG_NVDIMM=y
>  CONFIG_ACPI_NVDIMM=y
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index fd0cb29ba9..6a176caeb9 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/virtio/virtio-pmem-pci.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -2224,6 +2225,64 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      numa_cpu_pre_plug(cpu_slot, dev, errp);
>  }
>  
> +static void pc_virtio_pmem_pci_pre_plug(HotplugHandler *hotplug_dev,
> +                                        DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    if (!hotplug_dev2) {
> +        /*
> +         * Without a bus hotplug handler, we cannot control the plug/unplug
> +         * order. This should never be the case on x86, however better add
> +         * a safety net.
> +         */
> +        error_setg(errp, "virtio-pmem-pci not supported on this bus.");
> +        return;
> +    }
> +    virtio_pmem_pci_pre_plug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev),
       ^^^
doesn't do anything beside being dumb proxy to memory_device_pre_plug()
I'd just use memory_device_pre_plug() here and avoid so far needles wrappers

> +                             &local_err);
> +    if (!local_err) {
since logic is not trivial I'd add comment somewhere in this function
explaining why handlers are called in this particular order.

> +        hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err);
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
> +static void pc_virtio_pmem_pci_plug(HotplugHandler *hotplug_dev,
> +                                    DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    virtio_pmem_pci_plug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev));
> +    hotplug_handler_plug(hotplug_dev2, dev, &local_err);
> +    if (local_err) {
> +        virtio_pmem_pci_unplug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev));
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
> +static void pc_virtio_pmem_pci_unplug_request(HotplugHandler *hotplug_dev,
> +                                              DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +
> +    hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
> +}
> +
> +static void pc_virtio_pmem_pci_unplug(HotplugHandler *hotplug_dev,
> +                                      DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
> +    if (!local_err) {
> +        virtio_pmem_pci_unplug(VIRTIO_PMEM_PCI(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 +2290,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_VIRTIO_PMEM_PCI)) {
> +        pc_virtio_pmem_pci_pre_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> @@ -2241,6 +2302,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_VIRTIO_PMEM_PCI)) {
> +        pc_virtio_pmem_pci_plug(hotplug_dev, dev, errp);
>      }
>  }
>  
> @@ -2251,6 +2314,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_VIRTIO_PMEM_PCI)) {
> +        pc_virtio_pmem_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 +2329,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_VIRTIO_PMEM_PCI)) {
> +        pc_virtio_pmem_pci_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 +2341,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_VIRTIO_PMEM_PCI)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>

On 06.02.19 14:01, Igor Mammedov wrote:
> On Wed, 23 Jan 2019 20:55:27 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Override the device hotplug handler to properly handle the memory device
>> part via virtio-pmem-pci callbacks 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 PCI host
>> buses don't have a PCI hotplug handler at all yet, just to be sure that
>> we alway have a hotplug handler on x86, add a simple error check.
>>
>> Unlocking virtio-pmem will unlock virtio-pmem-pci.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  default-configs/i386-softmmu.mak |  1 +
>>  hw/i386/pc.c                     | 70 +++++++++++++++++++++++++++++++-
>>  2 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
>> index 64c998c4c8..3f63e95a55 100644
>> --- a/default-configs/i386-softmmu.mak
>> +++ b/default-configs/i386-softmmu.mak
>> @@ -52,6 +52,7 @@ CONFIG_APIC=y
>>  CONFIG_IOAPIC=y
>>  CONFIG_PVPANIC=y
>>  CONFIG_MEM_DEVICE=y
>> +CONFIG_VIRTIO_PMEM=y
>>  CONFIG_DIMM=y
>>  CONFIG_NVDIMM=y
>>  CONFIG_ACPI_NVDIMM=y
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index fd0cb29ba9..6a176caeb9 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/virtio/virtio-pmem-pci.h"
>>  
>>  /* debug PC/ISA interrupts */
>>  //#define DEBUG_IRQ
>> @@ -2224,6 +2225,64 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>      numa_cpu_pre_plug(cpu_slot, dev, errp);
>>  }
>>  
>> +static void pc_virtio_pmem_pci_pre_plug(HotplugHandler *hotplug_dev,
>> +                                        DeviceState *dev, Error **errp)
>> +{
>> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> +    Error *local_err = NULL;
>> +
>> +    if (!hotplug_dev2) {
>> +        /*
>> +         * Without a bus hotplug handler, we cannot control the plug/unplug
>> +         * order. This should never be the case on x86, however better add
>> +         * a safety net.
>> +         */
>> +        error_setg(errp, "virtio-pmem-pci not supported on this bus.");
>> +        return;
>> +    }
>> +    virtio_pmem_pci_pre_plug(VIRTIO_PMEM_PCI(dev), MACHINE(hotplug_dev),
>        ^^^
> doesn't do anything beside being dumb proxy to memory_device_pre_plug()
> I'd just use memory_device_pre_plug() here and avoid so far needles wrappers

Had that initially but considered it cleaner. But I can change that back.

> 
>> +                             &local_err);
>> +    if (!local_err) {
> since logic is not trivial I'd add comment somewhere in this function
> explaining why handlers are called in this particular order.

Good idea, will do that.

Thanks!