LinuxLists.cc - [PATCH 1/2] efi: Export boot-services code and data as debugfs-blobs

[permalink] [raw]

Subject: [PATCH 2/2] efi: Add embedded peripheral firmware support

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 making this available to peripheral drivers through the
standard firmware loading mechanism.

Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware pretty
late in the init sequence, this is on purpose because the typical
EFI_BOOT_SERVICES_CODE memory-segment is too large for early_memremap().
This means we rely on the EFI_BOOT_SERVICES_CODE not being free-ed until
efi_free_boot_services() is called, which means that this will only work
on x86, if we ever want this on ARM we should make ARM delay the freeing
of the EFI_BOOT_SERVICES_* memory-segments too.

Note this commit also modifies efi_mem_desc_lookup() to not skip
EFI_BOOT_SERVICES_CODE memory-segments, so that efi_mem_reserve() works
on such segments.

Reported-by: Dave Olsthoorn <[email protected]>
Suggested-by: Peter Jones <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/base/firmware_class.c | 29 +++
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/efi.c | 1 +
drivers/firmware/efi/embedded-firmware.c | 232 +++++++++++++++++++++++
include/linux/efi.h | 2 +
init/main.c | 1 +
6 files changed, 266 insertions(+)
create mode 100644 drivers/firmware/efi/embedded-firmware.c

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7dd36ace6152..b1e7b3de1975 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -12,6 +12,7 @@

#include <linux/capability.h>
#include <linux/device.h>
+#include <linux/efi.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/timer.h>
@@ -1207,6 +1208,32 @@ static inline void unregister_sysfs_loader(void)

#endif /* CONFIG_FW_LOADER_USER_HELPER */

+#ifdef CONFIG_EFI
+static int
+fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
+{
+ size_t size;
+ int rc;
+
+ rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size,
+ fw_priv->data ? fw_priv->allocated_size : 0);
+ if (rc == 0) {
+ dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
+ fw_priv->size = size;
+ fw_state_done(fw_priv);
+ ret = 0;
+ }
+
+ return ret;
+}
+#else
+static inline int
+fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
+{
+ return ret;
+}
+#endif
+
/* prepare firmware and firmware_buf structs;
* return 0 if a firmware is already assigned, 1 if need to load one,
* or a negative error code
@@ -1296,6 +1323,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
goto out;

ret = fw_get_filesystem_firmware(device, fw->priv);
+ if (ret)
+ ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
if (ret) {
if (!(opt_flags & FW_OPT_NO_WARN))
dev_warn(device,
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index cb805374f4bc..cb946f7d0181 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -13,6 +13,7 @@ KASAN_SANITIZE_runtime-wrappers.o := n
obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o
obj-$(CONFIG_EFI) += capsule.o memmap.o
+obj-$(CONFIG_EFI) += embedded-firmware.o
obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_EFI_ESRT) += esrt.o
obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index fddc5f706fd2..1a5ea950f58f 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -455,6 +455,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
u64 end;

if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
+ md->type != EFI_BOOT_SERVICES_CODE &&
md->type != EFI_BOOT_SERVICES_DATA &&
md->type != EFI_RUNTIME_SERVICES_DATA) {
continue;
diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
new file mode 100644
index 000000000000..80848f332b22
--- /dev/null
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for extracting embedded firmware for peripherals from EFI code,
+ *
+ * Copyright (c) 2018 Hans de Goede <[email protected]>
+ */
+
+#include <linux/crc32.h>
+#include <linux/dmi.h>
+#include <linux/efi.h>
+#include <linux/types.h>
+
+/* Sofar there are no machines with more then 1 interesting embedded firmware */
+#define MAX_EMBEDDED_FIRMWARES 1
+
+struct embedded_fw_desc {
+ const char *name;
+ u8 prefix[8];
+ u32 length;
+ u32 crc;
+};
+
+struct embedded_fw {
+ const char *name;
+ void *data;
+ size_t length;
+};
+
+static struct embedded_fw found_fw[MAX_EMBEDDED_FIRMWARES];
+static int found_fw_count;
+
+static struct embedded_fw_desc chuwi_vi8_plus_fw[] __initdata = {
+ {
+ .name = "chipone/icn8318-HAMP0002.fw",
+ .prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
+ .length = 35012,
+ .crc = 0x74dfd3fc,
+ },
+ {}
+};
+
+static struct embedded_fw_desc chuwi_hi8_pro_fw[] __initdata = {
+ {
+ .name = "silead/gsl3680-chuwi-hi8-pro.fw",
+ .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+ .length = 39864,
+ .crc = 0xfe2bedba,
+ },
+ {}
+};
+
+static struct embedded_fw_desc cube_iwork8_air_fw[] __initdata = {
+ {
+ .name = "silead/gsl3670-cube-iwork8-air.fw",
+ .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+ .length = 38808,
+ .crc = 0xfecde51f,
+ },
+ {}
+};
+
+static struct embedded_fw_desc pipo_w2s_fw[] __initdata = {
+ {
+ .name = "silead/gsl1680-pipo-w2s.fw",
+ .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+ .length = 39072,
+ .crc = 0x28d5dc6c,
+ },
+ {}
+};
+
+static struct dmi_system_id embedded_fw_table[] __initdata = {
+ {
+ /* Chuwi Vi8 Plus (CWI506) */
+ .driver_data = (void *)chuwi_vi8_plus_fw,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
+ DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+ },
+ },
+ {
+ /* Chuwi Hi8 Pro (CWI513) */
+ .driver_data = (void *)chuwi_hi8_pro_fw,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "X1D3_C806N"),
+ },
+ },
+ {
+ /* Cube iWork8 Air */
+ .driver_data = (void *)cube_iwork8_air_fw,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "cube"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "i1-TF"),
+ DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+ },
+ },
+ {
+ /* Pipo W2s */
+ .driver_data = (void *)pipo_w2s_fw,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "PIPO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "W2S"),
+ },
+ },
+ {}
+};
+
+/*
+ * 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 embedded_fw_desc *desc)
+{
+ u64 i, size;
+ u32 crc;
+ u8 *mem;
+
+ size = md->num_pages << EFI_PAGE_SHIFT;
+ mem = memremap(md->phys_addr, size, MEMREMAP_WB);
+ if (!mem) {
+ pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
+ return -ENOMEM;
+ }
+
+ size -= desc->length;
+ for (i = 0; i < size; i += 8) {
+ if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
+ continue;
+
+ /* Seed with ~0, invert to match crc32 userspace utility */
+ crc = ~crc32(~0, mem + i, desc->length);
+ if (crc == desc->crc)
+ break;
+ }
+
+ memunmap(mem);
+
+ if (i >= size)
+ return -ENOENT;
+
+ pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
+
+ if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
+ pr_err("Error already have %d embedded firmwares\n",
+ MAX_EMBEDDED_FIRMWARES);
+ return -ENOSPC;
+ }
+
+ found_fw[found_fw_count].data =
+ memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
+ if (!found_fw[found_fw_count].data) {
+ pr_err("Error mapping embedded firmware\n");
+ return -ENOMEM;
+ }
+
+ found_fw[found_fw_count].name = desc->name;
+ found_fw[found_fw_count].length = desc->length;
+ found_fw_count++;
+
+ /* Note md points to *unmapped* memory after this!!! */
+ efi_mem_reserve(md->phys_addr + i, desc->length);
+ return 0;
+}
+
+void __init efi_check_for_embedded_firmwares(void)
+{
+ const struct embedded_fw_desc *fw_desc;
+ const struct dmi_system_id *dmi_id;
+ efi_memory_desc_t *md;
+ int i, r;
+
+ dmi_id = dmi_first_match(embedded_fw_table);
+ if (!dmi_id)
+ return;
+
+ fw_desc = dmi_id->driver_data;
+ for (i = 0; fw_desc[i].length; i++) {
+ for_each_efi_memory_desc(md) {
+ if (md->type != EFI_BOOT_SERVICES_CODE)
+ continue;
+
+ r = efi_check_md_for_embedded_firmware(md, &fw_desc[i]);
+ if (r == 0) {
+ /*
+ * On success efi_mem_reserve() has been called
+ * installing a new memmap, so our pointers
+ * are invalid now and we MUST break the loop.
+ */
+ break;
+ }
+ }
+ }
+}
+
+int efi_get_embedded_fw(const char *name, void **data, size_t *size,
+ size_t msize)
+{
+ struct embedded_fw *fw = NULL;
+ void *buf = *data;
+ int i;
+
+ for (i = 0; i < found_fw_count; i++) {
+ if (strcmp(name, found_fw[i].name) == 0) {
+ fw = &found_fw[i];
+ break;
+ }
+ }
+
+ if (!fw)
+ return -ENOENT;
+
+ if (msize && msize < fw->length)
+ return -EFBIG;
+
+ if (!buf) {
+ buf = vmalloc(fw->length);
+ if (!buf)
+ return -ENOMEM;
+ }
+
+ memcpy(buf, fw->data, fw->length);
+ *size = fw->length;
+ *data = buf;
+
+ return 0;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f5083aa72eae..bbdfda1d9e8d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1573,6 +1573,8 @@ efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
#endif

void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
+void efi_check_for_embedded_firmwares(void);
+int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);

/*
* Arch code can implement the following three template macros, avoiding
diff --git a/init/main.c b/init/main.c
index 969eaf140ef0..79b4a1b12509 100644
--- a/init/main.c
+++ b/init/main.c
@@ -710,6 +710,7 @@ asmlinkage __visible void __init start_kernel(void)
sfi_init_late();

if (efi_enabled(EFI_RUNTIME_SERVICES)) {
+ efi_check_for_embedded_firmwares();
efi_free_boot_services();
}

--
2.17.0.rc2

2018-03-31 14:11:55

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 1/2] efi: Export boot-services code and data as debugfs-blobs

On Sat, Mar 31, 2018 at 02:19:43PM +0200, Hans de Goede wrote:
> Sometimes it is useful to be able to dump the efi boot-services code and
> data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi,
> but only if efi=debug is passed on the kernel-commandline as this requires
> not freeing those memory-regions, which costs 20+ MB of RAM.
>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> arch/x86/platform/efi/quirks.c | 4 +++
> drivers/firmware/efi/efi.c | 57 ++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 5b513ccffde4..0f968c7bcfec 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -374,6 +374,10 @@ void __init efi_free_boot_services(void)
> int num_entries = 0;
> void *new, *new_md;
>
> + /* Keep all regions for /sys/kernel/debug/efi */
> + if (efi_enabled(EFI_DBG))
> + return;
> +
> for_each_efi_memory_desc(md) {
> unsigned long long start = md->phys_addr;
> unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index cd42f66a7c85..fddc5f706fd2 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -18,6 +18,7 @@
> #include <linux/kobject.h>
> #include <linux/module.h>
> #include <linux/init.h>
> +#include <linux/debugfs.h>
> #include <linux/device.h>
> #include <linux/efi.h>
> #include <linux/of.h>
> @@ -316,6 +317,59 @@ static __init int efivar_ssdt_load(void)
> static inline int efivar_ssdt_load(void) { return 0; }
> #endif
>
> +#ifdef CONFIG_DEBUG_FS
> +
> +#define EFI_DEBUGFS_MAX_BLOBS 32
> +
> +struct debugfs_blob_wrapper debugfs_blob[EFI_DEBUGFS_MAX_BLOBS];
> +
> +static void __init efi_debugfs_init(void)
> +{
> + struct dentry *efi_debugfs;
> + efi_memory_desc_t *md;
> + char name[32];
> + int type_count[EFI_BOOT_SERVICES_DATA + 1] = {};
> + int i = 0;
> +
> + efi_debugfs = debugfs_create_dir("efi", NULL);
> + if (IS_ERR_OR_NULL(efi_debugfs)) {
> + pr_warn("Could not create efi debugfs entry\n");
> + return;
> + }

{sigh}

No, don't warn, or complain, or do anything else if a debugfs call
fails. Just keep on moving, you can always use the return value
properly in any future call if you need it, and no code flow should ever
care if a debugfs call succeeded or failed.

> /*
> * We register the efi subsystem with the firmware subsystem and the
> * efivars subsystem with the efi subsystem, if the system was booted with
> @@ -360,6 +414,9 @@ static int __init efisubsys_init(void)
> goto err_remove_group;
> }
>
> + if (efi_enabled(EFI_DBG))
> + efi_debugfs_init();

You never remove the directory?

thanks,

greg k-h

2018-03-31 16:59:51

[permalink] [raw]

Subject: Re: [PATCH 1/2] efi: Export boot-services code and data as debugfs-blobs

Hi,

On 03/31/2018 04:10 PM, Greg Kroah-Hartman wrote:
> On Sat, Mar 31, 2018 at 02:19:43PM +0200, Hans de Goede wrote:
>> Sometimes it is useful to be able to dump the efi boot-services code and
>> data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi,
>> but only if efi=debug is passed on the kernel-commandline as this requires
>> not freeing those memory-regions, which costs 20+ MB of RAM.
>>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> arch/x86/platform/efi/quirks.c | 4 +++
>> drivers/firmware/efi/efi.c | 57 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 61 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index 5b513ccffde4..0f968c7bcfec 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -374,6 +374,10 @@ void __init efi_free_boot_services(void)
>> int num_entries = 0;
>> void *new, *new_md;
>>
>> + /* Keep all regions for /sys/kernel/debug/efi */
>> + if (efi_enabled(EFI_DBG))
>> + return;
>> +
>> for_each_efi_memory_desc(md) {
>> unsigned long long start = md->phys_addr;
>> unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index cd42f66a7c85..fddc5f706fd2 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -18,6 +18,7 @@
>> #include <linux/kobject.h>
>> #include <linux/module.h>
>> #include <linux/init.h>
>> +#include <linux/debugfs.h>
>> #include <linux/device.h>
>> #include <linux/efi.h>
>> #include <linux/of.h>
>> @@ -316,6 +317,59 @@ static __init int efivar_ssdt_load(void)
>> static inline int efivar_ssdt_load(void) { return 0; }
>> #endif
>>
>> +#ifdef CONFIG_DEBUG_FS
>> +
>> +#define EFI_DEBUGFS_MAX_BLOBS 32
>> +
>> +struct debugfs_blob_wrapper debugfs_blob[EFI_DEBUGFS_MAX_BLOBS];
>> +
>> +static void __init efi_debugfs_init(void)
>> +{
>> + struct dentry *efi_debugfs;
>> + efi_memory_desc_t *md;
>> + char name[32];
>> + int type_count[EFI_BOOT_SERVICES_DATA + 1] = {};
>> + int i = 0;
>> +
>> + efi_debugfs = debugfs_create_dir("efi", NULL);
>> + if (IS_ERR_OR_NULL(efi_debugfs)) {
>> + pr_warn("Could not create efi debugfs entry\n");
>> + return;
>> + }
>
> {sigh}
>
> No, don't warn, or complain, or do anything else if a debugfs call
> fails. Just keep on moving, you can always use the return value
> properly in any future call if you need it, and no code flow should ever
> care if a debugfs call succeeded or failed.

Ok.

>> /*
>> * We register the efi subsystem with the firmware subsystem and the
>> * efivars subsystem with the efi subsystem, if the system was booted with
>> @@ -360,6 +414,9 @@ static int __init efisubsys_init(void)
>> goto err_remove_group;
>> }
>>
>> + if (efi_enabled(EFI_DBG))
>> + efi_debugfs_init();
>
> You never remove the directory?

Correct, this is happening from a subsys_initcall as such there is no
efi_cleanup() counterpart.

Note the "if (efi_enabled(EFI_DBG))" check checks for efi=debug on the
kernel cmdline, so this only happens if the user asked for it.

Regards,

Hans

2018-04-01 00:15:58

[permalink] [raw]

Subject: Re: [PATCH 1/2] efi: Export boot-services code and data as debugfs-blobs

Hi Hans,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.16-rc7 next-20180329]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/efi-Export-boot-services-code-and-data-as-debugfs-blobs/20180401-062627
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

>> drivers/firmware/efi/efi.c:324:29: sparse: symbol 'debugfs_blob' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

2018-04-01 00:15:58

[permalink] [raw]

Subject: [RFC PATCH] efi: debugfs_blob[] can be static

Fixes: ac46fdb7891f ("efi: Export boot-services code and data as debugfs-blobs")
Signed-off-by: Fengguang Wu <[email protected]>
---
efi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index fddc5f7..9f1b8e6 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -321,7 +321,7 @@ static inline int efivar_ssdt_load(void) { return 0; }

#define EFI_DEBUGFS_MAX_BLOBS 32

-struct debugfs_blob_wrapper debugfs_blob[EFI_DEBUGFS_MAX_BLOBS];
+static struct debugfs_blob_wrapper debugfs_blob[EFI_DEBUGFS_MAX_BLOBS];

static void __init efi_debugfs_init(void)
{

2018-04-01 00:22:03

[permalink] [raw]

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

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc7]
[cannot apply to next-20180329]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/efi-Export-boot-services-code-and-data-as-debugfs-blobs/20180401-062627
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64

All error/warnings (new ones prefixed by >>):

drivers/firmware/efi/embedded-firmware.c: In function 'efi_check_md_for_embedded_firmware':
>> drivers/firmware/efi/embedded-firmware.c:125:8: error: implicit declaration of function 'memremap'; did you mean 'memset_p'? [-Werror=implicit-function-declaration]
mem = memremap(md->phys_addr, size, MEMREMAP_WB);
^~~~~~~~
memset_p
>> drivers/firmware/efi/embedded-firmware.c:125:38: error: 'MEMREMAP_WB' undeclared (first use in this function)
mem = memremap(md->phys_addr, size, MEMREMAP_WB);
^~~~~~~~~~~
drivers/firmware/efi/embedded-firmware.c:125:38: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/firmware/efi/embedded-firmware.c:142:2: error: implicit declaration of function 'memunmap'; did you mean 'memcmp'? [-Werror=implicit-function-declaration]
memunmap(mem);
^~~~~~~~
memcmp
drivers/firmware/efi/embedded-firmware.c: In function 'efi_get_embedded_fw':
>> drivers/firmware/efi/embedded-firmware.c:222:9: error: implicit declaration of function 'vmalloc'; did you mean 'd_alloc'? [-Werror=implicit-function-declaration]
buf = vmalloc(fw->length);
^~~~~~~
d_alloc
>> drivers/firmware/efi/embedded-firmware.c:222:7: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
buf = vmalloc(fw->length);
^
cc1: some warnings being treated as errors

vim +125 drivers/firmware/efi/embedded-firmware.c

109
110 /*
111 * Note the efi_check_for_embedded_firmwares() code currently makes the
112 * following 2 assumptions. This may needs to be revisited if embedded firmware
113 * is found where this is not true:
114 * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
115 * 2) The firmware always starts at an offset which is a multiple of 8 bytes
116 */
117 static int __init efi_check_md_for_embedded_firmware(
118 efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
119 {
120 u64 i, size;
121 u32 crc;
122 u8 *mem;
123
124 size = md->num_pages << EFI_PAGE_SHIFT;
> 125 mem = memremap(md->phys_addr, size, MEMREMAP_WB);
126 if (!mem) {
127 pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
128 return -ENOMEM;
129 }
130
131 size -= desc->length;
132 for (i = 0; i < size; i += 8) {
133 if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
134 continue;
135
136 /* Seed with ~0, invert to match crc32 userspace utility */
137 crc = ~crc32(~0, mem + i, desc->length);
138 if (crc == desc->crc)
139 break;
140 }
141
> 142 memunmap(mem);
143
144 if (i >= size)
145 return -ENOENT;
146
147 pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
148
149 if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
150 pr_err("Error already have %d embedded firmwares\n",
151 MAX_EMBEDDED_FIRMWARES);
152 return -ENOSPC;
153 }
154
155 found_fw[found_fw_count].data =
156 memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
157 if (!found_fw[found_fw_count].data) {
158 pr_err("Error mapping embedded firmware\n");
159 return -ENOMEM;
160 }
161
162 found_fw[found_fw_count].name = desc->name;
163 found_fw[found_fw_count].length = desc->length;
164 found_fw_count++;
165
166 /* Note md points to *unmapped* memory after this!!! */
167 efi_mem_reserve(md->phys_addr + i, desc->length);
168 return 0;
169 }
170
171 void __init efi_check_for_embedded_firmwares(void)
172 {
173 const struct embedded_fw_desc *fw_desc;
174 const struct dmi_system_id *dmi_id;
175 efi_memory_desc_t *md;
176 int i, r;
177
178 dmi_id = dmi_first_match(embedded_fw_table);
179 if (!dmi_id)
180 return;
181
182 fw_desc = dmi_id->driver_data;
183 for (i = 0; fw_desc[i].length; i++) {
184 for_each_efi_memory_desc(md) {
185 if (md->type != EFI_BOOT_SERVICES_CODE)
186 continue;
187
188 r = efi_check_md_for_embedded_firmware(md, &fw_desc[i]);
189 if (r == 0) {
190 /*
191 * On success efi_mem_reserve() has been called
192 * installing a new memmap, so our pointers
193 * are invalid now and we MUST break the loop.
194 */
195 break;
196 }
197 }
198 }
199 }
200
201 int efi_get_embedded_fw(const char *name, void **data, size_t *size,
202 size_t msize)
203 {
204 struct embedded_fw *fw = NULL;
205 void *buf = *data;
206 int i;
207
208 for (i = 0; i < found_fw_count; i++) {
209 if (strcmp(name, found_fw[i].name) == 0) {
210 fw = &found_fw[i];
211 break;
212 }
213 }
214
215 if (!fw)
216 return -ENOENT;
217
218 if (msize && msize < fw->length)
219 return -EFBIG;
220
221 if (!buf) {
> 222 buf = vmalloc(fw->length);

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (6.03 kB)
.config.gz (36.95 kB)
Download all attachments

2018-04-01 00:23:51

[permalink] [raw]

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

Attachments:

(No filename) (1.09 kB)
.config.gz (5.80 kB)
Download all attachments

2018-04-02 23:25:16

[permalink] [raw]

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

On Sat, Mar 31, 2018 at 02:19:44PM +0200, Hans de Goede 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.

Some devices have OTP and use this sort of calibration data, I was unaware of
the use of EFI to stash firmware. Good to know, but can you also provide
references to what part of what standard should be followed for it in
documentation?

> 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 making this available to peripheral drivers through the
> standard firmware loading mechanism.

Neat.

> Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware pretty
> late in the init sequence,

This also creates a technical limitation on use for the API that users
should be aware of. Its important to document such limitation.

Also if we can address the limitation that would be even better.

For instance, on what part of the driver is the call to request firmware
being made? Note that we support async probe now, so if the call was done
on probe, it may be wise to use async probe, however, can we be *certain*
that the EFI firmware would have been parsed and ready by then? Please
check. It just may be the case.

Or, if we use late_initcall() would that suffice on the driver, if they
used a request firmware call on init or probe?

> this is on purpose because the typical
> EFI_BOOT_SERVICES_CODE memory-segment is too large for early_memremap().

To be clear you neede to use memremap()

What mechanism would have in place to ensure that a driver which expects
firmware to be on EFI data to be already available prior to its driver's
call to initialize?

You seem to say its this consumes about about 25 MiB now, and for now you
have made this a debug thing only? How have these size requirements changed
over time? Has EFI_BOOT_SERVICES_CODE grown over time? How much? Do we
expect it will blow up later?

> This means we rely on the EFI_BOOT_SERVICES_CODE not being free-ed until
> efi_free_boot_services() is called, which means that this will only work
> on x86, if we ever want this on ARM we should make ARM delay the freeing
> of the EFI_BOOT_SERVICES_* memory-segments too.

Why not do that as well with your patch?

> Note this commit also modifies efi_mem_desc_lookup() to not skip
> EFI_BOOT_SERVICES_CODE memory-segments, so that efi_mem_reserve() works
> on such segments.
>
> Reported-by: Dave Olsthoorn <[email protected]>
> Suggested-by: Peter Jones <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> drivers/base/firmware_class.c | 29 +++
> drivers/firmware/efi/Makefile | 1 +
> drivers/firmware/efi/efi.c | 1 +
> drivers/firmware/efi/embedded-firmware.c | 232 +++++++++++++++++++++++
> include/linux/efi.h | 2 +
> init/main.c | 1 +
> 6 files changed, 266 insertions(+)
> create mode 100644 drivers/firmware/efi/embedded-firmware.c
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 7dd36ace6152..b1e7b3de1975 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -12,6 +12,7 @@
>
> #include <linux/capability.h>
> #include <linux/device.h>
> +#include <linux/efi.h>
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/timer.h>
> @@ -1207,6 +1208,32 @@ static inline void unregister_sysfs_loader(void)
>
> #endif /* CONFIG_FW_LOADER_USER_HELPER */
>
> +#ifdef CONFIG_EFI
> +static int
> +fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
> +{
> + size_t size;
> + int rc;
> +
> + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size,
> + fw_priv->data ? fw_priv->allocated_size : 0);
> + if (rc == 0) {
> + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> + fw_priv->size = size;
> + fw_state_done(fw_priv);
> + ret = 0;
> + }
> +
> + return ret;
> +}
> +#else
> +static inline int
> +fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
> +{
> + return ret;
> +}
> +#endif
> +
> /* prepare firmware and firmware_buf structs;
> * return 0 if a firmware is already assigned, 1 if need to load one,
> * or a negative error code
> @@ -1296,6 +1323,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> goto out;
>
> ret = fw_get_filesystem_firmware(device, fw->priv);
> + if (ret)
> + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);

This EFI firmware lookup is being used as a fallback mechanism, for *all*
requests. That's pretty aggressive and I'd like a bit more justification
for that approach.

For instance, if its just a few drivers that really can use this, can't we just
add anew API call, say firmware_request_efi(), then add an internal flag for
this type of lookup and then this fallback mechanism would *only* be used for
those drivers.

BTW please use linux-next to base your changes as a lot of things have changed
on the firmware API code, on queue on its way for v4.17-rc1. Please be sure
to also extend the documentation on Documentation/driver-api/firmware/
respectively.

> if (ret) {
> if (!(opt_flags & FW_OPT_NO_WARN))
> dev_warn(device,
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index cb805374f4bc..cb946f7d0181 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -13,6 +13,7 @@ KASAN_SANITIZE_runtime-wrappers.o := n
> obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
> obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o
> obj-$(CONFIG_EFI) += capsule.o memmap.o
> +obj-$(CONFIG_EFI) += embedded-firmware.o
> obj-$(CONFIG_EFI_VARS) += efivars.o
> obj-$(CONFIG_EFI_ESRT) += esrt.o
> obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fddc5f706fd2..1a5ea950f58f 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -455,6 +455,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> u64 end;
>
> if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> + md->type != EFI_BOOT_SERVICES_CODE &&
> md->type != EFI_BOOT_SERVICES_DATA &&
> md->type != EFI_RUNTIME_SERVICES_DATA) {
> continue;
> diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
> new file mode 100644
> index 000000000000..80848f332b22
> --- /dev/null
> +++ b/drivers/firmware/efi/embedded-firmware.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for extracting embedded firmware for peripherals from EFI code,
> + *
> + * Copyright (c) 2018 Hans de Goede <[email protected]>
> + */
> +
> +#include <linux/crc32.h>
> +#include <linux/dmi.h>
> +#include <linux/efi.h>
> +#include <linux/types.h>
> +
> +/* Sofar there are no machines with more then 1 interesting embedded firmware */
> +#define MAX_EMBEDDED_FIRMWARES 1
> +
> +struct embedded_fw_desc {
> + const char *name;
> + u8 prefix[8];
> + u32 length;
> + u32 crc;
> +};
> +
> +struct embedded_fw {
> + const char *name;
> + void *data;
> + size_t length;
> +};
> +
> +static struct embedded_fw found_fw[MAX_EMBEDDED_FIRMWARES];

This is just saving a few bytes, and is still pretty inflexible.
If were going to support this, this is a rather inflexible way to
support this. I'd prefer we link list this. This way if we support,
its an empty list and grows depending on what we find.

I don't see the benefit of a static array here in any way.

> +static int found_fw_count;
> +
> +static struct embedded_fw_desc chuwi_vi8_plus_fw[] __initdata = {
> + {
> + .name = "chipone/icn8318-HAMP0002.fw",
> + .prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
> + .length = 35012,
> + .crc = 0x74dfd3fc,
> + },
> + {}
> +};
> +
> +static struct embedded_fw_desc chuwi_hi8_pro_fw[] __initdata = {
> + {
> + .name = "silead/gsl3680-chuwi-hi8-pro.fw",
> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
> + .length = 39864,
> + .crc = 0xfe2bedba,
> + },
> + {}
> +};
> +
> +static struct embedded_fw_desc cube_iwork8_air_fw[] __initdata = {
> + {
> + .name = "silead/gsl3670-cube-iwork8-air.fw",
> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
> + .length = 38808,
> + .crc = 0xfecde51f,
> + },
> + {}
> +};
> +
> +static struct embedded_fw_desc pipo_w2s_fw[] __initdata = {
> + {
> + .name = "silead/gsl1680-pipo-w2s.fw",
> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
> + .length = 39072,
> + .crc = 0x28d5dc6c,
> + },
> + {}
> +};
> +
> +static struct dmi_system_id embedded_fw_table[] __initdata = {
> + {
> + /* Chuwi Vi8 Plus (CWI506) */
> + .driver_data = (void *)chuwi_vi8_plus_fw,
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
> + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
> + },
> + },
> + {
> + /* Chuwi Hi8 Pro (CWI513) */
> + .driver_data = (void *)chuwi_hi8_pro_fw,
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "X1D3_C806N"),
> + },
> + },
> + {
> + /* Cube iWork8 Air */
> + .driver_data = (void *)cube_iwork8_air_fw,
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "cube"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "i1-TF"),
> + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
> + },
> + },
> + {
> + /* Pipo W2s */
> + .driver_data = (void *)pipo_w2s_fw,
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "PIPO"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "W2S"),
> + },
> + },
> + {}
> +};

Maintaining these on a separate file might be easier to maintain.

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

Who's defining this? Is this an agreed upon thing between a few companies, or
is this written as part of a standard which we can refer to in documentation.

> + */
> +static int __init efi_check_md_for_embedded_firmware(
> + efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
> +{
> + u64 i, size;
> + u32 crc;
> + u8 *mem;
> +
> + size = md->num_pages << EFI_PAGE_SHIFT;
> + mem = memremap(md->phys_addr, size, MEMREMAP_WB);
> + if (!mem) {
> + pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
> + return -ENOMEM;
> + }
> +
> + size -= desc->length;
> + for (i = 0; i < size; i += 8) {
> + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
> + continue;
> +
> + /* Seed with ~0, invert to match crc32 userspace utility */
> + crc = ~crc32(~0, mem + i, desc->length);
> + if (crc == desc->crc)
> + break;
> + }
> +
> + memunmap(mem);
> +
> + if (i >= size)
> + return -ENOENT;
> +
> + pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
> +
> + if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
> + pr_err("Error already have %d embedded firmwares\n",
> + MAX_EMBEDDED_FIRMWARES);
> + return -ENOSPC;
> + }
> +
> + found_fw[found_fw_count].data =
> + memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);

I've heard of some firmware bing over hundreds of MB these days. Once
the can of worms is open its just a matter of time before someone
tries to abuse, so do we have any limitation size? How about spec
wise? Are there any limitations implied by it?

If there are rather small, do we stand to gain instead to just kzalloc()
and memcpy the found firmware? If done this way, wouldn't you be able
to run this earlier?

> + if (!found_fw[found_fw_count].data) {
> + pr_err("Error mapping embedded firmware\n");
> + return -ENOMEM;
> + }
> +
> + found_fw[found_fw_count].name = desc->name;
> + found_fw[found_fw_count].length = desc->length;
> + found_fw_count++;
> +
> + /* Note md points to *unmapped* memory after this!!! */
> + efi_mem_reserve(md->phys_addr + i, desc->length);

Do you need a for_each_efi_memory_desc_safe() perhaps?

> + return 0;
> +}
> +
> +void __init efi_check_for_embedded_firmwares(void)
> +{
> + const struct embedded_fw_desc *fw_desc;
> + const struct dmi_system_id *dmi_id;
> + efi_memory_desc_t *md;
> + int i, r;
> +
> + dmi_id = dmi_first_match(embedded_fw_table);
> + if (!dmi_id)
> + return;
> +
> + fw_desc = dmi_id->driver_data;
> + for (i = 0; fw_desc[i].length; i++) {
> + for_each_efi_memory_desc(md) {
> + if (md->type != EFI_BOOT_SERVICES_CODE)
> + continue;
> +
> + r = efi_check_md_for_embedded_firmware(md, &fw_desc[i]);
> + if (r == 0) {
> + /*
> + * On success efi_mem_reserve() has been called
> + * installing a new memmap, so our pointers
> + * are invalid now and we MUST break the loop.
> + */
> + break;

Yeah this seems fragile. Can we do better?

Luis

> + }
> + }
> + }
> +}
> +
> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
> + size_t msize)
> +{
> + struct embedded_fw *fw = NULL;
> + void *buf = *data;
> + int i;
> +
> + for (i = 0; i < found_fw_count; i++) {
> + if (strcmp(name, found_fw[i].name) == 0) {
> + fw = &found_fw[i];
> + break;
> + }
> + }
> +
> + if (!fw)
> + return -ENOENT;
> +
> + if (msize && msize < fw->length)
> + return -EFBIG;
> +
> + if (!buf) {
> + buf = vmalloc(fw->length);
> + if (!buf)
> + return -ENOMEM;
> + }
> +
> + memcpy(buf, fw->data, fw->length);
> + *size = fw->length;
> + *data = buf;
> +
> + return 0;
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index f5083aa72eae..bbdfda1d9e8d 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1573,6 +1573,8 @@ efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
> #endif
>
> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
> +void efi_check_for_embedded_firmwares(void);
> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
>
> /*
> * Arch code can implement the following three template macros, avoiding
> diff --git a/init/main.c b/init/main.c
> index 969eaf140ef0..79b4a1b12509 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -710,6 +710,7 @@ asmlinkage __visible void __init start_kernel(void)
> sfi_init_late();
>
> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + efi_check_for_embedded_firmwares();
> efi_free_boot_services();
> }
>
> --
> 2.17.0.rc2
>
>

--
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

2018-04-03 08:36:01

[permalink] [raw]

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

Hi Luis,

Thank you for the review.

On 03-04-18 01:23, Luis R. Rodriguez wrote:
> On Sat, Mar 31, 2018 at 02:19:44PM +0200, Hans de Goede 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.
>
> Some devices have OTP and use this sort of calibration data,

Right, I'm not sure it really is OTP and not flash, but many touchscreen
controllers do come with their firmware embedded into the controller, but
not all unfortunately.

> I was unaware of
> the use of EFI to stash firmware. Good to know, but can you also provide
> references to what part of what standard should be followed for it in
> documentation?

This is not part of the standard. There has been a long(ish) standing issue
with us not being able to get re-distribute permission for the firmware for
some touchscreen controllers found on cheap x86 devices. Which means that
we cannot put it in Linux firmware.

Dave Olsthoorn (in the Cc) noticed that the touchscreen did work in the
refind bootload UI, so the EFI code must have a copy of the firmware.

I asked Peter Jones for suggestions how to extract this during boot and
he suggested seeing if there was a copy of the firmware in the
EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.

My patch to add support for this contains a table of device-model (dmi
strings), firmware header (first 64 bits), length and crc32 and then if
we boot on a device-model which is in the table the code scans the
EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
caches the firmware for later use by request-firmware.

So I just do a brute-force search for the firmware, this really is hack,
nothing standard about it I'm afraid. But it works on 4 different x86
tablets I have and makes the touchscreen work OOTB on them, so I believe
it is a worthwhile hack to have.

>> 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 making this available to peripheral drivers through the
>> standard firmware loading mechanism.
>
> Neat.
>
>> Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware pretty
>> late in the init sequence,
>
> This also creates a technical limitation on use for the API that users
> should be aware of. Its important to document such limitation.

I don't think this is a problem for any normal drivers, when I say pretty
late I mean late in init/main.c: start_kernel(), so still before any normal
drivers load.

The first idea was to scan for the firmware at the same time we check for
things as the ACPI BGRT logo stuff, but as mentioned that requires using
early_mmap() which does not work for the amount of memory we want to map.

> Also if we can address the limitation that would be even better.
>
> For instance, on what part of the driver is the call to request firmware
> being made? Note that we support async probe now, so if the call was done
> on probe, it may be wise to use async probe, however, can we be *certain*
> that the EFI firmware would have been parsed and ready by then? Please
> check. It just may be the case.
>
> Or, if we use late_initcall() would that suffice on the driver, if they
> used a request firmware call on init or probe?

As said I think we still do it early enough for any driver use, when
I wrote "late in the init sequence" I should have probably written something
else, like "near the end of start_kernel() instead of from setup_arch()"

>
>> this is on purpose because the typical
>> EFI_BOOT_SERVICES_CODE memory-segment is too large for early_memremap().
>
> To be clear you neede to use memremap()

Yes.

> What mechanism would have in place to ensure that a driver which expects
> firmware to be on EFI data to be already available prior to its driver's
> call to initialize?

See above, this still runs before start_kernel() calls rest_init() which is
where any normal init calls (and driver probing) happens so still early
enough for any users I can think of. I think my poorly worded commit
message is causing a bit of unnecessary confusion here, sorry about that.

> You seem to say its this consumes about about 25 MiB now, and for now you
> have made this a debug thing only? How have these size requirements changed
> over time? Has EFI_BOOT_SERVICES_CODE grown over time? How much? Do we
> expect it will blow up later?

The debug only thing is only patch 1/2, which is mostly independent of this
patch (which is 2/2), patch 1 exports the EFI_BOOT_SERVICES_* memory segments
as blobs under /sys/kernel/debug/efi, which requires not freeing them
(or making a copy) and this costs memory. The purpose of this is to be
able to easily check them for embedded firmwares when adding new entries
to the table of known embedded firmwares used by this patch.

This patch will work fine without the first patch even being present in
the kernel and will also work fine without efi=debug.

>> This means we rely on the EFI_BOOT_SERVICES_CODE not being free-ed until
>> efi_free_boot_services() is called, which means that this will only work
>> on x86, if we ever want this on ARM we should make ARM delay the freeing
>> of the EFI_BOOT_SERVICES_* memory-segments too.
>
> Why not do that as well with your patch?

That requires making significant changes to the early bringup code on
ARM, x86 keeps EFI_BOOT_SERVICES_* memory-segments around until near
the end of start_kernel() because freeing them earlier triggers bugs
in some x86 EFI implementations, ARM EFI implementations do not have
these bugs, so they free them almost directly at boot.

Changing this really falls outside the scope of this patch.

>
>> Note this commit also modifies efi_mem_desc_lookup() to not skip
>> EFI_BOOT_SERVICES_CODE memory-segments, so that efi_mem_reserve() works
>> on such segments.
>>
>> Reported-by: Dave Olsthoorn <[email protected]>
>> Suggested-by: Peter Jones <[email protected]>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> drivers/base/firmware_class.c | 29 +++
>> drivers/firmware/efi/Makefile | 1 +
>> drivers/firmware/efi/efi.c | 1 +
>> drivers/firmware/efi/embedded-firmware.c | 232 +++++++++++++++++++++++
>> include/linux/efi.h | 2 +
>> init/main.c | 1 +
>> 6 files changed, 266 insertions(+)
>> create mode 100644 drivers/firmware/efi/embedded-firmware.c
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 7dd36ace6152..b1e7b3de1975 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -12,6 +12,7 @@
>>
>> #include <linux/capability.h>
>> #include <linux/device.h>
>> +#include <linux/efi.h>
>> #include <linux/module.h>
>> #include <linux/init.h>
>> #include <linux/timer.h>
>> @@ -1207,6 +1208,32 @@ static inline void unregister_sysfs_loader(void)
>>
>> #endif /* CONFIG_FW_LOADER_USER_HELPER */
>>
>> +#ifdef CONFIG_EFI
>> +static int
>> +fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
>> +{
>> + size_t size;
>> + int rc;
>> +
>> + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size,
>> + fw_priv->data ? fw_priv->allocated_size : 0);
>> + if (rc == 0) {
>> + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
>> + fw_priv->size = size;
>> + fw_state_done(fw_priv);
>> + ret = 0;
>> + }
>> +
>> + return ret;
>> +}
>> +#else
>> +static inline int
>> +fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
>> +{
>> + return ret;
>> +}
>> +#endif
>> +
>> /* prepare firmware and firmware_buf structs;
>> * return 0 if a firmware is already assigned, 1 if need to load one,
>> * or a negative error code
>> @@ -1296,6 +1323,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>> goto out;
>>
>> ret = fw_get_filesystem_firmware(device, fw->priv);
>> + if (ret)
>> + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
>
> This EFI firmware lookup is being used as a fallback mechanism, for *all*
> requests. That's pretty aggressive and I'd like a bit more justification
> for that approach.

The fw_get_efi_embedded_fw() call is not that expensive, it walks the
list of found firmwares, does a strcmp on the name and that is all it does,
so I did not really see this as a problem, but if you want me to change this
that is certainly possible.

> For instance, if its just a few drivers that really can use this, can't we just
> add anew API call, say firmware_request_efi(), then add an internal flag for
> this type of lookup and then this fallback mechanism would *only* be used for
> those drivers.

Yes that is certainly possible, currently there are 2 touchscreen drivers which
can use this drivers/input/touchscreen/silead.c and
drivers/input/touchscreen/chipone_icn8505.c, with the latter being a driver I just
finished this weekend and which I will submit upstream soon.

> BTW please use linux-next to base your changes as a lot of things have changed
> on the firmware API code, on queue on its way for v4.17-rc1.

Ok, I usually prefer to only merge the relevant subsys-next into my personal
tree rather then consuming the entirety of -next, which subsys tree has
the firmware bits ?

> Please be sure
> to also extend the documentation on Documentation/driver-api/firmware/
> respectively.

Ok.

>> if (ret) {
>> if (!(opt_flags & FW_OPT_NO_WARN))
>> dev_warn(device,
>> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
>> index cb805374f4bc..cb946f7d0181 100644
>> --- a/drivers/firmware/efi/Makefile
>> +++ b/drivers/firmware/efi/Makefile
>> @@ -13,6 +13,7 @@ KASAN_SANITIZE_runtime-wrappers.o := n
>> obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
>> obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o
>> obj-$(CONFIG_EFI) += capsule.o memmap.o
>> +obj-$(CONFIG_EFI) += embedded-firmware.o
>> obj-$(CONFIG_EFI_VARS) += efivars.o
>> obj-$(CONFIG_EFI_ESRT) += esrt.o
>> obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index fddc5f706fd2..1a5ea950f58f 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -455,6 +455,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>> u64 end;
>>
>> if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
>> + md->type != EFI_BOOT_SERVICES_CODE &&
>> md->type != EFI_BOOT_SERVICES_DATA &&
>> md->type != EFI_RUNTIME_SERVICES_DATA) {
>> continue;
>> diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
>> new file mode 100644
>> index 000000000000..80848f332b22
>> --- /dev/null
>> +++ b/drivers/firmware/efi/embedded-firmware.c
>> @@ -0,0 +1,232 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Support for extracting embedded firmware for peripherals from EFI code,
>> + *
>> + * Copyright (c) 2018 Hans de Goede <[email protected]>
>> + */
>> +
>> +#include <linux/crc32.h>
>> +#include <linux/dmi.h>
>> +#include <linux/efi.h>
>> +#include <linux/types.h>
>> +
>> +/* Sofar there are no machines with more then 1 interesting embedded firmware */
>> +#define MAX_EMBEDDED_FIRMWARES 1
>> +
>> +struct embedded_fw_desc {
>> + const char *name;
>> + u8 prefix[8];
>> + u32 length;
>> + u32 crc;
>> +};
>> +
>> +struct embedded_fw {
>> + const char *name;
>> + void *data;
>> + size_t length;
>> +};
>> +
>> +static struct embedded_fw found_fw[MAX_EMBEDDED_FIRMWARES];
>
> This is just saving a few bytes, and is still pretty inflexible.
> If were going to support this, this is a rather inflexible way to
> support this. I'd prefer we link list this. This way if we support,
> its an empty list and grows depending on what we find.
>
> I don't see the benefit of a static array here in any way.

It is not like we are ever going to have more then 2-3 embedded
firmwares in the foreseeable future and having a static array
saves the need to kmalloc the struct embedded_fw and the additional
error handling for when this fails, so the array leads to simpler
code. But if you really want me to change this over to a linked
list I can change it.

>> +static int found_fw_count;
>> +
>> +static struct embedded_fw_desc chuwi_vi8_plus_fw[] __initdata = {
>> + {
>> + .name = "chipone/icn8318-HAMP0002.fw",
>> + .prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
>> + .length = 35012,
>> + .crc = 0x74dfd3fc,
>> + },
>> + {}
>> +};
>> +
>> +static struct embedded_fw_desc chuwi_hi8_pro_fw[] __initdata = {
>> + {
>> + .name = "silead/gsl3680-chuwi-hi8-pro.fw",
>> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
>> + .length = 39864,
>> + .crc = 0xfe2bedba,
>> + },
>> + {}
>> +};
>> +
>> +static struct embedded_fw_desc cube_iwork8_air_fw[] __initdata = {
>> + {
>> + .name = "silead/gsl3670-cube-iwork8-air.fw",
>> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
>> + .length = 38808,
>> + .crc = 0xfecde51f,
>> + },
>> + {}
>> +};
>> +
>> +static struct embedded_fw_desc pipo_w2s_fw[] __initdata = {
>> + {
>> + .name = "silead/gsl1680-pipo-w2s.fw",
>> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
>> + .length = 39072,
>> + .crc = 0x28d5dc6c,
>> + },
>> + {}
>> +};
>> +
>> +static struct dmi_system_id embedded_fw_table[] __initdata = {
>> + {
>> + /* Chuwi Vi8 Plus (CWI506) */
>> + .driver_data = (void *)chuwi_vi8_plus_fw,
>> + .matches = {
>> + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
>> + DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
>> + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
>> + },
>> + },
>> + {
>> + /* Chuwi Hi8 Pro (CWI513) */
>> + .driver_data = (void *)chuwi_hi8_pro_fw,
>> + .matches = {
>> + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
>> + DMI_MATCH(DMI_PRODUCT_NAME, "X1D3_C806N"),
>> + },
>> + },
>> + {
>> + /* Cube iWork8 Air */
>> + .driver_data = (void *)cube_iwork8_air_fw,
>> + .matches = {
>> + DMI_MATCH(DMI_SYS_VENDOR, "cube"),
>> + DMI_MATCH(DMI_PRODUCT_NAME, "i1-TF"),
>> + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
>> + },
>> + },
>> + {
>> + /* Pipo W2s */
>> + .driver_data = (void *)pipo_w2s_fw,
>> + .matches = {
>> + DMI_MATCH(DMI_SYS_VENDOR, "PIPO"),
>> + DMI_MATCH(DMI_PRODUCT_NAME, "W2S"),
>> + },
>> + },
>> + {}
>> +};
>
> Maintaining these on a separate file might be easier to maintain.

Sure, I can move these to say:

drivers/firmware/efi/embedded-firmware-table.c ?

>> +
>> +/*
>> + * 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
>
> Who's defining this? Is this an agreed upon thing between a few companies, or
> is this written as part of a standard which we can refer to in documentation.

Definitely not part of the standard, this is just observed behavior on
devices which have (interesting) peripheral firmware embedded in their EFI
code.

>> + */
>> +static int __init efi_check_md_for_embedded_firmware(
>> + efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
>> +{
>> + u64 i, size;
>> + u32 crc;
>> + u8 *mem;
>> +
>> + size = md->num_pages << EFI_PAGE_SHIFT;
>> + mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>> + if (!mem) {
>> + pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
>> + return -ENOMEM;
>> + }
>> +
>> + size -= desc->length;
>> + for (i = 0; i < size; i += 8) {
>> + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>> + continue;
>> +
>> + /* Seed with ~0, invert to match crc32 userspace utility */
>> + crc = ~crc32(~0, mem + i, desc->length);
>> + if (crc == desc->crc)
>> + break;
>> + }
>> +
>> + memunmap(mem);
>> +
>> + if (i >= size)
>> + return -ENOENT;
>> +
>> + pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
>> +
>> + if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
>> + pr_err("Error already have %d embedded firmwares\n",
>> + MAX_EMBEDDED_FIRMWARES);
>> + return -ENOSPC;
>> + }
>> +
>> + found_fw[found_fw_count].data =
>> + memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>
> I've heard of some firmware bing over hundreds of MB these days. Once
> the can of worms is open its just a matter of time before someone
> tries to abuse, so do we have any limitation size? How about spec
> wise? Are there any limitations implied by it?
>
> If there are rather small, do we stand to gain instead to just kzalloc()
> and memcpy the found firmware? If done this way, wouldn't you be able
> to run this earlier?

Using kmalloc still requires memory-management to be setup, just as
using memremap does. The whole "needs to be run late" comment is
about this needing to run after mm_init(). Anyways as said I think
the whole when to run this discussion is a red herring based on my
poor choice of words in the commit message.

But doing a kmemdup on found firmware instead would avoid
the need for efi_mem_reserve()...

>> + if (!found_fw[found_fw_count].data) {
>> + pr_err("Error mapping embedded firmware\n");
>> + return -ENOMEM;
>> + }
>> +
>> + found_fw[found_fw_count].name = desc->name;
>> + found_fw[found_fw_count].length = desc->length;
>> + found_fw_count++;
>> +
>> + /* Note md points to *unmapped* memory after this!!! */
>> + efi_mem_reserve(md->phys_addr + i, desc->length);
>
> Do you need a for_each_efi_memory_desc_safe() perhaps?

See below.

>> + return 0;
>> +}
>> +
>> +void __init efi_check_for_embedded_firmwares(void)
>> +{
>> + const struct embedded_fw_desc *fw_desc;
>> + const struct dmi_system_id *dmi_id;
>> + efi_memory_desc_t *md;
>> + int i, r;
>> +
>> + dmi_id = dmi_first_match(embedded_fw_table);
>> + if (!dmi_id)
>> + return;
>> +
>> + fw_desc = dmi_id->driver_data;
>> + for (i = 0; fw_desc[i].length; i++) {
>> + for_each_efi_memory_desc(md) {
>> + if (md->type != EFI_BOOT_SERVICES_CODE)
>> + continue;
>> +
>> + r = efi_check_md_for_embedded_firmware(md, &fw_desc[i]);
>> + if (r == 0) {
>> + /*
>> + * On success efi_mem_reserve() has been called
>> + * installing a new memmap, so our pointers
>> + * are invalid now and we MUST break the loop.
>> + */
>> + break;
>
> Yeah this seems fragile. Can we do better?

If we want to use efi_mem_reserve() no, because the memory descriptors
are in an array and the entire array gets re-allocated on changes.

Note AFAICT this MUST be an array because we pass it to the EFI firmware,
but your suggestion to use kmemdup on the firmware would fix the need for
efi_mem_reserve() fixing the fragility, so that probably is a better way
to deal with this.

Regards,

Hans

>
> Luis
>
>> + }
>> + }
>> + }
>> +}
>> +
>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
>> + size_t msize)
>> +{
>> + struct embedded_fw *fw = NULL;
>> + void *buf = *data;
>> + int i;
>> +
>> + for (i = 0; i < found_fw_count; i++) {
>> + if (strcmp(name, found_fw[i].name) == 0) {
>> + fw = &found_fw[i];
>> + break;
>> + }
>> + }
>> +
>> + if (!fw)
>> + return -ENOENT;
>> +
>> + if (msize && msize < fw->length)
>> + return -EFBIG;
>> +
>> + if (!buf) {
>> + buf = vmalloc(fw->length);
>> + if (!buf)
>> + return -ENOMEM;
>> + }
>> +
>> + memcpy(buf, fw->data, fw->length);
>> + *size = fw->length;
>> + *data = buf;
>> +
>> + return 0;
>> +}
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index f5083aa72eae..bbdfda1d9e8d 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -1573,6 +1573,8 @@ efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
>> #endif
>>
>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>> +void efi_check_for_embedded_firmwares(void);
>> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
>>
>> /*
>> * Arch code can implement the following three template macros, avoiding
>> diff --git a/init/main.c b/init/main.c
>> index 969eaf140ef0..79b4a1b12509 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -710,6 +710,7 @@ asmlinkage __visible void __init start_kernel(void)
>> sfi_init_late();
>>
>> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>> + efi_check_for_embedded_firmwares();
>> efi_free_boot_services();
>> }
>>
>> --
>> 2.17.0.rc2
>>
>>
>

2018-04-03 18:09:55

by Lukas Wunner

[permalink] [raw]

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

On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:
> I asked Peter Jones for suggestions how to extract this during boot and
> he suggested seeing if there was a copy of the firmware in the
> EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.
>
> My patch to add support for this contains a table of device-model (dmi
> strings), firmware header (first 64 bits), length and crc32 and then if
> we boot on a device-model which is in the table the code scans the
> EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
> caches the firmware for later use by request-firmware.
>
> So I just do a brute-force search for the firmware, this really is hack,
> nothing standard about it I'm afraid. But it works on 4 different x86
> tablets I have and makes the touchscreen work OOTB on them, so I believe
> it is a worthwhile hack to have.

The EFI Firmware Volume contains a kind of filesystem with files
identified by GUIDs. Those files include EFI drivers, ACPI tables,
DMI data and so on. It is actually quite common for vendors to
also include device firmware on the Firmware Volume. Apple is doing
this to ship firmware updates e.g. for the GMUX controller found on
dual GPU MacBook Pros. If they want to update the controller's
firmware, they include it in a BIOS update, and an EFI driver checks
on boot if the firmware update for the controller is necessary and
if so, flashes it.

The firmware files you're looking for are almost certainly included
on the Firmware Volume as individual files. Rather than scraping
the EFI memory for firmware, I think it would be cleaner and more
elegant if you just retrieve the files you're interested in from
the Firmware Volume.

We're doing something similar with Apple EFI properties, see
58c5475aba67 and c9cc3aaa0281.

Basically what you need to do to implement this approach is:

* Determine the GUIDs used by vendors for the files you're interested
in. Either dump the Firmware Volume or take an EFI update as
shipped by the vendor, then feed it to UEFIExtract:
https://github.com/LongSoft/UEFITool

* Add the EFI Firmware Volume Protocol to include/linux/efi.h:
https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf

* Amend arch/x86/boot/compressed/eboot.c to read the files with the
GUIDs you're interested in into memory and pass the files to the
kernel as setup_data payloads.

* Once the kernel has booted, make the files you've retrieved
available to device drivers as firmware blobs.

The end result is mostly the same as what you're doing here,
and you'll also have a similar array of structs, but instead
of hardcoding 8-byte signatures of firmware files, you'll be
using GUIDs. I envision lots of interesting use cases for
a generic Firmware Volume file retrieval mechanism. What you
need to keep in mind though is that this approach only works
if the kernel is booted via the EFI stub.

Thanks,

Lukas

2018-04-03 18:49:10

[permalink] [raw]

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

In your next patches please Cc the folks I added for future review as well.
We don't have a mailing list for the firmware API so I just tend to Cc
who I think should help review when needed.

On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:
> Hi Luis,
>
> Thank you for the review.
>
> On 03-04-18 01:23, Luis R. Rodriguez wrote:
> > On Sat, Mar 31, 2018 at 02:19:44PM +0200, Hans de Goede 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.
> >
> > Some devices have OTP and use this sort of calibration data,
>
> Right, I'm not sure it really is OTP and not flash, but many touchscreen
> controllers do come with their firmware embedded into the controller,

It varies, sometimes firmware has default fluff calibration data as well which
can be used if the device does not have specific calibration data.

> but not all unfortunately.

Indeed.

> > I was unaware of
> > the use of EFI to stash firmware. Good to know, but can you also provide
> > references to what part of what standard should be followed for it in
> > documentation?
>
> This is not part of the standard. There has been a long(ish) standing issue
> with us not being able to get re-distribute permission for the firmware for
> some touchscreen controllers found on cheap x86 devices. Which means that
> we cannot put it in Linux firmware.

BTW do these cheap x86 devices have hw signing support? Just curious thinking
long term here. Because of it is not-standard then perhaps wen can partner
later up with a vendor to this properly and actually support hw firmware
singing.

> Dave Olsthoorn (in the Cc) noticed that the touchscreen did work in the
> refind bootload UI, so the EFI code must have a copy of the firmware.

:)

> I asked Peter Jones for suggestions how to extract this during boot and
> he suggested seeing if there was a copy of the firmware in the
> EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.

Sneaky Pete, nice. So essentially we're reverse engineering support for this.

Anyway please mention that this is not part of standard in the documentation,
and we've just found out in practice some vendors are doing this. That would
avoid having people ask later.

> My patch to add support for this contains a table of device-model (dmi
> strings), firmware header (first 64 bits), length and crc32 and then if
> we boot on a device-model which is in the table the code scans the
> EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
> caches the firmware for later use by request-firmware.

Neat, best to add proper docs for this.

> So I just do a brute-force search for the firmware, this really is hack,
> nothing standard about it I'm afraid. But it works on 4 different x86
> tablets I have and makes the touchscreen work OOTB on them, so I believe
> it is a worthwhile hack to have.

Absolutely, just not to shove an entire fallback firmware path to all users.

> > > 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 making this available to peripheral drivers through the
> > > standard firmware loading mechanism.
> >
> > Neat.
> >
> > > Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware pretty
> > > late in the init sequence,
> >
> > This also creates a technical limitation on use for the API that users
> > should be aware of. Its important to document such limitation.
>
> I don't think this is a problem for any normal drivers, when I say pretty
> late I mean late in init/main.c: start_kernel(), so still before any normal
> drivers load.
>
> The first idea was to scan for the firmware at the same time we check for
> things as the ACPI BGRT logo stuff, but as mentioned that requires using
> early_mmap() which does not work for the amount of memory we want to map.

Either way its good we went down this rabbit hole so its clear.

> > Also if we can address the limitation that would be even better.
> >
> > For instance, on what part of the driver is the call to request firmware
> > being made? Note that we support async probe now, so if the call was done
> > on probe, it may be wise to use async probe, however, can we be *certain*
> > that the EFI firmware would have been parsed and ready by then? Please
> > check. It just may be the case.
> >
> > Or, if we use late_initcall() would that suffice on the driver, if they
> > used a request firmware call on init or probe?
>
> As said I think we still do it early enough for any driver use, when
> I wrote "late in the init sequence" I should have probably written something
> else, like "near the end of start_kernel() instead of from setup_arch()"

Alright.

> >
> > > this is on purpose because the typical
> > > EFI_BOOT_SERVICES_CODE memory-segment is too large for early_memremap().
> >
> > To be clear you neede to use memremap()
>
> Yes.
>
> > What mechanism would have in place to ensure that a driver which expects
> > firmware to be on EFI data to be already available prior to its driver's
> > call to initialize?
>
> See above, this still runs before start_kernel() calls rest_init() which is
> where any normal init calls (and driver probing) happens so still early
> enough for any users I can think of.

The firmware API is even used to load microcode, but that relies on built-in
firmware support. That code needs to be refactored to be a proper citizen of the
firmware API, right now its just a hack. Reason for asking all these details
was to ensure we document the restrictions correctly so that expecations are
set correctly for callers prior to rest_init(). Please be sure to document the
limitations.

> I think my poorly worded commit
> message is causing a bit of unnecessary confusion here, sorry about that.
>
> > You seem to say its this consumes about about 25 MiB now, and for now you
> > have made this a debug thing only? How have these size requirements changed
> > over time? Has EFI_BOOT_SERVICES_CODE grown over time? How much? Do we
> > expect it will blow up later?
>
> The debug only thing is only patch 1/2, which is mostly independent of this
> patch (which is 2/2), patch 1 exports the EFI_BOOT_SERVICES_* memory segments
> as blobs under /sys/kernel/debug/efi, which requires not freeing them
> (or making a copy) and this costs memory.

Ah got it.

> The purpose of this is to be
> able to easily check them for embedded firmwares when adding new entries
> to the table of known embedded firmwares used by this patch.
>
> This patch will work fine without the first patch even being present in
> the kernel and will also work fine without efi=debug.

Thanks for the clarification.

> > > This means we rely on the EFI_BOOT_SERVICES_CODE not being free-ed until
> > > efi_free_boot_services() is called, which means that this will only work
> > > on x86, if we ever want this on ARM we should make ARM delay the freeing
> > > of the EFI_BOOT_SERVICES_* memory-segments too.
> >
> > Why not do that as well with your patch?
>
> That requires making significant changes to the early bringup code on
> ARM, x86 keeps EFI_BOOT_SERVICES_* memory-segments around until near
> the end of start_kernel() because freeing them earlier triggers bugs
> in some x86 EFI implementations, ARM EFI implementations do not have
> these bugs, so they free them almost directly at boot.
>
> Changing this really falls outside the scope of this patch.

Sure but did you poke ARM folks about it? Maybe they can do it?
And if this becomes a common practice, perhaps they can do it with
actual firmware signing instead of a CRC.

Not sure how hard it is to exploit EFI_BOOT_SERVICES_CODE... but it
may help UEFI folks with a nice warm fuzzy to start doing this right
later instead of propagating what seems to be a cheap hack.

> > > Note this commit also modifies efi_mem_desc_lookup() to not skip
> > > EFI_BOOT_SERVICES_CODE memory-segments, so that efi_mem_reserve() works
> > > on such segments.
> > >
> > > Reported-by: Dave Olsthoorn <[email protected]>
> > > Suggested-by: Peter Jones <[email protected]>
> > > Signed-off-by: Hans de Goede <[email protected]>
> > > ---
> > > drivers/base/firmware_class.c | 29 +++
> > > drivers/firmware/efi/Makefile | 1 +
> > > drivers/firmware/efi/efi.c | 1 +
> > > drivers/firmware/efi/embedded-firmware.c | 232 +++++++++++++++++++++++
> > > include/linux/efi.h | 2 +
> > > init/main.c | 1 +
> > > 6 files changed, 266 insertions(+)
> > > create mode 100644 drivers/firmware/efi/embedded-firmware.c
> > >
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 7dd36ace6152..b1e7b3de1975 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -12,6 +12,7 @@
> > > #include <linux/capability.h>
> > > #include <linux/device.h>
> > > +#include <linux/efi.h>
> > > #include <linux/module.h>
> > > #include <linux/init.h>
> > > #include <linux/timer.h>
> > > @@ -1207,6 +1208,32 @@ static inline void unregister_sysfs_loader(void)
> > > #endif /* CONFIG_FW_LOADER_USER_HELPER */
> > > +#ifdef CONFIG_EFI
> > > +static int
> > > +fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
> > > +{
> > > + size_t size;
> > > + int rc;
> > > +
> > > + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size,
> > > + fw_priv->data ? fw_priv->allocated_size : 0);
> > > + if (rc == 0) {
> > > + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> > > + fw_priv->size = size;
> > > + fw_state_done(fw_priv);
> > > + ret = 0;
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +#else
> > > +static inline int
> > > +fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
> > > +{
> > > + return ret;
> > > +}
> > > +#endif
> > > +
> > > /* prepare firmware and firmware_buf structs;
> > > * return 0 if a firmware is already assigned, 1 if need to load one,
> > > * or a negative error code
> > > @@ -1296,6 +1323,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> > > goto out;
> > > ret = fw_get_filesystem_firmware(device, fw->priv);
> > > + if (ret)
> > > + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
> >
> > This EFI firmware lookup is being used as a fallback mechanism, for *all*
> > requests. That's pretty aggressive and I'd like a bit more justification
> > for that approach.
>
> The fw_get_efi_embedded_fw() call is not that expensive, it walks the
> list of found firmwares, does a strcmp on the name and that is all it does,
> so I did not really see this as a problem, but if you want me to change this
> that is certainly possible.

The fallback mechanism / code has been a maintainer irritation for a long time now,
to verify its functionality, document it, and ensure folks understand how things
work. Best to be clear and simple about this functionality, adding more extensions
without any need just makes things more complex.

Another big reason is the amount of code implicated to support this which brings
me to the next request: please use a new kconfig to wrap this code into its own
kconfig entry.

The fallback mechanism code with CONFIG_FW_LOADER_USER_HELPER eats up
13436 bytes for instance and Josh has told me that even if Android
does enable it, there are still embedded devices out there that do not
want it and do want to reduce the size of their kernels by these
13436 bytes when possible.

How much code does enabling your code have to the kernel? Please add
that to the kconfig entry.

Oping in for a new mechanism via a kconfig is clearer to understand and
maintain, less code for folks, and specially since only a coupe of drivers
would be using this, its otherwise insane to enable by default.

In fact I'm now thinking this new fallback mechanism may be an
alternative to CONFIG_FW_LOADER_USER_HELPER but it very likely
can only work well for small firmware. How big are the currently
known firmwares BTW?

[0] https://lkml.kernel.org/r/20180301021956.GA12202@localhost

> > For instance, if its just a few drivers that really can use this, can't we just
> > add anew API call, say firmware_request_efi(), then add an internal flag for
> > this type of lookup and then this fallback mechanism would *only* be used for
> > those drivers.
>
> Yes that is certainly possible, currently there are 2 touchscreen drivers which
> can use this drivers/input/touchscreen/silead.c and
> drivers/input/touchscreen/chipone_icn8505.c, with the latter being a driver I just
> finished this weekend and which I will submit upstream soon.

OK, so only *one* upstream driver... Yeah with even more reason to never
consider this seriously by default upstream. It should be an opt-in
mechanism by drivers explicitly. In fact, it makes then wonder if we want
to even allow for the default fallback mechanism on the new call. I'm inclined
to suggest to *not* use it, given the intent and goal is clear by the
driver: first look for the file, if not found look for the firmware stashed
on on the EFI EFI_BOOT_SERVICES_CODE.

> > BTW please use linux-next to base your changes as a lot of things have changed
> > on the firmware API code, on queue on its way for v4.17-rc1.
>
> Ok, I usually prefer to only merge the relevant subsys-next into my personal
> tree rather then consuming the entirety of -next,

I just track linux-next separately on a different directory for this reason and
have it just --follow Linus' tree

[1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/

> which subsys tree has
> the firmware bits ?

You can also rely on just Greg's driver-core tree and use the driver-core-next
branch [1], you can look at the new firmware_request_cache() for an example of
a new call but note that you want something more like
request_firmware_direct(). Note that this will soon be renamed to
firmware_request_direct() to follow the convention of API name prefix first, so
be sure to use the firmware_ prefix for your new call as with
firmware_request_cache().

> > Please be sure
> > to also extend the documentation on Documentation/driver-api/firmware/
> > respectively.
>
> Ok.
>
> > > if (ret) {
> > > if (!(opt_flags & FW_OPT_NO_WARN))
> > > dev_warn(device,
> > > diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> > > index cb805374f4bc..cb946f7d0181 100644
> > > --- a/drivers/firmware/efi/Makefile
> > > +++ b/drivers/firmware/efi/Makefile
> > > @@ -13,6 +13,7 @@ KASAN_SANITIZE_runtime-wrappers.o := n
> > > obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
> > > obj-$(CONFIG_EFI) += efi.o vars.o reboot.o memattr.o tpm.o
> > > obj-$(CONFIG_EFI) += capsule.o memmap.o
> > > +obj-$(CONFIG_EFI) += embedded-firmware.o
> > > obj-$(CONFIG_EFI_VARS) += efivars.o
> > > obj-$(CONFIG_EFI_ESRT) += esrt.o
> > > obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index fddc5f706fd2..1a5ea950f58f 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -455,6 +455,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > u64 end;
> > > if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> > > + md->type != EFI_BOOT_SERVICES_CODE &&
> > > md->type != EFI_BOOT_SERVICES_DATA &&
> > > md->type != EFI_RUNTIME_SERVICES_DATA) {
> > > continue;
> > > diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
> > > new file mode 100644
> > > index 000000000000..80848f332b22
> > > --- /dev/null
> > > +++ b/drivers/firmware/efi/embedded-firmware.c
> > > @@ -0,0 +1,232 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Support for extracting embedded firmware for peripherals from EFI code,
> > > + *
> > > + * Copyright (c) 2018 Hans de Goede <[email protected]>
> > > + */
> > > +
> > > +#include <linux/crc32.h>
> > > +#include <linux/dmi.h>
> > > +#include <linux/efi.h>
> > > +#include <linux/types.h>
> > > +
> > > +/* Sofar there are no machines with more then 1 interesting embedded firmware */
> > > +#define MAX_EMBEDDED_FIRMWARES 1
> > > +
> > > +struct embedded_fw_desc {
> > > + const char *name;
> > > + u8 prefix[8];
> > > + u32 length;
> > > + u32 crc;
> > > +};
> > > +
> > > +struct embedded_fw {
> > > + const char *name;
> > > + void *data;
> > > + size_t length;
> > > +};
> > > +
> > > +static struct embedded_fw found_fw[MAX_EMBEDDED_FIRMWARES];
> >
> > This is just saving a few bytes, and is still pretty inflexible.
> > If were going to support this, this is a rather inflexible way to
> > support this. I'd prefer we link list this. This way if we support,
> > its an empty list and grows depending on what we find.
> >
> > I don't see the benefit of a static array here in any way.
>
> It is not like we are ever going to have more then 2-3 embedded
> firmwares in the foreseeable future and having a static array
> saves the need to kmalloc the struct embedded_fw and the additional
> error handling for when this fails, so the array leads to simpler
> code.

Yes but you are not maintaining this code, I am and I have no faith a drive-by
patch author will come back and extend this later when needed. As such, yes
please use a linked list to enable us to easily grow this now.

> But if you really want me to change this over to a linked
> list I can change it.

If a linked list is *really* an issue, I'd like to know how. For instance, if
its a lot of bytes of code its worth considering then, specially if this is for
embedded. The inability to easily support growth is just concerning here.

> > > +static int found_fw_count;
> > > +
> > > +static struct embedded_fw_desc chuwi_vi8_plus_fw[] __initdata = {
> > > + {
> > > + .name = "chipone/icn8318-HAMP0002.fw",
> > > + .prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
> > > + .length = 35012,
> > > + .crc = 0x74dfd3fc,
> > > + },
> > > + {}
> > > +};
> > > +
> > > +static struct embedded_fw_desc chuwi_hi8_pro_fw[] __initdata = {
> > > + {
> > > + .name = "silead/gsl3680-chuwi-hi8-pro.fw",
> > > + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
> > > + .length = 39864,
> > > + .crc = 0xfe2bedba,
> > > + },
> > > + {}
> > > +};
> > > +
> > > +static struct embedded_fw_desc cube_iwork8_air_fw[] __initdata = {
> > > + {
> > > + .name = "silead/gsl3670-cube-iwork8-air.fw",
> > > + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
> > > + .length = 38808,
> > > + .crc = 0xfecde51f,
> > > + },
> > > + {}
> > > +};
> > > +
> > > +static struct embedded_fw_desc pipo_w2s_fw[] __initdata = {
> > > + {
> > > + .name = "silead/gsl1680-pipo-w2s.fw",
> > > + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
> > > + .length = 39072,
> > > + .crc = 0x28d5dc6c,
> > > + },
> > > + {}
> > > +};
> > > +
> > > +static struct dmi_system_id embedded_fw_table[] __initdata = {
> > > + {
> > > + /* Chuwi Vi8 Plus (CWI506) */
> > > + .driver_data = (void *)chuwi_vi8_plus_fw,
> > > + .matches = {
> > > + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
> > > + DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
> > > + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
> > > + },
> > > + },
> > > + {
> > > + /* Chuwi Hi8 Pro (CWI513) */
> > > + .driver_data = (void *)chuwi_hi8_pro_fw,
> > > + .matches = {
> > > + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
> > > + DMI_MATCH(DMI_PRODUCT_NAME, "X1D3_C806N"),
> > > + },
> > > + },
> > > + {
> > > + /* Cube iWork8 Air */
> > > + .driver_data = (void *)cube_iwork8_air_fw,
> > > + .matches = {
> > > + DMI_MATCH(DMI_SYS_VENDOR, "cube"),
> > > + DMI_MATCH(DMI_PRODUCT_NAME, "i1-TF"),
> > > + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
> > > + },
> > > + },
> > > + {
> > > + /* Pipo W2s */
> > > + .driver_data = (void *)pipo_w2s_fw,
> > > + .matches = {
> > > + DMI_MATCH(DMI_SYS_VENDOR, "PIPO"),
> > > + DMI_MATCH(DMI_PRODUCT_NAME, "W2S"),
> > > + },
> > > + },
> > > + {}
> > > +};
> >
> > Maintaining these on a separate file might be easier to maintain.
>
> Sure, I can move these to say:
>
> drivers/firmware/efi/embedded-firmware-table.c ?

Sure. So to be clear without the above mapping we won't be able to find
the firmware. Also, the above should imply we have a respective upstream
driver per entry ? If so can you annotate some how which driver?

Or better yet... maintaining the above seems rather painful. Why not just let
the driver list this on its own with a macro, this way we don't have such a
list? I think there are parsers for example of MODULE_FIRMWARE() and if a
driver lists it, and the driver requires the firmware on boot I think some
tools include the driver's firmware on initramfs. Could something similar be
done to construct such a table automatically given the drivers enabled only
with their respective macro issued?

This way also if no driver is enabled that needs this, the code can just be
disabled and we save some more bytes on the kernel.

> > > +
> > > +/*
> > > + * 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
> >
> > Who's defining this? Is this an agreed upon thing between a few companies, or
> > is this written as part of a standard which we can refer to in documentation.
>
> Definitely not part of the standard, this is just observed behavior on
> devices which have (interesting) peripheral firmware embedded in their EFI
> code.

Then best document very well.

> > > + */
> > > +static int __init efi_check_md_for_embedded_firmware(
> > > + efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
> > > +{
> > > + u64 i, size;
> > > + u32 crc;
> > > + u8 *mem;
> > > +
> > > + size = md->num_pages << EFI_PAGE_SHIFT;
> > > + mem = memremap(md->phys_addr, size, MEMREMAP_WB);
> > > + if (!mem) {
> > > + pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + size -= desc->length;
> > > + for (i = 0; i < size; i += 8) {
> > > + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
> > > + continue;
> > > +
> > > + /* Seed with ~0, invert to match crc32 userspace utility */
> > > + crc = ~crc32(~0, mem + i, desc->length);
> > > + if (crc == desc->crc)
> > > + break;
> > > + }
> > > +
> > > + memunmap(mem);
> > > +
> > > + if (i >= size)
> > > + return -ENOENT;
> > > +
> > > + pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
> > > +
> > > + if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
> > > + pr_err("Error already have %d embedded firmwares\n",
> > > + MAX_EMBEDDED_FIRMWARES);
> > > + return -ENOSPC;
> > > + }
> > > +
> > > + found_fw[found_fw_count].data =
> > > + memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
> >
> > I've heard of some firmware bing over hundreds of MB these days. Once
> > the can of worms is open its just a matter of time before someone
> > tries to abuse, so do we have any limitation size? How about spec
> > wise? Are there any limitations implied by it?
> >
> > If there are rather small, do we stand to gain instead to just kzalloc()
> > and memcpy the found firmware? If done this way, wouldn't you be able
> > to run this earlier?
>
> Using kmalloc still requires memory-management to be setup, just as
> using memremap does. The whole "needs to be run late" comment is
> about this needing to run after mm_init(). Anyways as said I think
> the whole when to run this discussion is a red herring based on my
> poor choice of words in the commit message.
>
> But doing a kmemdup on found firmware instead would avoid
> the need for efi_mem_reserve()...

Yay.

>
> > > + if (!found_fw[found_fw_count].data) {
> > > + pr_err("Error mapping embedded firmware\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + found_fw[found_fw_count].name = desc->name;
> > > + found_fw[found_fw_count].length = desc->length;
> > > + found_fw_count++;
> > > +
> > > + /* Note md points to *unmapped* memory after this!!! */
> > > + efi_mem_reserve(md->phys_addr + i, desc->length);
> >
> > Do you need a for_each_efi_memory_desc_safe() perhaps?
>
> See below.
>
> > > + return 0;
> > > +}
> > > +
> > > +void __init efi_check_for_embedded_firmwares(void)
> > > +{
> > > + const struct embedded_fw_desc *fw_desc;
> > > + const struct dmi_system_id *dmi_id;
> > > + efi_memory_desc_t *md;
> > > + int i, r;
> > > +
> > > + dmi_id = dmi_first_match(embedded_fw_table);
> > > + if (!dmi_id)
> > > + return;
> > > +
> > > + fw_desc = dmi_id->driver_data;
> > > + for (i = 0; fw_desc[i].length; i++) {
> > > + for_each_efi_memory_desc(md) {
> > > + if (md->type != EFI_BOOT_SERVICES_CODE)
> > > + continue;
> > > +
> > > + r = efi_check_md_for_embedded_firmware(md, &fw_desc[i]);
> > > + if (r == 0) {
> > > + /*
> > > + * On success efi_mem_reserve() has been called
> > > + * installing a new memmap, so our pointers
> > > + * are invalid now and we MUST break the loop.
> > > + */
> > > + break;
> >
> > Yeah this seems fragile. Can we do better?
>
> If we want to use efi_mem_reserve() no, because the memory descriptors
> are in an array and the entire array gets re-allocated on changes.
>
> Note AFAICT this MUST be an array because we pass it to the EFI firmware,
> but your suggestion to use kmemdup on the firmware would fix the need for
> efi_mem_reserve() fixing the fragility, so that probably is a better way
> to deal with this.

OK!

Luis
>
> Regards,
>
> Hans
>
>
> >
> > Luis
> >
> > > + }
> > > + }
> > > + }
> > > +}
> > > +
> > > +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
> > > + size_t msize)
> > > +{
> > > + struct embedded_fw *fw = NULL;
> > > + void *buf = *data;
> > > + int i;
> > > +
> > > + for (i = 0; i < found_fw_count; i++) {
> > > + if (strcmp(name, found_fw[i].name) == 0) {
> > > + fw = &found_fw[i];
> > > + break;
> > > + }
> > > + }
> > > +
> > > + if (!fw)
> > > + return -ENOENT;
> > > +
> > > + if (msize && msize < fw->length)
> > > + return -EFBIG;
> > > +
> > > + if (!buf) {
> > > + buf = vmalloc(fw->length);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + memcpy(buf, fw->data, fw->length);
> > > + *size = fw->length;
> > > + *data = buf;
> > > +
> > > + return 0;
> > > +}
> > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > index f5083aa72eae..bbdfda1d9e8d 100644
> > > --- a/include/linux/efi.h
> > > +++ b/include/linux/efi.h
> > > @@ -1573,6 +1573,8 @@ efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
> > > #endif
> > > void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
> > > +void efi_check_for_embedded_firmwares(void);
> > > +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
> > > /*
> > > * Arch code can implement the following three template macros, avoiding
> > > diff --git a/init/main.c b/init/main.c
> > > index 969eaf140ef0..79b4a1b12509 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > > @@ -710,6 +710,7 @@ asmlinkage __visible void __init start_kernel(void)
> > > sfi_init_late();
> > > if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> > > + efi_check_for_embedded_firmwares();
> > > efi_free_boot_services();
> > > }
> > > --
> > > 2.17.0.rc2
> > >
> > >
> >
>

--
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

2018-04-03 19:00:38

[permalink] [raw]

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

On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
> On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:
> > I asked Peter Jones for suggestions how to extract this during boot and
> > he suggested seeing if there was a copy of the firmware in the
> > EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.
> >
> > My patch to add support for this contains a table of device-model (dmi
> > strings), firmware header (first 64 bits), length and crc32 and then if
> > we boot on a device-model which is in the table the code scans the
> > EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
> > caches the firmware for later use by request-firmware.
> >
> > So I just do a brute-force search for the firmware, this really is hack,
> > nothing standard about it I'm afraid. But it works on 4 different x86
> > tablets I have and makes the touchscreen work OOTB on them, so I believe
> > it is a worthwhile hack to have.
>
> The EFI Firmware Volume contains a kind of filesystem with files
> identified by GUIDs. Those files include EFI drivers, ACPI tables,
> DMI data and so on. It is actually quite common for vendors to
> also include device firmware on the Firmware Volume. Apple is doing
> this to ship firmware updates e.g. for the GMUX controller found on
> dual GPU MacBook Pros. If they want to update the controller's
> firmware, they include it in a BIOS update, and an EFI driver checks
> on boot if the firmware update for the controller is necessary and
> if so, flashes it.
>
> The firmware files you're looking for are almost certainly included
> on the Firmware Volume as individual files.

What Hans implemented seems to have been for a specific x86 hack, best if we
confirm if indeed they are present on the Firmware Volume.

> Rather than scraping
> the EFI memory for firmware, I think it would be cleaner and more
> elegant if you just retrieve the files you're interested in from
> the Firmware Volume.
>
> We're doing something similar with Apple EFI properties, see
> 58c5475aba67 and c9cc3aaa0281.
>
> Basically what you need to do to implement this approach is:
>
> * Determine the GUIDs used by vendors for the files you're interested
> in. Either dump the Firmware Volume or take an EFI update as
> shipped by the vendor, then feed it to UEFIExtract:
> https://github.com/LongSoft/UEFITool
>
> * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
> https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
>
> * Amend arch/x86/boot/compressed/eboot.c to read the files with the
> GUIDs you're interested in into memory and pass the files to the
> kernel as setup_data payloads.
>
> * Once the kernel has booted, make the files you've retrieved
> available to device drivers as firmware blobs.

Happen to know if devices using Firmware Volumes also sign their firmware
and if hw checks the firmware at load time?

Luis

> The end result is mostly the same as what you're doing here,
> and you'll also have a similar array of structs, but instead
> of hardcoding 8-byte signatures of firmware files, you'll be
> using GUIDs. I envision lots of interesting use cases for
> a generic Firmware Volume file retrieval mechanism. What you
> need to keep in mind though is that this approach only works
> if the kernel is booted via the EFI stub.
>
> Thanks,
>
> Lukas
>

--
Do not panic

2018-04-03 19:55:48

by Peter Jones

[permalink] [raw]

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

On Sat, Mar 31, 2018 at 02:19:44PM +0200, Hans de Goede wrote:
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fddc5f706fd2..1a5ea950f58f 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -455,6 +455,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> u64 end;
>
> if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> + md->type != EFI_BOOT_SERVICES_CODE &&
> md->type != EFI_BOOT_SERVICES_DATA &&
> md->type != EFI_RUNTIME_SERVICES_DATA) {
> continue;

Might be worth adding a comment here to ensure nobody comes along later
and adds something like EFI_BOOT_LOADER_DATA or other stuff that's
allocated later here. I don't want to accidentally patch our way into
having the ability to stumble across a firmware blob somebody dumped
into the middle of a grub config file, especially since you only need to
collide crc32 (within the same length) to pre-alias a match.

...
> +static int __init efi_check_md_for_embedded_firmware(
> + efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
> +{
...
> + if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
> + pr_err("Error already have %d embedded firmwares\n",
> + MAX_EMBEDDED_FIRMWARES);
> + return -ENOSPC;
> + }

Doesn't seem like this needs to be pr_err(); after all we have already
found a valid match, so the firmware vendor has done something
moderately stupid, but we have a firmware that will probably work. Of
course it still needs to return != 0, but pr_warn() or even pr_info()
seems more reasonable.

Aside from those nits, looks good to me.

Reviewed-by: Peter Jones <[email protected]>

--
Peter

2018-04-04 17:20:48

by Peter Jones

[permalink] [raw]

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

On Tue, Apr 03, 2018 at 06:58:48PM +0000, Luis R. Rodriguez wrote:
> On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
> > On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:
> > > I asked Peter Jones for suggestions how to extract this during boot and
> > > he suggested seeing if there was a copy of the firmware in the
> > > EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.
> > >
> > > My patch to add support for this contains a table of device-model (dmi
> > > strings), firmware header (first 64 bits), length and crc32 and then if
> > > we boot on a device-model which is in the table the code scans the
> > > EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
> > > caches the firmware for later use by request-firmware.
> > >
> > > So I just do a brute-force search for the firmware, this really is hack,
> > > nothing standard about it I'm afraid. But it works on 4 different x86
> > > tablets I have and makes the touchscreen work OOTB on them, so I believe
> > > it is a worthwhile hack to have.
> >
> > The EFI Firmware Volume contains a kind of filesystem with files
> > identified by GUIDs. Those files include EFI drivers, ACPI tables,
> > DMI data and so on. It is actually quite common for vendors to
> > also include device firmware on the Firmware Volume. Apple is doing
> > this to ship firmware updates e.g. for the GMUX controller found on
> > dual GPU MacBook Pros. If they want to update the controller's
> > firmware, they include it in a BIOS update, and an EFI driver checks
> > on boot if the firmware update for the controller is necessary and
> > if so, flashes it.
> >
> > The firmware files you're looking for are almost certainly included
> > on the Firmware Volume as individual files.
>
> What Hans implemented seems to have been for a specific x86 hack, best if we
> confirm if indeed they are present on the Firmware Volume.

To be honest, I'm a bit skeptical about the firmware volume approach.
Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
years, still don't seem to reliably parse firmware images I see in the
wild, and have a fairly regular need for fixes. These are tools
maintained by smart people who are making a real effort, and it still
looks pretty hard to do a good job that applies across a lot of
platforms.

So I'd rather use Hans's existing patches, at least for now, and if
someone is interested in hacking on making an efi firmware volume parser
for the kernel, switch them to that when such a thing is ready.

[0] [email protected]:LongSoft/UEFITool.git
[1] [email protected]:theopolis/uefi-firmware-parser.git

> > Rather than scraping
> > the EFI memory for firmware, I think it would be cleaner and more
> > elegant if you just retrieve the files you're interested in from
> > the Firmware Volume.
> >
> > We're doing something similar with Apple EFI properties, see
> > 58c5475aba67 and c9cc3aaa0281.
> >
> > Basically what you need to do to implement this approach is:
> >
> > * Determine the GUIDs used by vendors for the files you're interested
> > in. Either dump the Firmware Volume or take an EFI update as
> > shipped by the vendor, then feed it to UEFIExtract:
> > https://github.com/LongSoft/UEFITool
> >
> > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
> > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
> >
> > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
> > GUIDs you're interested in into memory and pass the files to the
> > kernel as setup_data payloads.
> >
> > * Once the kernel has booted, make the files you've retrieved
> > available to device drivers as firmware blobs.
>
> Happen to know if devices using Firmware Volumes also sign their firmware
> and if hw checks the firmware at load time?

It varies on a per-device basis, of course. Most new Intel machines as
of Haswell *should* be verifying their system firmware via Boot Guard,
which both checks an RSA signature and measures the firmware into the
TPM, but as with everything of this nature, there are certainly vendors
that screw it up. (I think AMD has something similar, but I'm really not
sure.)

--
Peter

2018-04-04 20:26:30

[permalink] [raw]

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

HI,

On 04-04-18 19:18, Peter Jones wrote:
> On Tue, Apr 03, 2018 at 06:58:48PM +0000, Luis R. Rodriguez wrote:
>> On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
>>> On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:
>>>> I asked Peter Jones for suggestions how to extract this during boot and
>>>> he suggested seeing if there was a copy of the firmware in the
>>>> EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.
>>>>
>>>> My patch to add support for this contains a table of device-model (dmi
>>>> strings), firmware header (first 64 bits), length and crc32 and then if
>>>> we boot on a device-model which is in the table the code scans the
>>>> EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
>>>> caches the firmware for later use by request-firmware.
>>>>
>>>> So I just do a brute-force search for the firmware, this really is hack,
>>>> nothing standard about it I'm afraid. But it works on 4 different x86
>>>> tablets I have and makes the touchscreen work OOTB on them, so I believe
>>>> it is a worthwhile hack to have.
>>>
>>> The EFI Firmware Volume contains a kind of filesystem with files
>>> identified by GUIDs. Those files include EFI drivers, ACPI tables,
>>> DMI data and so on. It is actually quite common for vendors to
>>> also include device firmware on the Firmware Volume. Apple is doing
>>> this to ship firmware updates e.g. for the GMUX controller found on
>>> dual GPU MacBook Pros. If they want to update the controller's
>>> firmware, they include it in a BIOS update, and an EFI driver checks
>>> on boot if the firmware update for the controller is necessary and
>>> if so, flashes it.
>>>
>>> The firmware files you're looking for are almost certainly included
>>> on the Firmware Volume as individual files.
>>
>> What Hans implemented seems to have been for a specific x86 hack, best if we
>> confirm if indeed they are present on the Firmware Volume.
>
> To be honest, I'm a bit skeptical about the firmware volume approach.
> Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
> years, still don't seem to reliably parse firmware images I see in the
> wild, and have a fairly regular need for fixes. These are tools
> maintained by smart people who are making a real effort, and it still
> looks pretty hard to do a good job that applies across a lot of
> platforms.
>
> So I'd rather use Hans's existing patches, at least for now, and if
> someone is interested in hacking on making an efi firmware volume parser
> for the kernel, switch them to that when such a thing is ready.
>
> [0] [email protected]:LongSoft/UEFITool.git
> [1] [email protected]:theopolis/uefi-firmware-parser.git
>
>>> Rather than scraping
>>> the EFI memory for firmware, I think it would be cleaner and more
>>> elegant if you just retrieve the files you're interested in from
>>> the Firmware Volume.
>>>
>>> We're doing something similar with Apple EFI properties, see
>>> 58c5475aba67 and c9cc3aaa0281.
>>>
>>> Basically what you need to do to implement this approach is:
>>>
>>> * Determine the GUIDs used by vendors for the files you're interested
>>> in. Either dump the Firmware Volume or take an EFI update as
>>> shipped by the vendor, then feed it to UEFIExtract:
>>> https://github.com/LongSoft/UEFITool
>>>
>>> * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
>>> https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
>>>
>>> * Amend arch/x86/boot/compressed/eboot.c to read the files with the
>>> GUIDs you're interested in into memory and pass the files to the
>>> kernel as setup_data payloads.
>>>
>>> * Once the kernel has booted, make the files you've retrieved
>>> available to device drivers as firmware blobs.
>>
>> Happen to know if devices using Firmware Volumes also sign their firmware
>> and if hw checks the firmware at load time?
>
> It varies on a per-device basis, of course. Most new Intel machines as
> of Haswell *should* be verifying their system firmware via Boot Guard,
> which both checks an RSA signature and measures the firmware into the
> TPM, but as with everything of this nature, there are certainly vendors
> that screw it up. (I think AMD has something similar, but I'm really not
> sure.)

Lukas, thank you for your suggestions on this, but I doubt that these
devices use the Firmware Volume stuff.

These are really cheap x86 Windows 10 tablets, everything about them is
simply hacked together by the manufacturer till it boots Windows10 and
then it is shipped to the customer without receiving any update
afterwards ever.

What you are describing sounds like significantly more work then
the vendor just embedding the firmware as a char firmware[] in their
EFI mouse driver.

That combined with Peter's worries about difficulties parsing the
Firmware Volume stuff, makes me believe that it is best to just
stick with my current approach as Peter suggests.

Regards,

Hans

2018-04-05 00:29:36

by Ard Biesheuvel

[permalink] [raw]

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

On 4 April 2018 at 22:25, Hans de Goede <[email protected]> wrote:
> HI,
>
>
> On 04-04-18 19:18, Peter Jones wrote:
>>
>> On Tue, Apr 03, 2018 at 06:58:48PM +0000, Luis R. Rodriguez wrote:
>>>
>>> On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
>>>>
>>>> On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:
>>>>>
>>>>> I asked Peter Jones for suggestions how to extract this during boot and
>>>>> he suggested seeing if there was a copy of the firmware in the
>>>>> EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.
>>>>>
>>>>> My patch to add support for this contains a table of device-model (dmi
>>>>> strings), firmware header (first 64 bits), length and crc32 and then if
>>>>> we boot on a device-model which is in the table the code scans the
>>>>> EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
>>>>> caches the firmware for later use by request-firmware.
>>>>>
>>>>> So I just do a brute-force search for the firmware, this really is
>>>>> hack,
>>>>> nothing standard about it I'm afraid. But it works on 4 different x86
>>>>> tablets I have and makes the touchscreen work OOTB on them, so I
>>>>> believe
>>>>> it is a worthwhile hack to have.
>>>>
>>>>
>>>> The EFI Firmware Volume contains a kind of filesystem with files
>>>> identified by GUIDs. Those files include EFI drivers, ACPI tables,
>>>> DMI data and so on. It is actually quite common for vendors to
>>>> also include device firmware on the Firmware Volume. Apple is doing
>>>> this to ship firmware updates e.g. for the GMUX controller found on
>>>> dual GPU MacBook Pros. If they want to update the controller's
>>>> firmware, they include it in a BIOS update, and an EFI driver checks
>>>> on boot if the firmware update for the controller is necessary and
>>>> if so, flashes it.
>>>>
>>>> The firmware files you're looking for are almost certainly included
>>>> on the Firmware Volume as individual files.
>>>
>>>
>>> What Hans implemented seems to have been for a specific x86 hack, best if
>>> we
>>> confirm if indeed they are present on the Firmware Volume.
>>
>>
>> To be honest, I'm a bit skeptical about the firmware volume approach.
>> Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
>> years, still don't seem to reliably parse firmware images I see in the
>> wild, and have a fairly regular need for fixes. These are tools
>> maintained by smart people who are making a real effort, and it still
>> looks pretty hard to do a good job that applies across a lot of
>> platforms.
>>
>> So I'd rather use Hans's existing patches, at least for now, and if
>> someone is interested in hacking on making an efi firmware volume parser
>> for the kernel, switch them to that when such a thing is ready.
>>
>> [0] [email protected]:LongSoft/UEFITool.git
>> [1] [email protected]:theopolis/uefi-firmware-parser.git
>>
>>>> Rather than scraping
>>>> the EFI memory for firmware, I think it would be cleaner and more
>>>> elegant if you just retrieve the files you're interested in from
>>>> the Firmware Volume.
>>>>
>>>> We're doing something similar with Apple EFI properties, see
>>>> 58c5475aba67 and c9cc3aaa0281.
>>>>
>>>> Basically what you need to do to implement this approach is:
>>>>
>>>> * Determine the GUIDs used by vendors for the files you're interested
>>>> in. Either dump the Firmware Volume or take an EFI update as
>>>> shipped by the vendor, then feed it to UEFIExtract:
>>>> https://github.com/LongSoft/UEFITool
>>>> * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
>>>>
>>>> https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
>>>>
>>>> * Amend arch/x86/boot/compressed/eboot.c to read the files with the
>>>> GUIDs you're interested in into memory and pass the files to the
>>>> kernel as setup_data payloads.
>>>>
>>>> * Once the kernel has booted, make the files you've retrieved
>>>> available to device drivers as firmware blobs.
>>>
>>>
>>> Happen to know if devices using Firmware Volumes also sign their firmware
>>> and if hw checks the firmware at load time?
>>
>>
>> It varies on a per-device basis, of course. Most new Intel machines as
>> of Haswell *should* be verifying their system firmware via Boot Guard,
>> which both checks an RSA signature and measures the firmware into the
>> TPM, but as with everything of this nature, there are certainly vendors
>> that screw it up. (I think AMD has something similar, but I'm really not
>> sure.)
>
>
> Lukas, thank you for your suggestions on this, but I doubt that these
> devices use the Firmware Volume stuff.
>

Aren't Firmware Volumes a PI thing rather than a UEFI thing?

> These are really cheap x86 Windows 10 tablets, everything about them is
> simply hacked together by the manufacturer till it boots Windows10 and
> then it is shipped to the customer without receiving any update
> afterwards ever.
>
> What you are describing sounds like significantly more work then
> the vendor just embedding the firmware as a char firmware[] in their
> EFI mouse driver.
>
> That combined with Peter's worries about difficulties parsing the
> Firmware Volume stuff, makes me believe that it is best to just
> stick with my current approach as Peter suggests.
>
> Regards,
>
> Hans
>

2018-04-05 05:46:14

by Lukas Wunner

[permalink] [raw]

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

On Wed, Apr 04, 2018 at 01:18:36PM -0400, Peter Jones wrote:
> > On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
> > > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
> > > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
> > >
> > > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
> > > GUIDs you're interested in into memory and pass the files to the
> > > kernel as setup_data payloads.
>
> To be honest, I'm a bit skeptical about the firmware volume approach.
> Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
> years, still don't seem to reliably parse firmware images I see in the
> wild, and have a fairly regular need for fixes. These are tools
> maintained by smart people who are making a real effort, and it still
> looks pretty hard to do a good job that applies across a lot of
> platforms.
>
> So I'd rather use Hans's existing patches, at least for now, and if
> someone is interested in hacking on making an efi firmware volume parser
> for the kernel, switch them to that when such a thing is ready.

Hello? As I've written in the above-quoted e-mail the kernel should
read the files using EFI_FIRMWARE_VOLUME_PROTOCOL.ReadFile().

*Not* by parsing the firmware volume!

Parsing the firmware volume is only necessary to find out the GUIDs
of the files you're looking for. You only do that *once*.

I habitually extract Apple's EFI Firmware Volumes and found the
existing tools always did a sufficiently good job to get the
information I need (such as UEFIExtract, which I've linked to,
and which is part of the UEFITool suite, which you've linked to
once more).

On Wed, Apr 04, 2018 at 10:25:05PM +0200, Hans de Goede wrote:
> Lukas, thank you for your suggestions on this, but I doubt that these
> devices use the Firmware Volume stuff.

If they're using EFI, they're using a Firmware Volume and you should
be able to use the Firmware Volume Protocol to read files from it.

If that protocol wasn't supported, the existing EFI drivers wouldn't
be able to function as they couldn't load files from the volume.

> What you are describing sounds like significantly more work then
> the vendor just embedding the firmware as a char firmware[] in their
> EFI mouse driver.

In such cases, read the EFI mouse driver from the firmware volume and
extract the firmware from the offset you've pre-determined. That's
never going to change since the devices never receive updates, as
you're saying. So basically you'll have a struct with GUID, offset,
length, and the name under which the firmware is made available to
Linux drivers.

> That combined with Peter's worries about difficulties parsing the
> Firmware Volume stuff, makes me believe that it is best to just
> stick with my current approach as Peter suggests.

I don't know why you insist on a hackish solution (your own words)
even though a cleaner solution is suggested, but fortunately it's
not for me to decide what goes in and what doesn't. ;-)

Thanks,

Lukas

2018-04-05 11:52:51

[permalink] [raw]

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

Hi,

On 03-04-18 21:53, Peter Jones wrote:
> On Sat, Mar 31, 2018 at 02:19:44PM +0200, Hans de Goede wrote:
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index fddc5f706fd2..1a5ea950f58f 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -455,6 +455,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>> u64 end;
>>
>> if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
>> + md->type != EFI_BOOT_SERVICES_CODE &&
>> md->type != EFI_BOOT_SERVICES_DATA &&
>> md->type != EFI_RUNTIME_SERVICES_DATA) {
>> continue;
>
> Might be worth adding a comment here to ensure nobody comes along later
> and adds something like EFI_BOOT_LOADER_DATA or other stuff that's
> allocated later here. I don't want to accidentally patch our way into
> having the ability to stumble across a firmware blob somebody dumped
> into the middle of a grub config file, especially since you only need to
> collide crc32 (within the same length) to pre-alias a match.

As discussed elsewhere in the thread, I'm going to switch to doing a
kmemdup on the found firmware, so this chunk will go away :)

>
> ...
>> +static int __init efi_check_md_for_embedded_firmware(
>> + efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
>> +{
> ...
>> + if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
>> + pr_err("Error already have %d embedded firmwares\n",
>> + MAX_EMBEDDED_FIRMWARES);
>> + return -ENOSPC;
>> + }
>
> Doesn't seem like this needs to be pr_err(); after all we have already
> found a valid match, so the firmware vendor has done something
> moderately stupid, but we have a firmware that will probably work. Of
> course it still needs to return != 0, but pr_warn() or even pr_info()
> seems more reasonable.

We break from the search loop as soon as a firmware is found, this can
only trigger if someone adds a second firmware to the dmi data and then
does not update MAX_EMBEDDED_FIRMWARES...

But mcgrof wants me to switch to a linked list here, so this is going
away too.

> Aside from those nits, looks good to me.
>
> Reviewed-by: Peter Jones <[email protected]>

Thanks, but v2 is going to have so much changes that I don't feel
comfortable bringing this forward to v2.

Regards,

Hans

2018-04-05 14:27:54

[permalink] [raw]

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

Hi,

On 03-04-18 20:47, Luis R. Rodriguez wrote:
> In your next patches please Cc the folks I added for future review as well.
> We don't have a mailing list for the firmware API so I just tend to Cc
> who I think should help review when needed.

Hmm, quite a long Cc list, but ok.

> On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:

<snip>

>> This is not part of the standard. There has been a long(ish) standing issue
>> with us not being able to get re-distribute permission for the firmware for
>> some touchscreen controllers found on cheap x86 devices. Which means that
>> we cannot put it in Linux firmware.
>
> BTW do these cheap x86 devices have hw signing support?

Not sure what you mean with hw signing support here, they support UEFI
secure-boot and have a fTPM.

> Just curious thinking
> long term here. Because of it is not-standard then perhaps wen can partner
> later up with a vendor to this properly and actually support hw firmware
> singing.
>
>> Dave Olsthoorn (in the Cc) noticed that the touchscreen did work in the
>> refind bootload UI, so the EFI code must have a copy of the firmware.
>
> :)
>
>> I asked Peter Jones for suggestions how to extract this during boot and
>> he suggested seeing if there was a copy of the firmware in the
>> EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.
>
> Sneaky Pete, nice. So essentially we're reverse engineering support for this.

Yes.

> Anyway please mention that this is not part of standard in the documentation,
> and we've just found out in practice some vendors are doing this. That would
> avoid having people ask later.

Ok, I will add some docs for v2 of the patch.

>> My patch to add support for this contains a table of device-model (dmi
>> strings), firmware header (first 64 bits), length and crc32 and then if
>> we boot on a device-model which is in the table the code scans the
>> EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
>> caches the firmware for later use by request-firmware.
>
> Neat, best to add proper docs for this.

Ok.

>> So I just do a brute-force search for the firmware, this really is hack,
>> nothing standard about it I'm afraid. But it works on 4 different x86
>> tablets I have and makes the touchscreen work OOTB on them, so I believe
>> it is a worthwhile hack to have.
>
> Absolutely, just not to shove an entire fallback firmware path to all users.

Ok.

<snip>

>>> What mechanism would have in place to ensure that a driver which expects
>>> firmware to be on EFI data to be already available prior to its driver's
>>> call to initialize?
>>
>> See above, this still runs before start_kernel() calls rest_init() which is
>> where any normal init calls (and driver probing) happens so still early
>> enough for any users I can think of.
>
> The firmware API is even used to load microcode, but that relies on built-in
> firmware support. That code needs to be refactored to be a proper citizen of the
> firmware API, right now its just a hack. Reason for asking all these details
> was to ensure we document the restrictions correctly so that expecations are
> set correctly for callers prior to rest_init(). Please be sure to document the
> limitations.

Ok.

<snip>

>>>> This means we rely on the EFI_BOOT_SERVICES_CODE not being free-ed until
>>>> efi_free_boot_services() is called, which means that this will only work
>>>> on x86, if we ever want this on ARM we should make ARM delay the freeing
>>>> of the EFI_BOOT_SERVICES_* memory-segments too.
>>>
>>> Why not do that as well with your patch?
>>
>> That requires making significant changes to the early bringup code on
>> ARM, x86 keeps EFI_BOOT_SERVICES_* memory-segments around until near
>> the end of start_kernel() because freeing them earlier triggers bugs
>> in some x86 EFI implementations, ARM EFI implementations do not have
>> these bugs, so they free them almost directly at boot.
>>
>> Changing this really falls outside the scope of this patch.
>
> Sure but did you poke ARM folks about it? Maybe they can do it?
> And if this becomes a common practice, perhaps they can do it with
> actual firmware signing instead of a CRC.
>
> Not sure how hard it is to exploit EFI_BOOT_SERVICES_CODE... but it
> may help UEFI folks with a nice warm fuzzy to start doing this right
> later instead of propagating what seems to be a cheap hack.

If things are compromised before the kernel boots no amount of
checks are going to help us, an attacker can then just boot an entirely
different kernel, rather then attacking the system through a backdoored
firmware, so a CRC should be fine. Either way I'm happy to switch to a
crypto hash, but the crypto subsystem is not initialized yet at this
point AFAICT, so using a CRC is easier.

>>>> Note this commit also modifies efi_mem_desc_lookup() to not skip
>>>> EFI_BOOT_SERVICES_CODE memory-segments, so that efi_mem_reserve() works
>>>> on such segments.
>>>>
>>>> Reported-by: Dave Olsthoorn <[email protected]>
>>>> Suggested-by: Peter Jones <[email protected]>
>>>> Signed-off-by: Hans de Goede <[email protected]>
>>>> ---
>>>> drivers/base/firmware_class.c | 29 +++
>>>> drivers/firmware/efi/Makefile | 1 +
>>>> drivers/firmware/efi/efi.c | 1 +
>>>> drivers/firmware/efi/embedded-firmware.c | 232 +++++++++++++++++++++++
>>>> include/linux/efi.h | 2 +
>>>> init/main.c | 1 +
>>>> 6 files changed, 266 insertions(+)
>>>> create mode 100644 drivers/firmware/efi/embedded-firmware.c
>>>>
>>>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>>>> index 7dd36ace6152..b1e7b3de1975 100644

<snip>

>>>> @@ -1296,6 +1323,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>>>> goto out;
>>>> ret = fw_get_filesystem_firmware(device, fw->priv);
>>>> + if (ret)
>>>> + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
>>>
>>> This EFI firmware lookup is being used as a fallback mechanism, for *all*
>>> requests. That's pretty aggressive and I'd like a bit more justification
>>> for that approach.
>>
>> The fw_get_efi_embedded_fw() call is not that expensive, it walks the
>> list of found firmwares, does a strcmp on the name and that is all it does,
>> so I did not really see this as a problem, but if you want me to change this
>> that is certainly possible.
>
> The fallback mechanism / code has been a maintainer irritation for a long time now,
> to verify its functionality, document it, and ensure folks understand how things
> work. Best to be clear and simple about this functionality, adding more extensions
> without any need just makes things more complex.
>
> Another big reason is the amount of code implicated to support this which brings
> me to the next request: please use a new kconfig to wrap this code into its own
> kconfig entry.

Ok.

> The fallback mechanism code with CONFIG_FW_LOADER_USER_HELPER eats up
> 13436 bytes for instance and Josh has told me that even if Android
> does enable it, there are still embedded devices out there that do not
> want it and do want to reduce the size of their kernels by these
> 13436 bytes when possible.
>
> How much code does enabling your code have to the kernel? Please add
> that to the kconfig entry.
>
> Oping in for a new mechanism via a kconfig is clearer to understand and
> maintain, less code for folks, and specially since only a coupe of drivers
> would be using this, its otherwise insane to enable by default.

Right, I agree that using a new Kconfig entry for this is a good idea,
thinking more about that we can even make it a hidden option and
have the drivers which need this do a "select EFI_EMBEDDED_FIRMWARE",
if we're going to require a special flag during loading of the firmware,
then I think having the drivers also control the Kconfig option make
sense.

> In fact I'm now thinking this new fallback mechanism may be an
> alternative to CONFIG_FW_LOADER_USER_HELPER but it very likely
> can only work well for small firmware. How big are the currently
> known firmwares BTW?

They are all around 40k, so quite small.

> [0] https://lkml.kernel.org/r/20180301021956.GA12202@localhost
>
>>> For instance, if its just a few drivers that really can use this, can't we just
>>> add anew API call, say firmware_request_efi(), then add an internal flag for
>>> this type of lookup and then this fallback mechanism would *only* be used for
>>> those drivers.
>>
>> Yes that is certainly possible, currently there are 2 touchscreen drivers which
>> can use this drivers/input/touchscreen/silead.c and
>> drivers/input/touchscreen/chipone_icn8505.c, with the latter being a driver I just
>> finished this weekend and which I will submit upstream soon.
>
> OK, so only *one* upstream driver... Yeah with even more reason to never
> consider this seriously by default upstream. It should be an opt-in
> mechanism by drivers explicitly. In fact, it makes then wonder if we want
> to even allow for the default fallback mechanism on the new call. I'm inclined
> to suggest to *not* use it, given the intent and goal is clear by the
> driver: first look for the file, if not found look for the firmware stashed
> on on the EFI EFI_BOOT_SERVICES_CODE.

As the maintainer of that 1 upstream driver (soon to be 2 I submitted
the other driver a couple of days ago) which uses this, I'm fine with the
EFI fallback replacing the userspace-helper fallback path.

<snip>

>>>> diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
>>>> new file mode 100644
>>>> index 000000000000..80848f332b22
>>>> --- /dev/null
>>>> +++ b/drivers/firmware/efi/embedded-firmware.c
>>>> @@ -0,0 +1,232 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Support for extracting embedded firmware for peripherals from EFI code,
>>>> + *
>>>> + * Copyright (c) 2018 Hans de Goede <[email protected]>
>>>> + */
>>>> +
>>>> +#include <linux/crc32.h>
>>>> +#include <linux/dmi.h>
>>>> +#include <linux/efi.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +/* Sofar there are no machines with more then 1 interesting embedded firmware */
>>>> +#define MAX_EMBEDDED_FIRMWARES 1
>>>> +
>>>> +struct embedded_fw_desc {
>>>> + const char *name;
>>>> + u8 prefix[8];
>>>> + u32 length;
>>>> + u32 crc;
>>>> +};
>>>> +
>>>> +struct embedded_fw {
>>>> + const char *name;
>>>> + void *data;
>>>> + size_t length;
>>>> +};
>>>> +
>>>> +static struct embedded_fw found_fw[MAX_EMBEDDED_FIRMWARES];
>>>
>>> This is just saving a few bytes, and is still pretty inflexible.
>>> If were going to support this, this is a rather inflexible way to
>>> support this. I'd prefer we link list this. This way if we support,
>>> its an empty list and grows depending on what we find.
>>>
>>> I don't see the benefit of a static array here in any way.
>>
>> It is not like we are ever going to have more then 2-3 embedded
>> firmwares in the foreseeable future and having a static array
>> saves the need to kmalloc the struct embedded_fw and the additional
>> error handling for when this fails, so the array leads to simpler
>> code.
>
> Yes but you are not maintaining this code, I am and I have no faith a drive-by
> patch author will come back and extend this later when needed. As such, yes
> please use a linked list to enable us to easily grow this now.

Ok, I will use a linked-list for v2.

>> But if you really want me to change this over to a linked
>> list I can change it.
>
> If a linked list is *really* an issue, I'd like to know how. For instance, if
> its a lot of bytes of code its worth considering then, specially if this is for
> embedded. The inability to easily support growth is just concerning here.
>
>>>> +static int found_fw_count;
>>>> +
>>>> +static struct embedded_fw_desc chuwi_vi8_plus_fw[] __initdata = {
>>>> + {
>>>> + .name = "chipone/icn8318-HAMP0002.fw",
>>>> + .prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
>>>> + .length = 35012,
>>>> + .crc = 0x74dfd3fc,
>>>> + },
>>>> + {}
>>>> +};
>>>> +
>>>> +static struct embedded_fw_desc chuwi_hi8_pro_fw[] __initdata = {
>>>> + {
>>>> + .name = "silead/gsl3680-chuwi-hi8-pro.fw",
>>>> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
>>>> + .length = 39864,
>>>> + .crc = 0xfe2bedba,
>>>> + },
>>>> + {}
>>>> +};
>>>> +
>>>> +static struct embedded_fw_desc cube_iwork8_air_fw[] __initdata = {
>>>> + {
>>>> + .name = "silead/gsl3670-cube-iwork8-air.fw",
>>>> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
>>>> + .length = 38808,
>>>> + .crc = 0xfecde51f,
>>>> + },
>>>> + {}
>>>> +};
>>>> +
>>>> +static struct embedded_fw_desc pipo_w2s_fw[] __initdata = {
>>>> + {
>>>> + .name = "silead/gsl1680-pipo-w2s.fw",
>>>> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
>>>> + .length = 39072,
>>>> + .crc = 0x28d5dc6c,
>>>> + },
>>>> + {}
>>>> +};
>>>> +
>>>> +static struct dmi_system_id embedded_fw_table[] __initdata = {
>>>> + {
>>>> + /* Chuwi Vi8 Plus (CWI506) */
>>>> + .driver_data = (void *)chuwi_vi8_plus_fw,
>>>> + .matches = {
>>>> + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
>>>> + DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
>>>> + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
>>>> + },
>>>> + },
>>>> + {
>>>> + /* Chuwi Hi8 Pro (CWI513) */
>>>> + .driver_data = (void *)chuwi_hi8_pro_fw,
>>>> + .matches = {
>>>> + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
>>>> + DMI_MATCH(DMI_PRODUCT_NAME, "X1D3_C806N"),
>>>> + },
>>>> + },
>>>> + {
>>>> + /* Cube iWork8 Air */
>>>> + .driver_data = (void *)cube_iwork8_air_fw,
>>>> + .matches = {
>>>> + DMI_MATCH(DMI_SYS_VENDOR, "cube"),
>>>> + DMI_MATCH(DMI_PRODUCT_NAME, "i1-TF"),
>>>> + DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
>>>> + },
>>>> + },
>>>> + {
>>>> + /* Pipo W2s */
>>>> + .driver_data = (void *)pipo_w2s_fw,
>>>> + .matches = {
>>>> + DMI_MATCH(DMI_SYS_VENDOR, "PIPO"),
>>>> + DMI_MATCH(DMI_PRODUCT_NAME, "W2S"),
>>>> + },
>>>> + },
>>>> + {}
>>>> +};
>>>
>>> Maintaining these on a separate file might be easier to maintain.
>>
>> Sure, I can move these to say:
>>
>> drivers/firmware/efi/embedded-firmware-table.c ?
>
> Sure. So to be clear without the above mapping we won't be able to find
> the firmware. Also, the above should imply we have a respective upstream
> driver per entry ? If so can you annotate some how which driver?
>
> Or better yet... maintaining the above seems rather painful. Why not just let
> the driver list this on its own with a macro, this way we don't have such a
> list? I think there are parsers for example of MODULE_FIRMWARE() and if a
> driver lists it, and the driver requires the firmware on boot I think some
> tools include the driver's firmware on initramfs. Could something similar be
> done to construct such a table automatically given the drivers enabled only
> with their respective macro issued?

So thinking more about this, I agree that this needs to be driver driven,
for the silead touchscreens we have:

drivers/platform/x86/silead_dmi.c

Which contains touchscreen parameters such as x and y coordinate ranges
and touchscreen orientation vs lcd-panel orientation, as well as a
per model firmware filename as the firmware is model-specific.

It makes a lot of sense to also add embedded firmware info in the
very same dmi_system_id table, this can simply be done by making the
data pointed to by the dmi_system_id.driver_data member always
start with a struct embedded_fw_desc. Then the efi embedded firmware
code can use the same table.

The chipone-icn8505 touchscreens don't need any of the mentioned
parameters, there is an ACPI method (_SUB) to get a string which
should uniquely identify the needed firmware file and the other info
can be read back from the hardware after loading the firmware, but
we can still use the same mechanism there.

So my plan for v2 is to:
-rename silead_dmi to touchscreen_dmi (and also use it for chipone info)
-have the TOUCHSCREEN_DMI Kconfig option select EFI_EMBEDDED_FIRMWARE
-inside drivers/firmware/efi/embedded-firmware.c, have:

static const struct * const dmi_system_id embedded_fw_table[] = {
#ifdef CONFIG_TOUCHSCREEN_DMI
touchscreen_dmi_table,
#endif
NULL
};

With the idea that in the future it may become:

static const struct * const dmi_system_id embedded_fw_table[] = {
#ifdef CONFIG_TOUCHSCREEN_DMI
touchscreen_dmi_table,
#endif
#ifdef CONFIG_DRIVER_FOOBAR
driver_foobar_dmi_table,
#endif
#ifdef CONFIG_DRIVER_BARFOO
driver_foobar_dmi_table,
#endif
NULL
};

This means one commit touching drivers/firmware/efi/embedded-firmware.c
each time a new driver starts using EFI embedded firmware, but I think
that will be quite rare and if I turn out to be wrong we can always
do something more fancy later.

> This way also if no driver is enabled that needs this, the code can just be
> disabled and we save some more bytes on the kernel.

Ack.

>
>>>> +
>>>> +/*
>>>> + * 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
>>>
>>> Who's defining this? Is this an agreed upon thing between a few companies, or
>>> is this written as part of a standard which we can refer to in documentation.
>>
>> Definitely not part of the standard, this is just observed behavior on
>> devices which have (interesting) peripheral firmware embedded in their EFI
>> code.
>
> Then best document very well.

Ack.

>>>> + */
>>>> +static int __init efi_check_md_for_embedded_firmware(
>>>> + efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
>>>> +{
>>>> + u64 i, size;
>>>> + u32 crc;
>>>> + u8 *mem;
>>>> +
>>>> + size = md->num_pages << EFI_PAGE_SHIFT;
>>>> + mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>>>> + if (!mem) {
>>>> + pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + size -= desc->length;
>>>> + for (i = 0; i < size; i += 8) {
>>>> + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>>>> + continue;
>>>> +
>>>> + /* Seed with ~0, invert to match crc32 userspace utility */
>>>> + crc = ~crc32(~0, mem + i, desc->length);
>>>> + if (crc == desc->crc)
>>>> + break;
>>>> + }
>>>> +
>>>> + memunmap(mem);
>>>> +
>>>> + if (i >= size)
>>>> + return -ENOENT;
>>>> +
>>>> + pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
>>>> +
>>>> + if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
>>>> + pr_err("Error already have %d embedded firmwares\n",
>>>> + MAX_EMBEDDED_FIRMWARES);
>>>> + return -ENOSPC;
>>>> + }
>>>> +
>>>> + found_fw[found_fw_count].data =
>>>> + memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>>>
>>> I've heard of some firmware bing over hundreds of MB these days. Once
>>> the can of worms is open its just a matter of time before someone
>>> tries to abuse, so do we have any limitation size? How about spec
>>> wise? Are there any limitations implied by it?
>>>
>>> If there are rather small, do we stand to gain instead to just kzalloc()
>>> and memcpy the found firmware? If done this way, wouldn't you be able
>>> to run this earlier?
>>
>> Using kmalloc still requires memory-management to be setup, just as
>> using memremap does. The whole "needs to be run late" comment is
>> about this needing to run after mm_init(). Anyways as said I think
>> the whole when to run this discussion is a red herring based on my
>> poor choice of words in the commit message.
>>
>> But doing a kmemdup on found firmware instead would avoid
>> the need for efi_mem_reserve()...
>
> Yay.
>>
>>>> + if (!found_fw[found_fw_count].data) {
>>>> + pr_err("Error mapping embedded firmware\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + found_fw[found_fw_count].name = desc->name;
>>>> + found_fw[found_fw_count].length = desc->length;
>>>> + found_fw_count++;
>>>> +
>>>> + /* Note md points to *unmapped* memory after this!!! */
>>>> + efi_mem_reserve(md->phys_addr + i, desc->length);
>>>
>>> Do you need a for_each_efi_memory_desc_safe() perhaps?
>>
>> See below.
>>
>>>> + return 0;
>>>> +}
>>>> +
>>>> +void __init efi_check_for_embedded_firmwares(void)
>>>> +{
>>>> + const struct embedded_fw_desc *fw_desc;
>>>> + const struct dmi_system_id *dmi_id;
>>>> + efi_memory_desc_t *md;
>>>> + int i, r;
>>>> +
>>>> + dmi_id = dmi_first_match(embedded_fw_table);
>>>> + if (!dmi_id)
>>>> + return;
>>>> +
>>>> + fw_desc = dmi_id->driver_data;
>>>> + for (i = 0; fw_desc[i].length; i++) {
>>>> + for_each_efi_memory_desc(md) {
>>>> + if (md->type != EFI_BOOT_SERVICES_CODE)
>>>> + continue;
>>>> +
>>>> + r = efi_check_md_for_embedded_firmware(md, &fw_desc[i]);
>>>> + if (r == 0) {
>>>> + /*
>>>> + * On success efi_mem_reserve() has been called
>>>> + * installing a new memmap, so our pointers
>>>> + * are invalid now and we MUST break the loop.
>>>> + */
>>>> + break;
>>>
>>> Yeah this seems fragile. Can we do better?
>>
>> If we want to use efi_mem_reserve() no, because the memory descriptors
>> are in an array and the entire array gets re-allocated on changes.
>>
>> Note AFAICT this MUST be an array because we pass it to the EFI firmware,
>> but your suggestion to use kmemdup on the firmware would fix the need for
>> efi_mem_reserve() fixing the fragility, so that probably is a better way
>> to deal with this.
>
> OK!

I will switch to kmemdup for v2. The memory usage will be the same as we
can then omit the efi_mem_reserve() call and the original copy will get
freed.

Ok, time for me to start working on a v2, based on the latest driver-core/next
code, adding all the discussed changes as well as adding a bunch of documentation.

Regards,

Hans

2018-04-06 14:09:57

[permalink] [raw]

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

2018-04-06 14:15:59

by Ard Biesheuvel

[permalink] [raw]

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

On 6 April 2018 at 16:08, Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Apr 05, 2018 at 07:43:49AM +0200, Lukas Wunner wrote:
>> On Wed, Apr 04, 2018 at 01:18:36PM -0400, Peter Jones wrote:
>> > > On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
>> > > > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
>> > > > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
>> > > >
>> > > > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
>> > > > GUIDs you're interested in into memory and pass the files to the
>> > > > kernel as setup_data payloads.
>> >
>> > To be honest, I'm a bit skeptical about the firmware volume approach.
>> > Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
>> > years, still don't seem to reliably parse firmware images I see in the
>> > wild, and have a fairly regular need for fixes. These are tools
>> > maintained by smart people who are making a real effort, and it still
>> > looks pretty hard to do a good job that applies across a lot of
>> > platforms.
>> >
>> > So I'd rather use Hans's existing patches, at least for now, and if
>> > someone is interested in hacking on making an efi firmware volume parser
>> > for the kernel, switch them to that when such a thing is ready.
>>
>> Hello? As I've written in the above-quoted e-mail the kernel should
>> read the files using EFI_FIRMWARE_VOLUME_PROTOCOL.ReadFile().
>>
>> *Not* by parsing the firmware volume!
>>
>> Parsing the firmware volume is only necessary to find out the GUIDs
>> of the files you're looking for. You only do that *once*.
>
> How do you get the GUIDs for each driver BTW?
>
> Hans, I do believe we should *try* this approach at the very least.
>
> Why not?
>
> Otherwise it would be wise to provide a technical reason for why
> we'd choose one custom mechanism which would only serve a few tablets
> over a mechanism which could serve more devices.
>

Because EFI_FIRMWARE_VOLUME_PROTOCOL is not part of the UEFI spec but
of the PI spec, and so we will be adding dependencies on
implementation details of the firmware. I am aware we may already have
done so for the Apple properties support, but I think it makes sense
to make an exception there, given that Mac UEFI firmware is 'special'
already.

2018-04-06 14:18:11

by Peter Jones

[permalink] [raw]

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

On Thu, Apr 05, 2018 at 07:43:49AM +0200, Lukas Wunner wrote:
> On Wed, Apr 04, 2018 at 01:18:36PM -0400, Peter Jones wrote:
> > > On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
> > > > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
> > > > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
> > > >
> > > > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
> > > > GUIDs you're interested in into memory and pass the files to the
> > > > kernel as setup_data payloads.
> >
> > To be honest, I'm a bit skeptical about the firmware volume approach.
> > Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
> > years, still don't seem to reliably parse firmware images I see in the
> > wild, and have a fairly regular need for fixes. These are tools
> > maintained by smart people who are making a real effort, and it still
> > looks pretty hard to do a good job that applies across a lot of
> > platforms.
> >
> > So I'd rather use Hans's existing patches, at least for now, and if
> > someone is interested in hacking on making an efi firmware volume parser
> > for the kernel, switch them to that when such a thing is ready.
>
> Hello? As I've written in the above-quoted e-mail the kernel should
> read the files using EFI_FIRMWARE_VOLUME_PROTOCOL.ReadFile().
>
> *Not* by parsing the firmware volume!

Okay, that's a fair point that I'd missed reading your first mail. It's
worth a shot. That said, EFI_FIRMWARE_VOLUME_PROTOCOL is part of the
PI spec, not the UEFI spec. It's not required in order to implement UEFI,
and some implementations don't use it. Even when the implementation
does include PI, there's no guarantee PI protocols are exposed after the
"End of DXE" event (though edk2 will expose this, I think).

> Parsing the firmware volume is only necessary to find out the GUIDs
> of the files you're looking for. You only do that *once*.
>
> I habitually extract Apple's EFI Firmware Volumes and found the
> existing tools always did a sufficiently good job to get the
> information I need (such as UEFIExtract, which I've linked to,
> and which is part of the UEFITool suite, which you've linked to
> once more).

Yeah, it's probably worth implementing your way and using it when we
can.

> On Wed, Apr 04, 2018 at 10:25:05PM +0200, Hans de Goede wrote:
> > Lukas, thank you for your suggestions on this, but I doubt that these
> > devices use the Firmware Volume stuff.
>
> If they're using EFI, they're using a Firmware Volume and you should
> be able to use the Firmware Volume Protocol to read files from it.

This is not actually the case. There is more than one implementation of
UEFI, and they do not all implement PEI/DXE/etc from the PI spec. For
two examples, there are still vendors that ship EDK-I implementations on
some platforms, and current u-boot can be built to export a UEFI API.
(It's a subset, but so is every other implementation.)

> If that protocol wasn't supported, the existing EFI drivers wouldn't
> be able to function as they couldn't load files from the volume.

This is not correct. Not all UEFI implementations implement *any* of
PI. Also, AFAIK, nothing we use in Linux so far directly depends on
anything in PI.

> > What you are describing sounds like significantly more work then
> > the vendor just embedding the firmware as a char firmware[] in their
> > EFI mouse driver.
>
> In such cases, read the EFI mouse driver from the firmware volume and
> extract the firmware from the offset you've pre-determined. That's
> never going to change since the devices never receive updates, as
> you're saying. So basically you'll have a struct with GUID, offset,
> length, and the name under which the firmware is made available to
> Linux drivers.

No, he's saying that it's really easy to implement a driver with the
device firmware in a char [] in the code, so cheap ODMs who supply a
UEFI driver with their hardware part are very, very likely to have done
that instead of providing two things to go into the FV - a UEFI driver
*and* a separate image for the device firmware. This also cuts out a
point of failure between the OEM and the ODM.

> > That combined with Peter's worries about difficulties parsing the
> > Firmware Volume stuff, makes me believe that it is best to just
> > stick with my current approach as Peter suggests.
>
> I don't know why you insist on a hackish solution (your own words)
> even though a cleaner solution is suggested, but fortunately it's
> not for me to decide what goes in and what doesn't. ;-)

It already works...

--
Peter

2018-04-06 14:30:13

by Ard Biesheuvel

[permalink] [raw]

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

On 6 April 2018 at 16:14, Ard Biesheuvel <[email protected]> wrote:
> On 6 April 2018 at 16:08, Luis R. Rodriguez <[email protected]> wrote:
>> On Thu, Apr 05, 2018 at 07:43:49AM +0200, Lukas Wunner wrote:
>>> On Wed, Apr 04, 2018 at 01:18:36PM -0400, Peter Jones wrote:
>>> > > On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
>>> > > > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
>>> > > > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
>>> > > >
>>> > > > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
>>> > > > GUIDs you're interested in into memory and pass the files to the
>>> > > > kernel as setup_data payloads.
>>> >
>>> > To be honest, I'm a bit skeptical about the firmware volume approach.
>>> > Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
>>> > years, still don't seem to reliably parse firmware images I see in the
>>> > wild, and have a fairly regular need for fixes. These are tools
>>> > maintained by smart people who are making a real effort, and it still
>>> > looks pretty hard to do a good job that applies across a lot of
>>> > platforms.
>>> >
>>> > So I'd rather use Hans's existing patches, at least for now, and if
>>> > someone is interested in hacking on making an efi firmware volume parser
>>> > for the kernel, switch them to that when such a thing is ready.
>>>
>>> Hello? As I've written in the above-quoted e-mail the kernel should
>>> read the files using EFI_FIRMWARE_VOLUME_PROTOCOL.ReadFile().
>>>
>>> *Not* by parsing the firmware volume!
>>>
>>> Parsing the firmware volume is only necessary to find out the GUIDs
>>> of the files you're looking for. You only do that *once*.
>>
>> How do you get the GUIDs for each driver BTW?
>>
>> Hans, I do believe we should *try* this approach at the very least.
>>
>> Why not?
>>
>> Otherwise it would be wise to provide a technical reason for why
>> we'd choose one custom mechanism which would only serve a few tablets
>> over a mechanism which could serve more devices.
>>
>
> Because EFI_FIRMWARE_VOLUME_PROTOCOL is not part of the UEFI spec but
> of the PI spec, and so we will be adding dependencies on
> implementation details of the firmware. I am aware we may already have
> done so for the Apple properties support

... or maybe not: I thought Lukas alluded to that in this thread, but
I can't actually find any traces of that in the code so I must have
misunderstood.

, but I think it makes sense
> to make an exception there, given that Mac UEFI firmware is 'special'
> already.

2018-04-07 09:54:44

by Lukas Wunner

[permalink] [raw]

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

On Fri, Apr 06, 2018 at 02:08:32PM +0000, Luis R. Rodriguez wrote:
> How do you get the GUIDs for each driver BTW?

They're used as filenames when extracting a Firmware Volume with
UEFIExtract.

E.g. let's say I'm looking for the EFI driver containing the UCS-2
string "ThunderboltDROM" in the MacBookPro9,1 Firmware Volume:

$ UEFIExtract MBP91_00D3_B0C_LOCKED.scap
$ grep -rl T.h.u.n.d.e.r.b.o.l.t.D.R.O.M MBP91_00D3_B0C_LOCKED.scap.dump
MBP91_00D3_B0C_LOCKED.scap.dump/0 UEFI image/0 7A9354D9-0468-444A-81CE-0BF617D890DF/4 283FA2EE-532C-484D-9383-9F93B36F0B7E/0 7A9354D9-0468-444A-81CE-0BF617D890DF/0 77AD7FDB-DF2A-4302-8898-C72E4CDBD0F4/0 Compressed section/0 FC1BCDB0-7D31-49AA-936A-A4600D9DD083/0 Volume image section/0 7A9354D9-0468-444A-81CE-0BF617D890DF/97 9D1A8B60-6CB0-11DE-8E91-0002A5D5C51B/0 Compressed section/0 FC1BCDB0-7D31-49AA-936A-A4600D9DD083/0 PE32 image section/body.bin

That file can then be examined with a hex editor, disassembler, etc.

Since Hans knows the 8 byte signature of the file he's looking for,
he'd just use:

$ grep -rl '\x01\x23\x34\x56\x78\x9a\xbc\xde' ...

(The \x syntax works with BSD grep shipping with macOS, but not with
GNU grep. pcregrep should work, or grep -P, though the latter has
been unreliable for me.)

Pretty trivial obviously, the problem is how do you get the Firmware
Volume? Apple ships firmware blobs for all supported machines as part
of their OS updates, but if the vendor never provides updates (as Hans
wrote), your only chance is to dump the ROM. On Macs the ROM is at
physical address 0xffe00000. The rEFIt bootloader contains a "fvdump"
tool which can be used to dump the Firmware Volume at this address to
the ESP:
https://github.com/yep/refit/blob/master/refit/dumpfv/dumpfv.c

The tool dumps only 2 MByte, but contempary Macs use considerably larger
firmware blobs (8 MByte+). A quick Google search turns up these
alternative addresses where the ROM may be located:
https://github.com/tianocore/edk2/blob/master/OvmfPkg/README

"The base address for the 1MB image in QEMU physical memory is
0xfff00000. The base address for the 2MB image is 0xffe00000.
The base address for the 4MB image is 0xffc00000."

> Otherwise it would be wise to provide a technical reason for why
> we'd choose one custom mechanism which would only serve a few tablets
> over a mechanism which could serve more devices.

An advantage of the approach I'm suggesting is that it also catches
firmware even if the EFI driver wasn't loaded. There may even be
firmware for devices that aren't present, vendors ship lots of
surprising stuff on Firmware Volumes. (I've found a FireWire driver
on the 12" MacBook8,1 volume, even though the machine has neither a
FireWire port, nor a Thunderbolt port that you could connect a
FireWire adapter to. They were probably just too lazy to constrain
the firmware contents to what's actually needed on a specific machine.)

On Fri, Apr 06, 2018 at 10:16:44AM -0400, Peter Jones wrote:
> That said, EFI_FIRMWARE_VOLUME_PROTOCOL is part of the
> PI spec, not the UEFI spec. It's not required in order to implement UEFI,
> and some implementations don't use it. Even when the implementation
> does include PI, there's no guarantee PI protocols are exposed after the
> "End of DXE" event (though edk2 will expose this, I think).

Thanks for pointing that out, I wasn't aware of it.

On Fri, Apr 06, 2018 at 04:28:49PM +0200, Ard Biesheuvel wrote:
> > EFI_FIRMWARE_VOLUME_PROTOCOL is not part of the UEFI spec but
> > of the PI spec, and so we will be adding dependencies on
> > implementation details of the firmware. I am aware we may already have
> > done so for the Apple properties support
>
> ... or maybe not: I thought Lukas alluded to that in this thread, but
> I can't actually find any traces of that in the code so I must have
> misunderstood.

Retrieval of Apple device properties is done using a custom Apple
protocol, it doesn't involve the EFI_FIRMWARE_VOLUME_PROTOCOL.

What I meant to say is, the EFI stub already stashes PCI ROMs and
Apple device properties in setup_data payloads for consumption by
the kernel proper, so extending that pattern to retrieval of
device firmware (using EFI_FIRMWARE_VOLUME_PROTOCOL) seems kind of
natural.

Thanks,

Lukas

2018-04-07 11:17:46