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

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

[Thomas Huth]

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

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

[Philippe Mathieu-Daudé]

On 13/11/24 11:47, 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

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

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>

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

[Thomas Huth]

On 13/11/2024 15.49, Jared Rossi wrote:
...
> 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.

  Hi Jared,

yes, that scsi-hd problem is a little bit tricky, since it's common code 
that we should not "pollute" too much with s390x specific stuff, especially 
since we're in hard freeze now.

After staring at the code for quite a while, I think one option might be to 
just add a string "loadparm" property to the SCSI devices, that's just a 
simple two-line change to the common code. Patch suggestion can be found here:

  https://lore.kernel.org/qemu-devel/20241114122919.973930-1-thuth@redhat.com/

The only disadvantage is that this is now checking for valid characters in 
the string after the property has already been set, so it cannot prevent the 
setting of bad characters. But it still prints out an error message later, 
so I hope that is also still acceptable.

Let me know what you think about it!

  Thanks,
   Thomas

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

[Jared Rossi]

On 11/14/24 7:39 AM, Thomas Huth wrote:
> On 13/11/2024 15.49, Jared Rossi wrote:
> ...
>> 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.
>
>  Hi Jared,
>
> yes, that scsi-hd problem is a little bit tricky, since it's common 
> code that we should not "pollute" too much with s390x specific stuff, 
> especially since we're in hard freeze now.
>
> After staring at the code for quite a while, I think one option might 
> be to just add a string "loadparm" property to the SCSI devices, 
> that's just a simple two-line change to the common code. Patch 
> suggestion can be found here:
>
>  https://lore.kernel.org/qemu-devel/20241114122919.973930-1-thuth@redhat.com/ 
>
>
> The only disadvantage is that this is now checking for valid 
> characters in the string after the property has already been set, so 
> it cannot prevent the setting of bad characters. But it still prints 
> out an error message later, so I hope that is also still acceptable.
>
> Let me know what you think about it!
>
>  Thanks,
>   Thomas
>
Thanks Thomas,

I replied to the patch as well, but the setter does other validation 
beyond just the characters.  In particular it ensures a loadparm value 
is assigned only to boot devices.  This has significance when probing 
for devices, since the per-device loadparm value is stored in the IPLB, 
and an IPLB is only generated for devices with an assigned boot index.

I would say the scsi loadparm probing issue is actually a legitimate 
concern since it can result in unexpected behavior with which kernel is 
selected, but probably we want to avoid adding too much s390x specific 
validation to the common code, so I'm not sure what the best solution 
is.  Maybe there is a better way to restrict the loadparm to boot 
devices that doesn't need the property setter?

On a side note, while testing your patch I also realized that the 
machine loadparm was being ignored during probing, so I've sent out a 
patch to fix that...

Thanks again for all the help with this,
     Jared Rossi