[PATCH 0/6] Apple device properties

[PATCH 0/6] Apple device properties

[Lukas Wunner]

Apple EFI drivers supply device properties which are needed to support
Macs optimally.

This series extends the efistub to retrieve the device properties before
ExitBootServices is called (patch [1/6]). They are assigned to devices
in an fs_initcall (patch [5/6]). As a first use case, the Thunderbolt
driver is amended to take advantage of the Device ROM supplied by EFI
(patch [6/6]).

A by-product is a parser for EFI Device Paths which finds the struct
device corresponding to a given path. This is needed to assign
properties to their devices (patch [3/6]).


I've pushed these patches to GitHub where they can be reviewed more
comfortably with green/red highlighting:
https://github.com/l1k/linux/commits/apple_properties_v1


It would be good if one of the resident EFI experts could look over
patch [1/6] to see if I've made any mistakes that might prevent this
from working on 32 bit. It was only tested on 64 bit, I don't know
anyone with an older Mac who could test this.

Specifically, is the following okay:
efi_early->call((unsigned long)sys_table->boottime->locate_protocol, ...)

It would be convenient to have LocateProtocol or LocateHandleBuffer in
struct efi_config so that they can be called with efi_call_early().
Would a patch to add those be entertained? Right now we only offer
LocateHandle and HandleProtocol, which is somewhat cumbersome and
needs more code as the setup_pci() functions show.

Thanks,

Lukas


Lukas Wunner (6):
  efi: Retrieve Apple device properties
  ACPI / bus: Make acpi_get_first_physical_node() public
  efi: Add device path parser
  driver core: Don't leak secondary fwnode on device removal
  efi: Assign Apple device properties
  thunderbolt: Use Device ROM retrieved from EFI

 Documentation/kernel-parameters.txt     |   5 +
 arch/x86/boot/compressed/eboot.c        |  55 ++++++++
 arch/x86/include/uapi/asm/bootparam.h   |   1 +
 drivers/acpi/internal.h                 |   1 -
 drivers/base/core.c                     |   1 +
 drivers/firmware/efi/Kconfig            |  16 +++
 drivers/firmware/efi/Makefile           |   2 +
 drivers/firmware/efi/apple-properties.c | 219 ++++++++++++++++++++++++++++++++
 drivers/firmware/efi/dev-path-parser.c  | 186 +++++++++++++++++++++++++++
 drivers/thunderbolt/Kconfig             |   1 +
 drivers/thunderbolt/eeprom.c            |  42 ++++++
 drivers/thunderbolt/switch.c            |   2 +-
 include/linux/acpi.h                    |   7 +
 include/linux/efi.h                     |  38 ++++++
 14 files changed, 574 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/efi/apple-properties.c
 create mode 100644 drivers/firmware/efi/dev-path-parser.c

-- 
2.8.1

[PATCH 5/6] efi: Assign Apple device properties

[Lukas Wunner]

We've just extended the efistub to retrieve device properties from EFI
on Apple Macs and stash them in a setup_data payload.

Unmarshal that payload and assign the properties to their devices.

The property names and values are copied from the setup_data payload to
swappable virtual memory. The payload is afterwards made 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 since there's no locking for the list.

Most buses instantiate devices in "subsys" initcall level. Drivers are
usually bound to these devices in "device" initcall level. 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. This grossly inflates the ACPI table and still fails if
the user daisy-chained more than three devices.

Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Andreas Noever <andreas.noever@gmail.com>
Tested-by: Lukas Wunner <lukas@wunner.de> [MacBookPro9,1]
Tested-by: Pierre Moreau <pierre.morrow@free.fr> [MacBookPro11,3]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 Documentation/kernel-parameters.txt     |   5 +
 drivers/firmware/efi/Kconfig            |  11 ++
 drivers/firmware/efi/Makefile           |   1 +
 drivers/firmware/efi/apple-properties.c | 219 ++++++++++++++++++++++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 drivers/firmware/efi/apple-properties.c

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c9..d693fc3d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -993,6 +993,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/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 6ffafd2..df53e80 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -112,6 +112,17 @@ config EFI_CAPSULE_LOADER
 
 	  Most users should 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.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 85dcbae..dfc51e6 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_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..ff7b31d
--- /dev/null
+++ b/drivers/firmware/efi/apple-properties.c
@@ -0,0 +1,219 @@
+/*
+ * apple-properties.c - EFI device properties on Macs
+ * Copyright (C) 2016 Lukas Wunner <lukas@wunner.de>
+ *
+ * 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>
+
+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 bool __initdata dump_properties = false;
+
+static int __init dump_properties_enable(char *arg)
+{
+	dump_properties = true;
+	return 0;
+}
+
+__setup("dump_apple_properties", dump_properties_enable);
+
+
+static u8 __initdata one = 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;
+		}
+
+		ptr += key_len + val_len;
+
+		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);
+		}
+	}
+
+	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 %#lx\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 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 -ENODEV;
+
+	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;
+		}
+		ret = unmarshal_devices((struct properties_header *)data->data);
+		data->len = 0;
+		iounmap(data);
+		free_bootmem_late(pa_data + sizeof(*data), data_len);
+		return ret;
+	}
+	return 0;
+}
+
+fs_initcall(map_properties);
-- 
2.8.1

Re: [PATCH 5/6] efi: Assign Apple device properties

[Matt Fleming]

On Thu, 28 Jul, at 02:25:41AM, Lukas Wunner wrote:
> We've just extended the efistub to retrieve device properties from EFI
> on Apple Macs and stash them in a setup_data payload.

I'd really like to see this patch merged with the EFI boot stub patch,
if possible because it makes for a more cohesive patch (and it'd be
easier to review).

[PATCH 2/6] ACPI / bus: Make acpi_get_first_physical_node() public

[Lukas Wunner]

Following the fwnode of a device is currently a one-way road: We provide
ACPI_COMPANION() to obtain the fwnode but there's no (public) method to
do the reverse. Granted, there may be multiple physical_nodes, but often
the first one in the list is sufficient.

A handy function to obtain it was introduced with commit 3b95bd160547
("ACPI: introduce a function to find the first physical device"), but
currently it's only available internally.

We're about to add an EFI Device Path parser which needs this function.
Consider the following device path: ACPI(PNP0A03,0)/PCI(28,2)/PCI(0,0)
The PCI root is encoded as an ACPI device in the path, so the parser
has to find the corresponding ACPI device, then find its physical node,
find the PCI bridge in slot 1c (decimal 28), function 2 below it and
finally find the PCI device in slot 0, function 0.

To this end, make acpi_get_first_physical_node() public.

Cc: Aleksey Makarov <aleksey.makarov@linaro.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/acpi/internal.h | 1 -
 include/linux/acpi.h    | 7 +++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 27cc7fe..48e718e 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -113,7 +113,6 @@ bool acpi_device_is_present(struct acpi_device *adev);
 bool acpi_device_is_battery(struct acpi_device *adev);
 bool acpi_device_is_first_physical_node(struct acpi_device *adev,
 					const struct device *dev);
-struct device *acpi_get_first_physical_node(struct acpi_device *adev);
 
 /* --------------------------------------------------------------------------
                      Device Matching and Notification
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 288fac5..8ab6a95 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -85,6 +85,8 @@ static inline const char *acpi_dev_name(struct acpi_device *adev)
 	return dev_name(&adev->dev);
 }
 
+struct device *acpi_get_first_physical_node(struct acpi_device *adev);
+
 enum acpi_irq_model_id {
 	ACPI_IRQ_MODEL_PIC = 0,
 	ACPI_IRQ_MODEL_IOAPIC,
@@ -588,6 +590,11 @@ static inline const char *acpi_dev_name(struct acpi_device *adev)
 	return NULL;
 }
 
+static inline struct device *acpi_get_first_physical_node(struct acpi_device *adev)
+{
+	return NULL;
+}
+
 static inline void early_acpi_table_init(void *data, size_t size) { }
 static inline void acpi_early_init(void) { }
 static inline void acpi_subsystem_init(void) { }
-- 
2.8.1

Re: [PATCH 2/6] ACPI / bus: Make acpi_get_first_physical_node() public

[Rafael J. Wysocki]

On Thursday, July 28, 2016 02:25:41 AM Lukas Wunner wrote:
> Following the fwnode of a device is currently a one-way road: We provide
> ACPI_COMPANION() to obtain the fwnode but there's no (public) method to
> do the reverse. Granted, there may be multiple physical_nodes, but often
> the first one in the list is sufficient.
> 
> A handy function to obtain it was introduced with commit 3b95bd160547
> ("ACPI: introduce a function to find the first physical device"), but
> currently it's only available internally.
> 
> We're about to add an EFI Device Path parser which needs this function.
> Consider the following device path: ACPI(PNP0A03,0)/PCI(28,2)/PCI(0,0)
> The PCI root is encoded as an ACPI device in the path, so the parser
> has to find the corresponding ACPI device, then find its physical node,
> find the PCI bridge in slot 1c (decimal 28), function 2 below it and
> finally find the PCI device in slot 0, function 0.
> 
> To this end, make acpi_get_first_physical_node() public.
> 
> Cc: Aleksey Makarov <aleksey.makarov@linaro.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

ACK

Thanks,
Rafael

Re: [PATCH 2/6] ACPI / bus: Make acpi_get_first_physical_node() public

[Rafael J. Wysocki]

On Wednesday, August 17, 2016 02:38:15 AM Rafael J. Wysocki wrote:
> On Thursday, July 28, 2016 02:25:41 AM Lukas Wunner wrote:
> > Following the fwnode of a device is currently a one-way road: We provide
> > ACPI_COMPANION() to obtain the fwnode but there's no (public) method to
> > do the reverse. Granted, there may be multiple physical_nodes, but often
> > the first one in the list is sufficient.
> > 
> > A handy function to obtain it was introduced with commit 3b95bd160547
> > ("ACPI: introduce a function to find the first physical device"), but
> > currently it's only available internally.
> > 
> > We're about to add an EFI Device Path parser which needs this function.
> > Consider the following device path: ACPI(PNP0A03,0)/PCI(28,2)/PCI(0,0)
> > The PCI root is encoded as an ACPI device in the path, so the parser
> > has to find the corresponding ACPI device, then find its physical node,
> > find the PCI bridge in slot 1c (decimal 28), function 2 below it and
> > finally find the PCI device in slot 0, function 0.
> > 
> > To this end, make acpi_get_first_physical_node() public.
> > 
> > Cc: Aleksey Makarov <aleksey.makarov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> 
> ACK

I've applied this one, thanks!

Rafael

[PATCH 1/6] efi: Retrieve Apple device properties

[Lukas Wunner]

Retrieve device properties from EFI using Apple's proprietary protocol
with GUID 91BD12FE-F6C3-44FB-A5B7-5122AB303AE0, which looks like this:

typedef struct {
	unsigned long signature; /* 0x10000 */
	efi_status_t (*get) (
		IN struct apple_properties_protocol *this,
		IN struct efi_generic_dev_path *device,
		IN efi_char16_t *property_name,
		OUT void *buffer,
		IN OUT u32 *buffer_size);
		/* EFI_SUCCESS, EFI_NOT_FOUND, EFI_BUFFER_TOO_SMALL */
	efi_status_t (*set) (
		IN struct apple_properties_protocol *this,
		IN struct efi_generic_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_generic_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_size);
		/* EFI_SUCCESS, EFI_BUFFER_TOO_SMALL */
} apple_properties_protocol;

Apple's EFI driver implementing this protocol, "AAPL,PathProperties",
is a per-device key/value store which is populated by other EFI drivers.
On macOS, these device properties are retrieved by the bootloader
/usr/standalone/i386/boot.efi. The extension 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.

These device properties contain vital information which cannot be
obtained any other way (e.g. Thunderbolt Device ROM). EFI drivers also
use them to communicate the current device state so that OS drivers can
pick up where EFI drivers left (e.g. GPU mode setting).

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. 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.)

Thanks to osxreverser for this blog posting 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.

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: 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>
---
 arch/x86/boot/compressed/eboot.c      | 55 +++++++++++++++++++++++++++++++++++
 arch/x86/include/uapi/asm/bootparam.h |  1 +
 include/linux/efi.h                   | 17 +++++++++++
 3 files changed, 73 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ff574da..7262ee4 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -571,6 +571,55 @@ free_handle:
 	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_early->call(
+			(unsigned long)sys_table->boottime->locate_protocol,
+			&guid, NULL, &properties);
+	if (status != EFI_SUCCESS)
+		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_early->is64 ?
+			((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);
+
+	if (!size) {
+		efi_call_early(free_pool, new);
+		return;
+	}
+
+	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)
 {
@@ -1098,6 +1147,7 @@ free_mem_map:
 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;
@@ -1128,6 +1178,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/include/linux/efi.h b/include/linux/efi.h
index 7f80a75..e53b4b2 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -432,6 +432,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 signature;
+	u32 get;
+	u32 set;
+	u32 del;
+	u32 get_all;
+} apple_properties_protocol_32;
+
+typedef struct {
+	u64 signature;
+	u64 get;
+	u64 set;
+	u64 del;
+	u64 get_all;
+} apple_properties_protocol_64;
+
 /*
  * Types and defines for EFI ResetSystem
  */
@@ -580,6 +596,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.8.1

Re: [PATCH 1/6] efi: Retrieve Apple device properties

[Andrei Borzenkov]

27.07.2016 14:20, Lukas Wunner пишет:
> Retrieve device properties from EFI using Apple's proprietary protocol
> with GUID 91BD12FE-F6C3-44FB-A5B7-5122AB303AE0, which looks like this:
> 
> typedef struct {
> 	unsigned long signature; /* 0x10000 */
> 	efi_status_t (*get) (
> 		IN struct apple_properties_protocol *this,
> 		IN struct efi_generic_dev_path *device,
> 		IN efi_char16_t *property_name,
> 		OUT void *buffer,
> 		IN OUT u32 *buffer_size);
> 		/* EFI_SUCCESS, EFI_NOT_FOUND, EFI_BUFFER_TOO_SMALL */
> 	efi_status_t (*set) (
> 		IN struct apple_properties_protocol *this,
> 		IN struct efi_generic_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_generic_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_size);
> 		/* EFI_SUCCESS, EFI_BUFFER_TOO_SMALL */
> } apple_properties_protocol;
> 
> Apple's EFI driver implementing this protocol, "AAPL,PathProperties",
> is a per-device key/value store which is populated by other EFI drivers.
> On macOS, these device properties are retrieved by the bootloader
> /usr/standalone/i386/boot.efi. The extension 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.
> 
> These device properties contain vital information which cannot be
> obtained any other way (e.g. Thunderbolt Device ROM). EFI drivers also
> use them to communicate the current device state so that OS drivers can
> pick up where EFI drivers left (e.g. GPU mode setting).
> 
> 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. 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.)
> 

Intention was to partially merge linuxefi functionality into "linux" by
autodetecting and preferring EFI stub support. Peter, any progress on this?

> Thanks to osxreverser for this blog posting 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.
> 
> Cc: reverser@put.as
> Cc: grub-devel@gnu.org
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Andreas Noever <andreas.noever@gmail.com>
> Tested-by: Lukas Wunner <lukas@wunner.de> [MacBookPro9,1]
> Tested-by: Pierre Moreau <pierre.morrow@free.fr> [MacBookPro11,3]
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  arch/x86/boot/compressed/eboot.c      | 55 +++++++++++++++++++++++++++++++++++
>  arch/x86/include/uapi/asm/bootparam.h |  1 +
>  include/linux/efi.h                   | 17 +++++++++++
>  3 files changed, 73 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index ff574da..7262ee4 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -571,6 +571,55 @@ free_handle:
>  	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_early->call(
> +			(unsigned long)sys_table->boottime->locate_protocol,
> +			&guid, NULL, &properties);
> +	if (status != EFI_SUCCESS)
> +		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_early->is64 ?
> +			((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);
> +
> +	if (!size) {
> +		efi_call_early(free_pool, new);
> +		return;
> +	}
> +
> +	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)
>  {
> @@ -1098,6 +1147,7 @@ free_mem_map:
>  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;
> @@ -1128,6 +1178,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/include/linux/efi.h b/include/linux/efi.h
> index 7f80a75..e53b4b2 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -432,6 +432,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 signature;
> +	u32 get;
> +	u32 set;
> +	u32 del;
> +	u32 get_all;
> +} apple_properties_protocol_32;
> +
> +typedef struct {
> +	u64 signature;
> +	u64 get;
> +	u64 set;
> +	u64 del;
> +	u64 get_all;
> +} apple_properties_protocol_64;
> +
>  /*
>   * Types and defines for EFI ResetSystem
>   */
> @@ -580,6 +596,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
> 


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/6] efi: Retrieve Apple device properties

[Matt Fleming]

On Thu, 28 Jul, at 02:25:41AM, Lukas Wunner wrote:
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index ff574da..7262ee4 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -571,6 +571,55 @@ free_handle:
>  	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_early->call(
> +			(unsigned long)sys_table->boottime->locate_protocol,
> +			&guid, NULL, &properties);
> +	if (status != EFI_SUCCESS)
> +		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_early->is64 ?
> +			((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);

Is this looping really required? Do we not know ahead of time what we
expect the size to be? Writing this as a potentially infinite loop (if
broken firmware always returns EFI_BUFFER_TOO_SMALL) is a bad idea.

Re: [PATCH 1/6] efi: Retrieve Apple device properties

[Lukas Wunner]

On Thu, Aug 04, 2016 at 04:13:45PM +0100, Matt Fleming wrote:
> On Thu, 28 Jul, at 02:25:41AM, Lukas Wunner wrote:
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index ff574da..7262ee4 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -571,6 +571,55 @@ free_handle:
> >  	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_early->call(
> > +			(unsigned long)sys_table->boottime->locate_protocol,
> > +			&guid, NULL, &properties);
> > +	if (status != EFI_SUCCESS)
> > +		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_early->is64 ?
> > +			((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);
> 
> Is this looping really required? Do we not know ahead of time what we
> expect the size to be? Writing this as a potentially infinite loop (if
> broken firmware always returns EFI_BUFFER_TOO_SMALL) is a bad idea.

macOS' bootloader does exactly the same. So if the firmware was broken
in this way, macOS wouldn't boot and it's unlikely that Apple would
ship it. The code is not executed on non-Macs (due to the memcmp for
sys_table->fw_vendor[] == u"Apple" in efi_main()).

Looks like this in /usr/standalone/i386/boot.efi:
58b9         mov        rbx, 0x8000000000000005		; EFI_BUFFER_TOO_SMALL
...
58e6         mov        rcx, qword [ss:rbp+var_38]	; properties protocol
58ea         mov        rdx, rdi			; properties buffer
58ed         mov        r8, rsi				; buffer len
58f0         call       qword [ds:rcx+0x20]		; properties->get_all
58f3         cmp        rax, rbx
58f6         je         0x58c5				; infinite loop

And the code in the corresponding ->get_all function in the EFI driver
is such that it only returns either EFI_SUCCESS or EFI_BUFFER_TOO_SMALL.

So I could cap the number of loop iterations but it would be pointless.

I also checked the bootloader they shipped with OS X 10.6 (2009), they
used Universal EFI binaries back then (x86_64 + i386) in order to support
the very first Intel Macs of 2006. Found the same infinite loop there.

The reason for the loop is that the number of device properties is
dynamic. E.g. each attached Thunderbolt device is assigned 3 properties.
If a Thunderbolt device is plugged in between a first loop iteration
(to obtain the size) and a second loop iteration (to fill the buffer),
EFI_BUFFER_TOO_SMALL is returned and a third loop iteration is needed.

Thanks,

Lukas

Re: [PATCH 1/6] efi: Retrieve Apple device properties

[Matt Fleming]

On Fri, 05 Aug, at 01:42:19PM, Lukas Wunner wrote:
> 
> So I could cap the number of loop iterations but it would be pointless.

OK, thanks for the explanation.

[PATCH 4/6] driver core: Don't leak secondary fwnode on device removal

[Lukas Wunner]

If device_add_property_set() is called for a device, a secondary fwnode
is allocated and assigned to the device but currently not freed once the
device is removed.

This can be triggered on Apple Macs if a Thunderbolt device is plugged
in on boot since Apple's NHI EFI driver sets a number of properties for
that device which are leaked on unplug.

Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Andreas Noever <andreas.noever-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/base/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0a8bdad..70c5be5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1266,6 +1266,7 @@ void device_del(struct device *dev)
 	bus_remove_device(dev);
 	device_pm_remove(dev);
 	driver_deferred_probe_del(dev);
+	device_remove_properties(dev);
 
 	/* Notify the platform of the removal, in case they
 	 * need to do anything...
-- 
2.8.1

Re: [PATCH 4/6] driver core: Don't leak secondary fwnode on device removal

[Rafael J. Wysocki]

On Thursday, July 28, 2016 02:25:41 AM Lukas Wunner wrote:
> If device_add_property_set() is called for a device, a secondary fwnode
> is allocated and assigned to the device but currently not freed once the
> device is removed.
> 
> This can be triggered on Apple Macs if a Thunderbolt device is plugged
> in on boot since Apple's NHI EFI driver sets a number of properties for
> that device which are leaked on unplug.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Andreas Noever <andreas.noever@gmail.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

ACK, or I can apply it directly if you want me to do that.

Thanks,
Rafael

Re: [PATCH 4/6] driver core: Don't leak secondary fwnode on device removal

[Lukas Wunner]

On Wed, Aug 17, 2016 at 02:38:50AM +0200, Rafael J. Wysocki wrote:
> On Thursday, July 28, 2016 02:25:41 AM Lukas Wunner wrote:
> > If device_add_property_set() is called for a device, a secondary fwnode
> > is allocated and assigned to the device but currently not freed once the
> > device is removed.
> > 
> > This can be triggered on Apple Macs if a Thunderbolt device is plugged
> > in on boot since Apple's NHI EFI driver sets a number of properties for
> > that device which are leaked on unplug.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Andreas Noever <andreas.noever@gmail.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> ACK, or I can apply it directly if you want me to do that.

Yes, if you could apply it directly I'd be grateful.

Thanks!

Lukas

Re: [PATCH 4/6] driver core: Don't leak secondary fwnode on device removal

[Rafael J. Wysocki]

On Tuesday, August 30, 2016 11:03:25 AM Lukas Wunner wrote:
> On Wed, Aug 17, 2016 at 02:38:50AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, July 28, 2016 02:25:41 AM Lukas Wunner wrote:
> > > If device_add_property_set() is called for a device, a secondary fwnode
> > > is allocated and assigned to the device but currently not freed once the
> > > device is removed.
> > > 
> > > This can be triggered on Apple Macs if a Thunderbolt device is plugged
> > > in on boot since Apple's NHI EFI driver sets a number of properties for
> > > that device which are leaked on unplug.
> > > 
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: Andreas Noever <andreas.noever@gmail.com>
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > 
> > ACK, or I can apply it directly if you want me to do that.
> 
> Yes, if you could apply it directly I'd be grateful.

Applied.

Thanks,
Rafael

Re: [PATCH 0/6] Apple device properties

[Rafael J. Wysocki]

On Wednesday, July 27, 2016 01:20:41 PM Lukas Wunner wrote:
> Apple EFI drivers supply device properties which are needed to support
> Macs optimally.
> 
> This series extends the efistub to retrieve the device properties before
> ExitBootServices is called (patch [1/6]). They are assigned to devices
> in an fs_initcall (patch [5/6]). As a first use case, the Thunderbolt
> driver is amended to take advantage of the Device ROM supplied by EFI
> (patch [6/6]).

Can you please CC the whole series to linux-acpi for easier/better review?

Thanks,
Rafael

[PATCH 6/6] 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.

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.

Background information: The EFI firmware volume contains ROM files for
the NHI, GMUX and several other chips as well as key material. Drivers
do not access those files directly but rather through a file server via
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.

Cc: reverser@put.as
Cc: Andreas Noever <andreas.noever@gmail.com>
Tested-by: Lukas Wunner <lukas@wunner.de> [MacBookPro9,1]
Tested-by: Pierre Moreau <pierre.morrow@free.fr> [MacBookPro11,3]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 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 1e116f5..91804b8 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -458,7 +458,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) != 0 && sw->uid != uid) {
 		tb_sw_info(sw,
 			"changed while suspended (uid %#llx -> %#llx)\n",
 			sw->uid, uid);
-- 
2.8.1

[PATCH 3/6] efi: Add device path parser

[Lukas Wunner]

We've just 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@codeblueprint.co.uk>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 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 6394152..6ffafd2 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -116,3 +116,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 a219640..85dcbae 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 obj-$(CONFIG_EFI_STUB)			+= libstub/
 obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
 obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
+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..555e74c
--- /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@wunner.de>
+ *
+ * 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 increased 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,
+ *	   %-EPROTONOSUPPORT 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 = -EPROTONOSUPPORT;
+
+		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 e53b4b2..973ff44 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1122,6 +1122,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.8.1

Re: [PATCH 0/6] Apple device properties

[Matt Fleming]

On Thu, 28 Jul, at 02:25:41AM, Lukas Wunner wrote:
> Apple EFI drivers supply device properties which are needed to support
> Macs optimally.
> 
> This series extends the efistub to retrieve the device properties before
> ExitBootServices is called (patch [1/6]). They are assigned to devices
> in an fs_initcall (patch [5/6]). As a first use case, the Thunderbolt
> driver is amended to take advantage of the Device ROM supplied by EFI
> (patch [6/6]).
> 
> A by-product is a parser for EFI Device Paths which finds the struct
> device corresponding to a given path. This is needed to assign
> properties to their devices (patch [3/6]).
> 
> 
> I've pushed these patches to GitHub where they can be reviewed more
> comfortably with green/red highlighting:
> https://github.com/l1k/linux/commits/apple_properties_v1
> 
> 
> It would be good if one of the resident EFI experts could look over
> patch [1/6] to see if I've made any mistakes that might prevent this
> from working on 32 bit. It was only tested on 64 bit, I don't know
> anyone with an older Mac who could test this.
> 
> Specifically, is the following okay:
> efi_early->call((unsigned long)sys_table->boottime->locate_protocol, ...)

This probably isn't going to work with EFI mixed mode because you
can't jump through pointers at runtime - that's the whole point of the
setup_boot_services*bits() code.

> It would be convenient to have LocateProtocol or LocateHandleBuffer in
> struct efi_config so that they can be called with efi_call_early().
> Would a patch to add those be entertained? Right now we only offer
> LocateHandle and HandleProtocol, which is somewhat cumbersome and
> needs more code as the setup_pci() functions show.

Yes, go for it. Doing this would make it work with EFI mixed mode too.

Re: [PATCH 0/6] Apple device properties

[Lukas Wunner]

On Thu, Aug 04, 2016 at 03:57:10PM +0100, Matt Fleming wrote:
> On Thu, 28 Jul, at 02:25:41AM, Lukas Wunner wrote:
> > Specifically, is the following okay:
> > efi_early->call((unsigned long)sys_table->boottime->locate_protocol, ...)
> 
> This probably isn't going to work with EFI mixed mode because you
> can't jump through pointers at runtime - that's the whole point of the
> setup_boot_services*bits() code.
> 
> > It would be convenient to have LocateProtocol or LocateHandleBuffer in
> > struct efi_config so that they can be called with efi_call_early().
> > Would a patch to add those be entertained? Right now we only offer
> > LocateHandle and HandleProtocol, which is somewhat cumbersome and
> > needs more code as the setup_pci() functions show.
> 
> Yes, go for it. Doing this would make it work with EFI mixed mode too.

As an alternative to just adding LocateProtocol and LocateHandleBuffer
to struct efi_config, I've reworked the efi_call_early() macro to allow
invocation of arbitrary boot services by making the distinction between
64 bit and 32 bit in the macro itself using a ternary operator (see
patch below). Works fine on my 64 bit machine and looking at the
disassembled eboot.o I'm under the impression that it should work on
32 bit and mixed mode platforms as well.

Separate efi_call_early() macros could be defined for the CONFIG_X86_32
and for the (CONFIG_X86_64 && !CONFIG_EFI_MIXED) cases which omit the
ternary operator to speed things up.

And a similar macro could be defined for invocation of protocol
functions, e.g. efi_call_proto(efi_pci_io_protocol, get_location,
pci, ...), and that macro would resolve this to the get_location
element of "pci" cast to the appropriate efi_pci_io_protocol_32
or _64 struct.

You've sort of gone in the other direction with commit c116e8d60ada
("x86/efi: Split the boot stub into 32/64 code paths") by duplicating
functions, so I'm not sure if you welcome the below patch's approach.
If you'd prefer me to simply add LocateProtocol and LocateHandleBuffer
to struct efi_config just shout. :-)

Thanks,

Lukas

-- >8 --
Subject: [PATCH] x86/efi: Allow arbitrary boot services for efi_call_early()

We currently allow invocation of 8 boot services with efi_call_early().
In particular, LocateHandleBuffer and LocateProtocol are not among them.
To retrieve PCI ROMs and Apple device properties (upcoming) we're thus
forced to use the LocateHandle + HandleProtocol combo, which is
cumbersome and needs more code.

However rather than adding just LocateHandleBuffer and LocateProtocol to
struct efi_config, let's rework efi_call_early() to allow invocation of
arbitrary boot services by selecting the 64 bit vs 32 bit code path in
the macro itself. This adds one compare operation to each invocation of
a boot service and, on 32 bit platforms, two jump operations. Since this
is not a hot path, the minuscule performance penalty is arguably
outweighed by the attainable simplification of the C code.

The 8 boot services can thus be removed from struct efi_config.

No functional change intended (for now).

Cc: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 arch/x86/boot/compressed/eboot.c   | 13 +------------
 arch/x86/boot/compressed/head_32.S |  6 +++---
 arch/x86/boot/compressed/head_64.S |  8 ++++----
 arch/x86/include/asm/efi.h         | 14 +++++---------
 4 files changed, 13 insertions(+), 28 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ec6d2ef..2e0189c 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -29,22 +29,11 @@ __pure const struct efi_config *__efi_early(void)
 static void setup_boot_services##bits(struct efi_config *c)		\
 {									\
 	efi_system_table_##bits##_t *table;				\
-	efi_boot_services_##bits##_t *bt;				\
 									\
 	table = (typeof(table))sys_table;				\
 									\
+	c->boot_services = table->boottime;				\
 	c->text_output = table->con_out;				\
-									\
-	bt = (typeof(bt))(unsigned long)(table->boottime);		\
-									\
-	c->allocate_pool = bt->allocate_pool;				\
-	c->allocate_pages = bt->allocate_pages;				\
-	c->get_memory_map = bt->get_memory_map;				\
-	c->free_pool = bt->free_pool;					\
-	c->free_pages = bt->free_pages;					\
-	c->locate_handle = bt->locate_handle;				\
-	c->handle_protocol = bt->handle_protocol;			\
-	c->exit_boot_services = bt->exit_boot_services;			\
 }
 BOOT_SERVICES(32);
 BOOT_SERVICES(64);
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 1038524..fd0b6a2 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -82,7 +82,7 @@ ENTRY(efi_pe_entry)
 
 	/* Relocate efi_config->call() */
 	leal	efi32_config(%esi), %eax
-	add	%esi, 88(%eax)
+	add	%esi, 32(%eax)
 	pushl	%eax
 
 	call	make_boot_params
@@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry)
 
 	/* Relocate efi_config->call() */
 	leal	efi32_config(%esi), %eax
-	add	%esi, 88(%eax)
+	add	%esi, 32(%eax)
 	pushl	%eax
 2:
 	call	efi_main
@@ -264,7 +264,7 @@ relocated:
 #ifdef CONFIG_EFI_STUB
 	.data
 efi32_config:
-	.fill 11,8,0
+	.fill 4,8,0
 	.long efi_call_phys
 	.long 0
 	.byte 0
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 0d80a7a..efdfba2 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -265,7 +265,7 @@ ENTRY(efi_pe_entry)
 	/*
 	 * Relocate efi_config->call().
 	 */
-	addq	%rbp, efi64_config+88(%rip)
+	addq	%rbp, efi64_config+32(%rip)
 
 	movq	%rax, %rdi
 	call	make_boot_params
@@ -285,7 +285,7 @@ handover_entry:
 	 * Relocate efi_config->call().
 	 */
 	movq	efi_config(%rip), %rax
-	addq	%rbp, 88(%rax)
+	addq	%rbp, 32(%rax)
 2:
 	movq	efi_config(%rip), %rdi
 	call	efi_main
@@ -457,14 +457,14 @@ efi_config:
 #ifdef CONFIG_EFI_MIXED
 	.global efi32_config
 efi32_config:
-	.fill	11,8,0
+	.fill	4,8,0
 	.quad	efi64_thunk
 	.byte	0
 #endif
 
 	.global efi64_config
 efi64_config:
-	.fill	11,8,0
+	.fill	4,8,0
 	.quad	efi_call
 	.byte	1
 #endif /* CONFIG_EFI_STUB */
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 6b06939..306203c 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -192,14 +192,7 @@ static inline efi_status_t efi_thunk_set_virtual_address_map(
 struct efi_config {
 	u64 image_handle;
 	u64 table;
-	u64 allocate_pool;
-	u64 allocate_pages;
-	u64 get_memory_map;
-	u64 free_pool;
-	u64 free_pages;
-	u64 locate_handle;
-	u64 handle_protocol;
-	u64 exit_boot_services;
+	u64 boot_services;
 	u64 text_output;
 	efi_status_t (*call)(unsigned long, ...);
 	bool is64;
@@ -208,7 +201,10 @@ struct efi_config {
 __pure const struct efi_config *__efi_early(void);
 
 #define efi_call_early(f, ...)						\
-	__efi_early()->call(__efi_early()->f, __VA_ARGS__);
+	__efi_early()->call(__efi_early()->is64 ?			\
+	((efi_boot_services_64_t *)__efi_early()->boot_services)->f :	\
+	((efi_boot_services_32_t *)__efi_early()->boot_services)->f,	\
+						       __VA_ARGS__);
 
 #define __efi_call_early(f, ...)					\
 	__efi_early()->call((unsigned long)f, __VA_ARGS__);
-- 
2.8.1

Re: [PATCH 0/6] Apple device properties

[Matt Fleming]

On Tue, 09 Aug, at 03:38:16PM, Lukas Wunner wrote:
> @@ -208,7 +201,10 @@ struct efi_config {
>  __pure const struct efi_config *__efi_early(void);
>  
>  #define efi_call_early(f, ...)						\
> -	__efi_early()->call(__efi_early()->f, __VA_ARGS__);
> +	__efi_early()->call(__efi_early()->is64 ?			\
> +	((efi_boot_services_64_t *)__efi_early()->boot_services)->f :	\
> +	((efi_boot_services_32_t *)__efi_early()->boot_services)->f,	\
> +						       __VA_ARGS__);
>  

You cannot use pointers from the firmware directly in mixed mode
because the kernel is compiled for 64-bits but the firmware is using
32-bit addresses, so dereferencing a pointer causes a 64-bit load.

That's the reason we deconstruct the tables and copy the addresses
from the last level - so we don't have to jump through multiple
pointers.

Re: [PATCH 0/6] Apple device properties

[Lukas Wunner]

On Mon, Aug 15, 2016 at 12:54:14PM +0100, Matt Fleming wrote:
> On Tue, 09 Aug, at 03:38:16PM, Lukas Wunner wrote:
> > @@ -208,7 +201,10 @@ struct efi_config {
> >  __pure const struct efi_config *__efi_early(void);
> >  
> >  #define efi_call_early(f, ...)					\
> > -	__efi_early()->call(__efi_early()->f, __VA_ARGS__);
> > +	__efi_early()->call(__efi_early()->is64 ?			\
> > +	((efi_boot_services_64_t *)__efi_early()->boot_services)->f :	\
> > +	((efi_boot_services_32_t *)__efi_early()->boot_services)->f,	\
> > +						       __VA_ARGS__);
> >  
> 
> You cannot use pointers from the firmware directly in mixed mode
> because the kernel is compiled for 64-bits but the firmware is using
> 32-bit addresses, so dereferencing a pointer causes a 64-bit load.

Please behold the resulting binary code, which uses a 32-bit load,
not a 64-bit load (note the "mov edi, dword [ds:rax+0x2c]").

This is a call to AllocatePool *with* my patch:

0x22c1         mov        rax, qword [ds:efi_early]
0x22c8         add        rdx, 0x10                  ; buffer size argument
0x22cc         cmp        byte [ds:rax+0x28], 0x0    ; !efi_early->is64 ?
0x22d0         mov        r8, qword [ds:rax+0x20]    ; efi_early->call()
0x22d4         mov        rax, qword [ds:rax+0x10]   ; efi_early->boot_services
0x22d8         je         0x2410
0x22de         mov        rdi, qword [ds:rax+0x40]   ; allocate_pool (64 bit)
0x22e2         xor        eax, eax
0x22e4         mov        rcx, r13                   ; buffer argument
0x22e7         mov        esi, 0x2                   ; EfiLoaderData argument
0x22ec         call       r8
...
0x2410         mov        edi, dword [ds:rax+0x2c]   ; allocate_pool (32 bit)
0x2413         jmp        0x22e2

The same *without* my patch:

0x1d41         mov        r8, qword [ds:efi_early]
0x1d48         add        r15, 0x40
0x1d4c         mov        rcx, qword [ss:rsp-0x10+arg_20] ; buffer argument
0x1d51         mov        rdx, r15                   ; buffer size argument
0x1d54         mov        esi, 0x2                   ; EfiLoaderData argument
0x1d59         mov        rdi, qword [ds:r8+0x10]    ; allocate_pool
0x1d5d         call       qword [ds:r8+0x58]         ; efi_early->call

So it looks to me like my patch should work just fine on 32-bit,
even though I cannot verify it through testing.

The ARM folks afford invocation of arbitrary boot services, it just
seemed natural to me to allow the same for x86. The portion of the
stub code which is shared between arches cannot use more than the
8 boot services supported by x86 even though ARM would be capable
of using all of them.

Of course the binary code with my patch is longer, less readable,
and needs to follow multiple indirections and I can understand if
you would rather stay with the current approach for these reasons.

But I would like to understand the "cannot jump through pointers at
runtime" argument because the binary code looks to me like it should
work on 32 bit. I guess I must be missing something obvious?

Thanks,

Lukas

Re: [PATCH 0/6] Apple device properties

[Matt Fleming]

On Mon, 15 Aug, at 06:13:58PM, Lukas Wunner wrote:
> 
> But I would like to understand the "cannot jump through pointers at
> runtime" argument because the binary code looks to me like it should
> work on 32 bit. I guess I must be missing something obvious?

Ah no, I forgot that efi_boot_services_{32,64}_t doesn't contain
pointers - it contains u32/u64 objects. So yeah, your patch looks
fine.

It does trigger the following warnings when building for i386 though,

In file included from /dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:14:0:
/dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c: In function ‘efi_get_memory_map’:
/dev/shm/mfleming/git/efi/arch/x86/include/asm/efi.h:205:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  ((efi_boot_services_64_t *)__efi_early()->boot_services)->f : \
   ^
/dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:85:11: note: in expansion of macro ‘efi_call_early’
  status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
           ^
/dev/shm/mfleming/git/efi/arch/x86/include/asm/efi.h:206:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  ((efi_boot_services_32_t *)__efi_early()->boot_services)->f, \
   ^
/dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:85:11: note: in expansion of macro ‘efi_call_early’
  status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
           ^
/dev/shm/mfleming/git/efi/arch/x86/include/asm/efi.h:205:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  ((efi_boot_services_64_t *)__efi_early()->boot_services)->f : \
   ^
/dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:92:11: note: in expansion of macro ‘efi_call_early’
  status = efi_call_early(get_memory_map, map_size, m,
           ^
etc.

Re: [PATCH 0/6] Apple device properties

[Lukas Wunner]

On Thu, Aug 18, 2016 at 09:34:33PM +0100, Matt Fleming wrote:
> On Mon, 15 Aug, at 06:13:58PM, Lukas Wunner wrote:
> > But I would like to understand the "cannot jump through pointers at
> > runtime" argument because the binary code looks to me like it should
> > work on 32 bit. I guess I must be missing something obvious?
> 
> Ah no, I forgot that efi_boot_services_{32,64}_t doesn't contain
> pointers - it contains u32/u64 objects. So yeah, your patch looks
> fine.
> 
> It does trigger the following warnings when building for i386 though,
> 
> In file included from /dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:14:0:
> /dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c: In function ???efi_get_memory_map???:
> /dev/shm/mfleming/git/efi/arch/x86/include/asm/efi.h:205:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>   ((efi_boot_services_64_t *)__efi_early()->boot_services)->f : \
>    ^

Right, sorry, I didn't compile-test that version on x86_32.

I'm sending out a new version now which compiles cleanly in all three
cases (x86_32, x86_64 with and without mixed-mode), works fine on my
64-bit EFI and the 32-bit code at least *looks* okay when disassembled.

By the way, arch/x86/Kconfig says that "it is not possible to boot a
mixed-mode enabled kernel via the EFI boot stub - a bootloader that
supports the EFI handover protocol must be used".

Is this still correct? With all the mixed-mode support in head_64.S
and eboot.c, I'm wondering what's missing?

Thanks,

Lukas

Re: [PATCH 0/6] Apple device properties

[Matt Fleming]

On Mon, 22 Aug, at 11:58:50AM, Lukas Wunner wrote:
> By the way, arch/x86/Kconfig says that "it is not possible to boot a
> mixed-mode enabled kernel via the EFI boot stub - a bootloader that
> supports the EFI handover protocol must be used".
> 
> Is this still correct? With all the mixed-mode support in head_64.S
> and eboot.c, I'm wondering what's missing?

Yes, that's still correct.

The EFI boot stub technically refers to the feature of having the
firmware load the kernel directly, without the use of a boot loader.

In that scenario you need the kernel and firmware to agree on a CPU
mode since the kernel has no way to figure out if it needs to switch
or not. Nor does it have a direct way to figure out what bitness the
firmware is.

[ Yes, technically we could add code to the EFI stub to detect the
  current mode and perform the switch, we just don't have anything
  like that right now ]