Re: [PATCH v1 2/2] s390x/s390-virtio-ccw: Support plugging PCI-based virtio memory devices

[Michal Prívozník]

On 1/27/25 15:28, David Hildenbrand wrote:
> Let's just wire it up, unlocking virtio-mem-pci support on s390x.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3af613d4e9..71f3443a53 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -554,8 +554,7 @@ static void s390_machine_device_pre_plug(HotplugHandler 
> *hotplug_dev,
>      if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
>          virtio_ccw_md_pre_plug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), 
> errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> -        error_setg(errp,
> -                   "PCI-attached virtio based memory devices not supported");
> +        virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), 
> errp);
>      }
>  }
>  
> @@ -566,7 +565,8 @@ static void s390_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          s390_cpu_plug(hotplug_dev, dev, errp);
> -    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW) ||
> +               object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
>          /*
>           * At this point, the device is realized and set all memdevs mapped, 
> so
>           * qemu_maxrampagesize() will pick up the page sizes of these memdevs
> @@ -580,7 +580,11 @@ static void s390_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>                         " initial memory");
>              return;
>          }
> -        virtio_ccw_md_plug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), errp);
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
> +            virtio_ccw_md_plug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), 
> errp);
> +        } else {
> +            virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), 
> errp);
> +        }
>      }
>  }
>  
> @@ -589,10 +593,12 @@ static void 
> s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          error_setg(errp, "CPU hot unplug not supported on this machine");
> -        return;

This looks suspicious. Do we really want to continue executing the rest
of the function in this case (though there's nothing to execute
currently)? OTOH, the statement can be put back should it be ever needed.

>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
>          virtio_ccw_md_unplug_request(VIRTIO_MD_CCW(dev), 
> MACHINE(hotplug_dev),
>                                       errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> +        virtio_md_pci_unplug_request(VIRTIO_MD_PCI(dev), 
> MACHINE(hotplug_dev),
> +                                     errp);
>      }
>  }

Michal