Re: [PATCH v7 2/8] efi: Add embedded peripheral firmware support

[Luis Chamberlain <mcgrof@kernel.org>]

On Thu, Nov 14, 2019 at 09:48:38PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 14-11-2019 21:13, Hans de Goede wrote:
> > Hi,
> > 
> > On 14-11-2019 20:42, Luis Chamberlain wrote:
> > > On Thu, Nov 14, 2019 at 12:27:01PM +0100, Hans de Goede wrote:
> > > > Hi Luis,
> > > > 
> > > > Thank you for the reviews and sorry for being a bit slow to respind.
> > > > 
> > > > On 11-10-2019 16:48, Luis Chamberlain wrote:
> > > > > On Fri, Oct 04, 2019 at 04:50:50PM +0200, Hans de Goede wrote:
> > > > > > +static int __init efi_check_md_for_embedded_firmware(
> > > > > > +    efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
> > > > > > +{
> > > > > > +    const u64 prefix = *((u64 *)desc->prefix);
> > > > > > +    struct sha256_state sctx;
> > > > > > +    struct embedded_fw *fw;
> > > > > > +    u8 sha256[32];
> > > > > > +    u64 i, size;
> > > > > > +    void *map;
> > > > > > +
> > > > > > +    size = md->num_pages << EFI_PAGE_SHIFT;
> > > > > > +    map = memremap(md->phys_addr, size, MEMREMAP_WB);
> > > > > 
> > > > > Since our limitaiton is the init process must have mostly finished,
> > > > > it implies early x86 boot code cannot use this, what measures can we
> > > > > take to prevent / check for such conditions to be detected and
> > > > > gracefully errored out?
> > > > 
> > > > As with all (EFI) early boot code, there simply is a certain order
> > > > in which things need to be done. This needs to happen after the basic
> > > > mm is setup, but before efi_free_boot_services() gets called, there
> > > > isn't really a way to check for all these conditions. As with all
> > > > early boot code, people making changes need to be careful to not
> > > > break stuff.
> > > 
> > > I rather we take a proactive measure here and add whatever it is we need
> > > to ensure the API works only when its supposed to, rather than try and
> > > fail, and then expect the user to know these things.
> > > 
> > > I'd prefer if we at least try to address this.
> > 
> > This is purely internal x86/EFI API it is not intended for drivers
> > or anything like that. It has only one caller under arch/x86 and it is
> > not supposed to get any other callers outside of arch/* ever.
> > 
> > Note that this all runs before even core_initcall-s get run, none
> > if the code which runs before then has any sort of ordering checks
> > and I don't see how this bit is special and thus does need ordering
> > checks; and there really is no mechanism for such checks so early
> > during boot.
> > 
> > The drivers/firmware/efi/embedded-firmware.c file does add some API
> > which can be used normally, specifically the efi_get_embedded_fw()
> > but that has no special ordering constrains and it does not directly
> > use the function we are discussing now. It reads back data stored
> > by the earlier functions; and if somehow called before those functions
> > run (*), then it will simply return -ENOENT.
> 
> Ok, I just realized that we may have some miscommunication here,
> when you wrote:
> 
> "Since our limitation is the init process must have mostly finished,
>  it implies early x86 boot code cannot use this, what measures can we
>  take to prevent / check for such conditions to be detected and
>  gracefully errored out?"
> 
> I assumed you meant that to apply to the efi_check_md_for_embedded_firmware()
> helper or its caller.
> 
> But I guess what you really want is some error to be thrown if someone
> calls firmware_request_platform() before we are ready.

Yes.

> I guess I could make efi_check_for_embedded_firmwares() which scans
> for known firmwares and saved a copy set a flag that it has run.
> 
> And then combine that with making efi_get_embedded_fw() (which underpins
> firmware_request_platform()) print a warning when called if that flag
> is not set yet.
> 
> That would mean though that some code which runs earlier then
> a core_initcall would, would call firmware_request_platform() and
> such code is generally expected to know what they are doing.
> 
> I just checked and the cpu microcode stuff which comes to mind
> for this uses a late_initcall so runs long after efi_get_embedded_fw()
> and I have a feeling that trying to use the fw_loader before
> core_initcalls have run is going to end poorly anyways.
>
> Still if you want I can add a pr_warn or maybe even a WARN_ON
> to efi_get_embedded_fw() in case it somehow gets called before
> efi_check_for_embedded_firmwares().

That'd be great.

  Luis