hw/s390x/ipl.c | 22 +++++++++++++++++++--- hw/scsi/scsi-disk.c | 2 ++ 2 files changed, 21 insertions(+), 3 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 for the SCSI devices, too.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
NB: Unlike the ccw_device_set_loadparm() logic that we use for CCW
devices, I've decided to use a string property for the "loadparm"
in the SCSI devices to avoid spoiling the common code with too much
s390x logic. So the check for valid characters is now done after the
property has been set, i.e. we only print out an error instead of
forbidding the setting (like we do it with the CCW devices), which
is IMHO still perfectly acceptable. Or are there other opinions?
hw/s390x/ipl.c | 22 +++++++++++++++++++---
hw/scsi/scsi-disk.c | 2 ++
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index dc02b0fdda..c902b562cb 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -416,12 +416,10 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
return chain_addr;
}
-void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
+static void s390_sanitize_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 */
@@ -435,6 +433,13 @@ void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
}
}
+void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
+{
+ /* Initialize the loadparm with spaces */
+ memset(loadparm, ' ', LOADPARM_LEN);
+ s390_sanitize_loadparm(loadparm, str, errp);
+}
+
void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
{
int i;
@@ -452,6 +457,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 +469,16 @@ 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) {
+ Error *errp = NULL;
+ s390_sanitize_loadparm(scsi_lp, scsi_lp, &errp);
+ if (errp) {
+ error_report_err(errp);
+ } else {
+ 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..c1fa02883d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -111,6 +111,7 @@ struct SCSIDiskState {
char *vendor;
char *product;
char *device_id;
+ char *loadparm;
bool tray_open;
bool tray_locked;
/*
@@ -3165,6 +3166,7 @@ static const TypeInfo scsi_disk_base_info = {
DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor), \
DEFINE_PROP_STRING("product", SCSIDiskState, product), \
DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id), \
+ DEFINE_PROP_STRING("loadparm", SCSIDiskState, loadparm), \
DEFINE_PROP_BOOL("migrate-emulated-scsi-request", SCSIDiskState, migrate_emulated_scsi_request, true)
--
2.47.0With documentation of per-device loadparm behavior during device probing
similar to suggestion here:
https://lore.kernel.org/qemu-devel/20241115002742.3576842-1-jrossi@linux.ibm.com/
Reviewed-by: Jared Rossi <jrossi@linux.ibm.com>
On 11/14/24 7:29 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 for the SCSI devices, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> NB: Unlike the ccw_device_set_loadparm() logic that we use for CCW
> devices, I've decided to use a string property for the "loadparm"
> in the SCSI devices to avoid spoiling the common code with too much
> s390x logic. So the check for valid characters is now done after the
> property has been set, i.e. we only print out an error instead of
> forbidding the setting (like we do it with the CCW devices), which
> is IMHO still perfectly acceptable. Or are there other opinions?
>
> hw/s390x/ipl.c | 22 +++++++++++++++++++---
> hw/scsi/scsi-disk.c | 2 ++
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index dc02b0fdda..c902b562cb 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -416,12 +416,10 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
> return chain_addr;
> }
>
> -void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> +static void s390_sanitize_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 */
>
> @@ -435,6 +433,13 @@ void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> }
> }
>
> +void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> +{
> + /* Initialize the loadparm with spaces */
> + memset(loadparm, ' ', LOADPARM_LEN);
> + s390_sanitize_loadparm(loadparm, str, errp);
> +}
> +
> void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
> {
> int i;
> @@ -452,6 +457,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 +469,16 @@ 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) {
> + Error *errp = NULL;
> + s390_sanitize_loadparm(scsi_lp, scsi_lp, &errp);
> + if (errp) {
> + error_report_err(errp);
> + } else {
> + 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..c1fa02883d 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -111,6 +111,7 @@ struct SCSIDiskState {
> char *vendor;
> char *product;
> char *device_id;
> + char *loadparm;
> bool tray_open;
> bool tray_locked;
> /*
> @@ -3165,6 +3166,7 @@ static const TypeInfo scsi_disk_base_info = {
> DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor), \
> DEFINE_PROP_STRING("product", SCSIDiskState, product), \
> DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id), \
> + DEFINE_PROP_STRING("loadparm", SCSIDiskState, loadparm), \
> DEFINE_PROP_BOOL("migrate-emulated-scsi-request", SCSIDiskState, migrate_emulated_scsi_request, true)
>
>
On 11/14/24 7:29 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 for the SCSI devices, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> NB: Unlike the ccw_device_set_loadparm() logic that we use for CCW
> devices, I've decided to use a string property for the "loadparm"
> in the SCSI devices to avoid spoiling the common code with too much
> s390x logic. So the check for valid characters is now done after the
> property has been set, i.e. we only print out an error instead of
> forbidding the setting (like we do it with the CCW devices), which
> is IMHO still perfectly acceptable. Or are there other opinions?
I wasn't able to think of a way to abuse passing invalid characters, but
I did
find two additional differences about the string approach:
a) it is not possible to override the machine loadparm by assigning an empty
string (loadparm="") to the device
b) it is possible to assign a loadparm value to a non-boot device
I don't think that the inability to pass the empty string is a significant
problem because passing a single space will have the same effect.
Assigning a loadparm to a non-boot device generally does nothing, but in the
case of device probing (i.e. no boot devices assigned at all), the
device with
the loadparm assigned could be selected for IPL, but it will not use the
assigned loadparm (because no IPLB was generated for the device). This
check is
normally handled by ccw_device_set_loadparm(), but I'm not sure if there
is a
way to do the validation without having a setter function for the property.
When both a bootindex and a loadparm are assigned it seems to work
correctly.
I would like to get other's opinions on if the two issues I pointed out
are actually significant enough to justify a more robust implementation,
since
I think it would require more s390x specific modifications to the common
code.
>
> hw/s390x/ipl.c | 22 +++++++++++++++++++---
> hw/scsi/scsi-disk.c | 2 ++
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index dc02b0fdda..c902b562cb 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -416,12 +416,10 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
> return chain_addr;
> }
>
> -void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> +static void s390_sanitize_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 */
>
> @@ -435,6 +433,13 @@ void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> }
> }
>
> +void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> +{
> + /* Initialize the loadparm with spaces */
> + memset(loadparm, ' ', LOADPARM_LEN);
> + s390_sanitize_loadparm(loadparm, str, errp);
> +}
> +
> void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
> {
> int i;
> @@ -452,6 +457,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 +469,16 @@ 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) {
> + Error *errp = NULL;
> + s390_sanitize_loadparm(scsi_lp, scsi_lp, &errp);
> + if (errp) {
> + error_report_err(errp);
> + } else {
> + 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..c1fa02883d 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -111,6 +111,7 @@ struct SCSIDiskState {
> char *vendor;
> char *product;
> char *device_id;
> + char *loadparm;
> bool tray_open;
> bool tray_locked;
> /*
> @@ -3165,6 +3166,7 @@ static const TypeInfo scsi_disk_base_info = {
> DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor), \
> DEFINE_PROP_STRING("product", SCSIDiskState, product), \
> DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id), \
> + DEFINE_PROP_STRING("loadparm", SCSIDiskState, loadparm), \
> DEFINE_PROP_BOOL("migrate-emulated-scsi-request", SCSIDiskState, migrate_emulated_scsi_request, true)
>
>
On 14/11/2024 16.55, Jared Rossi wrote: > > > On 11/14/24 7:29 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 for the SCSI devices, too. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> NB: Unlike the ccw_device_set_loadparm() logic that we use for CCW >> devices, I've decided to use a string property for the "loadparm" >> in the SCSI devices to avoid spoiling the common code with too much >> s390x logic. So the check for valid characters is now done after the >> property has been set, i.e. we only print out an error instead of >> forbidding the setting (like we do it with the CCW devices), which >> is IMHO still perfectly acceptable. Or are there other opinions? > > I wasn't able to think of a way to abuse passing invalid characters, but I did > find two additional differences about the string approach: > > a) it is not possible to override the machine loadparm by assigning an empty > string (loadparm="") to the device Agreed, that's a (small) problem. There does not seem to be a way to distinguish between "property has not been set" and "property has been set to a string with zero length" with object_property_get_str() ... > I don't think that the inability to pass the empty string is a significant > problem because passing a single space will have the same effect. That sounds like a good work-around, indeed. > b) it is possible to assign a loadparm value to a non-boot device > > Assigning a loadparm to a non-boot device generally does nothing, but in the > case of device probing (i.e. no boot devices assigned at all), the device with > the loadparm assigned could be selected for IPL, but it will not use the > assigned loadparm (because no IPLB was generated for the device). This check is > normally handled by ccw_device_set_loadparm(), but I'm not sure if there is a > way to do the validation without having a setter function for the property. Hmmm, that could be confusing for the users, indeed. But maybe it would be sufficient to properly document that loadparm is only working for devices with a bootindex? What do you think? By the way, the loadparm section in docs/system/s390x/bootdevices.rst looks like it should get an update, too ... if you have some spare minutes, could you maybe look at it? Thanks, Thomas
On 11/14/24 12:47 PM, Thomas Huth wrote: > On 14/11/2024 16.55, Jared Rossi wrote: >> >> >> On 11/14/24 7:29 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 for the SCSI devices, too. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> NB: Unlike the ccw_device_set_loadparm() logic that we use for CCW >>> devices, I've decided to use a string property for the "loadparm" >>> in the SCSI devices to avoid spoiling the common code with too >>> much >>> s390x logic. So the check for valid characters is now done >>> after the >>> property has been set, i.e. we only print out an error instead of >>> forbidding the setting (like we do it with the CCW devices), >>> which >>> is IMHO still perfectly acceptable. Or are there other opinions? >> >> I wasn't able to think of a way to abuse passing invalid characters, >> but I did >> find two additional differences about the string approach: >> >> a) it is not possible to override the machine loadparm by assigning >> an empty >> string (loadparm="") to the device > > Agreed, that's a (small) problem. There does not seem to be a way to > distinguish between "property has not been set" and "property has been > set to a string with zero length" with object_property_get_str() ... > >> I don't think that the inability to pass the empty string is a >> significant >> problem because passing a single space will have the same effect. > > That sounds like a good work-around, indeed. > > > b) it is possible to assign a loadparm value to a non-boot device > > >> Assigning a loadparm to a non-boot device generally does nothing, but >> in the >> case of device probing (i.e. no boot devices assigned at all), the >> device with >> the loadparm assigned could be selected for IPL, but it will not use the >> assigned loadparm (because no IPLB was generated for the device). >> This check is >> normally handled by ccw_device_set_loadparm(), but I'm not sure if >> there is a >> way to do the validation without having a setter function for the >> property. > > Hmmm, that could be confusing for the users, indeed. But maybe it > would be sufficient > to properly document that loadparm is only working for devices with a > bootindex? > What do you think? > > By the way, the loadparm section in docs/system/s390x/bootdevices.rst > looks like it should get an update, too ... if you have some spare > minutes, could you maybe look at it? > Yes, I suppose I would agree that documenting the behavior is sufficient in this case. I will update bootdevices.rst to include per-device loadparm support and also indicate that that per-device loadparm values are only used for devices that have an assigned bootindex. Thanks, Jared Rossi
On 15/11/2024 00.30, Jared Rossi wrote: > > > On 11/14/24 12:47 PM, Thomas Huth wrote: >> On 14/11/2024 16.55, Jared Rossi wrote: >>> >>> >>> On 11/14/24 7:29 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 for the SCSI devices, too. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> NB: Unlike the ccw_device_set_loadparm() logic that we use for CCW >>>> devices, I've decided to use a string property for the "loadparm" >>>> in the SCSI devices to avoid spoiling the common code with too much >>>> s390x logic. So the check for valid characters is now done after the >>>> property has been set, i.e. we only print out an error instead of >>>> forbidding the setting (like we do it with the CCW devices), which >>>> is IMHO still perfectly acceptable. Or are there other opinions? >>> >>> I wasn't able to think of a way to abuse passing invalid characters, but >>> I did >>> find two additional differences about the string approach: >>> >>> a) it is not possible to override the machine loadparm by assigning an empty >>> string (loadparm="") to the device >> >> Agreed, that's a (small) problem. There does not seem to be a way to >> distinguish between "property has not been set" and "property has been set >> to a string with zero length" with object_property_get_str() ... >> >>> I don't think that the inability to pass the empty string is a significant >>> problem because passing a single space will have the same effect. >> >> That sounds like a good work-around, indeed. >> >> > b) it is possible to assign a loadparm value to a non-boot device >> > >>> Assigning a loadparm to a non-boot device generally does nothing, but in the >>> case of device probing (i.e. no boot devices assigned at all), the device >>> with >>> the loadparm assigned could be selected for IPL, but it will not use the >>> assigned loadparm (because no IPLB was generated for the device). This >>> check is >>> normally handled by ccw_device_set_loadparm(), but I'm not sure if there >>> is a >>> way to do the validation without having a setter function for the property. >> >> Hmmm, that could be confusing for the users, indeed. But maybe it would be >> sufficient >> to properly document that loadparm is only working for devices with a >> bootindex? >> What do you think? >> >> By the way, the loadparm section in docs/system/s390x/bootdevices.rst >> looks like it should get an update, too ... if you have some spare >> minutes, could you maybe look at it? >> > > Yes, I suppose I would agree that documenting the behavior is sufficient in > this case. I will update bootdevices.rst to include per-device loadparm > support and also indicate that that per-device loadparm values are only used > for devices that have an assigned bootindex. After switching to object_class_property_add_str() in v2 (to be able to only add the property when running with the s390x target), I had to add getter and setter function anyway, so I now also added the check for the bootindex property there: https://lore.kernel.org/qemu-devel/20241115141202.1877294-1-thuth@redhat.com/ I hope that should solve now most issues. Thomas
On 15/11/2024 00.30, Jared Rossi wrote: > > > On 11/14/24 12:47 PM, Thomas Huth wrote: >> On 14/11/2024 16.55, Jared Rossi wrote: >>> >>> >>> On 11/14/24 7:29 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 for the SCSI devices, too. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> NB: Unlike the ccw_device_set_loadparm() logic that we use for CCW >>>> devices, I've decided to use a string property for the "loadparm" >>>> in the SCSI devices to avoid spoiling the common code with too much >>>> s390x logic. So the check for valid characters is now done after the >>>> property has been set, i.e. we only print out an error instead of >>>> forbidding the setting (like we do it with the CCW devices), which >>>> is IMHO still perfectly acceptable. Or are there other opinions? >>> >>> I wasn't able to think of a way to abuse passing invalid characters, but >>> I did >>> find two additional differences about the string approach: >>> >>> a) it is not possible to override the machine loadparm by assigning an empty >>> string (loadparm="") to the device >> >> Agreed, that's a (small) problem. There does not seem to be a way to >> distinguish between "property has not been set" and "property has been set >> to a string with zero length" with object_property_get_str() ... >> >>> I don't think that the inability to pass the empty string is a significant >>> problem because passing a single space will have the same effect. >> >> That sounds like a good work-around, indeed. >> >> > b) it is possible to assign a loadparm value to a non-boot device >> > >>> Assigning a loadparm to a non-boot device generally does nothing, but in the >>> case of device probing (i.e. no boot devices assigned at all), the device >>> with >>> the loadparm assigned could be selected for IPL, but it will not use the >>> assigned loadparm (because no IPLB was generated for the device). This >>> check is >>> normally handled by ccw_device_set_loadparm(), but I'm not sure if there >>> is a >>> way to do the validation without having a setter function for the property. >> >> Hmmm, that could be confusing for the users, indeed. But maybe it would be >> sufficient >> to properly document that loadparm is only working for devices with a >> bootindex? >> What do you think? >> >> By the way, the loadparm section in docs/system/s390x/bootdevices.rst >> looks like it should get an update, too ... if you have some spare >> minutes, could you maybe look at it? >> > > Yes, I suppose I would agree that documenting the behavior is sufficient in > this case. I will update bootdevices.rst to include per-device loadparm > support and also indicate that that per-device loadparm values are only used > for devices that have an assigned bootindex. Ok, great! Sounds like we have a passable solution for QEMU 9.2. We still can refine the loadparm handling of the scsi devices in future QEMU releases, but this will now at least solve the present problem that Boris reported from the libvirt side. Thomas
On Fri, Nov 15, 2024 at 7:30 AM Thomas Huth <thuth@redhat.com> wrote: > Ok, great! Sounds like we have a passable solution for QEMU 9.2. We still > can refine the loadparm handling of the scsi devices in future QEMU > releases, but this will now at least solve the present problem that Boris > reported from the libvirt side. Please put in the release notes that in the future loadparm might be limited to s390 emulators... Not sure how, but we'll come up with something. Paolo
On 15/11/2024 12.49, Paolo Bonzini wrote:
> On Fri, Nov 15, 2024 at 7:30 AM Thomas Huth <thuth@redhat.com> wrote:
>> Ok, great! Sounds like we have a passable solution for QEMU 9.2. We still
>> can refine the loadparm handling of the scsi devices in future QEMU
>> releases, but this will now at least solve the present problem that Boris
>> reported from the libvirt side.
>
> Please put in the release notes that in the future loadparm might be
> limited to s390 emulators... Not sure how, but we'll come up with
> something.
Sure, will do!
Alternatively, would you rather prefer something like this for the common
scsi code:
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index c1fa02883d..5c9362fa83 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"
@@ -3136,6 +3137,31 @@ 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)
+{
+ SCSI_DISK_BASE(obj)->loadparm = g_strdup(value);
+}
+
+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);
@@ -3166,7 +3192,6 @@ static const TypeInfo scsi_disk_base_info = {
DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor), \
DEFINE_PROP_STRING("product", SCSIDiskState, product), \
DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id), \
- DEFINE_PROP_STRING("loadparm", SCSIDiskState, loadparm), \
DEFINE_PROP_BOOL("migrate-emulated-scsi-request", SCSIDiskState, migrate_emulated_scsi_request, true)
@@ -3220,6 +3245,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 = {
@@ -3260,6 +3287,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 = {
?
Using the global arch_type variable is likely also not the
nicest solution, but it at least limits the property to the
s390x target for now...
Thomas
On 11/15/24 13:26, Thomas Huth wrote:
> On 15/11/2024 12.49, Paolo Bonzini wrote:
>> On Fri, Nov 15, 2024 at 7:30 AM Thomas Huth <thuth@redhat.com> wrote:
>>> Ok, great! Sounds like we have a passable solution for QEMU 9.2. We
>>> still
>>> can refine the loadparm handling of the scsi devices in future QEMU
>>> releases, but this will now at least solve the present problem that
>>> Boris
>>> reported from the libvirt side.
>>
>> Please put in the release notes that in the future loadparm might be
>> limited to s390 emulators... Not sure how, but we'll come up with
>> something.
>
> Sure, will do!
>
> Alternatively, would you rather prefer something like this for the common
> scsi code:
Sure! I completely forgot about arch_type.
Paolo
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index c1fa02883d..5c9362fa83 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"
> @@ -3136,6 +3137,31 @@ 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)
> +{
> + SCSI_DISK_BASE(obj)->loadparm = g_strdup(value);
> +}
> +
> +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);
> @@ -3166,7 +3192,6 @@ static const TypeInfo scsi_disk_base_info = {
> DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor), \
> DEFINE_PROP_STRING("product", SCSIDiskState, product), \
> DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id), \
> - DEFINE_PROP_STRING("loadparm", SCSIDiskState, loadparm), \
> DEFINE_PROP_BOOL("migrate-emulated-scsi-request", SCSIDiskState,
> migrate_emulated_scsi_request, true)
>
>
> @@ -3220,6 +3245,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 = {
> @@ -3260,6 +3287,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 = {
>
> ?
>
> Using the global arch_type variable is likely also not the
> nicest solution, but it at least limits the property to the
> s390x target for now...
>
> Thomas
>
>
>
© 2016 - 2024 Red Hat, Inc.