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

[Tzung-Bi Shih <tzungbi@kernel.org>]

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?