Re: [PATCH] s390x/ipl: support extended kernel command line size

[Marc Hartmayer <mhartmay@linux.ibm.com>]

David Hildenbrand <david@redhat.com> writes:

> On 22.11.21 12:29, Marc Hartmayer wrote:
>> In the past s390 used a fixed command line length of 896 bytes. This has changed
>> with the Linux commit 5ecb2da660ab ("s390: support command lines longer than 896
>> bytes"). There is now a parm area indicating the maximum command line size. This
>> parm area has always been initialized to zero, so with older kernels this field
>> would read zero and we must then assume that only 896 bytes are available.
>> 
>> Acked-by: Viktor Mihajlovski <mihajlov@de.ibm.com>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  hw/s390x/ipl.c | 23 ++++++++++++++++++++---
>>  1 file changed, 20 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 7ddca0127fc2..092c66b3f9f1 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -37,8 +37,9 @@
>>  
>>  #define KERN_IMAGE_START                0x010000UL
>>  #define LINUX_MAGIC_ADDR                0x010008UL
>> +#define KERN_PARM_AREA_SIZE_ADDR        0x010430UL
>>  #define KERN_PARM_AREA                  0x010480UL
>> -#define KERN_PARM_AREA_SIZE             0x000380UL
>> +#define LEGACY_KERN_PARM_AREA_SIZE      0x000380UL
>>  #define INITRD_START                    0x800000UL
>>  #define INITRD_PARM_START               0x010408UL
>>  #define PARMFILE_START                  0x001000UL
>> @@ -110,6 +111,21 @@ static uint64_t bios_translate_addr(void *opaque, uint64_t srcaddr)
>>      return srcaddr + dstaddr;
>>  }
>>  
>> +static uint64_t get_max_kernel_cmdline_size(void)
>> +{
>> +    uint64_t *size_ptr = rom_ptr(KERN_PARM_AREA_SIZE_ADDR, sizeof(*size_ptr));
>> +
>> +    if (size_ptr) {
>> +        uint64_t size;
>> +
>> +        size = be64_to_cpu(*size_ptr);
>> +        if (size != 0) {
>
> Could do "if (size) {"

Ok.

>
>> +            return size;
>> +        }
>> +    }
>> +    return LEGACY_KERN_PARM_AREA_SIZE;
>> +}
>> +
>>  static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>  {
>>      MachineState *ms = MACHINE(qdev_get_machine());
>> @@ -197,10 +213,11 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>              ipl->start_addr = KERN_IMAGE_START;
>>              /* Overwrite parameters in the kernel image, which are "rom" */
>>              if (parm_area) {
>> -                if (cmdline_size > KERN_PARM_AREA_SIZE) {
>> +                uint64_t max_cmdline_size = get_max_kernel_cmdline_size();
>
> We might want an empty line here.

Yep.

>
>> +                if (cmdline_size > max_cmdline_size) {
>>                      error_setg(errp,
>>                                 "kernel command line exceeds maximum size: %zu > %lu",
>> -                               cmdline_size, KERN_PARM_AREA_SIZE);
>> +                               cmdline_size, max_cmdline_size);
>>                      return;
>>                  }
>>  
>> 
>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks for the review!

>
> -- 
> Thanks,
>
> David / dhildenb
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294