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

[Jared Rossi]

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