Re: [PATCH v3 3/7] arm/virt: Use virtio-md-pci (un)plug functions

[David Hildenbrand <david@redhat.com>]

On 10.07.23 23:40, Michael S. Tsirkin wrote:
> On Mon, Jul 10, 2023 at 12:07:10PM +0200, David Hildenbrand wrote:
>> Let's use our new helper functions. Note that virtio-pmem-pci is not
>> enabled for arm and, therefore, not compiled in.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   hw/arm/virt.c | 81 ++++++++-------------------------------------------
>>   1 file changed, 12 insertions(+), 69 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 8a4c663735..4ae1996d37 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -73,11 +73,10 @@
>>   #include "hw/arm/smmuv3.h"
>>   #include "hw/acpi/acpi.h"
>>   #include "target/arm/internals.h"
>> -#include "hw/mem/memory-device.h"
>>   #include "hw/mem/pc-dimm.h"
>>   #include "hw/mem/nvdimm.h"
>>   #include "hw/acpi/generic_event_device.h"
>> -#include "hw/virtio/virtio-mem-pci.h"
>> +#include "hw/virtio/virtio-md-pci.h"
>>   #include "hw/virtio/virtio-iommu.h"
>>   #include "hw/char/pl011.h"
>>   #include "qemu/guest-random.h"
>> @@ -2740,64 +2739,6 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>>                            dev, &error_abort);
>>   }
>>   
>> -static void virt_virtio_md_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 && dev->hotplugged) {
>> -        /*
>> -         * Without a bus hotplug handler, we cannot control the plug/unplug
>> -         * order. We should never reach this point when hotplugging on ARM.
>> -         * However, it's nice to add a safety net, similar to what we have
>> -         * on x86.
>> -         */
>> -        error_setg(errp, "hotplug of virtio based memory devices not supported"
>> -                   " on this bus.");
>> -        return;
>> -    }
>> -    /*
>> -     * First, see if we can plug this memory device at all. If that
>> -     * succeeds, branch of to the actual hotplug handler.
>> -     */
>> -    memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev), NULL,
>> -                           &local_err);
>> -    if (!local_err && hotplug_dev2) {
>> -        hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err);
>> -    }
>> -    error_propagate(errp, local_err);
>> -}
>> -
>> -static void virt_virtio_md_pci_plug(HotplugHandler *hotplug_dev,
>> -                                    DeviceState *dev, Error **errp)
>> -{
>> -    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>> -    Error *local_err = NULL;
>> -
>> -    /*
>> -     * Plug the memory device first and then branch off to the actual
>> -     * hotplug handler. If that one fails, we can easily undo the memory
>> -     * device bits.
>> -     */
>> -    memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>> -    if (hotplug_dev2) {
>> -        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
>> -        if (local_err) {
>> -            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>> -        }
>> -    }
>> -    error_propagate(errp, local_err);
>> -}
>> -
>> -static void virt_virtio_md_pci_unplug_request(HotplugHandler *hotplug_dev,
>> -                                              DeviceState *dev, Error **errp)
>> -{
>> -    /* We don't support hot unplug of virtio based memory devices */
>> -    error_setg(errp, "virtio based memory devices cannot be unplugged.");
>> -}
>> -
>> -
>>   static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>                                               DeviceState *dev, Error **errp)
>>   {
>> @@ -2805,8 +2746,8 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>   
>>       if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>           virt_memory_pre_plug(hotplug_dev, dev, errp);
>> -    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
>> -        virt_virtio_md_pci_pre_plug(hotplug_dev, dev, errp);
>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
>> +        virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
>>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>>           hwaddr db_start = 0, db_end = 0;
>>           char *resv_prop_str;
>> @@ -2855,12 +2796,11 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>>                                        SYS_BUS_DEVICE(dev));
>>           }
>>       }
>> +
>>       if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>           virt_memory_plug(hotplug_dev, dev, errp);
>> -    }
>> -
>> -    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
>> -        virt_virtio_md_pci_plug(hotplug_dev, dev, errp);
>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
>> +        virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
>>       }
>>   
>>       if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> 
> 
> How is this supposed to link if virtio-md is disabled at compile time?
> 

Good point.

The old code unconditionally enabled MEM_DEVICE, so we never required 
subs for that.

We either need stubs or have to wrap this in #ifdef.

Stubs sound cleaner.

-- 
Cheers,

David / dhildenb