| Message ID | 75C90B50-9AB9-4F0A-B2CD-43427354D15C@live.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] efi: libstub: add support for the apple_set_os protocol | expand |
On Mon, 1 Jul 2024 at 06:46, Orlando Chamberlain <orlandoch.dev@gmail.com> wrote: > > > On Mon, 1 Jul 2024 at 05:24, Aditya Garg <gargaditya08@live.com> wrote: > > > > From: Aditya Garg <gargaditya08@live.com> > > > > 0c18184de990 ("platform/x86: apple-gmux: support MMIO gmux on T2 Macs") > > brought support for T2 Macs in apple-gmux. But in order to use dual GPU, > > the integrated GPU has to be enabled. On such dual GPU EFI Macs, the EFI > > stub needs to report that it is booting macOS in order to prevent the > > firmware from disabling the iGPU. > > > > This patch is also applicable for some non T2 Intel Macs. > > > > Based on this patch for GRUB by Andreas Heider <andreas@heider.io>: > > https://lists.gnu.org/archive/html/grub-devel/2013-12/msg00442.html > > > > Credits also goto Kerem Karabay <kekrby@gmail.com> for helping porting > > the patch to the Linux kernel. > > > > Signed-off-by: Aditya Garg <gargaditya08@live.com> > I've checked that this patch works for me when applied to 6.10-rc6. > I can also use https://github.com/0xbb/gpu-switch to make the boot gpu > the iGPU now. > > Tested-By: Orlando Chamberlain <orlandoch.dev@gmail.com> (MacBookPro16,1) > Thanks. Is there any point to making this set_os() call if CONFIG_APPLE_GMUX is not enabled?
On Mon, 1 Jul 2024 at 07:35, Lukas Wunner <lukas@wunner.de> wrote: > > On Sun, Jun 30, 2024 at 07:24:54PM +0000, Aditya Garg wrote: > > @@ -335,9 +359,12 @@ static const efi_char16_t apple[] = L"Apple"; > > > > static void setup_quirks(struct boot_params *boot_params) > > { > > - if (IS_ENABLED(CONFIG_APPLE_PROPERTIES) && > > - !memcmp(efistub_fw_vendor(), apple, sizeof(apple))) > > - retrieve_apple_device_properties(boot_params); > > + if (!memcmp(efistub_fw_vendor(), apple, sizeof(apple))) { > > + if (IS_ENABLED(CONFIG_APPLE_PROPERTIES)) { > > + retrieve_apple_device_properties(boot_params); > > + } > > + apple_set_os(); > > + } > > Nit: Unnecessary curly braces around retrieve_apple_device_properties(). > > (And I'd appreciate a blank line between it and the apple_set_os() call. Indeed. I can fix that up when applying. Any thoughts on whether this should depend on CONFIG_APPLE_GMUX or not?
On Mon, 1 Jul 2024 at 07:50, Lukas Wunner <lukas@wunner.de> wrote: > > On Mon, Jul 01, 2024 at 07:38:38AM +0200, Ard Biesheuvel wrote: > > Any thoughts on whether this should depend on CONFIG_APPLE_GMUX or not? > > I tend towards *not* making it depend on CONFIG_APPLE_GMUX: > > * The gpu-switch utility Orlando linked to doesn't use the apple-gmux > driver. (It changes EFI variables that influence to which GPU the > EFI driver will switch on next reboot.) > > * apple_set_os() has side effects beyond exposing the iGPU (such as > switching the keyboard+trackpad access method to SPI instead of USB). > If there are issues, they will be harder to debug if their occurrence > depends on a Kconfig option. Understood. I agree that having fewer possible combinations is strongly preferred. However, this change affects all Intel Macs. Is the latter side effect likely to cause any regressions on Intel Mac users that don't have two GPUs to begin with?
> On 1 Jul 2024, at 11:26 AM, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 1 Jul 2024 at 07:50, Lukas Wunner <lukas@wunner.de> wrote: >> >>> On Mon, Jul 01, 2024 at 07:38:38AM +0200, Ard Biesheuvel wrote: >>> Any thoughts on whether this should depend on CONFIG_APPLE_GMUX or not? >> >> I tend towards *not* making it depend on CONFIG_APPLE_GMUX: >> >> * The gpu-switch utility Orlando linked to doesn't use the apple-gmux >> driver. (It changes EFI variables that influence to which GPU the >> EFI driver will switch on next reboot.) >> >> * apple_set_os() has side effects beyond exposing the iGPU (such as >> switching the keyboard+trackpad access method to SPI instead of USB). >> If there are issues, they will be harder to debug if their occurrence >> depends on a Kconfig option. > > Understood. I agree that having fewer possible combinations is > strongly preferred. > > However, this change affects all Intel Macs. Is the latter side effect > likely to cause any regressions on Intel Mac users that don't have two > GPUs to begin with? It doesn't affect T2 Macs without dual GPU, I'm not sure about other Macs, although I doubt it would cause a regression. Although of the reasons of adding a parameter was this concern as well.
On Mon, Jul 01, 2024 at 08:41:05AM +0200, Lukas Wunner wrote: > If you think that we absolutely need to avoid these potential regressions, > a better approach than constraining to CONFIG_APPLE_GMUX would be to > match DMI data for dual-GPU MacBook Pros. I notice that the efistub > has been amended with SMBIOS support through efi_get_smbios_record() + > efi_get_smbios_string(). Would that get us to the laptop model name? > If so that would seem to be a viable way to avoid or at least minimize > regressions. FWIW, there would be only 6 models to match if this needs to be constrained to ones with dual GPUs: MacBookPro11,3 MacBookPro11,5 MacBookPro13,3 MacBookPro14,3 MacBookPro15,1 MacBookPro16,1 And it seems to me that the product_name in efi_smbios_type1_record would contain that model name. Thanks, Lukas
On Mon, Jul 01, 2024 at 09:44:21AM +0200, Lukas Wunner wrote: > > +static bool apple_match_product_name(void) > > Maybe something like apple_has_dual_gpu() ? On second thought, maybe not. A neutral name allows us to add models like the MacBook Airs once we have support for them in the SPI keyboard driver. Thanks, Lukas
> On 1 Jul 2024, at 1:21 PM, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 1 Jul 2024 at 09:44, Lukas Wunner <lukas@wunner.de> wrote: >> >>> On Mon, Jul 01, 2024 at 09:30:34AM +0200, Ard Biesheuvel wrote: >>> Assuming that Intel Macs implement the EFI SMBIOS protocol, reusing >>> the existing pieces should be rather straight-forward. Something like >>> the below should work in that case (whitespace damage courtesy of >>> gmail) >>> >>> Note that the smbios.c libstub source file needs some changes to >>> build correctly for x86 with CONFIG_EFI_MIXED=y, but I can take care >>> of that. >> >> Orlando, Aditya, could you test Ard's patch with CONFIG_EFI_MIXED=n? >> > > Yes, please test so we can check whether Intel Macs expose this > protocol in the first place. Sure > > Note that the following hunk is needed too: > > diff --git a/drivers/firmware/efi/libstub/Makefile > b/drivers/firmware/efi/libstub/Makefile > index 06f0428a723c..1f32d6cf98d6 100644 > --- a/drivers/firmware/efi/libstub/Makefile > +++ b/drivers/firmware/efi/libstub/Makefile > @@ -77,5 +77,5 @@ > lib-$(CONFIG_ARM) += arm32-stub.o > lib-$(CONFIG_ARM64) += kaslr.o arm64.o arm64-stub.o smbios.o > -lib-$(CONFIG_X86) += x86-stub.o > +lib-$(CONFIG_X86) += x86-stub.o smbios.o > lib-$(CONFIG_X86_64) += x86-5lvl.o > lib-$(CONFIG_RISCV) += kaslr.o riscv.o riscv-stub.o
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index 27abb4ce0..4257a8b7c 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -825,6 +825,21 @@ union apple_properties_protocol { } mixed_mode; }; +typedef union apple_set_os_protocol apple_set_os_protocol_t; + +union apple_set_os_protocol { + struct { + unsigned long version; + efi_status_t (__efiapi *set_os_version) (const char *); + efi_status_t (__efiapi *set_os_vendor) (const char *); + }; + struct { + u32 version; + u32 set_os_version; + u32 set_os_vendor; + } mixed_mode; +}; + typedef u32 efi_tcg2_event_log_format; #define INITRD_EVENT_TAG_ID 0x8F3B22ECU diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index 1983fd3bf..1eea4f7ba 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -225,6 +225,30 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) } } +static void apple_set_os(void) +{ + efi_guid_t guid = APPLE_SET_OS_PROTOCOL_GUID; + apple_set_os_protocol_t *set_os; + efi_status_t status; + + status = efi_bs_call(locate_protocol, &guid, NULL, (void **)&set_os); + if (status != EFI_SUCCESS) + return; + + if (efi_table_attr(set_os, version) >= 2) { + status = efi_fn_call(set_os, set_os_vendor, "Apple Inc."); + if (status != EFI_SUCCESS) + efi_err("Failed to set OS vendor via apple_set_os\n"); + } + + /* The version being set doesn't seem to matter */ + if (efi_table_attr(set_os, version) > 0) { + status = efi_fn_call(set_os, set_os_version, "Mac OS X 10.9"); + if (status != EFI_SUCCESS) + efi_err("Failed to set OS version via apple_set_os\n"); + } +} + efi_status_t efi_adjust_memory_range_protection(unsigned long start, unsigned long size) { @@ -335,9 +359,12 @@ static const efi_char16_t apple[] = L"Apple"; static void setup_quirks(struct boot_params *boot_params) { - if (IS_ENABLED(CONFIG_APPLE_PROPERTIES) && - !memcmp(efistub_fw_vendor(), apple, sizeof(apple))) - retrieve_apple_device_properties(boot_params); + if (!memcmp(efistub_fw_vendor(), apple, sizeof(apple))) { + if (IS_ENABLED(CONFIG_APPLE_PROPERTIES)) { + retrieve_apple_device_properties(boot_params); + } + apple_set_os(); + } } /* diff --git a/include/linux/efi.h b/include/linux/efi.h index 418e55545..e28873eb1 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -385,6 +385,7 @@ void efi_native_runtime_setup(void); #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) +#define APPLE_SET_OS_PROTOCOL_GUID EFI_GUID(0xc5c5da95, 0x7d5c, 0x45e6, 0xb2, 0xf1, 0x3f, 0xd5, 0x2b, 0xb1, 0x00, 0x77) #define EFI_TCG2_PROTOCOL_GUID EFI_GUID(0x607f766c, 0x7455, 0x42be, 0x93, 0x0b, 0xe4, 0xd7, 0x6d, 0xb2, 0x72, 0x0f) #define EFI_TCG2_FINAL_EVENTS_TABLE_GUID EFI_GUID(0x1e2ed096, 0x30e2, 0x4254, 0xbd, 0x89, 0x86, 0x3b, 0xbe, 0xf8, 0x23, 0x25) #define EFI_LOAD_FILE_PROTOCOL_GUID EFI_GUID(0x56ec3091, 0x954c, 0x11d2, 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)