Messages in this thread | | | Subject | Re: [PATCH v7 2/8] efi: Add embedded peripheral firmware support | From | Hans de Goede <> | Date | Thu, 14 Nov 2019 21:48:38 +0100 |
| |
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
| |