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] -=-=-=-=-=-=-=-=-=-=-=-