+
+ 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
