On 07/07/20 19:11, Tom Lendacky wrote:
> I commented out the setting of the GhcbTerminate fields in the
> SevEsProtocolFailure() routine of OvmfPkg/Sec/SecMain.c and the error
> disappeared. I'll see if changing from using UINT64 to multiple UINT32
> entries fixes the problem, but I wouldn't think that the bit fields
> would/should cause an issue here with 32-bit builds.
For two examples from
"MdePkg/Include/Register/Intel/ArchitecturalMsr.h": the
- MSR_IA32_FEATURE_CONTROL_REGISTER
- MSR_IA32_EFER_REGISTER
types use UINT32 bit-field members.
--*--
I've always disliked bit-fields. Consider:
C99 "6.7.2 Type specifiers"
5 [...] for bit-fields, it is implementation-defined whether the
specifier int designates the same type as signed int or the same
type as unsigned int.
C99 "6.7.2.1 Structure and union specifiers":
4 A bit-field shall have a type that is a qualified or unqualified
version of _Bool, signed int, unsigned int, or some other
implementation-defined type.
10 An implementation may allocate any addressable storage unit large
enough to hold a bit-field. If enough space remains, a bit-field
that immediately follows another bit-field in a structure shall be
packed into adjacent bits of the same unit. If insufficient space
remains, whether a bit-field that does not fit is put into the next
unit or overlaps adjacent units is implementation-defined. The
order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined.
The alignment of the addressable storage unit is unspecified.
The C99 *Rationale* says under "6.7.2.1. Structure and union
specifiers",
Three types of bit-fields are now defined: plain int calls for
implementation-defined signedness (as in K&R), signed int calls for
assuredly signed fields, and unsigned int calls for unsigned fields.
The old constraints on bit-fields crossing word boundaries have been
relaxed, since so many properties of bit-fields are implementation
dependent anyway.
This means that bit-fields are entirely non-portable.
--*--
The UEFI spec only says, in "Table 5. Common UEFI Data Types":
Bitfields are ordered such that bit 0 is the least significant bit.
So that removes only one variable (order of allocation of bit-fields).
Consider the following type:
typedef struct {
UINT16 Hello;
UINT16 Foo:15;
UINT32 Bar:27;
} FOOBAR;
Even considering the UEFI spec rules that structure members are
"naturally alined", plus the above order of allocation of bit-fields, we
still can't tell whether Bar starts at:
(a) bit 31,
(b) bit 32,
(c) bit 47,
(d) bit 64,
(e) bit 79.
- In case (a), the structure consists of the following units:
- UINT16 (Hello),
- UINT16 (Foo, plus one bit of Bar),
- UINT32 (all bits of Bar except the least significant one).
The implementation chooses to overlap adjacent units, for storing Bar.
Bar starts at bit 31.
- In case (b), the structure consists of the following units:
- UINT16 (Hello),
- UINT16 (Foo),
- UINT32 (Bar).
The imlementation chooses not to overlap adjacent units, for storing
Bar. Bar starts at bit 32.
- In case (c), the structure consists of the following units:
- UINT16 (Hello),
- UINT16 (padding),
- UINT32 (Foo, plus 17 low order bits of Bar),
- UINT32 (10 high order bits of Bar).
The padding is inserted because the implementation chooses UINT32 for
storing Foo, and said UINT32 has to be naturally aligned (per UEFI
spec). Furthermore, the implementation chooses to overlap adjacent
UINT32 units, for storing Bar. Bar starts at bit 47.
- In case (d), the structure consists of the following units:
- UINT16 (Hello),
- UINT16 (padding),
- UINT32 (Foo),
- UINT32 (Bar).
The padding is inserted because the implementation chooses UINT32 for
storing Foo, and said UINT32 has to be naturally aligned (per UEFI
spec). Furthermore, the implementation chooses not to overlap adjacent
UINT32 units, for storing Bar. Bar starts at bit 64.
- In case (e), the structure consists of the following units:
- UINT16 (Hello),
- UINT16[3] (padding),
- UINT64 (Foo and Bar).
The padding is inserted because the implementation chooses UINT64 for
storing Foo, and said UINT64 has to be naturally aligned (per UEFI
spec). And because 15+27=42, both Foo and Bar fit into UINT64, and so
the implementation must pack them both into that unit. Bar starts at
bit 79.
--*--
The edk2 C Coding Standards have a section on bit-fields:
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-4-bit-fields
Some part of that section are worth quoting:
> - Bit fields may only be of type INT32, signed INT32, UINT32, or a
> typedef name defined as one of the three INT32 variants.
Unfortunately, the edk2 codebase readily violates this; the following
"grep" confirms there are many UINT64 bit-fields:
$ git grep -E 'UINT64 *[A-Za-z0-9_]+ *: *[0-9]+ *;'
> - The order of allocation of bit-fields within a storage unit is
> compiler defined.
This conflicts with the UEFI spec, as the UEFI spec does define the
order of allocation.
> - A bit-field may not extend from one storage unit into another.
Overlapping adjacent units is up to the implementation, according to the
C standard.
So, is the programmer supposed to prevent that, by using unnamed ":0"
bitfields (per C99 6.7.2.1 p11)?
Then, further CCS sections:
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-4-1-visual-c-specific
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-4-2-gcc-specific
make toolchain / ABI specific notes. But I think ABI quirks and versions
are not what we should be thinking about when writing C source code. :(
I'm quite unhappy that bit-fields are so widely used in edk2. We should
always use unsigned integers and bitmask macros instead. That way at
least we're honest about the accesses. The bit-field types seem to work
mainly through sheer luck. (If there are ABI assumptions, they don't
seem to be clearly documented, anyway.)
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62236): https://edk2.groups.io/g/devel/message/62236
Mute This Topic: https://groups.io/mt/75378498/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
