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

[Thomas Huth <thuth@redhat.com>]

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 = {