print memory in MB units in initrd-too-large errmsg

[PATCH] print memory in MB units in initrd-too-large errmsg

Posted by Jim Cromie 9 months ago

Change 2 error messages to display sizes in MB, not bytes.

qemu: initrd is too large, cannot support this. (max: 2047 MB, need 5833 MB)

Also, distinguish 2 sites by adding "it" and "this" respectively.
This tells a careful reader that the error above is from the 2nd size
check.

With MB displayed, I have to ask: is it coincidence that max == 2048-1 ?

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 hw/i386/x86.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index a88a126123..0677fe2fd1 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -878,9 +878,9 @@ void x86_load_linux(X86MachineState *x86ms,
                 initrd_size = g_mapped_file_get_length(mapped_file);
                 initrd_max = x86ms->below_4g_mem_size - acpi_data_size - 1;
                 if (initrd_size >= initrd_max) {
-                    fprintf(stderr, "qemu: initrd is too large, cannot support."
-                            "(max: %"PRIu32", need %"PRId64")\n",
-                            initrd_max, (uint64_t)initrd_size);
+                    fprintf(stderr, "qemu: initrd is too large, cannot support it. "
+                            "(max: %"PRIu32" MB, need %"PRId64" MB)\n",
+                            initrd_max>>20, (uint64_t)initrd_size>>20);
                     exit(1);
                 }
 
@@ -1023,9 +1023,9 @@ void x86_load_linux(X86MachineState *x86ms,
         initrd_data = g_mapped_file_get_contents(mapped_file);
         initrd_size = g_mapped_file_get_length(mapped_file);
         if (initrd_size >= initrd_max) {
-            fprintf(stderr, "qemu: initrd is too large, cannot support."
-                    "(max: %"PRIu32", need %"PRId64")\n",
-                    initrd_max, (uint64_t)initrd_size);
+            fprintf(stderr, "qemu: initrd is too large, cannot support this. "
+                    "(max: %"PRIu32" MB, need %"PRId64" MB)\n",
+                    initrd_max>>20, (uint64_t)initrd_size>>20);
             exit(1);
         }
 
-- 
2.41.0

Re: [PATCH] print memory in MB units in initrd-too-large errmsg

Posted by Markus Armbruster 9 months ago

Jim Cromie <jim.cromie@gmail.com> writes:

> Change 2 error messages to display sizes in MB, not bytes.
>
> qemu: initrd is too large, cannot support this. (max: 2047 MB, need 5833 MB)
>
> Also, distinguish 2 sites by adding "it" and "this" respectively.
> This tells a careful reader that the error above is from the 2nd size
> check.

I don't like this part.

> With MB displayed, I have to ask: is it coincidence that max == 2048-1 ?

I don't know.

> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  hw/i386/x86.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index a88a126123..0677fe2fd1 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -878,9 +878,9 @@ void x86_load_linux(X86MachineState *x86ms,
>                  initrd_size = g_mapped_file_get_length(mapped_file);
>                  initrd_max = x86ms->below_4g_mem_size - acpi_data_size - 1;
>                  if (initrd_size >= initrd_max) {

Not this patch's problem, but here goes anyway: why reject initrd_size
== initrd_max?

> -                    fprintf(stderr, "qemu: initrd is too large, cannot support."
> -                            "(max: %"PRIu32", need %"PRId64")\n",
> -                            initrd_max, (uint64_t)initrd_size);
> +                    fprintf(stderr, "qemu: initrd is too large, cannot support it. "
> +                            "(max: %"PRIu32" MB, need %"PRId64" MB)\n",
> +                            initrd_max>>20, (uint64_t)initrd_size>>20);
>                      exit(1);
>                  }
>  
> @@ -1023,9 +1023,9 @@ void x86_load_linux(X86MachineState *x86ms,
>          initrd_data = g_mapped_file_get_contents(mapped_file);
>          initrd_size = g_mapped_file_get_length(mapped_file);
>          if (initrd_size >= initrd_max) {
> -            fprintf(stderr, "qemu: initrd is too large, cannot support."
> -                    "(max: %"PRIu32", need %"PRId64")\n",
> -                    initrd_max, (uint64_t)initrd_size);
> +            fprintf(stderr, "qemu: initrd is too large, cannot support this. "
> +                    "(max: %"PRIu32" MB, need %"PRId64" MB)\n",
> +                    initrd_max>>20, (uint64_t)initrd_size>>20);
>              exit(1);
>          }

You're rounding both values down.  Consider

    initrd_max = 1000.5MiB - 1 exactly
    initrd_size = 1000.5MiB + 4096

Before the patch, we report

    (max: 1049100287, need 1049104384)

Unfriendly, but at least it's correct.  Afterwards

    (max: 1000 MB, need 1000 MB)

Wat?  Long-suffering users may guess the rounding issue.  But let's not
make users guess.

Better would be something like

    size of initrd exceeds the limit of X MiB by Y Bytes

*if* X is actually a multiple of 1 MiB.  Else, we need to consider
showing a suitably rounded fractional part, or stick to Bytes after all.

Re: [PATCH] print memory in MB units in initrd-too-large errmsg

Posted by jim.cromie@gmail.com 9 months ago

On Sat, Aug 5, 2023 at 12:26 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Jim Cromie <jim.cromie@gmail.com> writes:
>
> > Change 2 error messages to display sizes in MB, not bytes.
> >
> > qemu: initrd is too large, cannot support this. (max: 2047 MB, need 5833 MB)
> >
> > Also, distinguish 2 sites by adding "it" and "this" respectively.
> > This tells a careful reader that the error above is from the 2nd size
> > check.
>
> I don't like this part.
>
> > With MB displayed, I have to ask: is it coincidence that max == 2048-1 ?
>
> I don't know.
>

I found this in hw/i386/x86.c

    924         /*
    925          * Linux has supported initrd up to 4 GB for a very
long time (2007,
    926          * long before XLF_CAN_BE_LOADED_ABOVE_4G which was
added in 2013),
    927          * though it only sets initrd_max to 2 GB to "work
around bootloader
    928          * bugs". Luckily, QEMU firmware(which does something
like bootloader)
    929          * has supported this.
    930          *
    931          * It's believed that if XLF_CAN_BE_LOADED_ABOVE_4G is
set, initrd can
    932          * be loaded into any address.
    933          *
    934          * In addition, initrd_max is uint32_t simply because
QEMU doesn't
    935          * support the 64-bit boot protocol (specifically the
ext_ramdisk_image
    936          * field).
    937          *
    938          * Therefore here just limit initrd_max to UINT32_MAX
simply as well.
    939          */



> You're rounding both values down.  Consider
>
>     initrd_max = 1000.5MiB - 1 exactly
>     initrd_size = 1000.5MiB + 4096
>
> Before the patch, we report
>
>     (max: 1049100287, need 1049104384)
>
> Unfriendly, but at least it's correct.  Afterwards
>
>     (max: 1000 MB, need 1000 MB)
>
> Wat?  Long-suffering users may guess the rounding issue.  But let's not
> make users guess.
>
> Better would be something like
>
>     size of initrd exceeds the limit of X MiB by Y Bytes
>
> *if* X is actually a multiple of 1 MiB.  Else, we need to consider
> showing a suitably rounded fractional part, or stick to Bytes after all.
>

ok, that makes sense.

But 1st I gotta figure out why a 2gb initrd size limit
is not a show-stopper problem for lkp-robot.

https://lore.kernel.org/oe-lkp/202308031432.fcb4197-oliver.sang@intel.com/
has reproducer, that I cannot execute.
I have deleted ~/.lkp and /lkp has 2 empty subdirs