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. > > --- 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 > > +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. > > > +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.
