From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v7 2/8] efi: Add embedded peripheral firmware support Date: Thu, 14 Nov 2019 21:13:22 +0100 Message-ID: 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> Mime-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20191114194233.GE11244@42.do-not-panic.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Luis Chamberlain 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 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( >>>> +=09efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc) >>>> +{ >>>> +=09const u64 prefix =3D *((u64 *)desc->prefix); >>>> +=09struct sha256_state sctx; >>>> +=09struct embedded_fw *fw; >>>> +=09u8 sha256[32]; >>>> +=09u64 i, size; >>>> +=09void *map; >>>> + >>>> +=09size =3D md->num_pages << EFI_PAGE_SHIFT; >>>> +=09map =3D 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. >=20 > 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. >=20 > 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. Regards, Hans *) which would mean before core_initcalls run so really really early