Re: [PATCH] hw: Add "loadparm" property to scsi disk devices for booting on s390x

[Jared Rossi]

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)
>   
>