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

[Brian Norris <briannorris@chromium.org>]

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