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

[Michal Gorlas <michal.gorlas@9elements.com>]

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