Re: [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices

[Thomas Huth <thuth@redhat.com>]

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