From: Mike Larkin Subject: Re: efiboot "machine memory" support for riscv64 To: tech@openbsd.org Date: Mon, 5 Aug 2024 10:18:44 -0700 On Sun, Aug 04, 2024 at 10:23:00PM +0200, Jeremie Courreges-Anglas wrote: > > The code below works fine on my home machine as I am able to print the > memory map and mask out chunks of memory. mach mem =XXXM isn't very > useful because only 22MB of memory are available in the lowest > addresses on that machine. > > The implementation is cheap: > - it doesn't manipulate the efi memory map from efiboot, instead it > passes hints to the kernel which already has the memreg_remove() > code. > - it doesn't add support for '+' ie forcefully adding memory to the > memory map. That looks like a hack for a problem I don't have, > something that *should* be fixed by providing a proper dtb. > > Does this look worth having? ok? > I think it makes sense. probably get kettenis to weigh in on it, but seems useful for debugging. > > PS: implementing a barebone memtest directly in efiboot seems the > least ugly way to provide the user with the physicall addresses of > possibly bad RAM cells (on amd64 I would use memtest86+). Before I > start hacking on this, does this sound reasonable, or maybe someone > has a better idea? > > > > Index: stand/efiboot/efiboot.c > =================================================================== > RCS file: /cvs/src/sys/arch/riscv64/stand/efiboot/efiboot.c,v diff -u > -p -r1.11 efiboot.c --- stand/efiboot/efiboot.c 23 Jun 2024 13:11:51 > -0000 1.11 +++ stand/efiboot/efiboot.c 4 Aug 2024 19:46:13 -0000 @@ > -495,6 +495,45 @@ efi_dma_constraint(void) dma_constraint, > sizeof(dma_constraint)); } > > +struct fdt_reg reserved_memory[64]; > +unsigned int nreserved; > + > +static void > +fdt_add_reserved_memory(long long addr, long long size) > +{ > + if (nreserved == nitems(reserved_memory)) { > + printf("too many reserved mappings\n"); > + return; > + } > + > + reserved_memory[nreserved].addr = addr; > + reserved_memory[nreserved].size = size; > + nreserved++; > +} > + > +static void > +fdt_reset_reserved_memory(void) > +{ > + memset(reserved_memory, 0, sizeof(reserved_memory)); > + nreserved = 0; > +} > + > +static void > +fdt_print_reserved_memory(void) > +{ > + int i; > + > + if (nreserved == 0) > + return; > + > + printf("Reserved memory ranges:\n"); > + for (i = 0; i < nreserved; i++) { > + printf(" pa 0x%llx size 0x%llx\n", > + reserved_memory[i].addr, > + reserved_memory[i].size); > + } > +} > + > char *bootmac = NULL; > > void * > @@ -536,6 +575,29 @@ efi_makebootargs(char *bootargs, int how > if (node == NULL) > fdt_node_add_node(fdt_find_node("/"), "chosen", &node); > > + if (nreserved > 0) { > + void *child; > + unsigned int i; > + > + node = fdt_find_node("/reserved-memory"); > + if (node == NULL) { > + fdt_node_add_node(fdt_find_node("/"), "reserved-memory", > + &node); > + } > + > + for (i = 0; i < nreserved; i++) { > + char childname[sizeof("openbsd,reserved-memoryXXXX")]; > + struct fdt_reg reg; > + > + snprintf(childname, sizeof(childname), > + "openbsd,reserved-memory%u", i); > + fdt_node_add_node(node, childname, &child); > + reg.addr = htobe64(reserved_memory[i].addr); > + reg.size = htobe64(reserved_memory[i].size); > + fdt_node_add_property(child, "reg", ®, sizeof(reg)); > + } > + } > + > node = fdt_find_node("/chosen"); > hartid = efi_get_boot_hart_id(); > if (hartid >= 0) { > @@ -973,6 +1035,23 @@ efi_memprobe_find(UINTN pages, UINTN ali > return EFI_OUT_OF_RESOURCES; > } > > +static void > +efi_dumpmem(void) > +{ > + EFI_MEMORY_DESCRIPTOR *mm; > + int i; > + > + efi_memprobe_internal(); /* sync the current map */ > + > + for (i = 0, mm = mmap; i < mmap_ndesc; > + i++, mm = NextMemoryDescriptor(mm, mmap_descsiz)) { > + printf("type 0x%x pa 0x%llx va 0x%llx pages 0x%llx attr 0x%llx\n", > + mm->Type, mm->PhysicalStart, > + mm->VirtualStart, mm->NumberOfPages, > + mm->Attribute); > + } > +} > + > void * > efi_fdt(void) > { > @@ -1060,11 +1139,13 @@ retry: > > int Xdtb_efi(void); > int Xexit_efi(void); > +int Xmemory_efi(void); > int Xpoweroff_efi(void); > > const struct cmd_table cmd_machine[] = { > { "dtb", CMDT_CMD, Xdtb_efi }, > { "exit", CMDT_CMD, Xexit_efi }, > + { "memory", CMDT_CMD, Xmemory_efi }, > { "poweroff", CMDT_CMD, Xpoweroff_efi }, > { NULL, 0 } > }; > @@ -1092,6 +1173,78 @@ Xexit_efi(void) > for (;;) > continue; > return (0); > +} > + > +int > +Xmemory_efi(void) > +{ > + if (cmd.argc >= 2) { > + int i; > + /* parse the memory specs */ > + > + for (i = 1; i < cmd.argc; i++) { > + char *p; > + long long addr, size; > + > + p = cmd.argv[i]; > + > + if (strcmp(p, "reset") == 0) { > + fdt_reset_reserved_memory(); > + continue; > + } > + > + size = strtoll(p + 1, &p, 0); > + /* Size the size */ > + switch (*p) { > + case 'G': > + case 'g': > + size *= 1024; > + case 'M': > + case 'm': > + size *= 1024; > + case 'K': > + case 'k': > + size *= 1024; > + p++; > + } > + > + /* Handle (possibly non-existent) address part */ > + switch (*p) { > + case '@': > + addr = strtoll(p + 1, NULL, 0); > + break; > + > + /* Adjust address if we don't need it */ > + default: > + if (cmd.argv[i][0] == '=') > + addr = -1; > + else > + addr = 0; > + } > + > + if (addr == 0 || size == 0) { > + printf("bad language\n"); > + return 0; > + } else { > + switch (cmd.argv[i][0]) { > + case '-': > + fdt_add_reserved_memory(addr, size); > + break; > + case '=': > + fdt_add_reserved_memory(size, -1L); > + break; > + default : > + printf("bad OP\n"); > + return 0; > + } > + } > + } > + } > + > + efi_dumpmem(); > + fdt_print_reserved_memory(); > + > + return 0; > } > > int > Index: stand/efiboot/fdt.h > =================================================================== > RCS file: /cvs/src/sys/arch/riscv64/stand/efiboot/fdt.h,v > diff -u -p -r1.1 fdt.h > --- stand/efiboot/fdt.h 28 Apr 2021 19:01:00 -0000 1.1 > +++ stand/efiboot/fdt.h 4 Aug 2024 19:46:13 -0000 > @@ -40,6 +40,11 @@ struct fdt { > int struct_size; > }; > > +struct fdt_reg { > + uint64_t addr; > + uint64_t size; > +}; > + > #define FDT_MAGIC 0xd00dfeed > #define FDT_NODE_BEGIN 0x01 > #define FDT_NODE_END 0x02 > > -- > jca >