[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aF4C9rPD1xWc0MrP@google.com>
Date: Fri, 27 Jun 2025 02:33:26 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Michal Gorlas <michal.gorlas@...ements.com>
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 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.
Powered by blists - more mailing lists