| Message ID | 20241113114741.681096-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/s390x: Restrict "loadparm" property to devices that can be used for booting | expand |
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>
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
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>
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
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
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(), };
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(-)