limine-bootloader / limine

Modern, advanced, portable, multiprotocol bootloader and boot manager.
https://limine-bootloader.org
BSD 2-Clause "Simplified" License
1.85k stars 141 forks source link

test: multiboot2: print efi memory map (and reveal memory bug) #358

Closed phip1611 closed 5 months ago

phip1611 commented 5 months ago

UPDATE I've just seen you did this in https://github.com/limine-bootloader/limine/commit/a9afcf89a092eae97cb33fc6c278d42d8ef9a58f. We can drop the PR, but I found a bug. It is clearly visible.

This extends test/multiboot2.c to also print the memory map. I created this to support my investigations in #356. I suggest keeping the core discussion there.

However, by using this change, and the following patch to common/protos/multiboot2.c:

diff --git a/common/protos/multiboot2.c b/common/protos/multiboot2.c
index e5f085f6..82618732 100644
--- a/common/protos/multiboot2.c
+++ b/common/protos/multiboot2.c
@@ -25,6 +25,7 @@
 #include <lib/misc.h>
 #include <drivers/vga_textmode.h>
 #include <pxe/pxe.h>
+#include <e9print.h>

 #define LIMINE_BRAND "Limine " LIMINE_VERSION

@@ -865,6 +866,9 @@ skip_modeset:;
         if ((efi_mmap_size / efi_desc_size) > MEMMAP_MAX) {
             panic(false, "multiboot2: too many EFI memory map entries");
         }
+        if (efi_mmap_size % efi_desc_size != 0) {
+            panic(false, "multiboot2: invalid EFI memory map size");
+        }

         // Create the EFI memory map tag.
         uint32_t size = sizeof(struct multiboot_tag_efi_mmap) + efi_mmap_size;
@@ -878,6 +882,20 @@ skip_modeset:;
         // Copy over the EFI memory map.
         memcpy(mmap_tag->efi_mmap, efi_mmap, efi_mmap_size);
         append_tag(info_idx, mmap_tag);
+
+        e9_printf("UEFI memory map: \n");
+        e9_printf("  <#> <phys> <size> <type> <attr>\n");
+        int n_entries = efi_mmap_size / efi_desc_size;
+        e9_printf("number of entries   = %d\n", n_entries);
+        e9_printf("tag size            = %x\n", size);
+        e9_printf("efi memory map size = %x\n", efi_mmap_size);
+
+        // Limine serial log output is weird. In the serial.txt file, there
+        // are never printed more than
+        for (int i = 0; i < n_entries; i++) {
+            EFI_MEMORY_DESCRIPTOR * e = ((char *) efi_mmap) + i * efi_desc_size;
+            e9_printf("  [%d] %x %x %x %x: \n", i, e->PhysicalStart, e->NumberOfPages * 4096, e->Type, e->Attribute);
+        }
     }
 #endif

I could find that there is indeed a bug. The first efi mmap output in the following excerpt refers to the output in common/protos/multiboot2.c and the second to test/multiboot2.c:

UEFI memory map: 
  <#> <phys> <size> <type> <attr>
number of entries   = 126
tag size            = 0x17b0
efi memory map size = 0x17a0
  [0] 0x0 0x1000 0x3 0xf: 
  [1] 0x1000 0x9f000 0x7 0xf: 
  [2] 0x100000 0x700000 0x7 0xf: 
  [3] 0x800000 0x8000 0xa 0xf: 
  [4] 0x808000 0x3000 0x7 0xf: 
  [5] 0x80b000 0x1000 0xa 0xf: 
  [6] 0x80c000 0x4000 0x7 0xf: 
  [7] // phys, size, and type look sane; attr is invalid?
Welcome to the multiboot2 test kernel: 
         size=7576
         reserved=0

Tags:
         cmdline:
                 string=
         bootloader_name:
                 string=Limine 7.5.2
         framebuffer:
                 framebuffer_pitch: 3200
                 framebuffer_width: 800
                 framebuffer_height: 600
                 framebuffer_bpp: 32
                 framebuffer_type: 1
                 framebuffer_address: 0x80000000
                 framebuffer_red_field_position: 0x10
                 framebuffer_red_mask_size: 0x8
                 framebuffer_green_field_position: 0x8
                 framebuffer_green_mask_size: 0x8
                 framebuffer_blue_field_position: 0x0
                 framebuffer_blue_mask_size: 0x8
         acpi_new:
                 rsdp=RSD PTR �BOCHS t�w$
         acpi_old:
                 rsdp=RSD PTR �BOCHS 
         mmap:
                 entry_size=24
                 entry_version=0
                 entry count: 24
                 entries:
                         addr=0x0, len=0x0, type=0xa0000
                         addr=0x100000, len=0x0, type=0x700000
                         addr=0x800000, len=0x0, type=0x8000
                         addr=0x808000, len=0x0, type=0x3000
                         addr=0x80b000, len=0x0, type=0x1000
                         addr=0x80c000, len=0x0, type=0x4000
                         addr=0x810000, len=0x0, type=0xf0000
                         addr=0x900000, len=0x0, type=0xdb4e000
                         addr=0xe44e000, len=0x0, type=0x6000
                         addr=0xe454000, len=0x0, type=0x7000
                         addr=0xe45b000, len=0x0, type=0x39000
                         addr=0xe494000, len=0x0, type=0x2f000
                         addr=0xe4c3000, len=0x0, type=0xb000
                         addr=0xe4ce000, len=0x0, type=0x101f000
                         addr=0xf4ed000, len=0x0, type=0x100000
                         addr=0xf5ed000, len=0x0, type=0x100000
                         addr=0xf6ed000, len=0x0, type=0x80000
                         addr=0xf76d000, len=0x0, type=0x12000
                         addr=0xf77f000, len=0x0, type=0x80000
                         addr=0xf7ff000, len=0x0, type=0x6f5000
                         addr=0xfef4000, len=0x0, type=0x84000
                         addr=0xff78000, len=0x0, type=0x88000
                         addr=0xe0000000, len=0x0, type=0x10000000
                         addr=0x0, len=0xfd, type=0x0
         basic_meminfo:
                 mem_lower=0x280
                 mem_upper=0x3e67c
         efi mmap:
                 map_size=6064
                 desc_size=48
                 desc_version=1
                 entry count: 126
                 entries:
                         addr=0x0, len=0x0 type=0x1000 attr=0x0
                         addr=0x1000, len=0x0 type=0x9f000 attr=0x0
                         addr=0x100000, len=0x0 type=0x700000 attr=0x0
                         addr=0x800000, len=0x0 type=0x8000 attr=0x0
                         // .. addr looks fine, but len is broken, type, and also attr
phip1611 commented 5 months ago

Yikes. Closing in favor of https://github.com/limine-bootloader/limine/commit/a9afcf89a092eae97cb33fc6c278d42d8ef9a58f

But still, there is a bug.