Re: Problems with implementing EFI runtime support for x86

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Thu, 15 Sep 2022 11:32:02 +0200
> From: Paweł Cichowski <pawel.cichowski%3mdeb.com@localhost>
> 
> I am trying it out now, unfortunately I'm running into this error:
> fatal privileged instruction fault in supervisor mode
> [...]
> kernel: privileged instruction fault trap, code=0
> Stopped in pid 0.0 (system) at netbsd:xrstor+0xa:	fxsave1
> This happens in efi_runtime_exit, more specifically - during
> fpu_kern_leave.  It's probably due to me naively debugging by just
> calling it during efi_init, so now I'll try to do it properly and
> add efi_gettime to /dev/efi.

efi_init is probably too early to call this stuff -- not sure exactly
at what point fpu_kern_enter/leave becomes safe but efi_init may be
too early.

> How did you go about testing it?

I just wrote a little userland program, attached, to list the
variables and query their values using /dev/efi, which I've been
running in qemu booted with OVMF:

qemu-system-x86_64 \
	-machine pc,accel=nvmm \
	-bios /usr/pkg/share/ovmf/OVMFX64.fd \
	...

> My main concern right now is not knowing where to try and start
> contributing to NetBSD, as it really sparked my interest. I found
> this project list https://wiki.netbsd.org/projects/all/#index4h1 and
> will probably look for some features of other BSDs which could be
> ported. Could you tell me where or how I could find some entry-level
> projects?

That's a reasonable place to start, although it's a good idea to ask
about any particular projects first -- many of those are a bit
outdated.

Also always a good strategy to try starting with something you want to
use or adjacent to something you want to use like this EFI business.
I noticed that fwupd needs to get at EFI configuration tables too so I
drafted a patch to expose this through /dev/efi (meant to be source-
compatible with FreeBSD) but I haven't tested it and it probably won't
work on the first try.

#include <sys/cdefs.h>
#include <sys/efiio.h>
#include <sys/ioctl.h>

#include <err.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <uuid.h>

#define	_PATH_EFI	"/dev/efi"

static const struct uuid EFI_GLOBAL_VARIABLE = {
	0x8BE4DF61,0x93CA,0x11d2,
	0xAA,0x0D,{0x00,0xE0,0x98,0x03,0x2B,0x8C},
};

static const unsigned char *const stdvars[] = {
	"AuditMode",
	"Boot0000",
	"Boot0001",
	"Boot0002",
	"Boot0003",
	"Boot0004",
	"Boot0005",
	"Boot0006",
	"Boot0007",
	"BootCurrent",
	"BootNext",
	"BootOrder",
	"BootOptionSupport",
	"ConIn",
	"ConInDev",
	"ConOut",
	"ConOutDev",
	"dbDefault",
	"dbrDefault",
	"dbtDefault",
	"dbxDefault",
	"DeployedMode",
	"Driver0000",
	"Driver0001",
	"Driver0002",
	"Driver0003",
	"DriverOrder",
	"ErrOut",
	"ErrOutDev",
	"HwErrRecSupport",
	"KEK",
	"KEKDefault",
	"Key0000",
	"Key0001",
	"Key0002",
	"Key0003",
	"Lang",
	"LangCodes",
	"OsIndications",
	"OsIndicationsSupported",
	"OsRecoveryOrder",
	"PK",
	"PKDefault",
	"PlatformLangCodes",
	"PlatformLang",
	"PlatformRecovery0000",
	"PlatformRecovery0001",
	"PlatformRecovery0002",
	"PlatformRecovery0003",
	"SignatureSupport",
	"SecureBoot",
	"SetupMode",
	"SysPrep0000",
	"SysPrep0001",
	"SysPrep0002",
	"SysPrep0003",
	"SysPrepOrder",
	"Timeout",
	"VendorKeys",
};

int
main(void)
{
	uint16_t name[128] = {0};
	uint8_t buf[1024];
	struct efi_var_ioc e = {
		.name = name,
		.namesize = __arraycount(name),
	};
	struct efi_var_ioc e1;
	int fd;
	unsigned i, j;

	if ((fd = open(_PATH_EFI, O_RDWR)) == -1)
		err(1, "open");
	for (;;) {
		if (ioctl(fd, EFIIOC_VAR_NEXT, &e) == -1) {
			warn("ioctl(EFIIOC_VAR_NEXT)");
			break;
		}
		if (e.name == NULL)
			break;
		char *uuidstr;
		uint32_t uuidstatus;
		uuid_to_string(&e.vendor, &uuidstr, &uuidstatus);
		if (uuidstatus == uuid_s_ok) {
			printf("[%s] ", uuidstr);
			free(uuidstr);
		} else {
			printf("[err 0x%x] ", uuidstatus);
		}
		for (i = 0; i < __arraycount(name); i++) {
			if (name[i] == 0)
				break;
			if (name[i] >= 256)
				printf("\\U+%04x", name[i]);
			else
				printf("%c", name[i]);
		}
		memset(buf, 0, sizeof(buf));
		e1 = e;
		e1.data = buf;
		e1.datasize = sizeof(buf);
		if (ioctl(fd, EFIIOC_VAR_GET, &e1) == -1) {
			printf("\n");
			fflush(stdout);
			warn("ioctl(EFIIOC_VAR_GET)");
			continue;
		}
		printf(" 0x%x\n", e1.attrib);
		for (i = 0; i < e1.datasize; i++) {
			if ((i % 16) == 8)
				printf(" ");
			printf(" %02x", buf[i]);
			if ((i % 16) == 15)
				printf("\n");
		}
		if (e1.datasize % 16)
			printf("\n");
	}

	for (i = 0; i < __arraycount(stdvars); i++) {
		printf("%s", stdvars[i]);
		for (j = 0; stdvars[i][j] != '\0'; j++)
			name[j] = stdvars[i][j];
		name[j] = 0;
		e.name = name;
		e.namesize = 2*(j + 1);
		e.vendor = EFI_GLOBAL_VARIABLE;
		memset(buf, 0, sizeof(buf));
		e.data = buf;
		e.datasize = sizeof(buf);
		if (ioctl(fd, EFIIOC_VAR_GET, &e) == -1) {
			printf("\n");
			fflush(stdout);
			warn("ioctl(EFIIOC_VAR_GET)");
			continue;
		}
		printf(" 0x%x\n", e.attrib);
		for (j = 0; j < e.datasize; j++) {
			if ((j % 16) == 0)
				printf(" ");
			printf(" %02x", buf[j]);
			if ((j % 16) == 15)
				printf("\n");
		}
		if (e.datasize % 16)
			printf("\n");
	}

	fflush(stdout);
	return ferror(stdout);
}

From cb0f9e34b035742fa323c43867fea1262e4b373c Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Thu, 15 Sep 2022 07:54:19 +0000
Subject: [PATCH 1/2] efi(4): Implement MI parts of EFIIOC_GET_TABLE.

Intended to be compatible with FreeBSD.

Not yet supported on any architectures.
---
 sys/dev/efi.c    | 200 +++++++++++++++++++++++++++++++++++++++++++++++
 sys/dev/efivar.h |   8 +-
 sys/sys/efiio.h  |   8 ++
 3 files changed, 214 insertions(+), 2 deletions(-)

diff --git a/sys/dev/efi.c b/sys/dev/efi.c
index 49a1f2e387e5..67c9b17ddde4 100644
--- a/sys/dev/efi.c
+++ b/sys/dev/efi.c
@@ -40,7 +40,10 @@ __KERNEL_RCSID(0, "$NetBSD: efi.c,v 1.3 2022/04/01 06:51:12 skrll Exp $");
 #include <sys/atomic.h>
 #include <sys/efiio.h>
 
+#include <uvm/uvm_extern.h>
+
 #include <dev/efivar.h>
+#include <dev/mm.h>
 
 #ifdef _LP64
 #define	EFIERR(x)		(0x8000000000000000 | x)
@@ -149,6 +152,201 @@ efi_status_to_error(efi_status status)
 	}
 }
 
+/* XXX move to efi.h */
+#define	EFI_SYSTEM_RESOURCE_TABLE_GUID					      \
+	{0xb122a263,0x3661,0x4f68,0x99,0x29,{0x78,0xf8,0xb0,0xd6,0x21,0x80}}
+#define	EFI_PROPERTIES_TABLE						      \
+	{0x880aaca3,0x4adc,0x4a04,0x90,0x79,{0xb7,0x47,0x34,0x08,0x25,0xe5}}
+
+#define	EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION	1
+
+struct EFI_SYSTEM_RESOURCE_ENTRY {
+	struct uuid	FwClass;
+	uint32_t	FwType;
+	uint32_t	FwVersion;
+	uint32_t	LowestSupportedFwVersion;
+	uint32_t	CapsuleFlags;
+	uint32_t	LastAttemptVersion;
+	uint32_t	LastAttemptStatus;
+};
+
+struct EFI_SYSTEM_RESOURCE_TABLE {
+	uint32_t	FwResourceCount;
+	uint32_t	FwResourceCountMax;
+	uint64_t	FwResourceVersion;
+	struct EFI_SYSTEM_RESOURCE_ENTRY	Entries[];
+};
+
+static void *
+efi_map_pa(uint64_t addr, bool *directp)
+{
+	paddr_t pa = addr;
+	vaddr_t va;
+
+	/*
+	 * Verify the address is not truncated by conversion to
+	 * paddr_t.  This might happen with a 64-bit EFI booting a
+	 * 32-bit OS.
+	 */
+	if (pa != addr)
+		return NULL;
+
+	/*
+	 * Try direct-map if we have it.  If it works, note that it was
+	 * direct-mapped for efi_unmap.
+	 */
+#ifdef __HAVE_MM_MD_DIRECT_MAPPED_PHYS
+	if (mm_md_direct_mapped_phys(pa, &va)) {
+		*directp = true;
+		return (void *)va;
+	}
+#endif
+
+	/*
+	 * No direct map.  Reserve a page of kernel virtual address
+	 * space, with no backing, to map to the physical address.
+	 */
+	va = uvm_km_alloc(kernel_map, PAGE_SIZE, 0,
+	    UVM_KMF_VAONLY|UVM_KMF_WAITVA);
+	KASSERT(va != 0);
+
+	/*
+	 * Map the kva page to the physical address and update the
+	 * kernel pmap so we can use it.
+	 */
+	pmap_kenter_pa(va, pa, VM_PROT_READ, 0);
+	pmap_update(pmap_kernel());
+
+	/*
+	 * Success!  Return the VA and note that it was not
+	 * direct-mapped for efi_unmap.
+	 */
+	*directp = false;
+	return (void *)va;
+}
+
+static void
+efi_unmap(void *ptr, bool direct)
+{
+	vaddr_t va = (vaddr_t)ptr;
+
+	/*
+	 * If it was direct-mapped, nothing to do here.
+	 */
+	if (direct)
+		return;
+
+	/*
+	 * First remove the mapping from the kernel pmap so that it can
+	 * be reused, before we free the kva and let anyone else reuse
+	 * it.
+	 */
+	pmap_kremove(va, PAGE_SIZE);
+	pmap_update(pmap_kernel());
+
+	/*
+	 * Next free the kva so it can be reused by someone else.
+	 */
+	uvm_km_free(kernel_map, va, PAGE_SIZE, UVM_KMF_VAONLY);
+}
+
+static int
+efi_ioctl_got_table(struct efi_get_table_ioc *ioc, void *ptr, size_t len)
+{
+
+	/*
+	 * Return the actual table length.
+	 */
+	ioc->table_len = len;
+
+	/*
+	 * Copy out as much as we can into the user's allocated buffer.
+	 */
+	return copyout(ioc->buf, ptr, MIN(ioc->buf_len, len));
+}
+
+static int
+efi_ioctl_get_esrt(struct efi_get_table_ioc *ioc,
+    struct EFI_SYSTEM_RESOURCE_TABLE *tab)
+{
+
+	/*
+	 * Verify the firmware resource version is one we understand.
+	 */
+	if (tab->FwResourceVersion !=
+	    EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION)
+		return ENOENT;
+
+	/*
+	 * Verify the resource count fits within the single page we
+	 * have mapped.
+	 *
+	 * XXX What happens if it doesn't?  Are we expected to map more
+	 * than one page, according to the table header?  The UEFI spec
+	 * is unclear on this.
+	 */
+	const size_t entry_space = PAGE_SIZE -
+	    offsetof(struct EFI_SYSTEM_RESOURCE_TABLE, Entries);
+	if (tab->FwResourceCount > entry_space/sizeof(tab->Entries[0]))
+		return ENOENT;
+
+	/*
+	 * Success!  Return everything through the last table entry.
+	 */
+	const size_t len = offsetof(struct EFI_SYSTEM_RESOURCE_TABLE,
+	    Entries[tab->FwResourceCount]);
+	return efi_ioctl_got_table(ioc, tab, len);
+}
+
+static int
+efi_ioctl_get_table(struct efi_get_table_ioc *ioc)
+{
+	uint64_t addr;
+	bool direct;
+	efi_status status;
+	int error;
+
+	/*
+	 * If the platform doesn't support it yet, fail now.
+	 */
+	if (efi_ops->efi_gettab == NULL)
+		return ENODEV;
+
+	/*
+	 * Get the address of the requested table out of the EFI
+	 * configuration table.
+	 */
+	status = efi_ops->efi_gettab(&ioc->uuid, &addr);
+	if (status != EFI_SUCCESS)
+		return efi_status_to_error(status);
+
+	/*
+	 * UEFI provides no generic way to identify the size of the
+	 * table, so we have to bake knowledge of every vendor GUID
+	 * into this code to safely expose the right amount of data to
+	 * userland.
+	 *
+	 * We even have to bake knowledge of which ones are physically
+	 * addressed and which ones might be virtually addressed
+	 * according to the vendor GUID into this code, although for
+	 * the moment we never use RT->SetVirtualAddressMap so we only
+	 * ever have to deal with physical addressing.
+	 */
+	if (memcmp(&ioc->uuid, &(struct uuid)EFI_SYSTEM_RESOURCE_TABLE_GUID,
+		sizeof(ioc->uuid)) == 0) {
+		struct EFI_SYSTEM_RESOURCE_TABLE *tab;
+
+		if ((tab = efi_map_pa(addr, &direct)) == NULL)
+			return ENOENT;
+		error = efi_ioctl_get_esrt(ioc, tab);
+		efi_unmap(tab, direct);
+	} else {
+		error = ENOENT;
+	}
+
+	return error;
+}
+
 static int
 efi_ioctl_var_get(struct efi_var_ioc *var)
 {
@@ -289,6 +487,8 @@ efi_ioctl(dev_t dev, u_long cmd, void *data, int flags, struct lwp *l)
 	KASSERT(efi_ops != NULL);
 
 	switch (cmd) {
+	case EFIIOC_GET_TABLE:
+		return efi_ioctl_get_table(data);
 	case EFIIOC_VAR_GET:
 		return efi_ioctl_var_get(data);
 	case EFIIOC_VAR_NEXT:
diff --git a/sys/dev/efivar.h b/sys/dev/efivar.h
index 21d6d61fd26a..72aeb8c6fbae 100644
--- a/sys/dev/efivar.h
+++ b/sys/dev/efivar.h
@@ -29,16 +29,20 @@
 #ifndef _DEV_EFIVAR_H
 #define _DEV_EFIVAR_H
 
+#include <sys/uuid.h>
+#include <sys/types.h>
+
 #include <machine/efi.h>
 
 struct efi_ops {
 	efi_status	(*efi_gettime)(struct efi_tm *, struct efi_tmcap *);
 	efi_status	(*efi_settime)(struct efi_tm *);
 	efi_status	(*efi_getvar)(uint16_t *, struct uuid *, uint32_t *,
-				      u_long *, void *);
+			    u_long *, void *);
 	efi_status	(*efi_setvar)(uint16_t *, struct uuid *, uint32_t,
-				      u_long, void *);
+			    u_long, void *);
 	efi_status	(*efi_nextvar)(u_long *, uint16_t *, struct uuid *);
+	efi_status	(*efi_gettab)(const struct uuid *, uint64_t *);
 };
 
 void	efi_register_ops(const struct efi_ops *);
diff --git a/sys/sys/efiio.h b/sys/sys/efiio.h
index 8f3a9a2d54e9..c50c2c416fa9 100644
--- a/sys/sys/efiio.h
+++ b/sys/sys/efiio.h
@@ -48,6 +48,13 @@
 #define	EFI_VARIABLE_APPEND_WRITE				0x00000040
 #define	EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS		0x00000080
 
+struct efi_get_table_ioc {
+	void *		buf;
+	struct uuid	uuid;
+	size_t		table_len;
+	size_t		buf_len;
+};
+
 struct efi_var_ioc {
 	uint16_t *	name;		/* vendor's variable name */
 	size_t		namesize;	/* size in bytes of the name buffer */
@@ -57,6 +64,7 @@ struct efi_var_ioc {
 	size_t		datasize;	/* size in bytes of the data buffer */
 };
 
+#define	EFIIOC_GET_TABLE	_IOWR('e', 1, struct efi_get_table_ioc)
 #define	EFIIOC_VAR_GET		_IOWR('e', 4, struct efi_var_ioc)
 #define	EFIIOC_VAR_NEXT		_IOWR('e', 5, struct efi_var_ioc)
 #define	EFIIOC_VAR_SET		_IOWR('e', 7, struct efi_var_ioc)

From f4b317934d0f1604190d8cc1cfe5dd8406bef5fe Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Thu, 15 Sep 2022 07:55:02 +0000
Subject: [PATCH 2/2] efi(4): Implement EFIIOC_GET_TABLE on x86.

---
 sys/arch/x86/x86/efi_machdep.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/sys/arch/x86/x86/efi_machdep.c b/sys/arch/x86/x86/efi_machdep.c
index 22c96e460d6c..6f29511c484b 100644
--- a/sys/arch/x86/x86/efi_machdep.c
+++ b/sys/arch/x86/x86/efi_machdep.c
@@ -589,8 +589,10 @@ efi_get_e820memmap(void)
 #define	EFIERR(x)	(0x80000000ul | (x))
 #endif
 
+#define	EFI_SUCCESS		EFIERR(0)
 #define	EFI_UNSUPPORTED		EFIERR(3)
 #define	EFI_DEVICE_ERROR	EFIERR(7)
+#define	EFI_NOT_FOUND		EFIERR(14)
 
 /*
  * efi_runtime_init()
@@ -963,12 +965,29 @@ efi_runtime_setvar(efi_char *name, struct uuid *vendor, uint32_t attrib,
 	return status;
 }
 
+static efi_status
+efi_runtime_gettab(const struct uuid *vendor, uint64_t *addrp)
+{
+	struct efi_cfgtbl *cfgtbl = efi_getcfgtblhead();
+	paddr_t pa;
+
+	if (cfgtbl == NULL)
+		return EFI_UNSUPPORTED;
+
+	pa = efi_getcfgtblpa(vendor);
+	if (pa == 0)
+		return EFI_NOT_FOUND;
+	*addrp = pa;
+	return EFI_SUCCESS;
+}
+
 static struct efi_ops efi_runtime_ops = {
 	.efi_gettime = efi_runtime_gettime,
 	.efi_settime = efi_runtime_settime,
 	.efi_getvar = efi_runtime_getvar,
 	.efi_setvar = efi_runtime_setvar,
 	.efi_nextvar = efi_runtime_nextvar,
+	.efi_gettab = efi_runtime_gettab,
 };
 
 #endif	/* EFI_RUNTIME */