[PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Hans de Goede]

Hi All,

Here is v7 of my patch-set to add support for EFI embedded fw to the kernel.

v6 was posted a long time ago, around the 4.18 days. The long wait was for
a suitable secure-hash for checking the firmware we find embedded in the EFI
is the one we expect.

With 5.4-rc1 we finally have a standalone sha256 lib, so that hurdle for
this patch-set is now gone.

I've tried to address all review-remarks against v6 in this new version:

Changes in v7:
- Split drivers/firmware/efi and drivers/base/firmware_loader changes into
  2 patches
- Use new, standalone, lib/crypto/sha256.c code
- Address kdoc comments from Randy Dunlap
- Add new FW_OPT_FALLBACK_PLATFORM flag and firmware_request_platform()
  _request_firmware() wrapper, as requested by Luis R. Rodriguez
- Stop using "efi-embedded-firmware" device-property, now that drivers need to
  use the new firmware_request_platform() to enable fallback to a device fw
  copy embedded in the platform's main firmware, we no longer need a property
  on the device to trigger this behavior
- Use security_kernel_load_data instead of calling
  security_kernel_read_file with a NULL file pointer argument
- Move the docs to Documentation/driver-api/firmware/fallback-mechanisms.rst
- Document the new firmware_request_platform() function in
  Documentation/driver-api/firmware/request_firmware.rst
- Add 2 new patches for the silead and chipone-icn8505 touchscreen drivers
  to use the new firmware_request_platform() method
- Rebased on top of 5.4-rc1

I guess this will probably need another round (ot two) of review + fixing,
but eventually this can hopefully be merged. Since this touches a bunch
of different subsystems the question is how to merge this? Most of the
touched files outside of the firmware-loader code do not see a lot of
churn, so my proposal would be to merge patches 1-6 through the tree
which carries firmware-loader changes; and then provide an immutable
branch for the platform/x86 maintainers to merge and then they can merge
the last 2 patches (as the touchscreen_dmi.c file does see quite a bit
of changes every release).

Regards,

Hans

[PATCH v7 1/8] efi: Export boot-services code and data as debugfs-blobs

[Hans de Goede]

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.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

Changes in v4:
-Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
 memory segments are available (and thus if it makes sense to register the
 debugfs bits for them)

Changes in v2:
-Do not call pr_err on debugfs call failures
---
 arch/x86/platform/efi/efi.c    |  1 +
 arch/x86/platform/efi/quirks.c |  4 +++
 drivers/firmware/efi/efi.c     | 53 ++++++++++++++++++++++++++++++++++
 include/linux/efi.h            |  1 +
 4 files changed, 59 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index c202e1b07e29..847730f7e74b 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -232,6 +232,7 @@ int __init efi_memblock_x86_reserve_range(void)
 	     efi.memmap.desc_version);
 
 	memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
+	set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags);
 
 	return 0;
 }
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 3b9fd679cea9..fab12ebf0ada 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -411,6 +411,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 8d3e778e988b..abba49c4c46d 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -17,6 +17,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>
@@ -314,6 +315,55 @@ 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
+
+static 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))
+		return;
+
+	for_each_efi_memory_desc(md) {
+		switch (md->type) {
+		case EFI_BOOT_SERVICES_CODE:
+			snprintf(name, sizeof(name), "boot_services_code%d",
+				 type_count[md->type]++);
+			break;
+		case EFI_BOOT_SERVICES_DATA:
+			snprintf(name, sizeof(name), "boot_services_data%d",
+				 type_count[md->type]++);
+			break;
+		default:
+			continue;
+		}
+
+		debugfs_blob[i].size = md->num_pages << EFI_PAGE_SHIFT;
+		debugfs_blob[i].data = memremap(md->phys_addr,
+						debugfs_blob[i].size,
+						MEMREMAP_WB);
+		if (!debugfs_blob[i].data)
+			continue;
+
+		debugfs_create_blob(name, 0400, efi_debugfs, &debugfs_blob[i]);
+		i++;
+		if (i == EFI_DEBUGFS_MAX_BLOBS)
+			break;
+	}
+}
+#else
+static inline void efi_debugfs_init(void) {}
+#endif
+
 /*
  * We register the efi subsystem with the firmware subsystem and the
  * efivars subsystem with the efi subsystem, if the system was booted with
@@ -370,6 +420,9 @@ static int __init efisubsys_init(void)
 		goto err_remove_group;
 	}
 
+	if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
+		efi_debugfs_init();
+
 	return 0;
 
 err_remove_group:
diff --git a/include/linux/efi.h b/include/linux/efi.h
index bd3837022307..2a30a1bd8bdf 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1202,6 +1202,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_DBG			8	/* Print additional debug info at runtime */
 #define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
 #define EFI_MEM_ATTR		10	/* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
+#define EFI_PRESERVE_BS_REGIONS	11	/* Are EFI boot-services memory segments available? */
 
 #ifdef CONFIG_EFI
 /*
-- 
2.23.0

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

[Hans de Goede]

Just like with PCI options ROMs, which we save in the setup_efi_pci*
functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself
sometimes may contain data which is useful/necessary for peripheral drivers
to have access to.

Specifically the EFI code may contain an embedded copy of firmware which
needs to be (re)loaded into the peripheral. Normally such firmware would be
part of linux-firmware, but in some cases this is not feasible, for 2
reasons:

1) The firmware is customized for a specific use-case of the chipset / use
with a specific hardware model, so we cannot have a single firmware file
for the chipset. E.g. touchscreen controller firmwares are compiled
specifically for the hardware model they are used with, as they are
calibrated for a specific model digitizer.

2) Despite repeated attempts we have failed to get permission to
redistribute the firmware. This is especially a problem with customized
firmwares, these get created by the chip vendor for a specific ODM and the
copyright may partially belong with the ODM, so the chip vendor cannot
give a blanket permission to distribute these.

This commit adds support for finding peripheral firmware embedded in the
EFI code and makes the found firmware available through the new
efi_get_embedded_fw() function.

Support for loading these firmwares through the standard firmware loading
mechanism is added in a follow-up commit in this patch-series.

Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end
of start_kernel(), just before calling rest_init(), this is on purpose
because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for
early_memremap(), so the check must be done after mm_init(). This relies
on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services()
is called, which means that this will only work on x86 for now.

Reported-by: Dave Olsthoorn <dave@bewaar.me>
Suggested-by: Peter Jones <pjones@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v7:
- Split drivers/firmware/efi and drivers/base/firmware_loader changes into
  2 patches
- Use new, standalone, lib/crypto/sha256.c code

Changes in v6:
- Rework code to remove casts from if (prefix == mem) comparison
- Use SHA256 hashes instead of crc32 sums
- Add new READING_FIRMWARE_EFI_EMBEDDED read_file_id and use it
- Call security_kernel_read_file(NULL, READING_FIRMWARE_EFI_EMBEDDED)
  to check if this is allowed before looking at EFI embedded fw
- Document why we are not using the UEFI PI Firmware Volume protocol

Changes in v5:
- Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

Changes in v4:
- Drop note in docs about EFI_FIRMWARE_VOLUME_PROTOCOL, it is not part of
  UEFI proper, so the EFI maintainers don't want us referring people to it
- Use new EFI_BOOT_SERVICES flag
- Put the new fw_get_efi_embedded_fw() function in its own fallback_efi.c
  file which only gets built when EFI_EMBEDDED_FIRMWARE is selected
- Define an empty stub for fw_get_efi_embedded_fw() in fallback.h hwen
  EFI_EMBEDDED_FIRMWARE is not selected, to avoid the need for #ifdefs
  in firmware_loader/main.c
- Properly call security_kernel_post_read_file() on the firmware returned
  by efi_get_embedded_fw() to make sure that we are allowed to use it

Changes in v3:
- Fix the docs using "efi-embedded-fw" as property name instead of
  "efi-embedded-firmware"

Changes in v2:
- Rebased on driver-core/driver-core-next
- Add documentation describing the EFI embedded firmware mechanism to:
  Documentation/driver-api/firmware/request_firmware.rst
- Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded
  fw support if this is set. This is an invisible option which should be
  selected by drivers which need this
- Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices
  from the efi-embedded-fw code, instead drivers using this are expected to
  export a dmi_system_id array, with each entries' driver_data pointing to a
  efi_embedded_fw_desc struct and register this with the efi-embedded-fw code
- Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware,
  this avoids us messing with the EFI memmap and avoids the need to make
  changes to efi_mem_desc_lookup()
- Make the firmware-loader code only fallback to efi_get_embedded_fw() if the
  passed in device has the "efi-embedded-firmware" device-property bool set
- Skip usermodehelper fallback when "efi-embedded-firmware" device-property
  is set
---
 arch/x86/platform/efi/efi.c              |   1 +
 drivers/firmware/efi/Kconfig             |   4 +
 drivers/firmware/efi/Makefile            |   1 +
 drivers/firmware/efi/embedded-firmware.c | 143 +++++++++++++++++++++++
 include/linux/efi.h                      |   6 +
 include/linux/efi_embedded_fw.h          |  25 ++++
 6 files changed, 180 insertions(+)
 create mode 100644 drivers/firmware/efi/embedded-firmware.c
 create mode 100644 include/linux/efi_embedded_fw.h

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 847730f7e74b..5db2cc011dc1 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -1019,6 +1019,7 @@ static void __init __efi_enter_virtual_mode(void)
 		panic("EFI call to SetVirtualAddressMap() failed!");
 	}
 
+	efi_check_for_embedded_firmwares();
 	efi_free_boot_services();
 
 	/*
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 178ee8106828..c2c003326265 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -193,6 +193,10 @@ config EFI_RCI2_TABLE
 
 	  Say Y here for Dell EMC PowerEdge systems.
 
+config EFI_EMBEDDED_FIRMWARE
+	bool
+	select CRYPTO_LIB_SHA256
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 4ac2de4dfa72..42bd310657f4 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_EFI_TEST)			+= test/
 obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
 obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
 obj-$(CONFIG_EFI_RCI2_TABLE)		+= rci2-table.o
+obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE)	+= embedded-firmware.o
 
 arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
 obj-$(CONFIG_ARM)			+= $(arm-obj-y)
diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
new file mode 100644
index 000000000000..75d652f3148b
--- /dev/null
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for extracting embedded firmware for peripherals from EFI code,
+ *
+ * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
+ */
+
+#include <linux/dmi.h>
+#include <linux/efi.h>
+#include <linux/efi_embedded_fw.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/vmalloc.h>
+#include <crypto/sha.h>
+
+struct embedded_fw {
+	struct list_head list;
+	const char *name;
+	void *data;
+	size_t length;
+};
+
+static LIST_HEAD(found_fw_list);
+
+static const struct dmi_system_id * const embedded_fw_table[] = {
+	NULL
+};
+
+/*
+ * Note the efi_check_for_embedded_firmwares() code currently makes the
+ * following 2 assumptions. This may needs to be revisited if embedded firmware
+ * is found where this is not true:
+ * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
+ * 2) The firmware always starts at an offset which is a multiple of 8 bytes
+ */
+static int __init efi_check_md_for_embedded_firmware(
+	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
+{
+	const u64 prefix = *((u64 *)desc->prefix);
+	struct sha256_state sctx;
+	struct embedded_fw *fw;
+	u8 sha256[32];
+	u64 i, size;
+	void *map;
+
+	size = md->num_pages << EFI_PAGE_SHIFT;
+	map = memremap(md->phys_addr, size, MEMREMAP_WB);
+	if (!map) {
+		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
+		return -ENOMEM;
+	}
+
+	size -= desc->length;
+	for (i = 0; i < size; i += 8) {
+		u64 *mem = map + i;
+
+		if (*mem != prefix)
+			continue;
+
+		sha256_init(&sctx);
+		sha256_update(&sctx, map + i, desc->length);
+		sha256_final(&sctx, sha256);
+		if (memcmp(sha256, desc->sha256, 32) == 0)
+			break;
+	}
+	if (i >= size) {
+		memunmap(map);
+		return -ENOENT;
+	}
+
+	pr_info("Found EFI embedded fw '%s'\n", desc->name);
+
+	fw = kmalloc(sizeof(*fw), GFP_KERNEL);
+	if (!fw) {
+		memunmap(map);
+		return -ENOMEM;
+	}
+
+	fw->data = kmemdup(map + i, desc->length, GFP_KERNEL);
+	memunmap(map);
+	if (!fw->data) {
+		kfree(fw);
+		return -ENOMEM;
+	}
+
+	fw->name = desc->name;
+	fw->length = desc->length;
+	list_add(&fw->list, &found_fw_list);
+
+	return 0;
+}
+
+void __init efi_check_for_embedded_firmwares(void)
+{
+	const struct efi_embedded_fw_desc *fw_desc;
+	const struct dmi_system_id *dmi_id;
+	efi_memory_desc_t *md;
+	int i, r;
+
+	for (i = 0; embedded_fw_table[i]; i++) {
+		dmi_id = dmi_first_match(embedded_fw_table[i]);
+		if (!dmi_id)
+			continue;
+
+		fw_desc = dmi_id->driver_data;
+		for_each_efi_memory_desc(md) {
+			if (md->type != EFI_BOOT_SERVICES_CODE)
+				continue;
+
+			r = efi_check_md_for_embedded_firmware(md, fw_desc);
+			if (r == 0)
+				break;
+		}
+	}
+}
+
+int efi_get_embedded_fw(const char *name, void **data, size_t *size)
+{
+	struct embedded_fw *iter, *fw = NULL;
+	void *buf = *data;
+
+	list_for_each_entry(iter, &found_fw_list, list) {
+		if (strcmp(name, iter->name) == 0) {
+			fw = iter;
+			break;
+		}
+	}
+
+	if (!fw)
+		return -ENOENT;
+
+	buf = vmalloc(fw->length);
+	if (!buf)
+		return -ENOMEM;
+
+	memcpy(buf, fw->data, fw->length);
+	*size = fw->length;
+	*data = buf;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2a30a1bd8bdf..429634be3ecf 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1632,6 +1632,12 @@ static inline void
 efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
 #endif
 
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+void efi_check_for_embedded_firmwares(void);
+#else
+static inline void efi_check_for_embedded_firmwares(void) { }
+#endif
+
 void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
 
 /*
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
new file mode 100644
index 000000000000..ac70ff146d58
--- /dev/null
+++ b/include/linux/efi_embedded_fw.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_EFI_EMBEDDED_FW_H
+#define _LINUX_EFI_EMBEDDED_FW_H
+
+#include <linux/mod_devicetable.h>
+
+/**
+ * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
+ *                               code to search for embedded firmwares.
+ *
+ * @name:   Name to register the firmware with if found
+ * @prefix: First 8 bytes of the firmware
+ * @length: Length of the firmware in bytes including prefix
+ * @sha256: SHA256 of the firmware
+ */
+struct efi_embedded_fw_desc {
+	const char *name;
+	u8 prefix[8];
+	u32 length;
+	u8 sha256[32];
+};
+
+int efi_get_embedded_fw(const char *name, void **dat, size_t *sz);
+
+#endif
-- 
2.23.0

[PATCH v7 3/8] firmware: Rename FW_OPT_NOFALLBACK to FW_OPT_NOFALLBACK_SYSFS

[Hans de Goede]

This is a preparation patch for adding a new platform fallback mechanism,
which will have its own enable/disable FW_OPT_xxx option.

Note this also fixes a typo in one of the re-wordwrapped comments:
enfoce -> enforce.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/base/firmware_loader/fallback.c | 11 ++++++-----
 drivers/base/firmware_loader/firmware.h | 16 ++++++++--------
 drivers/base/firmware_loader/main.c     |  2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 62ee90b4db56..665b350419cb 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -606,7 +606,7 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
 		return false;
 	}
 
-	if ((opt_flags & FW_OPT_NOFALLBACK))
+	if ((opt_flags & FW_OPT_NOFALLBACK_SYSFS))
 		return false;
 
 	/* Also permit LSMs and IMA to fail firmware sysfs fallback */
@@ -630,10 +630,11 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
  * interface. Userspace is in charge of loading the firmware through the sysfs
  * loading interface. This sysfs fallback mechanism may be disabled completely
  * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
- * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
- * flag, if so it would also disable the fallback mechanism. A system may want
- * to enfoce the sysfs fallback mechanism at all times, it can do this by
- * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
+ * If this false we check if the internal API caller set the
+ * @FW_OPT_NOFALLBACK_SYSFS flag, if so it would also disable the fallback
+ * mechanism. A system may want to enforce the sysfs fallback mechanism at all
+ * times, it can do this by setting ignore_sysfs_fallback to false and
+ * force_sysfs_fallback to true.
  * Enabling force_sysfs_fallback is functionally equivalent to build a kernel
  * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
  **/
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 7ecd590e67fe..8656e5239a80 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -27,16 +27,16 @@
  *	firmware file lookup on storage is avoided. Used for calls where the
  *	file may be too big, or where the driver takes charge of its own
  *	firmware caching mechanism.
- * @FW_OPT_NOFALLBACK: Disable the fallback mechanism. Takes precedence over
- *	&FW_OPT_UEVENT and &FW_OPT_USERHELPER.
+ * @FW_OPT_NOFALLBACK_SYSFS: Disable the sysfs fallback mechanism. Takes
+ *	precedence over &FW_OPT_UEVENT and &FW_OPT_USERHELPER.
  */
 enum fw_opt {
-	FW_OPT_UEVENT =         BIT(0),
-	FW_OPT_NOWAIT =         BIT(1),
-	FW_OPT_USERHELPER =     BIT(2),
-	FW_OPT_NO_WARN =        BIT(3),
-	FW_OPT_NOCACHE =        BIT(4),
-	FW_OPT_NOFALLBACK =     BIT(5),
+	FW_OPT_UEVENT			= BIT(0),
+	FW_OPT_NOWAIT			= BIT(1),
+	FW_OPT_USERHELPER		= BIT(2),
+	FW_OPT_NO_WARN			= BIT(3),
+	FW_OPT_NOCACHE			= BIT(4),
+	FW_OPT_NOFALLBACK_SYSFS		= BIT(5),
 };
 
 enum fw_status {
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index bf44c79beae9..08f8995a530a 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -876,7 +876,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
 				FW_OPT_UEVENT | FW_OPT_NO_WARN |
-				FW_OPT_NOFALLBACK);
+				FW_OPT_NOFALLBACK_SYSFS);
 	module_put(THIS_MODULE);
 	return ret;
 }
-- 
2.23.0

[PATCH v7 4/8] firmware: Add new platform fallback mechanism and firmware_request_platform()

[Hans de Goede]

In some cases the platform's main firmware (e.g. the UEFI fw) may contain
an embedded copy of device 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 a new platform fallback mechanism to the firmware loader
which will try to lookup a device fw copy embedded in the platform's main
firmware if direct filesystem lookup fails.

Drivers which need such embedded fw copies can enable this fallback
mechanism by using the new firmware_request_platform() function.

Note that for now this is only supported on EFI platforms and even on
these platforms firmware_fallback_platform() only works if
CONFIG_EFI_EMBEDDED_FIRMWARE is enabled (this gets selected by drivers
which need this), in all other cases firmware_fallback_platform() simply
always returns -ENOENT.

Reported-by: Dave Olsthoorn <dave@bewaar.me>
Suggested-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v7:
- Split drivers/firmware/efi and drivers/base/firmware_loader changes into
  2 patches
- Address kdoc comments from Randy Dunlap
- Add new FW_OPT_FALLBACK_PLATFORM flag and firmware_request_platform()
  _request_firmware() wrapper, as requested by Luis R. Rodriguez
- Stop using "efi-embedded-firmware" device-property, now that drivers need to
  use the new firmware_request_platform() to enable fallback to a device fw
  copy embedded in the platform's main firmware, we no longer need a property
  on the device to trigger this behavior
- Use security_kernel_load_data instead of calling
  security_kernel_read_file with a NULL file pointer argument
- Move the docs to Documentation/driver-api/firmware/fallback-mechanisms.rst
- Document the new firmware_request_platform() function in
  Documentation/driver-api/firmware/request_firmware.rst

Changes in v6:
- Rework code to remove casts from if (prefix == mem) comparison
- Use SHA256 hashes instead of crc32 sums
- Add new READING_FIRMWARE_EFI_EMBEDDED read_file_id and use it
- Call security_kernel_read_file(NULL, READING_FIRMWARE_EFI_EMBEDDED)
  to check if this is allowed before looking at EFI embedded fw
- Document why we are not using the UEFI PI Firmware Volume protocol

Changes in v5:
- Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

Changes in v4:
- Drop note in docs about EFI_FIRMWARE_VOLUME_PROTOCOL, it is not part of
  UEFI proper, so the EFI maintainers don't want us referring people to it
- Use new EFI_BOOT_SERVICES flag
- Put the new fw_get_efi_embedded_fw() function in its own fallback_efi.c
  file which only gets built when EFI_EMBEDDED_FIRMWARE is selected
- Define an empty stub for fw_get_efi_embedded_fw() in fallback.h hwen
  EFI_EMBEDDED_FIRMWARE is not selected, to avoid the need for #ifdefs
  in firmware_loader/main.c
- Properly call security_kernel_post_read_file() on the firmware returned
  by efi_get_embedded_fw() to make sure that we are allowed to use it

Changes in v3:
- Fix the docs using "efi-embedded-fw" as property name instead of
  "efi-embedded-firmware"

Changes in v2:
- Rebased on driver-core/driver-core-next
- Add documentation describing the EFI embedded firmware mechanism to:
  Documentation/driver-api/firmware/request_firmware.rst
- Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded
  fw support if this is set. This is an invisible option which should be
  selected by drivers which need this
- Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices
  from the efi-embedded-fw code, instead drivers using this are expected to
  export a dmi_system_id array, with each entries' driver_data pointing to a
  efi_embedded_fw_desc struct and register this with the efi-embedded-fw code
- Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware,
  this avoids us messing with the EFI memmap and avoids the need to make
  changes to efi_mem_desc_lookup()
- Make the firmware-loader code only fallback to efi_get_embedded_fw() if the
  passed in device has the "efi-embedded-firmware" device-property bool set
- Skip usermodehelper fallback when "efi-embedded-firmware" device-property
  is set
---
 .../firmware/fallback-mechanisms.rst          | 71 +++++++++++++++++++
 .../driver-api/firmware/lookup-order.rst      |  2 +
 .../driver-api/firmware/request_firmware.rst  |  5 ++
 drivers/base/firmware_loader/Makefile         |  2 +-
 drivers/base/firmware_loader/fallback.h       |  2 +
 .../base/firmware_loader/fallback_platform.c  | 33 +++++++++
 drivers/base/firmware_loader/firmware.h       |  4 ++
 drivers/base/firmware_loader/main.c           | 27 +++++++
 include/linux/firmware.h                      |  2 +
 include/linux/fs.h                            |  1 +
 10 files changed, 148 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/firmware_loader/fallback_platform.c

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index 8b041d0ab426..954a6b0ded40 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -202,3 +202,74 @@ the following file:
 
 If you echo 0 into it means MAX_JIFFY_OFFSET will be used. The data type
 for the timeout is an int.
+
+EFI embedded firmware fallback mechanism
+========================================
+
+On some devices the system's EFI code / ROM may contain an embedded copy
+of firmware for some of the system's integrated peripheral devices and
+the peripheral's Linux device-driver needs to access this firmware.
+
+Device drivers which need such firmware can use the
+firmware_request_platform() function for this, note that this is a
+separate fallback mechanism from the other fallback mechanisms and
+this does not use the sysfs interface.
+
+A device driver which needs this can describe the firmware it needs
+using an efi_embedded_fw_desc struct:
+
+.. kernel-doc:: include/linux/efi_embedded_fw.h
+   :functions: efi_embedded_fw_desc
+
+The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE memory
+segments for an eight byte sequence matching prefix; if the prefix is found it
+then does a sha256 over length bytes and if that matches makes a copy of length
+bytes and adds that to its list with found firmwares.
+
+To avoid doing this somewhat expensive scan on all systems, dmi matching is
+used. Drivers are expected to export a dmi_system_id array, with each entries'
+driver_data pointing to an efi_embedded_fw_desc.
+
+To register this array with the efi-embedded-fw code, a driver needs to:
+
+1. Always be builtin to the kernel or store the dmi_system_id array in a
+   separate object file which always gets builtin.
+
+2. Add an extern declaration for the dmi_system_id array to
+   include/linux/efi_embedded_fw.h.
+
+3. Add the dmi_system_id array to the embedded_fw_table in
+   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
+   the driver is being builtin.
+
+4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
+
+The firmware_request_platform() function will always first try to load firmware
+with the specified name directly from the disk, so the EFI embedded-fw can
+always be overridden by placing a file under /lib/firmware.
+
+Note that:
+
+1. The code scanning for EFI embedded-firmware runs near the end
+   of start_kernel(), just before calling rest_init(). For normal drivers and
+   subsystems using subsys_initcall() to register themselves this does not
+   matter. This means that code running earlier cannot use EFI
+   embedded-firmware.
+
+2. At the moment the EFI embedded-fw code assumes that firmwares always start at
+   an offset which is a multiple of 8 bytes, if this is not true for your case
+   send in a patch to fix this.
+
+3. At the moment the EFI embedded-fw code only works on x86 because other archs
+   free EFI_BOOT_SERVICES_CODE before the EFI embedded-fw code gets a chance to
+   scan it.
+
+4. The current brute-force scanning of EFI_BOOT_SERVICES_CODE is an ad-hoc
+   brute-force solution. There has been discussion to use the UEFI Platform
+   Initialization (PI) spec's Firmware Volume protocol. This has been rejected
+   because the FV Protocol relies on *internal* interfaces of the PI spec, and:
+   1. The PI spec does not define peripheral firmware at all
+   2. The internal interfaces of the PI spec does not guarantee any backward
+   compatibility. Any implementation details in FV may be subject to change,
+   and may vary system to system. Supporting the FV Protocol would be
+   difficult as it is purposely ambiguous.
diff --git a/Documentation/driver-api/firmware/lookup-order.rst b/Documentation/driver-api/firmware/lookup-order.rst
index 88c81739683c..6064672a782e 100644
--- a/Documentation/driver-api/firmware/lookup-order.rst
+++ b/Documentation/driver-api/firmware/lookup-order.rst
@@ -12,6 +12,8 @@ a driver issues a firmware API call.
   return it immediately
 * The ''Direct filesystem lookup'' is performed next, if found we
   return it immediately
+* The ''Platform firmware fallback'' is performed next, but only when
+  firmware_request_platform() is used, if found we return it immediately
 * If no firmware has been found and the fallback mechanism was enabled
   the sysfs interface is created. After this either a kobject uevent
   is issued or the custom firmware loading is relied upon for firmware
diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index f62bdcbfed5b..cd076462d235 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -25,6 +25,11 @@ firmware_request_nowarn
 .. kernel-doc:: drivers/base/firmware_loader/main.c
    :functions: firmware_request_nowarn
 
+firmware_request_platform
+-------------------------
+.. kernel-doc:: drivers/base/firmware_loader/main.c
+   :functions: firmware_request_platform
+
 request_firmware_direct
 -----------------------
 .. kernel-doc:: drivers/base/firmware_loader/main.c
diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
index 0b2dfa6259c9..fec75895faae 100644
--- a/drivers/base/firmware_loader/Makefile
+++ b/drivers/base/firmware_loader/Makefile
@@ -3,7 +3,7 @@
 
 obj-$(CONFIG_FW_LOADER_USER_HELPER) += fallback_table.o
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
-firmware_class-objs := main.o
+firmware_class-objs := main.o fallback_platform.o
 firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
 
 obj-y += builtin/
diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
index 21063503e4ea..c4350f2e7cc2 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -66,4 +66,6 @@ static inline void unregister_sysfs_loader(void)
 }
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
+int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags);
+
 #endif /* __FIRMWARE_FALLBACK_H */
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
new file mode 100644
index 000000000000..7e9d730e36bf
--- /dev/null
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/efi_embedded_fw.h>
+#include <linux/property.h>
+#include <linux/security.h>
+#include <linux/vmalloc.h>
+
+#include "fallback.h"
+#include "firmware.h"
+
+int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags)
+{
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+	int rc;
+
+	if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
+		return -ENOENT;
+
+	rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+	if (rc)
+		return rc;
+
+	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data,
+				 &fw_priv->size);
+	if (rc)
+		return rc; /* rc == -ENOENT when the fw was not found */
+
+	fw_state_done(fw_priv);
+	return 0;
+#else
+	return -ENOENT;
+#endif
+}
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 8656e5239a80..25836a6afc9f 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -29,6 +29,9 @@
  *	firmware caching mechanism.
  * @FW_OPT_NOFALLBACK_SYSFS: Disable the sysfs fallback mechanism. Takes
  *	precedence over &FW_OPT_UEVENT and &FW_OPT_USERHELPER.
+ * @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in
+ *	the platform's main firmware. If both this fallback and the sysfs
+ *      fallback are enabled, then this fallback will be tried first.
  */
 enum fw_opt {
 	FW_OPT_UEVENT			= BIT(0),
@@ -37,6 +40,7 @@ enum fw_opt {
 	FW_OPT_NO_WARN			= BIT(3),
 	FW_OPT_NOCACHE			= BIT(4),
 	FW_OPT_NOFALLBACK_SYSFS		= BIT(5),
+	FW_OPT_FALLBACK_PLATFORM	= BIT(6),
 };
 
 enum fw_status {
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 08f8995a530a..006ff71458b1 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -775,6 +775,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 						 fw_decompress_xz);
 #endif
 
+	if (ret == -ENOENT)
+		ret = firmware_fallback_platform(fw->priv, opt_flags);
+
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
@@ -882,6 +885,30 @@ int request_firmware_direct(const struct firmware **firmware_p,
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * firmware_request_platform() - request firmware with platform-fw fallback
+ * @firmware: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware, except that if
+ * direct filesystem lookup fails, it will fallback to looking for a copy of the
+ * requested firmware embedded in the platform's main (e.g. UEFI) firmware.
+ **/
+int firmware_request_platform(const struct firmware **firmware,
+			      const char *name, struct device *device)
+{
+	int ret;
+
+	/* Need to pin this module until return */
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware, name, device, NULL, 0,
+				FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_platform);
+
 /**
  * firmware_request_cache() - cache firmware for suspend so resume can use it
  * @name: name of firmware file
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 2dd566c91d44..75dbec0bcc06 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -44,6 +44,8 @@ int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
 int firmware_request_nowarn(const struct firmware **fw, const char *name,
 			    struct device *device);
+int firmware_request_platform(const struct firmware **fw, const char *name,
+			      struct device *device);
 int request_firmware_nowait(
 	struct module *module, bool uevent,
 	const char *name, struct device *device, gfp_t gfp, void *context,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e0d909d35763..3cbc955f6a1a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2961,6 +2961,7 @@ extern int do_pipe_flags(int *, int);
 	id(UNKNOWN, unknown)		\
 	id(FIRMWARE, firmware)		\
 	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
+	id(FIRMWARE_EFI_EMBEDDED, firmware)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
-- 
2.23.0

[PATCH v7 5/8] Input: silead - Switch to firmware_request_platform for retreiving the fw

[Hans de Goede]

Unfortunately sofar we have been unable to get permission to redistribute
Silead touchscreen firmwares in linux-firmware. This means that people
need to find and install the firmware themselves before the touchscreen
will work

Some UEFI/x86 tablets with a Silead touchscreen have a copy of the fw
embedded in their UEFI boot-services code.

This commit makes the silead driver use the new firmware_request_platform
function, which will fallback to looking for such an embedded copy when
direct filesystem lookup fails. This will make the touchscreen work OOTB
on devices where there is a fw copy embedded in the UEFI code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/silead.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index ad8b6a2bfd36..8fa2f3b7cfd8 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -288,7 +288,7 @@ static int silead_ts_load_fw(struct i2c_client *client)
 
 	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
 
-	error = request_firmware(&fw, data->fw_name, dev);
+	error = firmware_request_platform(&fw, data->fw_name, dev);
 	if (error) {
 		dev_err(dev, "Firmware request error %d\n", error);
 		return error;
-- 
2.23.0

[PATCH v7 6/8] Input: icn8505 - Switch to firmware_request_platform for retreiving the fw

[Hans de Goede]

Unfortunately sofar we have been unable to get permission to redistribute
icn8505 touchscreen firmwares in linux-firmware. This means that people
need to find and install the firmware themselves before the touchscreen
will work

Some UEFI/x86 tablets with an icn8505 touchscreen have a copy of the fw
embedded in their UEFI boot-services code.

This commit makes the icn8505 driver use the new firmware_request_platform
function, which will fallback to looking for such an embedded copy when
direct filesystem lookup fails. This will make the touchscreen work OOTB
on devices where there is a fw copy embedded in the UEFI code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/chipone_icn8505.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c
index c768186ce856..f9ca5502ac8c 100644
--- a/drivers/input/touchscreen/chipone_icn8505.c
+++ b/drivers/input/touchscreen/chipone_icn8505.c
@@ -288,7 +288,7 @@ static int icn8505_upload_fw(struct icn8505_data *icn8505)
 	 * we may need it at resume. Having loaded it once will make the
 	 * firmware class code cache it at suspend/resume.
 	 */
-	error = request_firmware(&fw, icn8505->firmware_name, dev);
+	error = firmware_request_platform(&fw, icn8505->firmware_name, dev);
 	if (error) {
 		dev_err(dev, "Firmware request error %d\n", error);
 		return error;
-- 
2.23.0

[PATCH v7 7/8] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support

[Hans de Goede]

Sofar we have been unable to get permission from the vendors to put the
firmware for touchscreens listed in touchscreen_dmi in linux-firmware.

Some of the tablets with such a touchscreen have a touchscreen driver, and
thus a copy of the firmware, as part of their EFI code.

This commit adds the necessary info for the new EFI embedded-firmware code
to extract these firmwares, making the touchscreen work OOTB without the
user needing to manually add the firmware.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v7:
- Remove adding of PROPERTY_ENTRY_BOOL("efi-embedded-firmware"), to touchscreen
  props, as this is no longer necessary

Changes in v6:
- Switch from crc sums to SHA256 hashes for the firmware hashes
---
 drivers/firmware/efi/embedded-firmware.c |  3 ++
 drivers/platform/x86/Kconfig             |  1 +
 drivers/platform/x86/touchscreen_dmi.c   | 41 +++++++++++++++++++++++-
 include/linux/efi_embedded_fw.h          |  2 ++
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
index 75d652f3148b..8038995fd258 100644
--- a/drivers/firmware/efi/embedded-firmware.c
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -24,6 +24,9 @@ struct embedded_fw {
 static LIST_HEAD(found_fw_list);
 
 static const struct dmi_system_id * const embedded_fw_table[] = {
+#ifdef CONFIG_TOUCHSCREEN_DMI
+	touchscreen_dmi_table,
+#endif
 	NULL
 };
 
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ae21d08c65e8..583af56d8bfa 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1251,6 +1251,7 @@ config INTEL_TURBO_MAX_3
 config TOUCHSCREEN_DMI
 	bool "DMI based touchscreen configuration info"
 	depends on ACPI && DMI && I2C=y && TOUCHSCREEN_SILEAD
+	select EFI_EMBEDDED_FIRMWARE if EFI
 	---help---
 	  Certain ACPI based tablets with e.g. Silead or Chipone touchscreens
 	  do not have enough data in ACPI tables for the touchscreen driver to
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 1c7d8324ff5c..8a7d5c8df114 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -11,12 +11,15 @@
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
+#include <linux/efi_embedded_fw.h>
 #include <linux/i2c.h>
 #include <linux/notifier.h>
 #include <linux/property.h>
 #include <linux/string.h>
 
 struct ts_dmi_data {
+	/* The EFI embedded-fw code expects this to be the first member! */
+	struct efi_embedded_fw_desc embedded_fw;
 	const char *acpi_name;
 	const struct property_entry *properties;
 };
@@ -64,6 +67,15 @@ static const struct property_entry chuwi_hi8_pro_props[] = {
 };
 
 static const struct ts_dmi_data chuwi_hi8_pro_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl3680-chuwi-hi8-pro.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 39864,
+		.sha256	= { 0xc0, 0x88, 0xc5, 0xef, 0xd1, 0x70, 0x77, 0x59,
+			    0x4e, 0xe9, 0xc4, 0xd8, 0x2e, 0xcd, 0xbf, 0x95,
+			    0x32, 0xd9, 0x03, 0x28, 0x0d, 0x48, 0x9f, 0x92,
+			    0x35, 0x37, 0xf6, 0x8b, 0x2a, 0xe4, 0x73, 0xff },
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= chuwi_hi8_pro_props,
 };
@@ -181,6 +193,15 @@ static const struct property_entry cube_iwork8_air_props[] = {
 };
 
 static const struct ts_dmi_data cube_iwork8_air_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl3670-cube-iwork8-air.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 38808,
+		.sha256	= { 0xff, 0x62, 0x2d, 0xd1, 0x8a, 0x78, 0x04, 0x7b,
+			    0x33, 0x06, 0xb0, 0x4f, 0x7f, 0x02, 0x08, 0x9c,
+			    0x96, 0xd4, 0x9f, 0x04, 0xe1, 0x47, 0x25, 0x25,
+			    0x60, 0x77, 0x41, 0x33, 0xeb, 0x12, 0x82, 0xfc },
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= cube_iwork8_air_props,
 };
@@ -374,6 +395,15 @@ static const struct property_entry onda_v80_plus_v3_props[] = {
 };
 
 static const struct ts_dmi_data onda_v80_plus_v3_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl3676-onda-v80-plus-v3.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 37224,
+		.sha256	= { 0x8f, 0xbd, 0x8f, 0x0c, 0x6b, 0xba, 0x5b, 0xf5,
+			    0xa3, 0xc7, 0xa3, 0xc0, 0x4f, 0xcd, 0xdf, 0x32,
+			    0xcc, 0xe4, 0x70, 0xd6, 0x46, 0x9c, 0xd7, 0xa7,
+			    0x4b, 0x82, 0x3f, 0xab, 0xc7, 0x90, 0xea, 0x23 },
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= onda_v80_plus_v3_props,
 };
@@ -440,6 +470,15 @@ static const struct property_entry pipo_w2s_props[] = {
 };
 
 static const struct ts_dmi_data pipo_w2s_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl1680-pipo-w2s.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 39072,
+		.sha256	= { 0xd0, 0x58, 0xc4, 0x7d, 0x55, 0x2d, 0x62, 0x18,
+			    0xd1, 0x6a, 0x71, 0x73, 0x0b, 0x3f, 0xbe, 0x60,
+			    0xbb, 0x45, 0x8c, 0x52, 0x27, 0xb7, 0x18, 0xf4,
+			    0x31, 0x00, 0x6a, 0x49, 0x76, 0xd8, 0x7c, 0xd3 },
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= pipo_w2s_props,
 };
@@ -608,7 +647,7 @@ static const struct ts_dmi_data trekstor_surftab_wintron70_data = {
 };
 
 /* NOTE: Please keep this table sorted alphabetically */
-static const struct dmi_system_id touchscreen_dmi_table[] = {
+const struct dmi_system_id touchscreen_dmi_table[] = {
 	{
 		/* Chuwi Hi8 */
 		.driver_data = (void *)&chuwi_hi8_data,
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
index ac70ff146d58..1ca4206ee006 100644
--- a/include/linux/efi_embedded_fw.h
+++ b/include/linux/efi_embedded_fw.h
@@ -20,6 +20,8 @@ struct efi_embedded_fw_desc {
 	u8 sha256[32];
 };
 
+extern const struct dmi_system_id touchscreen_dmi_table[];
+
 int efi_get_embedded_fw(const char *name, void **dat, size_t *sz);
 
 #endif
-- 
2.23.0

[PATCH v7 8/8] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet

[Hans de Goede]

Add touchscreen info for the Chuwi Vi8 Plus tablet. This tablet uses a
Chipone ICN8505 touchscreen controller, with the firmware used by the
touchscreen embedded in the EFI firmware.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v7:
- Remove PROPERTY_ENTRY_BOOL("efi-embedded-firmware") properties entry,
  as this is no longer necessary

Changes in v6:
- Switch from crc sums to SHA256 hashes for the firmware hash
---
 drivers/platform/x86/touchscreen_dmi.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 8a7d5c8df114..8bfef880e216 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -132,6 +132,18 @@ static const struct ts_dmi_data chuwi_vi8_data = {
 	.properties     = chuwi_vi8_props,
 };
 
+static const struct ts_dmi_data chuwi_vi8_plus_data = {
+	.embedded_fw = {
+		.name	= "chipone/icn8505-HAMP0002.fw",
+		.prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
+		.length	= 35012,
+		.sha256	= { 0x93, 0xe5, 0x49, 0xe0, 0xb6, 0xa2, 0xb4, 0xb3,
+			    0x88, 0x96, 0x34, 0x97, 0x5e, 0xa8, 0x13, 0x78,
+			    0x72, 0x98, 0xb8, 0x29, 0xeb, 0x5c, 0xa7, 0xf1,
+			    0x25, 0x13, 0x43, 0xf4, 0x30, 0x7c, 0xfc, 0x7c },
+	},
+};
+
 static const struct property_entry chuwi_vi10_props[] = {
 	PROPERTY_ENTRY_U32("touchscreen-min-x", 0),
 	PROPERTY_ENTRY_U32("touchscreen-min-y", 4),
@@ -709,6 +721,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 			DMI_MATCH(DMI_BIOS_VERSION, "CHUWI.D86JLBNR"),
 		},
 	},
+	{
+		/* Chuwi Vi8 Plus (CWI519) */
+		.driver_data = (void *)&chuwi_vi8_plus_data,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
+			DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+		},
+	},
 	{
 		/* Chuwi Vi10 (CWI505) */
 		.driver_data = (void *)&chuwi_vi10_data,
@@ -1085,6 +1106,9 @@ static int __init ts_dmi_init(void)
 		return 0; /* Not an error */
 
 	ts_data = dmi_id->driver_data;
+	/* Some dmi table entries only provide an efi_embedded_fw_desc */
+	if (!ts_data->properties)
+		return 0;
 
 	error = bus_register_notifier(&i2c_bus_type, &ts_dmi_notifier);
 	if (error)
-- 
2.23.0

Re: [PATCH v7 4/8] firmware: Add new platform fallback mechanism and firmware_request_platform()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on efi/next]
[cannot apply to v5.4-rc1 next-20191004]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/efi-firmware-platform-x86-Add-EFI-embedded-fw-support/20191005-021239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/base/firmware_loader/fallback_platform.o: in function `fw_grow_paged_buf':
>> fallback_platform.c:(.text+0x0): multiple definition of `fw_grow_paged_buf'; drivers/base/firmware_loader/main.o:main.c:(.text+0x760): first defined here
   ld: drivers/base/firmware_loader/fallback_platform.o: in function `fw_map_paged_buf':
>> fallback_platform.c:(.text+0x10): multiple definition of `fw_map_paged_buf'; drivers/base/firmware_loader/main.o:main.c:(.text+0x770): first defined here

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28091 bytes --]

Re: [PATCH v7 4/8] firmware: Add new platform fallback mechanism and firmware_request_platform()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on efi/next]
[cannot apply to v5.4-rc1 next-20191004]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/efi-firmware-platform-x86-Add-EFI-embedded-fw-support/20191005-021239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 7.4.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
        GCC_VERSION=7.4.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/base/firmware_loader/fallback_platform.o:(.opd+0x0): multiple definition of `fw_grow_paged_buf'
   drivers/base/firmware_loader/main.o:(.opd+0x1e0): first defined here
   drivers/base/firmware_loader/fallback_platform.o: In function `.fw_grow_paged_buf':
   fallback_platform.c:(.text+0x0): multiple definition of `.fw_grow_paged_buf'
   drivers/base/firmware_loader/main.o:main.c:(.text+0x10e0): first defined here
>> drivers/base/firmware_loader/fallback_platform.o:(.opd+0x18): multiple definition of `fw_map_paged_buf'
   drivers/base/firmware_loader/main.o:(.opd+0x1f8): first defined here
   drivers/base/firmware_loader/fallback_platform.o: In function `.fw_map_paged_buf':
   fallback_platform.c:(.text+0x30): multiple definition of `.fw_map_paged_buf'
   drivers/base/firmware_loader/main.o:main.c:(.text+0x1110): first defined here

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25332 bytes --]

Re: [PATCH v7 4/8] firmware: Add new platform fallback mechanism and firmware_request_platform()

[Dmitry Torokhov]

Hi Hans,

On Fri, Oct 04, 2019 at 04:50:52PM +0200, Hans de Goede wrote:
> In some cases the platform's main firmware (e.g. the UEFI fw) may contain
> an embedded copy of device 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 a new platform fallback mechanism to the firmware loader
> which will try to lookup a device fw copy embedded in the platform's main
> firmware if direct filesystem lookup fails.
> 
> Drivers which need such embedded fw copies can enable this fallback
> mechanism by using the new firmware_request_platform() function.

Why would drivers not want to fetch firmware from system firmware if it
is not present on disk? I would say let driver to opt-out of this
fallback, but default request_firmware() should do it by default.

Thanks.

-- 
Dmitry

Re: [PATCH v7 7/8] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6607 bytes --]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on efi/next]
[cannot apply to v5.4-rc1 next-20191004]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/efi-firmware-platform-x86-Add-EFI-embedded-fw-support/20191005-021239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/firmware/efi/embedded-firmware.c: In function 'efi_check_md_for_embedded_firmware':
>> drivers/firmware/efi/embedded-firmware.c:64:3: error: implicit declaration of function 'sha256_init'; did you mean 'sparse_init'? [-Werror=implicit-function-declaration]
      sha256_init(&sctx);
      ^~~~~~~~~~~
      sparse_init
>> drivers/firmware/efi/embedded-firmware.c:65:3: error: implicit declaration of function 'sha256_update'; did you mean 'key_update'? [-Werror=implicit-function-declaration]
      sha256_update(&sctx, map + i, desc->length);
      ^~~~~~~~~~~~~
      key_update
>> drivers/firmware/efi/embedded-firmware.c:66:3: error: implicit declaration of function 'sha256_final' [-Werror=implicit-function-declaration]
      sha256_final(&sctx, sha256);
      ^~~~~~~~~~~~
   cc1: some warnings being treated as errors

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

3977d46d946ae2 Hans de Goede 2019-10-04  32  
3977d46d946ae2 Hans de Goede 2019-10-04  33  /*
3977d46d946ae2 Hans de Goede 2019-10-04  34   * Note the efi_check_for_embedded_firmwares() code currently makes the
3977d46d946ae2 Hans de Goede 2019-10-04  35   * following 2 assumptions. This may needs to be revisited if embedded firmware
3977d46d946ae2 Hans de Goede 2019-10-04  36   * is found where this is not true:
3977d46d946ae2 Hans de Goede 2019-10-04  37   * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
3977d46d946ae2 Hans de Goede 2019-10-04  38   * 2) The firmware always starts at an offset which is a multiple of 8 bytes
3977d46d946ae2 Hans de Goede 2019-10-04  39   */
3977d46d946ae2 Hans de Goede 2019-10-04  40  static int __init efi_check_md_for_embedded_firmware(
3977d46d946ae2 Hans de Goede 2019-10-04  41  	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
3977d46d946ae2 Hans de Goede 2019-10-04  42  {
3977d46d946ae2 Hans de Goede 2019-10-04  43  	const u64 prefix = *((u64 *)desc->prefix);
3977d46d946ae2 Hans de Goede 2019-10-04  44  	struct sha256_state sctx;
3977d46d946ae2 Hans de Goede 2019-10-04  45  	struct embedded_fw *fw;
3977d46d946ae2 Hans de Goede 2019-10-04  46  	u8 sha256[32];
3977d46d946ae2 Hans de Goede 2019-10-04  47  	u64 i, size;
3977d46d946ae2 Hans de Goede 2019-10-04  48  	void *map;
3977d46d946ae2 Hans de Goede 2019-10-04  49  
3977d46d946ae2 Hans de Goede 2019-10-04  50  	size = md->num_pages << EFI_PAGE_SHIFT;
3977d46d946ae2 Hans de Goede 2019-10-04  51  	map = memremap(md->phys_addr, size, MEMREMAP_WB);
3977d46d946ae2 Hans de Goede 2019-10-04  52  	if (!map) {
3977d46d946ae2 Hans de Goede 2019-10-04  53  		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
3977d46d946ae2 Hans de Goede 2019-10-04  54  		return -ENOMEM;
3977d46d946ae2 Hans de Goede 2019-10-04  55  	}
3977d46d946ae2 Hans de Goede 2019-10-04  56  
3977d46d946ae2 Hans de Goede 2019-10-04  57  	size -= desc->length;
3977d46d946ae2 Hans de Goede 2019-10-04  58  	for (i = 0; i < size; i += 8) {
3977d46d946ae2 Hans de Goede 2019-10-04  59  		u64 *mem = map + i;
3977d46d946ae2 Hans de Goede 2019-10-04  60  
3977d46d946ae2 Hans de Goede 2019-10-04  61  		if (*mem != prefix)
3977d46d946ae2 Hans de Goede 2019-10-04  62  			continue;
3977d46d946ae2 Hans de Goede 2019-10-04  63  
3977d46d946ae2 Hans de Goede 2019-10-04 @64  		sha256_init(&sctx);
3977d46d946ae2 Hans de Goede 2019-10-04 @65  		sha256_update(&sctx, map + i, desc->length);
3977d46d946ae2 Hans de Goede 2019-10-04 @66  		sha256_final(&sctx, sha256);
3977d46d946ae2 Hans de Goede 2019-10-04  67  		if (memcmp(sha256, desc->sha256, 32) == 0)
3977d46d946ae2 Hans de Goede 2019-10-04  68  			break;
3977d46d946ae2 Hans de Goede 2019-10-04  69  	}
3977d46d946ae2 Hans de Goede 2019-10-04  70  	if (i >= size) {
3977d46d946ae2 Hans de Goede 2019-10-04  71  		memunmap(map);
3977d46d946ae2 Hans de Goede 2019-10-04  72  		return -ENOENT;
3977d46d946ae2 Hans de Goede 2019-10-04  73  	}
3977d46d946ae2 Hans de Goede 2019-10-04  74  
3977d46d946ae2 Hans de Goede 2019-10-04  75  	pr_info("Found EFI embedded fw '%s'\n", desc->name);
3977d46d946ae2 Hans de Goede 2019-10-04  76  
3977d46d946ae2 Hans de Goede 2019-10-04  77  	fw = kmalloc(sizeof(*fw), GFP_KERNEL);
3977d46d946ae2 Hans de Goede 2019-10-04  78  	if (!fw) {
3977d46d946ae2 Hans de Goede 2019-10-04  79  		memunmap(map);
3977d46d946ae2 Hans de Goede 2019-10-04  80  		return -ENOMEM;
3977d46d946ae2 Hans de Goede 2019-10-04  81  	}
3977d46d946ae2 Hans de Goede 2019-10-04  82  
3977d46d946ae2 Hans de Goede 2019-10-04  83  	fw->data = kmemdup(map + i, desc->length, GFP_KERNEL);
3977d46d946ae2 Hans de Goede 2019-10-04  84  	memunmap(map);
3977d46d946ae2 Hans de Goede 2019-10-04  85  	if (!fw->data) {
3977d46d946ae2 Hans de Goede 2019-10-04  86  		kfree(fw);
3977d46d946ae2 Hans de Goede 2019-10-04  87  		return -ENOMEM;
3977d46d946ae2 Hans de Goede 2019-10-04  88  	}
3977d46d946ae2 Hans de Goede 2019-10-04  89  
3977d46d946ae2 Hans de Goede 2019-10-04  90  	fw->name = desc->name;
3977d46d946ae2 Hans de Goede 2019-10-04  91  	fw->length = desc->length;
3977d46d946ae2 Hans de Goede 2019-10-04  92  	list_add(&fw->list, &found_fw_list);
3977d46d946ae2 Hans de Goede 2019-10-04  93  
3977d46d946ae2 Hans de Goede 2019-10-04  94  	return 0;
3977d46d946ae2 Hans de Goede 2019-10-04  95  }
3977d46d946ae2 Hans de Goede 2019-10-04  96  

:::::: The code at line 64 was first introduced by commit
:::::: 3977d46d946ae2699ac2c8196d52fb9b303909c9 efi: Add embedded peripheral firmware support

:::::: TO: Hans de Goede <hdegoede@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68841 bytes --]

Re: [PATCH v7 7/8] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6526 bytes --]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on efi/next]
[cannot apply to v5.4-rc1 next-20191004]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/efi-firmware-platform-x86-Add-EFI-embedded-fw-support/20191005-021239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: x86_64-randconfig-a004-201939 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/firmware/efi/embedded-firmware.c: In function 'efi_check_md_for_embedded_firmware':
>> drivers/firmware/efi/embedded-firmware.c:64:3: error: implicit declaration of function 'sha256_init' [-Werror=implicit-function-declaration]
      sha256_init(&sctx);
      ^
>> drivers/firmware/efi/embedded-firmware.c:65:3: error: implicit declaration of function 'sha256_update' [-Werror=implicit-function-declaration]
      sha256_update(&sctx, map + i, desc->length);
      ^
   drivers/firmware/efi/embedded-firmware.c:66:3: error: implicit declaration of function 'sha256_final' [-Werror=implicit-function-declaration]
      sha256_final(&sctx, sha256);
      ^
   cc1: some warnings being treated as errors

vim +/sha256_init +64 drivers/firmware/efi/embedded-firmware.c

3977d46d946ae2 Hans de Goede 2019-10-04  32  
3977d46d946ae2 Hans de Goede 2019-10-04  33  /*
3977d46d946ae2 Hans de Goede 2019-10-04  34   * Note the efi_check_for_embedded_firmwares() code currently makes the
3977d46d946ae2 Hans de Goede 2019-10-04  35   * following 2 assumptions. This may needs to be revisited if embedded firmware
3977d46d946ae2 Hans de Goede 2019-10-04  36   * is found where this is not true:
3977d46d946ae2 Hans de Goede 2019-10-04  37   * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
3977d46d946ae2 Hans de Goede 2019-10-04  38   * 2) The firmware always starts at an offset which is a multiple of 8 bytes
3977d46d946ae2 Hans de Goede 2019-10-04  39   */
3977d46d946ae2 Hans de Goede 2019-10-04  40  static int __init efi_check_md_for_embedded_firmware(
3977d46d946ae2 Hans de Goede 2019-10-04  41  	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
3977d46d946ae2 Hans de Goede 2019-10-04  42  {
3977d46d946ae2 Hans de Goede 2019-10-04  43  	const u64 prefix = *((u64 *)desc->prefix);
3977d46d946ae2 Hans de Goede 2019-10-04  44  	struct sha256_state sctx;
3977d46d946ae2 Hans de Goede 2019-10-04  45  	struct embedded_fw *fw;
3977d46d946ae2 Hans de Goede 2019-10-04  46  	u8 sha256[32];
3977d46d946ae2 Hans de Goede 2019-10-04  47  	u64 i, size;
3977d46d946ae2 Hans de Goede 2019-10-04  48  	void *map;
3977d46d946ae2 Hans de Goede 2019-10-04  49  
3977d46d946ae2 Hans de Goede 2019-10-04  50  	size = md->num_pages << EFI_PAGE_SHIFT;
3977d46d946ae2 Hans de Goede 2019-10-04  51  	map = memremap(md->phys_addr, size, MEMREMAP_WB);
3977d46d946ae2 Hans de Goede 2019-10-04  52  	if (!map) {
3977d46d946ae2 Hans de Goede 2019-10-04  53  		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
3977d46d946ae2 Hans de Goede 2019-10-04  54  		return -ENOMEM;
3977d46d946ae2 Hans de Goede 2019-10-04  55  	}
3977d46d946ae2 Hans de Goede 2019-10-04  56  
3977d46d946ae2 Hans de Goede 2019-10-04  57  	size -= desc->length;
3977d46d946ae2 Hans de Goede 2019-10-04  58  	for (i = 0; i < size; i += 8) {
3977d46d946ae2 Hans de Goede 2019-10-04  59  		u64 *mem = map + i;
3977d46d946ae2 Hans de Goede 2019-10-04  60  
3977d46d946ae2 Hans de Goede 2019-10-04  61  		if (*mem != prefix)
3977d46d946ae2 Hans de Goede 2019-10-04  62  			continue;
3977d46d946ae2 Hans de Goede 2019-10-04  63  
3977d46d946ae2 Hans de Goede 2019-10-04 @64  		sha256_init(&sctx);
3977d46d946ae2 Hans de Goede 2019-10-04 @65  		sha256_update(&sctx, map + i, desc->length);
3977d46d946ae2 Hans de Goede 2019-10-04  66  		sha256_final(&sctx, sha256);
3977d46d946ae2 Hans de Goede 2019-10-04  67  		if (memcmp(sha256, desc->sha256, 32) == 0)
3977d46d946ae2 Hans de Goede 2019-10-04  68  			break;
3977d46d946ae2 Hans de Goede 2019-10-04  69  	}
3977d46d946ae2 Hans de Goede 2019-10-04  70  	if (i >= size) {
3977d46d946ae2 Hans de Goede 2019-10-04  71  		memunmap(map);
3977d46d946ae2 Hans de Goede 2019-10-04  72  		return -ENOENT;
3977d46d946ae2 Hans de Goede 2019-10-04  73  	}
3977d46d946ae2 Hans de Goede 2019-10-04  74  
3977d46d946ae2 Hans de Goede 2019-10-04  75  	pr_info("Found EFI embedded fw '%s'\n", desc->name);
3977d46d946ae2 Hans de Goede 2019-10-04  76  
3977d46d946ae2 Hans de Goede 2019-10-04  77  	fw = kmalloc(sizeof(*fw), GFP_KERNEL);
3977d46d946ae2 Hans de Goede 2019-10-04  78  	if (!fw) {
3977d46d946ae2 Hans de Goede 2019-10-04  79  		memunmap(map);
3977d46d946ae2 Hans de Goede 2019-10-04  80  		return -ENOMEM;
3977d46d946ae2 Hans de Goede 2019-10-04  81  	}
3977d46d946ae2 Hans de Goede 2019-10-04  82  
3977d46d946ae2 Hans de Goede 2019-10-04  83  	fw->data = kmemdup(map + i, desc->length, GFP_KERNEL);
3977d46d946ae2 Hans de Goede 2019-10-04  84  	memunmap(map);
3977d46d946ae2 Hans de Goede 2019-10-04  85  	if (!fw->data) {
3977d46d946ae2 Hans de Goede 2019-10-04  86  		kfree(fw);
3977d46d946ae2 Hans de Goede 2019-10-04  87  		return -ENOMEM;
3977d46d946ae2 Hans de Goede 2019-10-04  88  	}
3977d46d946ae2 Hans de Goede 2019-10-04  89  
3977d46d946ae2 Hans de Goede 2019-10-04  90  	fw->name = desc->name;
3977d46d946ae2 Hans de Goede 2019-10-04  91  	fw->length = desc->length;
3977d46d946ae2 Hans de Goede 2019-10-04  92  	list_add(&fw->list, &found_fw_list);
3977d46d946ae2 Hans de Goede 2019-10-04  93  
3977d46d946ae2 Hans de Goede 2019-10-04  94  	return 0;
3977d46d946ae2 Hans de Goede 2019-10-04  95  }
3977d46d946ae2 Hans de Goede 2019-10-04  96  

:::::: The code at line 64 was first introduced by commit
:::::: 3977d46d946ae2699ac2c8196d52fb9b303909c9 efi: Add embedded peripheral firmware support

:::::: TO: Hans de Goede <hdegoede@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34825 bytes --]

Re: [PATCH v7 4/8] firmware: Add new platform fallback mechanism and firmware_request_platform()

[Hans de Goede]

Hi Dmitry,

On 05-10-2019 01:17, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Fri, Oct 04, 2019 at 04:50:52PM +0200, Hans de Goede wrote:
>> In some cases the platform's main firmware (e.g. the UEFI fw) may contain
>> an embedded copy of device 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 a new platform fallback mechanism to the firmware loader
>> which will try to lookup a device fw copy embedded in the platform's main
>> firmware if direct filesystem lookup fails.
>>
>> Drivers which need such embedded fw copies can enable this fallback
>> mechanism by using the new firmware_request_platform() function.
> 
> Why would drivers not want to fetch firmware from system firmware if it
> is not present on disk? I would say let driver to opt-out of this
> fallback, but default request_firmware() should do it by default.

Only few devices / device-drivers have / need firmware which is
embedded in the system-fw. Checking for this introduces an extra call
in the firmware-loader path and the firmware-loader maintainer have
requested to make this opt-in, rather then opt-out, so that these changes
do not impact the many many other drivers which do not need this.

To be precise so far only the 2 touchscreen drivers for which patches
are in this series are known to benefit from this approach. So since this
is somewhat of a special case opt-in makes more sense then opt-out.

Regards,

Hans

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Ingo Molnar]

* Hans de Goede <hdegoede@redhat.com> wrote:

> Hi All,
> 
> Here is v7 of my patch-set to add support for EFI embedded fw to the kernel.
> 
> v6 was posted a long time ago, around the 4.18 days. The long wait was for
> a suitable secure-hash for checking the firmware we find embedded in the EFI
> is the one we expect.
> 
> With 5.4-rc1 we finally have a standalone sha256 lib, so that hurdle for
> this patch-set is now gone.
> 
> I've tried to address all review-remarks against v6 in this new version:
> 
> Changes in v7:
> - Split drivers/firmware/efi and drivers/base/firmware_loader changes into
>   2 patches
> - Use new, standalone, lib/crypto/sha256.c code
> - Address kdoc comments from Randy Dunlap
> - Add new FW_OPT_FALLBACK_PLATFORM flag and firmware_request_platform()
>   _request_firmware() wrapper, as requested by Luis R. Rodriguez
> - Stop using "efi-embedded-firmware" device-property, now that drivers need to
>   use the new firmware_request_platform() to enable fallback to a device fw
>   copy embedded in the platform's main firmware, we no longer need a property
>   on the device to trigger this behavior
> - Use security_kernel_load_data instead of calling
>   security_kernel_read_file with a NULL file pointer argument
> - Move the docs to Documentation/driver-api/firmware/fallback-mechanisms.rst
> - Document the new firmware_request_platform() function in
>   Documentation/driver-api/firmware/request_firmware.rst
> - Add 2 new patches for the silead and chipone-icn8505 touchscreen drivers
>   to use the new firmware_request_platform() method
> - Rebased on top of 5.4-rc1
> 
> I guess this will probably need another round (ot two) of review + fixing,
> but eventually this can hopefully be merged. Since this touches a bunch
> of different subsystems the question is how to merge this? Most of the
> touched files outside of the firmware-loader code do not see a lot of
> churn, so my proposal would be to merge patches 1-6 through the tree
> which carries firmware-loader changes; and then provide an immutable
> branch for the platform/x86 maintainers to merge and then they can merge
> the last 2 patches (as the touchscreen_dmi.c file does see quite a bit
> of changes every release).

So I was looking for a high level 0/ boilerplate description of this 
series, to explain what "EFI embedded fw" is, what problems it solves and 
how it helps the kernel in general - and found this in 2/8:

>> Just like with PCI options ROMs, which we save in the setup_efi_pci*
>> functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself
>> sometimes may contain data which is useful/necessary for peripheral drivers
>> to have access to.
>>
>> Specifically the EFI code may contain an embedded copy of firmware which
>> needs to be (re)loaded into the peripheral. Normally such firmware would be
>> part of linux-firmware, but in some cases this is not feasible, for 2
>> reasons:
>>
>> 1) The firmware is customized for a specific use-case of the chipset / use
>> with a specific hardware model, so we cannot have a single firmware file
>> for the chipset. E.g. touchscreen controller firmwares are compiled
>> specifically for the hardware model they are used with, as they are
>> calibrated for a specific model digitizer.
>>
>> 2) Despite repeated attempts we have failed to get permission to
>> redistribute the firmware. This is especially a problem with customized
>> firmwares, these get created by the chip vendor for a specific ODM and the
>> copyright may partially belong with the ODM, so the chip vendor cannot
>> give a blanket permission to distribute these.
>>
>> This commit adds support for finding peripheral firmware embedded in the
>> EFI code and makes the found firmware available through the new
>> efi_get_embedded_fw() function.
>>
>> Support for loading these firmwares through the standard firmware loading
>> mechanism is added in a follow-up commit in this patch-series.
>>
>> Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end
>> of start_kernel(), just before calling rest_init(), this is on purpose
>> because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for
>> early_memremap(), so the check must be done after mm_init(). This relies
>> on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services()
>> is called, which means that this will only work on x86 for now.
>>
>> Reported-by: Dave Olsthoorn <dave@bewaar.me>
>> Suggested-by: Peter Jones <pjones@redhat.com>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

There's also patch #3, which explains how this is used:

>> This commit adds a new platform fallback mechanism to the firmware loader
>> which will try to lookup a device fw copy embedded in the platform's main
>> firmware if direct filesystem lookup fails.
>>
>> Drivers which need such embedded fw copies can enable this fallback
>> mechanism by using the new firmware_request_platform() function.
>>
>> Note that for now this is only supported on EFI platforms and even on
>> these platforms firmware_fallback_platform() only works if
>> CONFIG_EFI_EMBEDDED_FIRMWARE is enabled (this gets selected by drivers
>> which need this), in all other cases firmware_fallback_platform() simply
>> always returns -ENOENT.

Plus there's 3 patches that opt in three drivers to this new EFI-firmware 
loading mechanism, right?

A couple of high level questions:

- How common are these kinds of firmware files that should be loaded into 
  the device by the OS device driver? Common? Or 1% of systems? 0.1% of 
  systems? 0.0001%?

- Can there be a situation where linux-firmware already includes an older 
  copy of the firmware, and the EFI firmware has a newer version? If this 
  can plausibly happen, shouldn't the fallback mechanism do some sort of 
  version check (if that's possible), and load the newer version?

- I'm worried about the explicit opt-in nature of these firmware files - 
  the OS driver has to be explicitly aware of this possibility. Shouldn't 
  we at minimum have some sort of boot time check to see whether a device 
  has an embedded fw blob, and warn the user if we don't actually load 
  it? Which would generate some gentle pressure to fix our drivers?

- I think the config option should be default-y, because AFAICS this 
  mechanism makes broken drivers/devices work.

- Finally, is there any question of trust or a potential for other 
  security pitfalls here, where we'd trust linux-firmware over what the 
  EFI firmware says is the proper firmware for a device? My default 
  assumption would be that we are exposed to the EFI firmware anyway, and 
  it comes with the hardware just like the devices come with the 
  hardware, so we can generally trust it. But I might be missing 
  something. If there's any plausible question of trust (for example can 
  attackers hide rooted firmware in the EFI image, without triggering 
  filesystem integrity checks on the regular filesystem side?) then it 
  might make sense to offer a boot parameter to disable this, beyond the 
  config parameter.

Thanks,

	Ingo

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Hans de Goede]

Hi Ingo,

On 07-10-2019 16:19, Ingo Molnar wrote:
> 
> * Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi All,
>>
>> Here is v7 of my patch-set to add support for EFI embedded fw to the kernel.
>>
>> v6 was posted a long time ago, around the 4.18 days. The long wait was for
>> a suitable secure-hash for checking the firmware we find embedded in the EFI
>> is the one we expect.
>>
>> With 5.4-rc1 we finally have a standalone sha256 lib, so that hurdle for
>> this patch-set is now gone.
>>
>> I've tried to address all review-remarks against v6 in this new version:
>>
>> Changes in v7:
>> - Split drivers/firmware/efi and drivers/base/firmware_loader changes into
>>    2 patches
>> - Use new, standalone, lib/crypto/sha256.c code
>> - Address kdoc comments from Randy Dunlap
>> - Add new FW_OPT_FALLBACK_PLATFORM flag and firmware_request_platform()
>>    _request_firmware() wrapper, as requested by Luis R. Rodriguez
>> - Stop using "efi-embedded-firmware" device-property, now that drivers need to
>>    use the new firmware_request_platform() to enable fallback to a device fw
>>    copy embedded in the platform's main firmware, we no longer need a property
>>    on the device to trigger this behavior
>> - Use security_kernel_load_data instead of calling
>>    security_kernel_read_file with a NULL file pointer argument
>> - Move the docs to Documentation/driver-api/firmware/fallback-mechanisms.rst
>> - Document the new firmware_request_platform() function in
>>    Documentation/driver-api/firmware/request_firmware.rst
>> - Add 2 new patches for the silead and chipone-icn8505 touchscreen drivers
>>    to use the new firmware_request_platform() method
>> - Rebased on top of 5.4-rc1
>>
>> I guess this will probably need another round (or two) of review + fixing,
>> but eventually this can hopefully be merged. Since this touches a bunch
>> of different subsystems the question is how to merge this? Most of the
>> touched files outside of the firmware-loader code do not see a lot of
>> churn, so my proposal would be to merge patches 1-6 through the tree
>> which carries firmware-loader changes; and then provide an immutable
>> branch for the platform/x86 maintainers to merge and then they can merge
>> the last 2 patches (as the touchscreen_dmi.c file does see quite a bit
>> of changes every release).
> 
> So I was looking for a high level 0/ boilerplate description of this
> series, to explain what "EFI embedded fw" is, what problems it solves and
> how it helps the kernel in general - and found this in 2/8:

Sorry you had to dig into the individual patch changelogs for that
I sorta assumed that everyone involved would still vaguely remember
what this series is about.

>>> Just like with PCI options ROMs, which we save in the setup_efi_pci*
>>> functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself
>>> sometimes may contain data which is useful/necessary for peripheral drivers
>>> to have access to.
>>>
>>> Specifically the EFI code may contain an embedded copy of firmware which
>>> needs to be (re)loaded into the peripheral. Normally such firmware would be
>>> part of linux-firmware, but in some cases this is not feasible, for 2
>>> reasons:
>>>
>>> 1) The firmware is customized for a specific use-case of the chipset / use
>>> with a specific hardware model, so we cannot have a single firmware file
>>> for the chipset. E.g. touchscreen controller firmwares are compiled
>>> specifically for the hardware model they are used with, as they are
>>> calibrated for a specific model digitizer.
>>>
>>> 2) Despite repeated attempts we have failed to get permission to
>>> redistribute the firmware. This is especially a problem with customized
>>> firmwares, these get created by the chip vendor for a specific ODM and the
>>> copyright may partially belong with the ODM, so the chip vendor cannot
>>> give a blanket permission to distribute these.
>>>
>>> This commit adds support for finding peripheral firmware embedded in the
>>> EFI code and makes the found firmware available through the new
>>> efi_get_embedded_fw() function.
>>>
>>> Support for loading these firmwares through the standard firmware loading
>>> mechanism is added in a follow-up commit in this patch-series.
>>>
>>> Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end
>>> of start_kernel(), just before calling rest_init(), this is on purpose
>>> because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for
>>> early_memremap(), so the check must be done after mm_init(). This relies
>>> on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services()
>>> is called, which means that this will only work on x86 for now.
>>>
>>> Reported-by: Dave Olsthoorn <dave@bewaar.me>
>>> Suggested-by: Peter Jones <pjones@redhat.com>
>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> There's also patch #3, which explains how this is used:
> 
>>> This commit adds a new platform fallback mechanism to the firmware loader
>>> which will try to lookup a device fw copy embedded in the platform's main
>>> firmware if direct filesystem lookup fails.
>>>
>>> Drivers which need such embedded fw copies can enable this fallback
>>> mechanism by using the new firmware_request_platform() function.
>>>
>>> Note that for now this is only supported on EFI platforms and even on
>>> these platforms firmware_fallback_platform() only works if
>>> CONFIG_EFI_EMBEDDED_FIRMWARE is enabled (this gets selected by drivers
>>> which need this), in all other cases firmware_fallback_platform() simply
>>> always returns -ENOENT.
> 
> Plus there's 3 patches that opt in three drivers to this new EFI-firmware
> loading mechanism, right?

There is 1 pseudo driver drivers/platform/touchscreen_dmi.c which provides
info which under Windows is hardcoded in device-model specific drivers to
the touchscreen drivers as device-properties.

This pseudo driver is modified to provide the info which
drivers/firmware/efi/embedded-firmware.c needs to find the firmware,
so that we keep the per-model info in 1 file.

Then 2 normal touchscreen drivers are modified to opt-in into the
new EFI-firmware loading mechanism.

> A couple of high level questions:
> 
> - How common are these kinds of firmware files that should be loaded into
>    the device by the OS device driver? Common? Or 1% of systems? 0.1% of
>    systems? 0.0001%?

This whole thing is a clever hack to get around us being unable to get
redistribution permission for (touchscreen) firmwares where the chip vendor
says the firmware is model specific so part of the copyright lays with the
odm/oem and we should talk to them (the odm) and the odm/oem points back
to the chip vendor...

Windows does not get the firmware from the UEFI copy, the windows driver
is model specific. The windows driver is part of the os-image the device ships
with and you are in trouble if you loose the driver. This windows driver
has its own embedded copy of the model specific touchscreen firmware
(oh if only they had put said firmware in some nvram on the chip).

In some cases, mostly on Intel Cherry Trail based tablets, which are
cheap and thus use the cheapest touchscreens which have this problem,
the UEFI contains a "mouse" driver which has a copy, which is what this
patch set adds support for to get around the not being able to distribute
problem.

As such this is not really common, still there are a lot of those cheap
Cherry Trail tablets and new ones are still being manufactured. One of the
reasons I'm spending time on making these work 100% OOTB is to get around
the standard Linux sucks at tablets because desktop devs have no tablet
hw to develop a tablet ui on issue.

I also have tried to make the mechanism generic since I guess it might
become in handy for some other scenarios in the future.

Putting a number on this is hard. I do know several members of my
local hackerspace have such tablets since they are cheap and it is nice
to play around with Linux on them. I would guess a number would be between
1% and 0.1%, but I may be way off.

> - Can there be a situation where linux-firmware already includes an older
>    copy of the firmware, and the EFI firmware has a newer version? If this
>    can plausibly happen, shouldn't the fallback mechanism do some sort of
>    version check (if that's possible), and load the newer version?

I would assume that if we can get distribution permission we will also
be able to update the linux-firmware version later. So atm I'm not worried
about this scenario.

> - I'm worried about the explicit opt-in nature of these firmware files -
>    the OS driver has to be explicitly aware of this possibility. Shouldn't
>    we at minimum have some sort of boot time check to see whether a device
>    has an embedded fw blob, and warn the user if we don't actually load
>    it? Which would generate some gentle pressure to fix our drivers?

The way this works is that drivers/firmware/efi/embedded-firmware.c
only checks for firmwares listed in its embedded_fw_table[]. Entries in
this table need to be manually added. We basically do a brute-force
search of the EFI_BOOT_SERVICES_CODE segments looking for a magic prefix
and then checking for a known SHA256 sum. This is triggered by a DMI
match to avoid slowing the boot with the search on other devices
(UEFI has no standard way to get embedded firmware copies out of it).

I would assume someone going through the trouble to add an entry
to embedded_fw_table[] to also check that things work without a copy
in /lib/firmware.

So the scenario of a new entry showing up, without the matching driver
getting modified is somewhat unlikely. At the same time the firmware-loader
people want to avoid changing the fw-load code paths for all drivers, for
something which so far is used by only very few drivers.

> - I think the config option should be default-y, because AFAICS this
>    mechanism makes broken drivers/devices work.

It is a hidden option and drivers which need this already select it in
this patch-set.

> - Finally, is there any question of trust or a potential for other
>    security pitfalls here, where we'd trust linux-firmware over what the
>    EFI firmware says is the proper firmware for a device? My default
>    assumption would be that we are exposed to the EFI firmware anyway, and
>    it comes with the hardware just like the devices come with the
>    hardware, so we can generally trust it. But I might be missing
>    something. If there's any plausible question of trust (for example can
>    attackers hide rooted firmware in the EFI image, without triggering
>    filesystem integrity checks on the regular filesystem side?) then it
>    might make sense to offer a boot parameter to disable this, beyond the
>    config parameter.

You are right that we are exposed to the EFI firmware anyway on top we
only load embedded firmwares with a known SHA256 and so far the firmware
only gets used for touchscreen controllers which cannot do DMA.

So all in all I am not concerned about security. An attacker would need
to be able to write EFI_BOOT_SERVICES_CODE segments, yet not be able to
find a better attack vector then this; and he would need to be able to
fake an expected SHA256 sum on a Trojan firmware payload, which all
is quite unlikely.

Regards,

Hans

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Ingo Molnar]

* Hans de Goede <hdegoede@redhat.com> wrote:

> > So I was looking for a high level 0/ boilerplate description of this
> > series, to explain what "EFI embedded fw" is, what problems it solves and
> > how it helps the kernel in general - and found this in 2/8:
> 
> Sorry you had to dig into the individual patch changelogs for that
> I sorta assumed that everyone involved would still vaguely remember
> what this series is about.

Wasn't *that* hard to do and I intended to read the patches anyway. ;-)

Thanks for the explanation and the answers, this all looks good to me in 
principle and in implementation as well.

Thanks,

	Ingo

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

[Ard Biesheuvel]

On Fri, 4 Oct 2019 at 16:51, Hans de Goede <hdegoede@redhat.com> 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.
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v5:
> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
>
> Changes in v4:
> -Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
>  memory segments are available (and thus if it makes sense to register the
>  debugfs bits for them)
>
> Changes in v2:
> -Do not call pr_err on debugfs call failures
> ---
>  arch/x86/platform/efi/efi.c    |  1 +
>  arch/x86/platform/efi/quirks.c |  4 +++
>  drivers/firmware/efi/efi.c     | 53 ++++++++++++++++++++++++++++++++++
>  include/linux/efi.h            |  1 +
>  4 files changed, 59 insertions(+)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index c202e1b07e29..847730f7e74b 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -232,6 +232,7 @@ int __init efi_memblock_x86_reserve_range(void)
>              efi.memmap.desc_version);
>
>         memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
> +       set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags);

Should we add a Kconfig symbol to opt into this behavior [set by the
driver in question], instead of always preserving all boot services
regions on all x86 systems?

>
>         return 0;
>  }
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 3b9fd679cea9..fab12ebf0ada 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -411,6 +411,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 8d3e778e988b..abba49c4c46d 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -17,6 +17,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>
> @@ -314,6 +315,55 @@ 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
> +
> +static 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))
> +               return;
> +
> +       for_each_efi_memory_desc(md) {
> +               switch (md->type) {
> +               case EFI_BOOT_SERVICES_CODE:
> +                       snprintf(name, sizeof(name), "boot_services_code%d",
> +                                type_count[md->type]++);
> +                       break;
> +               case EFI_BOOT_SERVICES_DATA:
> +                       snprintf(name, sizeof(name), "boot_services_data%d",
> +                                type_count[md->type]++);
> +                       break;
> +               default:
> +                       continue;
> +               }
> +
> +               debugfs_blob[i].size = md->num_pages << EFI_PAGE_SHIFT;
> +               debugfs_blob[i].data = memremap(md->phys_addr,
> +                                               debugfs_blob[i].size,
> +                                               MEMREMAP_WB);
> +               if (!debugfs_blob[i].data)
> +                       continue;
> +
> +               debugfs_create_blob(name, 0400, efi_debugfs, &debugfs_blob[i]);
> +               i++;
> +               if (i == EFI_DEBUGFS_MAX_BLOBS)
> +                       break;
> +       }
> +}
> +#else
> +static inline void efi_debugfs_init(void) {}
> +#endif
> +
>  /*
>   * We register the efi subsystem with the firmware subsystem and the
>   * efivars subsystem with the efi subsystem, if the system was booted with
> @@ -370,6 +420,9 @@ static int __init efisubsys_init(void)
>                 goto err_remove_group;
>         }
>
> +       if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
> +               efi_debugfs_init();
> +
>         return 0;
>
>  err_remove_group:
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index bd3837022307..2a30a1bd8bdf 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1202,6 +1202,7 @@ extern int __init efi_setup_pcdp_console(char *);
>  #define EFI_DBG                        8       /* Print additional debug info at runtime */
>  #define EFI_NX_PE_DATA         9       /* Can runtime data regions be mapped non-executable? */
>  #define EFI_MEM_ATTR           10      /* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
> +#define EFI_PRESERVE_BS_REGIONS        11      /* Are EFI boot-services memory segments available? */
>
>  #ifdef CONFIG_EFI
>  /*
> --
> 2.23.0
>

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

[Hans de Goede]

Hi,

On 09-10-2019 15:07, Ard Biesheuvel wrote:
> On Fri, 4 Oct 2019 at 16:51, Hans de Goede <hdegoede@redhat.com> 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.
>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v5:
>> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
>>
>> Changes in v4:
>> -Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
>>   memory segments are available (and thus if it makes sense to register the
>>   debugfs bits for them)
>>
>> Changes in v2:
>> -Do not call pr_err on debugfs call failures
>> ---
>>   arch/x86/platform/efi/efi.c    |  1 +
>>   arch/x86/platform/efi/quirks.c |  4 +++
>>   drivers/firmware/efi/efi.c     | 53 ++++++++++++++++++++++++++++++++++
>>   include/linux/efi.h            |  1 +
>>   4 files changed, 59 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index c202e1b07e29..847730f7e74b 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -232,6 +232,7 @@ int __init efi_memblock_x86_reserve_range(void)
>>               efi.memmap.desc_version);
>>
>>          memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
>> +       set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags);
> 
> Should we add a Kconfig symbol to opt into this behavior [set by the
> driver in question], instead of always preserving all boot services
> regions on all x86 systems?

This bit does not control anything, it merely signals that the arch early
boot EFI code keeps the boot-services code around, which is something
which the x86 code already does. Where as e.g. on arm / aarch64 this is
freed early on, this ties in with the other bits:

> 
>>
>>          return 0;
>>   }
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index 3b9fd679cea9..fab12ebf0ada 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -411,6 +411,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;
>> +

This is the point where normally on x86 we do actually free the boot-services
code which is a lot later then on other arches. And this new code actually
does change things to keep the boot-services code *forever* but only
if EFI debugging is enabled on the kernel commandline.

This ties in with the next bit:

>>          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 8d3e778e988b..abba49c4c46d 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c

<snip>

>> @@ -370,6 +420,9 @@ static int __init efisubsys_init(void)
>>                  goto err_remove_group;
>>          }
>>
>> +       if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
>> +               efi_debugfs_init();
>> +
>>          return 0;
>>
>>   err_remove_group:

Here we register the debugfs dir + files, but only when the
boot services code has been kept around, so only if the
EFI_PRESERVE_BS_REGIONS arch feature flag has been set and
EFI debugging has been requested on the kernel commandline.

IOW this patch already offers to configurability you ask for, but instead
of through a Kconfig option (which IMHO would be cumbersome) the decision
is made runtime based on the presence of efi=debug on the kernel commandline.

Regards,

Hans




>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index bd3837022307..2a30a1bd8bdf 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -1202,6 +1202,7 @@ extern int __init efi_setup_pcdp_console(char *);
>>   #define EFI_DBG                        8       /* Print additional debug info at runtime */
>>   #define EFI_NX_PE_DATA         9       /* Can runtime data regions be mapped non-executable? */
>>   #define EFI_MEM_ATTR           10      /* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
>> +#define EFI_PRESERVE_BS_REGIONS        11      /* Are EFI boot-services memory segments available? */
>>
>>   #ifdef CONFIG_EFI
>>   /*
>> --
>> 2.23.0
>>

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

[Ard Biesheuvel]

On Wed, 9 Oct 2019 at 15:18, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 09-10-2019 15:07, Ard Biesheuvel wrote:
> > On Fri, 4 Oct 2019 at 16:51, Hans de Goede <hdegoede@redhat.com> 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.
> >>
> >> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Changes in v5:
> >> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
> >>
> >> Changes in v4:
> >> -Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
> >>   memory segments are available (and thus if it makes sense to register the
> >>   debugfs bits for them)
> >>
> >> Changes in v2:
> >> -Do not call pr_err on debugfs call failures
> >> ---
> >>   arch/x86/platform/efi/efi.c    |  1 +
> >>   arch/x86/platform/efi/quirks.c |  4 +++
> >>   drivers/firmware/efi/efi.c     | 53 ++++++++++++++++++++++++++++++++++
> >>   include/linux/efi.h            |  1 +
> >>   4 files changed, 59 insertions(+)
> >>
> >> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> >> index c202e1b07e29..847730f7e74b 100644
> >> --- a/arch/x86/platform/efi/efi.c
> >> +++ b/arch/x86/platform/efi/efi.c
> >> @@ -232,6 +232,7 @@ int __init efi_memblock_x86_reserve_range(void)
> >>               efi.memmap.desc_version);
> >>
> >>          memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
> >> +       set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags);
> >
> > Should we add a Kconfig symbol to opt into this behavior [set by the
> > driver in question], instead of always preserving all boot services
> > regions on all x86 systems?
>
> This bit does not control anything, it merely signals that the arch early
> boot EFI code keeps the boot-services code around, which is something
> which the x86 code already does. Where as e.g. on arm / aarch64 this is
> freed early on, this ties in with the other bits:
>
> >
> >>
> >>          return 0;
> >>   }
> >> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> >> index 3b9fd679cea9..fab12ebf0ada 100644
> >> --- a/arch/x86/platform/efi/quirks.c
> >> +++ b/arch/x86/platform/efi/quirks.c
> >> @@ -411,6 +411,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;
> >> +
>
> This is the point where normally on x86 we do actually free the boot-services
> code which is a lot later then on other arches. And this new code actually
> does change things to keep the boot-services code *forever* but only
> if EFI debugging is enabled on the kernel commandline.
>

I get this part. But at some point, your driver is going to expect
this memory to be preserved even if EFI_DBG is not set, right? My
question was whether we should only opt into that if such a driver is
enabled in the first place.

> This ties in with the next bit:
>
> >>          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 8d3e778e988b..abba49c4c46d 100644
> >> --- a/drivers/firmware/efi/efi.c
> >> +++ b/drivers/firmware/efi/efi.c
>
> <snip>
>
> >> @@ -370,6 +420,9 @@ static int __init efisubsys_init(void)
> >>                  goto err_remove_group;
> >>          }
> >>
> >> +       if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
> >> +               efi_debugfs_init();
> >> +
> >>          return 0;
> >>
> >>   err_remove_group:
>
> Here we register the debugfs dir + files, but only when the
> boot services code has been kept around, so only if the
> EFI_PRESERVE_BS_REGIONS arch feature flag has been set and
> EFI debugging has been requested on the kernel commandline.
>
> IOW this patch already offers to configurability you ask for, but instead
> of through a Kconfig option (which IMHO would be cumbersome) the decision
> is made runtime based on the presence of efi=debug on the kernel commandline.
>
> Regards,
>
> Hans
>
>
>
>
> >> diff --git a/include/linux/efi.h b/include/linux/efi.h
> >> index bd3837022307..2a30a1bd8bdf 100644
> >> --- a/include/linux/efi.h
> >> +++ b/include/linux/efi.h
> >> @@ -1202,6 +1202,7 @@ extern int __init efi_setup_pcdp_console(char *);
> >>   #define EFI_DBG                        8       /* Print additional debug info at runtime */
> >>   #define EFI_NX_PE_DATA         9       /* Can runtime data regions be mapped non-executable? */
> >>   #define EFI_MEM_ATTR           10      /* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
> >> +#define EFI_PRESERVE_BS_REGIONS        11      /* Are EFI boot-services memory segments available? */
> >>
> >>   #ifdef CONFIG_EFI
> >>   /*
> >> --
> >> 2.23.0
> >>

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

[Hans de Goede]

Hi,

On 09-10-2019 15:35, Ard Biesheuvel wrote:
> On Wed, 9 Oct 2019 at 15:18, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 09-10-2019 15:07, Ard Biesheuvel wrote:
>>> On Fri, 4 Oct 2019 at 16:51, Hans de Goede <hdegoede@redhat.com> 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.
>>>>
>>>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v5:
>>>> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
>>>>
>>>> Changes in v4:
>>>> -Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
>>>>    memory segments are available (and thus if it makes sense to register the
>>>>    debugfs bits for them)
>>>>
>>>> Changes in v2:
>>>> -Do not call pr_err on debugfs call failures
>>>> ---
>>>>    arch/x86/platform/efi/efi.c    |  1 +
>>>>    arch/x86/platform/efi/quirks.c |  4 +++
>>>>    drivers/firmware/efi/efi.c     | 53 ++++++++++++++++++++++++++++++++++
>>>>    include/linux/efi.h            |  1 +
>>>>    4 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>>>> index c202e1b07e29..847730f7e74b 100644
>>>> --- a/arch/x86/platform/efi/efi.c
>>>> +++ b/arch/x86/platform/efi/efi.c
>>>> @@ -232,6 +232,7 @@ int __init efi_memblock_x86_reserve_range(void)
>>>>                efi.memmap.desc_version);
>>>>
>>>>           memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
>>>> +       set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags);
>>>
>>> Should we add a Kconfig symbol to opt into this behavior [set by the
>>> driver in question], instead of always preserving all boot services
>>> regions on all x86 systems?
>>
>> This bit does not control anything, it merely signals that the arch early
>> boot EFI code keeps the boot-services code around, which is something
>> which the x86 code already does. Where as e.g. on arm / aarch64 this is
>> freed early on, this ties in with the other bits:
>>
>>>
>>>>
>>>>           return 0;
>>>>    }
>>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>>>> index 3b9fd679cea9..fab12ebf0ada 100644
>>>> --- a/arch/x86/platform/efi/quirks.c
>>>> +++ b/arch/x86/platform/efi/quirks.c
>>>> @@ -411,6 +411,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;
>>>> +
>>
>> This is the point where normally on x86 we do actually free the boot-services
>> code which is a lot later then on other arches. And this new code actually
>> does change things to keep the boot-services code *forever* but only
>> if EFI debugging is enabled on the kernel commandline.
>>
> 
> I get this part. But at some point, your driver is going to expect
> this memory to be preserved even if EFI_DBG is not set, right? My
> question was whether we should only opt into that if such a driver is
> enabled in the first place.

Ah, I see. No even with CONFIG_EFI_EMBEDDED_FIRMWARE selected, the
boot-services code still gets free-ed. The efi_get_embedded_fw()
function from drivers/firmware/efi/embedded-firmware.c runs before
efi_free_boot_services() and it memdup-s any found firmwares, so it
does not cause the EFI boot-services code to stick around longer
then usual.

The only thing which does cause it to stick around is enabling
EFI debugging with efi=debug, so that the various efi segments
(not only the code-services ones) can be inspected as debugfs
blobs.

Basically this first patch of the series is independent of the
rest. It is part of the series, because adding new
efi_embedded_fw_desc structs to the table of firmwares to check
for becomes a lot easier when we can easily inspect the efi
segments and see if they contain the firmware we want.


As for Kconfig options, the compiling of
drivers/firmware/efi/embedded-firmware.c is controlled by
CONFIG_EFI_EMBEDDED_FIRMWARE which is a hidden option, which
can be selected by code which needs this. currently this is
only selected by CONFIG_TOUCHSCREEN_DMI which is defined
in drivers/platform/x86/Kconfig.

Regards,

Hans

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

[Ard Biesheuvel]

On Wed, 9 Oct 2019 at 15:59, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 09-10-2019 15:35, Ard Biesheuvel wrote:
> > On Wed, 9 Oct 2019 at 15:18, Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 09-10-2019 15:07, Ard Biesheuvel wrote:
> >>> On Fri, 4 Oct 2019 at 16:51, Hans de Goede <hdegoede@redhat.com> 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.
> >>>>
> >>>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>> ---
> >>>> Changes in v5:
> >>>> -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS
> >>>>
> >>>> Changes in v4:
> >>>> -Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
> >>>>    memory segments are available (and thus if it makes sense to register the
> >>>>    debugfs bits for them)
> >>>>
> >>>> Changes in v2:
> >>>> -Do not call pr_err on debugfs call failures
> >>>> ---
> >>>>    arch/x86/platform/efi/efi.c    |  1 +
> >>>>    arch/x86/platform/efi/quirks.c |  4 +++
> >>>>    drivers/firmware/efi/efi.c     | 53 ++++++++++++++++++++++++++++++++++
> >>>>    include/linux/efi.h            |  1 +
> >>>>    4 files changed, 59 insertions(+)
> >>>>
> >>>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> >>>> index c202e1b07e29..847730f7e74b 100644
> >>>> --- a/arch/x86/platform/efi/efi.c
> >>>> +++ b/arch/x86/platform/efi/efi.c
> >>>> @@ -232,6 +232,7 @@ int __init efi_memblock_x86_reserve_range(void)
> >>>>                efi.memmap.desc_version);
> >>>>
> >>>>           memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
> >>>> +       set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags);
> >>>
> >>> Should we add a Kconfig symbol to opt into this behavior [set by the
> >>> driver in question], instead of always preserving all boot services
> >>> regions on all x86 systems?
> >>
> >> This bit does not control anything, it merely signals that the arch early
> >> boot EFI code keeps the boot-services code around, which is something
> >> which the x86 code already does. Where as e.g. on arm / aarch64 this is
> >> freed early on, this ties in with the other bits:
> >>
> >>>
> >>>>
> >>>>           return 0;
> >>>>    }
> >>>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> >>>> index 3b9fd679cea9..fab12ebf0ada 100644
> >>>> --- a/arch/x86/platform/efi/quirks.c
> >>>> +++ b/arch/x86/platform/efi/quirks.c
> >>>> @@ -411,6 +411,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;
> >>>> +
> >>
> >> This is the point where normally on x86 we do actually free the boot-services
> >> code which is a lot later then on other arches. And this new code actually
> >> does change things to keep the boot-services code *forever* but only
> >> if EFI debugging is enabled on the kernel commandline.
> >>
> >
> > I get this part. But at some point, your driver is going to expect
> > this memory to be preserved even if EFI_DBG is not set, right? My
> > question was whether we should only opt into that if such a driver is
> > enabled in the first place.
>
> Ah, I see. No even with CONFIG_EFI_EMBEDDED_FIRMWARE selected, the
> boot-services code still gets free-ed. The efi_get_embedded_fw()
> function from drivers/firmware/efi/embedded-firmware.c runs before
> efi_free_boot_services() and it memdup-s any found firmwares, so it
> does not cause the EFI boot-services code to stick around longer
> then usual.
>
> The only thing which does cause it to stick around is enabling
> EFI debugging with efi=debug, so that the various efi segments
> (not only the code-services ones) can be inspected as debugfs
> blobs.
>
> Basically this first patch of the series is independent of the
> rest. It is part of the series, because adding new
> efi_embedded_fw_desc structs to the table of firmwares to check
> for becomes a lot easier when we can easily inspect the efi
> segments and see if they contain the firmware we want.
>
>
> As for Kconfig options, the compiling of
> drivers/firmware/efi/embedded-firmware.c is controlled by
> CONFIG_EFI_EMBEDDED_FIRMWARE which is a hidden option, which
> can be selected by code which needs this. currently this is
> only selected by CONFIG_TOUCHSCREEN_DMI which is defined
> in drivers/platform/x86/Kconfig.
>

OK, thanks for clearing that up.

Re: [PATCH v7 5/8] Input: silead - Switch to firmware_request_platform for retreiving the fw

[Dmitry Torokhov]

On Fri, Oct 04, 2019 at 04:50:53PM +0200, Hans de Goede wrote:
> Unfortunately sofar we have been unable to get permission to redistribute
> Silead touchscreen firmwares in linux-firmware. This means that people
> need to find and install the firmware themselves before the touchscreen
> will work
> 
> Some UEFI/x86 tablets with a Silead touchscreen have a copy of the fw
> embedded in their UEFI boot-services code.
> 
> This commit makes the silead driver use the new firmware_request_platform
> function, which will fallback to looking for such an embedded copy when
> direct filesystem lookup fails. This will make the touchscreen work OOTB
> on devices where there is a fw copy embedded in the UEFI code.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/input/touchscreen/silead.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> index ad8b6a2bfd36..8fa2f3b7cfd8 100644
> --- a/drivers/input/touchscreen/silead.c
> +++ b/drivers/input/touchscreen/silead.c
> @@ -288,7 +288,7 @@ static int silead_ts_load_fw(struct i2c_client *client)
>  
>  	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
>  
> -	error = request_firmware(&fw, data->fw_name, dev);
> +	error = firmware_request_platform(&fw, data->fw_name, dev);
>  	if (error) {
>  		dev_err(dev, "Firmware request error %d\n", error);
>  		return error;
> -- 
> 2.23.0
> 

-- 
Dmitry

Re: [PATCH v7 6/8] Input: icn8505 - Switch to firmware_request_platform for retreiving the fw

[Dmitry Torokhov]

On Fri, Oct 04, 2019 at 04:50:54PM +0200, Hans de Goede wrote:
> Unfortunately sofar we have been unable to get permission to redistribute
> icn8505 touchscreen firmwares in linux-firmware. This means that people
> need to find and install the firmware themselves before the touchscreen
> will work
> 
> Some UEFI/x86 tablets with an icn8505 touchscreen have a copy of the fw
> embedded in their UEFI boot-services code.
> 
> This commit makes the icn8505 driver use the new firmware_request_platform
> function, which will fallback to looking for such an embedded copy when
> direct filesystem lookup fails. This will make the touchscreen work OOTB
> on devices where there is a fw copy embedded in the UEFI code.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Feel free to merge with the rest of the series.

> ---
>  drivers/input/touchscreen/chipone_icn8505.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c
> index c768186ce856..f9ca5502ac8c 100644
> --- a/drivers/input/touchscreen/chipone_icn8505.c
> +++ b/drivers/input/touchscreen/chipone_icn8505.c
> @@ -288,7 +288,7 @@ static int icn8505_upload_fw(struct icn8505_data *icn8505)
>  	 * we may need it at resume. Having loaded it once will make the
>  	 * firmware class code cache it at suspend/resume.
>  	 */
> -	error = request_firmware(&fw, icn8505->firmware_name, dev);
> +	error = firmware_request_platform(&fw, icn8505->firmware_name, dev);
>  	if (error) {
>  		dev_err(dev, "Firmware request error %d\n", error);
>  		return error;
> -- 
> 2.23.0
> 

-- 
Dmitry

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis Chamberlain]

Hey Hans, thanks for staying on top of this and follow up! For some
reason the universe conspired against your first and last patch ([1/8],
[8/8]), and I never got them. Could you bounce these or resend in case
others confirm they also didn't get it?

While at it, can you Cc scott.branden@broadcom.com in further
communications about this patchset, he's interest in some other changes
we'll need to coordinate if we get to have some other development in
line for the next merge window.

  Luis

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Hans de Goede]

Hi,

On 10/11/19 4:10 PM, Luis Chamberlain wrote:
> Hey Hans, thanks for staying on top of this and follow up! For some
> reason the universe conspired against your first and last patch ([1/8],
> [8/8]), and I never got them. Could you bounce these or resend in case
> others confirm they also didn't get it?

I have received feedback from others on the first patch, so at least
that one has reached others. I've bounced patches 1 and 8 to you.

> While at it, can you Cc scott.branden@broadcom.com in further
> communications about this patchset, he's interest in some other changes
> we'll need to coordinate if we get to have some other development in
> line for the next merge window.

Will do.

Regards,

Hans

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

[Luis Chamberlain]

On Fri, Oct 04, 2019 at 04:50:50PM +0200, Hans de Goede wrote:
> +static int __init efi_check_md_for_embedded_firmware(
> +	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
> +{
> +	const u64 prefix = *((u64 *)desc->prefix);
> +	struct sha256_state sctx;
> +	struct embedded_fw *fw;
> +	u8 sha256[32];
> +	u64 i, size;
> +	void *map;
> +
> +	size = md->num_pages << EFI_PAGE_SHIFT;
> +	map = memremap(md->phys_addr, size, MEMREMAP_WB);

Since our limitaiton is the init process must have mostly finished,
it implies early x86 boot code cannot use this, what measures can we
take to prevent / check for such conditions to be detected and
gracefully errored out?

> +	if (!map) {
> +		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
> +		return -ENOMEM;
> +	}
> +
> +	size -= desc->length;

Remind me again, why we decrement the size here?
I was going to ask if we didn't need a:

if (desc->length > size) {
	memunmap(map);
	return -EINVAL;
}

> +	for (i = 0; i < size; i += 8) {
> +		u64 *mem = map + i;
> +
> +		if (*mem != prefix)
> +			continue;
> +
> +		sha256_init(&sctx);
> +		sha256_update(&sctx, map + i, desc->length);
> +		sha256_final(&sctx, sha256);
> +		if (memcmp(sha256, desc->sha256, 32) == 0)
> +			break;
> +	}
> +	if (i >= size) {
> +		memunmap(map);
> +		return -ENOENT;
> +	}
> +
> +	pr_info("Found EFI embedded fw '%s'\n", desc->name);

Otherwise looks good.

  Luis

Re: [PATCH v7 3/8] firmware: Rename FW_OPT_NOFALLBACK to FW_OPT_NOFALLBACK_SYSFS

[Luis Chamberlain]

On Fri, Oct 04, 2019 at 04:50:51PM +0200, Hans de Goede wrote:
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index 62ee90b4db56..665b350419cb 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -606,7 +606,7 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
>  		return false;
>  	}
>  
> -	if ((opt_flags & FW_OPT_NOFALLBACK))
> +	if ((opt_flags & FW_OPT_NOFALLBACK_SYSFS))
>  		return false;
>  
>  	/* Also permit LSMs and IMA to fail firmware sysfs fallback */
> @@ -630,10 +630,11 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
>   * interface. Userspace is in charge of loading the firmware through the sysfs
>   * loading interface. This sysfs fallback mechanism may be disabled completely
>   * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
> - * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
> - * flag, if so it would also disable the fallback mechanism. A system may want
> - * to enfoce the sysfs fallback mechanism at all times, it can do this by
> - * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
> + * If this false we check if the internal API caller set the
         ignore_sysfs_fallback set to true or force_sysfs_fallback is
	 set to false

Otherwise looks good. You can add:

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v7 4/8] firmware: Add new platform fallback mechanism and firmware_request_platform()

[Luis Chamberlain]

On Fri, Oct 04, 2019 at 04:50:52PM +0200, Hans de Goede wrote:
> diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
> index 0b2dfa6259c9..fec75895faae 100644
> --- a/drivers/base/firmware_loader/Makefile
> +++ b/drivers/base/firmware_loader/Makefile
> @@ -3,7 +3,7 @@
>  
>  obj-$(CONFIG_FW_LOADER_USER_HELPER) += fallback_table.o
>  obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
> -firmware_class-objs := main.o
> +firmware_class-objs := main.o fallback_platform.o
>  firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o

Why not just:

firmware_class-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += fallback_platform.o

>  obj-y += builtin/
> diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
> index 21063503e4ea..c4350f2e7cc2 100644
> --- a/drivers/base/firmware_loader/fallback.h
> +++ b/drivers/base/firmware_loader/fallback.h
> @@ -66,4 +66,6 @@ static inline void unregister_sysfs_loader(void)
>  }
>  #endif /* CONFIG_FW_LOADER_USER_HELPER */
>  
> +int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags);
> +

Inline this if not defined.

>  #endif /* __FIRMWARE_FALLBACK_H */
> diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
> new file mode 100644
> index 000000000000..7e9d730e36bf
> --- /dev/null
> +++ b/drivers/base/firmware_loader/fallback_platform.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/efi_embedded_fw.h>
> +#include <linux/property.h>
> +#include <linux/security.h>
> +#include <linux/vmalloc.h>
> +
> +#include "fallback.h"
> +#include "firmware.h"
> +
> +int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags)
> +{
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE

And we can do away with this eyesore.

Otherwise looks good!

  Luis

Re: [PATCH v7 4/8] firmware: Add new platform fallback mechanism and firmware_request_platform()

[Luis Chamberlain]

On Fri, Oct 04, 2019 at 04:17:33PM -0700, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Fri, Oct 04, 2019 at 04:50:52PM +0200, Hans de Goede wrote:
> > In some cases the platform's main firmware (e.g. the UEFI fw) may contain
> > an embedded copy of device 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 a new platform fallback mechanism to the firmware loader
> > which will try to lookup a device fw copy embedded in the platform's main
> > firmware if direct filesystem lookup fails.
> > 
> > Drivers which need such embedded fw copies can enable this fallback
> > mechanism by using the new firmware_request_platform() function.
> 
> Why would drivers not want to fetch firmware from system firmware if it
> is not present on disk? I would say let driver to opt-out of this
> fallback, but default request_firmware() should do it by default.

It is the otherw way around, this looks first for the file on disk, and
if not present it looks for the firmware sprinked on EFI firmware, if
the driver was expected this fallback option for the device.

  Luis

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis Chamberlain]

On Fri, Oct 11, 2019 at 04:31:26PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/11/19 4:10 PM, Luis Chamberlain wrote:
> > Hey Hans, thanks for staying on top of this and follow up! For some
> > reason the universe conspired against your first and last patch ([1/8],
> > [8/8]), and I never got them. Could you bounce these or resend in case
> > others confirm they also didn't get it?
> 
> I have received feedback from others on the first patch, so at least
> that one has reached others. I've bounced patches 1 and 8 to you.

Thanks, can you also bounce the feedback received?

  Luis

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Greg Kroah-Hartman]

On Fri, Oct 11, 2019 at 03:38:23PM +0000, Luis Chamberlain wrote:
> On Fri, Oct 11, 2019 at 04:31:26PM +0200, Hans de Goede wrote:
> > Hi,
> > 
> > On 10/11/19 4:10 PM, Luis Chamberlain wrote:
> > > Hey Hans, thanks for staying on top of this and follow up! For some
> > > reason the universe conspired against your first and last patch ([1/8],
> > > [8/8]), and I never got them. Could you bounce these or resend in case
> > > others confirm they also didn't get it?
> > 
> > I have received feedback from others on the first patch, so at least
> > that one has reached others. I've bounced patches 1 and 8 to you.
> 
> Thanks, can you also bounce the feedback received?

That is what lore.kernel.org is for...

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

[Luis Chamberlain]

On Fri, Oct 04, 2019 at 04:50:49PM +0200, Hans de Goede wrote:
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 8d3e778e988b..abba49c4c46d 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -314,6 +315,55 @@ 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
> +
> +static 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))
> +		return;
> +
> +	for_each_efi_memory_desc(md) {
> +		switch (md->type) {
> +		case EFI_BOOT_SERVICES_CODE:
> +			snprintf(name, sizeof(name), "boot_services_code%d",
> +				 type_count[md->type]++);
> +			break;
> +		case EFI_BOOT_SERVICES_DATA:
> +			snprintf(name, sizeof(name), "boot_services_data%d",
> +				 type_count[md->type]++);
> +			break;
> +		default:
> +			continue;
> +		}
> +
> +		debugfs_blob[i].size = md->num_pages << EFI_PAGE_SHIFT;
> +		debugfs_blob[i].data = memremap(md->phys_addr,
> +						debugfs_blob[i].size,
> +						MEMREMAP_WB);
> +		if (!debugfs_blob[i].data)
> +			continue;
> +
> +		debugfs_create_blob(name, 0400, efi_debugfs, &debugfs_blob[i]);
> +		i++;
> +		if (i == EFI_DEBUGFS_MAX_BLOBS)
> +			break;

Why do we silently ignore more entries ? And could documentation be
added for ways in which this could be used in practice?

  Luis

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis Chamberlain]

On Fri, Oct 11, 2019 at 06:38:19PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 11, 2019 at 03:38:23PM +0000, Luis Chamberlain wrote:
> > On Fri, Oct 11, 2019 at 04:31:26PM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 10/11/19 4:10 PM, Luis Chamberlain wrote:
> > > > Hey Hans, thanks for staying on top of this and follow up! For some
> > > > reason the universe conspired against your first and last patch ([1/8],
> > > > [8/8]), and I never got them. Could you bounce these or resend in case
> > > > others confirm they also didn't get it?
> > > 
> > > I have received feedback from others on the first patch, so at least
> > > that one has reached others. I've bounced patches 1 and 8 to you.
> > 
> > Thanks, can you also bounce the feedback received?
> 
> That is what lore.kernel.org is for...

If I have feedback on an email which I did not get I cannot easily reply to it.
In the future I'd like lore to let me bounce emails from a thread to me,
but that is not possible today AFAICT?

  Luis

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Greg Kroah-Hartman]

On Mon, Oct 14, 2019 at 09:22:16AM +0000, Luis Chamberlain wrote:
> On Fri, Oct 11, 2019 at 06:38:19PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Oct 11, 2019 at 03:38:23PM +0000, Luis Chamberlain wrote:
> > > On Fri, Oct 11, 2019 at 04:31:26PM +0200, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 10/11/19 4:10 PM, Luis Chamberlain wrote:
> > > > > Hey Hans, thanks for staying on top of this and follow up! For some
> > > > > reason the universe conspired against your first and last patch ([1/8],
> > > > > [8/8]), and I never got them. Could you bounce these or resend in case
> > > > > others confirm they also didn't get it?
> > > > 
> > > > I have received feedback from others on the first patch, so at least
> > > > that one has reached others. I've bounced patches 1 and 8 to you.
> > > 
> > > Thanks, can you also bounce the feedback received?
> > 
> > That is what lore.kernel.org is for...
> 
> If I have feedback on an email which I did not get I cannot easily reply to it.

I meant, use lore.kernel.org to download the mbox of the thread and then
use your email client to respond to whatever you need there.  This all
is public, no need to ask anyone else to bounce emails to you.

greg k-h

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis Chamberlain]

On Mon, Oct 14, 2019 at 11:29:29AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 14, 2019 at 09:22:16AM +0000, Luis Chamberlain wrote:
> > On Fri, Oct 11, 2019 at 06:38:19PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Oct 11, 2019 at 03:38:23PM +0000, Luis Chamberlain wrote:
> > > > On Fri, Oct 11, 2019 at 04:31:26PM +0200, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 10/11/19 4:10 PM, Luis Chamberlain wrote:
> > > > > > Hey Hans, thanks for staying on top of this and follow up! For some
> > > > > > reason the universe conspired against your first and last patch ([1/8],
> > > > > > [8/8]), and I never got them. Could you bounce these or resend in case
> > > > > > others confirm they also didn't get it?
> > > > > 
> > > > > I have received feedback from others on the first patch, so at least
> > > > > that one has reached others. I've bounced patches 1 and 8 to you.
> > > > 
> > > > Thanks, can you also bounce the feedback received?
> > > 
> > > That is what lore.kernel.org is for...
> > 
> > If I have feedback on an email which I did not get I cannot easily reply to it.
> 
> I meant, use lore.kernel.org to download the mbox of the thread and then
> use your email client to respond to whatever you need there.  This all
> is public, no need to ask anyone else to bounce emails to you.

Last I looked it didn't allow you to downlaod an mbox of a thread...
I'll take a look next time I miss a few emails.

  Luis

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Greg Kroah-Hartman]

On Mon, Oct 14, 2019 at 10:31:50AM +0000, Luis Chamberlain wrote:
> On Mon, Oct 14, 2019 at 11:29:29AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 14, 2019 at 09:22:16AM +0000, Luis Chamberlain wrote:
> > > On Fri, Oct 11, 2019 at 06:38:19PM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Oct 11, 2019 at 03:38:23PM +0000, Luis Chamberlain wrote:
> > > > > On Fri, Oct 11, 2019 at 04:31:26PM +0200, Hans de Goede wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 10/11/19 4:10 PM, Luis Chamberlain wrote:
> > > > > > > Hey Hans, thanks for staying on top of this and follow up! For some
> > > > > > > reason the universe conspired against your first and last patch ([1/8],
> > > > > > > [8/8]), and I never got them. Could you bounce these or resend in case
> > > > > > > others confirm they also didn't get it?
> > > > > > 
> > > > > > I have received feedback from others on the first patch, so at least
> > > > > > that one has reached others. I've bounced patches 1 and 8 to you.
> > > > > 
> > > > > Thanks, can you also bounce the feedback received?
> > > > 
> > > > That is what lore.kernel.org is for...
> > > 
> > > If I have feedback on an email which I did not get I cannot easily reply to it.
> > 
> > I meant, use lore.kernel.org to download the mbox of the thread and then
> > use your email client to respond to whatever you need there.  This all
> > is public, no need to ask anyone else to bounce emails to you.
> 
> Last I looked it didn't allow you to downlaod an mbox of a thread...

It can, from the front page of "all" threads, or on the thread itself,
at the bottom of the page.  Search for "download" on the page.

greg k-h

Re: [PATCH v7 0/8] efi/firmware/platform-x86: Add EFI embedded fw support

[Luis Chamberlain]

On Mon, Oct 14, 2019 at 12:57:54PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 14, 2019 at 10:31:50AM +0000, Luis Chamberlain wrote:
> > On Mon, Oct 14, 2019 at 11:29:29AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 14, 2019 at 09:22:16AM +0000, Luis Chamberlain wrote:
> > > > On Fri, Oct 11, 2019 at 06:38:19PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Oct 11, 2019 at 03:38:23PM +0000, Luis Chamberlain wrote:
> > > > > > On Fri, Oct 11, 2019 at 04:31:26PM +0200, Hans de Goede wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 10/11/19 4:10 PM, Luis Chamberlain wrote:
> > > > > > > > Hey Hans, thanks for staying on top of this and follow up! For some
> > > > > > > > reason the universe conspired against your first and last patch ([1/8],
> > > > > > > > [8/8]), and I never got them. Could you bounce these or resend in case
> > > > > > > > others confirm they also didn't get it?
> > > > > > > 
> > > > > > > I have received feedback from others on the first patch, so at least
> > > > > > > that one has reached others. I've bounced patches 1 and 8 to you.
> > > > > > 
> > > > > > Thanks, can you also bounce the feedback received?
> > > > > 
> > > > > That is what lore.kernel.org is for...
> > > > 
> > > > If I have feedback on an email which I did not get I cannot easily reply to it.
> > > 
> > > I meant, use lore.kernel.org to download the mbox of the thread and then
> > > use your email client to respond to whatever you need there.  This all
> > > is public, no need to ask anyone else to bounce emails to you.
> > 
> > Last I looked it didn't allow you to downlaod an mbox of a thread...
> 
> It can, from the front page of "all" threads, or on the thread itself,
> at the bottom of the page.  Search for "download" on the page.

Sweet! <insert sound of discovering an item in Legend of Zelda>

  Luis

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

[Hans de Goede]

Hi Luis,

Thank you for the reviews and sorry for being a bit slow to respind.

On 11-10-2019 16:48, Luis Chamberlain wrote:
> On Fri, Oct 04, 2019 at 04:50:50PM +0200, Hans de Goede wrote:
>> +static int __init efi_check_md_for_embedded_firmware(
>> +	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>> +{
>> +	const u64 prefix = *((u64 *)desc->prefix);
>> +	struct sha256_state sctx;
>> +	struct embedded_fw *fw;
>> +	u8 sha256[32];
>> +	u64 i, size;
>> +	void *map;
>> +
>> +	size = md->num_pages << EFI_PAGE_SHIFT;
>> +	map = memremap(md->phys_addr, size, MEMREMAP_WB);
> 
> Since our limitaiton is the init process must have mostly finished,
> it implies early x86 boot code cannot use this, what measures can we
> take to prevent / check for such conditions to be detected and
> gracefully errored out?

As with all (EFI) early boot code, there simply is a certain order
in which things need to be done. This needs to happen after the basic
mm is setup, but before efi_free_boot_services() gets called, there
isn't really a way to check for all these conditions. As with all
early boot code, people making changes need to be careful to not
break stuff.

> 
>> +	if (!map) {
>> +		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	size -= desc->length;
> 
> Remind me again, why we decrement the size here?

Basically this is another way of writing:

	for (i = 0; (i + desc->length) < size; i += 8) {

> I was going to ask if we didn't need a:
> 
> if (desc->length > size) {
> 	memunmap(map);
> 	return -EINVAL;
> }

That is a good point, unlikely but still a good point,
so I guess that writing:

	for (i = 0; (i + desc->length) < size; i += 8) {

Instead would better as that avoids the need for that check.
I will fix this for the next version.

Regards,

Hans

> 
>> +	for (i = 0; i < size; i += 8) {
>> +		u64 *mem = map + i;
>> +
>> +		if (*mem != prefix)
>> +			continue;
>> +
>> +		sha256_init(&sctx);
>> +		sha256_update(&sctx, map + i, desc->length);
>> +		sha256_final(&sctx, sha256);
>> +		if (memcmp(sha256, desc->sha256, 32) == 0)
>> +			break;
>> +	}
>> +	if (i >= size) {
>> +		memunmap(map);
>> +		return -ENOENT;
>> +	}
>> +
>> +	pr_info("Found EFI embedded fw '%s'\n", desc->name);
> 
> Otherwise looks good.
> 
>    Luis
> 

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

[Hans de Goede]

Hi,

On 14-10-2019 11:11, Luis Chamberlain wrote:
> On Fri, Oct 04, 2019 at 04:50:49PM +0200, Hans de Goede wrote:
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index 8d3e778e988b..abba49c4c46d 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -314,6 +315,55 @@ 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
>> +
>> +static 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))
>> +		return;
>> +
>> +	for_each_efi_memory_desc(md) {
>> +		switch (md->type) {
>> +		case EFI_BOOT_SERVICES_CODE:
>> +			snprintf(name, sizeof(name), "boot_services_code%d",
>> +				 type_count[md->type]++);
>> +			break;
>> +		case EFI_BOOT_SERVICES_DATA:
>> +			snprintf(name, sizeof(name), "boot_services_data%d",
>> +				 type_count[md->type]++);
>> +			break;
>> +		default:
>> +			continue;
>> +		}
>> +
>> +		debugfs_blob[i].size = md->num_pages << EFI_PAGE_SHIFT;
>> +		debugfs_blob[i].data = memremap(md->phys_addr,
>> +						debugfs_blob[i].size,
>> +						MEMREMAP_WB);
>> +		if (!debugfs_blob[i].data)
>> +			continue;
>> +
>> +		debugfs_create_blob(name, 0400, efi_debugfs, &debugfs_blob[i]);
>> +		i++;
>> +		if (i == EFI_DEBUGFS_MAX_BLOBS)
>> +			break;
> 
> Why do we silently ignore more entries ?

A valid remark, I agree that adding a pr_warn_once here would be good
I will do so for the next version.

> And could documentation be
> added for ways in which this could be used in practice?

I can write a little how to use this to get an embedded firmware,
I will add this to firmware/fallback-mechanisms.rst for the next
version.

Regards,

Hans

Re: [PATCH v7 4/8] firmware: Add new platform fallback mechanism and firmware_request_platform()

[Hans de Goede]

Hi,

On 11-10-2019 17:29, Luis Chamberlain wrote:
> On Fri, Oct 04, 2019 at 04:50:52PM +0200, Hans de Goede wrote:
>> diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
>> index 0b2dfa6259c9..fec75895faae 100644
>> --- a/drivers/base/firmware_loader/Makefile
>> +++ b/drivers/base/firmware_loader/Makefile
>> @@ -3,7 +3,7 @@
>>   
>>   obj-$(CONFIG_FW_LOADER_USER_HELPER) += fallback_table.o
>>   obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
>> -firmware_class-objs := main.o
>> +firmware_class-objs := main.o fallback_platform.o
>>   firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
> 
> Why not just:
> 
> firmware_class-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += fallback_platform.o
> 
>>   obj-y += builtin/
>> diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
>> index 21063503e4ea..c4350f2e7cc2 100644
>> --- a/drivers/base/firmware_loader/fallback.h
>> +++ b/drivers/base/firmware_loader/fallback.h
>> @@ -66,4 +66,6 @@ static inline void unregister_sysfs_loader(void)
>>   }
>>   #endif /* CONFIG_FW_LOADER_USER_HELPER */
>>   
>> +int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags);
>> +
> 
> Inline this if not defined.
> 
>>   #endif /* __FIRMWARE_FALLBACK_H */
>> diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
>> new file mode 100644
>> index 000000000000..7e9d730e36bf
>> --- /dev/null
>> +++ b/drivers/base/firmware_loader/fallback_platform.c
>> @@ -0,0 +1,33 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/efi_embedded_fw.h>
>> +#include <linux/property.h>
>> +#include <linux/security.h>
>> +#include <linux/vmalloc.h>
>> +
>> +#include "fallback.h"
>> +#include "firmware.h"
>> +
>> +int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags)
>> +{
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> 
> And we can do away with this eyesore.

Ok will fix for the next version.

> Otherwise looks good!

Thanks.

Regards,

Hans

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

[Luis Chamberlain]

On Thu, Nov 14, 2019 at 12:27:01PM +0100, Hans de Goede wrote:
> Hi Luis,
> 
> Thank you for the reviews and sorry for being a bit slow to respind.
> 
> On 11-10-2019 16:48, Luis Chamberlain wrote:
> > On Fri, Oct 04, 2019 at 04:50:50PM +0200, Hans de Goede wrote:
> > > +static int __init efi_check_md_for_embedded_firmware(
> > > +	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
> > > +{
> > > +	const u64 prefix = *((u64 *)desc->prefix);
> > > +	struct sha256_state sctx;
> > > +	struct embedded_fw *fw;
> > > +	u8 sha256[32];
> > > +	u64 i, size;
> > > +	void *map;
> > > +
> > > +	size = md->num_pages << EFI_PAGE_SHIFT;
> > > +	map = memremap(md->phys_addr, size, MEMREMAP_WB);
> > 
> > Since our limitaiton is the init process must have mostly finished,
> > it implies early x86 boot code cannot use this, what measures can we
> > take to prevent / check for such conditions to be detected and
> > gracefully errored out?
> 
> As with all (EFI) early boot code, there simply is a certain order
> in which things need to be done. This needs to happen after the basic
> mm is setup, but before efi_free_boot_services() gets called, there
> isn't really a way to check for all these conditions. As with all
> early boot code, people making changes need to be careful to not
> break stuff.

I rather we take a proactive measure here and add whatever it is we need
to ensure the API works only when its supposed to, rather than try and
fail, and then expect the user to know these things.

I'd prefer if we at least try to address this.

> > > +	if (!map) {
> > > +		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	size -= desc->length;
> > 
> > Remind me again, why we decrement the size here?
> 
> Basically this is another way of writing:
> 
> 	for (i = 0; (i + desc->length) < size; i += 8) {
> 
> > I was going to ask if we didn't need a:
> > 
> > if (desc->length > size) {
> > 	memunmap(map);
> > 	return -EINVAL;
> > }
> 
> That is a good point, unlikely but still a good point,
> so I guess that writing:
> 
> 	for (i = 0; (i + desc->length) < size; i += 8) {
> 
> Instead would better as that avoids the need for that check.
> I will fix this for the next version.

Great thanks.

  Luis

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

[Hans de Goede]

Hi,

On 14-11-2019 20:42, Luis Chamberlain wrote:
> On Thu, Nov 14, 2019 at 12:27:01PM +0100, Hans de Goede wrote:
>> Hi Luis,
>>
>> Thank you for the reviews and sorry for being a bit slow to respind.
>>
>> On 11-10-2019 16:48, Luis Chamberlain wrote:
>>> On Fri, Oct 04, 2019 at 04:50:50PM +0200, Hans de Goede wrote:
>>>> +static int __init efi_check_md_for_embedded_firmware(
>>>> +	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>>>> +{
>>>> +	const u64 prefix = *((u64 *)desc->prefix);
>>>> +	struct sha256_state sctx;
>>>> +	struct embedded_fw *fw;
>>>> +	u8 sha256[32];
>>>> +	u64 i, size;
>>>> +	void *map;
>>>> +
>>>> +	size = md->num_pages << EFI_PAGE_SHIFT;
>>>> +	map = memremap(md->phys_addr, size, MEMREMAP_WB);
>>>
>>> Since our limitaiton is the init process must have mostly finished,
>>> it implies early x86 boot code cannot use this, what measures can we
>>> take to prevent / check for such conditions to be detected and
>>> gracefully errored out?
>>
>> As with all (EFI) early boot code, there simply is a certain order
>> in which things need to be done. This needs to happen after the basic
>> mm is setup, but before efi_free_boot_services() gets called, there
>> isn't really a way to check for all these conditions. As with all
>> early boot code, people making changes need to be careful to not
>> break stuff.
> 
> I rather we take a proactive measure here and add whatever it is we need
> to ensure the API works only when its supposed to, rather than try and
> fail, and then expect the user to know these things.
> 
> I'd prefer if we at least try to address this.

This is purely internal x86/EFI API it is not intended for drivers
or anything like that. It has only one caller under arch/x86 and it is
not supposed to get any other callers outside of arch/* ever.

Note that this all runs before even core_initcall-s get run, none
if the code which runs before then has any sort of ordering checks
and I don't see how this bit is special and thus does need ordering
checks; and there really is no mechanism for such checks so early
during boot.

The drivers/firmware/efi/embedded-firmware.c file does add some API
which can be used normally, specifically the efi_get_embedded_fw()
but that has no special ordering constrains and it does not directly
use the function we are discussing now. It reads back data stored
by the earlier functions; and if somehow called before those functions
run (*), then it will simply return -ENOENT.

Regards,

Hans



*)  which would mean before core_initcalls run so really really early

Re: [PATCH v7 3/8] firmware: Rename FW_OPT_NOFALLBACK to FW_OPT_NOFALLBACK_SYSFS

[Hans de Goede]

Hi,

On 11-10-2019 17:02, Luis Chamberlain wrote:
> On Fri, Oct 04, 2019 at 04:50:51PM +0200, Hans de Goede wrote:
>> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
>> index 62ee90b4db56..665b350419cb 100644
>> --- a/drivers/base/firmware_loader/fallback.c
>> +++ b/drivers/base/firmware_loader/fallback.c
>> @@ -606,7 +606,7 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
>>   		return false;
>>   	}
>>   
>> -	if ((opt_flags & FW_OPT_NOFALLBACK))
>> +	if ((opt_flags & FW_OPT_NOFALLBACK_SYSFS))
>>   		return false;
>>   
>>   	/* Also permit LSMs and IMA to fail firmware sysfs fallback */
>> @@ -630,10 +630,11 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
>>    * interface. Userspace is in charge of loading the firmware through the sysfs
>>    * loading interface. This sysfs fallback mechanism may be disabled completely
>>    * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
>> - * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
>> - * flag, if so it would also disable the fallback mechanism. A system may want
>> - * to enfoce the sysfs fallback mechanism at all times, it can do this by
>> - * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
>> + * If this false we check if the internal API caller set the
>           ignore_sysfs_fallback set to true or force_sysfs_fallback is
> 	 set to false

I do not think that that is correct, looking at the code the order of
checks is:

	if (fw_fallback_config.ignore_sysfs_fallback)
		BAIL

	if (opt_flags & FW_OPT_NOFALLBACK_SYSFS)
		BAIL

	if (fw_fallback_config.force_sysfs_fallback)
		DO_FALLBACK (and return)

	if (!(opt_flags & FW_OPT_USERHELPER))
		BAIL

	DO_FALLBACK

So the original comment seems correct as FW_OPT_NOFALLBACK trumps / has
higher prio then force_sysfs_fallback.

Anyways I do not believe that fixing up/rewording this comment (if it needs
fixing) belongs in the commit/patch. This patch is purely about renaming
FW_OPT_NOFALLBACK to FW_OPT_NOFALLBACK_SYSFS, the lines changed in this
chunk are not changed, they are merely re-word/line-wrapped with the
exception of fixing the enfoce typo to enforce, as mentioned in the
commit message.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 14-11-2019 21:13, Hans de Goede wrote:
> Hi,
> 
> On 14-11-2019 20:42, Luis Chamberlain wrote:
>> On Thu, Nov 14, 2019 at 12:27:01PM +0100, Hans de Goede wrote:
>>> Hi Luis,
>>>
>>> Thank you for the reviews and sorry for being a bit slow to respind.
>>>
>>> On 11-10-2019 16:48, Luis Chamberlain wrote:
>>>> On Fri, Oct 04, 2019 at 04:50:50PM +0200, Hans de Goede wrote:
>>>>> +static int __init efi_check_md_for_embedded_firmware(
>>>>> +    efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>>>>> +{
>>>>> +    const u64 prefix = *((u64 *)desc->prefix);
>>>>> +    struct sha256_state sctx;
>>>>> +    struct embedded_fw *fw;
>>>>> +    u8 sha256[32];
>>>>> +    u64 i, size;
>>>>> +    void *map;
>>>>> +
>>>>> +    size = md->num_pages << EFI_PAGE_SHIFT;
>>>>> +    map = memremap(md->phys_addr, size, MEMREMAP_WB);
>>>>
>>>> Since our limitaiton is the init process must have mostly finished,
>>>> it implies early x86 boot code cannot use this, what measures can we
>>>> take to prevent / check for such conditions to be detected and
>>>> gracefully errored out?
>>>
>>> As with all (EFI) early boot code, there simply is a certain order
>>> in which things need to be done. This needs to happen after the basic
>>> mm is setup, but before efi_free_boot_services() gets called, there
>>> isn't really a way to check for all these conditions. As with all
>>> early boot code, people making changes need to be careful to not
>>> break stuff.
>>
>> I rather we take a proactive measure here and add whatever it is we need
>> to ensure the API works only when its supposed to, rather than try and
>> fail, and then expect the user to know these things.
>>
>> I'd prefer if we at least try to address this.
> 
> This is purely internal x86/EFI API it is not intended for drivers
> or anything like that. It has only one caller under arch/x86 and it is
> not supposed to get any other callers outside of arch/* ever.
> 
> Note that this all runs before even core_initcall-s get run, none
> if the code which runs before then has any sort of ordering checks
> and I don't see how this bit is special and thus does need ordering
> checks; and there really is no mechanism for such checks so early
> during boot.
> 
> The drivers/firmware/efi/embedded-firmware.c file does add some API
> which can be used normally, specifically the efi_get_embedded_fw()
> but that has no special ordering constrains and it does not directly
> use the function we are discussing now. It reads back data stored
> by the earlier functions; and if somehow called before those functions
> run (*), then it will simply return -ENOENT.

Ok, I just realized that we may have some miscommunication here,
when you wrote:

"Since our limitation is the init process must have mostly finished,
  it implies early x86 boot code cannot use this, what measures can we
  take to prevent / check for such conditions to be detected and
  gracefully errored out?"

I assumed you meant that to apply to the efi_check_md_for_embedded_firmware()
helper or its caller.

But I guess what you really want is some error to be thrown if someone
calls firmware_request_platform() before we are ready.

I guess I could make efi_check_for_embedded_firmwares() which scans
for known firmwares and saved a copy set a flag that it has run.

And then combine that with making efi_get_embedded_fw() (which underpins
firmware_request_platform()) print a warning when called if that flag
is not set yet.

That would mean though that some code which runs earlier then
a core_initcall would, would call firmware_request_platform() and
such code is generally expected to know what they are doing.

I just checked and the cpu microcode stuff which comes to mind
for this uses a late_initcall so runs long after efi_get_embedded_fw()
and I have a feeling that trying to use the fw_loader before
core_initcalls have run is going to end poorly anyways.

Still if you want I can add a pr_warn or maybe even a WARN_ON
to efi_get_embedded_fw() in case it somehow gets called before
efi_check_for_embedded_firmwares().

Regards,

Hans

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

[Luis Chamberlain]

On Thu, Nov 14, 2019 at 09:48:38PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 14-11-2019 21:13, Hans de Goede wrote:
> > Hi,
> > 
> > On 14-11-2019 20:42, Luis Chamberlain wrote:
> > > On Thu, Nov 14, 2019 at 12:27:01PM +0100, Hans de Goede wrote:
> > > > Hi Luis,
> > > > 
> > > > Thank you for the reviews and sorry for being a bit slow to respind.
> > > > 
> > > > On 11-10-2019 16:48, Luis Chamberlain wrote:
> > > > > On Fri, Oct 04, 2019 at 04:50:50PM +0200, Hans de Goede wrote:
> > > > > > +static int __init efi_check_md_for_embedded_firmware(
> > > > > > +    efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
> > > > > > +{
> > > > > > +    const u64 prefix = *((u64 *)desc->prefix);
> > > > > > +    struct sha256_state sctx;
> > > > > > +    struct embedded_fw *fw;
> > > > > > +    u8 sha256[32];
> > > > > > +    u64 i, size;
> > > > > > +    void *map;
> > > > > > +
> > > > > > +    size = md->num_pages << EFI_PAGE_SHIFT;
> > > > > > +    map = memremap(md->phys_addr, size, MEMREMAP_WB);
> > > > > 
> > > > > Since our limitaiton is the init process must have mostly finished,
> > > > > it implies early x86 boot code cannot use this, what measures can we
> > > > > take to prevent / check for such conditions to be detected and
> > > > > gracefully errored out?
> > > > 
> > > > As with all (EFI) early boot code, there simply is a certain order
> > > > in which things need to be done. This needs to happen after the basic
> > > > mm is setup, but before efi_free_boot_services() gets called, there
> > > > isn't really a way to check for all these conditions. As with all
> > > > early boot code, people making changes need to be careful to not
> > > > break stuff.
> > > 
> > > I rather we take a proactive measure here and add whatever it is we need
> > > to ensure the API works only when its supposed to, rather than try and
> > > fail, and then expect the user to know these things.
> > > 
> > > I'd prefer if we at least try to address this.
> > 
> > This is purely internal x86/EFI API it is not intended for drivers
> > or anything like that. It has only one caller under arch/x86 and it is
> > not supposed to get any other callers outside of arch/* ever.
> > 
> > Note that this all runs before even core_initcall-s get run, none
> > if the code which runs before then has any sort of ordering checks
> > and I don't see how this bit is special and thus does need ordering
> > checks; and there really is no mechanism for such checks so early
> > during boot.
> > 
> > The drivers/firmware/efi/embedded-firmware.c file does add some API
> > which can be used normally, specifically the efi_get_embedded_fw()
> > but that has no special ordering constrains and it does not directly
> > use the function we are discussing now. It reads back data stored
> > by the earlier functions; and if somehow called before those functions
> > run (*), then it will simply return -ENOENT.
> 
> Ok, I just realized that we may have some miscommunication here,
> when you wrote:
> 
> "Since our limitation is the init process must have mostly finished,
>  it implies early x86 boot code cannot use this, what measures can we
>  take to prevent / check for such conditions to be detected and
>  gracefully errored out?"
> 
> I assumed you meant that to apply to the efi_check_md_for_embedded_firmware()
> helper or its caller.
> 
> But I guess what you really want is some error to be thrown if someone
> calls firmware_request_platform() before we are ready.

Yes.

> I guess I could make efi_check_for_embedded_firmwares() which scans
> for known firmwares and saved a copy set a flag that it has run.
> 
> And then combine that with making efi_get_embedded_fw() (which underpins
> firmware_request_platform()) print a warning when called if that flag
> is not set yet.
> 
> That would mean though that some code which runs earlier then
> a core_initcall would, would call firmware_request_platform() and
> such code is generally expected to know what they are doing.
> 
> I just checked and the cpu microcode stuff which comes to mind
> for this uses a late_initcall so runs long after efi_get_embedded_fw()
> and I have a feeling that trying to use the fw_loader before
> core_initcalls have run is going to end poorly anyways.
>
> Still if you want I can add a pr_warn or maybe even a WARN_ON
> to efi_get_embedded_fw() in case it somehow gets called before
> efi_check_for_embedded_firmwares().

That'd be great.

  Luis

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

[Hans de Goede]

Hi,

On 14-11-2019 22:50, Luis Chamberlain wrote:
> On Thu, Nov 14, 2019 at 09:48:38PM +0100, Hans de Goede wrote:

<snip>

>> But I guess what you really want is some error to be thrown if someone
>> calls firmware_request_platform() before we are ready.
> 
> Yes.
> 
>> I guess I could make efi_check_for_embedded_firmwares() which scans
>> for known firmwares and saved a copy set a flag that it has run.
>>
>> And then combine that with making efi_get_embedded_fw() (which underpins
>> firmware_request_platform()) print a warning when called if that flag
>> is not set yet.

<snip>

> That'd be great.

So I've been working on this, my first though was to use WARN_ON as
calling this too early would be a bug, but there is a bunch of
normal circumstances where efi_check_for_embedded_firmwares() never
runs. One of the being classic BIOS boot, but e.g. also when running
paravirtualized in a paravirt env. using UEFI.

Normally we should not end up calling efi_get_embedded_fw() in those
cases, for one it is unlikely for any drivers using firmware_request_platform()
to be used in such an environment, and if we somehow do end up with
a case where firmware_request_platform() is called, since the EFI
emebedded fw fallback then will not work I would expect a copy of
the necessary fw to be under /lib/firmware so we never hit the fallback.

This all makes efi_get_embedded_fw() getting called in cases where
efi_check_for_embedded_firmwares() will never run unlikely, but not
impossible. Making a WARN_ON the wrong thing to do so for v8 of this
patch-set I will add a pr_warn for this.

Note I've looked into detecting all the circumstances where it is normal
for efi_check_for_embedded_firmwares() to never run, but after tracing
the call path leading up to it getting called I've found that a check
for that is complicated and more importantly error-prone and likely
to get out of sync with reality if any of the functions higher up
the call path ever change the conditions.

So a pr_warn it is, and since as explained one would normally not
expect to ever hit the fallback on systems where
efi_check_for_embedded_firmwares() does not get called, I see no
harm in simply always printing the warning if
efi_check_for_embedded_firmwares() was not called.

Regards,

Hans