Re: [PATCH v5 14/19] s390x: Add individual loadparm assignment to CCW device

[Thomas Huth <thuth@redhat.com>]

On 20/10/2024 03.29, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Add a loadparm property to the VirtioCcwDevice object so that different
> loadparms can be defined on a per-device basis for CCW boot devices.
> 
> The machine/global loadparm is still supported. If both a global and per-device
> loadparm are defined, the per-device value will override the global value for
> that device, but any other devices that do not specify a per-device loadparm
> will still use the global loadparm.
> 
> It is invalid to assign a loadparm to a non-boot device.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   hw/s390x/ccw-device.h       |  2 ++
>   hw/s390x/ipl.h              |  3 +-
>   include/hw/s390x/ipl/qipl.h |  1 +
>   hw/s390x/ccw-device.c       | 46 +++++++++++++++++++++++++
>   hw/s390x/ipl.c              | 68 ++++++++++++++++++++++---------------
>   hw/s390x/s390-virtio-ccw.c  | 18 +---------
>   hw/s390x/sclp.c             |  9 ++---
>   pc-bios/s390-ccw/main.c     | 10 ++++--
>   8 files changed, 102 insertions(+), 55 deletions(-)
> 
> diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
> index 5feeb0ee7a..1e1737c0f3 100644
> --- a/hw/s390x/ccw-device.h
> +++ b/hw/s390x/ccw-device.h
> @@ -26,6 +26,8 @@ struct CcwDevice {
>       CssDevId dev_id;
>       /* The actual busid of the virtual subchannel. */
>       CssDevId subch_id;
> +    /* If set, use this loadparm value when device is boot target */
> +    uint8_t loadparm[8];
>   };
>   typedef struct CcwDevice CcwDevice;
>   
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index fa394c339d..b670bad551 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -21,7 +21,8 @@
>   
>   #define DIAG308_FLAGS_LP_VALID 0x80
>   
> -int s390_ipl_set_loadparm(uint8_t *loadparm);
> +void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
> +void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
>   void s390_ipl_update_diag308(IplParameterBlock *iplb);
>   int s390_ipl_prepare_pv_header(Error **errp);
>   int s390_ipl_pv_unpack(void);
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index 0ef04af027..b67d2ae061 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -18,6 +18,7 @@
>   
>   #define QIPL_ADDRESS  0xcc
>   #define LOADPARM_LEN    8
> +#define NO_LOADPARM "\0\0\0\0\0\0\0\0"
>   
>   /*
>    * The QEMU IPL Parameters will be stored at absolute address
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index 14c24e3890..230cc09e03 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -13,6 +13,10 @@
>   #include "ccw-device.h"
>   #include "hw/qdev-properties.h"
>   #include "qemu/module.h"
> +#include "ipl.h"
> +#include "qapi/visitor.h"
> +#include "qemu/ctype.h"
> +#include "qapi/error.h"
>   
>   static void ccw_device_refill_ids(CcwDevice *dev)
>   {
> @@ -37,10 +41,52 @@ static bool ccw_device_realize(CcwDevice *dev, Error **errp)
>       return true;
>   }
>   
> +static void ccw_device_get_loadparm(Object *obj, Visitor *v,
> +                                 const char *name, void *opaque,
> +                                 Error **errp)
> +{
> +    CcwDevice *dev = CCW_DEVICE(obj);
> +    char *str = g_strndup((char *) dev->loadparm, sizeof(dev->loadparm));
> +
> +    visit_type_str(v, name, &str, errp);
> +    g_free(str);
> +}
> +
> +static void ccw_device_set_loadparm(Object *obj, Visitor *v,
> +                                 const char *name, void *opaque,
> +                                 Error **errp)
> +{
> +    CcwDevice *dev = CCW_DEVICE(obj);
> +    char *val;
> +    int index;
> +
> +    index = object_property_get_int(obj, "bootindex", NULL);
> +
> +    if (index < 0) {
> +        error_setg(errp, "LOADPARM is only valid for boot devices!");
> +    }
> +
> +    if (!visit_type_str(v, name, &val, errp)) {
> +        return;
> +    }
> +
> +    s390_ipl_fmt_loadparm(dev->loadparm, val, errp);
> +}
> +
> +static 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",
> +    .get = ccw_device_get_loadparm,
> +    .set = ccw_device_set_loadparm,
> +};
> +
>   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(),
>   };

  Hi Jared,

while doing more tests with these new boot order patches, I realized that 
really *all* virtio-ccw devices now have a "loadparm" property, also devices 
like virtio-gpu and virtio-tablet, where it does not make sense at all.

Wouldn't it be better to add this property only to the virtio-ccw-blk device?

Also, is there a way to specify the "loadparm" property for virtio-scsi 
devices? It does not seem to be available for "scsi-hd" ?

   Thomas