| Message ID | 20200111145703.533809-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | efi/firmware/platform-x86: Add EFI embedded fw support | expand |
On Sat, Jan 11, 2020 at 6:57 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Just like with PCI options ROMs, which we save in the setup_efi_pci* > functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself > sometimes may contain data which is useful/necessary for peripheral drivers > to have access to. > > Specifically the EFI code may contain an embedded copy of firmware which > needs to be (re)loaded into the peripheral. Normally such firmware would be > part of linux-firmware, but in some cases this is not feasible, for 2 > reasons: > > 1) The firmware is customized for a specific use-case of the chipset / use > with a specific hardware model, so we cannot have a single firmware file > for the chipset. E.g. touchscreen controller firmwares are compiled > specifically for the hardware model they are used with, as they are > calibrated for a specific model digitizer. > > 2) Despite repeated attempts we have failed to get permission to > redistribute the firmware. This is especially a problem with customized > firmwares, these get created by the chip vendor for a specific ODM and the > copyright may partially belong with the ODM, so the chip vendor cannot > give a blanket permission to distribute these. > > This commit adds support for finding peripheral firmware embedded in the > EFI code and makes the found firmware available through the new > efi_get_embedded_fw() function. > > Support for loading these firmwares through the standard firmware loading > mechanism is added in a follow-up commit in this patch-series. > > Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end > of start_kernel(), just before calling rest_init(), this is on purpose > because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for > early_memremap(), so the check must be done after mm_init(). This relies > on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services() > is called, which means that this will only work on x86 for now. > A couple general comments: How does this interact with modules? It looks like you're referencing the dmi table from otherwise modular or potentially modular code in early EFI code, which seems like it will either prevent modular builds or will actually fail at compile time depending on config. Perhaps you should have a single collection of EFI firmware references in a separate place in the kernel tree and reference *that* from the EFI code. In the event you have many DMI matches (e.g. if anyone ends up using this in a case where the DMI match has to match very broad things), you'll iterate over the EFI code and data multiple times and performance will suck. It would be much better to iterate once and search for everything. I suspect that a rolling hash would be better than the prefix you're using, but this could be changed later, assuming someone can actually find all the firmware needed. > +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 efi_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); > + if (!map) { > + pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr); > + return -ENOMEM; > + } > + > + for (i = 0; (i + desc->length) <= size; i += 8) { > + u64 *mem = map + i; > + > + if (*mem != prefix) > + continue; This is very ugly. You're casting a pointer to a bunch of bytes to u64* and then dereferencing it like it's an integer. This has major endian issues with are offset by the similar endianness issues when you type-pun prefix to u64. You should instead just memcmp the pointer with the data. This will get rid of all the type punning in this function. You will also fail to detect firmware that isn't 8-byte-aligned. So perhaps just use memmem() to replace this whole mess? > + > + sha256_init(&sctx); > + sha256_update(&sctx, map + i, desc->length); > + sha256_final(&sctx, sha256); > + if (memcmp(sha256, desc->sha256, 32) == 0) > + break; > + } > + if ((i + desc->length) > size) { > + memunmap(map); > + return -ENOENT; > + } > + > + pr_info("Found EFI embedded fw '%s'\n", desc->name); > + It might be nice to also log which EFI section it was in? > + fw = kmalloc(sizeof(*fw), GFP_KERNEL); > + if (!fw) { > + memunmap(map); > + return -ENOMEM; > + } > + > + fw->data = kmemdup(map + i, desc->length, GFP_KERNEL); > + memunmap(map); > + if (!fw->data) { > + kfree(fw); > + return -ENOMEM; > + } > + > + fw->name = desc->name; > + fw->length = desc->length; > + list_add(&fw->list, &efi_embedded_fw_list); > + If you actually copy the firmware name instead of just a pointer to it, then you could potentially free the list of EFI firmwares. Why are you copying the firmware into linear (kmemdup) memory here just to copy it to vmalloc space down below... > + return 0; > +} > + > +void __init efi_check_for_embedded_firmwares(void) > +{ > + const struct efi_embedded_fw_desc *fw_desc; > + const struct dmi_system_id *dmi_id; > + efi_memory_desc_t *md; > + int i, r; > + > + for (i = 0; embedded_fw_table[i]; i++) { > + dmi_id = dmi_first_match(embedded_fw_table[i]); > + if (!dmi_id) > + continue; > + > + fw_desc = dmi_id->driver_data; > + > + /* > + * In some drivers the struct driver_data contains may contain > + * other driver specific data after the fw_desc struct; and > + * the fw_desc struct itself may be empty, skip these. > + */ > + if (!fw_desc->name) > + continue; > + > + for_each_efi_memory_desc(md) { > + if (md->type != EFI_BOOT_SERVICES_CODE) > + continue; > + > + r = efi_check_md_for_embedded_firmware(md, fw_desc); > + if (r == 0) > + break; > + } > + } > + > + checked_for_fw = true; > +} Have you measured how long this takes on a typical system per matching DMI id? > + > +int efi_get_embedded_fw(const char *name, void **data, size_t *size) > +{ > + struct efi_embedded_fw *iter, *fw = NULL; > + void *buf = *data; > + > + if (!checked_for_fw) { WARN_ON_ONCE? A stack dump would be quite nice here. > + buf = vmalloc(fw->length); > + if (!buf) > + return -ENOMEM; > + > + memcpy(buf, fw->data, fw->length); > + *size = fw->length; > + *data = buf; See above. What's vmalloc() for? Where's the vfree()? BTW, it would be very nice to have a straightforward way (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares.
Hi Andy, Thank you for your feedback. On 12-01-2020 23:45, Andy Lutomirski wrote: > On Sat, Jan 11, 2020 at 6:57 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Just like with PCI options ROMs, which we save in the setup_efi_pci* >> functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself >> sometimes may contain data which is useful/necessary for peripheral drivers >> to have access to. >> >> Specifically the EFI code may contain an embedded copy of firmware which >> needs to be (re)loaded into the peripheral. Normally such firmware would be >> part of linux-firmware, but in some cases this is not feasible, for 2 >> reasons: >> >> 1) The firmware is customized for a specific use-case of the chipset / use >> with a specific hardware model, so we cannot have a single firmware file >> for the chipset. E.g. touchscreen controller firmwares are compiled >> specifically for the hardware model they are used with, as they are >> calibrated for a specific model digitizer. >> >> 2) Despite repeated attempts we have failed to get permission to >> redistribute the firmware. This is especially a problem with customized >> firmwares, these get created by the chip vendor for a specific ODM and the >> copyright may partially belong with the ODM, so the chip vendor cannot >> give a blanket permission to distribute these. >> >> This commit adds support for finding peripheral firmware embedded in the >> EFI code and makes the found firmware available through the new >> efi_get_embedded_fw() function. >> >> Support for loading these firmwares through the standard firmware loading >> mechanism is added in a follow-up commit in this patch-series. >> >> Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end >> of start_kernel(), just before calling rest_init(), this is on purpose >> because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for >> early_memremap(), so the check must be done after mm_init(). This relies >> on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services() >> is called, which means that this will only work on x86 for now. >> > > A couple general comments: > > How does this interact with modules? It looks like you're referencing > the dmi table from otherwise modular or potentially modular code in > early EFI code, which seems like it will either prevent modular builds > or will actually fail at compile time depending on config. Perhaps > you should have a single collection of EFI firmware references in a > separate place in the kernel tree and reference *that* from the EFI > code. You are right that this currently relies on the DMI tables added to the embedded_fw_table[] being built in. There currently really only is one (niche) group of X86 devices which benefit from this patch-set. On some cheap X86 tablets and 2-in-1s cheap touchscreen controllers are used which do not have their (model specific) firmware in (p)ROM instead it needs to be loaded by the touchscreen driver. We load the firmware using a model-specific firmware filename through the standard firmware loading mechanism. This is painful for end users of such devices, since they need to get the firmware from somewhere (despite repeated tries we do not have redistribution rights to put them in linux-firmware). Dave Olsthoorn pointed out to me that on his device model the touchscreen worked in Refind, so there is an EFI driver for it, so the firmware should be somewhere in the EFI firmware addressspace. That resulted in this patch-set which will make the touchscreen work OOTB on devices where the firmware is embedded in the UEFI code somewhere. So (for now) this new mechanism is only used by a small niche group of devices. The X86 devices which these touchscreens already need a bunch of device model specific metadata, like the model specific firmware filename and also the resolution of the touchscreen, orientation, etc. We already have a "driver" which adds this info to the device during enumeration using device-properties: drivers/platform/x86/touchscreen_dmi.c This code already is builtin as it needs to add a bus-notifier before i2c bus enumeration to be able to add the device-properties before the devices are proped (it uses an arch_initcall). So here we already have a dmi_system_id table with matches for all devices in our small niche group. The current mechanism where a driver "registers" its dmi_system_id table in the embedded_fw_table[] table is based on this use-case. This avoids needing to duplicate the dmi-match strings in 2 places and allows us keeping all the info related to a touchscreen in a single place, e.g. the entry for the Cube iWorks 8 air looks like this: static const struct property_entry cube_iwork8_air_props[] = { PROPERTY_ENTRY_U32("touchscreen-min-x", 1), PROPERTY_ENTRY_U32("touchscreen-min-y", 3), PROPERTY_ENTRY_U32("touchscreen-size-x", 1664), PROPERTY_ENTRY_U32("touchscreen-size-y", 896), PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"), PROPERTY_ENTRY_STRING("firmware-name", "gsl3670-cube-iwork8-air.fw"), PROPERTY_ENTRY_U32("silead,max-fingers", 10), { } }; static const struct ts_dmi_data cube_iwork8_air_data = { .embedded_fw = { .name = "silead/gsl3670-cube-iwork8-air.fw", .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 }, .length = 38808, .sha256 = { 0xff, 0x62, 0x2d, 0xd1, 0x8a, 0x78, 0x04, 0x7b, 0x33, 0x06, 0xb0, 0x4f, 0x7f, 0x02, 0x08, 0x9c, 0x96, 0xd4, 0x9f, 0x04, 0xe1, 0x47, 0x25, 0x25, 0x60, 0x77, 0x41, 0x33, 0xeb, 0x12, 0x82, 0xfc }, }, .acpi_name = "MSSL1680:00", .properties = cube_iwork8_air_props, }; And then the driver_data for the DMI match for this device points to cube_iwork8_air_data. For now doing it this way makes more sense IMHO then duplicating the DMI matches inside drivers/firmware/efi/embedded-firmware.c and putting the embedded_fw info there, where the rest of the info lives in drivers/platform/x86/touchscreen_dmi.c. This is all internal kernel API (not even exported to modules) so if more users of the EFI-embedded-fw mechanism show up and this is a poor fit for them, then we can re-evaluate this. Also note that the drivers/platform/x86/touchscreen_dmi.c sees a number of commits each kernel cycle, keeping the churn for adding touchscreen info for new device models located to 1 file also is a good thing about the current approach. So far the drivers/platform/x86 maintainers have not complained about the churn being concentrated there... > In the event you have many DMI matches (e.g. if anyone ends up using > this in a case where the DMI match has to match very broad things), > you'll iterate over the EFI code and data multiple times and > performance will suck. It would be much better to iterate once and > search for everything. As I said this is targeting a small niche group of X86 device models, so this is very much optimized for there being zero DMI matches and since all the touchscreen info is model specific we use narrow DMI matches up to matching the exact BIOS version and/or date when the other DMI info is too generic. And since ATM touchscreen drivers are the only user there should never be more then 1 DMI match. Note this is also assumed in the code, it only calls dmi_first_match() once on each registered dmi_system_id table and ATM we only have 0 or 1 registered dmi_system_id tables. Even if we get more use of this the chances of 1 device model using more then 1 embedded fw are small. Where as if we first map the EFI_BOOT_SERVICES_CODE segment before doing the DMI match then we do this for all EFI_BOOT_SERVICES_CODE segments on all hardware. The current implementation is very much optimized to do as little work as possible when there is no DMI match, which will be true on almost all devices out there. > I suspect that a rolling hash would be better than the prefix you're > using, but this could be changed later, assuming someone can actually > find all the firmware needed. > >> +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 efi_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); >> + if (!map) { >> + pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr); >> + return -ENOMEM; >> + } >> + >> + for (i = 0; (i + desc->length) <= size; i += 8) { >> + u64 *mem = map + i; >> + >> + if (*mem != prefix) >> + continue; > > This is very ugly. You're casting a pointer to a bunch of bytes to > u64* and then dereferencing it like it's an integer. This has major > endian issues with are offset by the similar endianness issues when > you type-pun prefix to u64. You should instead just memcmp the > pointer with the data. This will get rid of all the type punning in > this function. Ok, I will switch to memcmp for the next version. > You will also fail to detect firmware that isn't > 8-byte-aligned. This is by design, currently being 8 byte aligned holds true for all devices on which this is used, also see the kdoc added in "[PATCH v11 04/10] firmware: Add new platform fallback mechanism and firmware_request_platform()" Specifically: +EFI embedded firmware fallback mechanism +======================================== + +On some devices the system's EFI code / ROM may contain an embedded copy +of firmware for some of the system's integrated peripheral devices and +the peripheral's Linux device-driver needs to access this firmware. + +Device drivers which need such firmware can use the +firmware_request_platform() function for this, note that this is a +separate fallback mechanism from the other fallback mechanisms and +this does not use the sysfs interface. + +A device driver which needs this can describe the firmware it needs +using an efi_embedded_fw_desc struct: + +.. kernel-doc:: include/linux/efi_embedded_fw.h + :functions: efi_embedded_fw_desc + +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory +segments for an eight byte sequence matching prefix; if the prefix is found it +then does a sha256 over length bytes and if that matches makes a copy of length +bytes and adds that to its list with found firmwares. + +To avoid doing this somewhat expensive scan on all systems, dmi matching is +used. Drivers are expected to export a dmi_system_id array, with each entries' +driver_data pointing to an efi_embedded_fw_desc. + +To register this array with the efi-embedded-fw code, a driver needs to: + +1. Always be builtin to the kernel or store the dmi_system_id array in a + separate object file which always gets builtin. + +2. Add an extern declaration for the dmi_system_id array to + include/linux/efi_embedded_fw.h. + +3. Add the dmi_system_id array to the embedded_fw_table in + drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that + the driver is being builtin. + +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry. + +The firmware_request_platform() function will always first try to load firmware +with the specified name directly from the disk, so the EFI embedded-fw can +always be overridden by placing a file under /lib/firmware. + +Note that: + +1. The code scanning for EFI embedded-firmware runs near the end + of start_kernel(), just before calling rest_init(). For normal drivers and + subsystems using subsys_initcall() to register themselves this does not + matter. This means that code running earlier cannot use EFI + embedded-firmware. + +2. At the moment the EFI embedded-fw code assumes that firmwares always start at + an offset which is a multiple of 8 bytes, if this is not true for your case + send in a patch to fix this. Note this also documents your concern about the dmi_system_id table needing to be builtin. > So perhaps just use memmem() to replace this whole mess? If we hit firmware which is not 8 byte aligned, then yes that would be a good idea, but for now I feel that that would just cause a slowdown in the scanning without any benefits. >> + >> + sha256_init(&sctx); >> + sha256_update(&sctx, map + i, desc->length); >> + sha256_final(&sctx, sha256); >> + if (memcmp(sha256, desc->sha256, 32) == 0) >> + break; >> + } >> + if ((i + desc->length) > size) { >> + memunmap(map); >> + return -ENOENT; >> + } >> + >> + pr_info("Found EFI embedded fw '%s'\n", desc->name); >> + > > It might be nice to also log which EFI section it was in? The EFI sections do not have names, so all I could is log the section number which does not really feel useful? >> + fw = kmalloc(sizeof(*fw), GFP_KERNEL); >> + if (!fw) { >> + memunmap(map); >> + return -ENOMEM; >> + } >> + >> + fw->data = kmemdup(map + i, desc->length, GFP_KERNEL); >> + memunmap(map); >> + if (!fw->data) { >> + kfree(fw); >> + return -ENOMEM; >> + } >> + >> + fw->name = desc->name; >> + fw->length = desc->length; >> + list_add(&fw->list, &efi_embedded_fw_list); >> + > > If you actually copy the firmware name instead of just a pointer to > it, then you could potentially free the list of EFI firmwares. If we move to having a separate dmi_system_id table for this then that would be true. ATM the dmi_system_id from drivers/platform/x86/touchscreen_dmi.c is not freed as it is referenced from a bus-notifier. > Why are you copying the firmware into linear (kmemdup) memory here The kmemdup is because the EFI_BOOT_SERVICES_CODE section is free-ed almost immediately after we run. > just to copy it to vmalloc space down below... The vmalloc is because the efi_get_embedded_fw() function is a helper for later implementing firmware_Request_platform which returns a struct firmware *fw and release_firmware() uses vfree() to free the firmware contents. I guess we could have efi_get_embedded_fw() return an const u8 * and have the firmware code do the vmalloc + memcpy but it boils down to the same thing. > >> + return 0; >> +} >> + >> +void __init efi_check_for_embedded_firmwares(void) >> +{ >> + const struct efi_embedded_fw_desc *fw_desc; >> + const struct dmi_system_id *dmi_id; >> + efi_memory_desc_t *md; >> + int i, r; >> + >> + for (i = 0; embedded_fw_table[i]; i++) { >> + dmi_id = dmi_first_match(embedded_fw_table[i]); >> + if (!dmi_id) >> + continue; >> + >> + fw_desc = dmi_id->driver_data; >> + >> + /* >> + * In some drivers the struct driver_data contains may contain >> + * other driver specific data after the fw_desc struct; and >> + * the fw_desc struct itself may be empty, skip these. >> + */ >> + if (!fw_desc->name) >> + continue; >> + >> + for_each_efi_memory_desc(md) { >> + if (md->type != EFI_BOOT_SERVICES_CODE) >> + continue; >> + >> + r = efi_check_md_for_embedded_firmware(md, fw_desc); >> + if (r == 0) >> + break; >> + } >> + } >> + >> + checked_for_fw = true; >> +} > > Have you measured how long this takes on a typical system per matching DMI id? I originally wrote this approx. 18 months ago, it has been on hold for a while since it needed a sha256 method which would work before subsys_initcall-s run so I couldn't use the standard crypto_alloc_shash() stuff. In the end I ended up merging the duplicate purgatory and crypto/sha256_generic.c generic C SHA256 implementation into a set of new directly callable lib functions in lib/crypto/sha256.c, just so that I could move forward with this... Anyways the reason for this background info is that because this is a while ago I do not remember exactly how, but yes I did measure this (but not very scientifically) and there was no discernible difference in boot to init (from the initrd) with this in place vs without this. >> + >> +int efi_get_embedded_fw(const char *name, void **data, size_t *size) >> +{ >> + struct efi_embedded_fw *iter, *fw = NULL; >> + void *buf = *data; >> + >> + if (!checked_for_fw) { > > WARN_ON_ONCE? A stack dump would be quite nice here. As discussed when this check was added (discussion in v7 of the set I guess, check added in v8) we can also hit this without it being a bug, e.g. when booted through kexec the whole efi_check_for_embedded_firmwares() call we be skipped, hence the pr_warn. >> + buf = vmalloc(fw->length); >> + if (!buf) >> + return -ENOMEM; >> + >> + memcpy(buf, fw->data, fw->length); >> + *size = fw->length; >> + *data = buf; > > See above. What's vmalloc() for? Where's the vfree()? See above for my answer. I'm fine with moving this into the firmware code, since that is where the matching vfree is, please let me know how you want to proceed with this. > BTW, it would be very nice to have a straightforward way > (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares. That is an interesting idea, we could add a subsys_init call or some such to add this to debugfs (efi_check_for_embedded_firmwares runs too early). But given how long this patch-set has been in the making I would really like to get this (or at least the first 2 patches as a start) upstream, so for now I would like to keep the changes to a minimum. Are you ok with me adding the debugfs support with a follow-up patch ? Let me also reply to your summary comment elsewhere in the thread here: > Early in boot, you check a bunch of firmware descriptors, bundled with > drivers, to search EFI code and data for firmware before you free said > code and data. You catalogue it by name. Later on, you use this list > as a fallback, again catalogued by name. I think it would be rather > nicer if you simply had a list in a single file containing all these > descriptors rather than commingling it with the driver's internal dmi > data. This gets rid of all the ifdeffery and module issues. It also > avoids any potential nastiness when you have a dmi entry that contains > driver_data that points into the driver when the driver is a module. > > And you can mark the entire list __initdata. And you can easily (now > or later on) invert the code flow so you map each EFI region exactly > once and then search for all the firmware in it. I believe we have mostly discussed this above. At least for the X86 touchscreen case, which is the only user of this for now, I believe that re-using the table from drivers/platform/x86/touchscreen_dmi.c is better as it avoids duplication of DMI strings and it keeps all the info in one place (also avoiding churn in 2 files / 2 different trees when new models are added). I agree that when looking at this as a generic mechanism which may be used elsewhere, your suggestion makes a lot of sense. But even though this is very much written so that it can be used as a generic mechanism I'm not sure if other users will appear. So for now, with only the X86 touchscreen use-case actually using this I believe the current implementation is best, but I'm definitely open to changing this around if more users show up. Regards, Hans
On Tue, Jan 14, 2020 at 2:25 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Andy, > > Thank you for your feedback. Another Andy is here. Sorry for the summary at the top of letter, I left below specifically for Sakari to have a bit of context. Sakari, do you know if IPU firmware is going to utilize something like touchscreen devices? Hans, I am completely in favour of more generic approach with less quirks applied. For now the burden on PDx86, from maintenance perspective, is not big, we may survive. > > On 12-01-2020 23:45, Andy Lutomirski wrote: > > On Sat, Jan 11, 2020 at 6:57 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Just like with PCI options ROMs, which we save in the setup_efi_pci* > >> functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself > >> sometimes may contain data which is useful/necessary for peripheral drivers > >> to have access to. > >> > >> Specifically the EFI code may contain an embedded copy of firmware which > >> needs to be (re)loaded into the peripheral. Normally such firmware would be > >> part of linux-firmware, but in some cases this is not feasible, for 2 > >> reasons: > >> > >> 1) The firmware is customized for a specific use-case of the chipset / use > >> with a specific hardware model, so we cannot have a single firmware file > >> for the chipset. E.g. touchscreen controller firmwares are compiled > >> specifically for the hardware model they are used with, as they are > >> calibrated for a specific model digitizer. > >> > >> 2) Despite repeated attempts we have failed to get permission to > >> redistribute the firmware. This is especially a problem with customized > >> firmwares, these get created by the chip vendor for a specific ODM and the > >> copyright may partially belong with the ODM, so the chip vendor cannot > >> give a blanket permission to distribute these. > >> > >> This commit adds support for finding peripheral firmware embedded in the > >> EFI code and makes the found firmware available through the new > >> efi_get_embedded_fw() function. > >> > >> Support for loading these firmwares through the standard firmware loading > >> mechanism is added in a follow-up commit in this patch-series. > >> > >> Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end > >> of start_kernel(), just before calling rest_init(), this is on purpose > >> because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for > >> early_memremap(), so the check must be done after mm_init(). This relies > >> on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services() > >> is called, which means that this will only work on x86 for now. > >> > > > > A couple general comments: > > > > How does this interact with modules? It looks like you're referencing > > the dmi table from otherwise modular or potentially modular code in > > early EFI code, which seems like it will either prevent modular builds > > or will actually fail at compile time depending on config. Perhaps > > you should have a single collection of EFI firmware references in a > > separate place in the kernel tree and reference *that* from the EFI > > code. > > You are right that this currently relies on the DMI tables > added to the embedded_fw_table[] being built in. > > There currently really only is one (niche) group of X86 devices which > benefit from this patch-set. On some cheap X86 tablets and 2-in-1s > cheap touchscreen controllers are used which do not have their > (model specific) firmware in (p)ROM instead it needs to be loaded > by the touchscreen driver. We load the firmware using a model-specific > firmware filename through the standard firmware loading mechanism. > This is painful for end users of such devices, since they need to get > the firmware from somewhere (despite repeated tries we do not > have redistribution rights to put them in linux-firmware). > > Dave Olsthoorn pointed out to me that on his device model the touchscreen > worked in Refind, so there is an EFI driver for it, so the firmware should > be somewhere in the EFI firmware addressspace. That resulted in this > patch-set which will make the touchscreen work OOTB on devices where > the firmware is embedded in the UEFI code somewhere. > > So (for now) this new mechanism is only used by a small niche > group of devices. > > The X86 devices which these touchscreens already need a bunch of > device model specific metadata, like the model specific firmware > filename and also the resolution of the touchscreen, orientation, etc. > > We already have a "driver" which adds this info to the device > during enumeration using device-properties: > drivers/platform/x86/touchscreen_dmi.c > > This code already is builtin as it needs to add a bus-notifier > before i2c bus enumeration to be able to add the device-properties > before the devices are proped (it uses an arch_initcall). > > So here we already have a dmi_system_id table with matches for > all devices in our small niche group. The current mechanism > where a driver "registers" its dmi_system_id table in the > embedded_fw_table[] table is based on this use-case. > > This avoids needing to duplicate the dmi-match strings in 2 > places and allows us keeping all the info related to a > touchscreen in a single place, e.g. the entry for the Cube > iWorks 8 air looks like this: > > static const struct property_entry cube_iwork8_air_props[] = { > PROPERTY_ENTRY_U32("touchscreen-min-x", 1), > PROPERTY_ENTRY_U32("touchscreen-min-y", 3), > PROPERTY_ENTRY_U32("touchscreen-size-x", 1664), > PROPERTY_ENTRY_U32("touchscreen-size-y", 896), > PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"), > PROPERTY_ENTRY_STRING("firmware-name", "gsl3670-cube-iwork8-air.fw"), > PROPERTY_ENTRY_U32("silead,max-fingers", 10), > { } > }; > > static const struct ts_dmi_data cube_iwork8_air_data = { > .embedded_fw = { > .name = "silead/gsl3670-cube-iwork8-air.fw", > .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 }, > .length = 38808, > .sha256 = { 0xff, 0x62, 0x2d, 0xd1, 0x8a, 0x78, 0x04, 0x7b, > 0x33, 0x06, 0xb0, 0x4f, 0x7f, 0x02, 0x08, 0x9c, > 0x96, 0xd4, 0x9f, 0x04, 0xe1, 0x47, 0x25, 0x25, > 0x60, 0x77, 0x41, 0x33, 0xeb, 0x12, 0x82, 0xfc }, > }, > .acpi_name = "MSSL1680:00", > .properties = cube_iwork8_air_props, > }; > > And then the driver_data for the DMI match for this device > points to cube_iwork8_air_data. > > For now doing it this way makes more sense IMHO then duplicating the > DMI matches inside drivers/firmware/efi/embedded-firmware.c and > putting the embedded_fw info there, where the rest of the info > lives in drivers/platform/x86/touchscreen_dmi.c. > > This is all internal kernel API (not even exported to modules) so > if more users of the EFI-embedded-fw mechanism show up and this is a > poor fit for them, then we can re-evaluate this. > > Also note that the drivers/platform/x86/touchscreen_dmi.c sees a number > of commits each kernel cycle, keeping the churn for adding touchscreen > info for new device models located to 1 file also is a good thing > about the current approach. So far the drivers/platform/x86 maintainers > have not complained about the churn being concentrated there... > > > In the event you have many DMI matches (e.g. if anyone ends up using > > this in a case where the DMI match has to match very broad things), > > you'll iterate over the EFI code and data multiple times and > > performance will suck. It would be much better to iterate once and > > search for everything. > > As I said this is targeting a small niche group of X86 device models, > so this is very much optimized for there being zero DMI matches and > since all the touchscreen info is model specific we use narrow DMI > matches up to matching the exact BIOS version and/or date when the > other DMI info is too generic. > > And since ATM touchscreen drivers are the only user there should > never be more then 1 DMI match. Note this is also assumed in > the code, it only calls dmi_first_match() once on each registered > dmi_system_id table and ATM we only have 0 or 1 registered > dmi_system_id tables. > > Even if we get more use of this the chances of 1 device model using > more then 1 embedded fw are small. Where as if we first map the > EFI_BOOT_SERVICES_CODE segment before doing the DMI match then we > do this for all EFI_BOOT_SERVICES_CODE segments on all hardware. > The current implementation is very much optimized to do as little > work as possible when there is no DMI match, which will be true > on almost all devices out there. > > > I suspect that a rolling hash would be better than the prefix you're > > using, but this could be changed later, assuming someone can actually > > find all the firmware needed. > > > >> +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 efi_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); > >> + if (!map) { > >> + pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr); > >> + return -ENOMEM; > >> + } > >> + > >> + for (i = 0; (i + desc->length) <= size; i += 8) { > >> + u64 *mem = map + i; > >> + > >> + if (*mem != prefix) > >> + continue; > > > > This is very ugly. You're casting a pointer to a bunch of bytes to > > u64* and then dereferencing it like it's an integer. This has major > > endian issues with are offset by the similar endianness issues when > > you type-pun prefix to u64. You should instead just memcmp the > > pointer with the data. This will get rid of all the type punning in > > this function. > > Ok, I will switch to memcmp for the next version. > > > You will also fail to detect firmware that isn't > > 8-byte-aligned. > > This is by design, currently being 8 byte aligned holds true for > all devices on which this is used, also see the kdoc added in > "[PATCH v11 04/10] firmware: Add new platform fallback mechanism and firmware_request_platform()" > > Specifically: > > +EFI embedded firmware fallback mechanism > +======================================== > + > +On some devices the system's EFI code / ROM may contain an embedded copy > +of firmware for some of the system's integrated peripheral devices and > +the peripheral's Linux device-driver needs to access this firmware. > + > +Device drivers which need such firmware can use the > +firmware_request_platform() function for this, note that this is a > +separate fallback mechanism from the other fallback mechanisms and > +this does not use the sysfs interface. > + > +A device driver which needs this can describe the firmware it needs > +using an efi_embedded_fw_desc struct: > + > +.. kernel-doc:: include/linux/efi_embedded_fw.h > + :functions: efi_embedded_fw_desc > + > +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory > +segments for an eight byte sequence matching prefix; if the prefix is found it > +then does a sha256 over length bytes and if that matches makes a copy of length > +bytes and adds that to its list with found firmwares. > + > +To avoid doing this somewhat expensive scan on all systems, dmi matching is > +used. Drivers are expected to export a dmi_system_id array, with each entries' > +driver_data pointing to an efi_embedded_fw_desc. > + > +To register this array with the efi-embedded-fw code, a driver needs to: > + > +1. Always be builtin to the kernel or store the dmi_system_id array in a > + separate object file which always gets builtin. > + > +2. Add an extern declaration for the dmi_system_id array to > + include/linux/efi_embedded_fw.h. > + > +3. Add the dmi_system_id array to the embedded_fw_table in > + drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that > + the driver is being builtin. > + > +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry. > + > +The firmware_request_platform() function will always first try to load firmware > +with the specified name directly from the disk, so the EFI embedded-fw can > +always be overridden by placing a file under /lib/firmware. > + > +Note that: > + > +1. The code scanning for EFI embedded-firmware runs near the end > + of start_kernel(), just before calling rest_init(). For normal drivers and > + subsystems using subsys_initcall() to register themselves this does not > + matter. This means that code running earlier cannot use EFI > + embedded-firmware. > + > +2. At the moment the EFI embedded-fw code assumes that firmwares always start at > + an offset which is a multiple of 8 bytes, if this is not true for your case > + send in a patch to fix this. > > > Note this also documents your concern about the dmi_system_id > table needing to be builtin. > > > So perhaps just use memmem() to replace this whole mess? > > If we hit firmware which is not 8 byte aligned, then yes that would be > a good idea, but for now I feel that that would just cause a slowdown > in the scanning without any benefits. > > >> + > >> + sha256_init(&sctx); > >> + sha256_update(&sctx, map + i, desc->length); > >> + sha256_final(&sctx, sha256); > >> + if (memcmp(sha256, desc->sha256, 32) == 0) > >> + break; > >> + } > >> + if ((i + desc->length) > size) { > >> + memunmap(map); > >> + return -ENOENT; > >> + } > >> + > >> + pr_info("Found EFI embedded fw '%s'\n", desc->name); > >> + > > > > It might be nice to also log which EFI section it was in? > > The EFI sections do not have names, so all I could is log > the section number which does not really feel useful? > > >> + fw = kmalloc(sizeof(*fw), GFP_KERNEL); > >> + if (!fw) { > >> + memunmap(map); > >> + return -ENOMEM; > >> + } > >> + > >> + fw->data = kmemdup(map + i, desc->length, GFP_KERNEL); > >> + memunmap(map); > >> + if (!fw->data) { > >> + kfree(fw); > >> + return -ENOMEM; > >> + } > >> + > >> + fw->name = desc->name; > >> + fw->length = desc->length; > >> + list_add(&fw->list, &efi_embedded_fw_list); > >> + > > > > If you actually copy the firmware name instead of just a pointer to > > it, then you could potentially free the list of EFI firmwares. > > If we move to having a separate dmi_system_id table for this then > that would be true. ATM the dmi_system_id from > drivers/platform/x86/touchscreen_dmi.c > is not freed as it is referenced from a bus-notifier. > > > Why are you copying the firmware into linear (kmemdup) memory here > > The kmemdup is because the EFI_BOOT_SERVICES_CODE section is > free-ed almost immediately after we run. > > > just to copy it to vmalloc space down below... > > The vmalloc is because the efi_get_embedded_fw() function is > a helper for later implementing firmware_Request_platform > which returns a struct firmware *fw and release_firmware() > uses vfree() to free the firmware contents. > > I guess we could have efi_get_embedded_fw() return an const u8 * > and have the firmware code do the vmalloc + memcpy but it boils > down to the same thing. > > > > > >> + return 0; > >> +} > >> + > >> +void __init efi_check_for_embedded_firmwares(void) > >> +{ > >> + const struct efi_embedded_fw_desc *fw_desc; > >> + const struct dmi_system_id *dmi_id; > >> + efi_memory_desc_t *md; > >> + int i, r; > >> + > >> + for (i = 0; embedded_fw_table[i]; i++) { > >> + dmi_id = dmi_first_match(embedded_fw_table[i]); > >> + if (!dmi_id) > >> + continue; > >> + > >> + fw_desc = dmi_id->driver_data; > >> + > >> + /* > >> + * In some drivers the struct driver_data contains may contain > >> + * other driver specific data after the fw_desc struct; and > >> + * the fw_desc struct itself may be empty, skip these. > >> + */ > >> + if (!fw_desc->name) > >> + continue; > >> + > >> + for_each_efi_memory_desc(md) { > >> + if (md->type != EFI_BOOT_SERVICES_CODE) > >> + continue; > >> + > >> + r = efi_check_md_for_embedded_firmware(md, fw_desc); > >> + if (r == 0) > >> + break; > >> + } > >> + } > >> + > >> + checked_for_fw = true; > >> +} > > > > Have you measured how long this takes on a typical system per matching DMI id? > > I originally wrote this approx. 18 months ago, it has been on hold for a while > since it needed a sha256 method which would work before subsys_initcall-s > run so I couldn't use the standard crypto_alloc_shash() stuff. In the end > I ended up merging the duplicate purgatory and crypto/sha256_generic.c > generic C SHA256 implementation into a set of new directly callable lib > functions in lib/crypto/sha256.c, just so that I could move forward with > this... > > Anyways the reason for this background info is that because this is a while > ago I do not remember exactly how, but yes I did measure this (but not > very scientifically) and there was no discernible difference in boot > to init (from the initrd) with this in place vs without this. > > >> + > >> +int efi_get_embedded_fw(const char *name, void **data, size_t *size) > >> +{ > >> + struct efi_embedded_fw *iter, *fw = NULL; > >> + void *buf = *data; > >> + > >> + if (!checked_for_fw) { > > > > WARN_ON_ONCE? A stack dump would be quite nice here. > > As discussed when this check was added (discussion in v7 of > the set I guess, check added in v8) we can also hit this without > it being a bug, e.g. when booted through kexec the whole > efi_check_for_embedded_firmwares() call we be skipped, hence the > pr_warn. > > > >> + buf = vmalloc(fw->length); > >> + if (!buf) > >> + return -ENOMEM; > >> + > >> + memcpy(buf, fw->data, fw->length); > >> + *size = fw->length; > >> + *data = buf; > > > > See above. What's vmalloc() for? Where's the vfree()? > > See above for my answer. I'm fine with moving this into the > firmware code, since that is where the matching vfree is, please > let me know how you want to proceed with this. > > > BTW, it would be very nice to have a straightforward way > > (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares. > > That is an interesting idea, we could add a subsys_init call or > some such to add this to debugfs (efi_check_for_embedded_firmwares runs > too early). > > But given how long this patch-set has been in the making I would really > like to get this (or at least the first 2 patches as a start) upstream, > so for now I would like to keep the changes to a minimum. Are you ok > with me adding the debugfs support with a follow-up patch ? > > Let me also reply to your summary comment elsewhere in the thread here: > > > Early in boot, you check a bunch of firmware descriptors, bundled with > > drivers, to search EFI code and data for firmware before you free said > > code and data. You catalogue it by name. Later on, you use this list > > as a fallback, again catalogued by name. I think it would be rather > > nicer if you simply had a list in a single file containing all these > > descriptors rather than commingling it with the driver's internal dmi > > data. This gets rid of all the ifdeffery and module issues. It also > > avoids any potential nastiness when you have a dmi entry that contains > > driver_data that points into the driver when the driver is a module. > > > > And you can mark the entire list __initdata. And you can easily (now > > or later on) invert the code flow so you map each EFI region exactly > > once and then search for all the firmware in it. > > I believe we have mostly discussed this above. At least for the > X86 touchscreen case, which is the only user of this for now, I > believe that re-using the table from drivers/platform/x86/touchscreen_dmi.c > is better as it avoids duplication of DMI strings and it keeps all > the info in one place (also avoiding churn in 2 files / 2 different > trees when new models are added). > > I agree that when looking at this as a generic mechanism which may be > used elsewhere, your suggestion makes a lot of sense. But even though > this is very much written so that it can be used as a generic mechanism > I'm not sure if other users will appear. So for now, with only the X86 > touchscreen use-case actually using this I believe the current > implementation is best, but I'm definitely open to changing this around > if more users show up. > > Regards, > > Hans >
> On Jan 14, 2020, at 4:25 AM, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Andy, > > Even if we get more use of this the chances of 1 device model using > more then 1 embedded fw are small. Where as if we first map the > EFI_BOOT_SERVICES_CODE segment before doing the DMI match then we > do this for all EFI_BOOT_SERVICES_CODE segments on all hardware. > The current implementation is very much optimized to do as little > work as possible when there is no DMI match, which will be true > on almost all devices out there. Fine with me. > If we hit firmware which is not 8 byte aligned, then yes that would be > a good idea, but for now I feel that that would just cause a slowdown > in the scanning without any benefits. > It would shorten the code and remove a comment :). Also, a good memmem implementation should be very fast, potentially faster than your loop. I suspect the latter is only true in user code where SSE would get used, but still. it would also be unfortunate if some firmware update switches to 4-byte alignment and touchscreens stop working with no explanation. >>> + >>> + sha256_init(&sctx); >>> + sha256_update(&sctx, map + i, desc->length); >>> + sha256_final(&sctx, sha256); >>> + if (memcmp(sha256, desc->sha256, 32) == 0) >>> + break; >>> + } >>> + if ((i + desc->length) > size) { >>> + memunmap(map); >>> + return -ENOENT; >>> + } >>> + >>> + pr_info("Found EFI embedded fw '%s'\n", desc->name); >>> + >> It might be nice to also log which EFI section it was in? > > The EFI sections do not have names, so all I could is log > the section number which does not really feel useful? > >>> + fw = kmalloc(sizeof(*fw), GFP_KERNEL); >>> + if (!fw) { >>> + memunmap(map); >>> + return -ENOMEM; >>> + } >>> + >>> + fw->data = kmemdup(map + i, desc->length, GFP_KERNEL); >>> + memunmap(map); >>> + if (!fw->data) { >>> + kfree(fw); >>> + return -ENOMEM; >>> + } >>> + >>> + fw->name = desc->name; >>> + fw->length = desc->length; >>> + list_add(&fw->list, &efi_embedded_fw_list); >>> + >> If you actually copy the firmware name instead of just a pointer to >> it, then you could potentially free the list of EFI firmwares. > > If we move to having a separate dmi_system_id table for this then > that would be true. ATM the dmi_system_id from > drivers/platform/x86/touchscreen_dmi.c > is not freed as it is referenced from a bus-notifier. > >> Why are you copying the firmware into linear (kmemdup) memory here > > The kmemdup is because the EFI_BOOT_SERVICES_CODE section is > free-ed almost immediately after we run. > >> just to copy it to vmalloc space down below... > > The vmalloc is because the efi_get_embedded_fw() function is > a helper for later implementing firmware_Request_platform > which returns a struct firmware *fw and release_firmware() > uses vfree() to free the firmware contents. > > I guess we could have efi_get_embedded_fw() return an const u8 * > and have the firmware code do the vmalloc + memcpy but it boils > down to the same thing. > > >>> + return 0; >>> +} >>> + >>> +void __init efi_check_for_embedded_firmwares(void) >>> +{ >>> + const struct efi_embedded_fw_desc *fw_desc; >>> + const struct dmi_system_id *dmi_id; >>> + efi_memory_desc_t *md; >>> + int i, r; >>> + >>> + for (i = 0; embedded_fw_table[i]; i++) { >>> + dmi_id = dmi_first_match(embedded_fw_table[i]); >>> + if (!dmi_id) >>> + continue; >>> + >>> + fw_desc = dmi_id->driver_data; >>> + >>> + /* >>> + * In some drivers the struct driver_data contains may contain >>> + * other driver specific data after the fw_desc struct; and >>> + * the fw_desc struct itself may be empty, skip these. >>> + */ >>> + if (!fw_desc->name) >>> + continue; >>> + >>> + for_each_efi_memory_desc(md) { >>> + if (md->type != EFI_BOOT_SERVICES_CODE) >>> + continue; >>> + >>> + r = efi_check_md_for_embedded_firmware(md, fw_desc); >>> + if (r == 0) >>> + break; >>> + } >>> + } >>> + >>> + checked_for_fw = true; >>> +} >> Have you measured how long this takes on a typical system per matching DMI id? > > I originally wrote this approx. 18 months ago, it has been on hold for a while > since it needed a sha256 method which would work before subsys_initcall-s > run so I couldn't use the standard crypto_alloc_shash() stuff. In the end > I ended up merging the duplicate purgatory and crypto/sha256_generic.c > generic C SHA256 implementation into a set of new directly callable lib > functions in lib/crypto/sha256.c, just so that I could move forward with > this... > > Anyways the reason for this background info is that because this is a while > ago I do not remember exactly how, but yes I did measure this (but not > very scientifically) and there was no discernible difference in boot > to init (from the initrd) with this in place vs without this. > >>> + >>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size) >>> +{ >>> + struct efi_embedded_fw *iter, *fw = NULL; >>> + void *buf = *data; >>> + >>> + if (!checked_for_fw) { >> WARN_ON_ONCE? A stack dump would be quite nice here. > > As discussed when this check was added (discussion in v7 of > the set I guess, check added in v8) we can also hit this without > it being a bug, e.g. when booted through kexec the whole > efi_check_for_embedded_firmwares() call we be skipped, hence the > pr_warn. > > >>> + buf = vmalloc(fw->length); >>> + if (!buf) >>> + return -ENOMEM; >>> + >>> + memcpy(buf, fw->data, fw->length); >>> + *size = fw->length; >>> + *data = buf; >> See above. What's vmalloc() for? Where's the vfree()? > > See above for my answer. I'm fine with moving this into the > firmware code, since that is where the matching vfree is, please > let me know how you want to proceed with this. > >> BTW, it would be very nice to have a straightforward way >> (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares. > > That is an interesting idea, we could add a subsys_init call or > some such to add this to debugfs (efi_check_for_embedded_firmwares runs > too early). > > But given how long this patch-set has been in the making I would really > like to get this (or at least the first 2 patches as a start) upstream, > so for now I would like to keep the changes to a minimum. Are you ok > with me adding the debugfs support with a follow-up patch ? > > Let me also reply to your summary comment elsewhere in the thread here: > > > Early in boot, you check a bunch of firmware descriptors, bundled with > > drivers, to search EFI code and data for firmware before you free said > > code and data. You catalogue it by name. Later on, you use this list > > as a fallback, again catalogued by name. I think it would be rather > > nicer if you simply had a list in a single file containing all these > > descriptors rather than commingling it with the driver's internal dmi > > data. This gets rid of all the ifdeffery and module issues. It also > > avoids any potential nastiness when you have a dmi entry that contains > > driver_data that points into the driver when the driver is a module. > > > > And you can mark the entire list __initdata. And you can easily (now > > or later on) invert the code flow so you map each EFI region exactly > > once and then search for all the firmware in it. > > I believe we have mostly discussed this above. At least for the > X86 touchscreen case, which is the only user of this for now, I > believe that re-using the table from drivers/platform/x86/touchscreen_dmi.c > is better as it avoids duplication of DMI strings and it keeps all > the info in one place (also avoiding churn in 2 files / 2 different > trees when new models are added). > > I agree that when looking at this as a generic mechanism which may be > used elsewhere, your suggestion makes a lot of sense. But even though > this is very much written so that it can be used as a generic mechanism > I'm not sure if other users will appear. So for now, with only the X86 > touchscreen use-case actually using this I believe the current > implementation is best, but I'm definitely open to changing this around > if more users show up. > > Regards, > > Hans >
Hi Andy, On 17-01-2020 21:06, Andy Lutomirski wrote: >> On Jan 14, 2020, at 4:25 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Andy, >> > >> Even if we get more use of this the chances of 1 device model using >> more then 1 embedded fw are small. Where as if we first map the >> EFI_BOOT_SERVICES_CODE segment before doing the DMI match then we >> do this for all EFI_BOOT_SERVICES_CODE segments on all hardware. >> The current implementation is very much optimized to do as little >> work as possible when there is no DMI match, which will be true >> on almost all devices out there. > > Fine with me. > > >> If we hit firmware which is not 8 byte aligned, then yes that would be >> a good idea, but for now I feel that that would just cause a slowdown >> in the scanning without any benefits. >> > > It would shorten the code and remove a comment :). Also, a good memmem > implementation should be very fast, potentially faster than your loop. > I suspect the latter is only true in user code where SSE would get > used, but still. > > it would also be unfortunate if some firmware update switches to > 4-byte alignment and touchscreens stop working with no explanation. So I was thinking sure, fine lets replace the loop with memmem, but the kernel has no memmem, it has memscan but that takes an int to search for, and reducing the prefix to an int seems likely to cause more false positives and unnecessary sha256summing. So I believe it is best to keep this as is for now, we can always change the alignment requirement later, now that we use memcmp to check the prefix changing it is just a matter of changing the i += 8 to e.g. i += 4. Regards, Hans >>>> + >>>> + sha256_init(&sctx); >>>> + sha256_update(&sctx, map + i, desc->length); >>>> + sha256_final(&sctx, sha256); >>>> + if (memcmp(sha256, desc->sha256, 32) == 0) >>>> + break; >>>> + } >>>> + if ((i + desc->length) > size) { >>>> + memunmap(map); >>>> + return -ENOENT; >>>> + } >>>> + >>>> + pr_info("Found EFI embedded fw '%s'\n", desc->name); >>>> + >>> It might be nice to also log which EFI section it was in? >> >> The EFI sections do not have names, so all I could is log >> the section number which does not really feel useful? >> >>>> + fw = kmalloc(sizeof(*fw), GFP_KERNEL); >>>> + if (!fw) { >>>> + memunmap(map); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + fw->data = kmemdup(map + i, desc->length, GFP_KERNEL); >>>> + memunmap(map); >>>> + if (!fw->data) { >>>> + kfree(fw); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + fw->name = desc->name; >>>> + fw->length = desc->length; >>>> + list_add(&fw->list, &efi_embedded_fw_list); >>>> + >>> If you actually copy the firmware name instead of just a pointer to >>> it, then you could potentially free the list of EFI firmwares. >> >> If we move to having a separate dmi_system_id table for this then >> that would be true. ATM the dmi_system_id from >> drivers/platform/x86/touchscreen_dmi.c >> is not freed as it is referenced from a bus-notifier. >> >>> Why are you copying the firmware into linear (kmemdup) memory here >> >> The kmemdup is because the EFI_BOOT_SERVICES_CODE section is >> free-ed almost immediately after we run. >> >>> just to copy it to vmalloc space down below... >> >> The vmalloc is because the efi_get_embedded_fw() function is >> a helper for later implementing firmware_Request_platform >> which returns a struct firmware *fw and release_firmware() >> uses vfree() to free the firmware contents. >> >> I guess we could have efi_get_embedded_fw() return an const u8 * >> and have the firmware code do the vmalloc + memcpy but it boils >> down to the same thing. >> >> >>>> + return 0; >>>> +} >>>> + >>>> +void __init efi_check_for_embedded_firmwares(void) >>>> +{ >>>> + const struct efi_embedded_fw_desc *fw_desc; >>>> + const struct dmi_system_id *dmi_id; >>>> + efi_memory_desc_t *md; >>>> + int i, r; >>>> + >>>> + for (i = 0; embedded_fw_table[i]; i++) { >>>> + dmi_id = dmi_first_match(embedded_fw_table[i]); >>>> + if (!dmi_id) >>>> + continue; >>>> + >>>> + fw_desc = dmi_id->driver_data; >>>> + >>>> + /* >>>> + * In some drivers the struct driver_data contains may contain >>>> + * other driver specific data after the fw_desc struct; and >>>> + * the fw_desc struct itself may be empty, skip these. >>>> + */ >>>> + if (!fw_desc->name) >>>> + continue; >>>> + >>>> + for_each_efi_memory_desc(md) { >>>> + if (md->type != EFI_BOOT_SERVICES_CODE) >>>> + continue; >>>> + >>>> + r = efi_check_md_for_embedded_firmware(md, fw_desc); >>>> + if (r == 0) >>>> + break; >>>> + } >>>> + } >>>> + >>>> + checked_for_fw = true; >>>> +} >>> Have you measured how long this takes on a typical system per matching DMI id? >> >> I originally wrote this approx. 18 months ago, it has been on hold for a while >> since it needed a sha256 method which would work before subsys_initcall-s >> run so I couldn't use the standard crypto_alloc_shash() stuff. In the end >> I ended up merging the duplicate purgatory and crypto/sha256_generic.c >> generic C SHA256 implementation into a set of new directly callable lib >> functions in lib/crypto/sha256.c, just so that I could move forward with >> this... >> >> Anyways the reason for this background info is that because this is a while >> ago I do not remember exactly how, but yes I did measure this (but not >> very scientifically) and there was no discernible difference in boot >> to init (from the initrd) with this in place vs without this. >> >>>> + >>>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size) >>>> +{ >>>> + struct efi_embedded_fw *iter, *fw = NULL; >>>> + void *buf = *data; >>>> + >>>> + if (!checked_for_fw) { >>> WARN_ON_ONCE? A stack dump would be quite nice here. >> >> As discussed when this check was added (discussion in v7 of >> the set I guess, check added in v8) we can also hit this without >> it being a bug, e.g. when booted through kexec the whole >> efi_check_for_embedded_firmwares() call we be skipped, hence the >> pr_warn. >> >> >>>> + buf = vmalloc(fw->length); >>>> + if (!buf) >>>> + return -ENOMEM; >>>> + >>>> + memcpy(buf, fw->data, fw->length); >>>> + *size = fw->length; >>>> + *data = buf; >>> See above. What's vmalloc() for? Where's the vfree()? >> >> See above for my answer. I'm fine with moving this into the >> firmware code, since that is where the matching vfree is, please >> let me know how you want to proceed with this. >> >>> BTW, it would be very nice to have a straightforward way >>> (/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares. >> >> That is an interesting idea, we could add a subsys_init call or >> some such to add this to debugfs (efi_check_for_embedded_firmwares runs >> too early). >> >> But given how long this patch-set has been in the making I would really >> like to get this (or at least the first 2 patches as a start) upstream, >> so for now I would like to keep the changes to a minimum. Are you ok >> with me adding the debugfs support with a follow-up patch ? >> >> Let me also reply to your summary comment elsewhere in the thread here: >> >>> Early in boot, you check a bunch of firmware descriptors, bundled with >>> drivers, to search EFI code and data for firmware before you free said >>> code and data. You catalogue it by name. Later on, you use this list >>> as a fallback, again catalogued by name. I think it would be rather >>> nicer if you simply had a list in a single file containing all these >>> descriptors rather than commingling it with the driver's internal dmi >>> data. This gets rid of all the ifdeffery and module issues. It also >>> avoids any potential nastiness when you have a dmi entry that contains >>> driver_data that points into the driver when the driver is a module. >>> >>> And you can mark the entire list __initdata. And you can easily (now >>> or later on) invert the code flow so you map each EFI region exactly >>> once and then search for all the firmware in it. >> >> I believe we have mostly discussed this above. At least for the >> X86 touchscreen case, which is the only user of this for now, I >> believe that re-using the table from drivers/platform/x86/touchscreen_dmi.c >> is better as it avoids duplication of DMI strings and it keeps all >> the info in one place (also avoiding churn in 2 files / 2 different >> trees when new models are added). >> >> I agree that when looking at this as a generic mechanism which may be >> used elsewhere, your suggestion makes a lot of sense. But even though >> this is very much written so that it can be used as a generic mechanism >> I'm not sure if other users will appear. So for now, with only the X86 >> touchscreen use-case actually using this I believe the current >> implementation is best, but I'm definitely open to changing this around >> if more users show up. >> >> Regards, >> >> Hans >> >
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 51ee2e2a18d6..cc10a925d9e7 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -944,6 +944,7 @@ static void __init __efi_enter_virtual_mode(void) goto err; } + efi_check_for_embedded_firmwares(); efi_free_boot_services(); /* diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index ecc83e2f032c..613828d3f106 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -239,6 +239,11 @@ config EFI_DISABLE_PCI_DMA endmenu +config EFI_EMBEDDED_FIRMWARE + bool + depends on EFI + select CRYPTO_LIB_SHA256 + config UEFI_CPER bool diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile index 554d795270d9..256d6121b2b3 100644 --- a/drivers/firmware/efi/Makefile +++ b/drivers/firmware/efi/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_EFI_TEST) += test/ obj-$(CONFIG_EFI_DEV_PATH_PARSER) += dev-path-parser.o obj-$(CONFIG_APPLE_PROPERTIES) += apple-properties.o obj-$(CONFIG_EFI_RCI2_TABLE) += rci2-table.o +obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += embedded-firmware.o fake_map-y += fake_mem.o fake_map-$(CONFIG_X86) += x86_fake_mem.o diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c new file mode 100644 index 000000000000..6668ad48133f --- /dev/null +++ b/drivers/firmware/efi/embedded-firmware.c @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Support for extracting embedded firmware for peripherals from EFI code, + * + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com> + */ + +#include <linux/dmi.h> +#include <linux/efi.h> +#include <linux/efi_embedded_fw.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/vmalloc.h> +#include <crypto/sha.h> + +/* Exported for use by lib/test_firmware.c only */ +LIST_HEAD(efi_embedded_fw_list); +EXPORT_SYMBOL_GPL(efi_embedded_fw_list); + +static bool checked_for_fw; + +static const struct dmi_system_id * const embedded_fw_table[] = { + NULL +}; + +/* + * Note the efi_check_for_embedded_firmwares() code currently makes the + * following 2 assumptions. This may needs to be revisited if embedded firmware + * is found where this is not true: + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments + * 2) The firmware always starts at an offset which is a multiple of 8 bytes + */ +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 efi_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); + if (!map) { + pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr); + return -ENOMEM; + } + + for (i = 0; (i + desc->length) <= size; i += 8) { + u64 *mem = map + i; + + if (*mem != prefix) + continue; + + sha256_init(&sctx); + sha256_update(&sctx, map + i, desc->length); + sha256_final(&sctx, sha256); + if (memcmp(sha256, desc->sha256, 32) == 0) + break; + } + if ((i + desc->length) > size) { + memunmap(map); + return -ENOENT; + } + + pr_info("Found EFI embedded fw '%s'\n", desc->name); + + fw = kmalloc(sizeof(*fw), GFP_KERNEL); + if (!fw) { + memunmap(map); + return -ENOMEM; + } + + fw->data = kmemdup(map + i, desc->length, GFP_KERNEL); + memunmap(map); + if (!fw->data) { + kfree(fw); + return -ENOMEM; + } + + fw->name = desc->name; + fw->length = desc->length; + list_add(&fw->list, &efi_embedded_fw_list); + + return 0; +} + +void __init efi_check_for_embedded_firmwares(void) +{ + const struct efi_embedded_fw_desc *fw_desc; + const struct dmi_system_id *dmi_id; + efi_memory_desc_t *md; + int i, r; + + for (i = 0; embedded_fw_table[i]; i++) { + dmi_id = dmi_first_match(embedded_fw_table[i]); + if (!dmi_id) + continue; + + fw_desc = dmi_id->driver_data; + + /* + * In some drivers the struct driver_data contains may contain + * other driver specific data after the fw_desc struct; and + * the fw_desc struct itself may be empty, skip these. + */ + if (!fw_desc->name) + continue; + + for_each_efi_memory_desc(md) { + if (md->type != EFI_BOOT_SERVICES_CODE) + continue; + + r = efi_check_md_for_embedded_firmware(md, fw_desc); + if (r == 0) + break; + } + } + + checked_for_fw = true; +} + +int efi_get_embedded_fw(const char *name, void **data, size_t *size) +{ + struct efi_embedded_fw *iter, *fw = NULL; + void *buf = *data; + + if (!checked_for_fw) { + pr_warn("Warning %s called while we did not check for embedded fw\n", + __func__); + return -ENOENT; + } + + list_for_each_entry(iter, &efi_embedded_fw_list, list) { + if (strcmp(name, iter->name) == 0) { + fw = iter; + break; + } + } + + if (!fw) + return -ENOENT; + + buf = vmalloc(fw->length); + if (!buf) + return -ENOMEM; + + memcpy(buf, fw->data, fw->length); + *size = fw->length; + *data = buf; + + return 0; +} +EXPORT_SYMBOL_GPL(efi_get_embedded_fw); diff --git a/include/linux/efi.h b/include/linux/efi.h index a6e1a2d8511e..23392b88bcc0 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1554,6 +1554,12 @@ static inline void efi_enable_reset_attack_mitigation(void) { } #endif +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE +void efi_check_for_embedded_firmwares(void); +#else +static inline void efi_check_for_embedded_firmwares(void) { } +#endif + efi_status_t efi_random_get_seed(void); void efi_retrieve_tpm2_eventlog(void); diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h new file mode 100644 index 000000000000..5a8ae662911b --- /dev/null +++ b/include/linux/efi_embedded_fw.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_EFI_EMBEDDED_FW_H +#define _LINUX_EFI_EMBEDDED_FW_H + +#include <linux/list.h> +#include <linux/mod_devicetable.h> + +/* + * This struct and efi_embedded_fw_list are private to the efi-embedded fw + * implementation they are in this header for use by lib/test_firmware.c only! + */ +struct efi_embedded_fw { + struct list_head list; + const char *name; + void *data; + size_t length; +}; + +extern struct list_head efi_embedded_fw_list; + +/** + * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw + * code to search for embedded firmwares. + * + * @name: Name to register the firmware with if found + * @prefix: First 8 bytes of the firmware + * @length: Length of the firmware in bytes including prefix + * @sha256: SHA256 of the firmware + */ +struct efi_embedded_fw_desc { + const char *name; + u8 prefix[8]; + u32 length; + u8 sha256[32]; +}; + +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz); + +#endif