Re: [PATCH v4 19/28] pc-bios/s390-ccw: Refactor zipl_load_segment function

[Zhuoying Cai <zycai@linux.ibm.com>]

On 7/14/25 6:10 PM, Collin Walling wrote:
> 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 <zycai@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?
> 

A component table entry points to a segment table, which holds pointers
to code segments loaded into contiguous memory.

Since segments can vary in length, the block count field may be nonzero.

Block size indicates the number of bytes in a single logical block, so
the total component size is the block size multiplied by the block count.

>>              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;
>>          }
>>  
>