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

[Jared Rossi <jrossi@linux.ibm.com>]

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(),
>   };
>   

Thomas,

Please disregard my previous response to this patch.

While I do think per-device loadparm will need further revision, I 
believe it
will be in addition to these changes, not in place of them.

Thanks again for putting this together!

Reviewed-by: Jared Rossi <jrossi@linux.ibm.com>