Re: [PATCH 2/4] efi: Add a function to check if Secure Boot mode is enabled

[Jan Beulich <jbeulich@xxxxxxxx>]

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