Re: [PATCH v1] pc: Support coldplugging of virtio-pmem-pci devices on all buses

[Michael S. Tsirkin]

On Mon, May 25, 2020 at 10:45:11AM +0200, David Hildenbrand wrote:
> E.g., with "pc-q35-4.2", trying to coldplug a virtio-pmem-pci devices
> results in
>     "virtio-pmem-pci not supported on this bus"
> 
> Reasons is, that the bus does not support hotplug and, therefore, does
> not have a hotplug handler. Let's allow coldplugging virtio-pmem devices
> on such buses. The hotplug order is only relevant for virtio-pmem-pci
> when the guest is already alive and the device is visible before
> memory_device_plug() wired up the memory device bits.
> 
> Hotplug attempts will still fail with:
>     "Error: Bus 'pcie.0' does not support hotplugging"
> 
> Hotunplug attempts will still fail with:
>     "Error: Bus 'pcie.0' does not support hotplugging"
> 
> Reported-by: Vivek Goyal <vgoyal@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

I assume you are still debugging Vivek's issues, right?
Let me know when you feel it's time to merge this ...

> ---
>  hw/i386/pc.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2128f3d6fe..c740495eb6 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1663,13 +1663,13 @@ static void 
> pc_virtio_pmem_pci_pre_plug(HotplugHandler *hotplug_dev,
>      HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>      Error *local_err = NULL;
>  
> -    if (!hotplug_dev2) {
> +    if (!hotplug_dev2 && dev->hotplugged) {
>          /*
>           * 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.
> +         * order. We should never reach this point when hotplugging on x86,
> +         * however, better add a safety net.
>           */
> -        error_setg(errp, "virtio-pmem-pci not supported on this bus.");
> +        error_setg(errp, "virtio-pmem-pci hotplug not supported on this 
> bus.");
>          return;
>      }
>      /*
> @@ -1678,7 +1678,7 @@ static void pc_virtio_pmem_pci_pre_plug(HotplugHandler 
> *hotplug_dev,
>       */
>      memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev), NULL,
>                             &local_err);
> -    if (!local_err) {
> +    if (!local_err && hotplug_dev2) {
>          hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err);
>      }
>      error_propagate(errp, local_err);
> @@ -1696,9 +1696,11 @@ static void pc_virtio_pmem_pci_plug(HotplugHandler 
> *hotplug_dev,
>       * device bits.
>       */
>      memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
> -    hotplug_handler_plug(hotplug_dev2, dev, &local_err);
> -    if (local_err) {
> -        memory_device_unplug(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);
>  }
> -- 
> 2.25.4