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

Hans de Goede
2019-11-14T20:48:48Z

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