qemu-s390x
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice


From: Thomas Huth
Subject: Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice
Date: Tue, 4 Jun 2024 16:27:23 +0200
User-agent: Mozilla Thunderbird

On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
From: Jared Rossi <jrossi@linux.ibm.com>

Add a loadparm property to the CcwDevice object so that different loadparms
can be defined on a per-device basis when using multiple 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.

Assigning a loadparm to a non-boot device is invalid and will be rejected.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
...
diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index fb8c1acc64..143e085279 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)
  {
@@ -36,10 +40,55 @@ static void ccw_device_realize(CcwDevice *dev, Error **errp)
      ccw_device_refill_ids(dev);
  }
+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", &error_abort);
+
+    if (index < 0) {
+        error_setg(errp, "LOADPARM: non-boot device");

If the user is not very experienced, this error message is not very helpful. Maybe: "loadparm is only supported on devices with bootindex property" ?

+    }
+
+    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. ] (lower case"
+            " chars converted to upper case) to pass to machine loader,"
+            " boot manager, and guest kernel",

I know, we're using the same text for the description of the machine property already, but maybe we could do better here: The string is very long, it wraps around unless you use a very huge console window when running QEMU with e.g. "-device virtio-blk-ccw,help".

What do you think about using something like this:

.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,
+};
+
+#define DEFINE_PROP_CCW_LOADPARM(_n, _s, _f) \
+    DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(uint8_t[8]))

Not sure if it's worth the effort to create a macro just for this if you only need it once below. I'd maybe rather inline the DEFINE_PROP below.

  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_CCW_LOADPARM("loadparm", CcwDevice, loadparm),
      DEFINE_PROP_END_OF_LIST(),
  };
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index e934bf89d1..2d4f5152b3 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -34,6 +34,7 @@
  #include "qemu/config-file.h"
  #include "qemu/cutils.h"
  #include "qemu/option.h"
+#include "qemu/ctype.h"
  #include "standard-headers/linux/virtio_ids.h"
#define KERN_IMAGE_START 0x010000UL
@@ -390,12 +391,44 @@ static CcwDevice *s390_get_ccw_device(DeviceState 
*dev_st, int *devtype)
      return ccw_dev;
  }
+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 (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || (c == '.') ||
+            (c == ' ')) {

You could simplify it to:

    if (qemu_isalpha(c) || c == '.' || c == ' ')

+            loadparm[i] = c;
+        } else {
+            error_setg(errp, "LOADPARM: invalid character '%c' (ASCII 0x%02x)",
+                       c, c);
+            return;
+        }
+    }
+}
+
+void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)

I'd maybe rename this function now, maybe s390_ipl_convert_loadparm() or something similar?

+{
+    int i;
+
+    /* Initialize the loadparm with EBCDIC spaces (0x40) */
+    memset(ebcdic_lp, '@', LOADPARM_LEN);
+    for (i = 0; i < LOADPARM_LEN && ascii_lp[i]; i++) {
+        ebcdic_lp[i] = ascii2ebcdic[(uint8_t) ascii_lp[i]];
+    }
+}
+
  static bool s390_gen_initial_iplb(S390IPLState *ipl)
  {
      DeviceState *dev_st;
      CcwDevice *ccw_dev = NULL;
      SCSIDevice *sd;
      int devtype;
+    uint8_t *lp;
dev_st = get_boot_device(0);
      if (dev_st) {
@@ -406,6 +439,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
       * Currently allow IPL only from CCW devices.
       */
      if (ccw_dev) {
+        lp = ccw_dev->loadparm;
+
          switch (devtype) {
          case CCW_DEVTYPE_SCSI:
              sd = SCSI_DEVICE(dev_st);
@@ -438,40 +473,20 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
              break;
          }
- if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
-            ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
+        /* If the device loadparm is empty use the global machine loadparm */
+        if (memcmp(lp, "\0\0\0\0\0\0\0\0", 8) == 0) {
+            lp = S390_CCW_MACHINE(qdev_get_machine())->loadparm;
          }
+ s390_ipl_set_loadparm((char *)lp, ipl->iplb.loadparm);
+        ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;

That means DIAG308_FLAGS_LP_VALID is now always set, even if no loadparm has been specified? Shouldn't it be omitted if the user did not specify the loadparm property?

          return true;
      }
return false;
  }

 Thomas





reply via email to

[Prev in Thread] Current Thread [Next in Thread]