include/hw/qdev-properties-system.h | 3 ++ hw/core/qdev-properties-system.c | 26 +++++++++++++++++ hw/s390x/ipl.c | 19 ++++--------- hw/scsi/scsi-disk.c | 43 +++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 13 deletions(-)
While adding the new flexible boot order feature on s390x recently,
we missed to add the "loadparm" property to the scsi-hd and scsi-cd
devices. This property is required on s390x to pass the information
to the boot loader about which kernel should be started or whether
the boot menu should be shown. But even more serious: The missing
property is now causing trouble with the corresponding libvirt patches
that assume that the "loadparm" property is either settable for all
bootable devices (when the "boot order" feature is implemented in
QEMU), or none (meaning the behaviour of older QEMUs that only allowed
one "loadparm" at the machine level). To fix this broken situation,
let's implement the "loadparm" property in for the SCSI devices, too.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
v2:
- Only add the property when running with the s390x target
(checked via the arch_type variable during runtime)
- Check bootindex property before setting the loadparm property
- Call the sanitize function before setting the property, so we
can now immediately reject bad properties for the scsi devices,
too (had to move the sanitize function to the common code in
qdev-properties-system.c for this)
include/hw/qdev-properties-system.h | 3 ++
hw/core/qdev-properties-system.c | 26 +++++++++++++++++
hw/s390x/ipl.c | 19 ++++---------
hw/scsi/scsi-disk.c | 43 +++++++++++++++++++++++++++++
4 files changed, 78 insertions(+), 13 deletions(-)
diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index cdcc63056e..7ec37f6316 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -3,6 +3,9 @@
#include "hw/qdev-properties.h"
+bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
+ Error **errp);
+
extern const PropertyInfo qdev_prop_chr;
extern const PropertyInfo qdev_prop_macaddr;
extern const PropertyInfo qdev_prop_reserved_region;
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 35deef05f3..a61c5ee6dd 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -58,6 +58,32 @@ static bool check_prop_still_unset(Object *obj, const char *name,
return false;
}
+bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
+ Error **errp)
+{
+ int i, len;
+
+ len = strlen(str);
+ if (len > 8) {
+ error_setg(errp, "'loadparm' can only contain up to 8 characters");
+ return false;
+ }
+
+ for (i = 0; i < len; i++) {
+ uint8_t c = qemu_toupper(str[i]); /* mimic HMC */
+
+ if (qemu_isalnum(c) || c == '.' || c == ' ') {
+ loadparm[i] = c;
+ } else {
+ error_setg(errp,
+ "invalid character in 'loadparm': '%c' (ASCII 0x%02x)",
+ c, c);
+ return false;
+ }
+ }
+
+ return true;
+}
/* --- drive --- */
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index dc02b0fdda..30734661ad 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -418,21 +418,9 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
{
- int i;
-
/* Initialize the loadparm with spaces */
memset(loadparm, ' ', LOADPARM_LEN);
- for (i = 0; i < LOADPARM_LEN && str[i]; i++) {
- uint8_t c = qemu_toupper(str[i]); /* mimic HMC */
-
- if (qemu_isalnum(c) || c == '.' || c == ' ') {
- loadparm[i] = c;
- } else {
- error_setg(errp, "LOADPARM: invalid character '%c' (ASCII 0x%02x)",
- c, c);
- return;
- }
- }
+ qdev_prop_sanitize_s390x_loadparm(loadparm, str, errp);
}
void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
@@ -452,6 +440,7 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
SCSIDevice *sd;
int devtype;
uint8_t *lp;
+ g_autofree void *scsi_lp = NULL;
/*
* Currently allow IPL only from CCW devices.
@@ -463,6 +452,10 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
switch (devtype) {
case CCW_DEVTYPE_SCSI:
sd = SCSI_DEVICE(dev_st);
+ scsi_lp = object_property_get_str(OBJECT(sd), "loadparm", NULL);
+ if (scsi_lp && strlen(scsi_lp) > 0) {
+ lp = scsi_lp;
+ }
iplb->len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
iplb->blk0_len =
cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN - S390_IPLB_HEADER_LEN);
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index cb222da7a5..8e553487d5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -32,6 +32,7 @@
#include "migration/vmstate.h"
#include "hw/scsi/emulation.h"
#include "scsi/constants.h"
+#include "sysemu/arch_init.h"
#include "sysemu/block-backend.h"
#include "sysemu/blockdev.h"
#include "hw/block/block.h"
@@ -111,6 +112,7 @@ struct SCSIDiskState {
char *vendor;
char *product;
char *device_id;
+ char *loadparm; /* only for s390x */
bool tray_open;
bool tray_locked;
/*
@@ -3135,6 +3137,43 @@ BlockAIOCB *scsi_dma_writev(int64_t offset, QEMUIOVector *iov,
return blk_aio_pwritev(s->qdev.conf.blk, offset, iov, 0, cb, cb_opaque);
}
+static char *scsi_property_get_loadparm(Object *obj, Error **errp)
+{
+ return g_strdup(SCSI_DISK_BASE(obj)->loadparm);
+}
+
+static void scsi_property_set_loadparm(Object *obj, const char *value,
+ Error **errp)
+{
+ void *lp_str;
+
+ if (object_property_get_int(obj, "bootindex", NULL) < 0) {
+ error_setg(errp, "'loadparm' is only valid for boot devices");
+ return;
+ }
+
+ lp_str = g_malloc0(strlen(value));
+ if (!qdev_prop_sanitize_s390x_loadparm(lp_str, value, errp)) {
+ g_free(lp_str);
+ return;
+ }
+ SCSI_DISK_BASE(obj)->loadparm = lp_str;
+}
+
+static void scsi_property_add_specifics(DeviceClass *dc)
+{
+ ObjectClass *oc = OBJECT_CLASS(dc);
+
+ /* The loadparm property is only supported on s390x */
+ if (arch_type & QEMU_ARCH_S390X) {
+ object_class_property_add_str(oc, "loadparm",
+ scsi_property_get_loadparm,
+ scsi_property_set_loadparm);
+ object_class_property_set_description(oc, "loadparm",
+ "load parameter (s390x only)");
+ }
+}
+
static void scsi_disk_base_class_initfn(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -3218,6 +3257,8 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void *data)
dc->desc = "virtual SCSI disk";
device_class_set_props(dc, scsi_hd_properties);
dc->vmsd = &vmstate_scsi_disk_state;
+
+ scsi_property_add_specifics(dc);
}
static const TypeInfo scsi_hd_info = {
@@ -3258,6 +3299,8 @@ static void scsi_cd_class_initfn(ObjectClass *klass, void *data)
dc->desc = "virtual SCSI CD-ROM";
device_class_set_props(dc, scsi_cd_properties);
dc->vmsd = &vmstate_scsi_disk_state;
+
+ scsi_property_add_specifics(dc);
}
static const TypeInfo scsi_cd_info = {
--
2.47.0Looks OK to me.
Reviewed-by Jared Rossi <jrossi@linux.ibm.com>
On 11/15/24 9:12 AM, Thomas Huth wrote:
> While adding the new flexible boot order feature on s390x recently,
> we missed to add the "loadparm" property to the scsi-hd and scsi-cd
> devices. This property is required on s390x to pass the information
> to the boot loader about which kernel should be started or whether
> the boot menu should be shown. But even more serious: The missing
> property is now causing trouble with the corresponding libvirt patches
> that assume that the "loadparm" property is either settable for all
> bootable devices (when the "boot order" feature is implemented in
> QEMU), or none (meaning the behaviour of older QEMUs that only allowed
> one "loadparm" at the machine level). To fix this broken situation,
> let's implement the "loadparm" property in for the SCSI devices, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2:
> - Only add the property when running with the s390x target
> (checked via the arch_type variable during runtime)
> - Check bootindex property before setting the loadparm property
> - Call the sanitize function before setting the property, so we
> can now immediately reject bad properties for the scsi devices,
> too (had to move the sanitize function to the common code in
> qdev-properties-system.c for this)
>
> include/hw/qdev-properties-system.h | 3 ++
> hw/core/qdev-properties-system.c | 26 +++++++++++++++++
> hw/s390x/ipl.c | 19 ++++---------
> hw/scsi/scsi-disk.c | 43 +++++++++++++++++++++++++++++
> 4 files changed, 78 insertions(+), 13 deletions(-)
>
> diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
> index cdcc63056e..7ec37f6316 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -3,6 +3,9 @@
>
> #include "hw/qdev-properties.h"
>
> +bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
> + Error **errp);
> +
> extern const PropertyInfo qdev_prop_chr;
> extern const PropertyInfo qdev_prop_macaddr;
> extern const PropertyInfo qdev_prop_reserved_region;
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 35deef05f3..a61c5ee6dd 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -58,6 +58,32 @@ static bool check_prop_still_unset(Object *obj, const char *name,
> return false;
> }
>
> +bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
> + Error **errp)
> +{
> + int i, len;
> +
> + len = strlen(str);
> + if (len > 8) {
> + error_setg(errp, "'loadparm' can only contain up to 8 characters");
> + return false;
> + }
> +
> + for (i = 0; i < len; i++) {
> + uint8_t c = qemu_toupper(str[i]); /* mimic HMC */
> +
> + if (qemu_isalnum(c) || c == '.' || c == ' ') {
> + loadparm[i] = c;
> + } else {
> + error_setg(errp,
> + "invalid character in 'loadparm': '%c' (ASCII 0x%02x)",
> + c, c);
> + return false;
> + }
> + }
> +
> + return true;
> +}
>
> /* --- drive --- */
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index dc02b0fdda..30734661ad 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -418,21 +418,9 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
>
> void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> {
> - int i;
> -
> /* Initialize the loadparm with spaces */
> memset(loadparm, ' ', LOADPARM_LEN);
> - for (i = 0; i < LOADPARM_LEN && str[i]; i++) {
> - uint8_t c = qemu_toupper(str[i]); /* mimic HMC */
> -
> - if (qemu_isalnum(c) || c == '.' || c == ' ') {
> - loadparm[i] = c;
> - } else {
> - error_setg(errp, "LOADPARM: invalid character '%c' (ASCII 0x%02x)",
> - c, c);
> - return;
> - }
> - }
> + qdev_prop_sanitize_s390x_loadparm(loadparm, str, errp);
> }
>
> void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
> @@ -452,6 +440,7 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
> SCSIDevice *sd;
> int devtype;
> uint8_t *lp;
> + g_autofree void *scsi_lp = NULL;
>
> /*
> * Currently allow IPL only from CCW devices.
> @@ -463,6 +452,10 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
> switch (devtype) {
> case CCW_DEVTYPE_SCSI:
> sd = SCSI_DEVICE(dev_st);
> + scsi_lp = object_property_get_str(OBJECT(sd), "loadparm", NULL);
> + if (scsi_lp && strlen(scsi_lp) > 0) {
> + lp = scsi_lp;
> + }
> iplb->len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
> iplb->blk0_len =
> cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN - S390_IPLB_HEADER_LEN);
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index cb222da7a5..8e553487d5 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -32,6 +32,7 @@
> #include "migration/vmstate.h"
> #include "hw/scsi/emulation.h"
> #include "scsi/constants.h"
> +#include "sysemu/arch_init.h"
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "hw/block/block.h"
> @@ -111,6 +112,7 @@ struct SCSIDiskState {
> char *vendor;
> char *product;
> char *device_id;
> + char *loadparm; /* only for s390x */
> bool tray_open;
> bool tray_locked;
> /*
> @@ -3135,6 +3137,43 @@ BlockAIOCB *scsi_dma_writev(int64_t offset, QEMUIOVector *iov,
> return blk_aio_pwritev(s->qdev.conf.blk, offset, iov, 0, cb, cb_opaque);
> }
>
> +static char *scsi_property_get_loadparm(Object *obj, Error **errp)
> +{
> + return g_strdup(SCSI_DISK_BASE(obj)->loadparm);
> +}
> +
> +static void scsi_property_set_loadparm(Object *obj, const char *value,
> + Error **errp)
> +{
> + void *lp_str;
> +
> + if (object_property_get_int(obj, "bootindex", NULL) < 0) {
> + error_setg(errp, "'loadparm' is only valid for boot devices");
> + return;
> + }
> +
> + lp_str = g_malloc0(strlen(value));
> + if (!qdev_prop_sanitize_s390x_loadparm(lp_str, value, errp)) {
> + g_free(lp_str);
> + return;
> + }
> + SCSI_DISK_BASE(obj)->loadparm = lp_str;
> +}
> +
> +static void scsi_property_add_specifics(DeviceClass *dc)
> +{
> + ObjectClass *oc = OBJECT_CLASS(dc);
> +
> + /* The loadparm property is only supported on s390x */
> + if (arch_type & QEMU_ARCH_S390X) {
> + object_class_property_add_str(oc, "loadparm",
> + scsi_property_get_loadparm,
> + scsi_property_set_loadparm);
> + object_class_property_set_description(oc, "loadparm",
> + "load parameter (s390x only)");
> + }
> +}
> +
> static void scsi_disk_base_class_initfn(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -3218,6 +3257,8 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void *data)
> dc->desc = "virtual SCSI disk";
> device_class_set_props(dc, scsi_hd_properties);
> dc->vmsd = &vmstate_scsi_disk_state;
> +
> + scsi_property_add_specifics(dc);
> }
>
> static const TypeInfo scsi_hd_info = {
> @@ -3258,6 +3299,8 @@ static void scsi_cd_class_initfn(ObjectClass *klass, void *data)
> dc->desc = "virtual SCSI CD-ROM";
> device_class_set_props(dc, scsi_cd_properties);
> dc->vmsd = &vmstate_scsi_disk_state;
> +
> + scsi_property_add_specifics(dc);
> }
>
> static const TypeInfo scsi_cd_info = {
Loadparm set with boot index works properly and I confirmed the getter/setter are working as well. On 11/18/24 10:29 AM, Jared Rossi wrote: > Looks OK to me. > > Reviewed-by Jared Rossi <jrossi@linux.ibm.com> > > On 11/15/24 9:12 AM, Thomas Huth wrote: >> While adding the new flexible boot order feature on s390x recently, >> we missed to add the "loadparm" property to the scsi-hd and scsi-cd >> devices. This property is required on s390x to pass the information >> to the boot loader about which kernel should be started or whether >> the boot menu should be shown. But even more serious: The missing >> property is now causing trouble with the corresponding libvirt patches >> that assume that the "loadparm" property is either settable for all >> bootable devices (when the "boot order" feature is implemented in >> QEMU), or none (meaning the behaviour of older QEMUs that only allowed >> one "loadparm" at the machine level). To fix this broken situation, >> let's implement the "loadparm" property in for the SCSI devices, too. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> v2: >> - Only add the property when running with the s390x target >> (checked via the arch_type variable during runtime) >> - Check bootindex property before setting the loadparm property >> - Call the sanitize function before setting the property, so we >> can now immediately reject bad properties for the scsi devices, >> too (had to move the sanitize function to the common code in >> qdev-properties-system.c for this) >> >> include/hw/qdev-properties-system.h | 3 ++ >> hw/core/qdev-properties-system.c | 26 +++++++++++++++++ >> hw/s390x/ipl.c | 19 ++++--------- >> hw/scsi/scsi-disk.c | 43 +++++++++++++++++++++++++++++ >> 4 files changed, 78 insertions(+), 13 deletions(-) >> >> [snip...]
Am 18.11.24 um 16:53 schrieb Jared Rossi: > Loadparm set with boot index works properly and I confirmed the getter/setter are working as well. So this is a Tested-by: then? > > On 11/18/24 10:29 AM, Jared Rossi wrote: >> Looks OK to me. >> >> Reviewed-by Jared Rossi <jrossi@linux.ibm.com> >> >> On 11/15/24 9:12 AM, Thomas Huth wrote: >>> While adding the new flexible boot order feature on s390x recently, >>> we missed to add the "loadparm" property to the scsi-hd and scsi-cd >>> devices. This property is required on s390x to pass the information >>> to the boot loader about which kernel should be started or whether >>> the boot menu should be shown. But even more serious: The missing >>> property is now causing trouble with the corresponding libvirt patches >>> that assume that the "loadparm" property is either settable for all >>> bootable devices (when the "boot order" feature is implemented in >>> QEMU), or none (meaning the behaviour of older QEMUs that only allowed >>> one "loadparm" at the machine level). To fix this broken situation, >>> let's implement the "loadparm" property in for the SCSI devices, too. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> v2: >>> - Only add the property when running with the s390x target >>> (checked via the arch_type variable during runtime) >>> - Check bootindex property before setting the loadparm property >>> - Call the sanitize function before setting the property, so we >>> can now immediately reject bad properties for the scsi devices, >>> too (had to move the sanitize function to the common code in >>> qdev-properties-system.c for this) >>> >>> include/hw/qdev-properties-system.h | 3 ++ >>> hw/core/qdev-properties-system.c | 26 +++++++++++++++++ >>> hw/s390x/ipl.c | 19 ++++--------- >>> hw/scsi/scsi-disk.c | 43 +++++++++++++++++++++++++++++ >>> 4 files changed, 78 insertions(+), 13 deletions(-) >>> >>> [snip...] >
On 11/18/24 12:12 PM, Christian Borntraeger wrote: > Am 18.11.24 um 16:53 schrieb Jared Rossi: >> Loadparm set with boot index works properly and I confirmed the >> getter/setter are working as well. > > So this is a Tested-by: then? Yes. Tested-by Jared Rossi <jrossi@linux.ibm.com> Reviewed-by Jared Rossi <jrossi@linux.ibm.com> >> >> On 11/18/24 10:29 AM, Jared Rossi wrote: >>> Looks OK to me. >>> >>> Reviewed-by Jared Rossi <jrossi@linux.ibm.com> >>> >>> On 11/15/24 9:12 AM, Thomas Huth wrote: >>>> While adding the new flexible boot order feature on s390x recently, >>>> we missed to add the "loadparm" property to the scsi-hd and scsi-cd >>>> devices. This property is required on s390x to pass the information >>>> to the boot loader about which kernel should be started or whether >>>> the boot menu should be shown. But even more serious: The missing >>>> property is now causing trouble with the corresponding libvirt patches >>>> that assume that the "loadparm" property is either settable for all >>>> bootable devices (when the "boot order" feature is implemented in >>>> QEMU), or none (meaning the behaviour of older QEMUs that only allowed >>>> one "loadparm" at the machine level). To fix this broken situation, >>>> let's implement the "loadparm" property in for the SCSI devices, too. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> v2: >>>> - Only add the property when running with the s390x target >>>> (checked via the arch_type variable during runtime) >>>> - Check bootindex property before setting the loadparm property >>>> - Call the sanitize function before setting the property, so we >>>> can now immediately reject bad properties for the scsi devices, >>>> too (had to move the sanitize function to the common code in >>>> qdev-properties-system.c for this) >>>> >>>> include/hw/qdev-properties-system.h | 3 ++ >>>> hw/core/qdev-properties-system.c | 26 +++++++++++++++++ >>>> hw/s390x/ipl.c | 19 ++++--------- >>>> hw/scsi/scsi-disk.c | 43 >>>> +++++++++++++++++++++++++++++ >>>> 4 files changed, 78 insertions(+), 13 deletions(-) >>>> >>>> [snip...] >>
On 15/11/2024 15.12, Thomas Huth wrote:
> While adding the new flexible boot order feature on s390x recently,
> we missed to add the "loadparm" property to the scsi-hd and scsi-cd
> devices. This property is required on s390x to pass the information
> to the boot loader about which kernel should be started or whether
> the boot menu should be shown. But even more serious: The missing
> property is now causing trouble with the corresponding libvirt patches
> that assume that the "loadparm" property is either settable for all
> bootable devices (when the "boot order" feature is implemented in
> QEMU), or none (meaning the behaviour of older QEMUs that only allowed
> one "loadparm" at the machine level). To fix this broken situation,
> let's implement the "loadparm" property in for the SCSI devices, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2:
> - Only add the property when running with the s390x target
> (checked via the arch_type variable during runtime)
> - Check bootindex property before setting the loadparm property
> - Call the sanitize function before setting the property, so we
> can now immediately reject bad properties for the scsi devices,
> too (had to move the sanitize function to the common code in
> qdev-properties-system.c for this)
>
> include/hw/qdev-properties-system.h | 3 ++
> hw/core/qdev-properties-system.c | 26 +++++++++++++++++
> hw/s390x/ipl.c | 19 ++++---------
> hw/scsi/scsi-disk.c | 43 +++++++++++++++++++++++++++++
> 4 files changed, 78 insertions(+), 13 deletions(-)
If there are no objections, I'll pick this up for my pull request for QEMU
9.2-rc1.
Thomas
> diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
> index cdcc63056e..7ec37f6316 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -3,6 +3,9 @@
>
> #include "hw/qdev-properties.h"
>
> +bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
> + Error **errp);
> +
> extern const PropertyInfo qdev_prop_chr;
> extern const PropertyInfo qdev_prop_macaddr;
> extern const PropertyInfo qdev_prop_reserved_region;
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 35deef05f3..a61c5ee6dd 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -58,6 +58,32 @@ static bool check_prop_still_unset(Object *obj, const char *name,
> return false;
> }
>
> +bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
> + Error **errp)
> +{
> + int i, len;
> +
> + len = strlen(str);
> + if (len > 8) {
> + error_setg(errp, "'loadparm' can only contain up to 8 characters");
> + return false;
> + }
> +
> + for (i = 0; i < len; i++) {
> + uint8_t c = qemu_toupper(str[i]); /* mimic HMC */
> +
> + if (qemu_isalnum(c) || c == '.' || c == ' ') {
> + loadparm[i] = c;
> + } else {
> + error_setg(errp,
> + "invalid character in 'loadparm': '%c' (ASCII 0x%02x)",
> + c, c);
> + return false;
> + }
> + }
> +
> + return true;
> +}
>
> /* --- drive --- */
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index dc02b0fdda..30734661ad 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -418,21 +418,9 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
>
> void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> {
> - int i;
> -
> /* Initialize the loadparm with spaces */
> memset(loadparm, ' ', LOADPARM_LEN);
> - for (i = 0; i < LOADPARM_LEN && str[i]; i++) {
> - uint8_t c = qemu_toupper(str[i]); /* mimic HMC */
> -
> - if (qemu_isalnum(c) || c == '.' || c == ' ') {
> - loadparm[i] = c;
> - } else {
> - error_setg(errp, "LOADPARM: invalid character '%c' (ASCII 0x%02x)",
> - c, c);
> - return;
> - }
> - }
> + qdev_prop_sanitize_s390x_loadparm(loadparm, str, errp);
> }
>
> void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
> @@ -452,6 +440,7 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
> SCSIDevice *sd;
> int devtype;
> uint8_t *lp;
> + g_autofree void *scsi_lp = NULL;
>
> /*
> * Currently allow IPL only from CCW devices.
> @@ -463,6 +452,10 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
> switch (devtype) {
> case CCW_DEVTYPE_SCSI:
> sd = SCSI_DEVICE(dev_st);
> + scsi_lp = object_property_get_str(OBJECT(sd), "loadparm", NULL);
> + if (scsi_lp && strlen(scsi_lp) > 0) {
> + lp = scsi_lp;
> + }
> iplb->len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
> iplb->blk0_len =
> cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN - S390_IPLB_HEADER_LEN);
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index cb222da7a5..8e553487d5 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -32,6 +32,7 @@
> #include "migration/vmstate.h"
> #include "hw/scsi/emulation.h"
> #include "scsi/constants.h"
> +#include "sysemu/arch_init.h"
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "hw/block/block.h"
> @@ -111,6 +112,7 @@ struct SCSIDiskState {
> char *vendor;
> char *product;
> char *device_id;
> + char *loadparm; /* only for s390x */
> bool tray_open;
> bool tray_locked;
> /*
> @@ -3135,6 +3137,43 @@ BlockAIOCB *scsi_dma_writev(int64_t offset, QEMUIOVector *iov,
> return blk_aio_pwritev(s->qdev.conf.blk, offset, iov, 0, cb, cb_opaque);
> }
>
> +static char *scsi_property_get_loadparm(Object *obj, Error **errp)
> +{
> + return g_strdup(SCSI_DISK_BASE(obj)->loadparm);
> +}
> +
> +static void scsi_property_set_loadparm(Object *obj, const char *value,
> + Error **errp)
> +{
> + void *lp_str;
> +
> + if (object_property_get_int(obj, "bootindex", NULL) < 0) {
> + error_setg(errp, "'loadparm' is only valid for boot devices");
> + return;
> + }
> +
> + lp_str = g_malloc0(strlen(value));
> + if (!qdev_prop_sanitize_s390x_loadparm(lp_str, value, errp)) {
> + g_free(lp_str);
> + return;
> + }
> + SCSI_DISK_BASE(obj)->loadparm = lp_str;
> +}
> +
> +static void scsi_property_add_specifics(DeviceClass *dc)
> +{
> + ObjectClass *oc = OBJECT_CLASS(dc);
> +
> + /* The loadparm property is only supported on s390x */
> + if (arch_type & QEMU_ARCH_S390X) {
> + object_class_property_add_str(oc, "loadparm",
> + scsi_property_get_loadparm,
> + scsi_property_set_loadparm);
> + object_class_property_set_description(oc, "loadparm",
> + "load parameter (s390x only)");
> + }
> +}
> +
> static void scsi_disk_base_class_initfn(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -3218,6 +3257,8 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void *data)
> dc->desc = "virtual SCSI disk";
> device_class_set_props(dc, scsi_hd_properties);
> dc->vmsd = &vmstate_scsi_disk_state;
> +
> + scsi_property_add_specifics(dc);
> }
>
> static const TypeInfo scsi_hd_info = {
> @@ -3258,6 +3299,8 @@ static void scsi_cd_class_initfn(ObjectClass *klass, void *data)
> dc->desc = "virtual SCSI CD-ROM";
> device_class_set_props(dc, scsi_cd_properties);
> dc->vmsd = &vmstate_scsi_disk_state;
> +
> + scsi_property_add_specifics(dc);
> }
>
> static const TypeInfo scsi_cd_info = {
On Mon, 2024-11-18 at 11:02 +0100, Thomas Huth wrote: > On 15/11/2024 15.12, Thomas Huth wrote: > > While adding the new flexible boot order feature on s390x recently, > > we missed to add the "loadparm" property to the scsi-hd and scsi-cd > > devices. This property is required on s390x to pass the information > > to the boot loader about which kernel should be started or whether > > the boot menu should be shown. But even more serious: The missing > > property is now causing trouble with the corresponding libvirt patches > > that assume that the "loadparm" property is either settable for all > > bootable devices (when the "boot order" feature is implemented in > > QEMU), or none (meaning the behaviour of older QEMUs that only allowed > > one "loadparm" at the machine level). To fix this broken situation, > > let's implement the "loadparm" property in for the SCSI devices, too. > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > v2: > > - Only add the property when running with the s390x target > > (checked via the arch_type variable during runtime) > > - Check bootindex property before setting the loadparm property > > - Call the sanitize function before setting the property, so we > > can now immediately reject bad properties for the scsi devices, > > too (had to move the sanitize function to the common code in > > qdev-properties-system.c for this) > > > > include/hw/qdev-properties-system.h | 3 ++ > > hw/core/qdev-properties-system.c | 26 +++++++++++++++++ > > hw/s390x/ipl.c | 19 ++++--------- > > hw/scsi/scsi-disk.c | 43 +++++++++++++++++++++++++++++ > > 4 files changed, 78 insertions(+), 13 deletions(-) > > If there are no objections, I'll pick this up for my pull request for QEMU > 9.2-rc1. Hi Thomas, Jared was out Friday so I'm hoping he can give it a look over today, but no objections from me: Acked-by: Eric Farman <farman@linux.ibm.com>
© 2016 - 2024 Red Hat, Inc.