Message ID | 20230804190101.759753-1-jim.cromie@gmail.com |
---|---|
State | New |
Headers | show |
Series | print memory in MB units in initrd-too-large errmsg | expand |
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.
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
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); }
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(-)