[Skiboot] [PATCH] secvar/backend: use endian-aware types in edk2.h

[Daniel Axtens]

Nicholas Piggin <npiggin at gmail.com> writes:

>> There's one annotation that I'm not super happy about - the
>> annotation of efi_time->timezone. Happy for other ideas on that
>> one.
>
> Is timezone used anywhere?

No. I could kill off the annotation but it seems silly not to mark that
it's a little-endian s16 given that I'm marking everything else. I guess
we could make all the unused fields into an array of reserved u8s if we
really wanted.

>> 
>> Signed-off-by: Daniel Axtens <dja at axtens.net>
>> ---
>>  libstb/secvar/backend/edk2.h | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/libstb/secvar/backend/edk2.h b/libstb/secvar/backend/edk2.h
>> index ef6d7c79e7ff..17df2b7bc536 100644
>> --- a/libstb/secvar/backend/edk2.h
>> +++ b/libstb/secvar/backend/edk2.h
>> @@ -125,15 +125,15 @@ static const uuid_t EFI_CERT_RSA2048_GUID = {{ 0xe8, 0x66, 0x57, 0x3c, 0x9c, 0x2
>>   *   TimeZone:   -1440 to 1440 or 2047
>>   */
>>  struct efi_time {
>> -	u16 year;
>> +	le16 year;
>>  	u8 month;
>>  	u8 day;
>>  	u8 hour;
>>  	u8 minute;
>>  	u8 second;
>>  	u8 pad1;
>> -	u32 nanosecond;
>> -	s16 timezone;
>> +	le32 nanosecond;
>> +	s16 ENDIAN_TYPE timezone;
>>  	u8 daylight;
>>  	u8 pad2;
>>  };
>
> Why does it need to be annotated? None of the Linux tree's efi structs 
> seem to have any endian annotations, is that just because they're only
> used on little endian platforms I wonder?

I didn't check Linux's ones, that's interesting to know. The UEFI spec
explictly states that UEFI machines are little-endian machines (s1.8.1
in v2.8), even when the underlying chip is capable of running in either
endianness, so that might explain it.

The rationale for changing them here is that if we don't change them,
every time we do

u16 year = le16_to_cpu(time->year);

we get a sparse warning saying that you can't do an endian conversion on
a non-endian-marked type. I suspect Linux EFI code doesn't bother with
the conversion and therefore we don't get the sparse warnings on Linux.

However, even if we were OK with skiboot being LE forever, we don't want
to get rid of the endianness conversions here, because this code is also
shared with secvarctl (and hopefully other things soon), and they can
run as BE and so do need to get endianness right.

Kind regards,
Daniel

>
> Thanks,
> Nick