On 08/29/17 20:51, Ard Biesheuvel wrote: > Hi, > > Some comments below. > > On 25 August 2017 at 09:57, Ruiyu Ni <[email protected]> wrote: >> The patch adds two PciSegmentLib instances that consumes >> PciSegmentInfoLib to provide multiple segments PCI configuration >> access. >> >> BasePciSegmentLibSegmentInfo instance is a BASE library. >> DxeRuntimePciSegmentLibSegmentInfo instance is to be linked with >> runtime drivers to provide not only boot time but also runtime >> PCI configuration access. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ruiyu Ni <[email protected]> >> Cc: Liming Gao <[email protected]> >> --- >> .../PciSegmentLibSegmentInfo/BasePciSegmentLib.c | 71 + >> .../BasePciSegmentLibSegmentInfo.inf | 46 + >> .../BasePciSegmentLibSegmentInfo.uni | 21 + >> .../DxeRuntimePciSegmentLib.c | 321 +++++ >> .../DxeRuntimePciSegmentLibSegmentInfo.inf | 55 + >> .../DxeRuntimePciSegmentLibSegmentInfo.uni | 21 + >> .../PciSegmentLibSegmentInfo/PciSegmentLibCommon.c | 1375 >> ++++++++++++++++++++ >> .../PciSegmentLibSegmentInfo/PciSegmentLibCommon.h | 57 + >> MdePkg/MdePkg.dsc | 2 + >> 9 files changed, 1969 insertions(+) >> create mode 100644 >> MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegmentLib.c >> create mode 100644 >> MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegmentLibSegmentInfo.inf >> create mode 100644 >> MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegmentLibSegmentInfo.uni >> create mode 100644 >> MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c >> create mode 100644 >> MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLibSegmentInfo.inf >> create mode 100644 >> MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLibSegmentInfo.uni >> create mode 100644 >> MdePkg/Library/PciSegmentLibSegmentInfo/PciSegmentLibCommon.c >> create mode 100644 >> MdePkg/Library/PciSegmentLibSegmentInfo/PciSegmentLibCommon.h >> > [...] >> diff --git a/MdePkg/Library/PciSegmentLibSegmentInfo/PciSegmentLibCommon.c >> b/MdePkg/Library/PciSegmentLibSegmentInfo/PciSegmentLibCommon.c >> new file mode 100644 >> index 0000000000..7b7324d673 >> --- /dev/null >> +++ b/MdePkg/Library/PciSegmentLibSegmentInfo/PciSegmentLibCommon.c >> @@ -0,0 +1,1375 @@ >> +/** @file >> + Provide common routines used by BasePciSegmentLibSegmentInfo and >> + DxeRuntimePciSegmentLibSegmentInfo libraries. >> + >> + Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> >> + This program and the accompanying materials are >> + licensed and made available under the terms and conditions of >> + the BSD License which accompanies this distribution. The full >> + text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php. >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >> IMPLIED. >> + >> +**/ >> + >> +#include "PciSegmentLibCommon.h" >> + >> +typedef struct { >> + UINT64 Register : 12; >> + UINT64 Function : 3; >> + UINT64 Device : 5; >> + UINT64 Bus : 8; >> + UINT64 Reserved1 : 4; >> + UINT64 Segment : 16; >> + UINT64 Reserved2 : 16; >> +} PCI_SEGMENT_LIB_ADDRESS_STRUCTURE; >> + > > Is this guaranteed to work as expected by the C spec?
>From C99, "6.7.2.1 Structure and union specifiers", paragraph 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." Due to the above, I consider bit-fields totally nonportable, and avoid introducing bit-fields in any code I write. However, "implementation-defined" means the compiler docs have to describe how bit-fields are laid out. If you know your toolchain (...all of your toolchains...), I guess you can make them work. FWIW, edk2 is chock-full of bit-fields. ... For example, the clang build options contain "-mms-bitfields": https://clang.llvm.org/docs/ClangCommandLineReference.html --> "Set the default structure layout to be compatible with the Microsoft compiler standard". The GCC docs are here: https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html --> "Determined by ABI." These structures make me shudder, but if they work, I just close my eyes and move on. :/ Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
