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

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

On Wed, Jun 25, 2025 at 02:26:11PM +0200, Michal Gorlas wrote:
> On Wed, Jun 25, 2025 at 05:58:54AM +0000, Tzung-Bi Shih wrote:
> > > +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.

Yes, please merge them if that makes sense. Otherwise, you could still
separate them via providing a "nop" mm_handler (e.g. just returns success)
in the 2nd patch and implement the handler details in the 3rd patch.

> > > +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.

Let trigger_smi() return 0 on success; or, -errno. Eliminate the 2 macros
(PAYLOAD_MM_RET_*).

-EIO makes sense.