[PATCH v1 0/3] firmware: coreboot: Support for System Management Interrupt (SMI) handling in coreboot payload (MM payload concept)

[PATCH v1 0/3] firmware: coreboot: Support for System Management Interrupt (SMI) handling in coreboot payload (MM payload concept)

[Michal Gorlas]

Hi,

This patchset adds support for MM payload when Linux is used as
coreboot's payload. The main idea is to delegate higher-level SMM
functions to the payload, limiting the coreboot's System Management
Mode (SMM) related responsibilities to the minimum of basic SMM setup [1]. This is 
done by loading a blob with SMI handler to the shared buffer, from 
which it is copied to SMRAM [2].

The MM payload is still in a Proof of Concept stage, and we are still 
working on getting the patches needed for coreboot upstreamed, but I would
appreciate any feedback that you may have.

Thanks,
Michal

[1]: https://github.com/9elements/LinuxBootSMM/wiki/Proposed-design#current-design-approach---mm-payload
[2]: https://github.com/9elements/LinuxBootSMM/wiki/Proof-of-Concept-(PoC)

Michal Gorlas (3):
  firmware: coreboot: support for parsing SMM related informations from
    coreboot tables
  firmware: coreboot: loader for Linux-owned SMI handler
  firmware: coreboot: Linux-owned SMI handler to be loaded by coreboot

 drivers/firmware/google/Kconfig               |  12 +
 drivers/firmware/google/Makefile              |  12 +
 drivers/firmware/google/coreboot_table.h      |  34 +-
 drivers/firmware/google/mm_blob.S             |  20 +
 drivers/firmware/google/mm_handler/Makefile   |  51 ++
 .../firmware/google/mm_handler/handler.lds.S  |  46 ++
 .../firmware/google/mm_handler/mm_handler.S   | 510 ++++++++++++++++++
 .../firmware/google/mm_handler/mm_handler.h   |  21 +
 .../firmware/google/mm_handler/mm_header.S    |  19 +
 drivers/firmware/google/mm_info.c             |  63 +++
 drivers/firmware/google/mm_loader.c           | 186 +++++++
 drivers/firmware/google/mm_payload.h          |  58 ++
 12 files changed, 1020 insertions(+), 12 deletions(-)
 create mode 100644 drivers/firmware/google/mm_blob.S
 create mode 100644 drivers/firmware/google/mm_handler/Makefile
 create mode 100644 drivers/firmware/google/mm_handler/handler.lds.S
 create mode 100644 drivers/firmware/google/mm_handler/mm_handler.S
 create mode 100644 drivers/firmware/google/mm_handler/mm_handler.h
 create mode 100644 drivers/firmware/google/mm_handler/mm_header.S
 create mode 100644 drivers/firmware/google/mm_info.c
 create mode 100644 drivers/firmware/google/mm_loader.c
 create mode 100644 drivers/firmware/google/mm_payload.h

-- 
2.49.0

[PATCH v1 1/3] firmware: coreboot: support for parsing SMM related informations from coreboot tables

[Michal Gorlas]

coreboot exposes (S)MM related data in the coreboot table. Extends existing interface,
with structure corresponding to (S)MM data, and adds parser. Parser exposes this data
to the modules executed later.

Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
---
 drivers/firmware/google/Kconfig          | 12 +++++
 drivers/firmware/google/Makefile         |  3 ++
 drivers/firmware/google/coreboot_table.h | 34 ++++++++-----
 drivers/firmware/google/mm_info.c        | 63 ++++++++++++++++++++++++
 drivers/firmware/google/mm_payload.h     | 58 ++++++++++++++++++++++
 5 files changed, 158 insertions(+), 12 deletions(-)
 create mode 100644 drivers/firmware/google/mm_info.c
 create mode 100644 drivers/firmware/google/mm_payload.h

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 41b78f5cb735..5d918c076f25 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -81,4 +81,16 @@ config GOOGLE_VPD
 	  This option enables the kernel to expose the content of Google VPD
 	  under /sys/firmware/vpd.
 
+config COREBOOT_PAYLOAD_MM
+	tristate "SMI handling in Linux (LinuxBootSMM)"
+	depends on GOOGLE_COREBOOT_TABLE
+	help
+	  Enables support for SMI handling by Linux-owned code.
+	  coreboot reserves region for payload-owned SMI handler, the Linux
+	  driver prepares its SMI handler outside of SMRAM, and lets coreboot
+	  know where the handler is placed by issuing an SMI. On this SMI, the
+	  handler is being placed in SMRAM and all supported SMIs from that point
+	  on are handled by Linux-owned SMI handler.
+	  If in doubt, say N.
+
 endif # GOOGLE_FIRMWARE
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index 8151e323cc43..d2a690e8379d 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -12,3 +12,6 @@ obj-$(CONFIG_GOOGLE_CBMEM)		+= cbmem.o
 
 vpd-sysfs-y := vpd.o vpd_decode.o
 obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
+
+# LinuxBootSMM related.
+obj-$(CONFIG_COREBOOT_PAYLOAD_MM)		+= mm_info.o
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index bb6f0f7299b4..e0b087933c5a 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -41,7 +41,6 @@ struct lb_cbmem_ref {
 };
 
 #define LB_TAG_CBMEM_ENTRY 0x31
-
 /* Corresponds to LB_TAG_CBMEM_ENTRY */
 struct lb_cbmem_entry {
 	u32 tag;
@@ -52,6 +51,16 @@ struct lb_cbmem_entry {
 	u32 id;
 };
 
+/* Corresponds to LB_TAG_PLD_MM_INTERFACE_INFO */
+#define LB_TAG_PLD_MM_INTERFACE_INFO 0x3b
+struct lb_pld_mm_interface_info {
+	u32 tag;
+	u32 size;
+	u8 revision;
+	u8 requires_long_mode_call;
+	u8 register_mm_entry_command;
+};
+
 /* Describes framebuffer setup by coreboot */
 struct lb_framebuffer {
 	u32 tag;
@@ -61,15 +70,15 @@ struct lb_framebuffer {
 	u32 x_resolution;
 	u32 y_resolution;
 	u32 bytes_per_line;
-	u8  bits_per_pixel;
-	u8  red_mask_pos;
-	u8  red_mask_size;
-	u8  green_mask_pos;
-	u8  green_mask_size;
-	u8  blue_mask_pos;
-	u8  blue_mask_size;
-	u8  reserved_mask_pos;
-	u8  reserved_mask_size;
+	u8 bits_per_pixel;
+	u8 red_mask_pos;
+	u8 red_mask_size;
+	u8 green_mask_pos;
+	u8 green_mask_size;
+	u8 blue_mask_pos;
+	u8 blue_mask_size;
+	u8 reserved_mask_pos;
+	u8 reserved_mask_size;
 };
 
 /* A device, additionally with information from coreboot. */
@@ -80,6 +89,7 @@ struct coreboot_device {
 		struct lb_cbmem_ref cbmem_ref;
 		struct lb_cbmem_entry cbmem_entry;
 		struct lb_framebuffer framebuffer;
+		struct lb_pld_mm_interface_info mm_info;
 		DECLARE_FLEX_ARRAY(u8, raw);
 	};
 };
@@ -112,8 +122,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
  * boilerplate.  Each module may only use this macro once, and
  * calling it replaces module_init() and module_exit()
  */
-#define module_coreboot_driver(__coreboot_driver) \
+#define module_coreboot_driver(__coreboot_driver)                  \
 	module_driver(__coreboot_driver, coreboot_driver_register, \
-			coreboot_driver_unregister)
+		      coreboot_driver_unregister)
 
 #endif /* __COREBOOT_TABLE_H */
diff --git a/drivers/firmware/google/mm_info.c b/drivers/firmware/google/mm_info.c
new file mode 100644
index 000000000000..55bcdc8b8d53
--- /dev/null
+++ b/drivers/firmware/google/mm_info.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * mm_info.c
+ *
+ * Driver for exporting MM payload information from coreboot table.
+ *
+ * Copyright 2025 9elements gmbh
+ * Copyright 2025 Michal Gorlas <michal.gorlas@9elements.com>
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "coreboot_table.h"
+#include "mm_payload.h"
+
+static struct lb_pld_mm_interface_info *mm_cbtable_info;
+struct mm_info *mm_info;
+
+static int mm_driver_probe(struct coreboot_device *dev)
+{
+	mm_cbtable_info = &dev->mm_info;
+	if (mm_cbtable_info->tag != LB_TAG_PLD_MM_INTERFACE_INFO)
+		return -ENXIO;
+
+	mm_info = kmalloc(sizeof(*mm_info), GFP_KERNEL);
+	mm_info->revision = mm_cbtable_info->revision;
+	mm_info->requires_long_mode_call =
+		mm_cbtable_info->requires_long_mode_call;
+	mm_info->register_mm_entry_command =
+		mm_cbtable_info->register_mm_entry_command;
+	return 0;
+}
+EXPORT_SYMBOL(mm_info);
+
+static void mm_driver_remove(struct coreboot_device *dev)
+{
+	if (mm_info)
+		kfree(mm_info);
+}
+
+static const struct coreboot_device_id mm_info_ids[] = {
+	{ .tag = LB_TAG_PLD_MM_INTERFACE_INFO },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(coreboot, mm_info_ids);
+
+static struct coreboot_driver mm_driver = {
+	.probe = mm_driver_probe,
+	.remove = mm_driver_remove,
+	.drv = {
+		.name = "mm_info",
+	},
+	.id_table = mm_info_ids,
+};
+
+module_coreboot_driver(mm_driver);
+
+MODULE_AUTHOR("Michal Gorlas <michal.gorlas@9elements.com>");
+MODULE_DESCRIPTION("Driver for exporting MM info from coreboot table");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/firmware/google/mm_payload.h b/drivers/firmware/google/mm_payload.h
new file mode 100644
index 000000000000..bb2f55c4f240
--- /dev/null
+++ b/drivers/firmware/google/mm_payload.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * mm_payload.h
+ *
+ * Internal header for MM payload driver.
+ *
+ * Copyright 2025 9elements gmbh
+ * Copyright 2025 Michal Gorlas <michal.gorlas@9elements.com>
+ */
+
+#ifndef __MM_PAYLOAD_H
+#define __MM_PAYLOAD_H
+
+#define PAYLOAD_MM_RET_SUCCESS 0
+#define PAYLOAD_MM_RET_FAILURE 1
+#define PAYLOAD_MM_REGISTER_ENTRY 2
+
+#define REALMODE_END_SIGNATURE	0x65a22c82
+
+struct mm_info {
+	u8 revision;
+	u8 requires_long_mode_call;
+	u8 register_mm_entry_command;
+};
+
+extern struct mm_info *mm_info;
+
+#ifndef __ASSEMBLY__
+
+#include <linux/types.h>
+
+/* This must match data at mm_handler/mm_header.S */
+struct mm_header {
+	u32	text_start;
+	u32	mm_entry_32;
+	u32	mm_entry_64;
+	u32	mm_signature;
+	u32	mm_blob_size;
+};
+
+extern struct mm_header *mm_header;
+extern unsigned char mm_blob_end[];
+
+extern unsigned char mm_blob[];
+extern unsigned char mm_relocs[];
+
+/*
+ * This has to be under 1MB (see tseg_region.c in coreboot source tree).
+ * The actual check for this is made in coreboot after we fill the header
+ * (see above) with the blob size.
+ */
+static inline size_t mm_payload_size_needed(void)
+{
+	return mm_blob_end - mm_blob;
+}
+
+#endif /* __ASSEMBLER__ */
+#endif /* __MM_PAYLOAD_H */
-- 
2.49.0

[PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler

[Michal Gorlas]

Places a blob with Linux-owned SMI handler in the lower 4GB of memory, calculates
entry points for the it and triggers SMI to coreboot's SMI handler
informing it where to look for Linux-owned SMI handler.

Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
---
 drivers/firmware/google/Makefile    |   9 ++
 drivers/firmware/google/mm_blob.S   |  20 +++
 drivers/firmware/google/mm_loader.c | 186 ++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/firmware/google/mm_blob.S
 create mode 100644 drivers/firmware/google/mm_loader.c

diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d2a690e8379d..eab5a62d7500 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -15,3 +15,12 @@ obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
 
 # LinuxBootSMM related.
 obj-$(CONFIG_COREBOOT_PAYLOAD_MM)		+= mm_info.o
+payload-mm-$(CONFIG_COREBOOT_PAYLOAD_MM)	:= mm_loader.o mm_blob.o
+
+subdir-						:= mm_handler
+obj-$(CONFIG_COREBOOT_PAYLOAD_MM)		+= payload-mm.o
+
+$(obj)/mm_blob.o: $(obj)/mm_handler/handler.bin
+
+$(obj)/mm_handler/handler.bin: FORCE
+	$(Q)$(MAKE) $(build)=$(obj)/mm_handler $@
diff --git a/drivers/firmware/google/mm_blob.S b/drivers/firmware/google/mm_blob.S
new file mode 100644
index 000000000000..87557d67c47b
--- /dev/null
+++ b/drivers/firmware/google/mm_blob.S
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Derived from rmpiggy.S.
+ *
+ * Wrapper script for the MM payload binary as a transport object
+ * before copying to SMRAM memory.
+ */
+#include <linux/linkage.h>
+#include <asm/page_types.h>
+
+	.section ".init.data","aw"
+
+	.balign PAGE_SIZE
+
+SYM_DATA_START(mm_blob)
+	.incbin	"drivers/firmware/google/mm_handler/handler.bin"
+SYM_DATA_END_LABEL(mm_blob, SYM_L_GLOBAL, mm_blob_end)
+
+SYM_DATA_START(mm_relocs)
+	.incbin	"drivers/firmware/google/mm_handler/handler.relocs"
+SYM_DATA_END(mm_relocs)
diff --git a/drivers/firmware/google/mm_loader.c b/drivers/firmware/google/mm_loader.c
new file mode 100644
index 000000000000..51fbfd07f525
--- /dev/null
+++ b/drivers/firmware/google/mm_loader.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for installing Linux-owned SMI handler
+ *
+ * Copyright (c) 2025 9elements GmbH
+ *
+ * Author: Michal Gorlas <michal.gorlas@9elements.com>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/cpu.h>
+#include <linux/delay.h>
+#include <linux/gfp.h>
+#include <linux/mm.h>
+
+#include "mm_payload.h"
+
+#define DRIVER_NAME "mm_loader"
+
+struct mm_header *mm_header;
+
+static void *shared_buffer;
+static size_t blob_size;
+
+/*
+ * This is x86_64 specific, assuming that we want this to also work on i386,
+ * we either need to have "trigger_smi32" bounded by preprocessor guards(?)
+ * or mm_loader32 and then mm_loader$(BITS) in Makefile(?).
+ */
+static int trigger_smi(u64 cmd, u64 arg, u64 retry)
+{
+	u64 status;
+	u16 apmc_port = 0xb2;
+
+	asm volatile("movq	%[cmd],  %%rax\n\t"
+		     "movq   %%rax,	%%rcx\n\t"
+		     "movq	%[arg],  %%rbx\n\t"
+		     "movq   %[retry],  %%r8\n\t"
+		     ".trigger:\n\t"
+		     "mov	%[apmc_port], %%dx\n\t"
+		     "outb	%%al, %%dx\n\t"
+		     "cmpq	%%rcx, %%rax\n\t"
+		     "jne .return_changed\n\t"
+		     "pushq  %%rcx\n\t"
+		     "movq   $10000, %%rcx\n\t"
+		     "rep    nop\n\t"
+		     "popq   %%rcx\n\t"
+		     "cmpq   $0, %%r8\n\t"
+		     "je     .return_not_changed\n\t"
+		     "decq   %%r8\n\t"
+		     "jmp    .trigger\n\t"
+		     ".return_changed:\n\t"
+		     "movq	%%rax, %[status]\n\t"
+		     "jmp	.end\n\t"
+		     ".return_not_changed:"
+		     "movq	%%rcx, %[status]\n\t"
+		     ".end:\n\t"
+		     : [status] "=r"(status)
+		     : [cmd] "r"(cmd), [arg] "r"(arg), [retry] "r"(retry),
+		       [apmc_port] "r"(apmc_port)
+		     : "%rax", "%rbx", "%rdx", "%rcx", "%r8");
+
+	if (status == cmd || status == PAYLOAD_MM_RET_FAILURE)
+		status = PAYLOAD_MM_RET_FAILURE;
+	else
+		status = PAYLOAD_MM_RET_SUCCESS;
+
+	return status;
+}
+
+static int register_entry_point(struct mm_info *data, uint32_t entry_point)
+{
+	u64 cmd;
+	u8 status;
+
+	cmd = data->register_mm_entry_command |
+	      (PAYLOAD_MM_REGISTER_ENTRY << 8);
+	status = trigger_smi(cmd, entry_point, 5);
+	pr_info(DRIVER_NAME ": %s: SMI returned %x\n", __func__, status);
+
+	return status;
+}
+
+static u32 __init place_handler(void)
+{
+	/*
+	 * The handler (aka MM blob) has to be placed in low 4GB of the memory.
+	 * This is because we can not assume that coreboot will be in long mode
+	 * while trying to copy the blob to SMRAM. Even if so, (can be checked by
+	 * reading cb_data->mm_info.requires_long_mode_call), it would make our life
+	 * way too complicated (e.g. no need for shared page table).
+	 */
+	size_t entry32_offset;
+	size_t entry64_offset;
+	u16 real_mode_seg;
+	const u32 *rel;
+	u32 count;
+	unsigned long phys_base;
+
+	blob_size = mm_payload_size_needed();
+	shared_buffer = (void *)__get_free_pages(GFP_DMA32, get_order(blob_size));
+
+	memcpy(shared_buffer, mm_blob, blob_size);
+	wbinvd();
+
+	/*
+	 * Based on arch/x86/realmode/init.c
+	 * The sole purpose of doing relocations is to be able to calculate the offsets
+	 * for entry points. While the absolute addresses are not valid anymore after the
+	 * blob is copied to SMRAM, the distances between sections stay the same, so we
+	 * can still calculate the correct entry point based on coreboot's bitness.
+	 */
+	phys_base = __pa(shared_buffer);
+	real_mode_seg = phys_base >> 4;
+	rel = (u32 *)mm_relocs;
+
+	/* 16-bit segment relocations. */
+	count = *rel++;
+	while (count--) {
+		u16 *seg = (u16 *)(shared_buffer + *rel++);
+		*seg = real_mode_seg;
+	}
+
+	/* 32-bit linear relocations. */
+	count = *rel++;
+	while (count--) {
+		u32 *ptr = (u32 *)(shared_buffer + *rel++);
+		*ptr += phys_base;
+	}
+
+	mm_header =  (struct mm_header *)shared_buffer;
+
+	mm_header->mm_signature = REALMODE_END_SIGNATURE;
+	mm_header->mm_blob_size = mm_payload_size_needed();
+
+	/* At this point relocations are done and we can do some cool
+	 * pointer arithmetics to help coreboot determine correct entry
+	 * point based on offsets.
+	 */
+	entry32_offset = mm_header->mm_entry_32 - (unsigned long)shared_buffer;
+	entry64_offset = mm_header->mm_entry_64 - (unsigned long)shared_buffer;
+
+	mm_header->mm_entry_32 = entry32_offset;
+	mm_header->mm_entry_64 = entry64_offset;
+
+	return (unsigned long)shared_buffer;
+}
+
+static int __init mm_loader_init(void)
+{
+	u32 entry_point;
+
+	if (!mm_info)
+		return -ENOMEM;
+
+	entry_point = place_handler();
+
+	if (register_entry_point(mm_info, entry_point)) {
+		pr_warn(DRIVER_NAME ": registering entry point for MM payload failed.\n");
+		kfree(mm_info);
+		mm_info = NULL;
+		free_pages((unsigned long)shared_buffer, get_order(blob_size));
+		return -1;
+	}
+
+	mdelay(100);
+
+	kfree(mm_info);
+	mm_info = NULL;
+	free_pages((unsigned long)shared_buffer, get_order(blob_size));
+
+	return 0;
+}
+
+static void __exit mm_loader_exit(void)
+{
+	pr_info(DRIVER_NAME ": DONE\n");
+}
+
+module_init(mm_loader_init);
+module_exit(mm_loader_exit);
+
+MODULE_AUTHOR("Michal Gorlas <michal.gorlas@9elements.com>");
+MODULE_DESCRIPTION("MM Payload loader - installs Linux-owned SMI handler");
+MODULE_LICENSE("GPL v2");
-- 
2.49.0

[PATCH v1 3/3] firmware: coreboot: Linux-owned SMI handler to be loaded by coreboot

[Michal Gorlas]

Compiled in similar fashion to the realmode trampolines for x86. Currently
supported are two SMIs: ACPI enable and disable. After being placed in SMRAM,
this handler takes over handling of the supported SMIs from coreboot.

Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
---
 drivers/firmware/google/mm_handler/Makefile   |  51 ++
 .../firmware/google/mm_handler/handler.lds.S  |  46 ++
 .../firmware/google/mm_handler/mm_handler.S   | 510 ++++++++++++++++++
 .../firmware/google/mm_handler/mm_handler.h   |  21 +
 .../firmware/google/mm_handler/mm_header.S    |  19 +
 5 files changed, 647 insertions(+)
 create mode 100644 drivers/firmware/google/mm_handler/Makefile
 create mode 100644 drivers/firmware/google/mm_handler/handler.lds.S
 create mode 100644 drivers/firmware/google/mm_handler/mm_handler.S
 create mode 100644 drivers/firmware/google/mm_handler/mm_handler.h
 create mode 100644 drivers/firmware/google/mm_handler/mm_header.S

diff --git a/drivers/firmware/google/mm_handler/Makefile b/drivers/firmware/google/mm_handler/Makefile
new file mode 100644
index 000000000000..c0069e88f51d
--- /dev/null
+++ b/drivers/firmware/google/mm_handler/Makefile
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: GPL-2.0
+
+## Based on realmode/rm/Makefile
+
+always-y := handler.bin handler.relocs
+
+handler-y			+= mm_header.o
+handler-y			+= mm_handler.o
+
+targets	+= $(handler-y)
+
+REALMODE_OBJS = $(addprefix $(obj)/,$(handler-y))
+
+sed-pasyms := -n -r -e 's/^([0-9a-fA-F]+) [ABCDGRSTVW] (.+)$$/pa_\2 = \2;/p'
+
+quiet_cmd_pasyms = PASYMS  $@
+      cmd_pasyms = $(NM) $(real-prereqs) | sed $(sed-pasyms) | sort | uniq > $@
+
+targets += pasyms.h
+$(obj)/pasyms.h: $(REALMODE_OBJS) FORCE
+	$(call if_changed,pasyms)
+
+targets += handler.lds
+$(obj)/handler.lds: $(obj)/pasyms.h
+
+LDFLAGS_handler.elf := -m elf_i386 --emit-relocs -T
+CPPFLAGS_handler.lds += -P -C -I$(objtree)/$(obj)
+
+targets += handler.elf
+$(obj)/handler.elf: $(obj)/handler.lds $(REALMODE_OBJS) FORCE
+	$(call if_changed,ld)
+
+OBJCOPYFLAGS_handler.bin := -O binary
+
+targets += handler.bin
+$(obj)/handler.bin: $(obj)/handler.elf $(obj)/handler.relocs FORCE
+	$(call if_changed,objcopy)
+
+quiet_cmd_relocs = RELOCS  $@
+      cmd_relocs = arch/x86/tools/relocs --realmode $< > $@
+
+targets += handler.relocs
+$(obj)/handler.relocs: $(obj)/handler.elf FORCE
+	$(call if_changed,relocs)
+
+# ---------------------------------------------------------------------------
+
+KBUILD_CFLAGS	:= $(REALMODE_CFLAGS) -D_SETUP -D_WAKEUP \
+		   -I$(srctree)/arch/x86/boot
+KBUILD_AFLAGS	:= $(KBUILD_CFLAGS) -D__ASSEMBLY__
+KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables
diff --git a/drivers/firmware/google/mm_handler/handler.lds.S b/drivers/firmware/google/mm_handler/handler.lds.S
new file mode 100644
index 000000000000..c92c9f2fbd62
--- /dev/null
+++ b/drivers/firmware/google/mm_handler/handler.lds.S
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * blob.lds.S
+ *
+ * Linker script for the MM handler. Based on realmode.lds.S
+ */
+
+#include <asm/page_types.h>
+
+#undef i386
+
+OUTPUT_FORMAT("elf32-i386")
+OUTPUT_ARCH(i386)
+ENTRY(pa_text_start)
+
+SECTIONS
+{
+	. = 0;
+	.header : {
+		*(.header)
+	}
+
+	pa_text_start = .;
+	.text32 : {
+		*(.text32)
+		*(.text32.*)
+	}
+
+	.text64 : {
+		*(.text64)
+		*(.text64.*)
+	}
+
+	. = ALIGN(128);
+	.bss : {
+		*(.bss*)
+	}
+
+	/DISCARD/ : {
+		*(.data*)
+		*(.note*)
+		*(.debug*)
+	}
+
+#include "pasyms.h"
+}
diff --git a/drivers/firmware/google/mm_handler/mm_handler.S b/drivers/firmware/google/mm_handler/mm_handler.S
new file mode 100644
index 000000000000..19322010a423
--- /dev/null
+++ b/drivers/firmware/google/mm_handler/mm_handler.S
@@ -0,0 +1,510 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *	Payload owned SMI handler that is placed in SMRAM (mm_loader.c) and called
+ *	by coreboot's SMI handler
+ *
+ *	Also the general comment in arch/x86/realmode/rm/trampoline_64.S is relevant
+ *	here as well.
+ */
+
+#include <linux/linkage.h>
+#include <asm/pgtable_types.h>
+#include <asm/page_types.h>
+#include <asm/msr.h>
+#include <asm/segment.h>
+#include <asm/processor-flags.h>
+#include <asm/realmode.h>
+
+#include "mm_handler.h"
+
+	.section ".text32","ax"
+	.code32
+	.balign 4
+SYM_CODE_START(mm_entry_32)
+	mov	$0x3f8, %dx
+	mov	$'L', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'u', %al
+	out	%al, (%dx)
+	mov	$'x', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'S', %al
+	out	%al, (%dx)
+	mov	$'M', %al
+	out	%al, (%dx)
+	mov	$'I', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'h', %al
+	out	%al, (%dx)
+	mov	$'a', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'d', %al
+	out	%al, (%dx)
+	mov	$'l', %al
+	out	%al, (%dx)
+	mov	$'e', %al
+	out	%al, (%dx)
+	mov	$'r', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'s', %al
+	out	%al, (%dx)
+	mov	$'t', %al
+	out	%al, (%dx)
+	mov	$'a', %al
+	out	%al, (%dx)
+	mov	$'r', %al
+	out	%al, (%dx)
+	mov	$'t', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'g', %al
+	out	%al, (%dx)
+	mov	$'\n', %al
+	out	%al, (%dx)
+
+	push	%esp
+	// ebx, esi, edi and ebp are going to be preserved
+	// (see comment in smm_stub.S if target is x86_64)
+	push	%ebx
+	push	%esi
+	push	%edi
+	push	%ebp
+	push	%eax
+
+	/*
+	 * Switch-case to jump to appropriate section for given functionality.
+	 * Macros are defined in mm_handler.h.
+	 * Short explaination of where does the magic n(%esp) came from.
+	 * Calling mm_entry_32 from coreboot pushes lb_entry_context
+	 * (see include/payload_mm_interface.h) to stack and
+	 * increments esp by 4, so now our stack looks like this:
+	 * | third arg |
+	 * | second arg |
+	 * | first arg |
+	 * | return address |
+	 * | stack pointer | <- esp
+	 * Then we push all the registers (see above) and hence our stack looks like this now:
+	 * | third arg |
+	 * | second arg |
+	 * | first arg |
+	 * | return address |
+	 * | esp |
+	 * | ebx |
+	 * | esi |
+	 * | edi |
+	 * | ebp |
+	 * | eax |
+	 * | stack pointer | <- esp
+	 * So, now to get the entry we need, we do (9 * 4)(%esp) to get third
+	 * argument (ACPI base address), (8 * 4)(%esp) to get second argument
+	 * (PM1_CNT byte), and (7 * 4) to get the command.
+	 */
+
+	cmpl $MM_ACPI_ENABLE, 28(%esp)
+	je acpi_enable32
+
+	cmpl $MM_ACPI_DISABLE, 28(%esp)
+	je acpi_disable32
+
+	cmpl $MM_STORE, 28(%esp)
+	je mm_store32
+
+	jmp restore_cb_state
+
+acpi_enable32:
+	mov	$0x3f8, %dx
+	mov	$'E', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'a', %al
+	out	%al, (%dx)
+	mov	$'b', %al
+	out	%al, (%dx)
+	mov	$'l', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'g', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'A', %al
+	out	%al, (%dx)
+	mov	$'C', %al
+	out	%al, (%dx)
+	mov	$'P', %al
+	out	%al, (%dx)
+	mov	$'I', %al
+	out	%al, (%dx)
+	mov	$'\n', %al
+	out	%al, (%dx)
+
+	// PM1_CNT & ~SCI_EN
+	mov	32(%esp), %ax
+	add	$MM_ACPI_ENABLE, %ax
+
+	// ACPI_BASE_ADDR
+	mov	36(%esp), %dx
+
+	out	%ax, %dx
+
+	jmp	restore_cb_state
+
+acpi_disable32:
+	mov	$0x3f8, %dx
+	mov	$'D', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'s', %al
+	out	%al, (%dx)
+	mov	$'a', %al
+	out	%al, (%dx)
+	mov	$'b', %al
+	out	%al, (%dx)
+	mov	$'l', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'g', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'A', %al
+	out	%al, (%dx)
+	mov	$'C', %al
+	out	%al, (%dx)
+	mov	$'P', %al
+	out	%al, (%dx)
+	mov	$'I', %al
+	out	%al, (%dx)
+	mov	$'\n', %al
+	out	%al, (%dx)
+
+	// PM1_CNT | SCI_EN
+	mov	32(%esp), %ax
+	add	$MM_ACPI_DISABLE, %ax
+
+	// ACPI_BASE_ADDR
+	mov	36(%esp), %dx
+
+	out	%ax, %dx
+
+	jmp	restore_cb_state
+
+mm_store32:
+	// Not implemented yet. Probably would be better to do that in C.
+
+restore_cb_state:
+	mov	$0x3f8, %dx
+	mov	$'M', %al
+	out	%al, (%dx)
+	mov	$'M', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'r', %al
+	out	%al, (%dx)
+	mov	$'e', %al
+	out	%al, (%dx)
+	mov	$'t', %al
+	out	%al, (%dx)
+	mov	$'u', %al
+	out	%al, (%dx)
+	mov	$'r', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'s', %al
+	out	%al, (%dx)
+	mov	$'\n', %al
+	out	%al, (%dx)
+
+	pop	%eax
+	pop	%ebp
+	pop	%edi
+	pop	%esi
+	pop	%ebx
+	pop	%esp
+
+	ret
+SYM_CODE_END(mm_entry_32)
+
+
+	.section ".text64","ax"
+	.code64
+	.balign 4
+SYM_CODE_START(mm_entry_64)
+	mov	$0x3f8, %dx
+	mov	$'L', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'u', %al
+	out	%al, (%dx)
+	mov	$'x', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'S', %al
+	out	%al, (%dx)
+	mov	$'M', %al
+	out	%al, (%dx)
+	mov	$'I', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'h', %al
+	out	%al, (%dx)
+	mov	$'a', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'d', %al
+	out	%al, (%dx)
+	mov	$'l', %al
+	out	%al, (%dx)
+	mov	$'e', %al
+	out	%al, (%dx)
+	mov	$'r', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'s', %al
+	out	%al, (%dx)
+	mov	$'t', %al
+	out	%al, (%dx)
+	mov	$'a', %al
+	out	%al, (%dx)
+	mov	$'r', %al
+	out	%al, (%dx)
+	mov	$'t', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'g', %al
+	out	%al, (%dx)
+	mov	$'\n', %al
+	out	%al, (%dx)
+
+	pushq	%rsp
+
+	pushq	%rbp
+	pushq	%rbx
+	pushq	%r12
+	pushq	%r13
+	pushq	%r14
+	pushq	%r15
+
+	movq	%cr3, %rax
+	pushq	%rax
+
+	movq	%cr4, %rbx
+	pushq	%rbx
+	or	$0x640, %rbx
+
+	movq	%rbx, %cr4
+
+	movq	%cr0, %rbx
+	pushq	%rbx
+	or	$0x22, %rbx
+
+	mov	%rbx, %cr0
+
+	movq	%rsp, %r12
+	andq	$~0xF, %rsp
+
+	subq	$0x200, %rsp
+	fxsave64 (%rsp)
+
+	/*
+	 * All the macros we compare (r)di to are defined in mm_handler.h
+	 * This differs a bit from what we do above, as ABI calling convention
+	 * is not the same for protected and long mode. First two elements of
+	 * the struct fits rdi. Command is in the first byte of the rdi,
+	 * so we can just read of di.
+	 */
+	cmp $MM_ACPI_DISABLE, %di
+	je acpi_disable
+
+	cmp $MM_ACPI_ENABLE, %di
+	je acpi_enable
+
+	cmp $MM_STORE, %di
+	je mm_store
+
+	jmp restore_cb_state64
+
+acpi_enable:
+	mov	$0x3f8, %dx
+	mov	$'E', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'a', %al
+	out	%al, (%dx)
+	mov	$'b', %al
+	out	%al, (%dx)
+	mov	$'l', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'g', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'A', %al
+	out	%al, (%dx)
+	mov	$'C', %al
+	out	%al, (%dx)
+	mov	$'P', %al
+	out	%al, (%dx)
+	mov	$'I', %al
+	out	%al, (%dx)
+	mov	$'\n', %al
+	out	%al, (%dx)
+
+	// Stash the command from rdi.
+	shr	$32, %rdi
+	// PM1_CNT & ~SCI_EN
+	mov	%di, %ax
+	add	$MM_ACPI_ENABLE, %ax
+	// si contains ACPI_BASE_ADDR
+	mov	%si, %dx
+
+	out	%ax, %dx
+
+	jmp	restore_cb_state64
+
+acpi_disable:
+	mov	$0x3f8, %dx
+	mov	$'D', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'s', %al
+	out	%al, (%dx)
+	mov	$'a', %al
+	out	%al, (%dx)
+	mov	$'b', %al
+	out	%al, (%dx)
+	mov	$'l', %al
+	out	%al, (%dx)
+	mov	$'i', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'g', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'A', %al
+	out	%al, (%dx)
+	mov	$'C', %al
+	out	%al, (%dx)
+	mov	$'P', %al
+	out	%al, (%dx)
+	mov	$'I', %al
+	out	%al, (%dx)
+	mov	$'\n', %al
+	out	%al, (%dx)
+
+	// Stash command
+	shr	$32, %rdi
+	// PM1_CNT | SCI_EN
+	mov	%di, %ax
+	add	$MM_ACPI_DISABLE, %ax
+	// si contains ACPI_BASE_ADDR
+	mov	%si, %dx
+
+	out	%ax, %dx
+
+	jmp	restore_cb_state64
+
+mm_store:
+	// see comment above in mm_store32
+
+restore_cb_state64:
+	mov	$0x3f8, %dx
+	mov	$'M', %al
+	out	%al, (%dx)
+	mov	$'M', %al
+	out	%al, (%dx)
+	mov	$'6', %al
+	out	%al, (%dx)
+	mov	$'4', %al
+	out	%al, (%dx)
+	mov	$' ', %al
+	out	%al, (%dx)
+	mov	$'r', %al
+	out	%al, (%dx)
+	mov	$'e', %al
+	out	%al, (%dx)
+	mov	$'t', %al
+	out	%al, (%dx)
+	mov	$'u', %al
+	out	%al, (%dx)
+	mov	$'r', %al
+	out	%al, (%dx)
+	mov	$'n', %al
+	out	%al, (%dx)
+	mov	$'s', %al
+	out	%al, (%dx)
+	mov	$'\n', %al
+	out	%al, (%dx)
+
+	fxrstor64 (%rsp)
+	addq	$0x200, %rsp
+	movq	%r12, %rsp
+
+	popq	%rbx
+	movq	%rbx, %cr0
+
+	popq	%rbx
+	movq	%rbx, %cr4
+
+	popq	%rax
+	movq	%rax, %cr3
+
+	popq	%r15
+	popq	%r14
+	popq	%r13
+	popq	%r12
+	popq	%rbx
+	popq	%rbp
+	popq	%rsp
+	ret
+SYM_CODE_END(mm_entry_64)
+
+	.bss
+	.balign	4
+SYM_DATA(mm_signature,		.space 4)
+SYM_DATA(mm_blob_size,		.space 2)
+SYM_DATA(mm_entry_32_offset,	.space 4)
+SYM_DATA(mm_entry_64_offset,	.space 4)
diff --git a/drivers/firmware/google/mm_handler/mm_handler.h b/drivers/firmware/google/mm_handler/mm_handler.h
new file mode 100644
index 000000000000..4f32f84371d5
--- /dev/null
+++ b/drivers/firmware/google/mm_handler/mm_handler.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Derived from arch/x86/include/realmode.h
+ */
+
+#ifndef _MM_HANDLER_H
+#define _MM_HANDLER_H
+
+#define REALMODE_END_SIGNATURE	0x65a22c82
+
+/*
+ * These macros correspond to the arguments
+ * passed by coreboot's SMI handler. Depending
+ * on which one is passed in rdi or esp + x, handler
+ * will jump to the appropriate section.
+ */
+#define MM_ACPI_ENABLE		1
+#define MM_ACPI_DISABLE		0
+#define MM_STORE		2
+
+#endif /* _MM_HANDLER_H */
diff --git a/drivers/firmware/google/mm_handler/mm_header.S b/drivers/firmware/google/mm_handler/mm_header.S
new file mode 100644
index 000000000000..342cd60492f8
--- /dev/null
+++ b/drivers/firmware/google/mm_handler/mm_header.S
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * MM blob header; this should match mm_payload.h
+ */
+
+#include <linux/linkage.h>
+#include <asm/page_types.h>
+#include <asm/segment.h>
+
+	.section ".header", "a"
+
+	.balign	16
+SYM_DATA_START(mm_header)
+	.long	pa_text_start
+	.long	pa_mm_entry_32
+	.long	pa_mm_entry_64
+	.long	pa_mm_signature
+	.long	pa_mm_blob_size
+SYM_DATA_END(mm_header)
-- 
2.49.0

Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM related informations from coreboot tables

[Brian Norris]

Hi,

On Thu, Jun 12, 2025 at 04:05:48PM +0200, Michal Gorlas wrote:
> coreboot exposes (S)MM related data in the coreboot table. Extends existing interface,
> with structure corresponding to (S)MM data, and adds parser. Parser exposes this data
> to the modules executed later.

I don't think I have much opinion or knowledge about this feature, so
I'd probably defer to someone actually involved in the coreboot project
(Julius?) for some of that.

But a few cursory thoughts on the driver mechanics:

> Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
> ---
>  drivers/firmware/google/Kconfig          | 12 +++++
>  drivers/firmware/google/Makefile         |  3 ++
>  drivers/firmware/google/coreboot_table.h | 34 ++++++++-----
>  drivers/firmware/google/mm_info.c        | 63 ++++++++++++++++++++++++
>  drivers/firmware/google/mm_payload.h     | 58 ++++++++++++++++++++++
>  5 files changed, 158 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/firmware/google/mm_info.c
>  create mode 100644 drivers/firmware/google/mm_payload.h
> 
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index 41b78f5cb735..5d918c076f25 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -81,4 +81,16 @@ config GOOGLE_VPD
>  	  This option enables the kernel to expose the content of Google VPD
>  	  under /sys/firmware/vpd.
>  
> +config COREBOOT_PAYLOAD_MM
> +	tristate "SMI handling in Linux (LinuxBootSMM)"
> +	depends on GOOGLE_COREBOOT_TABLE

This all looks highly X86-specific, so you probably need 'depends on
X86'.

> +	help
> +	  Enables support for SMI handling by Linux-owned code.
> +	  coreboot reserves region for payload-owned SMI handler, the Linux
> +	  driver prepares its SMI handler outside of SMRAM, and lets coreboot
> +	  know where the handler is placed by issuing an SMI. On this SMI, the
> +	  handler is being placed in SMRAM and all supported SMIs from that point
> +	  on are handled by Linux-owned SMI handler.
> +	  If in doubt, say N.
> +
>  endif # GOOGLE_FIRMWARE

> diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
> index bb6f0f7299b4..e0b087933c5a 100644
> --- a/drivers/firmware/google/coreboot_table.h
> +++ b/drivers/firmware/google/coreboot_table.h

> @@ -112,8 +122,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
>   * boilerplate.  Each module may only use this macro once, and
>   * calling it replaces module_init() and module_exit()
>   */
> -#define module_coreboot_driver(__coreboot_driver) \
> +#define module_coreboot_driver(__coreboot_driver)                  \
>  	module_driver(__coreboot_driver, coreboot_driver_register, \
> -			coreboot_driver_unregister)
> +		      coreboot_driver_unregister)

You're making arbitrary whitespace changes in this hunk. Try to avoid
that, please.

>  
>  #endif /* __COREBOOT_TABLE_H */
> diff --git a/drivers/firmware/google/mm_info.c b/drivers/firmware/google/mm_info.c
> new file mode 100644
> index 000000000000..55bcdc8b8d53
> --- /dev/null
> +++ b/drivers/firmware/google/mm_info.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * mm_info.c
> + *
> + * Driver for exporting MM payload information from coreboot table.
> + *
> + * Copyright 2025 9elements gmbh
> + * Copyright 2025 Michal Gorlas <michal.gorlas@9elements.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include "coreboot_table.h"
> +#include "mm_payload.h"
> +
> +static struct lb_pld_mm_interface_info *mm_cbtable_info;
> +struct mm_info *mm_info;
> +
> +static int mm_driver_probe(struct coreboot_device *dev)
> +{
> +	mm_cbtable_info = &dev->mm_info;
> +	if (mm_cbtable_info->tag != LB_TAG_PLD_MM_INTERFACE_INFO)
> +		return -ENXIO;
> +
> +	mm_info = kmalloc(sizeof(*mm_info), GFP_KERNEL);

Error handling? (Needs a NULL check.)

And might as well use devm_*() (e.g., devm_kzalloc()); then you can drop
mm_driver_remove().

> +	mm_info->revision = mm_cbtable_info->revision;
> +	mm_info->requires_long_mode_call =
> +		mm_cbtable_info->requires_long_mode_call;
> +	mm_info->register_mm_entry_command =
> +		mm_cbtable_info->register_mm_entry_command;
> +	return 0;
> +}
> +EXPORT_SYMBOL(mm_info);

Why non-GPL? IIUC, EXPORT_SYMBOL_GPL() is the usual default.

Or, I suspect you don't actually need two separate modules, so you
probably don't need this EXPORT at all.

> +
> +static void mm_driver_remove(struct coreboot_device *dev)
> +{
> +	if (mm_info)
> +		kfree(mm_info);
> +}
> +
...


Brian

Re: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler

[Brian Norris]

On Thu, Jun 12, 2025 at 04:05:49PM +0200, Michal Gorlas wrote:
> Places a blob with Linux-owned SMI handler in the lower 4GB of memory, calculates
> entry points for the it and triggers SMI to coreboot's SMI handler
> informing it where to look for Linux-owned SMI handler.
> 
> Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
> ---
>  drivers/firmware/google/Makefile    |   9 ++
>  drivers/firmware/google/mm_blob.S   |  20 +++
>  drivers/firmware/google/mm_loader.c | 186 ++++++++++++++++++++++++++++
...
> --- /dev/null
> +++ b/drivers/firmware/google/mm_loader.c
> @@ -0,0 +1,186 @@
...
> +static int register_entry_point(struct mm_info *data, uint32_t entry_point)
> +{
> +	u64 cmd;
> +	u8 status;
> +
> +	cmd = data->register_mm_entry_command |
> +	      (PAYLOAD_MM_REGISTER_ENTRY << 8);
> +	status = trigger_smi(cmd, entry_point, 5);
> +	pr_info(DRIVER_NAME ": %s: SMI returned %x\n", __func__, status);

Don't print this kind of debug stuff at INFO level. If you need it, use
KERN_DEBUG.

Once this gets attached to a proper device/driver, you probably want
dev_dbg(), if anything.

> +
> +	return status;
> +}
> +


> +	/* At this point relocations are done and we can do some cool

	/*
	 * Multiline comment style is like this.
	 * i.e., start with "/*" on its own line.
	 * You got this right most of the time.
	 */

> +	 * pointer arithmetics to help coreboot determine correct entry
> +	 * point based on offsets.
> +	 */
> +	entry32_offset = mm_header->mm_entry_32 - (unsigned long)shared_buffer;
> +	entry64_offset = mm_header->mm_entry_64 - (unsigned long)shared_buffer;
> +
> +	mm_header->mm_entry_32 = entry32_offset;
> +	mm_header->mm_entry_64 = entry64_offset;
> +
> +	return (unsigned long)shared_buffer;
> +}
> +
> +static int __init mm_loader_init(void)
> +{
> +	u32 entry_point;
> +
> +	if (!mm_info)
> +		return -ENOMEM;

Hmm, so you have two modules, mm_info and mm_loader. mm_loader depends
on mm_info, but doesn't actually express that dependency. Can you just
merge mm_loader into mm_info or vice versa? Or at least, pass the
necessary data directly between the two, not as some implicit ordering
like this.

> +
> +	entry_point = place_handler();
> +
> +	if (register_entry_point(mm_info, entry_point)) {
> +		pr_warn(DRIVER_NAME ": registering entry point for MM payload failed.\n");
> +		kfree(mm_info);
> +		mm_info = NULL;
> +		free_pages((unsigned long)shared_buffer, get_order(blob_size));
> +		return -1;
> +	}
> +
> +	mdelay(100);

Why the delay? At least use a comment to tell us. And if it's really
needed, use msleep(), not mdelay(). scripts/checkpatch.pl should have
warned you. And, please use scripts/checkpatch.pl if you aren't already
;)

> +
> +	kfree(mm_info);
> +	mm_info = NULL;

This is odd and racy, having one module free data provided by another,
where that other module might also free it. Hopefully this gets
simplified if you manage to combine the modules, like I suggest.

> +	free_pages((unsigned long)shared_buffer, get_order(blob_size));
> +
> +	return 0;
> +}
> +
> +static void __exit mm_loader_exit(void)
> +{
> +	pr_info(DRIVER_NAME ": DONE\n");
> +}

Remove this function. We don't do prints like this.

Brian

> +
> +module_init(mm_loader_init);
> +module_exit(mm_loader_exit);
> +
> +MODULE_AUTHOR("Michal Gorlas <michal.gorlas@9elements.com>");
> +MODULE_DESCRIPTION("MM Payload loader - installs Linux-owned SMI handler");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.49.0
> 

Re: [PATCH v1 3/3] firmware: coreboot: Linux-owned SMI handler to be loaded by coreboot

[Brian Norris]

On Thu, Jun 12, 2025 at 04:05:50PM +0200, Michal Gorlas wrote:
> Compiled in similar fashion to the realmode trampolines for x86. Currently
> supported are two SMIs: ACPI enable and disable. After being placed in SMRAM,
> this handler takes over handling of the supported SMIs from coreboot.
> 
> Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
> ---
>  drivers/firmware/google/mm_handler/Makefile   |  51 ++
>  .../firmware/google/mm_handler/handler.lds.S  |  46 ++
>  .../firmware/google/mm_handler/mm_handler.S   | 510 ++++++++++++++++++
>  .../firmware/google/mm_handler/mm_handler.h   |  21 +
>  .../firmware/google/mm_handler/mm_header.S    |  19 +
>  5 files changed, 647 insertions(+)
>  create mode 100644 drivers/firmware/google/mm_handler/Makefile
>  create mode 100644 drivers/firmware/google/mm_handler/handler.lds.S
>  create mode 100644 drivers/firmware/google/mm_handler/mm_handler.S
>  create mode 100644 drivers/firmware/google/mm_handler/mm_handler.h
>  create mode 100644 drivers/firmware/google/mm_handler/mm_header.S

I'm not reviewing most of this patch right now (for one, I don't speak
x86), but for starters, I think you need to add a .gitignore file in
here somewhere. After building your code, I see these untracked files:

	drivers/firmware/google/mm_handler/handler.lds
	drivers/firmware/google/mm_handler/handler.relocs
	drivers/firmware/google/mm_handler/pasyms.h

Brian

Re: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler

[kernel test robot]

Hi Michal,

kernel test robot noticed the following build errors:

[auto build test ERROR on chrome-platform/for-next]
[also build test ERROR on chrome-platform/for-firmware-next linus/master v6.16-rc1 next-20250612]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Michal-Gorlas/firmware-coreboot-support-for-parsing-SMM-related-informations-from-coreboot-tables/20250612-221612
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link:    https://lore.kernel.org/r/6cfb5bae79c153c54da298c396adb8a28b5e785a.1749734094.git.michal.gorlas%409elements.com
patch subject: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250613/202506131541.RxswGh7u-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250613/202506131541.RxswGh7u-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506131541.RxswGh7u-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/google/mm_loader.c: In function 'place_handler':
>> drivers/firmware/google/mm_loader.c:105:9: error: implicit declaration of function 'wbinvd' [-Wimplicit-function-declaration]
     105 |         wbinvd();
         |         ^~~~~~


vim +/wbinvd +105 drivers/firmware/google/mm_loader.c

    84	
    85	static u32 __init place_handler(void)
    86	{
    87		/*
    88		 * The handler (aka MM blob) has to be placed in low 4GB of the memory.
    89		 * This is because we can not assume that coreboot will be in long mode
    90		 * while trying to copy the blob to SMRAM. Even if so, (can be checked by
    91		 * reading cb_data->mm_info.requires_long_mode_call), it would make our life
    92		 * way too complicated (e.g. no need for shared page table).
    93		 */
    94		size_t entry32_offset;
    95		size_t entry64_offset;
    96		u16 real_mode_seg;
    97		const u32 *rel;
    98		u32 count;
    99		unsigned long phys_base;
   100	
   101		blob_size = mm_payload_size_needed();
   102		shared_buffer = (void *)__get_free_pages(GFP_DMA32, get_order(blob_size));
   103	
   104		memcpy(shared_buffer, mm_blob, blob_size);
 > 105		wbinvd();
   106	
   107		/*
   108		 * Based on arch/x86/realmode/init.c
   109		 * The sole purpose of doing relocations is to be able to calculate the offsets
   110		 * for entry points. While the absolute addresses are not valid anymore after the
   111		 * blob is copied to SMRAM, the distances between sections stay the same, so we
   112		 * can still calculate the correct entry point based on coreboot's bitness.
   113		 */
   114		phys_base = __pa(shared_buffer);
   115		real_mode_seg = phys_base >> 4;
   116		rel = (u32 *)mm_relocs;
   117	
   118		/* 16-bit segment relocations. */
   119		count = *rel++;
   120		while (count--) {
   121			u16 *seg = (u16 *)(shared_buffer + *rel++);
   122			*seg = real_mode_seg;
   123		}
   124	
   125		/* 32-bit linear relocations. */
   126		count = *rel++;
   127		while (count--) {
   128			u32 *ptr = (u32 *)(shared_buffer + *rel++);
   129			*ptr += phys_base;
   130		}
   131	
   132		mm_header =  (struct mm_header *)shared_buffer;
   133	
   134		mm_header->mm_signature = REALMODE_END_SIGNATURE;
   135		mm_header->mm_blob_size = mm_payload_size_needed();
   136	
   137		/* At this point relocations are done and we can do some cool
   138		 * pointer arithmetics to help coreboot determine correct entry
   139		 * point based on offsets.
   140		 */
   141		entry32_offset = mm_header->mm_entry_32 - (unsigned long)shared_buffer;
   142		entry64_offset = mm_header->mm_entry_64 - (unsigned long)shared_buffer;
   143	
   144		mm_header->mm_entry_32 = entry32_offset;
   145		mm_header->mm_entry_64 = entry64_offset;
   146	
   147		return (unsigned long)shared_buffer;
   148	}
   149	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 3/3] firmware: coreboot: Linux-owned SMI handler to be loaded by coreboot

[kernel test robot]

Hi Michal,

kernel test robot noticed the following build errors:

[auto build test ERROR on chrome-platform/for-next]
[also build test ERROR on chrome-platform/for-firmware-next linus/master v6.16-rc1 next-20250613]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Michal-Gorlas/firmware-coreboot-support-for-parsing-SMM-related-informations-from-coreboot-tables/20250612-221612
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link:    https://lore.kernel.org/r/410d4d62b031d0e751e1933cf746540d5cb1682c.1749734094.git.michal.gorlas%409elements.com
patch subject: [PATCH v1 3/3] firmware: coreboot: Linux-owned SMI handler to be loaded by coreboot
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250613/202506132239.FTTwSHeX-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250613/202506132239.FTTwSHeX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506132239.FTTwSHeX-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/firmware/google/mm_handler/mm_header.S:7:10: fatal error: asm/page_types.h: No such file or directory
       7 | #include <asm/page_types.h>
         |          ^~~~~~~~~~~~~~~~~~
   compilation terminated.
--
>> drivers/firmware/google/mm_handler/mm_handler.S:11:10: fatal error: asm/pgtable_types.h: No such file or directory
      11 | #include <asm/pgtable_types.h>
         |          ^~~~~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +7 drivers/firmware/google/mm_handler/mm_header.S

   > 7	#include <asm/page_types.h>

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM related informations from coreboot tables

[Michal Gorlas]

Hi,
Thanks for taking your time to go over the patches.

On Thu, Jun 12, 2025 at 03:37:40PM -0700, Brian Norris wrote:
> 
> This all looks highly X86-specific, so you probably need 'depends on
> X86'.
> 

Indeed, this was also why CI build was failed.

> > @@ -112,8 +122,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
> >   * boilerplate.  Each module may only use this macro once, and
> >   * calling it replaces module_init() and module_exit()
> >   */
> > -#define module_coreboot_driver(__coreboot_driver) \
> > +#define module_coreboot_driver(__coreboot_driver)                  \
> >  	module_driver(__coreboot_driver, coreboot_driver_register, \
> > -			coreboot_driver_unregister)
> > +		      coreboot_driver_unregister)
> 
> You're making arbitrary whitespace changes in this hunk. Try to avoid
> that, please.
> 

Sure, will do. It came from a style warning when running
scripts/checkpatch.pl. I thought it could be useful to fix it on the
same go.

> >  
> >  #endif /* __COREBOOT_TABLE_H */
> > diff --git a/drivers/firmware/google/mm_info.c b/drivers/firmware/google/mm_info.c
> > new file mode 100644
> > index 000000000000..55bcdc8b8d53
> > --- /dev/null
> > +++ b/drivers/firmware/google/mm_info.c
> > @@ -0,0 +1,63 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * mm_info.c
> > + *
> > + * Driver for exporting MM payload information from coreboot table.
> > + *
> > + * Copyright 2025 9elements gmbh
> > + * Copyright 2025 Michal Gorlas <michal.gorlas@9elements.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +
> > +#include "coreboot_table.h"
> > +#include "mm_payload.h"
> > +
> > +static struct lb_pld_mm_interface_info *mm_cbtable_info;
> > +struct mm_info *mm_info;
> > +
> > +static int mm_driver_probe(struct coreboot_device *dev)
> > +{
> > +	mm_cbtable_info = &dev->mm_info;
> > +	if (mm_cbtable_info->tag != LB_TAG_PLD_MM_INTERFACE_INFO)
> > +		return -ENXIO;
> > +
> > +	mm_info = kmalloc(sizeof(*mm_info), GFP_KERNEL);
> 
> Error handling? (Needs a NULL check.)
> 
> And might as well use devm_*() (e.g., devm_kzalloc()); then you can drop
> mm_driver_remove().
> 
> > +	mm_info->revision = mm_cbtable_info->revision;
> > +	mm_info->requires_long_mode_call =
> > +		mm_cbtable_info->requires_long_mode_call;
> > +	mm_info->register_mm_entry_command =
> > +		mm_cbtable_info->register_mm_entry_command;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mm_info);
> 
> Why non-GPL? IIUC, EXPORT_SYMBOL_GPL() is the usual default.
> 
> Or, I suspect you don't actually need two separate modules, so you
> probably don't need this EXPORT at all.
> 

Yep these two definetely can be merged into one module. Will do that in
v2 series.

Best,
Michal

Re: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler

[Michal Gorlas]

On Thu, Jun 12, 2025 at 03:38:21PM -0700, Brian Norris wrote:
> > +static int register_entry_point(struct mm_info *data, uint32_t entry_point)
> > +{
> > +	u64 cmd;
> > +	u8 status;
> > +
> > +	cmd = data->register_mm_entry_command |
> > +	      (PAYLOAD_MM_REGISTER_ENTRY << 8);
> > +	status = trigger_smi(cmd, entry_point, 5);
> > +	pr_info(DRIVER_NAME ": %s: SMI returned %x\n", __func__, status);
> 
> Don't print this kind of debug stuff at INFO level. If you need it, use
> KERN_DEBUG.
> 
> Once this gets attached to a proper device/driver, you probably want
> dev_dbg(), if anything.
> 
...
> 
> 
> > +	/* At this point relocations are done and we can do some cool
> 
> 	/*
> 	 * Multiline comment style is like this.
> 	 * i.e., start with "/*" on its own line.
> 	 * You got this right most of the time.
> 	 */
> 

Got it.

> > +	 * pointer arithmetics to help coreboot determine correct entry
> > +	 * point based on offsets.
> > +	 */
> > +	entry32_offset = mm_header->mm_entry_32 - (unsigned long)shared_buffer;
> > +	entry64_offset = mm_header->mm_entry_64 - (unsigned long)shared_buffer;
> > +
> > +	mm_header->mm_entry_32 = entry32_offset;
> > +	mm_header->mm_entry_64 = entry64_offset;
> > +
> > +	return (unsigned long)shared_buffer;
> > +}
> > +
> > +static int __init mm_loader_init(void)
> > +{
> > +	u32 entry_point;
> > +
> > +	if (!mm_info)
> > +		return -ENOMEM;
> 
> Hmm, so you have two modules, mm_info and mm_loader. mm_loader depends
> on mm_info, but doesn't actually express that dependency. Can you just
> merge mm_loader into mm_info or vice versa? Or at least, pass the
> necessary data directly between the two, not as some implicit ordering
> like this.
> 

Yep, will do that. As long as there is only one cbtable entry (and I
think this will stay like this), mm_info can be part of mm_loader.

> > +
> > +	entry_point = place_handler();
> > +
> > +	if (register_entry_point(mm_info, entry_point)) {
> > +		pr_warn(DRIVER_NAME ": registering entry point for MM payload failed.\n");
> > +		kfree(mm_info);
> > +		mm_info = NULL;
> > +		free_pages((unsigned long)shared_buffer, get_order(blob_size));
> > +		return -1;
> > +	}
> > +
> > +	mdelay(100);
> 
> Why the delay? At least use a comment to tell us. And if it's really
> needed, use msleep(), not mdelay(). scripts/checkpatch.pl should have
> warned you. And, please use scripts/checkpatch.pl if you aren't already
> ;)
> 

Long story short, SMIs on real hardware like to take longer from time to
time, and the delay was a "safeguard". It is probably not the proper way
to handle it, but locking here was not helpful at all, lock was released
regardless of CPU being still in SMM context (I assume due to SMIs being
invisible to whatever runs in ring-0). Have to admit though, that 100ms
is a consequence of trial and error. I would actually use some on advice
how to handle this properly. scripts/checkpatch.pl was not complaining
about it. It only gave me:

WARNING: quoted string split across lines
#57: FILE: drivers/firmware/google/mm_loader.c:57:
+		     ".return_not_changed:"
+		     "movq	%%rcx, %[status]\n\t"

total: 0 errors, 1 warnings, 0 checks, 186 lines checked

> > +
> > +	kfree(mm_info);
> > +	mm_info = NULL;
> 
> This is odd and racy, having one module free data provided by another,
> where that other module might also free it. Hopefully this gets
> simplified if you manage to combine the modules, like I suggest.
> 

Yep, got it.

Best,
Michal

Re: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler

[Brian Norris]

On Sat, Jun 14, 2025 at 02:59:33PM +0200, Michal Gorlas wrote:
> On Thu, Jun 12, 2025 at 03:38:21PM -0700, Brian Norris wrote:
> > > +	mdelay(100);
> > 
> > Why the delay? At least use a comment to tell us. And if it's really
> > needed, use msleep(), not mdelay(). scripts/checkpatch.pl should have
> > warned you. And, please use scripts/checkpatch.pl if you aren't already
> > ;)
> > 
> 
> Long story short, SMIs on real hardware like to take longer from time to
> time, and the delay was a "safeguard". It is probably not the proper way
> to handle it, but locking here was not helpful at all, lock was released
> regardless of CPU being still in SMM context (I assume due to SMIs being
> invisible to whatever runs in ring-0). Have to admit though, that 100ms
> is a consequence of trial and error. I would actually use some on advice
> how to handle this properly.

Sorry, I don't have any advice here at the moment.

> scripts/checkpatch.pl was not complaining
> about it. It only gave me:
> 
> WARNING: quoted string split across lines
> #57: FILE: drivers/firmware/google/mm_loader.c:57:
> +		     ".return_not_changed:"
> +		     "movq	%%rcx, %[status]\n\t"
> 
> total: 0 errors, 1 warnings, 0 checks, 186 lines checked

I must have either misread or misremembered checkpatch's behavior.
Possibly both. It has various other delay-realted warnings that point
you at the kerneldoc comments for mdelay() and msleep() though, and the
mdelay() comments say:

 * Please double check, whether mdelay() is the right way to go or whether a
 * refactoring of the code is the better variant to be able to use msleep()
 * instead.

Brian

Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM related informations from coreboot tables

[Brian Norris]

On Sat, Jun 14, 2025 at 02:53:18PM +0200, Michal Gorlas wrote:
> On Thu, Jun 12, 2025 at 03:37:40PM -0700, Brian Norris wrote:
> > > @@ -112,8 +122,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
> > >   * boilerplate.  Each module may only use this macro once, and
> > >   * calling it replaces module_init() and module_exit()
> > >   */
> > > -#define module_coreboot_driver(__coreboot_driver) \
> > > +#define module_coreboot_driver(__coreboot_driver)                  \
> > >  	module_driver(__coreboot_driver, coreboot_driver_register, \
> > > -			coreboot_driver_unregister)
> > > +		      coreboot_driver_unregister)
> > 
> > You're making arbitrary whitespace changes in this hunk. Try to avoid
> > that, please.
> > 
> 
> Sure, will do. It came from a style warning when running
> scripts/checkpatch.pl. I thought it could be useful to fix it on the
> same go.

That's odd, I don't see any such warning. Anyway, typically I'd expect
such things not to be lumped together under the "separate your changes"
guidance of Documentation/process/submitting-patches.rst (if they're
worth changing at all), although that may not be a hard and fast rule.

Brian

Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM related informations from coreboot tables

[Michal Gorlas]

On Mon, Jun 16, 2025 at 11:16:18AM -0700, Brian Norris wrote:
> On Sat, Jun 14, 2025 at 02:53:18PM +0200, Michal Gorlas wrote:
> > On Thu, Jun 12, 2025 at 03:37:40PM -0700, Brian Norris wrote:
> > > > @@ -112,8 +122,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
> > > >   * boilerplate.  Each module may only use this macro once, and
> > > >   * calling it replaces module_init() and module_exit()
> > > >   */
> > > > -#define module_coreboot_driver(__coreboot_driver) \
> > > > +#define module_coreboot_driver(__coreboot_driver)                  \
> > > >  	module_driver(__coreboot_driver, coreboot_driver_register, \
> > > > -			coreboot_driver_unregister)
> > > > +		      coreboot_driver_unregister)
> > > 
> > > You're making arbitrary whitespace changes in this hunk. Try to avoid
> > > that, please.
> > > 
> > 
> > Sure, will do. It came from a style warning when running
> > scripts/checkpatch.pl. I thought it could be useful to fix it on the
> > same go.
> 
> That's odd, I don't see any such warning. Anyway, typically I'd expect
> such things not to be lumped together under the "separate your changes"
> guidance of Documentation/process/submitting-patches.rst (if they're
> worth changing at all), although that may not be a hard and fast rule.

My bad, the whitespace were not a consequence of checkpatch.pl
complaints, but rather running clang-format on coreboot_table.h
on a clean tree:

 * $ clang-format -i drivers/firmware/google/coreboot_table.h
 * $ git diff
 * diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
 * index bb6f0f7299b4..3d933c657535 100644
 * --- a/drivers/firmware/google/coreboot_table.h
 * +++ b/drivers/firmware/google/coreboot_table.h
 * @@ -61,15 +61,15 @@ struct lb_framebuffer {
 *         u32 x_resolution;
 *         u32 y_resolution;
 *         u32 bytes_per_line;
 * -       u8  bits_per_pixel;
 * -       u8  red_mask_pos;
 * -       u8  red_mask_size;
 * -       u8  green_mask_pos;
 * -       u8  green_mask_size;
 * -       u8  blue_mask_pos;
 * -       u8  blue_mask_size;
 * -       u8  reserved_mask_pos;
 * -       u8  reserved_mask_size;
 * +       u8 bits_per_pixel;
 * +       u8 red_mask_pos;
 * +       u8 red_mask_size;
 * +       u8 green_mask_pos;
 * +       u8 green_mask_size;
 * +       u8 blue_mask_pos;
 * +       u8 blue_mask_size;
 * +       u8 reserved_mask_pos;
 * +       u8 reserved_mask_size;
 *  };
 * 
 *  /* A device, additionally with information from coreboot. */
 * @@ -112,8 +112,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
 *   * boilerplate.  Each module may only use this macro once, and
 *   * calling it replaces module_init() and module_exit()
 *   */
 * -#define module_coreboot_driver(__coreboot_driver) \
 * +#define module_coreboot_driver(__coreboot_driver)                  \
 *         module_driver(__coreboot_driver, coreboot_driver_register, \
 * -                       coreboot_driver_unregister)
 * +                     coreboot_driver_unregister)
 * 
 *  #endif /* __COREBOOT_TABLE_H */

Anyway, I am not sure if it is even worth changing, its rather cosmetic. I
removed these changes in v2. 

Best,
Michal

Re: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler

[Michal Gorlas]

On Mon, Jun 16, 2025 at 11:07:57AM -0700, Brian Norris wrote:
> I must have either misread or misremembered checkpatch's behavior.
> Possibly both. It has various other delay-realted warnings that point
> you at the kerneldoc comments for mdelay() and msleep() though, and the
> mdelay() comments say:
> 
>  * Please double check, whether mdelay() is the right way to go or whether a
>  * refactoring of the code is the better variant to be able to use msleep()
>  * instead.

Okay, will check if msleep() has the same effects in case SMI takes too
long.

Best,
Michal