Re: [PATCH v6 17/28] pc-bios/s390-ccw: Refactor zipl_run()

[Thomas Huth <thuth@redhat.com>]

On 18/09/2025 01.21, Zhuoying Cai wrote:
> Refactor to enhance readability before enabling secure IPL in later
> patches.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/bootmap.c | 49 ++++++++++++++++++++++++--------------
>   1 file changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 0f8baa0198..ff0fa78cf0 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -674,6 +674,35 @@ static int zipl_load_segment(ComponentEntry *entry)
>       return 0;
>   }
>   
> +static int zipl_run_normal(ComponentEntry **entry_ptr, uint8_t *tmp_sec)
> +{
> +    ComponentEntry *entry = *entry_ptr;
> +
> +    while (entry->component_type == ZIPL_COMP_ENTRY_LOAD ||
> +           entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
> +
> +        /* Secure boot is off, so we skip signature entries */
> +        if (entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
> +            entry++;
> +            continue;
> +        }
> +
> +        if (zipl_load_segment(entry)) {
> +            return -1;
> +        }
> +
> +        entry++;
> +
> +        if ((uint8_t *)&entry[1] > tmp_sec + MAX_SECTOR_SIZE) {
> +            puts("Wrong entry value");
> +            return -EINVAL;
> +        }
> +    }
> +
> +    *entry_ptr = entry;
> +    return 0;
> +}
> +
>   /* Run a zipl program */
>   static int zipl_run(ScsiBlockPtr *pte)
>   {
> @@ -700,25 +729,9 @@ static int zipl_run(ScsiBlockPtr *pte)
>   
>       /* Load image(s) into RAM */
>       entry = (ComponentEntry *)(&header[1]);
> -    while (entry->component_type == ZIPL_COMP_ENTRY_LOAD ||
> -           entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
> -
> -        /* We don't support secure boot yet, so we skip signature entries */
> -        if (entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
> -            entry++;
> -            continue;
> -        }
>   
> -        if (zipl_load_segment(entry)) {
> -            return -1;
> -        }
> -
> -        entry++;
> -
> -        if ((uint8_t *)(&entry[1]) > (tmp_sec + MAX_SECTOR_SIZE)) {
> -            puts("Wrong entry value");
> -            return -EINVAL;

So in this case the code returned -EINVAL ...

> -        }
> +    if (zipl_run_normal(&entry, tmp_sec)) {
> +        return -1;

... but now it's always returning -1.

It likely does not make much difference in this case here, but it's 
certainly better style to preserve the error value.

  Thomas


>       }
>   
>       if (entry->component_type != ZIPL_COMP_ENTRY_EXEC) {