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

[Thomas Huth <thuth@redhat.com>]

On 27/09/2024 02.51, 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>
> 

Note: There's an empty line below your SoB in most of your patch 
descriptions ... it's just cosmetics, but in case you touch the patch 
anyway, please remove it.

> ---
...
> @@ -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?

> +    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() ?

> +        }
> +
> +        ipl->qipl.next_iplb = s390_ipl_map_iplb_chain(iplb_chain);
> +    }
> +
> +    return iplb_num;
> +}
> +
>   static bool is_virtio_ccw_device_of_type(IplParameterBlock *iplb,
>                                            int virtio_id)
>   {
> @@ -620,7 +687,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 ||
> @@ -714,7 +781,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.chain_len = 0;
>           }
>       }
>       s390_ipl_set_boot_menu(ipl);

  Thomas