[PATCH v2 0/3] Apple device properties

[PATCH v2 0/3] Apple device properties

[Lukas Wunner]

Resending this series as requested by Matt, now that 4.9-rc1 is out
which contains all prerequisites:

On Tue, Sep 13, 2016 at 12:29:21PM +0100, Matt Fleming wrote:
> I had a quick look over these series and nothing looks too crazy, but
> I still need to do an in-depth review.
> 
> Given the patch dependencies you outlined above, could you resubmit
> this after the v4.9 merge window closes? That way it won't be
> forgotten about.


Retrieve device properties from EFI on Macs before ExitBootServices is
called and assign them to devices (patch [2/3]). The devices that
properties pertain to are encoded as EFI Device Paths, so add a parser
for these (patch [1/3]). As a first use case, amend the Thunderbolt driver
to take advantage of the Device ROM supplied by EFI (patch [3/3]).


Changes since v1:

- Previously there were two separate patches for retrieving properties
  and assigning them to devices. These are now squashed together in
  patch [2/3]. (Requested by Matt Fleming.)

- The version of the EFI properties protocol as well as the properties
  payload is now checked.

- Applied a bit of polish all over.


Link to v1:
https://lkml.org/lkml/2016/7/27/218

Browseable on GitHub:
https://github.com/l1k/linux/commits/apple_properties_v2

Thanks,

Lukas


Lukas Wunner (3):
  efi: Add device path parser
  x86/efi: Retrieve and assign Apple device properties
  thunderbolt: Use Device ROM retrieved from EFI

 Documentation/kernel-parameters.txt     |   5 +
 arch/x86/boot/compressed/eboot.c        |  63 +++++++++
 arch/x86/include/uapi/asm/bootparam.h   |   1 +
 drivers/firmware/efi/Kconfig            |  18 +++
 drivers/firmware/efi/Makefile           |   2 +
 drivers/firmware/efi/apple-properties.c | 232 ++++++++++++++++++++++++++++++++
 drivers/firmware/efi/dev-path-parser.c  | 186 +++++++++++++++++++++++++
 drivers/thunderbolt/Kconfig             |   1 +
 drivers/thunderbolt/eeprom.c            |  42 ++++++
 drivers/thunderbolt/switch.c            |   2 +-
 include/linux/efi.h                     |  38 ++++++
 11 files changed, 589 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/apple-properties.c
 create mode 100644 drivers/firmware/efi/dev-path-parser.c

-- 
2.9.3

[PATCH v2 3/3] thunderbolt: Use Device ROM retrieved from EFI

[Lukas Wunner]

Macs with Thunderbolt 1 do not have a unit-specific DROM: The DROM is
empty with uid 0x1000000000000. (Apple started factory-burning a unit-
specific DROM with Thunderbolt 2.)

Instead, the NHI EFI driver supplies a DROM in a device property. Use
it if available. It's only available when booting with the efistub.
If it's not available, silently fall back to our hardcoded DROM.

The size of the DROM is always 256 bytes. The number is hardcoded into
the NHI EFI driver. This commit can deal with an arbitrary size however,
just in case they ever change that.

Background information: The EFI firmware volume contains ROM files for
the NHI, GMUX and several other chips as well as key material. This
strategy allows Apple to deploy ROM or key updates by simply publishing
an EFI firmware update on their website. Drivers do not access those
files directly but rather through a file server via EFI protocol
AC5E4829-A8FD-440B-AF33-9FFE013B12D8. Files are identified by GUID, the
NHI DROM has 339370BD-CFC6-4454-8EF7-704653120818.

The NHI EFI driver amends that file with a unit-specific uid. The uid
has 64 bit but its entropy is much lower: 24 bit represent the model,
24 bit are taken from a serial number, 16 bit are fixed. The NHI EFI
driver obtains the serial number via the DataHub protocol, copies it
into the DROM, calculates the CRC and submits the result as a device
property.

A modification is needed in the resume code where we currently read the
uid of all switches in the hierarchy to detect plug events that occurred
during sleep. On Thunderbolt 1 root switches this will now lead to a
mismatch between the uid of the empty DROM and the EFI DROM. Exempt the
root switch from this check: It's built in, so the uid should never
change. However we continue to *read* the uid of the root switch, this
seems like a good way to test its reachability after resume.

Cc: reverser-P1ImLx+pFc0@public.gmane.org
Cc: Andreas Noever <andreas.noever-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> [MacBookPro9,1]
Tested-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org> [MacBookPro11,3]
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/thunderbolt/Kconfig  |  1 +
 drivers/thunderbolt/eeprom.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/switch.c |  2 +-
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
index c121acc..0056df7 100644
--- a/drivers/thunderbolt/Kconfig
+++ b/drivers/thunderbolt/Kconfig
@@ -1,6 +1,7 @@
 menuconfig THUNDERBOLT
 	tristate "Thunderbolt support for Apple devices"
 	depends on PCI
+	select APPLE_PROPERTIES
 	select CRC32
 	help
 	  Cactus Ridge Thunderbolt Controller driver
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 2b9602c..785a171 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/crc32.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include "tb.h"
 
@@ -360,6 +361,39 @@ static int tb_drom_parse_entries(struct tb_switch *sw)
 }
 
 /**
+ * tb_drom_copy_efi - copy drom supplied by EFI to sw->drom if present
+ */
+static int tb_drom_copy_efi(struct tb_switch *sw, u16 *size)
+{
+	struct device *dev = &sw->tb->nhi->pdev->dev;
+	int len, res;
+
+	len = device_property_read_u8_array(dev, "ThunderboltDROM", NULL, 0);
+	if (len < 0 || len < sizeof(struct tb_drom_header))
+		return -EINVAL;
+
+	sw->drom = kmalloc(len, GFP_KERNEL);
+	if (!sw->drom)
+		return -ENOMEM;
+
+	res = device_property_read_u8_array(dev, "ThunderboltDROM", sw->drom,
+									len);
+	if (res)
+		goto err;
+
+	*size = ((struct tb_drom_header *)sw->drom)->data_len +
+							  TB_DROM_DATA_START;
+	if (*size > len)
+		goto err;
+
+	return 0;
+
+err:
+	kfree(sw->drom);
+	return -EINVAL;
+}
+
+/**
  * tb_drom_read - copy drom to sw->drom and parse it
  */
 int tb_drom_read(struct tb_switch *sw)
@@ -374,6 +408,13 @@ int tb_drom_read(struct tb_switch *sw)
 
 	if (tb_route(sw) == 0) {
 		/*
+		 * Apple's NHI EFI driver supplies a DROM for the root switch
+		 * in a device property. Use it if available.
+		 */
+		if (tb_drom_copy_efi(sw, &size) == 0)
+			goto parse;
+
+		/*
 		 * The root switch contains only a dummy drom (header only,
 		 * no entries). Hardcode the configuration here.
 		 */
@@ -418,6 +459,7 @@ int tb_drom_read(struct tb_switch *sw)
 	if (res)
 		goto err;
 
+parse:
 	header = (void *) sw->drom;
 
 	if (header->data_len + TB_DROM_DATA_START != size) {
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 9840fde..d11d00c 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -460,7 +460,7 @@ int tb_switch_resume(struct tb_switch *sw)
 		tb_sw_warn(sw, "uid read failed\n");
 		return err;
 	}
-	if (sw->uid != uid) {
+	if (tb_route(sw) && sw->uid != uid) {
 		tb_sw_info(sw,
 			"changed while suspended (uid %#llx -> %#llx)\n",
 			sw->uid, uid);
-- 
2.9.3

[PATCH v2 2/3] x86/efi: Retrieve and assign Apple device properties

[Lukas Wunner]

Apple's EFI drivers supply device properties which are needed to support
Macs optimally. They contain vital information which cannot be obtained
any other way (e.g. Thunderbolt Device ROM). They're also used to convey
the current device state so that OS drivers can pick up where EFI
drivers left (e.g. GPU mode setting).

There's an EFI driver dubbed "AAPL,PathProperties" which implements a
per-device key/value store. Other EFI drivers populate it using a custom
protocol. The macOS bootloader /usr/standalone/i386/boot.efi retrieves
the properties with the same protocol. The module AppleACPIPlatform.kext
subsequently merges them into the I/O Kit registry (see ioreg(8)) where
they can be queried by other kernel extensions and user space.

This commit extends the efistub to retrieve the device properties before
ExitBootServices is called. It assigns them to devices in an fs_initcall
so that they can be queried with the API in <linux/property.h>.

Note that the device properties will only be available if the kernel is
booted with the efistub. Distros should adjust their installers to
always use the efistub on Macs. grub with the "linux" directive will not
work unless the functionality of this commit is duplicated in grub.
(The "linuxefi" directive should work but is not included upstream as of
this writing.)

The custom protocol has GUID 91BD12FE-F6C3-44FB-A5B7-5122AB303AE0 and
looks like this:

typedef struct {
	unsigned long version; /* 0x10000 */
	efi_status_t (*get) (
		IN	struct apple_properties_protocol *this,
		IN	struct efi_dev_path *device,
		IN	efi_char16_t *property_name,
		OUT	void *buffer,
		IN OUT	u32 *buffer_len);
		/* EFI_SUCCESS, EFI_NOT_FOUND, EFI_BUFFER_TOO_SMALL */
	efi_status_t (*set) (
		IN	struct apple_properties_protocol *this,
		IN	struct efi_dev_path *device,
		IN	efi_char16_t *property_name,
		IN	void *property_value,
		IN	u32 property_value_len);
		/* allocates copies of property name and value */
		/* EFI_SUCCESS, EFI_OUT_OF_RESOURCES */
	efi_status_t (*del) (
		IN	struct apple_properties_protocol *this,
		IN	struct efi_dev_path *device,
		IN	efi_char16_t *property_name);
		/* EFI_SUCCESS, EFI_NOT_FOUND */
	efi_status_t (*get_all) (
		IN	struct apple_properties_protocol *this,
		OUT	void *buffer,
		IN OUT	u32 *buffer_len);
		/* EFI_SUCCESS, EFI_BUFFER_TOO_SMALL */
} apple_properties_protocol;

Thanks to @osxreverser for this blog post which was helpful in reverse
engineering Apple's EFI drivers and bootloader:
https://reverse.put.as/2016/06/25/apple-efi-firmware-passwords-and-the-scbo-myth/

If someone at Apple is reading this, please note there's a memory leak
in your implementation of the del() function as the property struct is
freed but the name and value allocations are not.

Neither the macOS bootloader nor Apple's EFI drivers check the protocol
version, but we do to avoid breakage if it's ever changed. It's been the
same since at least OS X 10.6 (2009).

The get_all() function conveniently fills a buffer with all properties
in marshalled form which can be passed to the kernel as a setup_data
payload. The number of device properties is dynamic and can change
between a first invocation of get_all() (to determine the buffer size)
and a second invocation (to retrieve the actual buffer), hence the
peculiar loop which does not finish until the buffer size settles.
The macOS bootloader does the same.

The setup_data payload is later on unmarshalled in an fs_initcall. The
idea is that most buses instantiate devices in "subsys" initcall level
and drivers are usually bound to these devices in "device" initcall
level, so we assign the properties in-between, i.e. in "fs" initcall
level.

This assumes that devices to which properties pertain are instantiated
from a "subsys" initcall or earlier. That should always be the case
since on macOS, AppleACPIPlatformExpert::matchEFIDevicePath() only
supports ACPI and PCI nodes and we've fully scanned those buses during
"subsys" initcall level.

The second assumption is that properties are only needed from a "device"
initcall or later. Seems reasonable to me, but should this ever not work
out, an alternative approach would be to store the property sets e.g. in
a btree early during boot. Then whenever device_add() is called, an EFI
Device Path would have to be constructed for the newly added device,
and looked up in the btree. That way, the property set could be assigned
to the device immediately on instantiation. And this would also work for
devices instantiated in a deferred fashion. It seems like this approach
would be more complicated and require more code. That doesn't seem
justified without a specific use case.

For comparison, the strategy on macOS is to assign properties to objects
in the ACPI namespace (AppleACPIPlatformExpert::mergeEFIProperties()).
That approach is definitely wrong as it fails for devices not present in
the namespace: The NHI EFI driver supplies properties for attached
Thunderbolt devices, yet on Macs with Thunderbolt 1 only one device
level behind the host controller is described in the namespace.
Consequently macOS cannot assign properties for chained devices. With
Thunderbolt 2 they started to describe three device levels behind host
controllers in the namespace but this grossly inflates the SSDT and
still fails if the user daisy-chained more than three devices.

We copy the property names and values from the setup_data payload to
swappable virtual memory and afterwards make the payload available to
the page allocator. This is just for the sake of good housekeeping, it
wouldn't occupy a meaningful amount of physical memory (4444 bytes on my
machine). Only the payload is freed, not the setup_data header since
otherwise we'd break the list linkage and we cannot safely update the
predecessor's ->next link because there's no locking for the list.

The payload is currently not passed on to kexec'ed kernels, same for PCI
ROMs retrieved by setup_efi_pci(). This can be added later if there is
demand by amending setup_efi_state(). The payload can then no longer be
made available to the page allocator of course.

Cc: reverser-P1ImLx+pFc0@public.gmane.org
Cc: grub-devel-mXXj517/zsQ@public.gmane.org
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Andreas Noever <andreas.noever-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> [MacBookPro9,1]
Tested-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org> [MacBookPro11,3]
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 Documentation/kernel-parameters.txt     |   5 +
 arch/x86/boot/compressed/eboot.c        |  63 +++++++++
 arch/x86/include/uapi/asm/bootparam.h   |   1 +
 drivers/firmware/efi/Kconfig            |  13 ++
 drivers/firmware/efi/Makefile           |   1 +
 drivers/firmware/efi/apple-properties.c | 232 ++++++++++++++++++++++++++++++++
 include/linux/efi.h                     |  17 +++
 7 files changed, 332 insertions(+)
 create mode 100644 drivers/firmware/efi/apple-properties.c

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 37babf9..86a31df 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1062,6 +1062,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	dscc4.setup=	[NET]
 
+	dump_apple_properties	[X86]
+			Dump name and content of EFI device properties on
+			x86 Macs.  Useful for driver authors to determine
+			what data is available or for reverse-engineering.
+
 	dyndbg[="val"]		[KNL,DYNAMIC_DEBUG]
 	module.dyndbg[="val"]
 			Enable debug messages at boot time.  See
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index cc69e37..ff01637 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -537,6 +537,63 @@ static void setup_efi_pci(struct boot_params *params)
 	efi_call_early(free_pool, pci_handle);
 }
 
+static void retrieve_apple_device_properties(struct boot_params *params)
+{
+	efi_guid_t guid = APPLE_PROPERTIES_PROTOCOL_GUID;
+	struct setup_data *data, *new;
+	efi_status_t status;
+	void *properties;
+	u32 size = 0;
+
+	status = efi_call_early(locate_protocol, &guid, NULL, &properties);
+	if (status != EFI_SUCCESS)
+		return;
+
+	if ((efi_is_64bit() ?
+			((apple_properties_protocol_64 *)properties)->version :
+			((apple_properties_protocol_32 *)properties)->version)
+								   != 0x10000) {
+		efi_printk(sys_table, "Unsupported properties proto version\n");
+		return;
+	}
+
+	efi_early->call(efi_is_64bit() ?
+			((apple_properties_protocol_64 *)properties)->get_all :
+			((apple_properties_protocol_32 *)properties)->get_all,
+			properties, NULL, &size);
+	if (!size)
+		return;
+
+	do {
+		status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+			size + sizeof(struct setup_data), &new);
+		if (status != EFI_SUCCESS) {
+			efi_printk(sys_table,
+				   "Failed to alloc mem for properties\n");
+			return;
+		}
+		status = efi_early->call(efi_is_64bit() ?
+			((apple_properties_protocol_64 *)properties)->get_all :
+			((apple_properties_protocol_32 *)properties)->get_all,
+			properties, new->data, &size);
+		if (status == EFI_BUFFER_TOO_SMALL)
+			efi_call_early(free_pool, new);
+	} while (status == EFI_BUFFER_TOO_SMALL);
+
+	new->type = SETUP_APPLE_PROPERTIES;
+	new->len  = size;
+	new->next = 0;
+
+	data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
+	if (!data)
+		params->hdr.setup_data = (unsigned long)new;
+	else {
+		while (data->next)
+			data = (struct setup_data *)(unsigned long)data->next;
+		data->next = (unsigned long)new;
+	}
+}
+
 static efi_status_t
 setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
 {
@@ -1068,6 +1125,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
 struct boot_params *efi_main(struct efi_config *c,
 			     struct boot_params *boot_params)
 {
+	efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
 	struct desc_ptr *gdt = NULL;
 	efi_loaded_image_t *image;
 	struct setup_header *hdr = &boot_params->hdr;
@@ -1098,6 +1156,11 @@ struct boot_params *efi_main(struct efi_config *c,
 
 	setup_efi_pci(boot_params);
 
+	if (!memcmp((void *)sys_table->fw_vendor, apple, sizeof(apple))) {
+		if (IS_ENABLED(CONFIG_APPLE_PROPERTIES))
+			retrieve_apple_device_properties(boot_params);
+	}
+
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
 				sizeof(*gdt), (void **)&gdt);
 	if (status != EFI_SUCCESS) {
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c18ce67..b10bf31 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -7,6 +7,7 @@
 #define SETUP_DTB			2
 #define SETUP_PCI			3
 #define SETUP_EFI			4
+#define SETUP_APPLE_PROPERTIES		5
 
 /* ram_size flags */
 #define RAMDISK_IMAGE_START_MASK	0x07FF
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 893fda4..2e78b0b 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -129,6 +129,19 @@ config EFI_TEST
 	  Say Y here to enable the runtime services support via /dev/efi_test.
 	  If unsure, say N.
 
+config APPLE_PROPERTIES
+	bool "Apple Device Properties"
+	depends on EFI_STUB && X86
+	select EFI_DEV_PATH_PARSER
+	select UCS2_STRING
+	help
+	  Retrieve properties from EFI on Apple Macs and assign them to
+	  devices, allowing for improved support of Apple hardware.
+	  Properties that would otherwise be missing include the
+	  Thunderbolt Device ROM and GPU configuration data.
+
+	  If unsure, say Y if you have a Mac.  Otherwise N.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 3e91ae3..ad67342 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
 obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
 obj-$(CONFIG_EFI_TEST)			+= test/
 obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
+obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
 
 arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
 obj-$(CONFIG_ARM)			+= $(arm-obj-y)
diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
new file mode 100644
index 0000000..3d08e1a
--- /dev/null
+++ b/drivers/firmware/efi/apple-properties.c
@@ -0,0 +1,232 @@
+/*
+ * apple-properties.c - EFI device properties on Macs
+ * Copyright (C) 2016 Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2) as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define pr_fmt(fmt) "apple-properties: " fmt
+
+#include <linux/bootmem.h>
+#include <linux/dmi.h>
+#include <linux/efi.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/ucs2_string.h>
+#include <asm/setup.h>
+
+static bool dump_properties __initdata;
+
+static int __init dump_properties_enable(char *arg)
+{
+	dump_properties = true;
+	return 0;
+}
+
+__setup("dump_apple_properties", dump_properties_enable);
+
+struct dev_header {
+	u32 len;
+	u32 prop_count;
+	struct efi_dev_path path[0];
+	/*
+	 * followed by key/value pairs, each key and value preceded by u32 len,
+	 * len includes itself, value may be empty (in which case its len is 4)
+	 */
+};
+
+struct properties_header {
+	u32 len;
+	u32 version;
+	u32 dev_count;
+	struct dev_header dev_header[0];
+};
+
+static u8 one __initdata = 1;
+
+static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
+					     struct device *dev, void *ptr,
+					     struct property_entry entry[])
+{
+	int i;
+
+	for (i = 0; i < dev_header->prop_count; i++) {
+		int remaining = dev_header->len - (ptr - (void *)dev_header);
+		u32 key_len, val_len;
+		char *key;
+
+		if (sizeof(key_len) > remaining)
+			break;
+		key_len = *(typeof(key_len) *)ptr;
+		if (key_len + sizeof(val_len) > remaining ||
+		    key_len < sizeof(key_len) + sizeof(efi_char16_t) ||
+		    *(efi_char16_t *)(ptr + sizeof(key_len)) == 0) {
+			dev_err(dev, "invalid property name len at %#zx\n",
+				ptr - (void *)dev_header);
+			break;
+		}
+		val_len = *(typeof(val_len) *)(ptr + key_len);
+		if (key_len + val_len > remaining ||
+		    val_len < sizeof(val_len)) {
+			dev_err(dev, "invalid property val len at %#zx\n",
+				ptr - (void *)dev_header + key_len);
+			break;
+		}
+
+		key = kzalloc((key_len - sizeof(key_len)) * 4 + 1, GFP_KERNEL);
+		if (!key) {
+			dev_err(dev, "cannot allocate property name\n");
+			break;
+		}
+		ucs2_as_utf8(key, ptr + sizeof(key_len),
+			     key_len - sizeof(key_len));
+
+		entry[i].name = key;
+		entry[i].is_array = true;
+		entry[i].length = val_len - sizeof(val_len);
+		entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
+		if (!entry[i].length) {
+			/* driver core doesn't accept empty properties */
+			entry[i].length = 1;
+			entry[i].pointer.raw_data = &one;
+		}
+
+		if (dump_properties) {
+			dev_info(dev, "property: %s\n", entry[i].name);
+			print_hex_dump(KERN_INFO, pr_fmt(), DUMP_PREFIX_OFFSET,
+				16, 1, entry[i].pointer.raw_data,
+				entry[i].length, true);
+		}
+
+		ptr += key_len + val_len;
+	}
+
+	if (i != dev_header->prop_count) {
+		dev_err(dev, "got %d device properties, expected %u\n", i,
+			dev_header->prop_count);
+		print_hex_dump(KERN_ERR, pr_fmt(), DUMP_PREFIX_OFFSET,
+			16, 1, dev_header, dev_header->len, true);
+	} else
+		dev_info(dev, "assigning %d device properties\n", i);
+}
+
+static int __init unmarshal_devices(struct properties_header *properties)
+{
+	size_t offset = offsetof(struct properties_header, dev_header[0]);
+
+	while (offset + sizeof(struct dev_header) < properties->len) {
+		struct dev_header *dev_header = (void *)properties + offset;
+		struct property_entry *entry = NULL;
+		struct device *dev;
+		int ret, i;
+		void *ptr;
+
+		if (offset + dev_header->len > properties->len) {
+			pr_err("invalid len in dev_header at %#zx\n", offset);
+			return -EINVAL;
+		}
+
+		ptr = dev_header->path;
+		ret = get_device_by_efi_path((struct efi_dev_path **)&ptr,
+			       dev_header->len - sizeof(*dev_header), &dev);
+		if (ret) {
+			pr_err("device path parse error %d at %#zx:\n", ret,
+			       ptr - (void *)dev_header);
+			print_hex_dump(KERN_ERR, pr_fmt(), DUMP_PREFIX_OFFSET,
+			       16, 1, dev_header, dev_header->len, true);
+			goto skip_device;
+		}
+
+		entry = kcalloc(dev_header->prop_count + 1, sizeof(*entry),
+				GFP_KERNEL);
+		if (!entry) {
+			dev_err(dev, "cannot allocate properties\n");
+			goto skip_device;
+		}
+
+		unmarshal_key_value_pairs(dev_header, dev, ptr, entry);
+		if (!entry[0].name)
+			goto skip_device;
+
+		ret = device_add_properties(dev, entry); /* makes deep copy */
+		if (ret)
+			dev_err(dev, "error %d assigning properties\n", ret);
+		for (i = 0; entry[i].name; i++)
+			kfree(entry[i].name);
+
+skip_device:
+		kfree(entry);
+		put_device(dev);
+		offset += dev_header->len;
+	}
+
+	return 0;
+}
+
+static int __init map_properties(void)
+{
+	struct properties_header *properties;
+	struct setup_data *data;
+	u32 data_len;
+	u64 pa_data;
+	int ret;
+
+	if (!dmi_match(DMI_SYS_VENDOR, "Apple Inc.") &&
+	    !dmi_match(DMI_SYS_VENDOR, "Apple Computer, Inc."))
+		return 0;
+
+	pa_data = boot_params.hdr.setup_data;
+	while (pa_data) {
+		data = ioremap(pa_data, sizeof(*data));
+		if (!data) {
+			pr_err("cannot map setup_data header\n");
+			return -ENOMEM;
+		}
+
+		if (data->type != SETUP_APPLE_PROPERTIES) {
+			pa_data = data->next;
+			iounmap(data);
+			continue;
+		}
+
+		data_len = data->len;
+		iounmap(data);
+		data = ioremap(pa_data, sizeof(*data) + data_len);
+		if (!data) {
+			pr_err("cannot map setup_data payload\n");
+			return -ENOMEM;
+		}
+
+		properties = (struct properties_header *)data->data;
+		if (properties->version == 1)
+			ret = unmarshal_devices(properties);
+		else {
+			pr_err("unsupported version:\n");
+			print_hex_dump(KERN_ERR, pr_fmt(), DUMP_PREFIX_OFFSET,
+			       16, 1, properties, properties->len, true);
+			ret = -ENOTSUPP;
+		}
+
+		/*
+		 * can only free the setup_data payload but not its header
+		 * to avoid breaking the chain of ->next pointers
+		 */
+		data->len = 0;
+		iounmap(data);
+		free_bootmem_late(pa_data + sizeof(*data), data_len);
+		return ret;
+	}
+	return 0;
+}
+
+fs_initcall(map_properties);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 4faf339..91f6153 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -443,6 +443,22 @@ typedef struct {
 #define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
 #define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
 
+typedef struct {
+	u32 version;
+	u32 get;
+	u32 set;
+	u32 del;
+	u32 get_all;
+} apple_properties_protocol_32;
+
+typedef struct {
+	u64 version;
+	u64 get;
+	u64 set;
+	u64 del;
+	u64 get_all;
+} apple_properties_protocol_64;
+
 /*
  * Types and defines for EFI ResetSystem
  */
@@ -591,6 +607,7 @@ void efi_native_runtime_setup(void);
 #define EFI_RNG_PROTOCOL_GUID			EFI_GUID(0x3152bca5, 0xeade, 0x433d,  0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
 #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
 #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
+#define APPLE_PROPERTIES_PROTOCOL_GUID		EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
 
 /*
  * This GUID is used to pass to the kernel proper the struct screen_info
-- 
2.9.3

[PATCH v2 1/3] efi: Add device path parser

[Lukas Wunner]

We're about to extended the efistub to retrieve device properties from
EFI on Apple Macs. The properties use EFI Device Paths to indicate the
device they belong to. This commit adds a parser which, given an EFI
Device Path, locates the corresponding struct device and returns a
reference to it.

Initially only ACPI and PCI Device Path nodes are supported, these are
the only types needed for Apple device properties (the corresponding
macOS function AppleACPIPlatformExpert::matchEFIDevicePath() does not
support any others). Further node types can be added with moderate
effort.

Apple device properties is currently the only use case of this parser,
but since this may be useful to others, make it a separate component
which can be selected with config option EFI_DEV_PATH_PARSER. It can
in principle be compiled as a module if acpi_get_first_physical_node()
and acpi_bus_type are exported (and get_device_by_efi_path() itself is
exported).

The dependency on CONFIG_ACPI is needed for acpi_match_device_ids().
It can be removed if an empty inline stub is added for that function.

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/firmware/efi/Kconfig           |   5 +
 drivers/firmware/efi/Makefile          |   1 +
 drivers/firmware/efi/dev-path-parser.c | 186 +++++++++++++++++++++++++++++++++
 include/linux/efi.h                    |  21 ++++
 4 files changed, 213 insertions(+)
 create mode 100644 drivers/firmware/efi/dev-path-parser.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index c981be1..893fda4 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -133,3 +133,8 @@ endmenu
 
 config UEFI_CPER
 	bool
+
+config EFI_DEV_PATH_PARSER
+	bool
+	depends on ACPI
+	default n
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index c8a439f..3e91ae3 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_EFI_STUB)			+= libstub/
 obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
 obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
 obj-$(CONFIG_EFI_TEST)			+= test/
+obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
 
 arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
 obj-$(CONFIG_ARM)			+= $(arm-obj-y)
diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
new file mode 100644
index 0000000..549f80b
--- /dev/null
+++ b/drivers/firmware/efi/dev-path-parser.c
@@ -0,0 +1,186 @@
+/*
+ * dev-path-parser.c - EFI Device Path parser
+ * Copyright (C) 2016 Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2) as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/acpi.h>
+#include <linux/efi.h>
+#include <linux/pci.h>
+
+struct acpi_hid_uid {
+	struct acpi_device_id hid[2];
+	char uid[11]; /* UINT_MAX + null byte */
+};
+
+static int __init acpi_match(struct device *dev, void *data)
+{
+	struct acpi_hid_uid hid_uid = *(struct acpi_hid_uid *)data;
+	struct acpi_device *adev = to_acpi_device(dev);
+
+	if (acpi_match_device_ids(adev, hid_uid.hid))
+		return 0;
+
+	if (adev->pnp.unique_id)
+		return !strcmp(adev->pnp.unique_id, hid_uid.uid);
+	else
+		return !strcmp("0", hid_uid.uid);
+}
+
+static int __init get_device_by_acpi_path(struct efi_dev_path *node,
+					  struct device *parent,
+					  struct device **child)
+{
+	struct acpi_hid_uid hid_uid = {};
+	struct device *phys_dev;
+
+	if (node->length != 12)
+		return -EINVAL;
+
+	sprintf(hid_uid.hid[0].id, "%c%c%c%04X",
+		'A' + ((node->acpi.hid >> 10) & 0x1f) - 1,
+		'A' + ((node->acpi.hid >>  5) & 0x1f) - 1,
+		'A' + ((node->acpi.hid >>  0) & 0x1f) - 1,
+			node->acpi.hid >> 16);
+	sprintf(hid_uid.uid, "%u", node->acpi.uid);
+
+	*child = bus_find_device(&acpi_bus_type, NULL, &hid_uid, acpi_match);
+	if (!*child)
+		return -ENODEV;
+
+	phys_dev = acpi_get_first_physical_node(to_acpi_device(*child));
+	if (phys_dev) {
+		get_device(phys_dev);
+		put_device(*child);
+		*child = phys_dev;
+	}
+
+	return 0;
+}
+
+static int __init pci_match(struct device *dev, void *data)
+{
+	unsigned int devfn = *(unsigned int *)data;
+
+	return dev_is_pci(dev) && to_pci_dev(dev)->devfn == devfn;
+}
+
+static int __init get_device_by_pci_path(struct efi_dev_path *node,
+					 struct device *parent,
+					 struct device **child)
+{
+	unsigned int devfn;
+
+	if (node->length != 6)
+		return -EINVAL;
+	if (!parent)
+		return -EINVAL;
+
+	devfn = PCI_DEVFN(node->pci.dev, node->pci.fn);
+
+	*child = device_find_child(parent, &devfn, pci_match);
+	if (!*child)
+		return -ENODEV;
+
+	return 0;
+}
+
+/*
+ * Insert parsers for further node types here.
+ *
+ * Each parser takes a pointer to the @node and to the @parent (will be NULL
+ * for the first device path node). If a device corresponding to @node was
+ * found below @parent, its reference count should be incremented and the
+ * device returned in @child.
+ *
+ * The return value should be 0 on success or a negative int on failure.
+ * The special return value 1 signals the end of the device path, only
+ * get_device_by_end_path() is supposed to return this.
+ *
+ * Be sure to validate the node length and contents before commencing the
+ * search for a device.
+ */
+
+static int __init get_device_by_end_path(struct efi_dev_path *node,
+					 struct device *parent,
+					 struct device **child)
+{
+	if (node->length != 4)
+		return -EINVAL;
+	if (!parent)
+		return -ENODEV;
+
+	*child = get_device(parent);
+	return 1;
+}
+
+/**
+ * get_device_by_efi_path - find device by EFI Device Path
+ * @node: EFI Device Path
+ * @len: maximum length of EFI Device Path in bytes
+ * @dev: device found
+ *
+ * Parse a series of EFI Device Path nodes at @node and find the corresponding
+ * device. If the device was found, its reference count is incremented and a
+ * pointer to it is returned in @dev. The caller needs to drop the reference
+ * with put_device() after use. The @node pointer is updated to point to the
+ * location immediately after the "End Entire Device Path" node.
+ *
+ * If a Device Path node is malformed or its corresponding device is not found,
+ * @node is updated to point to this offending node and @dev will be %NULL.
+ *
+ * Most buses instantiate devices in "subsys" initcall level, hence the
+ * earliest initcall level in which this function should be called is "fs".
+ *
+ * Returns 0 on success,
+ *	   %-ENODEV if no device was found,
+ *	   %-EINVAL if a node is malformed or exceeds @len,
+ *	   %-ENOTSUPP if support for a node type is not yet implemented.
+ */
+int __init get_device_by_efi_path(struct efi_dev_path **node, size_t len,
+				  struct device **dev)
+{
+	struct device *parent = NULL, *child;
+	int ret = 0;
+
+	*dev = NULL;
+
+	while (ret != 1) {
+		if (len < 4 || len < (*node)->length)
+			ret = -EINVAL;
+		else if ((*node)->type     == EFI_DEV_ACPI &&
+			 (*node)->sub_type == EFI_DEV_BASIC_ACPI)
+			ret = get_device_by_acpi_path(*node, parent, &child);
+		else if ((*node)->type     == EFI_DEV_HW &&
+			 (*node)->sub_type == EFI_DEV_PCI)
+			ret = get_device_by_pci_path(*node, parent, &child);
+		else if (((*node)->type    == EFI_DEV_END_PATH ||
+			  (*node)->type    == EFI_DEV_END_PATH2) &&
+			 (*node)->sub_type == EFI_DEV_END_ENTIRE)
+			ret = get_device_by_end_path(*node, parent, &child);
+		else
+			ret = -ENOTSUPP;
+
+		put_device(parent);
+		if (ret < 0)
+			return ret;
+
+		parent = child;
+		*node  = (void *)*node + (*node)->length;
+		len   -= (*node)->length;
+	}
+
+	*dev = child;
+	return 0;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2d08948..4faf339 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1145,6 +1145,27 @@ struct efi_generic_dev_path {
 	u16 length;
 } __attribute ((packed));
 
+struct efi_dev_path {
+	u8 type;	/* can be replaced with unnamed */
+	u8 sub_type;	/* struct efi_generic_dev_path; */
+	u16 length;	/* once we've moved to -std=c11 */
+	union {
+		struct {
+			u32 hid;
+			u32 uid;
+		} acpi;
+		struct {
+			u8 fn;
+			u8 dev;
+		} pci;
+	};
+} __attribute ((packed));
+
+#if IS_ENABLED(CONFIG_EFI_DEV_PATH_PARSER)
+int get_device_by_efi_path(struct efi_dev_path **node, size_t len,
+			   struct device **dev);
+#endif
+
 static inline void memrange_efi_to_native(u64 *addr, u64 *npages)
 {
 	*npages = PFN_UP(*addr + (*npages<<EFI_PAGE_SHIFT)) - PFN_DOWN(*addr);
-- 
2.9.3

Re: [PATCH v2 1/3] efi: Add device path parser

[Matt Fleming]

(Cc'ing Peter since he's played with device path parsing before)

On Mon, 17 Oct, at 12:57:23PM, Lukas Wunner wrote:
> We're about to extended the efistub to retrieve device properties from
> EFI on Apple Macs. The properties use EFI Device Paths to indicate the
> device they belong to. This commit adds a parser which, given an EFI
> Device Path, locates the corresponding struct device and returns a
> reference to it.
> 
> Initially only ACPI and PCI Device Path nodes are supported, these are
> the only types needed for Apple device properties (the corresponding
> macOS function AppleACPIPlatformExpert::matchEFIDevicePath() does not
> support any others). Further node types can be added with moderate
> effort.
> 
> Apple device properties is currently the only use case of this parser,
> but since this may be useful to others, make it a separate component
> which can be selected with config option EFI_DEV_PATH_PARSER. It can
> in principle be compiled as a module if acpi_get_first_physical_node()
> and acpi_bus_type are exported (and get_device_by_efi_path() itself is
> exported).
> 
> The dependency on CONFIG_ACPI is needed for acpi_match_device_ids().
> It can be removed if an empty inline stub is added for that function.
> 
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/firmware/efi/Kconfig           |   5 +
>  drivers/firmware/efi/Makefile          |   1 +
>  drivers/firmware/efi/dev-path-parser.c | 186 +++++++++++++++++++++++++++++++++
>  include/linux/efi.h                    |  21 ++++
>  4 files changed, 213 insertions(+)
>  create mode 100644 drivers/firmware/efi/dev-path-parser.c
> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index c981be1..893fda4 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -133,3 +133,8 @@ endmenu
>  
>  config UEFI_CPER
>  	bool
> +
> +config EFI_DEV_PATH_PARSER
> +	bool
> +	depends on ACPI
> +	default n
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index c8a439f..3e91ae3 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_EFI_STUB)			+= libstub/
>  obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
>  obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
>  obj-$(CONFIG_EFI_TEST)			+= test/
> +obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
>  
>  arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
>  obj-$(CONFIG_ARM)			+= $(arm-obj-y)
> diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
> new file mode 100644
> index 0000000..549f80b
> --- /dev/null
> +++ b/drivers/firmware/efi/dev-path-parser.c
> @@ -0,0 +1,186 @@
> +/*
> + * dev-path-parser.c - EFI Device Path parser
> + * Copyright (C) 2016 Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2) as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/efi.h>
> +#include <linux/pci.h>
> +
> +struct acpi_hid_uid {
> +	struct acpi_device_id hid[2];
> +	char uid[11]; /* UINT_MAX + null byte */
> +};
> +
> +static int __init acpi_match(struct device *dev, void *data)
> +{
> +	struct acpi_hid_uid hid_uid = *(struct acpi_hid_uid *)data;
> +	struct acpi_device *adev = to_acpi_device(dev);
> +
> +	if (acpi_match_device_ids(adev, hid_uid.hid))
> +		return 0;
> +
> +	if (adev->pnp.unique_id)
> +		return !strcmp(adev->pnp.unique_id, hid_uid.uid);
> +	else
> +		return !strcmp("0", hid_uid.uid);
> +}
> +
> +static int __init get_device_by_acpi_path(struct efi_dev_path *node,
> +					  struct device *parent,
> +					  struct device **child)
> +{
> +	struct acpi_hid_uid hid_uid = {};
> +	struct device *phys_dev;
> +
> +	if (node->length != 12)
> +		return -EINVAL;
> +
> +	sprintf(hid_uid.hid[0].id, "%c%c%c%04X",
> +		'A' + ((node->acpi.hid >> 10) & 0x1f) - 1,
> +		'A' + ((node->acpi.hid >>  5) & 0x1f) - 1,
> +		'A' + ((node->acpi.hid >>  0) & 0x1f) - 1,
> +			node->acpi.hid >> 16);
> +	sprintf(hid_uid.uid, "%u", node->acpi.uid);
> +
> +	*child = bus_find_device(&acpi_bus_type, NULL, &hid_uid, acpi_match);
> +	if (!*child)
> +		return -ENODEV;
> +
> +	phys_dev = acpi_get_first_physical_node(to_acpi_device(*child));
> +	if (phys_dev) {
> +		get_device(phys_dev);
> +		put_device(*child);
> +		*child = phys_dev;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init pci_match(struct device *dev, void *data)
> +{
> +	unsigned int devfn = *(unsigned int *)data;
> +
> +	return dev_is_pci(dev) && to_pci_dev(dev)->devfn == devfn;
> +}
> +
> +static int __init get_device_by_pci_path(struct efi_dev_path *node,
> +					 struct device *parent,
> +					 struct device **child)
> +{
> +	unsigned int devfn;
> +
> +	if (node->length != 6)
> +		return -EINVAL;
> +	if (!parent)
> +		return -EINVAL;
> +
> +	devfn = PCI_DEVFN(node->pci.dev, node->pci.fn);
> +
> +	*child = device_find_child(parent, &devfn, pci_match);
> +	if (!*child)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +/*
> + * Insert parsers for further node types here.
> + *
> + * Each parser takes a pointer to the @node and to the @parent (will be NULL
> + * for the first device path node). If a device corresponding to @node was
> + * found below @parent, its reference count should be incremented and the
> + * device returned in @child.
> + *
> + * The return value should be 0 on success or a negative int on failure.
> + * The special return value 1 signals the end of the device path, only
> + * get_device_by_end_path() is supposed to return this.
> + *
> + * Be sure to validate the node length and contents before commencing the
> + * search for a device.
> + */
> +
> +static int __init get_device_by_end_path(struct efi_dev_path *node,
> +					 struct device *parent,
> +					 struct device **child)
> +{
> +	if (node->length != 4)
> +		return -EINVAL;
> +	if (!parent)
> +		return -ENODEV;
> +
> +	*child = get_device(parent);
> +	return 1;
> +}
> +
> +/**
> + * get_device_by_efi_path - find device by EFI Device Path
> + * @node: EFI Device Path
> + * @len: maximum length of EFI Device Path in bytes
> + * @dev: device found
> + *
> + * Parse a series of EFI Device Path nodes at @node and find the corresponding
> + * device. If the device was found, its reference count is incremented and a
> + * pointer to it is returned in @dev. The caller needs to drop the reference
> + * with put_device() after use. The @node pointer is updated to point to the
> + * location immediately after the "End Entire Device Path" node.
> + *
> + * If a Device Path node is malformed or its corresponding device is not found,
> + * @node is updated to point to this offending node and @dev will be %NULL.
> + *
> + * Most buses instantiate devices in "subsys" initcall level, hence the
> + * earliest initcall level in which this function should be called is "fs".
> + *
> + * Returns 0 on success,
> + *	   %-ENODEV if no device was found,
> + *	   %-EINVAL if a node is malformed or exceeds @len,
> + *	   %-ENOTSUPP if support for a node type is not yet implemented.
> + */
> +int __init get_device_by_efi_path(struct efi_dev_path **node, size_t len,
> +				  struct device **dev)
> +{
> +	struct device *parent = NULL, *child;
> +	int ret = 0;
> +
> +	*dev = NULL;
> +
> +	while (ret != 1) {
> +		if (len < 4 || len < (*node)->length)
> +			ret = -EINVAL;
> +		else if ((*node)->type     == EFI_DEV_ACPI &&
> +			 (*node)->sub_type == EFI_DEV_BASIC_ACPI)
> +			ret = get_device_by_acpi_path(*node, parent, &child);
> +		else if ((*node)->type     == EFI_DEV_HW &&
> +			 (*node)->sub_type == EFI_DEV_PCI)
> +			ret = get_device_by_pci_path(*node, parent, &child);
> +		else if (((*node)->type    == EFI_DEV_END_PATH ||
> +			  (*node)->type    == EFI_DEV_END_PATH2) &&
> +			 (*node)->sub_type == EFI_DEV_END_ENTIRE)
> +			ret = get_device_by_end_path(*node, parent, &child);
> +		else
> +			ret = -ENOTSUPP;
> +
> +		put_device(parent);
> +		if (ret < 0)
> +			return ret;
> +
> +		parent = child;
> +		*node  = (void *)*node + (*node)->length;
> +		len   -= (*node)->length;
> +	}
> +
> +	*dev = child;
> +	return 0;
> +}

Where in your patch series is this function called? Am I missing
something?

Also, unless there's some existing namespace with "get_device_by_*"
I'd prefer for this function name to have "efi_" as the prefix.

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 2d08948..4faf339 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1145,6 +1145,27 @@ struct efi_generic_dev_path {
>  	u16 length;
>  } __attribute ((packed));
>  
> +struct efi_dev_path {
> +	u8 type;	/* can be replaced with unnamed */
> +	u8 sub_type;	/* struct efi_generic_dev_path; */
> +	u16 length;	/* once we've moved to -std=c11 */
> +	union {
> +		struct {
> +			u32 hid;
> +			u32 uid;
> +		} acpi;
> +		struct {
> +			u8 fn;
> +			u8 dev;
> +		} pci;
> +	};
> +} __attribute ((packed));
> +
> +#if IS_ENABLED(CONFIG_EFI_DEV_PATH_PARSER)
> +int get_device_by_efi_path(struct efi_dev_path **node, size_t len,
> +			   struct device **dev);
> +#endif
> +
>  static inline void memrange_efi_to_native(u64 *addr, u64 *npages)
>  {
>  	*npages = PFN_UP(*addr + (*npages<<EFI_PAGE_SHIFT)) - PFN_DOWN(*addr);
> -- 
> 2.9.3
> 

Re: [PATCH v2 1/3] efi: Add device path parser

[Lukas Wunner]

On Wed, Oct 19, 2016 at 12:17:28PM +0100, Matt Fleming wrote:
> On Mon, 17 Oct, at 12:57:23PM, Lukas Wunner wrote:
> > +/**
> > + * get_device_by_efi_path - find device by EFI Device Path
> > + * @node: EFI Device Path
> > + * @len: maximum length of EFI Device Path in bytes
> > + * @dev: device found
> > + *
> > + * Parse a series of EFI Device Path nodes at @node and find the corresponding
> > + * device. If the device was found, its reference count is incremented and a
> > + * pointer to it is returned in @dev. The caller needs to drop the reference
> > + * with put_device() after use. The @node pointer is updated to point to the
> > + * location immediately after the "End Entire Device Path" node.
> > + *
> > + * If a Device Path node is malformed or its corresponding device is not found,
> > + * @node is updated to point to this offending node and @dev will be %NULL.
> > + *
> > + * Most buses instantiate devices in "subsys" initcall level, hence the
> > + * earliest initcall level in which this function should be called is "fs".
> > + *
> > + * Returns 0 on success,
> > + *	   %-ENODEV if no device was found,
> > + *	   %-EINVAL if a node is malformed or exceeds @len,
> > + *	   %-ENOTSUPP if support for a node type is not yet implemented.
> > + */
> > +int __init get_device_by_efi_path(struct efi_dev_path **node, size_t len,
> > +				  struct device **dev)
> > +{
> > +	struct device *parent = NULL, *child;
> > +	int ret = 0;
> > +
> > +	*dev = NULL;
> > +
> > +	while (ret != 1) {
> > +		if (len < 4 || len < (*node)->length)
> > +			ret = -EINVAL;
> > +		else if ((*node)->type     == EFI_DEV_ACPI &&
> > +			 (*node)->sub_type == EFI_DEV_BASIC_ACPI)
> > +			ret = get_device_by_acpi_path(*node, parent, &child);
> > +		else if ((*node)->type     == EFI_DEV_HW &&
> > +			 (*node)->sub_type == EFI_DEV_PCI)
> > +			ret = get_device_by_pci_path(*node, parent, &child);
> > +		else if (((*node)->type    == EFI_DEV_END_PATH ||
> > +			  (*node)->type    == EFI_DEV_END_PATH2) &&
> > +			 (*node)->sub_type == EFI_DEV_END_ENTIRE)
> > +			ret = get_device_by_end_path(*node, parent, &child);
> > +		else
> > +			ret = -ENOTSUPP;
> > +
> > +		put_device(parent);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		parent = child;
> > +		*node  = (void *)*node + (*node)->length;
> > +		len   -= (*node)->length;
> > +	}
> > +
> > +	*dev = child;
> > +	return 0;
> > +}
> 
> Where in your patch series is this function called? Am I missing
> something?

This is called by unmarshal_devices() in patch [2/3] of this series.


> Also, unless there's some existing namespace with "get_device_by_*"
> I'd prefer for this function name to have "efi_" as the prefix.

There's get_device() defined in include/linux/device.h which returns a
reference to a device, this function here is basically named after it
because it likewise returns a reference.

How about efi_get_device_by_path()?

Thanks,

Lukas

Re: [PATCH v2 1/3] efi: Add device path parser

[Matt Fleming]

On Wed, 19 Oct, at 01:51:19PM, Lukas Wunner wrote:
> > Where in your patch series is this function called? Am I missing
> > something?
> 
> This is called by unmarshal_devices() in patch [2/3] of this series.
 
Where is PATCH 2? I can't find it in my inbox or on linux-efi.

> > Also, unless there's some existing namespace with "get_device_by_*"
> > I'd prefer for this function name to have "efi_" as the prefix.
> 
> There's get_device() defined in include/linux/device.h which returns a
> reference to a device, this function here is basically named after it
> because it likewise returns a reference.
> 
> How about efi_get_device_by_path()?

That'd be great, thanks!

Re: [PATCH v2 1/3] efi: Add device path parser

[Lukas Wunner]

On Tue, Oct 25, 2016 at 01:44:06PM +0100, Matt Fleming wrote:
> On Wed, 19 Oct, at 01:51:19PM, Lukas Wunner wrote:
> > > Where in your patch series is this function called? Am I missing
> > > something?
> > 
> > This is called by unmarshal_devices() in patch [2/3] of this series.
>  
> Where is PATCH 2? I can't find it in my inbox or on linux-efi.

I notice you're using gmail, perhaps the message was classified as spam
due to its length (570 lines, 21 KByte)?  I definitely got the full series
back from vger.kernel.org as a subscriber to linux-efi@.

If all else fails, the patches can be viewed or fetched from GitHub:
https://github.com/l1k/linux/commits/apple_properties_v2

Direct link to patch [2/3]:
https://github.com/l1k/linux/commit/ac8b37bbe4b6

Thanks,

Lukas

Re: [PATCH v2 1/3] efi: Add device path parser

[Peter Jones]

On Wed, Oct 19, 2016 at 12:17:28PM +0100, Matt Fleming wrote:
> (Cc'ing Peter since he's played with device path parsing before)

So the only two thoughts I have:

1) I think we should probably be a bit more aggressive about bounds
   checking.  Every once in a while there are implementations that build
   device paths from templates and occasionally leave
   efi_dev_path.length as 0 or 0xffff, or forget to put an End Entire
   Device Path node at the end (or put an End Device Path Instance in
   instead.)  So checking for ->length < PAGE_SIZE and that when a dp
   node crosses a page boundary, the new page is actually in the same map
   as the previous one wouldn't be a terrible idea.
2) I think we should decide how multiple-instance device path are going
   to work sooner rather than later, mostly because I have a use for them
   and it seems harder to graft them in later.  My first idea here is to
   make get_device_by_efi_path() return 0 for "stop iterating" and 1 for
   "there's more device paths later", and then make it merely set *dev =
   NULL when we just don't /support/ the device path we found, rather than
   returning error.  Either case would still set *node to the (possibly
   inexistent) next node.  If we did that, we could easily rename it
   get_devices_by_path() and make get_device_by_path() a special case
   with exactly the semantics we have now.

> 
> On Mon, 17 Oct, at 12:57:23PM, Lukas Wunner wrote:
> > We're about to extended the efistub to retrieve device properties from
> > EFI on Apple Macs. The properties use EFI Device Paths to indicate the
> > device they belong to. This commit adds a parser which, given an EFI
> > Device Path, locates the corresponding struct device and returns a
> > reference to it.
> > 
> > Initially only ACPI and PCI Device Path nodes are supported, these are
> > the only types needed for Apple device properties (the corresponding
> > macOS function AppleACPIPlatformExpert::matchEFIDevicePath() does not
> > support any others). Further node types can be added with moderate
> > effort.
> > 
> > Apple device properties is currently the only use case of this parser,
> > but since this may be useful to others, make it a separate component
> > which can be selected with config option EFI_DEV_PATH_PARSER. It can
> > in principle be compiled as a module if acpi_get_first_physical_node()
> > and acpi_bus_type are exported (and get_device_by_efi_path() itself is
> > exported).
> > 
> > The dependency on CONFIG_ACPI is needed for acpi_match_device_ids().
> > It can be removed if an empty inline stub is added for that function.
> > 
> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > ---
> >  drivers/firmware/efi/Kconfig           |   5 +
> >  drivers/firmware/efi/Makefile          |   1 +
> >  drivers/firmware/efi/dev-path-parser.c | 186 +++++++++++++++++++++++++++++++++
> >  include/linux/efi.h                    |  21 ++++
> >  4 files changed, 213 insertions(+)
> >  create mode 100644 drivers/firmware/efi/dev-path-parser.c
> > 
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index c981be1..893fda4 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -133,3 +133,8 @@ endmenu
> >  
> >  config UEFI_CPER
> >  	bool
> > +
> > +config EFI_DEV_PATH_PARSER
> > +	bool
> > +	depends on ACPI
> > +	default n
> > diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> > index c8a439f..3e91ae3 100644
> > --- a/drivers/firmware/efi/Makefile
> > +++ b/drivers/firmware/efi/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_EFI_STUB)			+= libstub/
> >  obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
> >  obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
> >  obj-$(CONFIG_EFI_TEST)			+= test/
> > +obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
> >  
> >  arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
> >  obj-$(CONFIG_ARM)			+= $(arm-obj-y)
> > diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
> > new file mode 100644
> > index 0000000..549f80b
> > --- /dev/null
> > +++ b/drivers/firmware/efi/dev-path-parser.c
> > @@ -0,0 +1,186 @@
> > +/*
> > + * dev-path-parser.c - EFI Device Path parser
> > + * Copyright (C) 2016 Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License (version 2) as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/efi.h>
> > +#include <linux/pci.h>
> > +
> > +struct acpi_hid_uid {
> > +	struct acpi_device_id hid[2];
> > +	char uid[11]; /* UINT_MAX + null byte */
> > +};
> > +
> > +static int __init acpi_match(struct device *dev, void *data)
> > +{
> > +	struct acpi_hid_uid hid_uid = *(struct acpi_hid_uid *)data;
> > +	struct acpi_device *adev = to_acpi_device(dev);
> > +
> > +	if (acpi_match_device_ids(adev, hid_uid.hid))
> > +		return 0;
> > +
> > +	if (adev->pnp.unique_id)
> > +		return !strcmp(adev->pnp.unique_id, hid_uid.uid);
> > +	else
> > +		return !strcmp("0", hid_uid.uid);
> > +}
> > +
> > +static int __init get_device_by_acpi_path(struct efi_dev_path *node,
> > +					  struct device *parent,
> > +					  struct device **child)
> > +{
> > +	struct acpi_hid_uid hid_uid = {};
> > +	struct device *phys_dev;
> > +
> > +	if (node->length != 12)
> > +		return -EINVAL;
> > +
> > +	sprintf(hid_uid.hid[0].id, "%c%c%c%04X",
> > +		'A' + ((node->acpi.hid >> 10) & 0x1f) - 1,
> > +		'A' + ((node->acpi.hid >>  5) & 0x1f) - 1,
> > +		'A' + ((node->acpi.hid >>  0) & 0x1f) - 1,
> > +			node->acpi.hid >> 16);
> > +	sprintf(hid_uid.uid, "%u", node->acpi.uid);
> > +
> > +	*child = bus_find_device(&acpi_bus_type, NULL, &hid_uid, acpi_match);
> > +	if (!*child)
> > +		return -ENODEV;
> > +
> > +	phys_dev = acpi_get_first_physical_node(to_acpi_device(*child));
> > +	if (phys_dev) {
> > +		get_device(phys_dev);
> > +		put_device(*child);
> > +		*child = phys_dev;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __init pci_match(struct device *dev, void *data)
> > +{
> > +	unsigned int devfn = *(unsigned int *)data;
> > +
> > +	return dev_is_pci(dev) && to_pci_dev(dev)->devfn == devfn;
> > +}
> > +
> > +static int __init get_device_by_pci_path(struct efi_dev_path *node,
> > +					 struct device *parent,
> > +					 struct device **child)
> > +{
> > +	unsigned int devfn;
> > +
> > +	if (node->length != 6)
> > +		return -EINVAL;
> > +	if (!parent)
> > +		return -EINVAL;
> > +
> > +	devfn = PCI_DEVFN(node->pci.dev, node->pci.fn);
> > +
> > +	*child = device_find_child(parent, &devfn, pci_match);
> > +	if (!*child)
> > +		return -ENODEV;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Insert parsers for further node types here.
> > + *
> > + * Each parser takes a pointer to the @node and to the @parent (will be NULL
> > + * for the first device path node). If a device corresponding to @node was
> > + * found below @parent, its reference count should be incremented and the
> > + * device returned in @child.
> > + *
> > + * The return value should be 0 on success or a negative int on failure.
> > + * The special return value 1 signals the end of the device path, only
> > + * get_device_by_end_path() is supposed to return this.
> > + *
> > + * Be sure to validate the node length and contents before commencing the
> > + * search for a device.
> > + */
> > +
> > +static int __init get_device_by_end_path(struct efi_dev_path *node,
> > +					 struct device *parent,
> > +					 struct device **child)
> > +{
> > +	if (node->length != 4)
> > +		return -EINVAL;
> > +	if (!parent)
> > +		return -ENODEV;
> > +
> > +	*child = get_device(parent);
> > +	return 1;
> > +}
> > +
> > +/**
> > + * get_device_by_efi_path - find device by EFI Device Path
> > + * @node: EFI Device Path
> > + * @len: maximum length of EFI Device Path in bytes
> > + * @dev: device found
> > + *
> > + * Parse a series of EFI Device Path nodes at @node and find the corresponding
> > + * device. If the device was found, its reference count is incremented and a
> > + * pointer to it is returned in @dev. The caller needs to drop the reference
> > + * with put_device() after use. The @node pointer is updated to point to the
> > + * location immediately after the "End Entire Device Path" node.
> > + *
> > + * If a Device Path node is malformed or its corresponding device is not found,
> > + * @node is updated to point to this offending node and @dev will be %NULL.
> > + *
> > + * Most buses instantiate devices in "subsys" initcall level, hence the
> > + * earliest initcall level in which this function should be called is "fs".
> > + *
> > + * Returns 0 on success,
> > + *	   %-ENODEV if no device was found,
> > + *	   %-EINVAL if a node is malformed or exceeds @len,
> > + *	   %-ENOTSUPP if support for a node type is not yet implemented.
> > + */
> > +int __init get_device_by_efi_path(struct efi_dev_path **node, size_t len,
> > +				  struct device **dev)
> > +{
> > +	struct device *parent = NULL, *child;
> > +	int ret = 0;
> > +
> > +	*dev = NULL;
> > +
> > +	while (ret != 1) {
> > +		if (len < 4 || len < (*node)->length)
> > +			ret = -EINVAL;
> > +		else if ((*node)->type     == EFI_DEV_ACPI &&
> > +			 (*node)->sub_type == EFI_DEV_BASIC_ACPI)
> > +			ret = get_device_by_acpi_path(*node, parent, &child);
> > +		else if ((*node)->type     == EFI_DEV_HW &&
> > +			 (*node)->sub_type == EFI_DEV_PCI)
> > +			ret = get_device_by_pci_path(*node, parent, &child);
> > +		else if (((*node)->type    == EFI_DEV_END_PATH ||
> > +			  (*node)->type    == EFI_DEV_END_PATH2) &&
> > +			 (*node)->sub_type == EFI_DEV_END_ENTIRE)
> > +			ret = get_device_by_end_path(*node, parent, &child);
> > +		else
> > +			ret = -ENOTSUPP;
> > +
> > +		put_device(parent);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		parent = child;
> > +		*node  = (void *)*node + (*node)->length;
> > +		len   -= (*node)->length;
> > +	}
> > +
> > +	*dev = child;
> > +	return 0;
> > +}
> 
> Where in your patch series is this function called? Am I missing
> something?
> 
> Also, unless there's some existing namespace with "get_device_by_*"
> I'd prefer for this function name to have "efi_" as the prefix.
> 
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 2d08948..4faf339 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1145,6 +1145,27 @@ struct efi_generic_dev_path {
> >  	u16 length;
> >  } __attribute ((packed));
> >  
> > +struct efi_dev_path {
> > +	u8 type;	/* can be replaced with unnamed */
> > +	u8 sub_type;	/* struct efi_generic_dev_path; */
> > +	u16 length;	/* once we've moved to -std=c11 */
> > +	union {
> > +		struct {
> > +			u32 hid;
> > +			u32 uid;
> > +		} acpi;
> > +		struct {
> > +			u8 fn;
> > +			u8 dev;
> > +		} pci;
> > +	};
> > +} __attribute ((packed));
> > +
> > +#if IS_ENABLED(CONFIG_EFI_DEV_PATH_PARSER)
> > +int get_device_by_efi_path(struct efi_dev_path **node, size_t len,
> > +			   struct device **dev);
> > +#endif
> > +
> >  static inline void memrange_efi_to_native(u64 *addr, u64 *npages)
> >  {
> >  	*npages = PFN_UP(*addr + (*npages<<EFI_PAGE_SHIFT)) - PFN_DOWN(*addr);
> > -- 
> > 2.9.3
> > 

-- 
        Peter

Re: [PATCH v2 1/3] efi: Add device path parser

[Lukas Wunner]

On Fri, Oct 21, 2016 at 12:34:36PM -0400, Peter Jones wrote:
> On Wed, Oct 19, 2016 at 12:17:28PM +0100, Matt Fleming wrote:
> > (Cc'ing Peter since he's played with device path parsing before)
> 
> So the only two thoughts I have:
> 
> 1) I think we should probably be a bit more aggressive about bounds
>    checking.  Every once in a while there are implementations that build
>    device paths from templates and occasionally leave
>    efi_dev_path.length as 0 or 0xffff,

The patch already handles this:  The length of each node is fixed
for each type in the spec (e.g. 6 bytes for PCI nodes) and there's
strict checking of this in the patch.  A node with length 0 or 0xffff
would result in -EINVAL.


>    or forget to put an End Entire
>    Device Path node at the end (or put an End Device Path Instance in
>    instead.)  So checking for ->length < PAGE_SIZE

This is also already handled by the patch:  get_device_by_efi_path()
is called with a maximum length parameter.  In the case of Apple device
properties, I know the size of the payload which contains a device's
properties, so I pass that as the maximum length.  If in your use case
you don't have such an upper bound and feel that device paths should
never exceed PAGE_SIZE, then just pass that as the maximum length
argument to get_device_by_efi_path().


>    and that when a dp
>    node crosses a page boundary, the new page is actually in the same map
>    as the previous one wouldn't be a terrible idea.

In the case of Apple device properties, that isn't needed as they're
all contained in one payload that gets mapped into virtual memory.
I think checking page mapping should only be done if really needed,
i.e. if you expect to get such a bogus device path, you should probably
perform such checks in your code, but it shouldn't be done in
get_device_by_efi_path(), i.e. for *every* device path.


> 2) I think we should decide how multiple-instance device path are going
>    to work sooner rather than later, mostly because I have a use for them
>    and it seems harder to graft them in later.  My first idea here is to
>    make get_device_by_efi_path() return 0 for "stop iterating" and 1 for
>    "there's more device paths later"

I'm fine with that approach.


>    and then make it merely set *dev =
>    NULL when we just don't /support/ the device path we found, rather than
>    returning error.  Either case would still set *node to the (possibly
>    inexistent) next node.

That however I'm very sceptical about.

If the device path is malformed (-EINVAL), e.g. because it contains a
bogus length, all bets are off and I have to stop parsing.  It's just
not safe to try to find any following instances.  If on the other hand
the device path contains an unsupported node type (-ENOTSUPP), well,
we should simply add support for it.  Which node types do you expect
to parse that aren't currently supported by the patch?  (The patch
supports ACPI and PCI device paths.)

If I would pass over unsupported node types, I couldn't apply strict
length checking, so I would indeed have to apply some heuristic,
such as < PAGE_SIZE, which seems kludgy at best.

Also, it is intentional that get_device_by_efi_path() stops parsing
if it hits upon an unsupported node type because while ACPI and PCI
nodes are currently the only ones used by Apple, it's possible that
they'll add others in the future and in that case I want to get an
error back and I want to know the position of the unsupported node
type.  The caller unmarshal_devices() in patch [2/3] of this series
prints a hex dump of the device path and the properties and an error
message showing the position of the offending node, so that it's
easy to identify what went wrong and which node type needs to be
added.


>    If we did that, we could easily rename it
>    get_devices_by_path() and make get_device_by_path() a special case
>    with exactly the semantics we have now.

I don't quite follow.  With the approach to support multiple instances
that you've outlined above, you'd simply do:

	while ((ret = get_device_by_efi_path(node, len, &dev)) >= 0) {
		/* do something */
		if (ret == 0)
			break;
	}

What would you need a get_devices_by_path() function for?

Thanks,

Lukas

> > On Mon, 17 Oct, at 12:57:23PM, Lukas Wunner wrote:
> > > We're about to extended the efistub to retrieve device properties from
> > > EFI on Apple Macs. The properties use EFI Device Paths to indicate the
> > > device they belong to. This commit adds a parser which, given an EFI
> > > Device Path, locates the corresponding struct device and returns a
> > > reference to it.
> > > 
> > > Initially only ACPI and PCI Device Path nodes are supported, these are
> > > the only types needed for Apple device properties (the corresponding
> > > macOS function AppleACPIPlatformExpert::matchEFIDevicePath() does not
> > > support any others). Further node types can be added with moderate
> > > effort.
> > > 
> > > Apple device properties is currently the only use case of this parser,
> > > but since this may be useful to others, make it a separate component
> > > which can be selected with config option EFI_DEV_PATH_PARSER. It can
> > > in principle be compiled as a module if acpi_get_first_physical_node()
> > > and acpi_bus_type are exported (and get_device_by_efi_path() itself is
> > > exported).
> > > 
> > > The dependency on CONFIG_ACPI is needed for acpi_match_device_ids().
> > > It can be removed if an empty inline stub is added for that function.
> > > 
> > > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > > ---
> > >  drivers/firmware/efi/Kconfig           |   5 +
> > >  drivers/firmware/efi/Makefile          |   1 +
> > >  drivers/firmware/efi/dev-path-parser.c | 186 +++++++++++++++++++++++++++++++++
> > >  include/linux/efi.h                    |  21 ++++
> > >  4 files changed, 213 insertions(+)
> > >  create mode 100644 drivers/firmware/efi/dev-path-parser.c
> > > 
> > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > index c981be1..893fda4 100644
> > > --- a/drivers/firmware/efi/Kconfig
> > > +++ b/drivers/firmware/efi/Kconfig
> > > @@ -133,3 +133,8 @@ endmenu
> > >  
> > >  config UEFI_CPER
> > >  	bool
> > > +
> > > +config EFI_DEV_PATH_PARSER
> > > +	bool
> > > +	depends on ACPI
> > > +	default n
> > > diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> > > index c8a439f..3e91ae3 100644
> > > --- a/drivers/firmware/efi/Makefile
> > > +++ b/drivers/firmware/efi/Makefile
> > > @@ -21,6 +21,7 @@ obj-$(CONFIG_EFI_STUB)			+= libstub/
> > >  obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
> > >  obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
> > >  obj-$(CONFIG_EFI_TEST)			+= test/
> > > +obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
> > >  
> > >  arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
> > >  obj-$(CONFIG_ARM)			+= $(arm-obj-y)
> > > diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
> > > new file mode 100644
> > > index 0000000..549f80b
> > > --- /dev/null
> > > +++ b/drivers/firmware/efi/dev-path-parser.c
> > > @@ -0,0 +1,186 @@
> > > +/*
> > > + * dev-path-parser.c - EFI Device Path parser
> > > + * Copyright (C) 2016 Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License (version 2) as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/efi.h>
> > > +#include <linux/pci.h>
> > > +
> > > +struct acpi_hid_uid {
> > > +	struct acpi_device_id hid[2];
> > > +	char uid[11]; /* UINT_MAX + null byte */
> > > +};
> > > +
> > > +static int __init acpi_match(struct device *dev, void *data)
> > > +{
> > > +	struct acpi_hid_uid hid_uid = *(struct acpi_hid_uid *)data;
> > > +	struct acpi_device *adev = to_acpi_device(dev);
> > > +
> > > +	if (acpi_match_device_ids(adev, hid_uid.hid))
> > > +		return 0;
> > > +
> > > +	if (adev->pnp.unique_id)
> > > +		return !strcmp(adev->pnp.unique_id, hid_uid.uid);
> > > +	else
> > > +		return !strcmp("0", hid_uid.uid);
> > > +}
> > > +
> > > +static int __init get_device_by_acpi_path(struct efi_dev_path *node,
> > > +					  struct device *parent,
> > > +					  struct device **child)
> > > +{
> > > +	struct acpi_hid_uid hid_uid = {};
> > > +	struct device *phys_dev;
> > > +
> > > +	if (node->length != 12)
> > > +		return -EINVAL;
> > > +
> > > +	sprintf(hid_uid.hid[0].id, "%c%c%c%04X",
> > > +		'A' + ((node->acpi.hid >> 10) & 0x1f) - 1,
> > > +		'A' + ((node->acpi.hid >>  5) & 0x1f) - 1,
> > > +		'A' + ((node->acpi.hid >>  0) & 0x1f) - 1,
> > > +			node->acpi.hid >> 16);
> > > +	sprintf(hid_uid.uid, "%u", node->acpi.uid);
> > > +
> > > +	*child = bus_find_device(&acpi_bus_type, NULL, &hid_uid, acpi_match);
> > > +	if (!*child)
> > > +		return -ENODEV;
> > > +
> > > +	phys_dev = acpi_get_first_physical_node(to_acpi_device(*child));
> > > +	if (phys_dev) {
> > > +		get_device(phys_dev);
> > > +		put_device(*child);
> > > +		*child = phys_dev;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __init pci_match(struct device *dev, void *data)
> > > +{
> > > +	unsigned int devfn = *(unsigned int *)data;
> > > +
> > > +	return dev_is_pci(dev) && to_pci_dev(dev)->devfn == devfn;
> > > +}
> > > +
> > > +static int __init get_device_by_pci_path(struct efi_dev_path *node,
> > > +					 struct device *parent,
> > > +					 struct device **child)
> > > +{
> > > +	unsigned int devfn;
> > > +
> > > +	if (node->length != 6)
> > > +		return -EINVAL;
> > > +	if (!parent)
> > > +		return -EINVAL;
> > > +
> > > +	devfn = PCI_DEVFN(node->pci.dev, node->pci.fn);
> > > +
> > > +	*child = device_find_child(parent, &devfn, pci_match);
> > > +	if (!*child)
> > > +		return -ENODEV;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Insert parsers for further node types here.
> > > + *
> > > + * Each parser takes a pointer to the @node and to the @parent (will be NULL
> > > + * for the first device path node). If a device corresponding to @node was
> > > + * found below @parent, its reference count should be incremented and the
> > > + * device returned in @child.
> > > + *
> > > + * The return value should be 0 on success or a negative int on failure.
> > > + * The special return value 1 signals the end of the device path, only
> > > + * get_device_by_end_path() is supposed to return this.
> > > + *
> > > + * Be sure to validate the node length and contents before commencing the
> > > + * search for a device.
> > > + */
> > > +
> > > +static int __init get_device_by_end_path(struct efi_dev_path *node,
> > > +					 struct device *parent,
> > > +					 struct device **child)
> > > +{
> > > +	if (node->length != 4)
> > > +		return -EINVAL;
> > > +	if (!parent)
> > > +		return -ENODEV;
> > > +
> > > +	*child = get_device(parent);
> > > +	return 1;
> > > +}
> > > +
> > > +/**
> > > + * get_device_by_efi_path - find device by EFI Device Path
> > > + * @node: EFI Device Path
> > > + * @len: maximum length of EFI Device Path in bytes
> > > + * @dev: device found
> > > + *
> > > + * Parse a series of EFI Device Path nodes at @node and find the corresponding
> > > + * device. If the device was found, its reference count is incremented and a
> > > + * pointer to it is returned in @dev. The caller needs to drop the reference
> > > + * with put_device() after use. The @node pointer is updated to point to the
> > > + * location immediately after the "End Entire Device Path" node.
> > > + *
> > > + * If a Device Path node is malformed or its corresponding device is not found,
> > > + * @node is updated to point to this offending node and @dev will be %NULL.
> > > + *
> > > + * Most buses instantiate devices in "subsys" initcall level, hence the
> > > + * earliest initcall level in which this function should be called is "fs".
> > > + *
> > > + * Returns 0 on success,
> > > + *	   %-ENODEV if no device was found,
> > > + *	   %-EINVAL if a node is malformed or exceeds @len,
> > > + *	   %-ENOTSUPP if support for a node type is not yet implemented.
> > > + */
> > > +int __init get_device_by_efi_path(struct efi_dev_path **node, size_t len,
> > > +				  struct device **dev)
> > > +{
> > > +	struct device *parent = NULL, *child;
> > > +	int ret = 0;
> > > +
> > > +	*dev = NULL;
> > > +
> > > +	while (ret != 1) {
> > > +		if (len < 4 || len < (*node)->length)
> > > +			ret = -EINVAL;
> > > +		else if ((*node)->type     == EFI_DEV_ACPI &&
> > > +			 (*node)->sub_type == EFI_DEV_BASIC_ACPI)
> > > +			ret = get_device_by_acpi_path(*node, parent, &child);
> > > +		else if ((*node)->type     == EFI_DEV_HW &&
> > > +			 (*node)->sub_type == EFI_DEV_PCI)
> > > +			ret = get_device_by_pci_path(*node, parent, &child);
> > > +		else if (((*node)->type    == EFI_DEV_END_PATH ||
> > > +			  (*node)->type    == EFI_DEV_END_PATH2) &&
> > > +			 (*node)->sub_type == EFI_DEV_END_ENTIRE)
> > > +			ret = get_device_by_end_path(*node, parent, &child);
> > > +		else
> > > +			ret = -ENOTSUPP;
> > > +
> > > +		put_device(parent);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		parent = child;
> > > +		*node  = (void *)*node + (*node)->length;
> > > +		len   -= (*node)->length;
> > > +	}
> > > +
> > > +	*dev = child;
> > > +	return 0;
> > > +}
> > 
> > Where in your patch series is this function called? Am I missing
> > something?
> > 
> > Also, unless there's some existing namespace with "get_device_by_*"
> > I'd prefer for this function name to have "efi_" as the prefix.
> > 
> > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > index 2d08948..4faf339 100644
> > > --- a/include/linux/efi.h
> > > +++ b/include/linux/efi.h
> > > @@ -1145,6 +1145,27 @@ struct efi_generic_dev_path {
> > >  	u16 length;
> > >  } __attribute ((packed));
> > >  
> > > +struct efi_dev_path {
> > > +	u8 type;	/* can be replaced with unnamed */
> > > +	u8 sub_type;	/* struct efi_generic_dev_path; */
> > > +	u16 length;	/* once we've moved to -std=c11 */
> > > +	union {
> > > +		struct {
> > > +			u32 hid;
> > > +			u32 uid;
> > > +		} acpi;
> > > +		struct {
> > > +			u8 fn;
> > > +			u8 dev;
> > > +		} pci;
> > > +	};
> > > +} __attribute ((packed));
> > > +
> > > +#if IS_ENABLED(CONFIG_EFI_DEV_PATH_PARSER)
> > > +int get_device_by_efi_path(struct efi_dev_path **node, size_t len,
> > > +			   struct device **dev);
> > > +#endif
> > > +
> > >  static inline void memrange_efi_to_native(u64 *addr, u64 *npages)
> > >  {
> > >  	*npages = PFN_UP(*addr + (*npages<<EFI_PAGE_SHIFT)) - PFN_DOWN(*addr);
> > > -- 
> > > 2.9.3

Re: [PATCH v2 1/3] efi: Add device path parser

[Peter Jones]

On Sat, Oct 22, 2016 at 12:16:08PM +0200, Lukas Wunner wrote:
> On Fri, Oct 21, 2016 at 12:34:36PM -0400, Peter Jones wrote:
> > On Wed, Oct 19, 2016 at 12:17:28PM +0100, Matt Fleming wrote:
> > > (Cc'ing Peter since he's played with device path parsing before)
> > 
> > So the only two thoughts I have:
> > 
> > 1) I think we should probably be a bit more aggressive about bounds
> >    checking.  Every once in a while there are implementations that build
> >    device paths from templates and occasionally leave
> >    efi_dev_path.length as 0 or 0xffff,
> 
> The patch already handles this:  The length of each node is fixed
> for each type in the spec (e.g. 6 bytes for PCI nodes) and there's
> strict checking of this in the patch.  A node with length 0 or 0xffff
> would result in -EINVAL.

But that's only true if we only handle device types with well defined
sizes, which means no VendorMessage() or VendhorHardware() devices at
all.  Apple might not be using those for what you're using this for so
far, but lots of other vendors are, for lots of things.  Which
effectively means we can't handle multiple-instance device paths.

> >    or forget to put an End Entire
> >    Device Path node at the end (or put an End Device Path Instance in
> >    instead.)  So checking for ->length < PAGE_SIZE
> 
> This is also already handled by the patch:  get_device_by_efi_path()
> is called with a maximum length parameter.  In the case of Apple device
> properties, I know the size of the payload which contains a device's
> properties, so I pass that as the maximum length.  If in your use case
> you don't have such an upper bound and feel that device paths should
> never exceed PAGE_SIZE, then just pass that as the maximum length
> argument to get_device_by_efi_path().

Fair enough.

> >    and that when a dp
> >    node crosses a page boundary, the new page is actually in the same map
> >    as the previous one wouldn't be a terrible idea.
> 
> In the case of Apple device properties, that isn't needed as they're
> all contained in one payload that gets mapped into virtual memory.
> I think checking page mapping should only be done if really needed,
> i.e. if you expect to get such a bogus device path, you should probably
> perform such checks in your code, but it shouldn't be done in
> get_device_by_efi_path(), i.e. for *every* device path.

Yeah, okay, I can buy that.

> > 2) I think we should decide how multiple-instance device path are going
> >    to work sooner rather than later, mostly because I have a use for them
> >    and it seems harder to graft them in later.  My first idea here is to
> >    make get_device_by_efi_path() return 0 for "stop iterating" and 1 for
> >    "there's more device paths later"
> 
> I'm fine with that approach.
> 
> 
> >    and then make it merely set *dev =
> >    NULL when we just don't /support/ the device path we found, rather than
> >    returning error.  Either case would still set *node to the (possibly
> >    inexistent) next node.
> 
> That however I'm very sceptical about.
> 
> If the device path is malformed (-EINVAL), e.g. because it contains a
> bogus length, all bets are off and I have to stop parsing.  It's just
> not safe to try to find any following instances.  If on the other hand
> the device path contains an unsupported node type (-ENOTSUPP), well,
> we should simply add support for it.  Which node types do you expect
> to parse that aren't currently supported by the patch?  (The patch
> supports ACPI and PCI device paths.)

By definition we cannot add any meaningful support for any of the vendor
defined device paths.  In the spec, these are defined in sections 9.3.2.4,
9.3.5.17, 9.3.6.3.  All we can do is say they are present and skip over
them.

So FWIW where I want to use this is matching up devices with the
ConInDev/ConOutDev/ErrOutDev variables, so we can add sysfs attributes
to devices to say the hardware supports using them as console.  These
variables are multi-instance device paths, with one instance per
hardware device.  A typical value is something like:

PciRoot(0x0)/Pci(0x14,0x0)/VenMsg(...)/EndInstance/PciRoot(0x0)/Pci(0x14,0x0)/USB(1,0)/USB(0,0)/EndInstance/PciRoot(0x0)/Pci(0x2,0x0)/AcpiAdr(0x1)/EndEntire


Without parsing the arbitrarily-sized vendor paths, we never get to the
parts we can actually do something with.

> If I would pass over unsupported node types, I couldn't apply strict
> length checking, so I would indeed have to apply some heuristic,
> such as < PAGE_SIZE, which seems kludgy at best.

Right, it's kludgy, it just might be the best thing we have given the
arbitrary nature of the data.  We could apply any such heuristic to only
the vendor paths, though.

> Also, it is intentional that get_device_by_efi_path() stops parsing
> if it hits upon an unsupported node type because while ACPI and PCI
> nodes are currently the only ones used by Apple, it's possible that
> they'll add others in the future and in that case I want to get an
> error back and I want to know the position of the unsupported node
> type.  The caller unmarshal_devices() in patch [2/3] of this series
> prints a hex dump of the device path and the properties and an error
> message showing the position of the offending node, so that it's
> easy to identify what went wrong and which node type needs to be
> added.

Right - I know it's intentional, I just don't think that'll hold if we
start using this for other purposes than yours.

> >    If we did that, we could easily rename it
> >    get_devices_by_path() and make get_device_by_path() a special case
> >    with exactly the semantics we have now.
> 
> I don't quite follow.  With the approach to support multiple instances
> that you've outlined above, you'd simply do:
> 
> 	while ((ret = get_device_by_efi_path(node, len, &dev)) >= 0) {
> 		/* do something */
> 		if (ret == 0)
> 			break;
> 	}
> 
> What would you need a get_devices_by_path() function for?

I was just suggesting it as a convenience function; no reason to get
hung up on it.
-- 
  Peter

Re: [PATCH v2 1/3] efi: Add device path parser

[Lukas Wunner]

On Tue, Oct 25, 2016 at 10:18:20AM -0400, Peter Jones wrote:
> On Sat, Oct 22, 2016 at 12:16:08PM +0200, Lukas Wunner wrote:
> > On Fri, Oct 21, 2016 at 12:34:36PM -0400, Peter Jones wrote:
> > I don't quite follow.  With the approach to support multiple instances
> > that you've outlined above, you'd simply do:
> > 
> > 	while ((ret = get_device_by_efi_path(node, len, &dev)) >= 0) {
> > 		/* do something */
> > 		if (ret == 0)
> > 			break;
> > 	}

Okay I've extended the patch to support multiple instance device paths,
but I've done it differently than shown above to hopefully make it
simpler and more intuitive.

I changed the function's signature to this:

	struct device *efi_get_device_by_path(struct efi_dev_path **node,
					      size_t *len);

It no longer returns an int, but a struct device. Once the end of entire
path has been reached, it returns NULL. On error it returns an ERR_PTR.
The maximum length is passed in by reference and decremented by the number
of bytes consumed by an instance. After the last instance, it's set to 0.

This allows the following idiom to iterate over all instances in a path:

	while (!IS_ERR_OR_NULL(dev = efi_get_device_by_path(&node, &len))) {
		// do something with dev
		put_device(dev);
	}
	if (IS_ERR(dev))
		// report error

For your use case, you'll want to omit the put_device() in the while loop
and release the reference when later on removing the sysfs entry.

I took inspiration from the corresponding PCI functions to iterate over
devices of a certain class or id, e.g.:

        struct pci_dev *pdev = NULL;

	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev))) {
		// do something with dev
	}

You can browse the updated patch here:
https://github.com/l1k/linux/commit/a3a29e8a58ff

If you want to fetch the branch and hack on it:
https://github.com/l1k/linux.git apple_properties_v3

Does this approach suit your needs?


> > If I would pass over unsupported node types, I couldn't apply strict
> > length checking, so I would indeed have to apply some heuristic,
> > such as < PAGE_SIZE, which seems kludgy at best.
> 
> Right, it's kludgy, it just might be the best thing we have given the
> arbitrary nature of the data.  We could apply any such heuristic to only
> the vendor paths, though.

Yes I think that's the way to go: Add a parser for Vendor-Defined Messaging
Device Path and check that its length is >= 20 and <= MAX_NODE_LEN, then
define MAX_NODE_LEN as e.g. PAGE_SIZE.


> > >    and then make it merely set *dev =
> > >    NULL when we just don't /support/ the device path we found, rather than
> > >    returning error.  Either case would still set *node to the (possibly
> > >    inexistent) next node.
> > 
> > That however I'm very sceptical about.
> > 
> > If the device path is malformed (-EINVAL), e.g. because it contains a
> > bogus length, all bets are off and I have to stop parsing.  It's just
> > not safe to try to find any following instances.  If on the other hand
> > the device path contains an unsupported node type (-ENOTSUPP), well,
> > we should simply add support for it.  Which node types do you expect
> > to parse that aren't currently supported by the patch?  (The patch
> > supports ACPI and PCI device paths.)
> 
> By definition we cannot add any meaningful support for any of the vendor
> defined device paths.  In the spec, these are defined in sections 9.3.2.4,
> 9.3.5.17, 9.3.6.3.  All we can do is say they are present and skip over
> them.
> 
> So FWIW where I want to use this is matching up devices with the
> ConInDev/ConOutDev/ErrOutDev variables, so we can add sysfs attributes
> to devices to say the hardware supports using them as console.  These
> variables are multi-instance device paths, with one instance per
> hardware device.  A typical value is something like:
> 
> PciRoot(0x0)/Pci(0x14,0x0)/VenMsg(...)/EndInstance/PciRoot(0x0)/Pci(0x14,0x0)/USB(1,0)/USB(0,0)/EndInstance/PciRoot(0x0)/Pci(0x2,0x0)/AcpiAdr(0x1)/EndEntire
> 
> 
> Without parsing the arbitrarily-sized vendor paths, we never get to the
> parts we can actually do something with.

I'd suggest to add a parser for Vendor-Defined Messaging Device Path which
just skips over it (by setting *child = get_device(parent), like
parse_end_path() does). The spec says this encodes VT-100 parameters and
such, I don't see how we could translate this to a device.

As for ACPI _ADR, this seems to encode the connector. The DRM subsystem
represents those as devices of class drm_class below the PCI device,
but they're only present if a DRM driver is loaded. Do you need to identify
the connector device at all for your use case? If not, it might be best to
again just skip over it.

And the only other one you'll need to parse the above-quoted path are USB
devices. You can probably just derive that from the PCI and ACPI parsers.


Are you only going to invoke efi_get_device_by_path() from an initcall?
Because right now I've declared everything __init. How do you identify
devices when removing the sysfs attributes, e.g. on shutdown? I'd recommend
to store the devices in a list somewhere, instead of parsing the path
once more.

Best regards,

Lukas