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

[Brian Norris <briannorris@chromium.org>]

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
>