> On 30 Jun 2024, at 6:29 PM, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > On Sun, 30 Jun 2024 at 13:56, Aditya Garg <gargaditya08@xxxxxxxx> wrote: >> >> >> >>>> On 30 Jun 2024, at 4:59 PM, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: >>> >>> Hello Aditya, Lukas, >>> >>>> On Sun, 30 Jun 2024 at 10:04, Lukas Wunner <lukas@xxxxxxxxx> 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@xxxxxxxxx>: >>>>> 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? > > Yes. To summarize: > - keep your signoff as-is > - drop the command line param and handling > - make the outer protocol struct a union > - use 'unsigned long' for the version field > - keep the mixed_mode inner struct > - reorganize the conditional in setup_quirks() so that there are two > nested if blocks, where the memcmp() appears in the outer if() and > apple_set_os() is only called on Apple hardware That's for the summary Ard. Sending a v2
