Re: [PATCH v13 21/33] pc-bios/s390-ccw: Rework zipl_load_segment function

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

On 7/2/26 10:38 PM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On 3/7/26 04:29, Zhuoying Cai wrote:
>> Change zipl_load_segment() to accept explicit blockno and address
>> parameters instead of ComponentEntry pointer and return segment length.
>>
>> Modify this function to allow the caller to specify a memory address
>> where segment data should be loaded into.
>>
>> seg_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.
>>
>> Remove static qualifier and add function declaration to bootmap.h to
>> make it accessible to other modules.
>>
>> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>> Reviewed-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 18 ++++++++++--------
>>   pc-bios/s390-ccw/bootmap.h |  2 ++
>>   2 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index b9ba004cfc..b19981feb1 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -613,19 +613,19 @@ static int ipl_eckd(void)
>>    * IPL a SCSI disk
>>    */
>>   
>> -static int zipl_load_segment(ComponentEntry *entry)
>> +/*
>> + * Returns: length of the segment on success,
>> + *          negative value on error.
>> + */
>> +int zipl_load_segment(block_number_t blockno, uint64_t address)
>>   {
> 
> 
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index 95943441d3..8d61ac383c 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -113,6 +113,8 @@ typedef struct ScsiMbr {
>>       ScsiBlockPtr pt;   /* block pointer to program table */
>>   } __attribute__ ((packed)) ScsiMbr;
>>   
> 
> Isn't it better to document the declaration rather than the
> definition?
> 

Thanks for the review! I can move the documentation to the declaration.

>> +int zipl_load_segment(block_number_t blockno, uint64_t address);
>> +
>>   #define ZIPL_MAGIC              "zIPL"
>>   #define ZIPL_MAGIC_EBCDIC       "\xa9\xc9\xd7\xd3"
>>   #define IPL1_MAGIC "\xc9\xd7\xd3\xf1" /* == "IPL1" in EBCDIC */
>