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

[PATCH v2 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)

Changes in v2:
- merged mm_info into mm_loader
- removed mm_loader_exit in favour of allocating memory with devm_*()
- added forgotten "\n\t" in trigger_smi()
- added gitignore in mm_handler/
- added COREBOOT_PAYLOAD_MM dependency on x86
- error handling for mm_info struct
- error handling for shared_buffer
- removed whitespace changes in coreboot_table.h
- removed INFO level prints 

---
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                 |  11 +
 drivers/firmware/google/coreboot_table.h         |  11 +
 drivers/firmware/google/mm_blob.S                |  20 +
 drivers/firmware/google/mm_handler/.gitignore    |   4 +
 drivers/firmware/google/mm_handler/Makefile      |  51 +++
 drivers/firmware/google/mm_handler/handler.lds.S |  46 ++
 drivers/firmware/google/mm_handler/mm_handler.S  | 510 +++++++++++++++++++++++
 drivers/firmware/google/mm_handler/mm_handler.h  |  21 +
 drivers/firmware/google/mm_handler/mm_header.S   |  19 +
 drivers/firmware/google/mm_loader.c              | 215 ++++++++++
 drivers/firmware/google/mm_payload.h             |  58 +++
 12 files changed, 978 insertions(+)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250613-coreboot-payload-mm-6642bb42284a

Best regards,
-- 
Michal Gorlas <michal.gorlas@9elements.com>

[PATCH v2 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 COREBOOT_PAYLOAD_MM config used by
follow-up patches.

Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
---
 drivers/firmware/google/Kconfig          | 12 +++++++
 drivers/firmware/google/coreboot_table.h | 11 ++++++
 drivers/firmware/google/mm_payload.h     | 58 ++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 41b78f5cb7351e512cbaeb6443634a4b97bf6255..ff443f0c4a9a990052d83903fb0f0b0795cc76bd 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 X86 && 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/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index bb6f0f7299b4670d0b1f91bd7a3c038cdb412f7b..8a7934e679903e39ce1d86b4aecbc2b4e89363fd 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -52,6 +52,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;
@@ -80,6 +90,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);
 	};
 };
diff --git a/drivers/firmware/google/mm_payload.h b/drivers/firmware/google/mm_payload.h
new file mode 100644
index 0000000000000000000000000000000000000000..bb2f55c4f24096dfd526e281a937f100031d5315
--- /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 v2 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    |  11 ++
 drivers/firmware/google/mm_blob.S   |  20 ++++
 drivers/firmware/google/mm_loader.c | 215 ++++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)

diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index 8151e323cc434673b0fe56f1df1cec2fb7873295..bcac8dd1509ed251e3739b5905d6267ac097379d 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -12,3 +12,14 @@ obj-$(CONFIG_GOOGLE_CBMEM)		+= cbmem.o
 
 vpd-sysfs-y := vpd.o vpd_decode.o
 obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
+
+# LinuxBootSMM related.
+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 0000000000000000000000000000000000000000..d45ff661d74ab85fbb283704df7ceeee475dcbc5
--- /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 ".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 0000000000000000000000000000000000000000..2e823b5d0677cf9a8a27f11c7e3f196cddc8a664
--- /dev/null
+++ b/drivers/firmware/google/mm_loader.c
@@ -0,0 +1,215 @@
+// 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 <linux/slab.h>
+#include <linux/device.h>
+
+#include "coreboot_table.h"
+#include "mm_payload.h"
+
+struct mm_header *mm_header;
+static void *shared_buffer;
+static size_t blob_size;
+static struct lb_pld_mm_interface_info *mm_cbtable_info;
+struct mm_info *mm_info;
+
+/*
+ * 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:\n\t"
+		     "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 device *dev, 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);
+	dev_dbg(dev, ": %s: SMI returned %x\n", __func__, status);
+
+	return status;
+}
+
+static u32 place_handler(struct device *dev)
+{
+	/*
+	 * 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 *)devm_get_free_pages(dev, GFP_DMA32, get_order(blob_size));
+	if (!shared_buffer)
+		return -ENOMEM;
+
+	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 get_mm_info(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 = devm_kzalloc(&dev->dev, sizeof(*mm_info), GFP_KERNEL);
+	if (!mm_info)
+		return -ENOMEM;
+
+	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;
+}
+
+static int mm_loader_probe(struct coreboot_device *dev)
+{
+	if (get_mm_info(dev))
+		return -ENOMEM;
+
+	u32 entry_point;
+
+	entry_point = place_handler(&dev->dev);
+
+	if (register_entry_point(&dev->dev, mm_info, entry_point)) {
+		dev_warn(&dev->dev, ": registering entry point for MM payload failed.\n");
+		return -1;
+	}
+
+	/*
+	 * Gives SMI some time in case it takes longer than expected.
+	 * Only useful on real hardware (tested on RaptorLake), not needed on emulation.
+	 */
+	mdelay(100);
+
+	return 0;
+}
+
+static const struct coreboot_device_id mm_loader_ids[] = {
+	{ .tag = LB_TAG_PLD_MM_INTERFACE_INFO },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(coreboot, mm_loader_ids);
+
+static struct coreboot_driver mm_loader = {
+	.probe = mm_loader_probe,
+	.drv = {
+		.name = "mm_loader",
+	},
+	.id_table = mm_loader_ids,
+};
+
+module_coreboot_driver(mm_loader);
+
+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 v2 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/.gitignore    |   4 +
 drivers/firmware/google/mm_handler/Makefile      |  51 +++
 drivers/firmware/google/mm_handler/handler.lds.S |  46 ++
 drivers/firmware/google/mm_handler/mm_handler.S  | 510 +++++++++++++++++++++++
 drivers/firmware/google/mm_handler/mm_handler.h  |  21 +
 drivers/firmware/google/mm_handler/mm_header.S   |  19 +
 6 files changed, 651 insertions(+)

diff --git a/drivers/firmware/google/mm_handler/.gitignore b/drivers/firmware/google/mm_handler/.gitignore
new file mode 100644
index 0000000000000000000000000000000000000000..662f71dca9ce9dceeaf526746a6f323044296439
--- /dev/null
+++ b/drivers/firmware/google/mm_handler/.gitignore
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0-only
+pasyms.h
+handler.lds
+handler.relocs
diff --git a/drivers/firmware/google/mm_handler/Makefile b/drivers/firmware/google/mm_handler/Makefile
new file mode 100644
index 0000000000000000000000000000000000000000..c0069e88f51dc536878429c8e1504e9376932c04
--- /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 0000000000000000000000000000000000000000..c92c9f2fbd624d0d988be0bd0215f70c3e4284e0
--- /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 0000000000000000000000000000000000000000..19322010a4231cbd7d9c0aac5605a56eac098789
--- /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 0000000000000000000000000000000000000000..4f32f84371d5cff3fcf6c9d1c1c33a2d2b50118e
--- /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 0000000000000000000000000000000000000000..342cd60492f89f8bea8d7051650365d1eaad4c2e
--- /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 v2 0/3] firmware: coreboot: Support for System Management Interrupt (SMI) handling in coreboot payload (MM payload concept)

[Tzung-Bi Shih]

On Mon, Jun 16, 2025 at 04:01:11PM +0200, Michal Gorlas wrote:
> Hi,
> 
> This patchset adds support for MM payload when Linux is used as

Please provide some annotation when you first time use the acronyms for
readability. From the context, here MM stands for Management Mode?

> 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.

I don't think I could provide any useful comments to the feature. Only
feedbacked some general comments for the driver parts. 

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

[Tzung-Bi Shih]

On Mon, Jun 16, 2025 at 04:01:12PM +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 COREBOOT_PAYLOAD_MM config used by
> follow-up patches.

s/Extends/Extend/;s/adds/add/ not a hard requirement but [1]("imperative mood").

It'd be better to annotate what MM and SMM stand for as well.

Symbols introduced in the patch are used in the following patch of the series.
How about just merge them into one patch?

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html

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

[Tzung-Bi Shih]

On Mon, Jun 16, 2025 at 04:01:13PM +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
                   ^^^
s/the//?
s/Places/Place/;s/calculates/calculate/;s/triggers/trigger/.

> informing it where to look for Linux-owned SMI handler.

How about repharse the message to something like:

    Load Linux-owned SMI handler:
    - Place Linux-owned SMI handler in ...
    - Inform coreboot the location of Linux-owned SMI handler via SMI ...

    On success, the Linux-owned SMI handler takes over all upcoming SMIs.

> diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
> [...]
> +
> +# LinuxBootSMM related.
> +payload-mm-$(CONFIG_COREBOOT_PAYLOAD_MM)	:= mm_loader.o mm_blob.o
> +
> +subdir-						:= mm_handler

subdir-$(CONFIG_COREBOOT_PAYLOAD_MM)?

> +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 $@

mm_handler/ isn't visible to this patch. Separate them into the following
patch of series?

> diff --git a/drivers/firmware/google/mm_blob.S b/drivers/firmware/google/mm_blob.S
> [...]
> +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)

mm_handler/ isn't visible to this patch. Separate them into the following
patch of series?

> diff --git a/drivers/firmware/google/mm_loader.c b/drivers/firmware/google/mm_loader.c
> [...]
> +#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 <linux/slab.h>
> +#include <linux/device.h>

Please review again if it really needs to include the headers. Does it need
to include cpu.h, mm.h, and slab.h?

Also sort them alphabetically.

> +struct mm_header *mm_header;
> +static void *shared_buffer;
> +static size_t blob_size;
> +static struct lb_pld_mm_interface_info *mm_cbtable_info;
> +struct mm_info *mm_info;

No. Please allocate a driver specific struct and access it via
dev_set_drvdata() and dev_get_drvdata() if the context needs to be kept.

> +static int trigger_smi(u64 cmd, u64 arg, u64 retry)
> +{
> +	u64 status;
> [...]
> +
> +	if (status == cmd || status == PAYLOAD_MM_RET_FAILURE)
> +		status = PAYLOAD_MM_RET_FAILURE;
> +	else
> +		status = PAYLOAD_MM_RET_SUCCESS;

No. Please use -errno in the kernel.

> +static int get_mm_info(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 = devm_kzalloc(&dev->dev, sizeof(*mm_info), GFP_KERNEL);
> +	if (!mm_info)
> +		return -ENOMEM;
> +
> +	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;

Does it really need to copy the data from `&dev->mm_info`?

> +static int mm_loader_probe(struct coreboot_device *dev)
> +{
> +	if (get_mm_info(dev))
> +		return -ENOMEM;

get_mm_info() isn't necessarily to be -ENOMEM. How about:

    ret = get_mm_info(...);
    if (ret)
        return ret;

> +
> +	u32 entry_point;
> +
> +	entry_point = place_handler(&dev->dev);
> +
> +	if (register_entry_point(&dev->dev, mm_info, entry_point)) {
> +		dev_warn(&dev->dev, ": registering entry point for MM payload failed.\n");
> +		return -1;

Please use -errno in the kernel. -ENOENT or -ENOTSUPP?

> +	}
> +
> +	/*
> +	 * Gives SMI some time in case it takes longer than expected.
> +	 * Only useful on real hardware (tested on RaptorLake), not needed on emulation.
> +	 */
> +	mdelay(100);

This looks weird. Are there some ways for Linux to be aware of the SMI has
completed?

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

[Michal Gorlas]

On Wed, Jun 25, 2025 at 05:58:54AM +0000, Tzung-Bi Shih wrote:
> How about repharse the message to something like:
> 
>     Load Linux-owned SMI handler:
>     - Place Linux-owned SMI handler in ...
>     - Inform coreboot the location of Linux-owned SMI handler via SMI ...
> 
>     On success, the Linux-owned SMI handler takes over all upcoming SMIs.
> 

Yep, will do.

> > diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
> > [...]
> > +
> > +# LinuxBootSMM related.
> > +payload-mm-$(CONFIG_COREBOOT_PAYLOAD_MM)	:= mm_loader.o mm_blob.o
> > +
> > +subdir-						:= mm_handler
> 
> subdir-$(CONFIG_COREBOOT_PAYLOAD_MM)?
> 
Right.

> > +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 $@
> 
> mm_handler/ isn't visible to this patch. Separate them into the following
> patch of series?
> 
> > diff --git a/drivers/firmware/google/mm_blob.S b/drivers/firmware/google/mm_blob.S
> > [...]
> > +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)
> 
> mm_handler/ isn't visible to this patch. Separate them into the following
> patch of series?
> 

Would it make sense then to merge patch 2/3 and 3/3 into one? mm_loader
depends on mm_blob, and mm_blob depends on mm_handler/ being visible.
I wanted to split these initially as the 3rd patch is already terrible
to read because of all the assembly code in mm_handler/. But if it makes
sense to have them as one patch, I'll do that.

> > diff --git a/drivers/firmware/google/mm_loader.c b/drivers/firmware/google/mm_loader.c
> > [...]
> > +#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 <linux/slab.h>
> > +#include <linux/device.h>
> 
> Please review again if it really needs to include the headers. Does it need
> to include cpu.h, mm.h, and slab.h?
> 
> Also sort them alphabetically.
> 

Right, I forgot to clean these up, my bad.

> > +struct mm_header *mm_header;
> > +static void *shared_buffer;
> > +static size_t blob_size;
> > +static struct lb_pld_mm_interface_info *mm_cbtable_info;
> > +struct mm_info *mm_info;
> 
> No. Please allocate a driver specific struct and access it via
> dev_set_drvdata() and dev_get_drvdata() if the context needs to be kept.
> 

Yep will do.

> > +static int trigger_smi(u64 cmd, u64 arg, u64 retry)
> > +{
> > +	u64 status;
> > [...]
> > +
> > +	if (status == cmd || status == PAYLOAD_MM_RET_FAILURE)
> > +		status = PAYLOAD_MM_RET_FAILURE;
> > +	else
> > +		status = PAYLOAD_MM_RET_SUCCESS;
> 
> No. Please use -errno in the kernel.
> 

In which line here exactly? In the conditional statement I explicitly
check for RAX (and hence status) being 1. Not sure if status == EPERM
would make any sense here. I guess you meant specifically 
status = PAYLOAD_MM_RET_FAILURE? Then what would be appropriate -errno?
I think it could be -EREMOTEIO or -EIO, since the APMC SMI which
trigger_smi does is an I/O write. But I am not sure if that's the
appropriate errno.

> > +static int get_mm_info(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 = devm_kzalloc(&dev->dev, sizeof(*mm_info), GFP_KERNEL);
> > +	if (!mm_info)
> > +		return -ENOMEM;
> > +
> > +	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;
> 
> Does it really need to copy the data from `&dev->mm_info`?
> 

Not necessarily, the concept of copying the data made sense with v1
patches where the "parser" was in separate module, and was exporting
mm_info to mm_loader. I think it would be sufficient here to get rid of
get_mm_info and just let mm_loader_probe check for the tag:

	mm_cbtable_info = &dev->mm_info;
	if (mm_cbtable_info->tag != LB_TAG_PLD_MM_INTERFACE_INFO)
		return -ENXIO;	

> > +
> > +	u32 entry_point;
> > +
> > +	entry_point = place_handler(&dev->dev);
> > +
> > +	if (register_entry_point(&dev->dev, mm_info, entry_point)) {
> > +		dev_warn(&dev->dev, ": registering entry point for MM payload failed.\n");
> > +		return -1;
> 
> Please use -errno in the kernel. -ENOENT or -ENOTSUPP?
> 

Yep. -ENOTSUPP fits here.

> > +	}
> > +
> > +	/*
> > +	 * Gives SMI some time in case it takes longer than expected.
> > +	 * Only useful on real hardware (tested on RaptorLake), not needed on emulation.
> > +	 */
> > +	mdelay(100);
> 
> This looks weird. Are there some ways for Linux to be aware of the SMI has
> completed?

Not in a straight forward fashion. On Intel SoCs we could read MSR_SMI_COUNT
[1] before and after sending an SMI, and wait till it increments. I am
not aware about any unified way that works for AMD SoCs. However, so far
none of the AMD boards supported by coreboot was tested with MM payload,
so to make it Intel-only in v3 is not a bad idea.

[1]: https://elixir.bootlin.com/linux/v6.16-rc3/source/arch/x86/include/asm/msr-index.h#L880

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

[Michal Gorlas]

On Wed, Jun 25, 2025 at 02:26:11PM +0200, Michal Gorlas wrote:
> > > +
> > > +	/*
> > > +	 * Gives SMI some time in case it takes longer than expected.
> > > +	 * Only useful on real hardware (tested on RaptorLake), not needed on emulation.
> > > +	 */
> > > +	mdelay(100);
> > 
> > This looks weird. Are there some ways for Linux to be aware of the SMI has
> > completed?
> 
> Not in a straight forward fashion. On Intel SoCs we could read MSR_SMI_COUNT
> [1] before and after sending an SMI, and wait till it increments. I am
> not aware about any unified way that works for AMD SoCs. However, so far
> none of the AMD boards supported by coreboot was tested with MM payload,
> so to make it Intel-only in v3 is not a bad idea.
> 
> [1]: https://elixir.bootlin.com/linux/v6.16-rc3/source/arch/x86/include/asm/msr-index.h#L880

As a follow-up here, making COREBOOT_PAYLOAD_MM dependent on !SMP
resolves the need of acknowledging SMI completion. If SMI takes longer,
Linux is just stalled until SMI handler gives the CPU back to the caller.
I think for this case it could be the way, LinuxBoot is by default
compiled without SMP support anyways when used as coreboot's payload [1],
[2].

[1]: https://github.com/coreboot/coreboot/blob/main/payloads/external/LinuxBoot/x86_64/defconfig
[2]: https://github.com/coreboot/coreboot/blob/main/payloads/external/LinuxBoot/i386/defconfig

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

[Tzung-Bi Shih]

On Wed, Jun 25, 2025 at 02:26:11PM +0200, Michal Gorlas wrote:
> On Wed, Jun 25, 2025 at 05:58:54AM +0000, Tzung-Bi Shih wrote:
> > > +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 $@
> > 
> > mm_handler/ isn't visible to this patch. Separate them into the following
> > patch of series?
> > 
> > > diff --git a/drivers/firmware/google/mm_blob.S b/drivers/firmware/google/mm_blob.S
> > > [...]
> > > +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)
> > 
> > mm_handler/ isn't visible to this patch. Separate them into the following
> > patch of series?
> > 
> 
> Would it make sense then to merge patch 2/3 and 3/3 into one? mm_loader
> depends on mm_blob, and mm_blob depends on mm_handler/ being visible.
> I wanted to split these initially as the 3rd patch is already terrible
> to read because of all the assembly code in mm_handler/. But if it makes
> sense to have them as one patch, I'll do that.

Yes, please merge them if that makes sense. Otherwise, you could still
separate them via providing a "nop" mm_handler (e.g. just returns success)
in the 2nd patch and implement the handler details in the 3rd patch.

> > > +static int trigger_smi(u64 cmd, u64 arg, u64 retry)
> > > +{
> > > +	u64 status;
> > > [...]
> > > +
> > > +	if (status == cmd || status == PAYLOAD_MM_RET_FAILURE)
> > > +		status = PAYLOAD_MM_RET_FAILURE;
> > > +	else
> > > +		status = PAYLOAD_MM_RET_SUCCESS;
> > 
> > No. Please use -errno in the kernel.
> > 
> 
> In which line here exactly? In the conditional statement I explicitly
> check for RAX (and hence status) being 1. Not sure if status == EPERM
> would make any sense here. I guess you meant specifically 
> status = PAYLOAD_MM_RET_FAILURE? Then what would be appropriate -errno?
> I think it could be -EREMOTEIO or -EIO, since the APMC SMI which
> trigger_smi does is an I/O write. But I am not sure if that's the
> appropriate errno.

Let trigger_smi() return 0 on success; or, -errno. Eliminate the 2 macros
(PAYLOAD_MM_RET_*).

-EIO makes sense.

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

[Tzung-Bi Shih]

On Thu, Jun 26, 2025 at 01:41:45PM +0200, Michal Gorlas wrote:
> On Wed, Jun 25, 2025 at 02:26:11PM +0200, Michal Gorlas wrote:
> > > > +
> > > > +	/*
> > > > +	 * Gives SMI some time in case it takes longer than expected.
> > > > +	 * Only useful on real hardware (tested on RaptorLake), not needed on emulation.
> > > > +	 */
> > > > +	mdelay(100);
> > > 
> > > This looks weird. Are there some ways for Linux to be aware of the SMI has
> > > completed?
> > 
> > Not in a straight forward fashion. On Intel SoCs we could read MSR_SMI_COUNT
> > [1] before and after sending an SMI, and wait till it increments. I am
> > not aware about any unified way that works for AMD SoCs. However, so far
> > none of the AMD boards supported by coreboot was tested with MM payload,
> > so to make it Intel-only in v3 is not a bad idea.
> > 
> > [1]: https://elixir.bootlin.com/linux/v6.16-rc3/source/arch/x86/include/asm/msr-index.h#L880
> 
> As a follow-up here, making COREBOOT_PAYLOAD_MM dependent on !SMP
> resolves the need of acknowledging SMI completion. If SMI takes longer,
> Linux is just stalled until SMI handler gives the CPU back to the caller.
> I think for this case it could be the way, LinuxBoot is by default
> compiled without SMP support anyways when used as coreboot's payload [1],
> [2].
> 
> [1]: https://github.com/coreboot/coreboot/blob/main/payloads/external/LinuxBoot/x86_64/defconfig
> [2]: https://github.com/coreboot/coreboot/blob/main/payloads/external/LinuxBoot/i386/defconfig

To make sure I understand: do you mean the trigger_smi() becomes a blocking
call without a timeout?

I may misunderstand: depends on !SMP which also means it can't use on most
modern architectures.

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

[Michal Gorlas]

On Fri, Jun 27, 2025 at 02:49:21AM +0000, Tzung-Bi Shih wrote:
> On Thu, Jun 26, 2025 at 01:41:45PM +0200, Michal Gorlas wrote:
> > On Wed, Jun 25, 2025 at 02:26:11PM +0200, Michal Gorlas wrote:
> > > > > +
> > > > > +	/*
> > > > > +	 * Gives SMI some time in case it takes longer than expected.
> > > > > +	 * Only useful on real hardware (tested on RaptorLake), not needed on emulation.
> > > > > +	 */
> > > > > +	mdelay(100);
> > > > 
> > > > This looks weird. Are there some ways for Linux to be aware of the SMI has
> > > > completed?
> > > 
> > > Not in a straight forward fashion. On Intel SoCs we could read MSR_SMI_COUNT
> > > [1] before and after sending an SMI, and wait till it increments. I am
> > > not aware about any unified way that works for AMD SoCs. However, so far
> > > none of the AMD boards supported by coreboot was tested with MM payload,
> > > so to make it Intel-only in v3 is not a bad idea.
> > > 
> > > [1]: https://elixir.bootlin.com/linux/v6.16-rc3/source/arch/x86/include/asm/msr-index.h#L880
> > 
> > As a follow-up here, making COREBOOT_PAYLOAD_MM dependent on !SMP
> > resolves the need of acknowledging SMI completion. If SMI takes longer,
> > Linux is just stalled until SMI handler gives the CPU back to the caller.
> > I think for this case it could be the way, LinuxBoot is by default
> > compiled without SMP support anyways when used as coreboot's payload [1],
> > [2].
> > 
> > [1]: https://github.com/coreboot/coreboot/blob/main/payloads/external/LinuxBoot/x86_64/defconfig
> > [2]: https://github.com/coreboot/coreboot/blob/main/payloads/external/LinuxBoot/i386/defconfig
> 
> To make sure I understand: do you mean the trigger_smi() becomes a blocking
> call without a timeout?

Short answer: potentially yes. Longer answer:

 "mov	%[apmc_port], %%dx\n\t"
 "outb	%%al, %%dx\n\t"

in trigger_smi() will switch CPU context to SMM, meaning, current state
is saved and CPU starts executing whatever instructions are under the
address stored under SMBASE register. At this point, Linux has no control
over that CPU, which can cause chaos, particulary on real hardware.
I am not an expert on how the scheduler is implemented, but I can imagine
it will assign next not-blocked CPU to execute the instructions coming
after trigger_smi(), while SMI is still being handled on other CPU.

> 
> I may misunderstand: depends on !SMP which also means it can't use on most
> modern architectures.

Not necessarily. This driver is useful only if Linux is used to be
coreboot payload (aka LinuxBoot). As far as I know there is no real need
to have SMP enabled there, sole responsibility of the LinuxBoot is to
bring-up remaining hardware (similar to Driver Execution Environment (DXE)
responsibilities when booting with UEFI), and kexec to the OS kernel,
which is likely built with SMP.