On 06.05.2025 18:55, Andrew Cooper wrote:
> On 06/05/2025 5:24 pm, Kevin Lampis wrote:
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index e39fbc3529..7c528cd5dd 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -870,6 +870,27 @@ static void __init pre_parse(const struct file *file)
>> " last line will be ignored.\r\n");
>> }
>>
>> +static void __init init_secure_boot_mode(void)
>> +{
>> + EFI_STATUS status;
>> + EFI_GUID gv_uuid = EFI_GLOBAL_VARIABLE;
>> + uint8_t data = 0;
>> + UINTN size = sizeof(data);
>> + UINT32 attr = 0;
>
> Newline between variables and code please.
>
>> + status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
>> + &size, &data);
>> +
>> + if ( status == EFI_NOT_FOUND ||
>> + (status == EFI_SUCCESS &&
>> + attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> EFI_VARIABLE_RUNTIME_ACCESS) &&
>> + size == 1 && data == 0) )
>> + /* Platform does not support Secure Boot or it's disabled. */
>> + efi_secure_boot = false;
>> + else
>> + /* Everything else play it safe and assume enabled. */
>> + efi_secure_boot = true;
>> +}
>
> I'm not sure this logic does what you want when a weird answer comes
> back from GetVariable().
>
> Also, you can't have this be a common function, yet ...
>
>> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
>> index 7e1fce291d..b63d21f16c 100644
>> --- a/xen/common/efi/runtime.c
>> +++ b/xen/common/efi/runtime.c
>> @@ -40,6 +40,9 @@ void efi_rs_leave(struct efi_rs_state *state);
>> unsigned int __read_mostly efi_num_ct;
>> const EFI_CONFIGURATION_TABLE *__read_mostly efi_ct;
>>
>> +#if defined(CONFIG_X86) && !defined(CONFIG_PV_SHIM)
>> +bool __ro_after_init efi_secure_boot;
>> +#endif
>
> ... this variable exist only on x86.
>
> Also, why adjust it for PV shim? None of this is even compiled for PV shim.
For shim it might be (and a proper variable is needed in that case); for
shim-exclusive it shouldn't be.
Jan
