[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<MA0P287MB021713D8583D4D53C736F3D8B8D22@MA0P287MB0217.INDP287.PROD.OUTLOOK.COM>
Date: Sun, 30 Jun 2024 11:56:39 +0000
From: Aditya Garg <gargaditya08@...e.com>
To: Ard Biesheuvel <ardb@...nel.org>
CC: Lukas Wunner <lukas@...ner.de>, 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 4:59 PM, Ard Biesheuvel <ardb@...nel.org> wrote:
>
> Hello Aditya, Lukas,
>
>> On Sun, 30 Jun 2024 at 10:04, Lukas Wunner <lukas@...ner.de> wrote:
>>
>>> On Sun, Jun 30, 2024 at 04:42:55AM +0000, Aditya Garg wrote:
>>> Commit 0c18184de990 brought support for T2 Macs in apple-gmux. But in order to
>>
>> Please run patches through scripts/checkpatch.pl before submission.
>> The subject of the commit is missing here and lines should be wrapped
>> at 72 or at least 74 chars.
>>
>>
>>> 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.
>>
>>
>
> No. You cannot simply add a signed-off-by on someone else's behalf,
> even if you cc the person.
>
> Andreas contributed code to GRUB (which is a GPLv3 project), and had
> no involvement whatsoever in creating this patch.
>
> A signed-off-by is a statement on the part of the contributor (which
> may or may not be the author) that the contribution in question
> complies with the requirements imposed by the project in terms of
> copyright and licensing. Linux is GPLv2 not v3, so this code should at
> least be dual licensed in order to be reused directly.
>
> I did not look at the GRUB patch, and IANAL, but this code invokes an
> Apple provided protocol (which is proprietary) in a hardcoded way for
> interoperability purposes, and so there is not much to
> copyright/license anyway. I would be fine with having just your
> signoff on this patch, but you could ask Andreas for a GPLv2+3 version
> of his GRUB patch if you are not sure.
>
I believe this should be GPL2 compatible since it's simple reverse engineered Apple stuff and there are many kernel drivers with apple specific stuff.
>>> --- 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?
>>
>>
>
> Agree that this is suboptimal. If we need a command line param for
> this, please make add it to the efi= list
I'll leave this to you. If you are fine with a parameter, I'll add it. If you have to be enforced by default, I'm fine with that.
Or, maybe we add add a parameter that disables this so that in case things break, we can atleast get it done.
>>> +struct apple_set_os_protocol {
>
> This should be a union not a struct
>
>>> + u64 version;
>
> This should be unsigned long
>
>>> + efi_status_t (__efiapi *set_os_version) (const char *);
>>> + efi_status_t (__efiapi *set_os_vendor) (const char *);
>>> + struct {
>>> + u32 version;
>
> ... to match the mixed_mode overlay which is u32. Alternatively, they
> could both be u64 but the current arrangement is definitely incorrect.
>
>>> + u32 set_os_version;
>>> + u32 set_os_vendor;
>>> + } mixed_mode;
>>> +};
>>
>> How about declaring this __packed, just to be on the safe side?
>>
>
> I don't think that is necessary. If the mixed_mode overlay is never
> used, it doesn't really matter and you can use unsigned long vs u32,
> in which case all struct members are native word size so there is no
> padding issue.
>
>> Why "mixed_mode"? Seems like an odd name given "mixed mode"
>> in EFI context usually means 64-bit OS, but 32-bit EFI.
>>
>
> This is how the x86 plumbing works for mixed mode. Every EFI protocol
> is a union, with a mixed_mode member describing the 32-bit view. The
> accessor macros (efi_bs_call, efi_table_attr) automatically do the
> right thing depending on the bitness of the firmware.
>
> This means, though, that even protocols that are known not to exist on
> 32-bit firmware need to be implemented in the same way, or the code
> will not build.
>
So should I keep mixed mode or not?
>>
>>> +static void apple_set_os(void)
>>> +{
>>> + efi_guid_t guid = APPLE_SET_OS_PROTOCOL_GUID;
>>
>> My recollection is that if you don't declare this static const,
>> gcc generates suboptimal code. (It constructs the GUID on the
>> stack at runtime instead of storing it in .rodata.)
>> Maybe it's become smarter in the meantime, but I doubt it.
>>
>
> I don't remember the details but it looks like we stopped doing this a
> while ago. I don't mind keeping it like this.
Powered by blists - more mailing lists