* Re: [PATCH] s390x/ipl: support extended kernel command line size
2021-11-22 11:29 [PATCH] s390x/ipl: support extended kernel command line size Marc Hartmayer
@ 2021-11-22 11:54 ` David Hildenbrand
2021-11-22 15:07 ` Marc Hartmayer
2021-11-29 17:28 ` Christian Borntraeger
2021-12-08 9:04 ` Thomas Huth
2 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2021-11-22 11:54 UTC (permalink / raw)
To: Marc Hartmayer, qemu-devel
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, Thomas Huth
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) {"
> + 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.
> + 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,
David / dhildenb
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] s390x/ipl: support extended kernel command line size
2021-11-22 11:54 ` David Hildenbrand
@ 2021-11-22 15:07 ` Marc Hartmayer
0 siblings, 0 replies; 5+ messages in thread
From: Marc Hartmayer @ 2021-11-22 15:07 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, Thomas Huth
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] s390x/ipl: support extended kernel command line size
2021-11-22 11:29 [PATCH] s390x/ipl: support extended kernel command line size Marc Hartmayer
2021-11-22 11:54 ` David Hildenbrand
@ 2021-11-29 17:28 ` Christian Borntraeger
2021-12-08 9:04 ` Thomas Huth
2 siblings, 0 replies; 5+ messages in thread
From: Christian Borntraeger @ 2021-11-29 17:28 UTC (permalink / raw)
To: Marc Hartmayer, qemu-devel
Cc: Halil Pasic, qemu-s390x, Thomas Huth, David Hildenbrand
Am 22.11.21 um 12:29 schrieb Marc Hartmayer:
> 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>
Reviewed-by: Christian Borntraeger <borntraeger@de.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) {
> + 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();
> + 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;
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] s390x/ipl: support extended kernel command line size
2021-11-22 11:29 [PATCH] s390x/ipl: support extended kernel command line size Marc Hartmayer
2021-11-22 11:54 ` David Hildenbrand
2021-11-29 17:28 ` Christian Borntraeger
@ 2021-12-08 9:04 ` Thomas Huth
2 siblings, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2021-12-08 9:04 UTC (permalink / raw)
To: Marc Hartmayer, qemu-devel
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, David Hildenbrand
On 22/11/2021 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) {
> + 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();
> + if (cmdline_size > max_cmdline_size) {
> error_setg(errp,
> "kernel command line exceeds maximum size: %zu > %lu",
I think the %lu likely needs to be turned into a PRIu64 now ... could you
please try compiling on a 32-bit host env (e.g. via gitlab-ci), fix the
patch accordingly (also with the cosmetics mentioned by David) and send a
v2? Thanks!
Thomas
> - cmdline_size, KERN_PARM_AREA_SIZE);
> + cmdline_size, max_cmdline_size);
> return;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread