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

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

On 9/30/24 7:59 AM, Thomas Huth wrote:
> On 27/09/2024 02.51, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>> ... 
>> @@ -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,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.
>> +     */
>
> The comment looks like it rather belongs to the whole function, and 
> not to the if-statement below? So maybe move it at the top?

Agreed.

>
>> +    if (!dev_st) {
>> +        ipl->qipl.chain_len = 0;
>> +        return false;
>> +    }
>> +
>> +    iplb_num = 1;
>> +    s390_build_iplb(dev_st, &ipl->iplb);
>> +    ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
>
> Isn't DIAG308_FLAGS_LP_VALID set within s390_build_iplb() already?
>
>> +    while (get_boot_device(iplb_num)) {
>> +        iplb_num++;
>> +    }
>
> I'd maybe move the code block below to this spot here, so you've got 
> to assign ipl->qipl.chain_len only once:
>
> +    if (iplb_num > MAX_IPLB_CHAIN + 1) {
> +        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.chain_len = 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 + 1) {
>> +            warn_report("Excess boot devices defined! %d boot 
>> devices found, "
>> +                        "but only the first %d will be considered.",
>> +                        iplb_num, MAX_IPLB_CHAIN + 1);
>> +
>> +            ipl->qipl.chain_len = MAX_IPLB_CHAIN;
>> +            iplb_num = MAX_IPLB_CHAIN + 1;
>> +        }
>
> i.e. move this code block -^
> and remove the "ipl->qipl.chain_len = MAX_IPLB_CHAIN;" in there.
>
>> +        /* 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;
>
> Again, setting DIAG308_FLAGS_LP_VALID should not be necessary since it 
> is already done in s390_build_iplb() ?
>

I’ll see if I can clean section this up and remove redundant flag 
assignments.

Jared Rossi