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>
---
hw/s390x/ccw-device.h | 2 ++
hw/s390x/ipl.h | 3 +-
hw/s390x/ccw-device.c | 49 ++++++++++++++++++++++++++++
hw/s390x/ipl.c | 67 +++++++++++++++++++++++---------------
hw/s390x/s390-virtio-ccw.c | 18 +---------
hw/s390x/sclp.c | 3 +-
pc-bios/s390-ccw/main.c | 10 ++++--
7 files changed, 104 insertions(+), 48 deletions(-)
diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
index 6dff95225d..35ccf1f7bb 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 b066d9e8e5..1dcb8984bb 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_set_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/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 (!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",
+ .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]))
+
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 == ' ')) {
+ 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)
+{
+ 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;
+
return true;
}
return false;
}
-int s390_ipl_set_loadparm(uint8_t *loadparm)
-{
- MachineState *machine = MACHINE(qdev_get_machine());
- char *lp = object_property_get_str(OBJECT(machine), "loadparm", NULL);
-
- if (lp) {
- int i;
-
- /* lp is an uppercase string without leading/embedded spaces */
- for (i = 0; i < 8 && lp[i]; i++) {
- loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]];
- }
-
- if (i < 8) {
- memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */
- }
-
- g_free(lp);
- return 0;
- }
-
- return -1;
-}
-
static int load_netboot_image(Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3d0bc3e7f2..00478bd6ee 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -714,28 +714,12 @@ static void machine_set_loadparm(Object *obj, Visitor *v,
{
S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
char *val;
- int i;
if (!visit_type_str(v, name, &val, errp)) {
return;
}
- for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) {
- uint8_t c = qemu_toupper(val[i]); /* mimic HMC */
-
- if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || (c == '.') ||
- (c == ' ')) {
- ms->loadparm[i] = c;
- } else {
- error_setg(errp, "LOADPARM: invalid character '%c' (ASCII 0x%02x)",
- c, c);
- return;
- }
- }
-
- for (; i < sizeof(ms->loadparm); i++) {
- ms->loadparm[i] = ' '; /* pad right with spaces */
- }
+ s390_ipl_fmt_loadparm(ms->loadparm, val, errp);
}
static void ccw_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index e725dcd5fd..00afa9ad52 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -175,7 +175,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
memcpy(&read_info->loadparm, &ipib->loadparm,
sizeof(read_info->loadparm));
} else {
- s390_ipl_set_loadparm(read_info->loadparm);
+ s390_ipl_set_loadparm((char *)S390_CCW_MACHINE(machine)->loadparm,
+ read_info->loadparm);
}
sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 5506798098..3e51d698d7 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -173,8 +173,14 @@ static void css_setup(void)
static void boot_setup(void)
{
char lpmsg[] = "LOADPARM=[________]\n";
+ have_iplb = store_iplb(&iplb);
+
+ if (memcmp(iplb.loadparm, "@@@@@@@@", LOADPARM_LEN) != 0) {
+ ebcdic_to_ascii((char *) iplb.loadparm, loadparm_str, LOADPARM_LEN);
+ } else {
+ sclp_get_loadparm_ascii(loadparm_str);
+ }
- sclp_get_loadparm_ascii(loadparm_str);
memcpy(lpmsg + 10, loadparm_str, 8);
sclp_print(lpmsg);
@@ -183,8 +189,6 @@ static void boot_setup(void)
* so we don't taint our decision-making process during a reboot.
*/
memset((char *)S390EP, 0, 6);
-
- have_iplb = store_iplb(&iplb);
}
static void find_boot_device(void)
--
2.45.1On 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);
The error_abort is rather unfortunate here, it can be used to terminate QEMU
unexpectedly:
$ ./qemu-system-s390x -no-shutdown -nographic -serial none -monitor stdio
QEMU 9.0.50 monitor - type 'help' for more information
(qemu) device_add virtio-rng-ccw,loadparm=1
Unexpected error in object_property_find_err() at
../../devel/qemu/qom/object.c:1358:
Property 'virtio-rng-ccw.bootindex' not found
Aborted (core dumped)
Since you check for the error via "index" afterwards anyway, I think it's
likely ok to replace &error_abort with NULL here.
But apart from that, it's also a bit ugly that each and every ccw device
gets a loadparm property now. Would it be possible to add it to the devices
that can be used for booting only?
Thomas
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
Hi Thomas,
Firstly, thanks for the reviews and I agree with your suggestions about
function names, info messages, simplifications, etc... I will make
those changes.
As for the DIAG308_FLAGS_LP_VALID flag...
> [snip...]
>> @@ -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?
No, I don't think it should be omitted if the loadparm hasn't been
specified because it is optional and uses a default value if not set.
My understanding is that the flag should, actually, always be set here.
As best as I can tell, the existing check is a redundant validation that
cannot fail and therefore is not needed. Currently the only way
s390_ipl_set_loadparm() can return non-zero is if
object_property_get_str() itself returns NULL pointer when getting the
loadparm; however, the loadparm is already validated at this point
because the string is initialized when the loadparm property is set. If
there were a problem with the loadparm string an error would have
already been thrown during initialization.
Furthermore, object_property_get_str() is no longer used here. As such,
s390_ipl_set_loadparm() is changed to a void function and the check is
removed.
Regards,
Jared Rossi
On 04/06/2024 18.27, Jared Rossi wrote:
> Hi Thomas,
>
> Firstly, thanks for the reviews and I agree with your suggestions about
> function names, info messages, simplifications, etc... I will make those
> changes.
>
> As for the DIAG308_FLAGS_LP_VALID flag...
>
>> [snip...]
>>> @@ -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?
>
> No, I don't think it should be omitted if the loadparm hasn't been specified
> because it is optional and uses a default value if not set. My understanding
> is that the flag should, actually, always be set here.
>
> As best as I can tell, the existing check is a redundant validation that
> cannot fail and therefore is not needed. Currently the only way
> s390_ipl_set_loadparm() can return non-zero is if object_property_get_str()
> itself returns NULL pointer when getting the loadparm; however, the loadparm
> is already validated at this point because the string is initialized when
> the loadparm property is set. If there were a problem with the loadparm
> string an error would have already been thrown during initialization.
>
> Furthermore, object_property_get_str() is no longer used here. As such,
> s390_ipl_set_loadparm() is changed to a void function and the check is removed.
Ok, makes sense thanks for the explanation!
Thomas
© 2016 - 2024 Red Hat, Inc.