[PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Lleyton Gray]

commit 71e49eccdca6 ("x86/efistub: Call Apple set_os protocol on dual GPU
Intel Macs") calls the Apple set_os protocol, but only on T2 Macbook Pro
models. This causes issues on other T2 models when an egpu is used.
Add two options which allow force enabling or disabling usage of the
protocol to fix.

Signed-off-by: Lleyton Gray <lleyton@fyralabs.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 7 ++++++-
 drivers/firmware/efi/libstub/efi-stub-helper.c  | 6 ++++++
 drivers/firmware/efi/libstub/efistub.h          | 2 ++
 drivers/firmware/efi/libstub/x86-stub.c         | 3 ++-
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe22..1d1b88c57c44 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1447,7 +1447,8 @@
 	efi=		[EFI,EARLY]
 			Format: { "debug", "disable_early_pci_dma",
 				  "nochunk", "noruntime", "nosoftreserve",
-				  "novamap", "no_disable_early_pci_dma" }
+				  "novamap", "no_disable_early_pci_dma",
+				  "enable_apple_set_os", "disable_apple_set_os" }
 			debug: enable misc debug output.
 			disable_early_pci_dma: disable the busmaster bit on all
 			PCI bridges while in the EFI boot stub.
@@ -1464,6 +1465,10 @@
 			novamap: do not call SetVirtualAddressMap().
 			no_disable_early_pci_dma: Leave the busmaster bit set
 			on all PCI bridges while in the EFI boot stub
+			enable_apple_set_os: Report that macOS is being booted
+			to the firmware, even if the device is not a dual GPU Mac.
+			disable_apple_set_os: Disable reporting that macOS is being booted
+			to the firmware, even if the device is a dual GPU Mac.
 
 	efi_no_storage_paranoia [EFI,X86,EARLY]
 			Using this parameter you can use more than 50% of
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index de659f6a815f..c33bb98bf79d 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -20,6 +20,8 @@
 bool efi_nochunk;
 bool efi_nokaslr = !IS_ENABLED(CONFIG_RANDOMIZE_BASE);
 bool efi_novamap;
+bool efi_enable_apple_set_os;
+bool efi_disable_apple_set_os;
 
 static bool efi_noinitrd;
 static bool efi_nosoftreserve;
@@ -95,6 +97,10 @@ efi_status_t efi_parse_options(char const *cmdline)
 				efi_disable_pci_dma = true;
 			if (parse_option_str(val, "no_disable_early_pci_dma"))
 				efi_disable_pci_dma = false;
+			if (parse_option_str(val, "enable_apple_set_os"))
+				efi_enable_apple_set_os = true;
+			if (parse_option_str(val, "disable_apple_set_os"))
+				efi_disable_apple_set_os = true;
 			if (parse_option_str(val, "debug"))
 				efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
 		} else if (!strcmp(param, "video") &&
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 685098f9626f..3be4b5393812 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -39,6 +39,8 @@ extern bool efi_nokaslr;
 extern int efi_loglevel;
 extern int efi_mem_encrypt;
 extern bool efi_novamap;
+extern bool efi_enable_apple_set_os;
+extern bool efi_disable_apple_set_os;
 extern const efi_system_table_t *efi_system_table;
 
 typedef union efi_dxe_services_table efi_dxe_services_table_t;
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index f8e465da344d..566118195f92 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -265,7 +265,8 @@ static void apple_set_os(void)
 	} *set_os;
 	efi_status_t status;
 
-	if (!efi_is_64bit() || !apple_match_product_name())
+	if (efi_disable_apple_set_os || !efi_is_64bit() ||
+		!efi_enable_apple_set_os && !apple_match_product_name())
 		return;
 
 	status = efi_bs_call(locate_protocol, &APPLE_SET_OS_PROTOCOL_GUID, NULL,
-- 
2.47.0

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Lukas Wunner]

On Sat, Dec 28, 2024 at 12:21:58PM -0800, Lleyton Gray wrote:
> commit 71e49eccdca6 ("x86/efistub: Call Apple set_os protocol on dual GPU
> Intel Macs") calls the Apple set_os protocol, but only on T2 Macbook Pro
> models. This causes issues on other T2 models when an egpu is used.

How so?  The commit specifically constrains the quirk only to
affected dual GPU MacBook Pros.  So I don't quite see how this
would affect other T2 models?

And what kind of issues are you seeing?  Could you provide
specifics please?

> Add two options which allow force enabling or disabling usage of the
> protocol to fix.

We generally try to avoid command line options.  The kernel
should automatically determine whether to apply the quirk
or not.  So I think the proper solution is to amend the
quirk such that it's applied to additional models or not
applied under certain circumstances.

Thanks,

Lukas

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Ard Biesheuvel]

On Sun, 29 Dec 2024 at 11:00, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Sat, Dec 28, 2024 at 12:21:58PM -0800, Lleyton Gray wrote:
> > commit 71e49eccdca6 ("x86/efistub: Call Apple set_os protocol on dual GPU
> > Intel Macs") calls the Apple set_os protocol, but only on T2 Macbook Pro
> > models. This causes issues on other T2 models when an egpu is used.
>
> How so?  The commit specifically constrains the quirk only to
> affected dual GPU MacBook Pros.  So I don't quite see how this
> would affect other T2 models?
>
> And what kind of issues are you seeing?  Could you provide
> specifics please?
>
> > Add two options which allow force enabling or disabling usage of the
> > protocol to fix.
>
> We generally try to avoid command line options.  The kernel
> should automatically determine whether to apply the quirk
> or not.  So I think the proper solution is to amend the
> quirk such that it's applied to additional models or not
> applied under certain circumstances.
>

Indeed.

But first I'd like to understand whether this is really the same
issue, or another issue that requires the quirk.

IIRC, the original issue is about not being able to use the discrete
GPU with the built-in panel, and the issue about an external GPU did
come up in the discussion - however, this does not affect the built-in
panel at all.

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Lukas Wunner]

On Sun, Dec 29, 2024 at 11:08:55AM +0100, Ard Biesheuvel wrote:
> IIRC, the original issue is about not being able to use the discrete
> GPU with the built-in panel, and the issue about an external GPU did
> come up in the discussion - however, this does not affect the built-in
> panel at all.

The original issue was that the integrated GPU is hidden (powered off)
unless the set_os protocol is used.  So only the discrete GPU is
available, which results in terrible battery life.  Using set_os
keeps the iGPU exposed so the OS can switch to it and power off
the dGPU.

We could have solved this by checking whether there are two PCI devices
with VGA class in the system.  But that would have triggered in the
iGPU + eGPU case.  We wanted to avoid that and thus quirked for the
DMI product names instead.

Thanks,

Lukas

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Ard Biesheuvel]

On Sun, 29 Dec 2024 at 11:38, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Sun, Dec 29, 2024 at 11:08:55AM +0100, Ard Biesheuvel wrote:
> > IIRC, the original issue is about not being able to use the discrete
> > GPU with the built-in panel, and the issue about an external GPU did
> > come up in the discussion - however, this does not affect the built-in
> > panel at all.
>
> The original issue was that the integrated GPU is hidden (powered off)
> unless the set_os protocol is used.  So only the discrete GPU is
> available, which results in terrible battery life.  Using set_os
> keeps the iGPU exposed so the OS can switch to it and power off
> the dGPU.
>

OK, thanks for clearing that up.

But this is distinctly different from the eGPU case we are dealing with here.

> We could have solved this by checking whether there are two PCI devices
> with VGA class in the system.  But that would have triggered in the
> iGPU + eGPU case.  We wanted to avoid that and thus quirked for the
> DMI product names instead.
>

Yeah, so it would be good to know what issue are trying to fix with this patch.

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Lleyton Gray]

I believe this is another issue that requires the same set_os quirk. In 
specific, amdgpu fails to initialize when using an AMD eGPU in a T2 
system, unless the set_os protocol is used. Because it's currently 
quirked for the product names of dual-gpu T2 Macs, if you're on a system 
that doesn't match those names (ex. 2018 Mac Mini), there's no way to 
enable the protocol to get an eGPU working.

On 12/29/24 10:22, Ard Biesheuvel wrote:
> On Sun, 29 Dec 2024 at 11:38, Lukas Wunner <lukas@wunner.de> wrote:
>> On Sun, Dec 29, 2024 at 11:08:55AM +0100, Ard Biesheuvel wrote:
>>> IIRC, the original issue is about not being able to use the discrete
>>> GPU with the built-in panel, and the issue about an external GPU did
>>> come up in the discussion - however, this does not affect the built-in
>>> panel at all.
>> The original issue was that the integrated GPU is hidden (powered off)
>> unless the set_os protocol is used.  So only the discrete GPU is
>> available, which results in terrible battery life.  Using set_os
>> keeps the iGPU exposed so the OS can switch to it and power off
>> the dGPU.
>>
> OK, thanks for clearing that up.
>
> But this is distinctly different from the eGPU case we are dealing with here.
>
>> We could have solved this by checking whether there are two PCI devices
>> with VGA class in the system.  But that would have triggered in the
>> iGPU + eGPU case.  We wanted to avoid that and thus quirked for the
>> DMI product names instead.
>>
> Yeah, so it would be good to know what issue are trying to fix with this patch.

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Lukas Wunner]

Hi Lleyton,

my apologies for the delay.

On Sun, Dec 29, 2024 at 07:09:15PM -0800, Lleyton Gray wrote:
> On 12/29/24 10:22, Ard Biesheuvel wrote:
> > On Sun, 29 Dec 2024 at 11:38, Lukas Wunner <lukas@wunner.de> wrote:
> > > The original issue was that the integrated GPU is hidden (powered off)
> > > unless the set_os protocol is used.  So only the discrete GPU is
> > > available, which results in terrible battery life.  Using set_os
> > > keeps the iGPU exposed so the OS can switch to it and power off
> > > the dGPU.
> > 
> > Yeah, so it would be good to know what issue are trying to fix with
> > this patch.
>
> I believe this is another issue that requires the same set_os quirk. In
> specific, amdgpu fails to initialize when using an AMD eGPU in a T2 system,
> unless the set_os protocol is used. Because it's currently quirked for the
> product names of dual-gpu T2 Macs, if you're on a system that doesn't match
> those names (ex. 2018 Mac Mini), there's no way to enable the protocol to
> get an eGPU working.

How does the amdgpu initialization issue manifest itself exactly?

Could you either provide full dmesg output both for the failing case
and the success case (i.e. with your patch)?

E.g. could you open a bug on bugzilla.kernel.org and attach the dmesg
output or at least provide a dmesg excerpt with the relevant amdgpu
initialization messages?

Any patch to fix the issue needs to provide a proper explanation
of what's going wrong without the patch and why, so that anyone
working on the code in the future can understand why the change
was made.  Just stating that amdgpu fails to initialize isn't
sufficient in my view.

Thanks,

Lukas

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Lukas Wunner]

Hi Aditya,

sorry for the delay!

On Wed, Jan 01, 2025 at 11:39:13AM +0000, Aditya Garg wrote:
> We have had issues with other people unable to use their eGPU if their
> Mac doesn't have the apple set os quirk. We probably could do dual VGA
> checks suggested by Lukas, but will it work with GPU hotplug?

FWIW, below would be my suggestion for replacing the DMI-based quirk
with one that is based on the number of GPUs.

It should invoke the apple_set_os protocol both on dual GPU laptops
as well as ones with an eGPU, hence my expectation is that it should
fix the issue reported by Lleyton.

The quirk is not applied e.g. on single GPU MacBook Airs, hence
should avoid regressing those.

The patch is compile-tested only.

It performs one additional 16-bit config space read for every PCI
device in the system.  If anyone objects to that and wants it
constrained to Apple systems, that could be changed easily.

Thanks,

Lukas

-- >8 --

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 863910e9eefc..3092a6e5166f 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -27,6 +27,7 @@ const efi_system_table_t *efi_system_table;
 const efi_dxe_services_table_t *efi_dxe_table;
 static efi_loaded_image_t *image = NULL;
 static efi_memory_attribute_protocol_t *memattr;
+static unsigned int nr_gpus;
 
 typedef union sev_memory_acceptance_protocol sev_memory_acceptance_protocol_t;
 union sev_memory_acceptance_protocol {
@@ -39,6 +40,23 @@ union sev_memory_acceptance_protocol {
 	} mixed_mode;
 };
 
+static void update_nr_gpus(efi_pci_io_protocol_t *pci)
+{
+	efi_status_t status;
+	u16 class;
+
+	status = efi_call_proto(pci, pci.read, EfiPciIoWidthUint16,
+				PCI_CLASS_DEVICE, 1, &class);
+
+	if (status != EFI_SUCCESS) {
+		efi_err("Failed to read device class\n");
+		return;
+	}
+
+	if (class >> 8 == PCI_BASE_CLASS_DISPLAY)
+		nr_gpus++;
+}
+
 static efi_status_t
 preserve_pci_rom_image(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
 {
@@ -150,6 +168,8 @@ static void setup_efi_pci(struct boot_params *params)
 			params->hdr.setup_data = (unsigned long)rom;
 
 		data = (struct setup_data *)rom;
+
+		update_nr_gpus(pci);
 	}
 }
 
@@ -203,37 +223,6 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
 	}
 }
 
-static bool apple_match_product_name(void)
-{
-	static const char type1_product_matches[][15] = {
-		"MacBookPro11,3",
-		"MacBookPro11,5",
-		"MacBookPro13,3",
-		"MacBookPro14,3",
-		"MacBookPro15,1",
-		"MacBookPro15,3",
-		"MacBookPro16,1",
-		"MacBookPro16,4",
-	};
-	const struct efi_smbios_type1_record *record;
-	const u8 *product;
-
-	record = (struct efi_smbios_type1_record *)efi_get_smbios_record(1);
-	if (!record)
-		return false;
-
-	product = efi_get_smbios_string(record, product_name);
-	if (!product)
-		return false;
-
-	for (int i = 0; i < ARRAY_SIZE(type1_product_matches); i++) {
-		if (!strcmp(product, type1_product_matches[i]))
-			return true;
-	}
-
-	return false;
-}
-
 static void apple_set_os(void)
 {
 	struct {
@@ -243,7 +232,7 @@ static void apple_set_os(void)
 	} *set_os;
 	efi_status_t status;
 
-	if (!efi_is_64bit() || !apple_match_product_name())
+	if (!efi_is_64bit() || nr_gpus < 2)
 		return;
 
 	status = efi_bs_call(locate_protocol, &APPLE_SET_OS_PROTOCOL_GUID, NULL,

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Aditya Garg]

Hi Lukas

> On 11 Feb 2025, at 1:28 AM, Lukas Wunner <lukas@wunner.de> wrote:
> 
> Hi Aditya,
> 
> sorry for the delay!
> 
> On Wed, Jan 01, 2025 at 11:39:13AM +0000, Aditya Garg wrote:
>> We have had issues with other people unable to use their eGPU if their
>> Mac doesn't have the apple set os quirk. We probably could do dual VGA
>> checks suggested by Lukas, but will it work with GPU hotplug?
> 
> FWIW, below would be my suggestion for replacing the DMI-based quirk
> with one that is based on the number of GPUs.
> 
> It should invoke the apple_set_os protocol both on dual GPU laptops
> as well as ones with an eGPU, hence my expectation is that it should
> fix the issue reported by Lleyton.
> 
> The quirk is not applied e.g. on single GPU MacBook Airs, hence
> should avoid regressing those.
> 
> The patch is compile-tested only.
> 
> It performs one additional 16-bit config space read for every PCI
> device in the system.  If anyone objects to that and wants it
> constrained to Apple systems, that could be changed easily.
> 
> Thanks,
> 
> Lukas
> 
> -- >8 --
> 
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 863910e9eefc..3092a6e5166f 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -27,6 +27,7 @@ const efi_system_table_t *efi_system_table;
> const efi_dxe_services_table_t *efi_dxe_table;
> static efi_loaded_image_t *image = NULL;
> static efi_memory_attribute_protocol_t *memattr;
> +static unsigned int nr_gpus;
> 
> typedef union sev_memory_acceptance_protocol sev_memory_acceptance_protocol_t;
> union sev_memory_acceptance_protocol {
> @@ -39,6 +40,23 @@ union sev_memory_acceptance_protocol {
> } mixed_mode;
> };
> 
> +static void update_nr_gpus(efi_pci_io_protocol_t *pci)
> +{
> + efi_status_t status;
> + u16 class;
> +
> + status = efi_call_proto(pci, pci.read, EfiPciIoWidthUint16,
> + PCI_CLASS_DEVICE, 1, &class);
> +
> + if (status != EFI_SUCCESS) {
> + efi_err("Failed to read device class\n");
> + return;
> + }
> +
> + if (class >> 8 == PCI_BASE_CLASS_DISPLAY)
> + nr_gpus++;
> +}
> +
> static efi_status_t
> preserve_pci_rom_image(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
> {
> @@ -150,6 +168,8 @@ static void setup_efi_pci(struct boot_params *params)
> params->hdr.setup_data = (unsigned long)rom;
> 
> data = (struct setup_data *)rom;
> +
> + update_nr_gpus(pci);
> }
> }
> 
> @@ -203,37 +223,6 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
> }
> }
> 
> -static bool apple_match_product_name(void)
> -{
> - static const char type1_product_matches[][15] = {
> - "MacBookPro11,3",
> - "MacBookPro11,5",
> - "MacBookPro13,3",
> - "MacBookPro14,3",
> - "MacBookPro15,1",
> - "MacBookPro15,3",
> - "MacBookPro16,1",
> - "MacBookPro16,4",
> - };
> - const struct efi_smbios_type1_record *record;
> - const u8 *product;
> -
> - record = (struct efi_smbios_type1_record *)efi_get_smbios_record(1);
> - if (!record)
> - return false;
> -
> - product = efi_get_smbios_string(record, product_name);
> - if (!product)
> - return false;
> -
> - for (int i = 0; i < ARRAY_SIZE(type1_product_matches); i++) {
> - if (!strcmp(product, type1_product_matches[i]))
> - return true;
> - }
> -
> - return false;
> -}
> -
> static void apple_set_os(void)
> {
> struct {
> @@ -243,7 +232,7 @@ static void apple_set_os(void)
> } *set_os;
> efi_status_t status;
> 
> - if (!efi_is_64bit() || !apple_match_product_name())
> + if (!efi_is_64bit() || nr_gpus < 2)
> return;
> 
> status = efi_bs_call(locate_protocol, &APPLE_SET_OS_PROTOCOL_GUID, NULL,

This patch does not enable the os set protocol on my MacBook Pro 16 inch 2019

journalctl -k: https://pastebin.com/7etWy0D5

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Lukas Wunner]

On Tue, Feb 11, 2025 at 04:05:12PM +0000, Aditya Garg wrote:
> > On 11 Feb 2025, at 1:28AM, Lukas Wunner <lukas@wunner.de> wrote:
> > FWIW, below would be my suggestion for replacing the DMI-based quirk
> > with one that is based on the number of GPUs.
> > 
> > It should invoke the apple_set_os protocol both on dual GPU laptops
> > as well as ones with an eGPU, hence my expectation is that it should
> > fix the issue reported by Lleyton.
> 
> This patch does not enable the os set protocol on my MacBook Pro 16 inch 2019
> 
> journalctl -k: https://pastebin.com/7etWy0D5

Hm, perhaps Apple's EFI disables the iGPU by default and re-enables it
upon the set_os protocol call.  Or I've botched the patch, but I just
double-checked the logic and it seems fine to me.

Could somebody with an eGPU test whether the patch results in the
expected invocation of set_os (and thus a working eGPU)?

If it does, we'd just have to keep apple_match_product_name() as an
alternative condition under which set_os is called.

Thanks,

Lukas

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Lukas Wunner]

On Wed, Jan 01, 2025 at 11:39:13AM +0000, Aditya Garg wrote:
> We have had issues with other people unable to use their eGPU if their
> Mac doesn't have the apple set os quirk. We probably could do dual VGA
> checks suggested by Lukas, but will it work with GPU hotplug?

About hotplug:  There is a tiny time window when the EFI stub runs
during which an eGPU may be hotplugged and incorrectly not be
detected.  I.e. the quirk is not applied.  I don't think this can
be fixed.

If an eGPU is hotplugged while EFI boot services are still running,
Apple's EFI Thunderbolt driver will set up a tunnel to the device
and it will then be visible to the kernel's efi stub.  E.g. if you
hold down the Option key during boot and attach an eGPU, then
select the operating system to boot, the eGPU should correctly be
seen by the efi stub and the quirk is applied.

However hotplugging an eGPU at exactly the moment when the efi stub
runs results in a race between the efi stub doing all the initialization
and exiting boot services on the one hand, and the EFI Thunderbolt
driver bringing up the eGPU on the other hand.

But IMO this is "good enough".

Thanks,

Lukas

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Aditya Garg]

> On 11 Feb 2025, at 1:39 AM, Lukas Wunner <lukas@wunner.de> wrote:
> 
> On Wed, Jan 01, 2025 at 11:39:13AM +0000, Aditya Garg wrote:
>> We have had issues with other people unable to use their eGPU if their
>> Mac doesn't have the apple set os quirk. We probably could do dual VGA
>> checks suggested by Lukas, but will it work with GPU hotplug?
> 
> About hotplug:  There is a tiny time window when the EFI stub runs
> during which an eGPU may be hotplugged and incorrectly not be
> detected.  I.e. the quirk is not applied.  I don't think this can
> be fixed.
> 
> If an eGPU is hotplugged while EFI boot services are still running,
> Apple's EFI Thunderbolt driver will set up a tunnel to the device
> and it will then be visible to the kernel's efi stub.  E.g. if you
> hold down the Option key during boot and attach an eGPU, then
> select the operating system to boot, the eGPU should correctly be
> seen by the efi stub and the quirk is applied.
> 
> However hotplugging an eGPU at exactly the moment when the efi stub
> runs results in a race between the efi stub doing all the initialization
> and exiting boot services on the one hand, and the EFI Thunderbolt
> driver bringing up the eGPU on the other hand.
> 
> But IMO this is "good enough".

Something is better than nothing.

I'll test your patch, and imo restricting it to Apple devices should be a good idea.
> 
> Thanks,
> 
> Lukas

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Aditya Garg]

Hi all

I managed to get feedbacks regarding eGPUs on older non T2 MacBooks, and can confirm that apple-set-os is needed there as well.

I was wondering if we can reach a conclusion for this case here.

> On 29 Dec 2024, at 1:51 AM, Lleyton Gray <lleyton@fyralabs.com> wrote:
> 
> commit 71e49eccdca6 ("x86/efistub: Call Apple set_os protocol on dual GPU
> Intel Macs") calls the Apple set_os protocol, but only on T2 Macbook Pro
> models. This causes issues on other T2 models when an egpu is used.
> Add two options which allow force enabling or disabling usage of the
> protocol to fix.
> 
> Signed-off-by: Lleyton Gray <lleyton@fyralabs.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 7 ++++++-
> drivers/firmware/efi/libstub/efi-stub-helper.c  | 6 ++++++
> drivers/firmware/efi/libstub/efistub.h          | 2 ++
> drivers/firmware/efi/libstub/x86-stub.c         | 3 ++-
> 4 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1518343bbe22..1d1b88c57c44 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1447,7 +1447,8 @@
> efi= [EFI,EARLY]
> Format: { "debug", "disable_early_pci_dma",
>  "nochunk", "noruntime", "nosoftreserve",
> -  "novamap", "no_disable_early_pci_dma" }
> +  "novamap", "no_disable_early_pci_dma",
> +  "enable_apple_set_os", "disable_apple_set_os" }
> debug: enable misc debug output.
> disable_early_pci_dma: disable the busmaster bit on all
> PCI bridges while in the EFI boot stub.
> @@ -1464,6 +1465,10 @@
> novamap: do not call SetVirtualAddressMap().
> no_disable_early_pci_dma: Leave the busmaster bit set
> on all PCI bridges while in the EFI boot stub
> + enable_apple_set_os: Report that macOS is being booted
> + to the firmware, even if the device is not a dual GPU Mac.
> + disable_apple_set_os: Disable reporting that macOS is being booted
> + to the firmware, even if the device is a dual GPU Mac.
> 
> efi_no_storage_paranoia [EFI,X86,EARLY]
> Using this parameter you can use more than 50% of
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index de659f6a815f..c33bb98bf79d 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -20,6 +20,8 @@
> bool efi_nochunk;
> bool efi_nokaslr = !IS_ENABLED(CONFIG_RANDOMIZE_BASE);
> bool efi_novamap;
> +bool efi_enable_apple_set_os;
> +bool efi_disable_apple_set_os;
> 
> static bool efi_noinitrd;
> static bool efi_nosoftreserve;
> @@ -95,6 +97,10 @@ efi_status_t efi_parse_options(char const *cmdline)
> efi_disable_pci_dma = true;
> if (parse_option_str(val, "no_disable_early_pci_dma"))
> efi_disable_pci_dma = false;
> + if (parse_option_str(val, "enable_apple_set_os"))
> + efi_enable_apple_set_os = true;
> + if (parse_option_str(val, "disable_apple_set_os"))
> + efi_disable_apple_set_os = true;
> if (parse_option_str(val, "debug"))
> efi_loglevel = CONSOLE_LOGLEVEL_DEBUG;
> } else if (!strcmp(param, "video") &&
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 685098f9626f..3be4b5393812 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -39,6 +39,8 @@ extern bool efi_nokaslr;
> extern int efi_loglevel;
> extern int efi_mem_encrypt;
> extern bool efi_novamap;
> +extern bool efi_enable_apple_set_os;
> +extern bool efi_disable_apple_set_os;
> extern const efi_system_table_t *efi_system_table;
> 
> typedef union efi_dxe_services_table efi_dxe_services_table_t;
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index f8e465da344d..566118195f92 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -265,7 +265,8 @@ static void apple_set_os(void)
> } *set_os;
> efi_status_t status;
> 
> - if (!efi_is_64bit() || !apple_match_product_name())
> + if (efi_disable_apple_set_os || !efi_is_64bit() ||
> + !efi_enable_apple_set_os && !apple_match_product_name())
> return;
> 
> status = efi_bs_call(locate_protocol, &APPLE_SET_OS_PROTOCOL_GUID, NULL,
> -- 
> 2.47.0
> 

Re: [PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Ard Biesheuvel]

Hi Aditya,

On Sun, 9 Feb 2025 at 17:13, Aditya Garg <gargaditya08@live.com> wrote:
>
> Hi all
>

Please don't top post.

> I managed to get feedbacks regarding eGPUs on older non T2 MacBooks, and can confirm that apple-set-os is needed there as well.
>

This is rather vague. Where did you get this feedback, and what
systems does it cover?

Also, this is not about being able to switch to the integrated GPU to
drive the built-in panel, but for driving an external GPU connected to
a different screen altogether. AIUI, this has never worked on these
systems, right? So why is this important now, for 10 year old
Macbooks?

> I was wondering if we can reach a conclusion for this case here.
>

I'd prefer to address this in a different way: instead of fixing this
in the Linux kernel for the handful of systems that need this (and
that will upgrade to v6.15 or later to get this fix), why not use a
separate boot-time EFI app to call the protocol. That way, it works
with existing kernels too, and no changes are needed to Linux.

https://paste.debian.net/1349311/

[PATCH] x86/efistub: Add options for forcing Apple set_os protocol

[Aditya Garg]

> On 10 Feb 2025, at 4:11 PM, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> Hi Aditya,
> 
>> On Sun, 9 Feb 2025 at 17:13, Aditya Garg <gargaditya08@live.com> wrote:
>> Hi all
> 
> Please don't top post.
> 
>> I managed to get feedbacks regarding eGPUs on older non T2 MacBooks, and can confirm that apple-set-os is needed there as well.
> 
> This is rather vague. Where did you get this feedback, and what
> systems does it cover?
> 
> Also, this is not about being able to switch to the integrated GPU to
> drive the built-in panel, but for driving an external GPU connected to
> a different screen altogether. AIUI, this has never worked on these
> systems, right? So why is this important now, for 10 year old
> Macbooks?
> 
>> I was wondering if we can reach a conclusion for this case here.
> 
> I'd prefer to address this in a different way: instead of fixing this
> in the Linux kernel for the handful of systems that need this (and
> that will upgrade to v6.15 or later to get this fix), why not use a
> separate boot-time EFI app to call the protocol. That way, it works
> with existing kernels too, and no changes are needed to Linux.
Fair enough, let's close this thing then.

Thanks
Aditya

> https://paste.debian.net/1349311/