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


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

my ongoing attempt...

[Dryden Personalis]

I was checking out the patch attempt I had done in May and rebasing it 
on top of current Git.


There were lines in lvm.c I had thought about changing, just some 
housekeeping so to speak.


Turns out those very same things had already been changed by Andrei back 
in May 2015:


commit 5082ea618439fe59956d071777be0c9c74fbbcf5
Author: Andrei Borzenkov 
Date:   Wed May 13 09:47:17 2015 +0300

remove extra newlines in grub_util_* strings

grub_util_{info,warn,error} already add trailing newlines, so remove
them from format strings. Also trailing full stops are already 
added.




But Ubuntu (16.04) still ships with a version older than that, 
apparently.


I would never have fathomed such a thing, my apologies.


I also hope you don't mind me using a developer nick here. My real name 
has the initials of B.S., which is no problem in Dutch, but not so nice 
in English :P.


One of many reasons I guess.

Anyway I came across two minor things that should be no issue here:

=

diff --git a/include/grub/emu/hostdisk.h b/include/grub/emu/hostdisk.h
index e006f0b..2eb0f0c 100644
--- a/include/grub/emu/hostdisk.h
+++ b/include/grub/emu/hostdisk.h
@@ -1,4 +1,4 @@
-/* biosdisk.h - emulate biosdisk */
+/* hostdisk.h - emulate biosdisk */
 /*
  *  GRUB  --  GRand Unified Bootloader
  *  Copyright (C) 2002,2007  Free Software Foundation, Inc.
diff --git a/util/setup.c b/util/setup.c
index 8aa5a39..902e4a9 100644
--- a/util/setup.c
+++ b/util/setup.c
@@ -1,4 +1,4 @@
-/* grub-setup.c - make GRUB usable */
+/* setup.c - make GRUB usable */
 /*
  *  GRUB  --  GRand Unified Bootloader
  *  Copyright (C) 
1999,2000,2001,2002,2003,2004,2005,2006,2007,2008,2009,2010,2011  Free 
Software Foundation, Inc.


=

But I don't know what "emulate biosdisk" should be ;-).

Regards.
diff --git a/include/grub/emu/hostdisk.h b/include/grub/emu/hostdisk.h
index e006f0b..2eb0f0c 100644
--- a/include/grub/emu/hostdisk.h
+++ b/include/grub/emu/hostdisk.h
@@ -1,4 +1,4 @@
-/* biosdisk.h - emulate biosdisk */
+/* hostdisk.h - emulate biosdisk */
 /*
  *  GRUB  --  GRand Unified Bootloader
  *  Copyright (C) 2002,2007  Free Software Foundation, Inc.
diff --git a/util/setup.c b/util/setup.c
index 8aa5a39..902e4a9 100644
--- a/util/setup.c
+++ b/util/setup.c
@@ -1,4 +1,4 @@
-/* grub-setup.c - make GRUB usable */
+/* setup.c - make GRUB usable */
 /*
  *  GRUB  --  GRand Unified Bootloader
  *  Copyright (C) 1999,2000,2001,2002,2003,2004,2005,2006,2007,2008,2009,2010,2011  Free Software Foundation, Inc.
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

XFSv5 with sparse inode chunks is not supported

[Marcos Mello]

Another XFSv5 feature missing from GRUB, introduced by kernel 4.2 [1] and
mkfs.xfs 4.2.0 (-i sparse=0|1) [2]. It is disabled by default in mkfs.xfs up to
current xfsprogs 4.7.0-rc and kernel code is experimental, thus not widely used.
But in kernel 4.8 will be marked as stable [3] and I guess mkfs.xfs will not
take long to enable it by default.

GRUB does not work with volumes created with "-i sparse=1":
(Xubuntu 16.04, 2.02-beta2 plus 1570140, a139188, d3ffeb9, b6e80c7, ff3c200,
049dcfa)

https://paste.fedoraproject.org/396243/

Savannah bug: http://savannah.gnu.org/bugs/?48645

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e5376fc15acdd6b03b6d5223391842717148f0d7
[2] 
http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/cmds/xfsprogs.git;a=commitdiff;h=6003fd816c2e0c4ec2b6352571e14765cca62d07
[3] http://oss.sgi.com/archives/xfs/2016-07/msg00445.html
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH 2/2] Update grub script template files

[Nick Vinson]

On 07/26/2016 10:01 PM, Andrei Borzenkov wrote:
> 20.06.2016 04:37, Nicholas Vinson пишет:
>> Update grub-mkconfig.in and 10_linux.in to support grub-probe's new
>> partuuid target.
>>
>> Signed-off-by: Nicholas Vinson 
>> ---
>>  util/grub-mkconfig.in   |  2 ++
>>  util/grub.d/10_linux.in | 11 +--
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
>> index f8496d2..fc42462 100644
>> --- a/util/grub-mkconfig.in
>> +++ b/util/grub-mkconfig.in
>> @@ -134,6 +134,7 @@ fi
>>  # Device containing our userland.  Typically used for root= parameter.
>>  GRUB_DEVICE="`${grub_probe} --target=device /`"
>>  GRUB_DEVICE_UUID="`${grub_probe} --device ${GRUB_DEVICE} --target=fs_uuid 
>> 2> /dev/null`" || true
>> +GRUB_DEVICE_PARTUUID="`${grub_probe} --device ${GRUB_DEVICE} 
>> --target=partuuid 2> /dev/null`" || true
>>  
>>  # Device containing our /boot partition.  Usually the same as GRUB_DEVICE.
>>  GRUB_DEVICE_BOOT="`${grub_probe} --target=device /boot`"
>> @@ -182,6 +183,7 @@ if [ "x${GRUB_ACTUAL_DEFAULT}" = "xsaved" ] ; then 
>> GRUB_ACTUAL_DEFAULT="`"${grub
>>  # override them.
>>  export GRUB_DEVICE \
>>GRUB_DEVICE_UUID \
>> +  GRUB_DEVICE_PARTUUID \
>>GRUB_DEVICE_BOOT \
>>GRUB_DEVICE_BOOT_UUID \
>>GRUB_FS \
>> diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
>> index de9044c..8081fdb 100644
>> --- a/util/grub.d/10_linux.in
>> +++ b/util/grub.d/10_linux.in
>> @@ -220,8 +220,15 @@ while [ "x$list" != "x" ] ; do
>>  gettext_printf "Found initrd image: %s\n" "${dirname}/${initrd}" >&2
>>elif test -z "${initramfs}" ; then
>>  # "UUID=" and "ZFS=" magic is parsed by initrd or initramfs.  Since 
>> there's
>> -# no initrd or builtin initramfs, it can't work here.
>> -linux_root_device_thisversion=${GRUB_DEVICE}
>> +# no initrd or builtin initramfs, it can't work here.  However, if
>> +# GRUB_DEVICE_PARTUUID is not empty we can use that here if
>> +# GRUD_DISABLE_LINUX_UUID is not set to true.
>> +if [ "x${GRUB_DISABLE_LINUX_UUID}" != "xtrue" ]
>> +&& [ "x${GRUB_DEVICE_PARTUUID}" != "x" ]; then
>> +linux_root_device_thisversion="PARTUUID=${GRUB_DEVICE_PARTUUID}"
> 
> Well, PARTUUID appeared first in 2.6.37 and MSDOS "UUID" in 3.10.
> Unfortunately we have no way to check for it, even as fragile as stored
> kernel config. So I am not sure we should do it by default. And if we
> add some extra knob to turn it on, as you mentioned yourself, you can
> simply add root=PARTUUID=xxx to stored kernel command line.

I've done that using /etc/default/grub and it works, but you end up with
two root=... entries in your kernel command line.  I could have gone
back and edited the grub.cfg to clean up the command-line.

I will adjust the logic so that using PARTUUID is an opt-in feature
instead of an opt-out feature.  Downstream distributions would then be
able to adjust GRUB's defaults in their own packaging and turn this
feature on by default if it made sense for them to do so.

> 
> One more consideration is that reinstalling in existing partition will
> invalidate FS UUID stored in grub.cfg which is arguably the right thing
> because you now have something different there, but PARTUUID will most
> likely remain the same.

I am sorry, but I am not sure I understand the issue you are raising here.

If I boot Linux without an initramfs, I must specify where the root
partition (or device, if the device has no partitions) is.  GRUB
currently does this by setting root to something like '/dev/sda3'.  That
said, this is a Linux limitation not GRUB's.  Linux is unable to use the
FS UUID this early in the boot process.

If I have an initramfs, I *may* be able to use the FS UUID.  In this
case, it all depends on the implementation of my initramfs.  However,
GRUB's current behavior is to assume that all initramfs implementations
understand how to handle UUIDs, that the UUID is the FS UUID, and are
able to mount rootfs properly.

I have no interest in debating if that behavior is correct or not, nor
do I have any interest in changing it.  As currently written, my patch
should favor the FS UUID when an initramfs is detected, the PARTUUID
when it is not, and the Linux device naming scheme when the UUID feature
is disabled or the PARTUUID could not be found.

The rationale for favoring PARTUUID over the linux device name is
because the PARTUUID would allow a system to boot if the device with the
root partition is ever renamed.  In other words, if the device was sda
when grub.cfg was created, but somehow became sdb (or the other way
around), the system would sill be able to boot because the kernel would
still be able to find /sbin/init.  Whereas with the linux device name,
the boot would fail because it would be looking at the wrong device for
/sbin/init (FS UUID provides the same protections, but in theory allows
use to reorder partitions on a single device and 

[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; /* 0x1 */
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: rever...@put.as
Cc: grub-devel@gnu.org
Cc: Matt Fleming 
Cc: Andreas Noever 
Tested-by: Lukas Wunner  [MacBookPro9,1]
Tested-by: Pierre Moreau  [MacBookPro11,3]
Signed-off-by: Lukas Wunner 
---
 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,
+   , NULL, );
+   if (status != EFI_SUCCESS)
+   return;
+
+   do {
+   status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+   size + sizeof(struct setup_data), );
+   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, );
+   if (status == EFI_BUFFER_TOO_SMALL)
+