Message-ID:
 <MA0P287MB0217B3F9B9E1CFA54FAD96CEB8D22@MA0P287MB0217.INDP287.PROD.OUTLOOK.COM>
Date: Sun, 30 Jun 2024 10:50:35 +0000
From: Aditya Garg <gargaditya08@...e.com>
To: Lukas Wunner <lukas@...ner.de>
CC: Ard Biesheuvel <ardb@...nel.org>, Hans de Goede <hdegoede@...hat.com>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>, Linux Kernel Mailing
 List <linux-kernel@...r.kernel.org>, Orlando Chamberlain
	<orlandoch.dev@...il.com>, Kerem Karabay <kekrby@...il.com>
Subject: Re: [PATCH] efi: libstub: add support for the apple_set_os protocol



> On 30 Jun 2024, at 3:32 PM, Lukas Wunner <lukas@...ner.de> wrote:
> 
> On Sun, Jun 30, 2024 at 09:13:33AM +0000, Aditya Garg wrote:
>>>> On 30 Jun 2024, at 1:34 PM, Lukas Wunner <lukas@...ner.de> wrote:
>>> On Sun, Jun 30, 2024 at 04:42:55AM +0000, Aditya Garg wrote:
>>>> Based on this patch for GRUB by Andreas Heider <andreas@...der.io>:
>>>> https://lists.gnu.org/archive/html/grub-devel/2013-12/msg00442.html
>>> 
>>> Please include his Signed-off-by and cc him.
>> 
>> Not sure about this since the patch was send to grub and not lkml,
>> and his work has been used without informing him for this patch solely
>> on the basis of GPL.
>> 
>> I've always been confused in signed-off-by in case of authors whose work
>> has been used without their explicit consent just because the license
>> permits it.
>> 
>> Should I still add his signed-off-by?
> 
> I would.
> 
> 
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -399,6 +399,8 @@
>>>>                 useful so that a dump capture kernel won't be
>>>>                 shot down by NMI
>>>> 
>>>> +    apple_set_os    [KNL] Report that macOS is being booted to the firmware
>>>> +
>>> 
>>> Why the kernel parameter?  Why not do this unconditionally?
>> 
>> 1. Not all Macs have dual GPU. We don't want to unnecessarily "fool"
>> the firmware in thinking macOS is being booted.
>> 2. apple-gmux is a reverse engineered driver, although upstreamed,
>> not very efficient in switching GPUs. So it's better to make unlocking
>> the GPU optional. + not everyone wants the intel GPU, people are happy
>> working with the dedicated AMD GPU (used by default if apple_set_os
>> isn't enabled).
> 
> So my opinion is that these aren't good arguments.  We should be
> identifying as Darwin by default in EFI, just as we've been doing
> in ACPI since 7bc5a2bad0b8.  If there are any adverse effects,
> we should look into them, but users shouldn't be forced to set
> an obscure kernel parameter only to enable certain features of
> their hardware.  It is for this reason that you'll usually get
> Greg KH's trademark "this isn't the 90s anymore" comment when
> adding new kernel parameters.  We need to handle the hardware
> correctly *automatically*.
> 
I'm not in a favour of "forcing" dual GPU on users, especially because the features are quite unstable. On some distros like Ubuntu, since kernel 6.8, unlocking the GPU results in blank screen instead of igpu due to a regression (note that this patch has nothing to do with this regression, it's something the platform drivers people will look into).

On the 2019 MacBook Pros the vgaswitchroo is not working and inputs from AMD are nil. Basically you get stuck to the Intel GPU and if you try to use the AMD GPU, but the GPUs remain on (currently no way has been found to switch off the AMD one)

So I guess we have 2 options:

1. Wait until apple-gmux becomes quite stable before merging this (fat chance)
2. Give me some better idea to handle this.

> There is one known issue affecting the keyboard on certain
> MacBook Air models:  Apple had been using dual SPI and USB
> keyboards and trackpads since about 2013 but started to ship
> SPI-only models in 2015 with the MacBook 12".
> 
> The models that shipped in the 2013/2014 time frame used USB
> by default and were automatically switched to SPI if apple_set_os
> is invoked.  Not a problem since drivers/input/keyboard/applespi.c
> has been in mainline for a while now, but there are some very
> specific models that lack certain data in the DSDT which the
> applespi.c driver needs:
> 
> https://github.com/cb22/macbook12-spi-driver/issues/65
> 
> However at this point that shouldn't stop us from making
> apple_set_os the default.  We should rather just try to amend
> the applespi.c driver with a DMI quirk or something.
> That requires someone to test patches, so we need to find
> someone with an affected machine before we can fix it.
> 
> But potential issues with 10+ years old hardware shouldn't
> stop us from enabling a feature by default that's useful on
> more recent hardware.  So I'm strongly in favor of getting rid
> of the kernel parameter and then let's work on addressing side
> effects of apple_set_os one by one.
> 
> 
>>>> +struct apple_set_os_protocol {
>>>> +    u64 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;
>>>> +};
>>> 
>>> How about declaring this __packed, just to be on the safe side?
>> You mean "struct __attribute__((__packed__)) apple_set_os_protocol {" ?
> 
> Just add __packed at the end of the declaration.
> See efi_manage_capsule_header in include/linux/efi.h for an example.
> 

Alright. I'll also remove mixed_mode if that works.

> I think we haven't been very consistent with __packed declarations.
> The rationale not to use them everywhere is probably that gcc generally
> doesn't insert padding if only 32-bit and 64-bit types are used,
> but that's not guaranteed to be true forever.
> 
> 
>>> 
>>> Why "mixed_mode"?  Seems like an odd name given "mixed mode"
>>> in EFI context usually means 64-bit OS, but 32-bit EFI.
>> 
>> EFI firmware on T2 Macs doesn't seem to follow the standard EFI specs
>> (expected from Apple). Earlier it claimed to follow EFI 2.0, but we
>> had to force linux to use EFI 1.1 for it. So as far as Apple is
>> concerned, you'll get to see such strange things.
>> 
>> I guess this strange behaviour is because the T2 security chip handles
>> the EFI.
> 
> I don't quite follow.  Andreas' grub patch doesn't have this
> "mixed_mode" struct, so where is it (and the name) coming from?
> 
> Thanks,
> 
> Lukas

Powered by blists - more mailing lists