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.1On 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
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
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
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
© 2016 - 2024 Red Hat, Inc.