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

[Lukas Wunner <lukas@wunner.de>]

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@heider.io>:
> https://lists.gnu.org/archive/html/grub-devel/2013-12/msg00442.html

Please include his Signed-off-by and cc him.


> --- 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?


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

Why "mixed_mode"?  Seems like an odd name given "mixed mode"
in EFI context usually means 64-bit OS, but 32-bit EFI.


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

Thanks,

Lukas