On 05/09/2025 11:44 am, Jan Beulich wrote:
> On 05.09.2025 12:36, Andrew Cooper wrote:
>> On 05/09/2025 11:05 am, Gerald Elder-Vass wrote:
>>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>>> index e12fa1a7ec04..e7e3dffa7ddc 100644
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -901,6 +901,28 @@ static void __init pre_parse(const struct file *file)
>>> " last line will be ignored.\r\n");
>>> }
>>>
>>> +static void __init init_secure_boot_mode(void)
>>> +{
>>> + static EFI_GUID __initdata gv_uuid = EFI_GLOBAL_VARIABLE;
>>> + EFI_STATUS status;
>>> + uint8_t data = 0;
>>> + UINTN size = sizeof(data);
>>> + UINT32 attr = 0;
>>> +
>>> + status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
>>> + &size, &data);
>> This turns out to be a MISRA R7.4 violation, complaining about casing a
>> string literal to a non-const pointer.
>>
>> The real problem here is that the EFI spec. GetVariable() ought to take
>> a const CHAR16 *, but doesn't.
>>
>> We could fix this with:
>>
>> diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
>> index a616d1238aa4..56775d553109 100644
>> --- a/xen/include/efi/efiapi.h
>> +++ b/xen/include/efi/efiapi.h
>> @@ -224,7 +224,7 @@ VOID
>> typedef
>> EFI_STATUS
>> (EFIAPI *EFI_GET_VARIABLE) (
>> - IN CHAR16 *VariableName,
>> + IN const CHAR16 *VariableName,
>> IN EFI_GUID *VendorGuid,
>> OUT UINT32 *Attributes OPTIONAL,
>> IN OUT UINTN *DataSize,
>>
>> but I fear this might get some objections.
> The interface lacking the const in principle means that we can't rely on
> there being implementations which actually do fiddle with the string.
Well, the IN and absence of OUT does mean this in practice.
> Hence ...
>
>> I don't think we want to be deviating every use of GetVariable() for a
>> problem ultimately outside of our control.
>>
>> Another option would be to have a wrapper for GetVariable() which does
>> the cast once, which lets us deviate in one place only.
> ... this doesn't look like a viable route to me. (Nor a scalable one,
> as down the road we then may need more such wrappers.)
>
>> Thoughts?
> Why not instead use
>
> static CHAR16 __initdata str_SecureBoot[] = L"SecureBoot";
>
> and be done?
I suppose, but that's still awkward to use.
~Andrew
