Index | Thread | Search

From:
Mike Larkin <mlarkin@nested.page>
Subject:
Re: efiboot "machine memory" support for riscv64
To:
tech@openbsd.org
Date:
Mon, 5 Aug 2024 10:18:44 -0700

Download raw body.

Thread
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", &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
>