From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luis Chamberlain Subject: Re: [PATCH v7 2/8] efi: Add embedded peripheral firmware support Date: Thu, 14 Nov 2019 21:50:09 +0000 Message-ID: <20191114215009.GF11244@42.do-not-panic.com> References: <20191004145056.43267-1-hdegoede@redhat.com> <20191004145056.43267-3-hdegoede@redhat.com> <20191011144834.GL16384@42.do-not-panic.com> <20191114194233.GE11244@42.do-not-panic.com> <9b0a0121-3e63-0602-6c0d-00547e389f76@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <9b0a0121-3e63-0602-6c0d-00547e389f76@redhat.com> Sender: linux-kernel-owner@vger.kernel.org To: Hans de Goede Cc: Ard Biesheuvel , Darren Hart , Andy Shevchenko , Greg Kroah-Hartman , "Rafael J . Wysocki" , Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H . Peter Anvin" , Jonathan Corbet , Dmitry Torokhov , Peter Jones , Dave Olsthoorn , x86@kernel.org, platform-driver-x86@vger.kernel.org, linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-input@vger.kernel.org List-Id: platform-driver-x86.vger.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