Re: [PATCH v5 15/19] hw/s390x: Build an IPLB for each boot device

[Jared Rossi <jrossi@linux.ibm.com>]

On 10/22/24 9:13 AM, Thomas Huth wrote:
> On 20/10/2024 03.29, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> Build an IPLB for any device with a bootindex (up to a maximum of 8 
>> devices).
>>
>> The IPLB chain is placed immediately before the BIOS in memory. 
>> 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 possible later 
>> access by
>> the guest during IPL.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
> ...
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 656996b500..ed152a9dd2 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -56,6 +56,13 @@ static bool iplb_extended_needed(void *opaque)
>>       return ipl->iplbext_migration;
>>   }
>>   +/* Place the IPLB chain immediately before the BIOS in memory */
>> +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,
>> @@ -398,6 +405,17 @@ static CcwDevice 
>> *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
>>       return ccw_dev;
>>   }
>>   +static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock 
>> *iplb_chain)
>> +{
>> +    S390IPLState *ipl = get_ipl_device();
>> +    uint16_t count = ipl->qipl.chain_len;
>
> I ran into problems when running the cdrom-test on my x86 laptop.
>
> I think the above line needs to be changed into this to get it working 
> on little endian hosts:
>
>     uint16_t count = be16_to_cpu(ipl->qipl.chain_len)
>
>
>> +    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, len);
>> +    return chain_addr;
>> +}
>> +
>>   void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
>>   {
>>       int i;
>> @@ -428,54 +446,51 @@ void s390_ipl_convert_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:
>>               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 true 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;
>>           }
>>   @@ -484,8 +499,8 @@ static bool s390_gen_initial_iplb(S390IPLState 
>> *ipl)
>>               lp = S390_CCW_MACHINE(qdev_get_machine())->loadparm;
>>           }
>>   -        s390_ipl_convert_loadparm((char *)lp, ipl->iplb.loadparm);
>> -        ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
>> +        s390_ipl_convert_loadparm((char *)lp, iplb->loadparm);
>> +        iplb->flags |= DIAG308_FLAGS_LP_VALID;
>>             return true;
>>       }
>> @@ -493,6 +508,62 @@ 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);
>> +    Object *machine = qdev_get_machine();
>> +
>> +    /*
>> +     * Parse the boot devices.  Generate an IPLB for only the first 
>> boot device
>> +     * which will later be set with DIAG308.
>> +     */
>> +    if (!dev_st) {
>> +        ipl->qipl.chain_len = 0;
>> +        return false;
>> +    }
>> +
>> +    /* If no machine loadparm was defined fill it with spaces */
>> +    if (memcmp(S390_CCW_MACHINE(machine)->loadparm, NO_LOADPARM, 8) 
>> == 0) {
>> +        object_property_set_str(machine, "loadparm", " ", NULL);
>> +    }
>> +
>> +    iplb_num = 1;
>> +    s390_build_iplb(dev_st, &ipl->iplb);
>> +
>> +    /*  Index any fallback boot devices */
>> +    while (get_boot_device(iplb_num)) {
>> +        iplb_num++;
>> +    }
>> +
>> +    if (iplb_num > MAX_BOOT_DEVS) {
>> +        warn_report("Excess boot devices defined! %d boot devices 
>> found, "
>> +                    "but only the first %d will be considered.",
>> +                    iplb_num, MAX_BOOT_DEVS);
>> +
>> +        iplb_num = MAX_BOOT_DEVS;
>> +    }
>> +
>> +    ipl->qipl.chain_len = iplb_num - 1;
>
> ... and this needs to be changed into:
>
>     ipl->qipl.chain_len = cpu_to_be16(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) {
>> +        /* 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]);
>> +        }
>> +
>> +        ipl->qipl.next_iplb = s390_ipl_map_iplb_chain(iplb_chain);
>
> And this needs to be change into:
>
>     ipl->qipl.next_iplb = 
> cpu_to_be64(s390_ipl_map_iplb_chain(iplb_chain));
>
> I am already in progress of picking up your patches, so if you agree 
> with my suggestions, I can fix it on my side, too.
>
>  Thomas
>

This is agreeable to me. Thank you for handling it on your side.

Regards,
     Jared Rossi