Re: [PATCH v7 2/8] efi: Add embedded peripheral firmware support
From: Hans de Goede
Date: Thu Nov 14 2019 - 15:48:49 EST
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.
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().
Regards,
Hans