* [PATCH 1/5] s390x: Create include files for s390x IPL definitions
2024-05-29 15:43 [PATCH 0/5] s390x: Add Full Boot Order Support jrossi
@ 2024-05-29 15:43 ` jrossi
2024-06-03 18:51 ` Thomas Huth
2024-05-29 15:43 ` [PATCH 2/5] s390x: Add loadparm to CcwDevice jrossi
` (5 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: jrossi @ 2024-05-29 15:43 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth; +Cc: frankja, nsg, jrossi
From: Jared Rossi <jrossi@linux.ibm.com>
Currently, stuctures defined in both hw/s390x/ipl.h and pc-bios/s390-ccw/iplb.h
must be kept in sync, which is prone to error. Instead, create a new directory
at include/hw/s390x/ipl/ to contain the definitions that must be shared.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
hw/s390x/ipl.h | 113 +-------------------------------
include/hw/s390x/ipl/qipl.h | 126 ++++++++++++++++++++++++++++++++++++
pc-bios/s390-ccw/iplb.h | 84 ++----------------------
pc-bios/s390-ccw/Makefile | 2 +-
4 files changed, 133 insertions(+), 192 deletions(-)
create mode 100644 include/hw/s390x/ipl/qipl.h
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 57cd125769..b066d9e8e5 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -16,95 +16,11 @@
#include "cpu.h"
#include "exec/address-spaces.h"
#include "hw/qdev-core.h"
+#include "hw/s390x/ipl/qipl.h"
#include "qom/object.h"
-struct IPLBlockPVComp {
- uint64_t tweak_pref;
- uint64_t addr;
- uint64_t size;
-} QEMU_PACKED;
-typedef struct IPLBlockPVComp IPLBlockPVComp;
-
-struct IPLBlockPV {
- uint8_t reserved18[87]; /* 0x18 */
- uint8_t version; /* 0x6f */
- uint32_t reserved70; /* 0x70 */
- uint32_t num_comp; /* 0x74 */
- uint64_t pv_header_addr; /* 0x78 */
- uint64_t pv_header_len; /* 0x80 */
- struct IPLBlockPVComp components[0];
-} QEMU_PACKED;
-typedef struct IPLBlockPV IPLBlockPV;
-
-struct IplBlockCcw {
- uint8_t reserved0[85];
- uint8_t ssid;
- uint16_t devno;
- uint8_t vm_flags;
- uint8_t reserved3[3];
- uint32_t vm_parm_len;
- uint8_t nss_name[8];
- uint8_t vm_parm[64];
- uint8_t reserved4[8];
-} QEMU_PACKED;
-typedef struct IplBlockCcw IplBlockCcw;
-
-struct IplBlockFcp {
- uint8_t reserved1[305 - 1];
- uint8_t opt;
- uint8_t reserved2[3];
- uint16_t reserved3;
- uint16_t devno;
- uint8_t reserved4[4];
- uint64_t wwpn;
- uint64_t lun;
- uint32_t bootprog;
- uint8_t reserved5[12];
- uint64_t br_lba;
- uint32_t scp_data_len;
- uint8_t reserved6[260];
- uint8_t scp_data[0];
-} QEMU_PACKED;
-typedef struct IplBlockFcp IplBlockFcp;
-
-struct IplBlockQemuScsi {
- uint32_t lun;
- uint16_t target;
- uint16_t channel;
- uint8_t reserved0[77];
- uint8_t ssid;
- uint16_t devno;
-} QEMU_PACKED;
-typedef struct IplBlockQemuScsi IplBlockQemuScsi;
-
#define DIAG308_FLAGS_LP_VALID 0x80
-union IplParameterBlock {
- struct {
- uint32_t len;
- uint8_t reserved0[3];
- uint8_t version;
- uint32_t blk0_len;
- uint8_t pbt;
- uint8_t flags;
- uint16_t reserved01;
- uint8_t loadparm[8];
- union {
- IplBlockCcw ccw;
- IplBlockFcp fcp;
- IPLBlockPV pv;
- IplBlockQemuScsi scsi;
- };
- } QEMU_PACKED;
- struct {
- uint8_t reserved1[110];
- uint16_t devno;
- uint8_t reserved2[88];
- uint8_t reserved_ext[4096 - 200];
- } QEMU_PACKED;
-} QEMU_PACKED;
-typedef union IplParameterBlock IplParameterBlock;
-
int s390_ipl_set_loadparm(uint8_t *loadparm);
void s390_ipl_update_diag308(IplParameterBlock *iplb);
int s390_ipl_prepare_pv_header(Error **errp);
@@ -125,33 +41,6 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type);
void s390_ipl_get_reset_request(CPUState **cs, enum s390_reset *reset_type);
void s390_ipl_clear_reset_request(void);
-#define QIPL_ADDRESS 0xcc
-
-/* Boot Menu flags */
-#define QIPL_FLAG_BM_OPTS_CMD 0x80
-#define QIPL_FLAG_BM_OPTS_ZIPL 0x40
-
-/*
- * The QEMU IPL Parameters will be stored at absolute address
- * 204 (0xcc) which means it is 32-bit word aligned but not
- * double-word aligned.
- * Placement of data fields in this area must account for
- * their alignment needs. E.g., netboot_start_address must
- * have an offset of 4 + n * 8 bytes within the struct in order
- * to keep it double-word aligned.
- * The total size of the struct must never exceed 28 bytes.
- * This definition must be kept in sync with the definition
- * in pc-bios/s390-ccw/iplb.h.
- */
-struct QemuIplParameters {
- uint8_t qipl_flags;
- uint8_t reserved1[3];
- uint64_t netboot_start_addr;
- uint32_t boot_menu_timeout;
- uint8_t reserved2[12];
-} QEMU_PACKED;
-typedef struct QemuIplParameters QemuIplParameters;
-
#define TYPE_S390_IPL "s390-ipl"
OBJECT_DECLARE_SIMPLE_TYPE(S390IPLState, S390_IPL)
diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
new file mode 100644
index 0000000000..a6ce6ddfe3
--- /dev/null
+++ b/include/hw/s390x/ipl/qipl.h
@@ -0,0 +1,126 @@
+/*
+ * S/390 boot structures
+ *
+ * Copyright 2024 IBM Corp.
+ * Author(s): Jared Rossi <jrossi@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef S390X_QIPL_H
+#define S390X_QIPL_H
+
+/* Boot Menu flags */
+#define QIPL_FLAG_BM_OPTS_CMD 0x80
+#define QIPL_FLAG_BM_OPTS_ZIPL 0x40
+
+#define QIPL_ADDRESS 0xcc
+#define LOADPARM_LEN 8
+
+/*
+ * The QEMU IPL Parameters will be stored at absolute address
+ * 204 (0xcc) which means it is 32-bit word aligned but not
+ * double-word aligned.
+ * Placement of data fields in this area must account for
+ * their alignment needs. E.g., netboot_start_address must
+ * have an offset of 4 + n * 8 bytes within the struct in order
+ * to keep it double-word aligned.
+ * The total size of the struct must never exceed 28 bytes.
+ */
+struct QemuIplParameters {
+ uint8_t qipl_flags;
+ uint8_t reserved1[3];
+ uint64_t netboot_start_addr;
+ uint32_t boot_menu_timeout;
+ uint8_t reserved2[12];
+} QEMU_PACKED;
+typedef struct QemuIplParameters QemuIplParameters;
+
+struct IPLBlockPVComp {
+ uint64_t tweak_pref;
+ uint64_t addr;
+ uint64_t size;
+} QEMU_PACKED;
+typedef struct IPLBlockPVComp IPLBlockPVComp;
+
+struct IPLBlockPV {
+ uint8_t reserved18[87]; /* 0x18 */
+ uint8_t version; /* 0x6f */
+ uint32_t reserved70; /* 0x70 */
+ uint32_t num_comp; /* 0x74 */
+ uint64_t pv_header_addr; /* 0x78 */
+ uint64_t pv_header_len; /* 0x80 */
+ struct IPLBlockPVComp components[0];
+} QEMU_PACKED;
+typedef struct IPLBlockPV IPLBlockPV;
+
+struct IplBlockCcw {
+ uint8_t reserved0[85];
+ uint8_t ssid;
+ uint16_t devno;
+ uint8_t vm_flags;
+ uint8_t reserved3[3];
+ uint32_t vm_parm_len;
+ uint8_t nss_name[8];
+ uint8_t vm_parm[64];
+ uint8_t reserved4[8];
+} QEMU_PACKED;
+typedef struct IplBlockCcw IplBlockCcw;
+
+struct IplBlockFcp {
+ uint8_t reserved1[305 - 1];
+ uint8_t opt;
+ uint8_t reserved2[3];
+ uint16_t reserved3;
+ uint16_t devno;
+ uint8_t reserved4[4];
+ uint64_t wwpn;
+ uint64_t lun;
+ uint32_t bootprog;
+ uint8_t reserved5[12];
+ uint64_t br_lba;
+ uint32_t scp_data_len;
+ uint8_t reserved6[260];
+ uint8_t scp_data[0];
+} QEMU_PACKED;
+typedef struct IplBlockFcp IplBlockFcp;
+
+struct IplBlockQemuScsi {
+ uint32_t lun;
+ uint16_t target;
+ uint16_t channel;
+ uint8_t reserved0[77];
+ uint8_t ssid;
+ uint16_t devno;
+} QEMU_PACKED;
+typedef struct IplBlockQemuScsi IplBlockQemuScsi;
+
+union IplParameterBlock {
+ struct {
+ uint32_t len;
+ uint8_t reserved0[3];
+ uint8_t version;
+ uint32_t blk0_len;
+ uint8_t pbt;
+ uint8_t flags;
+ uint16_t reserved01;
+ uint8_t loadparm[LOADPARM_LEN];
+ union {
+ IplBlockCcw ccw;
+ IplBlockFcp fcp;
+ IPLBlockPV pv;
+ IplBlockQemuScsi scsi;
+ };
+ } QEMU_PACKED;
+ struct {
+ uint8_t reserved1[110];
+ uint16_t devno;
+ uint8_t reserved2[88];
+ uint8_t reserved_ext[4096 - 200];
+ } QEMU_PACKED;
+} QEMU_PACKED;
+typedef union IplParameterBlock IplParameterBlock;
+
+#endif
diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index cb6ac8a880..16643f5879 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -12,88 +12,14 @@
#ifndef IPLB_H
#define IPLB_H
-#define LOADPARM_LEN 8
+#ifndef QEMU_PACKED
+#define QEMU_PACKED __attribute__((packed))
+#endif
-struct IplBlockCcw {
- uint8_t reserved0[85];
- uint8_t ssid;
- uint16_t devno;
- uint8_t vm_flags;
- uint8_t reserved3[3];
- uint32_t vm_parm_len;
- uint8_t nss_name[8];
- uint8_t vm_parm[64];
- uint8_t reserved4[8];
-} __attribute__ ((packed));
-typedef struct IplBlockCcw IplBlockCcw;
-
-struct IplBlockFcp {
- uint8_t reserved1[305 - 1];
- uint8_t opt;
- uint8_t reserved2[3];
- uint16_t reserved3;
- uint16_t devno;
- uint8_t reserved4[4];
- uint64_t wwpn;
- uint64_t lun;
- uint32_t bootprog;
- uint8_t reserved5[12];
- uint64_t br_lba;
- uint32_t scp_data_len;
- uint8_t reserved6[260];
- uint8_t scp_data[];
-} __attribute__ ((packed));
-typedef struct IplBlockFcp IplBlockFcp;
-
-struct IplBlockQemuScsi {
- uint32_t lun;
- uint16_t target;
- uint16_t channel;
- uint8_t reserved0[77];
- uint8_t ssid;
- uint16_t devno;
-} __attribute__ ((packed));
-typedef struct IplBlockQemuScsi IplBlockQemuScsi;
-
-struct IplParameterBlock {
- uint32_t len;
- uint8_t reserved0[3];
- uint8_t version;
- uint32_t blk0_len;
- uint8_t pbt;
- uint8_t flags;
- uint16_t reserved01;
- uint8_t loadparm[LOADPARM_LEN];
- union {
- IplBlockCcw ccw;
- IplBlockFcp fcp;
- IplBlockQemuScsi scsi;
- };
-} __attribute__ ((packed));
-typedef struct IplParameterBlock IplParameterBlock;
-
-extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
-
-#define QIPL_ADDRESS 0xcc
-
-/* Boot Menu flags */
-#define QIPL_FLAG_BM_OPTS_CMD 0x80
-#define QIPL_FLAG_BM_OPTS_ZIPL 0x40
-
-/*
- * This definition must be kept in sync with the definition
- * in hw/s390x/ipl.h
- */
-struct QemuIplParameters {
- uint8_t qipl_flags;
- uint8_t reserved1[3];
- uint64_t netboot_start_addr;
- uint32_t boot_menu_timeout;
- uint8_t reserved2[12];
-} __attribute__ ((packed));
-typedef struct QemuIplParameters QemuIplParameters;
+#include <qipl.h>
extern QemuIplParameters qipl;
+extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
#define S390_IPL_TYPE_FCP 0x00
#define S390_IPL_TYPE_CCW 0x02
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index acfcd1e71a..a771439acf 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -3,7 +3,7 @@ all: build-all
@true
include config-host.mak
-CFLAGS = -O2 -g
+CFLAGS = -O2 -g -I $(SRC_PATH)/../..//include/hw/s390x/ipl
MAKEFLAGS += -rR
GIT_SUBMODULES = roms/SLOF
--
2.45.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 1/5] s390x: Create include files for s390x IPL definitions
2024-05-29 15:43 ` [PATCH 1/5] s390x: Create include files for s390x IPL definitions jrossi
@ 2024-06-03 18:51 ` Thomas Huth
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-03 18:51 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
Hi Jared!
On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Currently, stuctures defined in both hw/s390x/ipl.h and pc-bios/s390-ccw/iplb.h
Typo: s/stuctures/structures/
> must be kept in sync, which is prone to error. Instead, create a new directory
> at include/hw/s390x/ipl/ to contain the definitions that must be shared.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
> hw/s390x/ipl.h | 113 +-------------------------------
> include/hw/s390x/ipl/qipl.h | 126 ++++++++++++++++++++++++++++++++++++
> pc-bios/s390-ccw/iplb.h | 84 ++----------------------
> pc-bios/s390-ccw/Makefile | 2 +-
> 4 files changed, 133 insertions(+), 192 deletions(-)
> create mode 100644 include/hw/s390x/ipl/qipl.h
...
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index acfcd1e71a..a771439acf 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -3,7 +3,7 @@ all: build-all
> @true
>
> include config-host.mak
> -CFLAGS = -O2 -g
> +CFLAGS = -O2 -g -I $(SRC_PATH)/../..//include/hw/s390x/ipl
Duplicate slash -----------------------^
Apart from these two nits, patch looks fine to me.
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 2/5] s390x: Add loadparm to CcwDevice
2024-05-29 15:43 [PATCH 0/5] s390x: Add Full Boot Order Support jrossi
2024-05-29 15:43 ` [PATCH 1/5] s390x: Create include files for s390x IPL definitions jrossi
@ 2024-05-29 15:43 ` jrossi
2024-06-04 14:27 ` Thomas Huth
2024-06-05 7:49 ` Thomas Huth
2024-05-29 15:43 ` [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices jrossi
` (4 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: jrossi @ 2024-05-29 15:43 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth; +Cc: frankja, nsg, jrossi
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.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice
2024-05-29 15:43 ` [PATCH 2/5] s390x: Add loadparm to CcwDevice jrossi
@ 2024-06-04 14:27 ` Thomas Huth
2024-06-04 16:27 ` Jared Rossi
2024-06-05 7:49 ` Thomas Huth
1 sibling, 1 reply; 29+ messages in thread
From: Thomas Huth @ 2024-06-04 14:27 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice
2024-06-04 14:27 ` Thomas Huth
@ 2024-06-04 16:27 ` Jared Rossi
2024-06-04 16:59 ` Thomas Huth
0 siblings, 1 reply; 29+ messages in thread
From: Jared Rossi @ 2024-06-04 16:27 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x; +Cc: frankja, nsg
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice
2024-06-04 16:27 ` Jared Rossi
@ 2024-06-04 16:59 ` Thomas Huth
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-04 16:59 UTC (permalink / raw)
To: Jared Rossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice
2024-05-29 15:43 ` [PATCH 2/5] s390x: Add loadparm to CcwDevice jrossi
2024-06-04 14:27 ` Thomas Huth
@ 2024-06-05 7:49 ` Thomas Huth
1 sibling, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-05 7:49 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
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);
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices
2024-05-29 15:43 [PATCH 0/5] s390x: Add Full Boot Order Support jrossi
2024-05-29 15:43 ` [PATCH 1/5] s390x: Create include files for s390x IPL definitions jrossi
2024-05-29 15:43 ` [PATCH 2/5] s390x: Add loadparm to CcwDevice jrossi
@ 2024-05-29 15:43 ` jrossi
2024-06-03 19:03 ` Thomas Huth
2024-06-04 18:26 ` Thomas Huth
2024-05-29 15:43 ` [PATCH 4/5] s390x: Add boot device fallback infrastructure jrossi
` (3 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: jrossi @ 2024-05-29 15:43 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth; +Cc: frankja, nsg, jrossi
From: Jared Rossi <jrossi@linux.ibm.com>
Write a chain of IPLBs into memory for future use.
The IPLB chain is placed immediately before the BIOS in memory at the highest
unused page boundary providing sufficient space to fit the chain. Because this
is not a fixed address, the location of the next IPLB and number of remaining
boot devices is stored in the QIPL global variable for later access.
At this stage the IPLB chain is not accessed by the guest during IPL.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
hw/s390x/ipl.h | 1 +
include/hw/s390x/ipl/qipl.h | 4 +-
hw/s390x/ipl.c | 129 +++++++++++++++++++++++++++---------
3 files changed, 103 insertions(+), 31 deletions(-)
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 1dcb8984bb..4f098d3a81 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -20,6 +20,7 @@
#include "qom/object.h"
#define DIAG308_FLAGS_LP_VALID 0x80
+#define MAX_IPLB_CHAIN 7
void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
index a6ce6ddfe3..481c459a53 100644
--- a/include/hw/s390x/ipl/qipl.h
+++ b/include/hw/s390x/ipl/qipl.h
@@ -34,7 +34,9 @@ struct QemuIplParameters {
uint8_t reserved1[3];
uint64_t netboot_start_addr;
uint32_t boot_menu_timeout;
- uint8_t reserved2[12];
+ uint8_t reserved2[2];
+ uint16_t num_iplbs;
+ uint64_t next_iplb;
} QEMU_PACKED;
typedef struct QemuIplParameters QemuIplParameters;
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 2d4f5152b3..79429acabd 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -55,6 +55,13 @@ static bool iplb_extended_needed(void *opaque)
return ipl->iplbext_migration;
}
+/* Start IPLB chain from the boundary of the first unused page before BIOS */
+static uint64_t find_iplb_chain_addr(uint64_t bios_addr, uint16_t count)
+{
+ return (bios_addr & TARGET_PAGE_MASK)
+ - (count * sizeof(IplParameterBlock));
+}
+
static const VMStateDescription vmstate_iplb_extended = {
.name = "ipl/iplb_extended",
.version_id = 0,
@@ -391,6 +398,17 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
return ccw_dev;
}
+static void s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
+{
+ S390IPLState *ipl = get_ipl_device();
+ uint16_t count = ipl->qipl.num_iplbs;
+ uint64_t len = sizeof(IplParameterBlock) * count;
+ uint64_t chain_addr = find_iplb_chain_addr(ipl->bios_start_addr, count);
+
+ cpu_physical_memory_write(chain_addr, iplb_chain, be32_to_cpu(len));
+ ipl->qipl.next_iplb = chain_addr;
+}
+
void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
{
int i;
@@ -422,54 +440,51 @@ void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
}
}
-static bool s390_gen_initial_iplb(S390IPLState *ipl)
+static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
{
- DeviceState *dev_st;
+ S390IPLState *ipl = get_ipl_device();
CcwDevice *ccw_dev = NULL;
SCSIDevice *sd;
int devtype;
uint8_t *lp;
- dev_st = get_boot_device(0);
- if (dev_st) {
- ccw_dev = s390_get_ccw_device(dev_st, &devtype);
- }
-
/*
* Currently allow IPL only from CCW devices.
*/
+ ccw_dev = s390_get_ccw_device(dev_st, &devtype);
if (ccw_dev) {
lp = ccw_dev->loadparm;
- switch (devtype) {
- case CCW_DEVTYPE_SCSI:
+ switch (devtype) {
+ case CCW_DEVTYPE_SCSI:
sd = SCSI_DEVICE(dev_st);
- ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
- ipl->iplb.blk0_len =
+ 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);
- ipl->iplb.pbt = S390_IPL_TYPE_QEMU_SCSI;
- ipl->iplb.scsi.lun = cpu_to_be32(sd->lun);
- ipl->iplb.scsi.target = cpu_to_be16(sd->id);
- ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
- ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
- ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
+ iplb->pbt = S390_IPL_TYPE_QEMU_SCSI;
+ iplb->scsi.lun = cpu_to_be32(sd->lun);
+ iplb->scsi.target = cpu_to_be16(sd->id);
+ iplb->scsi.channel = cpu_to_be16(sd->channel);
+ iplb->scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
+ iplb->scsi.ssid = ccw_dev->sch->ssid & 3;
break;
case CCW_DEVTYPE_VFIO:
- ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
- ipl->iplb.pbt = S390_IPL_TYPE_CCW;
- ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
- ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
+ iplb->len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
+ iplb->pbt = S390_IPL_TYPE_CCW;
+ iplb->ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
+ iplb->ccw.ssid = ccw_dev->sch->ssid & 3;
break;
case CCW_DEVTYPE_VIRTIO_NET:
+ /* The S390IPLState netboot is ture if ANY IPLB may use netboot */
ipl->netboot = true;
/* Fall through to CCW_DEVTYPE_VIRTIO case */
case CCW_DEVTYPE_VIRTIO:
- ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
- ipl->iplb.blk0_len =
+ iplb->len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
+ iplb->blk0_len =
cpu_to_be32(S390_IPLB_MIN_CCW_LEN - S390_IPLB_HEADER_LEN);
- ipl->iplb.pbt = S390_IPL_TYPE_CCW;
- ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
- ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
+ iplb->pbt = S390_IPL_TYPE_CCW;
+ iplb->ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
+ iplb->ccw.ssid = ccw_dev->sch->ssid & 3;
break;
}
@@ -478,8 +493,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
lp = S390_CCW_MACHINE(qdev_get_machine())->loadparm;
}
- s390_ipl_set_loadparm((char *)lp, ipl->iplb.loadparm);
- ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
+ s390_ipl_set_loadparm((char *)lp, iplb->loadparm);
+ iplb->flags |= DIAG308_FLAGS_LP_VALID;
return true;
}
@@ -487,6 +502,58 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
return false;
}
+static bool s390_init_all_iplbs(S390IPLState *ipl)
+{
+ int iplb_num = 0;
+ IplParameterBlock iplb_chain[7];
+ DeviceState *dev_st = get_boot_device(0);
+
+ /*
+ * Parse the boot devices. Generate an IPLB for the first boot device,
+ * which will later be set with DIAG308. Index any fallback boot devices.
+ */
+ if (!dev_st) {
+ ipl->qipl.num_iplbs = 0;
+ return false;
+ }
+
+ iplb_num = 1;
+ s390_build_iplb(dev_st, &ipl->iplb);
+ ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
+
+ while (get_boot_device(iplb_num)) {
+ iplb_num++;
+ }
+
+ ipl->qipl.num_iplbs = iplb_num - 1;
+
+ /*
+ * Build fallback IPLBs for any boot devices above index 0, up to a
+ * maximum amount as defined in ipl.h
+ */
+ if (iplb_num > 1) {
+ if (iplb_num > MAX_IPLB_CHAIN) {
+ warn_report("Excess boot devices defined! %d boot devices found, "
+ "but only the first %d will be considered.",
+ iplb_num, MAX_IPLB_CHAIN + 1);
+ iplb_num = MAX_IPLB_CHAIN + 1;
+ }
+
+ ipl->qipl.num_iplbs = iplb_num - 1;
+
+ /* Start at 1 because the IPLB for boot index 0 is not chained */
+ for (int i = 1; i < iplb_num; i++) {
+ dev_st = get_boot_device(i);
+ s390_build_iplb(dev_st, &iplb_chain[i - 1]);
+ iplb_chain[i - 1].flags |= DIAG308_FLAGS_LP_VALID;
+ }
+
+ s390_ipl_map_iplb_chain(iplb_chain);
+ }
+
+ return iplb_num;
+}
+
static int load_netboot_image(Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
@@ -664,7 +731,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
* this is the original boot device's SCSI
* so restore IPL parameter info from it
*/
- ipl->iplb_valid = s390_gen_initial_iplb(ipl);
+ ipl->iplb_valid = s390_build_iplb(get_boot_device(0), &ipl->iplb);
}
}
if (reset_type == S390_RESET_MODIFIED_CLEAR ||
@@ -758,7 +825,9 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
if (!ipl->kernel || ipl->iplb_valid) {
cpu->env.psw.addr = ipl->bios_start_addr;
if (!ipl->iplb_valid) {
- ipl->iplb_valid = s390_gen_initial_iplb(ipl);
+ ipl->iplb_valid = s390_init_all_iplbs(ipl);
+ } else {
+ ipl->qipl.num_iplbs = 0;
}
}
if (ipl->netboot) {
--
2.45.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices
2024-05-29 15:43 ` [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices jrossi
@ 2024-06-03 19:03 ` Thomas Huth
2024-06-04 18:26 ` Thomas Huth
1 sibling, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-03 19:03 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Write a chain of IPLBs into memory for future use.
>
> The IPLB chain is placed immediately before the BIOS in memory at the highest
> unused page boundary providing sufficient space to fit the chain. Because this
> is not a fixed address, the location of the next IPLB and number of remaining
> boot devices is stored in the QIPL global variable for later access.
>
> At this stage the IPLB chain is not accessed by the guest during IPL.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
...
> @@ -422,54 +440,51 @@ void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
> }
> }
>
> -static bool s390_gen_initial_iplb(S390IPLState *ipl)
> +static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
> {
> - DeviceState *dev_st;
> + S390IPLState *ipl = get_ipl_device();
> CcwDevice *ccw_dev = NULL;
> SCSIDevice *sd;
> int devtype;
> uint8_t *lp;
>
> - dev_st = get_boot_device(0);
> - if (dev_st) {
> - ccw_dev = s390_get_ccw_device(dev_st, &devtype);
> - }
> -
> /*
> * Currently allow IPL only from CCW devices.
> */
> + ccw_dev = s390_get_ccw_device(dev_st, &devtype);
> if (ccw_dev) {
> lp = ccw_dev->loadparm;
>
> - switch (devtype) {
> - case CCW_DEVTYPE_SCSI:
> + switch (devtype) {
> + case CCW_DEVTYPE_SCSI:
Bad indentation?
> sd = SCSI_DEVICE(dev_st);
> - ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
> - ipl->iplb.blk0_len =
> + 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);
> - ipl->iplb.pbt = S390_IPL_TYPE_QEMU_SCSI;
> - ipl->iplb.scsi.lun = cpu_to_be32(sd->lun);
> - ipl->iplb.scsi.target = cpu_to_be16(sd->id);
> - ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
> - ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
> - ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
> + iplb->pbt = S390_IPL_TYPE_QEMU_SCSI;
> + iplb->scsi.lun = cpu_to_be32(sd->lun);
> + iplb->scsi.target = cpu_to_be16(sd->id);
> + iplb->scsi.channel = cpu_to_be16(sd->channel);
> + iplb->scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
> + iplb->scsi.ssid = ccw_dev->sch->ssid & 3;
> break;
> case CCW_DEVTYPE_VFIO:
> - ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
> - ipl->iplb.pbt = S390_IPL_TYPE_CCW;
> - ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
> - ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
> + iplb->len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
> + iplb->pbt = S390_IPL_TYPE_CCW;
> + iplb->ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
> + iplb->ccw.ssid = ccw_dev->sch->ssid & 3;
> break;
> case CCW_DEVTYPE_VIRTIO_NET:
> + /* The S390IPLState netboot is ture if ANY IPLB may use netboot */
Typo: ture --> true
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices
2024-05-29 15:43 ` [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices jrossi
2024-06-03 19:03 ` Thomas Huth
@ 2024-06-04 18:26 ` Thomas Huth
2024-06-05 20:01 ` Jared Rossi
1 sibling, 1 reply; 29+ messages in thread
From: Thomas Huth @ 2024-06-04 18:26 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Write a chain of IPLBs into memory for future use.
>
> The IPLB chain is placed immediately before the BIOS in memory at the highest
> unused page boundary providing sufficient space to fit the chain. Because this
> is not a fixed address, the location of the next IPLB and number of remaining
> boot devices is stored in the QIPL global variable for later access.
>
> At this stage the IPLB chain is not accessed by the guest during IPL.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
> hw/s390x/ipl.h | 1 +
> include/hw/s390x/ipl/qipl.h | 4 +-
> hw/s390x/ipl.c | 129 +++++++++++++++++++++++++++---------
> 3 files changed, 103 insertions(+), 31 deletions(-)
>
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 1dcb8984bb..4f098d3a81 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -20,6 +20,7 @@
> #include "qom/object.h"
>
> #define DIAG308_FLAGS_LP_VALID 0x80
> +#define MAX_IPLB_CHAIN 7
>
> void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
> void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index a6ce6ddfe3..481c459a53 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -34,7 +34,9 @@ struct QemuIplParameters {
> uint8_t reserved1[3];
> uint64_t netboot_start_addr;
> uint32_t boot_menu_timeout;
> - uint8_t reserved2[12];
> + uint8_t reserved2[2];
> + uint16_t num_iplbs;
> + uint64_t next_iplb;
> } QEMU_PACKED;
> typedef struct QemuIplParameters QemuIplParameters;
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 2d4f5152b3..79429acabd 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -55,6 +55,13 @@ static bool iplb_extended_needed(void *opaque)
> return ipl->iplbext_migration;
> }
>
> +/* Start IPLB chain from the boundary of the first unused page before BIOS */
I'd maybe say "upper boundary" to make it clear that this is at the end of
the page, not at the beginning?
> +static uint64_t find_iplb_chain_addr(uint64_t bios_addr, uint16_t count)
> +{
> + return (bios_addr & TARGET_PAGE_MASK)
> + - (count * sizeof(IplParameterBlock));
> +}
> +
> static const VMStateDescription vmstate_iplb_extended = {
> .name = "ipl/iplb_extended",
> .version_id = 0,
> @@ -391,6 +398,17 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
> return ccw_dev;
> }
>
> +static void s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
> +{
> + S390IPLState *ipl = get_ipl_device();
> + uint16_t count = ipl->qipl.num_iplbs;
> + uint64_t len = sizeof(IplParameterBlock) * count;
> + uint64_t chain_addr = find_iplb_chain_addr(ipl->bios_start_addr, count);
> +
> + cpu_physical_memory_write(chain_addr, iplb_chain, be32_to_cpu(len));
The be32_to_cpu looks wrong here, since you just computed len in native
endianness.
> + ipl->qipl.next_iplb = chain_addr;
Just a matter of taste, but I'd prefer to set ipl->qipl.next_iplb in the
same function where you set ipl->qipl.num_iplbs ... so I'd rather return
chain_addr here and then do this on the calling site:
ipl->qipl.next_iplb = s390_ipl_map_iplb_chain(...);
> +}
> +
> void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> {
> int i;
> @@ -422,54 +440,51 @@ void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
> }
> }
>
> -static bool s390_gen_initial_iplb(S390IPLState *ipl)
> +static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
> {
> - DeviceState *dev_st;
> + S390IPLState *ipl = get_ipl_device();
> CcwDevice *ccw_dev = NULL;
> SCSIDevice *sd;
> int devtype;
> uint8_t *lp;
>
> - dev_st = get_boot_device(0);
> - if (dev_st) {
> - ccw_dev = s390_get_ccw_device(dev_st, &devtype);
> - }
> -
> /*
> * Currently allow IPL only from CCW devices.
> */
> + ccw_dev = s390_get_ccw_device(dev_st, &devtype);
> if (ccw_dev) {
> lp = ccw_dev->loadparm;
>
> - switch (devtype) {
> - case CCW_DEVTYPE_SCSI:
> + switch (devtype) {
> + case CCW_DEVTYPE_SCSI:
> sd = SCSI_DEVICE(dev_st);
> - ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
> - ipl->iplb.blk0_len =
> + 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);
> - ipl->iplb.pbt = S390_IPL_TYPE_QEMU_SCSI;
> - ipl->iplb.scsi.lun = cpu_to_be32(sd->lun);
> - ipl->iplb.scsi.target = cpu_to_be16(sd->id);
> - ipl->iplb.scsi.channel = cpu_to_be16(sd->channel);
> - ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
> - ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
> + iplb->pbt = S390_IPL_TYPE_QEMU_SCSI;
> + iplb->scsi.lun = cpu_to_be32(sd->lun);
> + iplb->scsi.target = cpu_to_be16(sd->id);
> + iplb->scsi.channel = cpu_to_be16(sd->channel);
> + iplb->scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
> + iplb->scsi.ssid = ccw_dev->sch->ssid & 3;
> break;
> case CCW_DEVTYPE_VFIO:
> - ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
> - ipl->iplb.pbt = S390_IPL_TYPE_CCW;
> - ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
> - ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
> + iplb->len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
> + iplb->pbt = S390_IPL_TYPE_CCW;
> + iplb->ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
> + iplb->ccw.ssid = ccw_dev->sch->ssid & 3;
> break;
> case CCW_DEVTYPE_VIRTIO_NET:
> + /* The S390IPLState netboot is ture if ANY IPLB may use netboot */
> ipl->netboot = true;
> /* Fall through to CCW_DEVTYPE_VIRTIO case */
> case CCW_DEVTYPE_VIRTIO:
> - ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
> - ipl->iplb.blk0_len =
> + iplb->len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
> + iplb->blk0_len =
> cpu_to_be32(S390_IPLB_MIN_CCW_LEN - S390_IPLB_HEADER_LEN);
> - ipl->iplb.pbt = S390_IPL_TYPE_CCW;
> - ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
> - ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
> + iplb->pbt = S390_IPL_TYPE_CCW;
> + iplb->ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
> + iplb->ccw.ssid = ccw_dev->sch->ssid & 3;
> break;
> }
>
> @@ -478,8 +493,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> lp = S390_CCW_MACHINE(qdev_get_machine())->loadparm;
> }
>
> - s390_ipl_set_loadparm((char *)lp, ipl->iplb.loadparm);
> - ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
> + s390_ipl_set_loadparm((char *)lp, iplb->loadparm);
> + iplb->flags |= DIAG308_FLAGS_LP_VALID;
>
> return true;
> }
> @@ -487,6 +502,58 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> return false;
> }
>
> +static bool s390_init_all_iplbs(S390IPLState *ipl)
> +{
> + int iplb_num = 0;
> + IplParameterBlock iplb_chain[7];
> + DeviceState *dev_st = get_boot_device(0);
> +
> + /*
> + * Parse the boot devices. Generate an IPLB for the first boot device,
> + * which will later be set with DIAG308. Index any fallback boot devices.
> + */
> + if (!dev_st) {
> + ipl->qipl.num_iplbs = 0;
> + return false;
> + }
> +
> + iplb_num = 1;
> + s390_build_iplb(dev_st, &ipl->iplb);
> + ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
> +
> + while (get_boot_device(iplb_num)) {
> + iplb_num++;
> + }
> +
> + ipl->qipl.num_iplbs = iplb_num - 1;
It's somewhat confusing that ipl->qipl.num_iplbs is one less than iplb_num
... what does ipl->qipl.num_iplbs exactly define? The amount of additional
chained devices beside the first one?
A comment either here or qipl.h that describes the exact meaning of
num_iplbs would be helpful.
> +
> + /*
> + * Build fallback IPLBs for any boot devices above index 0, up to a
> + * maximum amount as defined in ipl.h
> + */
> + if (iplb_num > 1) {
> + if (iplb_num > MAX_IPLB_CHAIN) {
> + warn_report("Excess boot devices defined! %d boot devices found, "
> + "but only the first %d will be considered.",
> + iplb_num, MAX_IPLB_CHAIN + 1);
> + iplb_num = MAX_IPLB_CHAIN + 1;
What's now the real maximum number of iplb_num ? If it is MAX_IPLB_CHAIN + 1
then the if-statement above looks wrong, should it be "if (iplb_num >
MAX_IPLB_CHAIN + 1)" instead?
> + }
> +
> + ipl->qipl.num_iplbs = iplb_num - 1;
You could move that into the body of the above if-statement, since otherwise
the value has been set earlier in this function already.
> + /* Start at 1 because the IPLB for boot index 0 is not chained */
> + for (int i = 1; i < iplb_num; i++) {
Just to double-check: Is "i < iplb_num" right?
Or should it be "i <= iplb_num" instead?
BTW, have you successfully tested booting with 8 devices that all have a
boot index, but only the last one is bootable?
> + dev_st = get_boot_device(i);
> + s390_build_iplb(dev_st, &iplb_chain[i - 1]);
> + iplb_chain[i - 1].flags |= DIAG308_FLAGS_LP_VALID;
> + }
> +
> + s390_ipl_map_iplb_chain(iplb_chain);
> + }
> +
> + return iplb_num;
> +}
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices
2024-06-04 18:26 ` Thomas Huth
@ 2024-06-05 20:01 ` Jared Rossi
2024-06-07 6:11 ` Thomas Huth
0 siblings, 1 reply; 29+ messages in thread
From: Jared Rossi @ 2024-06-05 20:01 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 6/4/24 2:26 PM, Thomas Huth wrote:
> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> Write a chain of IPLBs into memory for future use.
>>
>> The IPLB chain is placed immediately before the BIOS in memory at the
>> highest
>> unused page boundary providing sufficient space to fit the chain.
>> Because this
>> is not a fixed address, the location of the next IPLB and number of
>> remaining
>> boot devices is stored in the QIPL global variable for later access.
>>
>> At this stage the IPLB chain is not accessed by the guest during IPL.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
>> hw/s390x/ipl.h | 1 +
>> include/hw/s390x/ipl/qipl.h | 4 +-
>> hw/s390x/ipl.c | 129 +++++++++++++++++++++++++++---------
>> 3 files changed, 103 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 1dcb8984bb..4f098d3a81 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -20,6 +20,7 @@
>> #include "qom/object.h"
>> #define DIAG308_FLAGS_LP_VALID 0x80
>> +#define MAX_IPLB_CHAIN 7
>> void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
>> void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error
>> **errp);
>> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
>> index a6ce6ddfe3..481c459a53 100644
>> --- a/include/hw/s390x/ipl/qipl.h
>> +++ b/include/hw/s390x/ipl/qipl.h
>> @@ -34,7 +34,9 @@ struct QemuIplParameters {
>> uint8_t reserved1[3];
>> uint64_t netboot_start_addr;
>> uint32_t boot_menu_timeout;
>> - uint8_t reserved2[12];
>> + uint8_t reserved2[2];
>> + uint16_t num_iplbs;
>> + uint64_t next_iplb;
>> } QEMU_PACKED;
>> typedef struct QemuIplParameters QemuIplParameters;
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 2d4f5152b3..79429acabd 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -55,6 +55,13 @@ static bool iplb_extended_needed(void *opaque)
>> return ipl->iplbext_migration;
>> }
>> +/* Start IPLB chain from the boundary of the first unused page
>> before BIOS */
>
> I'd maybe say "upper boundary" to make it clear that this is at the
> end of the page, not at the beginning?
The chain does start at the beginning of a page. That being said, the
comment still needs to be reworded, I'm just not sure exactly how.
"Start the IPLB chain from the nearest page boundary providing
sufficient space before BIOS?" Basically because each IPLB is 4K, the
chain will occupy the N unused pages before the start of BIOS, where N
is the number of chained IPLBS (assuming 4K pages).
>
>> +static uint64_t find_iplb_chain_addr(uint64_t bios_addr, uint16_t
>> count)
>> +{
>> + return (bios_addr & TARGET_PAGE_MASK)
>> + - (count * sizeof(IplParameterBlock));
>> +}
>> +
>> static const VMStateDescription vmstate_iplb_extended = {
>> .name = "ipl/iplb_extended",
>> .version_id = 0,
>> @@ -391,6 +398,17 @@ static CcwDevice
>> *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
>> return ccw_dev;
>> }
>> +static void s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
>> +{
>> + S390IPLState *ipl = get_ipl_device();
>> + uint16_t count = ipl->qipl.num_iplbs;
>> + uint64_t len = sizeof(IplParameterBlock) * count;
>> + uint64_t chain_addr = find_iplb_chain_addr(ipl->bios_start_addr,
>> count);
>> +
>> + cpu_physical_memory_write(chain_addr, iplb_chain,
>> be32_to_cpu(len));
>
> The be32_to_cpu looks wrong here, since you just computed len in
> native endianness.
>
I'll check it out.
>> + ipl->qipl.next_iplb = chain_addr;
>
> Just a matter of taste, but I'd prefer to set ipl->qipl.next_iplb in
> the same function where you set ipl->qipl.num_iplbs ... so I'd rather
> return chain_addr here and then do this on the calling site:
>
> ipl->qipl.next_iplb = s390_ipl_map_iplb_chain(...);
That should be doable; I'll change it for v2.
>> [snip...]
>>
>> @@ -487,6 +502,58 @@ static bool s390_gen_initial_iplb(S390IPLState
>> *ipl)
>> return false;
>> }
>> +static bool s390_init_all_iplbs(S390IPLState *ipl)
>> +{
>> + int iplb_num = 0;
>> + IplParameterBlock iplb_chain[7];
>> + DeviceState *dev_st = get_boot_device(0);
>> +
>> + /*
>> + * Parse the boot devices. Generate an IPLB for the first boot
>> device,
>> + * which will later be set with DIAG308. Index any fallback boot
>> devices.
>> + */
>> + if (!dev_st) {
>> + ipl->qipl.num_iplbs = 0;
>> + return false;
>> + }
>> +
>> + iplb_num = 1;
>> + s390_build_iplb(dev_st, &ipl->iplb);
>> + ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
>> +
>> + while (get_boot_device(iplb_num)) {
>> + iplb_num++;
>> + }
>> +
>> + ipl->qipl.num_iplbs = iplb_num - 1;
>
> It's somewhat confusing that ipl->qipl.num_iplbs is one less than
> iplb_num ... what does ipl->qipl.num_iplbs exactly define? The amount
> of additional chained devices beside the first one?
>
> A comment either here or qipl.h that describes the exact meaning of
> num_iplbs would be helpful.
>
>> +
>> + /*
>> + * Build fallback IPLBs for any boot devices above index 0, up to a
>> + * maximum amount as defined in ipl.h
>> + */
>> + if (iplb_num > 1) {
>> + if (iplb_num > MAX_IPLB_CHAIN) {
>> + warn_report("Excess boot devices defined! %d boot
>> devices found, "
>> + "but only the first %d will be considered.",
>> + iplb_num, MAX_IPLB_CHAIN + 1);
>> + iplb_num = MAX_IPLB_CHAIN + 1;
>
> What's now the real maximum number of iplb_num ? If it is
> MAX_IPLB_CHAIN + 1 then the if-statement above looks wrong, should it
> be "if (iplb_num > MAX_IPLB_CHAIN + 1)" instead?
>
>> + }
>> +
>> + ipl->qipl.num_iplbs = iplb_num - 1;
>
> You could move that into the body of the above if-statement, since
> otherwise the value has been set earlier in this function already.
>
>> + /* Start at 1 because the IPLB for boot index 0 is not
>> chained */
>> + for (int i = 1; i < iplb_num; i++) {
>
> Just to double-check: Is "i < iplb_num" right?
> Or should it be "i <= iplb_num" instead?
Sorry that I did not realize how poorly named the variables are here. I
will rename them in v2, which I think will clarify all of these
comments/questions (and you are correct that the warning message is off
by one), but in the meantime the gist of it is that ipl->qipl.num_iplbs
tracks the number of chained IPLBs only (so it is one less than the
total boot devices). MAX_IPLB_CHAIN should also probably be
MAX_BOOT_DEVS instead. I could change "num_iplbs" to "chained_iplbs"
and similarly "iplb_num" could be changed to "num_boot_devs" to make it
more clear what is going on:
ipl->qipl.chained_iplbs = num_boot_devs - 1;
>
> BTW, have you successfully tested booting with 8 devices that all have
> a boot index, but only the last one is bootable?
Yes. I will write an automated test to include in v2 also.
Regards,
Jared Rossi
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices
2024-06-05 20:01 ` Jared Rossi
@ 2024-06-07 6:11 ` Thomas Huth
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-07 6:11 UTC (permalink / raw)
To: Jared Rossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 05/06/2024 22.01, Jared Rossi wrote:
>
> On 6/4/24 2:26 PM, Thomas Huth wrote:
>> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>
>>> Write a chain of IPLBs into memory for future use.
>>>
>>> The IPLB chain is placed immediately before the BIOS in memory at the
>>> highest
>>> unused page boundary providing sufficient space to fit the chain. Because
>>> this
>>> is not a fixed address, the location of the next IPLB and number of
>>> remaining
>>> boot devices is stored in the QIPL global variable for later access.
>>>
>>> At this stage the IPLB chain is not accessed by the guest during IPL.
>>>
>>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>>> ---
>>> hw/s390x/ipl.h | 1 +
>>> include/hw/s390x/ipl/qipl.h | 4 +-
>>> hw/s390x/ipl.c | 129 +++++++++++++++++++++++++++---------
>>> 3 files changed, 103 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>>> index 1dcb8984bb..4f098d3a81 100644
>>> --- a/hw/s390x/ipl.h
>>> +++ b/hw/s390x/ipl.h
>>> @@ -20,6 +20,7 @@
>>> #include "qom/object.h"
>>> #define DIAG308_FLAGS_LP_VALID 0x80
>>> +#define MAX_IPLB_CHAIN 7
>>> void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
>>> void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
>>> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
>>> index a6ce6ddfe3..481c459a53 100644
>>> --- a/include/hw/s390x/ipl/qipl.h
>>> +++ b/include/hw/s390x/ipl/qipl.h
>>> @@ -34,7 +34,9 @@ struct QemuIplParameters {
>>> uint8_t reserved1[3];
>>> uint64_t netboot_start_addr;
>>> uint32_t boot_menu_timeout;
>>> - uint8_t reserved2[12];
>>> + uint8_t reserved2[2];
>>> + uint16_t num_iplbs;
>>> + uint64_t next_iplb;
>>> } QEMU_PACKED;
>>> typedef struct QemuIplParameters QemuIplParameters;
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 2d4f5152b3..79429acabd 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -55,6 +55,13 @@ static bool iplb_extended_needed(void *opaque)
>>> return ipl->iplbext_migration;
>>> }
>>> +/* Start IPLB chain from the boundary of the first unused page before
>>> BIOS */
>>
>> I'd maybe say "upper boundary" to make it clear that this is at the end of
>> the page, not at the beginning?
>
> The chain does start at the beginning of a page. That being said, the
> comment still needs to be reworded, I'm just not sure exactly how. "Start
> the IPLB chain from the nearest page boundary providing sufficient space
> before BIOS?" Basically because each IPLB is 4K, the chain will occupy the
> N unused pages before the start of BIOS, where N is the number of chained
> IPLBS (assuming 4K pages).
Ah, right, I missed that sizeof(IplParameterBlock) == 4096 (I guess I was
looking at the old version in pc-bios/s390-ccw/iplb.h that does not seem to
have the padding), sorry for the confusion! It's really good that you now
unify the headers in your first patch!
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 4/5] s390x: Add boot device fallback infrastructure
2024-05-29 15:43 [PATCH 0/5] s390x: Add Full Boot Order Support jrossi
` (2 preceding siblings ...)
2024-05-29 15:43 ` [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices jrossi
@ 2024-05-29 15:43 ` jrossi
2024-06-05 8:20 ` Thomas Huth
2024-05-29 15:43 ` [PATCH 5/5] s390x: Enable and document boot device fallback on panic jrossi
` (2 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: jrossi @ 2024-05-29 15:43 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth; +Cc: frankja, nsg, jrossi
From: Jared Rossi <jrossi@linux.ibm.com>
Add a routine for loading the next IPLB if a device fails to boot.
This includes some minor changes to the List-Directed IPL routine so that the
failing device may be retried using the legacy boot pointers before moving on to
the next device.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
pc-bios/s390-ccw/bootmap.h | 5 +++++
pc-bios/s390-ccw/iplb.h | 24 ++++++++++++++++++++++
pc-bios/s390-ccw/bootmap.c | 41 ++++++++++++++++++++++++++------------
pc-bios/s390-ccw/main.c | 15 +++++++++-----
pc-bios/s390-ccw/netmain.c | 4 ++++
5 files changed, 71 insertions(+), 18 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index d4690a88c2..d5061ed6c8 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -366,6 +366,11 @@ static inline void read_block(block_number_t blockno,
IPL_assert(virtio_read(blockno, buffer) == 0, errmsg);
}
+static inline bool read_block_nonfatal(block_number_t blockno, void *buffer)
+{
+ return (virtio_read(blockno, buffer) == 0);
+}
+
static inline bool block_size_ok(uint32_t block_size)
{
return block_size == virtio_get_block_size();
diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index 16643f5879..3c29d23375 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -49,4 +49,28 @@ static inline bool set_iplb(IplParameterBlock *iplb)
return manage_iplb(iplb, false);
}
+/*
+ * The IPL started on the device, but failed in some way. If the IPLB chain
+ * still has more devices left to try, use the next device in order. Set the
+ * next IPLB and save the current qipl parameters state.
+ */
+static inline bool load_next_iplb(void)
+{
+ IplParameterBlock *next_iplb;
+
+ if (qipl.num_iplbs < 1) {
+ return false;
+ }
+
+ next_iplb = (IplParameterBlock *) qipl.next_iplb;
+ memcpy(&iplb, next_iplb, sizeof(IplParameterBlock));
+ set_iplb(&iplb);
+
+ qipl.num_iplbs--;
+ qipl.next_iplb = qipl.next_iplb + sizeof(IplParameterBlock);
+ memcpy((QemuIplParameters *)QIPL_ADDRESS, &qipl, sizeof(QemuIplParameters));
+
+ return true;
+}
+
#endif /* IPLB_H */
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index a2137449dc..69391557fa 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -144,7 +144,10 @@ static block_number_t load_eckd_segments(block_number_t blk, bool ldipl,
bool more_data;
memset(_bprs, FREE_SPACE_FILLER, sizeof(_bprs));
- read_block(blk, bprs, "BPRS read failed");
+ if (!read_block_nonfatal(blk, bprs)) {
+ IPL_assert(ldipl, "BPRS read failed");
+ return -1;
+ }
do {
more_data = false;
@@ -188,7 +191,10 @@ static block_number_t load_eckd_segments(block_number_t blk, bool ldipl,
* I.e. the next ptr must point to the unused memory area
*/
memset(_bprs, FREE_SPACE_FILLER, sizeof(_bprs));
- read_block(block_nr, bprs, "BPRS continuation read failed");
+ if (!read_block_nonfatal(block_nr, bprs)) {
+ IPL_assert(ldipl, "BPRS continuation read failed");
+ break;
+ }
more_data = true;
break;
}
@@ -197,7 +203,10 @@ static block_number_t load_eckd_segments(block_number_t blk, bool ldipl,
* to memory (address).
*/
rc = virtio_read_many(block_nr, (void *)(*address), count + 1);
- IPL_assert(rc == 0, "code chunk read failed");
+ if (rc != 0) {
+ IPL_assert(ldipl, "code chunk read failed");
+ break;
+ }
*address += (count + 1) * virtio_get_block_size();
}
@@ -295,13 +304,22 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
" maximum number of boot entries allowed");
memset(sec, FREE_SPACE_FILLER, sizeof(sec));
- read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
+ if (!read_block_nonfatal(bmt_block_nr, sec)) {
+ IPL_assert(ldipl, "Cannot read Boot Map Table");
+ return;
+ }
block_nr = gen_eckd_block_num(&bmt->entry[loadparm].xeckd, ldipl);
- IPL_assert(block_nr != -1, "Cannot find Boot Map Table Entry");
+ if (block_nr == -1) {
+ IPL_assert(ldipl, "Cannot find Boot Map Table Entry");
+ return;
+ }
memset(sec, FREE_SPACE_FILLER, sizeof(sec));
- read_block(block_nr, sec, "Cannot read Boot Map Script");
+ if (!read_block_nonfatal(block_nr, sec)) {
+ IPL_assert(ldipl, "Cannot read Boot Map Script");
+ return;
+ }
for (i = 0; bms->entry[i].type == BOOT_SCRIPT_LOAD ||
bms->entry[i].type == BOOT_SCRIPT_SIGNATURE; i++) {
@@ -319,13 +337,10 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
} while (block_nr != -1);
}
- if (ldipl && bms->entry[i].type != BOOT_SCRIPT_EXEC) {
- /* Abort LD-IPL and retry as CCW-IPL */
+ if (bms->entry[i].type != BOOT_SCRIPT_EXEC) {
+ IPL_assert(ldipl, "Unknown script entry type");
return;
}
-
- IPL_assert(bms->entry[i].type == BOOT_SCRIPT_EXEC,
- "Unknown script entry type");
write_reset_psw(bms->entry[i].address.load_address); /* no return */
jump_to_IPL_code(0); /* no return */
}
@@ -492,7 +507,7 @@ static void ipl_eckd(void)
/* LD-IPL does not use the S1B bock, just make it NULL */
run_eckd_boot_script(ldipl_bmt, NULL_BLOCK_NR);
/* Only return in error, retry as CCW-IPL */
- sclp_print("Retrying IPL ");
+ sclp_print("LD-IPL failed, retrying device\n");
print_eckd_msg();
}
memset(sec, FREE_SPACE_FILLER, sizeof(sec));
@@ -944,5 +959,5 @@ void zipl_load(void)
panic("\n! Unknown IPL device type !\n");
}
- sclp_print("zIPL load failed.\n");
+ panic("zIPL load failed.\n");
}
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 3e51d698d7..248ed5a410 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -53,6 +53,12 @@ unsigned int get_loadparm_index(void)
return atoui(loadparm_str);
}
+static void copy_qipl(void)
+{
+ QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
+ memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
+}
+
static int is_dev_possibly_bootable(int dev_no, int sch_no)
{
bool is_virtio;
@@ -194,7 +200,7 @@ static void boot_setup(void)
static void find_boot_device(void)
{
VDev *vdev = virtio_get_device();
- bool found;
+ bool found = false;
switch (iplb.pbt) {
case S390_IPL_TYPE_CCW:
@@ -221,11 +227,8 @@ static void find_boot_device(void)
static int virtio_setup(void)
{
VDev *vdev = virtio_get_device();
- QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
int ret;
- memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
-
if (have_iplb) {
menu_setup();
}
@@ -242,7 +245,8 @@ static int virtio_setup(void)
ret = virtio_scsi_setup_device(blk_schid);
break;
default:
- panic("\n! No IPL device available !\n");
+ ret = 1;
+ panic("Unrecognized virtio device type!\n");
}
if (!ret) {
@@ -296,6 +300,7 @@ static void probe_boot_device(void)
void main(void)
{
+ copy_qipl();
sclp_setup();
css_setup();
boot_setup();
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index 5cd619b2d6..65cee15fef 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -36,6 +36,7 @@
#include "cio.h"
#include "virtio.h"
#include "s390-time.h"
+#include "iplb.h"
#define DEFAULT_BOOT_RETRIES 10
#define DEFAULT_TFTP_RETRIES 20
@@ -51,6 +52,7 @@ void write_iplb_location(void) {}
#define STSI322_VMDB_UUID_OFFSET ((8 + 12) * 4)
IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
+QemuIplParameters qipl;
static char cfgbuf[2048];
static SubChannelId net_schid = { .one = 1 };
@@ -513,6 +515,8 @@ void main(void)
{
filename_ip_t fn_ip;
int rc, fnlen;
+ QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
+ memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
sclp_setup();
sclp_print("Network boot starting...\n");
--
2.45.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 4/5] s390x: Add boot device fallback infrastructure
2024-05-29 15:43 ` [PATCH 4/5] s390x: Add boot device fallback infrastructure jrossi
@ 2024-06-05 8:20 ` Thomas Huth
2024-06-05 12:13 ` Thomas Huth
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Huth @ 2024-06-05 8:20 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Add a routine for loading the next IPLB if a device fails to boot.
>
> This includes some minor changes to the List-Directed IPL routine so that the
> failing device may be retried using the legacy boot pointers before moving on to
> the next device.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
...
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index a2137449dc..69391557fa 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -144,7 +144,10 @@ static block_number_t load_eckd_segments(block_number_t blk, bool ldipl,
> bool more_data;
>
> memset(_bprs, FREE_SPACE_FILLER, sizeof(_bprs));
> - read_block(blk, bprs, "BPRS read failed");
> + if (!read_block_nonfatal(blk, bprs)) {
> + IPL_assert(ldipl, "BPRS read failed");
> + return -1;
> + }
>
> do {
> more_data = false;
> @@ -188,7 +191,10 @@ static block_number_t load_eckd_segments(block_number_t blk, bool ldipl,
> * I.e. the next ptr must point to the unused memory area
> */
> memset(_bprs, FREE_SPACE_FILLER, sizeof(_bprs));
> - read_block(block_nr, bprs, "BPRS continuation read failed");
> + if (!read_block_nonfatal(block_nr, bprs)) {
> + IPL_assert(ldipl, "BPRS continuation read failed");
> + break;
> + }
> more_data = true;
> break;
> }
> @@ -197,7 +203,10 @@ static block_number_t load_eckd_segments(block_number_t blk, bool ldipl,
> * to memory (address).
> */
> rc = virtio_read_many(block_nr, (void *)(*address), count + 1);
> - IPL_assert(rc == 0, "code chunk read failed");
> + if (rc != 0) {
> + IPL_assert(ldipl, "code chunk read failed");
> + break;
> + }
>
> *address += (count + 1) * virtio_get_block_size();
> }
> @@ -295,13 +304,22 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
> " maximum number of boot entries allowed");
>
> memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> - read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
> + if (!read_block_nonfatal(bmt_block_nr, sec)) {
> + IPL_assert(ldipl, "Cannot read Boot Map Table");
> + return;
> + }
>
> block_nr = gen_eckd_block_num(&bmt->entry[loadparm].xeckd, ldipl);
> - IPL_assert(block_nr != -1, "Cannot find Boot Map Table Entry");
> + if (block_nr == -1) {
> + IPL_assert(ldipl, "Cannot find Boot Map Table Entry");
> + return;
> + }
>
> memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> - read_block(block_nr, sec, "Cannot read Boot Map Script");
> + if (!read_block_nonfatal(block_nr, sec)) {
> + IPL_assert(ldipl, "Cannot read Boot Map Script");
> + return;
> + }
>
> for (i = 0; bms->entry[i].type == BOOT_SCRIPT_LOAD ||
> bms->entry[i].type == BOOT_SCRIPT_SIGNATURE; i++) {
> @@ -319,13 +337,10 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
> } while (block_nr != -1);
> }
>
> - if (ldipl && bms->entry[i].type != BOOT_SCRIPT_EXEC) {
> - /* Abort LD-IPL and retry as CCW-IPL */
> + if (bms->entry[i].type != BOOT_SCRIPT_EXEC) {
> + IPL_assert(ldipl, "Unknown script entry type");
> return;
> }
> -
> - IPL_assert(bms->entry[i].type == BOOT_SCRIPT_EXEC,
> - "Unknown script entry type");
> write_reset_psw(bms->entry[i].address.load_address); /* no return */
> jump_to_IPL_code(0); /* no return */
> }
> @@ -492,7 +507,7 @@ static void ipl_eckd(void)
> /* LD-IPL does not use the S1B bock, just make it NULL */
> run_eckd_boot_script(ldipl_bmt, NULL_BLOCK_NR);
> /* Only return in error, retry as CCW-IPL */
> - sclp_print("Retrying IPL ");
> + sclp_print("LD-IPL failed, retrying device\n");
> print_eckd_msg();
> }
> memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> @@ -944,5 +959,5 @@ void zipl_load(void)
> panic("\n! Unknown IPL device type !\n");
> }
>
> - sclp_print("zIPL load failed.\n");
> + panic("zIPL load failed.\n");
Why replacing the sclp_print() here? Wouldn't it be nicer to continue
panicking on the calling site instead?
> }
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 3e51d698d7..248ed5a410 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -53,6 +53,12 @@ unsigned int get_loadparm_index(void)
> return atoui(loadparm_str);
> }
>
> +static void copy_qipl(void)
> +{
> + QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
> + memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
> +}
You could move this function as a static inline into iplb.h ...
...
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index 5cd619b2d6..65cee15fef 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -36,6 +36,7 @@
> #include "cio.h"
> #include "virtio.h"
> #include "s390-time.h"
> +#include "iplb.h"
>
> #define DEFAULT_BOOT_RETRIES 10
> #define DEFAULT_TFTP_RETRIES 20
> @@ -51,6 +52,7 @@ void write_iplb_location(void) {}
> #define STSI322_VMDB_UUID_OFFSET ((8 + 12) * 4)
>
> IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
> +QemuIplParameters qipl;
> static char cfgbuf[2048];
>
> static SubChannelId net_schid = { .one = 1 };
> @@ -513,6 +515,8 @@ void main(void)
> {
> filename_ip_t fn_ip;
> int rc, fnlen;
> + QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
> + memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
... then you could use copy_qipl() here, too.
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 4/5] s390x: Add boot device fallback infrastructure
2024-06-05 8:20 ` Thomas Huth
@ 2024-06-05 12:13 ` Thomas Huth
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-05 12:13 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 05/06/2024 10.20, Thomas Huth wrote:
> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> Add a routine for loading the next IPLB if a device fails to boot.
>>
>> This includes some minor changes to the List-Directed IPL routine so that the
>> failing device may be retried using the legacy boot pointers before moving
>> on to
>> the next device.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
> ...
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index a2137449dc..69391557fa 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -144,7 +144,10 @@ static block_number_t
>> load_eckd_segments(block_number_t blk, bool ldipl,
>> bool more_data;
>> memset(_bprs, FREE_SPACE_FILLER, sizeof(_bprs));
>> - read_block(blk, bprs, "BPRS read failed");
>> + if (!read_block_nonfatal(blk, bprs)) {
>> + IPL_assert(ldipl, "BPRS read failed");
>> + return -1;
>> + }
>> do {
>> more_data = false;
>> @@ -188,7 +191,10 @@ static block_number_t
>> load_eckd_segments(block_number_t blk, bool ldipl,
>> * I.e. the next ptr must point to the unused memory area
>> */
>> memset(_bprs, FREE_SPACE_FILLER, sizeof(_bprs));
>> - read_block(block_nr, bprs, "BPRS continuation read failed");
>> + if (!read_block_nonfatal(block_nr, bprs)) {
>> + IPL_assert(ldipl, "BPRS continuation read failed");
>> + break;
>> + }
>> more_data = true;
>> break;
>> }
>> @@ -197,7 +203,10 @@ static block_number_t
>> load_eckd_segments(block_number_t blk, bool ldipl,
>> * to memory (address).
>> */
>> rc = virtio_read_many(block_nr, (void *)(*address), count + 1);
>> - IPL_assert(rc == 0, "code chunk read failed");
>> + if (rc != 0) {
>> + IPL_assert(ldipl, "code chunk read failed");
>> + break;
>> + }
>> *address += (count + 1) * virtio_get_block_size();
>> }
>> @@ -295,13 +304,22 @@ static void run_eckd_boot_script(block_number_t
>> bmt_block_nr,
>> " maximum number of boot entries allowed");
>> memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>> - read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
>> + if (!read_block_nonfatal(bmt_block_nr, sec)) {
>> + IPL_assert(ldipl, "Cannot read Boot Map Table");
>> + return;
>> + }
>> block_nr = gen_eckd_block_num(&bmt->entry[loadparm].xeckd, ldipl);
>> - IPL_assert(block_nr != -1, "Cannot find Boot Map Table Entry");
>> + if (block_nr == -1) {
>> + IPL_assert(ldipl, "Cannot find Boot Map Table Entry");
>> + return;
>> + }
>> memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>> - read_block(block_nr, sec, "Cannot read Boot Map Script");
>> + if (!read_block_nonfatal(block_nr, sec)) {
>> + IPL_assert(ldipl, "Cannot read Boot Map Script");
>> + return;
>> + }
>> for (i = 0; bms->entry[i].type == BOOT_SCRIPT_LOAD ||
>> bms->entry[i].type == BOOT_SCRIPT_SIGNATURE; i++) {
>> @@ -319,13 +337,10 @@ static void run_eckd_boot_script(block_number_t
>> bmt_block_nr,
>> } while (block_nr != -1);
>> }
>> - if (ldipl && bms->entry[i].type != BOOT_SCRIPT_EXEC) {
>> - /* Abort LD-IPL and retry as CCW-IPL */
>> + if (bms->entry[i].type != BOOT_SCRIPT_EXEC) {
>> + IPL_assert(ldipl, "Unknown script entry type");
>> return;
>> }
>> -
>> - IPL_assert(bms->entry[i].type == BOOT_SCRIPT_EXEC,
>> - "Unknown script entry type");
>> write_reset_psw(bms->entry[i].address.load_address); /* no return */
>> jump_to_IPL_code(0); /* no return */
>> }
>> @@ -492,7 +507,7 @@ static void ipl_eckd(void)
>> /* LD-IPL does not use the S1B bock, just make it NULL */
>> run_eckd_boot_script(ldipl_bmt, NULL_BLOCK_NR);
>> /* Only return in error, retry as CCW-IPL */
>> - sclp_print("Retrying IPL ");
>> + sclp_print("LD-IPL failed, retrying device\n");
>> print_eckd_msg();
>> }
>> memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>> @@ -944,5 +959,5 @@ void zipl_load(void)
>> panic("\n! Unknown IPL device type !\n");
>> }
>> - sclp_print("zIPL load failed.\n");
>> + panic("zIPL load failed.\n");
>
> Why replacing the sclp_print() here? Wouldn't it be nicer to continue
> panicking on the calling site instead?
Ok, after looking at the 5th patch, I think I understand it now: panic() is
not fatal anymore and might restart with the next boot device... not sure
whether I like that, but let's discuss that on patch 5 instead...
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 5/5] s390x: Enable and document boot device fallback on panic
2024-05-29 15:43 [PATCH 0/5] s390x: Add Full Boot Order Support jrossi
` (3 preceding siblings ...)
2024-05-29 15:43 ` [PATCH 4/5] s390x: Add boot device fallback infrastructure jrossi
@ 2024-05-29 15:43 ` jrossi
2024-06-05 13:37 ` Thomas Huth
2024-06-04 18:35 ` [PATCH 0/5] s390x: Add Full Boot Order Support Thomas Huth
2024-06-05 8:02 ` Thomas Huth
6 siblings, 1 reply; 29+ messages in thread
From: jrossi @ 2024-05-29 15:43 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth; +Cc: frankja, nsg, jrossi
From: Jared Rossi <jrossi@linux.ibm.com>
On a panic during IPL (i.e. a device failed to boot) check for another device
to boot from, as indicated by the presence of an unused IPLB.
If an IPLB is successfully loaded, then jump to the start of BIOS, restarting
IPL using the updated IPLB. Otherwise enter disabled wait.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
docs/system/bootindex.rst | 7 ++++---
docs/system/s390x/bootdevices.rst | 9 ++++++---
pc-bios/s390-ccw/s390-ccw.h | 6 ++++++
3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/docs/system/bootindex.rst b/docs/system/bootindex.rst
index 8b057f812f..de597561bd 100644
--- a/docs/system/bootindex.rst
+++ b/docs/system/bootindex.rst
@@ -50,9 +50,10 @@ Limitations
Some firmware has limitations on which devices can be considered for
booting. For instance, the PC BIOS boot specification allows only one
-disk to be bootable. If boot from disk fails for some reason, the BIOS
-won't retry booting from other disk. It can still try to boot from
-floppy or net, though.
+disk to be bootable, except for on s390x machines. If boot from disk fails for
+some reason, the BIOS won't retry booting from other disk. It can still try to
+boot from floppy or net, though. In the case of s390x, the BIOS will try up to
+8 total devices, any number of which may be disks.
Sometimes, firmware cannot map the device path QEMU wants firmware to
boot from to a boot method. It doesn't happen for devices the firmware
diff --git a/docs/system/s390x/bootdevices.rst b/docs/system/s390x/bootdevices.rst
index 1a7a18b43b..f096e1cc06 100644
--- a/docs/system/s390x/bootdevices.rst
+++ b/docs/system/s390x/bootdevices.rst
@@ -6,9 +6,7 @@ Booting with bootindex parameter
For classical mainframe guests (i.e. LPAR or z/VM installations), you always
have to explicitly specify the disk where you want to boot from (or "IPL" from,
-in s390x-speak -- IPL means "Initial Program Load"). In particular, there can
-also be only one boot device according to the architecture specification, thus
-specifying multiple boot devices is not possible (yet).
+in s390x-speak -- IPL means "Initial Program Load").
So for booting an s390x guest in QEMU, you should always mark the
device where you want to boot from with the ``bootindex`` property, for
@@ -17,6 +15,11 @@ example::
qemu-system-s390x -drive if=none,id=dr1,file=guest.qcow2 \
-device virtio-blk,drive=dr1,bootindex=1
+Multiple devices may have a bootindex. The lowest bootindex is assigned to the
+device to IPL first. If the IPL fails for the first, the device with the second
+lowest bootindex will be tried and so on until IPL is successful or there are no
+remaining boot devices to try.
+
For booting from a CD-ROM ISO image (which needs to include El-Torito boot
information in order to be bootable), it is recommended to specify a ``scsi-cd``
device, for example like this::
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index c977a52b50..de3d1f0d5a 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -43,6 +43,7 @@ typedef unsigned long long u64;
#include "iplb.h"
/* start.s */
+extern char _start[];
void disabled_wait(void) __attribute__ ((__noreturn__));
void consume_sclp_int(void);
void consume_io_int(void);
@@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
static inline void panic(const char *string)
{
sclp_print(string);
+ if (load_next_iplb()) {
+ sclp_print("\nTrying next boot device...");
+ jump_to_IPL_code((long)_start);
+ }
+
disabled_wait();
}
--
2.45.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic
2024-05-29 15:43 ` [PATCH 5/5] s390x: Enable and document boot device fallback on panic jrossi
@ 2024-06-05 13:37 ` Thomas Huth
2024-06-05 14:48 ` Jared Rossi
2024-06-17 14:49 ` Christian Borntraeger
0 siblings, 2 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-05 13:37 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg, Christian Borntraeger
On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> On a panic during IPL (i.e. a device failed to boot) check for another device
> to boot from, as indicated by the presence of an unused IPLB.
>
> If an IPLB is successfully loaded, then jump to the start of BIOS, restarting
> IPL using the updated IPLB. Otherwise enter disabled wait.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
> docs/system/bootindex.rst | 7 ++++---
> docs/system/s390x/bootdevices.rst | 9 ++++++---
> pc-bios/s390-ccw/s390-ccw.h | 6 ++++++
> 3 files changed, 16 insertions(+), 6 deletions(-)
Could you please split the documentation changes into a separate patch in v2
? ... I think that would be cleaner.
> diff --git a/docs/system/bootindex.rst b/docs/system/bootindex.rst
> index 8b057f812f..de597561bd 100644
> --- a/docs/system/bootindex.rst
> +++ b/docs/system/bootindex.rst
> @@ -50,9 +50,10 @@ Limitations
>
> Some firmware has limitations on which devices can be considered for
> booting. For instance, the PC BIOS boot specification allows only one
> -disk to be bootable. If boot from disk fails for some reason, the BIOS
> -won't retry booting from other disk. It can still try to boot from
> -floppy or net, though.
> +disk to be bootable, except for on s390x machines. If boot from disk fails for
> +some reason, the BIOS won't retry booting from other disk. It can still try to
> +boot from floppy or net, though. In the case of s390x, the BIOS will try up to
> +8 total devices, any number of which may be disks.
Since the old text was already talking about "PC BIOS", I'd rather leave
that paragraph as it is (maybe just replace "PC BIOS" with "x86 PC BIOS"),
and add a separate paragraph afterwards about s390x instead.
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index c977a52b50..de3d1f0d5a 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -43,6 +43,7 @@ typedef unsigned long long u64;
> #include "iplb.h"
>
> /* start.s */
> +extern char _start[];
> void disabled_wait(void) __attribute__ ((__noreturn__));
> void consume_sclp_int(void);
> void consume_io_int(void);
> @@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
> static inline void panic(const char *string)
> {
> sclp_print(string);
> + if (load_next_iplb()) {
> + sclp_print("\nTrying next boot device...");
> + jump_to_IPL_code((long)_start);
> + }
> +
> disabled_wait();
> }
Honestly, I am unsure whether this is a really cool idea or a very ugly hack
... but I think I tend towards the latter, sorry. Jumping back to the
startup code might cause various problem, e.g. pre-initialized variables
don't get their values reset, causing different behavior when the s390-ccw
bios runs a function a second time this way. Thus this sounds very fragile.
Could we please try to get things cleaned up correctly, so that functions
return with error codes instead of panicking when we can continue with
another boot device? Even if its more work right now, I think this will be
much more maintainable in the future.
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic
2024-06-05 13:37 ` Thomas Huth
@ 2024-06-05 14:48 ` Jared Rossi
2024-06-07 5:57 ` Thomas Huth
2024-06-17 14:49 ` Christian Borntraeger
1 sibling, 1 reply; 29+ messages in thread
From: Jared Rossi @ 2024-06-05 14:48 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x; +Cc: frankja, nsg, Christian Borntraeger
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index c977a52b50..de3d1f0d5a 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -43,6 +43,7 @@ typedef unsigned long long u64;
>> #include "iplb.h"
>> /* start.s */
>> +extern char _start[];
>> void disabled_wait(void) __attribute__ ((__noreturn__));
>> void consume_sclp_int(void);
>> void consume_io_int(void);
>> @@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
>> static inline void panic(const char *string)
>> {
>> sclp_print(string);
>> + if (load_next_iplb()) {
>> + sclp_print("\nTrying next boot device...");
>> + jump_to_IPL_code((long)_start);
>> + }
>> +
>> disabled_wait();
>> }
>
> Honestly, I am unsure whether this is a really cool idea or a very
> ugly hack ... but I think I tend towards the latter, sorry. Jumping
> back to the startup code might cause various problem, e.g.
> pre-initialized variables don't get their values reset, causing
> different behavior when the s390-ccw bios runs a function a second
> time this way. Thus this sounds very fragile. Could we please try to
> get things cleaned up correctly, so that functions return with error
> codes instead of panicking when we can continue with another boot
> device? Even if its more work right now, I think this will be much
> more maintainable in the future.
>
> Thomas
>
Thanks Thomas, I appreciate your insight. Your hesitation is perfectly
understandable as well. My initial design was like you suggest, where
the functions return instead of panic, but the issue I ran into is that
netboot uses a separate image, which we jump in to at the start of IPL
from a network device (see zipl_load() in pc-bios/s390-ccw/bootmap.c).
I wasn't able to come up with a simple way to return to the main BIOS
code if a netboot fails other than by jumping back. So, it seems to me
that netboot kind of throws a monkeywrench into the basic idea of
reworking the panics into returns.
I'm open to suggestions on a better way to recover from a failed
netboot, and it's certainly possible I've overlooked something, but as
far as I can tell a jump is necessary in that particular case at least.
Netboot could perhaps be handled as a special case where the jump back
is permitted whereas other device types return, but I don't think that
actually solves the main issue.
What are your thoughts on this?
Thanks,
Jared Rossi
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic
2024-06-05 14:48 ` Jared Rossi
@ 2024-06-07 5:57 ` Thomas Huth
2024-06-16 23:44 ` Jared Rossi
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Huth @ 2024-06-07 5:57 UTC (permalink / raw)
To: Jared Rossi, qemu-devel, qemu-s390x, Christian Borntraeger; +Cc: frankja, nsg
On 05/06/2024 16.48, Jared Rossi wrote:
>
>>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>>> index c977a52b50..de3d1f0d5a 100644
>>> --- a/pc-bios/s390-ccw/s390-ccw.h
>>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>>> @@ -43,6 +43,7 @@ typedef unsigned long long u64;
>>> #include "iplb.h"
>>> /* start.s */
>>> +extern char _start[];
>>> void disabled_wait(void) __attribute__ ((__noreturn__));
>>> void consume_sclp_int(void);
>>> void consume_io_int(void);
>>> @@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
>>> static inline void panic(const char *string)
>>> {
>>> sclp_print(string);
>>> + if (load_next_iplb()) {
>>> + sclp_print("\nTrying next boot device...");
>>> + jump_to_IPL_code((long)_start);
>>> + }
>>> +
>>> disabled_wait();
>>> }
>>
>> Honestly, I am unsure whether this is a really cool idea or a very ugly
>> hack ... but I think I tend towards the latter, sorry. Jumping back to the
>> startup code might cause various problem, e.g. pre-initialized variables
>> don't get their values reset, causing different behavior when the s390-ccw
>> bios runs a function a second time this way. Thus this sounds very
>> fragile. Could we please try to get things cleaned up correctly, so that
>> functions return with error codes instead of panicking when we can
>> continue with another boot device? Even if its more work right now, I
>> think this will be much more maintainable in the future.
>>
>> Thomas
>>
>
> Thanks Thomas, I appreciate your insight. Your hesitation is perfectly
> understandable as well. My initial design was like you suggest, where the
> functions return instead of panic, but the issue I ran into is that netboot
> uses a separate image, which we jump in to at the start of IPL from a
> network device (see zipl_load() in pc-bios/s390-ccw/bootmap.c). I wasn't
> able to come up with a simple way to return to the main BIOS code if a
> netboot fails other than by jumping back. So, it seems to me that netboot
> kind of throws a monkeywrench into the basic idea of reworking the panics
> into returns.
>
> I'm open to suggestions on a better way to recover from a failed netboot,
> and it's certainly possible I've overlooked something, but as far as I can
> tell a jump is necessary in that particular case at least. Netboot could
> perhaps be handled as a special case where the jump back is permitted
> whereas other device types return, but I don't think that actually solves
> the main issue.
>
> What are your thoughts on this?
Yes, I agree that jumping is currently required to get back from the netboot
code. So if you could rework your patches in a way that limits the jumping
to a failed netboot, that would be acceptable, I think.
Apart from that: We originally decided to put the netboot code into a
separate binary since the required roms/SLOF module might not always have
been checked out (it needed to be done manually), so we were not able to
compile it in all cases. But nowadays, this is handled in a much nicer way,
the submodule is automatically checked out once you compile the
s390x-softmmu target and have a s390x compiler available, so I wonder
whether we should maybe do the next step and integrate the netboot code into
the main s390-ccw.img now? Anybody got an opinion on this?
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic
2024-06-07 5:57 ` Thomas Huth
@ 2024-06-16 23:44 ` Jared Rossi
2024-06-20 8:10 ` Thomas Huth
0 siblings, 1 reply; 29+ messages in thread
From: Jared Rossi @ 2024-06-16 23:44 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x, Christian Borntraeger; +Cc: frankja, nsg
On 6/7/24 1:57 AM, Thomas Huth wrote:
> On 05/06/2024 16.48, Jared Rossi wrote:
>>
>>>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>>>> index c977a52b50..de3d1f0d5a 100644
>>>> --- a/pc-bios/s390-ccw/s390-ccw.h
>>>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>>>> @@ -43,6 +43,7 @@ typedef unsigned long long u64;
>>>> #include "iplb.h"
>>>> /* start.s */
>>>> +extern char _start[];
>>>> void disabled_wait(void) __attribute__ ((__noreturn__));
>>>> void consume_sclp_int(void);
>>>> void consume_io_int(void);
>>>> @@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
>>>> static inline void panic(const char *string)
>>>> {
>>>> sclp_print(string);
>>>> + if (load_next_iplb()) {
>>>> + sclp_print("\nTrying next boot device...");
>>>> + jump_to_IPL_code((long)_start);
>>>> + }
>>>> +
>>>> disabled_wait();
>>>> }
>>>
>>> Honestly, I am unsure whether this is a really cool idea or a very
>>> ugly hack ... but I think I tend towards the latter, sorry. Jumping
>>> back to the startup code might cause various problem, e.g.
>>> pre-initialized variables don't get their values reset, causing
>>> different behavior when the s390-ccw bios runs a function a second
>>> time this way. Thus this sounds very fragile. Could we please try to
>>> get things cleaned up correctly, so that functions return with error
>>> codes instead of panicking when we can continue with another boot
>>> device? Even if its more work right now, I think this will be much
>>> more maintainable in the future.
>>>
>>> Thomas
>>>
>>
>> Thanks Thomas, I appreciate your insight. Your hesitation is
>> perfectly understandable as well. My initial design was like you
>> suggest, where the functions return instead of panic, but the issue I
>> ran into is that netboot uses a separate image, which we jump in to
>> at the start of IPL from a network device (see zipl_load() in
>> pc-bios/s390-ccw/bootmap.c). I wasn't able to come up with a simple
>> way to return to the main BIOS code if a netboot fails other than by
>> jumping back. So, it seems to me that netboot kind of throws a
>> monkeywrench into the basic idea of reworking the panics into returns.
>>
>> I'm open to suggestions on a better way to recover from a failed
>> netboot, and it's certainly possible I've overlooked something, but
>> as far as I can tell a jump is necessary in that particular case at
>> least. Netboot could perhaps be handled as a special case where the
>> jump back is permitted whereas other device types return, but I don't
>> think that actually solves the main issue.
>>
>> What are your thoughts on this?
>
> Yes, I agree that jumping is currently required to get back from the
> netboot code. So if you could rework your patches in a way that limits
> the jumping to a failed netboot, that would be acceptable, I think.
>
> Apart from that: We originally decided to put the netboot code into a
> separate binary since the required roms/SLOF module might not always
> have been checked out (it needed to be done manually), so we were not
> able to compile it in all cases. But nowadays, this is handled in a
> much nicer way, the submodule is automatically checked out once you
> compile the s390x-softmmu target and have a s390x compiler available,
> so I wonder whether we should maybe do the next step and integrate the
> netboot code into the main s390-ccw.img now? Anybody got an opinion on
> this?
>
> Thomas
>
Hi Thomas,
I would generally defer the decision about integrating the netboot code
to someone with more insight than me, but for what it's worth, I am of
the opinion that if we want to rework all of panics into returns, then
it would make the most sense to also do the integration now so that we
can avoid using jump altogether. Unless I'm missing something simple, I
don't think the panic/return conversion will be trivial, and actually I
think it will be quite invasive since there are dozens of calls to panic
and assert that will need to be changed. It doesn't seem worthwhile to
do all of these conversions in order to avoid using jump, but then still
being exposed to possible problems caused by jumping due to netboot
requiring it anyway.
Regards,
Jared Rossi
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic
2024-06-16 23:44 ` Jared Rossi
@ 2024-06-20 8:10 ` Thomas Huth
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-20 8:10 UTC (permalink / raw)
To: Jared Rossi, qemu-devel, qemu-s390x, Christian Borntraeger; +Cc: frankja, nsg
On 17/06/2024 01.44, Jared Rossi wrote:
>
>
> On 6/7/24 1:57 AM, Thomas Huth wrote:
>> On 05/06/2024 16.48, Jared Rossi wrote:
>>>
>>>>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>>>>> index c977a52b50..de3d1f0d5a 100644
>>>>> --- a/pc-bios/s390-ccw/s390-ccw.h
>>>>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>>>>> @@ -43,6 +43,7 @@ typedef unsigned long long u64;
>>>>> #include "iplb.h"
>>>>> /* start.s */
>>>>> +extern char _start[];
>>>>> void disabled_wait(void) __attribute__ ((__noreturn__));
>>>>> void consume_sclp_int(void);
>>>>> void consume_io_int(void);
>>>>> @@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
>>>>> static inline void panic(const char *string)
>>>>> {
>>>>> sclp_print(string);
>>>>> + if (load_next_iplb()) {
>>>>> + sclp_print("\nTrying next boot device...");
>>>>> + jump_to_IPL_code((long)_start);
>>>>> + }
>>>>> +
>>>>> disabled_wait();
>>>>> }
>>>>
>>>> Honestly, I am unsure whether this is a really cool idea or a very ugly
>>>> hack ... but I think I tend towards the latter, sorry. Jumping back to
>>>> the startup code might cause various problem, e.g. pre-initialized
>>>> variables don't get their values reset, causing different behavior when
>>>> the s390-ccw bios runs a function a second time this way. Thus this
>>>> sounds very fragile. Could we please try to get things cleaned up
>>>> correctly, so that functions return with error codes instead of
>>>> panicking when we can continue with another boot device? Even if its
>>>> more work right now, I think this will be much more maintainable in the
>>>> future.
>>>>
>>>> Thomas
>>>>
>>>
>>> Thanks Thomas, I appreciate your insight. Your hesitation is perfectly
>>> understandable as well. My initial design was like you suggest, where
>>> the functions return instead of panic, but the issue I ran into is that
>>> netboot uses a separate image, which we jump in to at the start of IPL
>>> from a network device (see zipl_load() in pc-bios/s390-ccw/bootmap.c). I
>>> wasn't able to come up with a simple way to return to the main BIOS code
>>> if a netboot fails other than by jumping back. So, it seems to me that
>>> netboot kind of throws a monkeywrench into the basic idea of reworking
>>> the panics into returns.
>>>
>>> I'm open to suggestions on a better way to recover from a failed netboot,
>>> and it's certainly possible I've overlooked something, but as far as I
>>> can tell a jump is necessary in that particular case at least. Netboot
>>> could perhaps be handled as a special case where the jump back is
>>> permitted whereas other device types return, but I don't think that
>>> actually solves the main issue.
>>>
>>> What are your thoughts on this?
>>
>> Yes, I agree that jumping is currently required to get back from the
>> netboot code. So if you could rework your patches in a way that limits the
>> jumping to a failed netboot, that would be acceptable, I think.
>>
>> Apart from that: We originally decided to put the netboot code into a
>> separate binary since the required roms/SLOF module might not always have
>> been checked out (it needed to be done manually), so we were not able to
>> compile it in all cases. But nowadays, this is handled in a much nicer
>> way, the submodule is automatically checked out once you compile the
>> s390x-softmmu target and have a s390x compiler available, so I wonder
>> whether we should maybe do the next step and integrate the netboot code
>> into the main s390-ccw.img now? Anybody got an opinion on this?
>>
>> Thomas
>>
>
> Hi Thomas,
>
> I would generally defer the decision about integrating the netboot code to
> someone with more insight than me, but for what it's worth, I am of the
> opinion that if we want to rework all of panics into returns, then it would
> make the most sense to also do the integration now so that we can avoid
> using jump altogether. Unless I'm missing something simple, I don't think
> the panic/return conversion will be trivial, and actually I think it will be
> quite invasive since there are dozens of calls to panic and assert that will
> need to be changed. It doesn't seem worthwhile to do all of these
> conversions in order to avoid using jump, but then still being exposed to
> possible problems caused by jumping due to netboot requiring it anyway.
Agreed, we should either do it right and merge the two binaries, or it does
not make too much sense to only partly convert the code.
I can look into merging the two binaries, but it might also take some time.
So for the time being, I'm fine if we include the panic-jumping hack for
now, we can still then clean it up later.
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic
2024-06-05 13:37 ` Thomas Huth
2024-06-05 14:48 ` Jared Rossi
@ 2024-06-17 14:49 ` Christian Borntraeger
2024-06-20 8:14 ` Thomas Huth
1 sibling, 1 reply; 29+ messages in thread
From: Christian Borntraeger @ 2024-06-17 14:49 UTC (permalink / raw)
To: Thomas Huth, jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
Am 05.06.24 um 15:37 schrieb Thomas Huth:
> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> On a panic during IPL (i.e. a device failed to boot) check for another device
>> to boot from, as indicated by the presence of an unused IPLB.
>>
>> If an IPLB is successfully loaded, then jump to the start of BIOS, restarting
>> IPL using the updated IPLB. Otherwise enter disabled wait.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
>> docs/system/bootindex.rst | 7 ++++---
>> docs/system/s390x/bootdevices.rst | 9 ++++++---
>> pc-bios/s390-ccw/s390-ccw.h | 6 ++++++
>> 3 files changed, 16 insertions(+), 6 deletions(-)
>
> Could you please split the documentation changes into a separate patch in v2 ? ... I think that would be cleaner.
>
>> diff --git a/docs/system/bootindex.rst b/docs/system/bootindex.rst
>> index 8b057f812f..de597561bd 100644
>> --- a/docs/system/bootindex.rst
>> +++ b/docs/system/bootindex.rst
>> @@ -50,9 +50,10 @@ Limitations
>> Some firmware has limitations on which devices can be considered for
>> booting. For instance, the PC BIOS boot specification allows only one
>> -disk to be bootable. If boot from disk fails for some reason, the BIOS
>> -won't retry booting from other disk. It can still try to boot from
>> -floppy or net, though.
>> +disk to be bootable, except for on s390x machines. If boot from disk fails for
>> +some reason, the BIOS won't retry booting from other disk. It can still try to
>> +boot from floppy or net, though. In the case of s390x, the BIOS will try up to
>> +8 total devices, any number of which may be disks.
>
> Since the old text was already talking about "PC BIOS", I'd rather leave that paragraph as it is (maybe just replace "PC BIOS" with "x86 PC BIOS"), and add a separate paragraph afterwards about s390x instead.
>
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index c977a52b50..de3d1f0d5a 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -43,6 +43,7 @@ typedef unsigned long long u64;
>> #include "iplb.h"
>> /* start.s */
>> +extern char _start[];
>> void disabled_wait(void) __attribute__ ((__noreturn__));
>> void consume_sclp_int(void);
>> void consume_io_int(void);
>> @@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
>> static inline void panic(const char *string)
>> {
>> sclp_print(string);
>> + if (load_next_iplb()) {
>> + sclp_print("\nTrying next boot device...");
>> + jump_to_IPL_code((long)_start);
>> + }
>> +
>> disabled_wait();
>> }
>
> Honestly, I am unsure whether this is a really cool idea or a very ugly hack ... but I think I tend towards the latter, sorry. Jumping back to the startup code might cause various problem, e.g. pre-initialized variables don't get their values reset, causing different behavior when the s390-ccw bios runs a function a second time this way.
We jump back to _start and to me it looks like that this code does the resetting of bss segment.
So anything that has a zero value this should be fine. But static variables != 0 are indeed tricky.
As far as I can see we do have some of those :-(
So instead of jumping, is there a way that remember somewhere at which device we are and then trigger a re-ipl to reload the BIOS?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic
2024-06-17 14:49 ` Christian Borntraeger
@ 2024-06-20 8:14 ` Thomas Huth
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-20 8:14 UTC (permalink / raw)
To: Christian Borntraeger, jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 17/06/2024 16.49, Christian Borntraeger wrote:
>
>
> Am 05.06.24 um 15:37 schrieb Thomas Huth:
>> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>
>>> On a panic during IPL (i.e. a device failed to boot) check for another
>>> device
>>> to boot from, as indicated by the presence of an unused IPLB.
>>>
>>> If an IPLB is successfully loaded, then jump to the start of BIOS,
>>> restarting
>>> IPL using the updated IPLB. Otherwise enter disabled wait.
>>>
>>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>>> ---
>>> docs/system/bootindex.rst | 7 ++++---
>>> docs/system/s390x/bootdevices.rst | 9 ++++++---
>>> pc-bios/s390-ccw/s390-ccw.h | 6 ++++++
>>> 3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> Could you please split the documentation changes into a separate patch in
>> v2 ? ... I think that would be cleaner.
>>
>>> diff --git a/docs/system/bootindex.rst b/docs/system/bootindex.rst
>>> index 8b057f812f..de597561bd 100644
>>> --- a/docs/system/bootindex.rst
>>> +++ b/docs/system/bootindex.rst
>>> @@ -50,9 +50,10 @@ Limitations
>>> Some firmware has limitations on which devices can be considered for
>>> booting. For instance, the PC BIOS boot specification allows only one
>>> -disk to be bootable. If boot from disk fails for some reason, the BIOS
>>> -won't retry booting from other disk. It can still try to boot from
>>> -floppy or net, though.
>>> +disk to be bootable, except for on s390x machines. If boot from disk
>>> fails for
>>> +some reason, the BIOS won't retry booting from other disk. It can still
>>> try to
>>> +boot from floppy or net, though. In the case of s390x, the BIOS will
>>> try up to
>>> +8 total devices, any number of which may be disks.
>>
>> Since the old text was already talking about "PC BIOS", I'd rather leave
>> that paragraph as it is (maybe just replace "PC BIOS" with "x86 PC BIOS"),
>> and add a separate paragraph afterwards about s390x instead.
>>
>>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>>> index c977a52b50..de3d1f0d5a 100644
>>> --- a/pc-bios/s390-ccw/s390-ccw.h
>>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>>> @@ -43,6 +43,7 @@ typedef unsigned long long u64;
>>> #include "iplb.h"
>>> /* start.s */
>>> +extern char _start[];
>>> void disabled_wait(void) __attribute__ ((__noreturn__));
>>> void consume_sclp_int(void);
>>> void consume_io_int(void);
>>> @@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
>>> static inline void panic(const char *string)
>>> {
>>> sclp_print(string);
>>> + if (load_next_iplb()) {
>>> + sclp_print("\nTrying next boot device...");
>>> + jump_to_IPL_code((long)_start);
>>> + }
>>> +
>>> disabled_wait();
>>> }
>>
>> Honestly, I am unsure whether this is a really cool idea or a very ugly
>> hack ... but I think I tend towards the latter, sorry. Jumping back to the
>> startup code might cause various problem, e.g. pre-initialized variables
>> don't get their values reset, causing different behavior when the s390-ccw
>> bios runs a function a second time this way.
>
> We jump back to _start and to me it looks like that this code does the
> resetting of bss segment.
> So anything that has a zero value this should be fine. But static variables
> != 0 are indeed tricky.
> As far as I can see we do have some of those :-(
>
> So instead of jumping, is there a way that remember somewhere at which
> device we are and then trigger a re-ipl to reload the BIOS?
If there is an easy way, this could maybe an option, but in the long run,
I'd really prefer if we'd merge the binaries and get rid of such tricks,
since this makes the code flow quite hard to understand and maybe also more
difficult to debug if you run into problems later.
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/5] s390x: Add Full Boot Order Support
2024-05-29 15:43 [PATCH 0/5] s390x: Add Full Boot Order Support jrossi
` (4 preceding siblings ...)
2024-05-29 15:43 ` [PATCH 5/5] s390x: Enable and document boot device fallback on panic jrossi
@ 2024-06-04 18:35 ` Thomas Huth
2024-06-05 8:02 ` Thomas Huth
6 siblings, 0 replies; 29+ messages in thread
From: Thomas Huth @ 2024-06-04 18:35 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> This patch set primarily adds support for the specification of multiple boot
> devices, allowing for the guest to automatically use an alternative device on
> a failed boot without needing to be reconfigured. It additionally provides the
> ability to define the loadparm attribute on a per-device bases, which allows
> boot devices to use different loadparm values if needed.
>
> In brief, an IPLB is generated for each designated boot device (up to a maximum
> of 8) and stored in guest memory immediately before BIOS. If a device fails to
> boot, the next IPLB is retrieved and we jump back to the start of BIOS.
>
> Devices can be specified using the standard qemu device tag "bootindex" as with
> other architectures. Lower number indices are tried first, with "bootindex=0"
> indicating the first device to try.
>
> A subsequent Libvirt patch will be necessary to allow assignment of per-device
> loadparms in the guest XML
>
> Jared Rossi (5):
> Create include files for s390x IPL definitions
> Add loadparm to CcwDevice
> Build IPLB chain for multiple boot devices
> Add boot device fallback infrastructure
> Enable and document boot device fallback on panic
>
> docs/system/bootindex.rst | 7 +-
> docs/system/s390x/bootdevices.rst | 9 +-
> hw/s390x/ccw-device.h | 2 +
> hw/s390x/ipl.h | 117 +-------------------
> include/hw/s390x/ipl/qipl.h | 128 ++++++++++++++++++++++
> pc-bios/s390-ccw/bootmap.h | 5 +
> pc-bios/s390-ccw/iplb.h | 108 +++++--------------
> pc-bios/s390-ccw/s390-ccw.h | 6 ++
> hw/s390x/ccw-device.c | 49 +++++++++
> hw/s390x/ipl.c | 170 ++++++++++++++++++++++--------
> hw/s390x/s390-virtio-ccw.c | 18 +---
> hw/s390x/sclp.c | 3 +-
> pc-bios/s390-ccw/bootmap.c | 41 ++++---
> pc-bios/s390-ccw/main.c | 25 +++--
> pc-bios/s390-ccw/netmain.c | 4 +
> pc-bios/s390-ccw/Makefile | 2 +-
Hi Jared!
For v2, could you please add at least two tests: one that check booting from
the second disk and one that checks booting from the last boot disk when the
previous ones are invalid?
I could think of two easy ways for adding such tests, up to you what you prefer:
- Extend the tests/qtest/cdrom-test.c - see add_s390x_tests() there
- Add an avocado test - see "grep -l s390 tests/avocado/*.py" for examples.
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 0/5] s390x: Add Full Boot Order Support
2024-05-29 15:43 [PATCH 0/5] s390x: Add Full Boot Order Support jrossi
` (5 preceding siblings ...)
2024-06-04 18:35 ` [PATCH 0/5] s390x: Add Full Boot Order Support Thomas Huth
@ 2024-06-05 8:02 ` Thomas Huth
2024-06-06 19:22 ` Jared Rossi
6 siblings, 1 reply; 29+ messages in thread
From: Thomas Huth @ 2024-06-05 8:02 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> This patch set primarily adds support for the specification of multiple boot
> devices, allowing for the guest to automatically use an alternative device on
> a failed boot without needing to be reconfigured. It additionally provides the
> ability to define the loadparm attribute on a per-device bases, which allows
> boot devices to use different loadparm values if needed.
>
> In brief, an IPLB is generated for each designated boot device (up to a maximum
> of 8) and stored in guest memory immediately before BIOS. If a device fails to
> boot, the next IPLB is retrieved and we jump back to the start of BIOS.
>
> Devices can be specified using the standard qemu device tag "bootindex" as with
> other architectures. Lower number indices are tried first, with "bootindex=0"
> indicating the first device to try.
Is this supposed with multiple scsi-hd devices, too? I tried to boot a guest
with two scsi disks (attached to a single virtio-scsi-ccw adapter) where
only the second disk had a bootable installation, but that failed...?
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 0/5] s390x: Add Full Boot Order Support
2024-06-05 8:02 ` Thomas Huth
@ 2024-06-06 19:22 ` Jared Rossi
2024-06-07 6:19 ` Thomas Huth
0 siblings, 1 reply; 29+ messages in thread
From: Jared Rossi @ 2024-06-06 19:22 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 6/5/24 4:02 AM, Thomas Huth wrote:
> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> This patch set primarily adds support for the specification of
>> multiple boot
>> devices, allowing for the guest to automatically use an alternative
>> device on
>> a failed boot without needing to be reconfigured. It additionally
>> provides the
>> ability to define the loadparm attribute on a per-device bases, which
>> allows
>> boot devices to use different loadparm values if needed.
>>
>> In brief, an IPLB is generated for each designated boot device (up to
>> a maximum
>> of 8) and stored in guest memory immediately before BIOS. If a device
>> fails to
>> boot, the next IPLB is retrieved and we jump back to the start of BIOS.
>>
>> Devices can be specified using the standard qemu device tag
>> "bootindex" as with
>> other architectures. Lower number indices are tried first, with
>> "bootindex=0"
>> indicating the first device to try.
>
> Is this supposed with multiple scsi-hd devices, too? I tried to boot a
> guest with two scsi disks (attached to a single virtio-scsi-ccw
> adapter) where only the second disk had a bootable installation, but
> that failed...?
>
> Thomas
>
>
Hi Thomas,
Yes, I would expect that to work. I tried to reproduce this using a
non-bootable scsi disk as the first boot device and then a known-good
bootable scsi disk as the second boot device, with one controller. In
my instance the BIOS was not able to identify the first disk as bootable
and so that device failed to IPL, but it did move on to the next disk
after that, and the guest successfully IPL'd from the second device.
When you say it failed, do you mean the first disk failed to boot (as
expected), but then the guest died without attempting to boot from the
second disk? Or did something else happen? I am either not
understanding your configuration or I am not understanding your error.
Regards,
Jared Rossi
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/5] s390x: Add Full Boot Order Support
2024-06-06 19:22 ` Jared Rossi
@ 2024-06-07 6:19 ` Thomas Huth
2024-06-10 3:58 ` Jared Rossi
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Huth @ 2024-06-07 6:19 UTC (permalink / raw)
To: Jared Rossi, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 06/06/2024 21.22, Jared Rossi wrote:
>
>
> On 6/5/24 4:02 AM, Thomas Huth wrote:
>> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>
>>> This patch set primarily adds support for the specification of multiple boot
>>> devices, allowing for the guest to automatically use an alternative
>>> device on
>>> a failed boot without needing to be reconfigured. It additionally
>>> provides the
>>> ability to define the loadparm attribute on a per-device bases, which allows
>>> boot devices to use different loadparm values if needed.
>>>
>>> In brief, an IPLB is generated for each designated boot device (up to a
>>> maximum
>>> of 8) and stored in guest memory immediately before BIOS. If a device
>>> fails to
>>> boot, the next IPLB is retrieved and we jump back to the start of BIOS.
>>>
>>> Devices can be specified using the standard qemu device tag "bootindex"
>>> as with
>>> other architectures. Lower number indices are tried first, with
>>> "bootindex=0"
>>> indicating the first device to try.
>>
>> Is this supposed with multiple scsi-hd devices, too? I tried to boot a
>> guest with two scsi disks (attached to a single virtio-scsi-ccw adapter)
>> where only the second disk had a bootable installation, but that failed...?
>>
>> Thomas
>>
>>
>
> Hi Thomas,
>
> Yes, I would expect that to work. I tried to reproduce this using a
> non-bootable scsi disk as the first boot device and then a known-good
> bootable scsi disk as the second boot device, with one controller. In my
> instance the BIOS was not able to identify the first disk as bootable and so
> that device failed to IPL, but it did move on to the next disk after that,
> and the guest successfully IPL'd from the second device.
>
> When you say it failed, do you mean the first disk failed to boot (as
> expected), but then the guest died without attempting to boot from the
> second disk? Or did something else happen? I am either not understanding
> your configuration or I am not understanding your error.
I did this:
$ ./qemu-system-s390x -bios pc-bios/s390-ccw/s390-ccw.img -accel kvm \
-device virtio-scsi-ccw -drive if=none,id=d2,file=/tmp/bad.qcow2 \
-device scsi-hd,drive=d2,bootindex=2 \
-drive if=none,id=d8,file=/tmp/good.qcow2 \
-device scsi-hd,drive=d8,bootindex=3 -m 4G -nographic
LOADPARM=[ ]
Using virtio-scsi.
Using guessed DASD geometry.
Using ECKD scheme (block size 512), CDL
No zIPL section in IPL2 record.
zIPL load failed.
Trying next boot device...
LOADPARM=[ ]
Using virtio-scsi.
Using guessed DASD geometry.
Using ECKD scheme (block size 512), CDL
No zIPL section in IPL2 record.
zIPL load failed.
So it claims to try to load from the second disk, but it fails.
If I change the "bootindex=3" of the second disk to "bootindex=1", it boots
perfectly fine, so I'm sure that the installation on good.qcow2 is working fine.
Thomas
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 0/5] s390x: Add Full Boot Order Support
2024-06-07 6:19 ` Thomas Huth
@ 2024-06-10 3:58 ` Jared Rossi
0 siblings, 0 replies; 29+ messages in thread
From: Jared Rossi @ 2024-06-10 3:58 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x; +Cc: frankja, nsg
On 6/7/24 2:19 AM, Thomas Huth wrote:
> On 06/06/2024 21.22, Jared Rossi wrote:
>>
>>
>> On 6/5/24 4:02 AM, Thomas Huth wrote:
>>> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>>
>>>> This patch set primarily adds support for the specification of
>>>> multiple boot
>>>> devices, allowing for the guest to automatically use an alternative
>>>> device on
>>>> a failed boot without needing to be reconfigured. It additionally
>>>> provides the
>>>> ability to define the loadparm attribute on a per-device bases,
>>>> which allows
>>>> boot devices to use different loadparm values if needed.
>>>>
>>>> In brief, an IPLB is generated for each designated boot device (up
>>>> to a maximum
>>>> of 8) and stored in guest memory immediately before BIOS. If a
>>>> device fails to
>>>> boot, the next IPLB is retrieved and we jump back to the start of
>>>> BIOS.
>>>>
>>>> Devices can be specified using the standard qemu device tag
>>>> "bootindex" as with
>>>> other architectures. Lower number indices are tried first, with
>>>> "bootindex=0"
>>>> indicating the first device to try.
>>>
>>> Is this supposed with multiple scsi-hd devices, too? I tried to boot
>>> a guest with two scsi disks (attached to a single virtio-scsi-ccw
>>> adapter) where only the second disk had a bootable installation, but
>>> that failed...?
>>>
>>> Thomas
>>>
>>>
>>
>> Hi Thomas,
>>
>> Yes, I would expect that to work. I tried to reproduce this using a
>> non-bootable scsi disk as the first boot device and then a known-good
>> bootable scsi disk as the second boot device, with one controller.
>> In my instance the BIOS was not able to identify the first disk as
>> bootable and so that device failed to IPL, but it did move on to the
>> next disk after that, and the guest successfully IPL'd from the
>> second device.
>>
>> When you say it failed, do you mean the first disk failed to boot (as
>> expected), but then the guest died without attempting to boot from
>> the second disk? Or did something else happen? I am either not
>> understanding your configuration or I am not understanding your error.
>
> I did this:
>
> $ ./qemu-system-s390x -bios pc-bios/s390-ccw/s390-ccw.img -accel kvm \
> -device virtio-scsi-ccw -drive if=none,id=d2,file=/tmp/bad.qcow2 \
> -device scsi-hd,drive=d2,bootindex=2 \
> -drive if=none,id=d8,file=/tmp/good.qcow2 \
> -device scsi-hd,drive=d8,bootindex=3 -m 4G -nographic
> LOADPARM=[ ]
> Using virtio-scsi.
> Using guessed DASD geometry.
> Using ECKD scheme (block size 512), CDL
> No zIPL section in IPL2 record.
> zIPL load failed.
>
> Trying next boot device...
> LOADPARM=[ ]
> Using virtio-scsi.
> Using guessed DASD geometry.
> Using ECKD scheme (block size 512), CDL
> No zIPL section in IPL2 record.
> zIPL load failed.
>
> So it claims to try to load from the second disk, but it fails.
> If I change the "bootindex=3" of the second disk to "bootindex=1", it
> boots perfectly fine, so I'm sure that the installation on good.qcow2
> is working fine.
>
> Thomas
>
I am able to reproduce this now; I'll investigate the problem.
^ permalink raw reply [flat|nested] 29+ messages in thread