Message-ID: <ZoEtG0DUJOS4ROQC@wunner.de>
Date: Sun, 30 Jun 2024 12:02:03 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Aditya Garg <gargaditya08@...e.com>
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 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*.

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.

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