Re: [PATCH] hw/s390x: Restrict "loadparm" property to devices that can be used for booting

[Jared Rossi]

On 11/13/24 6:47 AM, Thomas Huth wrote:
> Commit bb185de423 ("s390x: Add individual loadparm assignment to
> CCW device") added a "loadparm" property to all CCW devices. This
> was a little bit unfortunate, since this property is only useful
> for devices that can be used for booting, but certainly it is not
> useful for devices like virtio-gpu or virtio-tablet.
>
> Thus let's restrict the property to CCW devices that we can boot from
> (i.e. virtio-block, virtio-net and vfio-ccw devices).
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/s390x/ccw-device.h     | 5 +++++
>   hw/s390x/ccw-device.c     | 4 +---
>   hw/s390x/virtio-ccw-blk.c | 1 +
>   hw/s390x/virtio-ccw-net.c | 1 +
>   hw/vfio/ccw.c             | 1 +
>   5 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
> index 1e1737c0f3..4439feb140 100644
> --- a/hw/s390x/ccw-device.h
> +++ b/hw/s390x/ccw-device.h
> @@ -51,4 +51,9 @@ static inline CcwDevice *to_ccw_dev_fast(DeviceState *d)
>   
>   OBJECT_DECLARE_TYPE(CcwDevice, CCWDeviceClass, CCW_DEVICE)
>   
> +extern const PropertyInfo ccw_loadparm;
> +
> +#define DEFINE_PROP_CCW_LOADPARM(_n, _s, _f) \
> +    DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(uint8_t[8]))
> +
>   #endif
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index 230cc09e03..30f2fb486f 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -73,7 +73,7 @@ static void ccw_device_set_loadparm(Object *obj, Visitor *v,
>       s390_ipl_fmt_loadparm(dev->loadparm, val, errp);
>   }
>   
> -static const PropertyInfo ccw_loadparm = {
> +const PropertyInfo ccw_loadparm = {
>       .name  = "ccw_loadparm",
>       .description = "Up to 8 chars in set of [A-Za-z0-9. ] to pass"
>               " to the guest loader/kernel",
> @@ -85,8 +85,6 @@ static Property ccw_device_properties[] = {
>       DEFINE_PROP_CSS_DEV_ID("devno", CcwDevice, devno),
>       DEFINE_PROP_CSS_DEV_ID_RO("dev_id", CcwDevice, dev_id),
>       DEFINE_PROP_CSS_DEV_ID_RO("subch_id", CcwDevice, subch_id),
> -    DEFINE_PROP("loadparm", CcwDevice, loadparm, ccw_loadparm,
> -            typeof(uint8_t[8])),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/s390x/virtio-ccw-blk.c b/hw/s390x/virtio-ccw-blk.c
> index 8e0e58b77d..2364432c6e 100644
> --- a/hw/s390x/virtio-ccw-blk.c
> +++ b/hw/s390x/virtio-ccw-blk.c
> @@ -48,6 +48,7 @@ static Property virtio_ccw_blk_properties[] = {
>                       VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
>       DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
>                          VIRTIO_CCW_MAX_REV),
> +    DEFINE_PROP_CCW_LOADPARM("loadparm", CcwDevice, loadparm),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/s390x/virtio-ccw-net.c b/hw/s390x/virtio-ccw-net.c
> index 484e617659..a4a3f65c7e 100644
> --- a/hw/s390x/virtio-ccw-net.c
> +++ b/hw/s390x/virtio-ccw-net.c
> @@ -51,6 +51,7 @@ static Property virtio_ccw_net_properties[] = {
>                       VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
>       DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
>                          VIRTIO_CCW_MAX_REV),
> +    DEFINE_PROP_CCW_LOADPARM("loadparm", CcwDevice, loadparm),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 24703c814a..c1cd7736cd 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -662,6 +662,7 @@ static Property vfio_ccw_properties[] = {
>       DEFINE_PROP_LINK("iommufd", VFIOCCWDevice, vdev.iommufd,
>                        TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
>   #endif
> +    DEFINE_PROP_CCW_LOADPARM("loadparm", CcwDevice, loadparm),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   

Hi Thomas,

Thanks for putting this fix together.  As we previously discussed, I do 
agree
that my naive implementation of the “loadparm” property at the top-level
CcwDevice was not satisfactory, and certainly virtio-gpu and virtio-tablet
should not have a “loadparm.”

The reason I had not yet submitted a fix is that I’ve gotten some 
feedback from
the Libvirt side that suggests the CcwDevice implementation is not 
sufficient
in general.  Libvirt will require that non-ccw devices (e.g. scsi-hd) also
support per-device loadparm.  I do not yet know how to add that type of 
support
and given that we are in hard freeze I’m not sure it is possible now.

Obviously this is not ideal, and I truly do apologize for the confusion.

I am amenable to the changes proposed in this patch if you want to 
include it
now, but I expect per-device loadparm will require further revision 
regardless.

Please let me know how you would like to proceed.  Thanks again,
 Jared Rossi