Re: [Qemu-devel] [PATCH RFCv2 9/9] pc: Support for virtio-pmem-pci

[David Hildenbrand <david@redhat.com>]

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!

-- 

Thanks,

David / dhildenb