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.0Hi 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
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
>
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
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
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
© 2016 - 2025 Red Hat, Inc.