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

[Nicholas Piggin]

Excerpts from Daniel Axtens's message of June 23, 2021 12:36 am:
> 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.

Ah yes I guess it would, thanks.

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

Right, I'm asking about the timezone annotation though. Oh actually on 
re-reading what I wrote, that wasn't clear, there were two trains of
thought colliding there.

I'll try again, AFAIK you can use le16 for it and do `s16 tz = 
le16_to_cpu(efi->timezone)` and the signedness works out.

Yes you do lose the signedness of the type when you do that but it seems 
to be the done thing. For non trivial cases you might have a host native
structure as well and convert between them.


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

No the patch is good, my question was rhetorical as-in, why do we have 
to and not Linux, you answered that.

Thanks,
Nick