Message-ID: <aFvq49ODR3XfcwZJ@cyber-t14sg4>
Date: Wed, 25 Jun 2025 14:26:11 +0200
From: Michal Gorlas <michal.gorlas@...ements.com>
To: Tzung-Bi Shih <tzungbi@...nel.org>
Cc: Brian Norris <briannorris@...omium.org>,
	Julius Werner <jwerner@...omium.org>, linux-kernel@...r.kernel.org,
	chrome-platform@...ts.linux.dev,
	Marcello Sylvester Bauer <marcello.bauer@...ements.com>
Subject: Re: [PATCH v2 2/3] firmware: coreboot: loader for Linux-owned SMI
 handler

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

Powered by blists - more mailing lists