Re: [PATCH] efi: libstub: add support for the apple_set_os protocol

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux