Message-ID: <CAMj1kXHR+mCR5eibj23S26-PN6yLPD1uf9+H2fEEDhNWOh6TjA@mail.gmail.com>
Date: Mon, 1 Jul 2024 09:30:34 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: Aditya Garg <gargaditya08@...e.com>, Hans de Goede <hdegoede@...hat.com>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, 
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>, Kerem Karabay <kekrby@...il.com>, 
	Orlando Chamberlain <orlandoch.dev@...il.com>
Subject: Re: [PATCH v2] efi: libstub: add support for the apple_set_os protocol

On Mon, 1 Jul 2024 at 08:41, Lukas Wunner <lukas@...ner.de> wrote:
>
> On Mon, Jul 01, 2024 at 07:56:04AM +0200, Ard Biesheuvel wrote:
> > On Mon, 1 Jul 2024 at 07:50, Lukas Wunner <lukas@...ner.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:
> > >
...
> > > * 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.
> >
...
> > Is the latter side effect
> > likely to cause any regressions on Intel Mac users that don't have two
> > GPUs to begin with?
>
> MacBook Air models introduced 2013/2014 will use SPI instead of USB
> to access the keyboard and trackpad.  And from a quick look, the
> applespi_tp_models[] array in drivers/input/keyboard/applespi.c
> seems to be missing the trackpad dimensions of those models.
> The driver may also need a quirk to work around missing properties
> in the DSDT on those models.  Back in 2018 someone tested apple_set_os()
> on such a machine and reported these issues, but the GitHub discussion
> to narrow them down and fix them fizzled out:
>
> https://github.com/cb22/macbook12-spi-driver/issues/65
>
> The problem is that users of such models will generally run a
> distribution kernel which has CONFIG_APPLE_GMUX enabled,
> so constraining apple_set_os() to CONFIG_APPLE_GMUX won't help them.
> Also, there is no incentive to amend the applespi.c driver for
> affected machines if apple_set_os() is never called on them,
> which is regrettable.
>
> Another potential regression is that exposing the iGPU may consume
> more power.  Or maybe the i915 driver will autosuspend if the panel
> is not connected, I don't know.  Likewise there is no incentive to
> fix the issue if apple_set_os() is never run.
>
> I've also heard rumors that the EFI firmware configures different
> CPU frequency scaling parameters if apple_set_os() is called,
> but maybe Linux overwrites them anyway.  Apple never cared for Linux,
> so apple_set_os() basically just differentiates between macOS and
> Windows (via Boot Camp).  We generally choose to masquerade as macOS
> to get access to the full set of hardware features, not the crippled
> setup exposed to Windows.
>
> 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.
>

DMI matching would be a nice way to apply this change only to systems
that have a need for it, without manual intervention via a command
line switch.

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.



diff --git a/drivers/firmware/efi/libstub/x86-stub.c
b/drivers/firmware/efi/libstub/x86-stub.c
index 250c964e93ee..826756c02657 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -225,12 +225,39 @@ static void
retrieve_apple_device_properties(struct boot_params *boot_params)
        }
 }

+static bool apple_match_product_name(void)
+{
+       static const char type1_matches[][16] = {
+               "MacBookPro16,1",
+       };
+       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->header, 1, product_name);
+       if (!product)
+               return false;
+
+       for (int i = 0; i < ARRAY_SIZE(type1_matches); i++) {
+               if (!strcmp(product, type1_matches[i]))
+                       return true;
+       }
+
+       return false;
+}
+
 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;

+       if (!apple_match_product_name())
+               return;
+
        status = efi_bs_call(locate_protocol, &guid, NULL, (void **)&set_os);
        if (status != EFI_SUCCESS)
                return;

Powered by blists - more mailing lists