On 7/11/25 5:10 PM, Zhuoying Cai wrote:
> Make the address variable a parameter of zipl_load_segment and return
> segment length.
There's mixed use of the term "comp_len" and "segment length". Since
the context here is "zipl_load_segment", perhaps the variable should be
"seg_len"?
>
> Modify this function for reuse in the next patch, which allows
> loading segment or signature data to the destination memory address.
The function is still loading a segment from the disk regardless if it's
a signature or something else. I'd suggest rewording the above for more
precision about the change:
"Modify this function to allow the caller to specify a memory address
where segment data should be loaded into."
>
> Add a comp_len variable to store the length of a segment and return this
> variable in zipl_load_segment.
This sentence is redundant since the change in the return behavior is
mentioned in the first sentence.
>
> comp_len variable is necessary to store the calculated segment length and
> is used during signature verification. Return the length on success, or
> a negative return code on failure.
>
> Signed-off-by: Zhuoying Cai <zy...@linux.ibm.com>
Bit of a nit: technically this isn't refactoring since the function's
behavior has changed (new param and different return meaning). Change
the commit header from "refactor" to "rework" or something akin to that.
> ---
> pc-bios/s390-ccw/bootmap.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index ced5190888..2513e6c131 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -613,19 +613,18 @@ static int ipl_eckd(void)
> * IPL a SCSI disk
> */
>
> -static int zipl_load_segment(ComponentEntry *entry)
> +static int zipl_load_segment(ComponentEntry *entry, uint64_t address)
The return value meaning of this function has changed from being "< 0
means error, 0 is okay" to "< 0 means error, otherwise the total size of
the component is returned". Please add a comment above this function to
describe its return behavior so it's easy for future developers to
understand it.
> {
> const int max_entries = (MAX_SECTOR_SIZE / sizeof(ScsiBlockPtr));
> ScsiBlockPtr *bprs = (void *)sec;
> const int bprs_size = sizeof(sec);
> block_number_t blockno;
> - uint64_t address;
> int i;
> char err_msg[] = "zIPL failed to read BPRS at 0xZZZZZZZZZZZZZZZZ";
> char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */
> + int comp_len = 0;
>
> blockno = entry->data.blockno;
> - address = entry->compdat.load_addr;
>
> debug_print_int("loading segment at block", blockno);
> debug_print_int("addr", address);
> @@ -662,6 +661,9 @@ static int zipl_load_segment(ComponentEntry *entry)
> */
> break;
> }
> +
> + comp_len += bprs->size * (bprs[i].blockct + 1);
> +
I'm confused by the arithmetic here. Why is size multiplied by the
block count? Won't that artificially inflate the value representing the
size of the component? What's the reason that comp_len += bprs->size
isn't sufficient?
> address = virtio_load_direct(cur_desc[0], cur_desc[1], 0,
> (void *)address);
> if (!address) {
> @@ -671,7 +673,7 @@ static int zipl_load_segment(ComponentEntry *entry)
> }
> } while (blockno);
>
> - return 0;
> + return comp_len;
> }
>
> static int zipl_run_normal(ComponentEntry *entry, uint8_t *tmp_sec)
> @@ -685,7 +687,7 @@ static int zipl_run_normal(ComponentEntry *entry, uint8_t
> *tmp_sec)
> continue;
> }
>
> - if (zipl_load_segment(entry)) {
> + if (zipl_load_segment(entry, entry->compdat.load_addr) < 0) {
> return -1;
> }
>
--
Regards,
Collin
