> On Aug 29, 2017, at 1:39 PM, Laszlo Ersek <[email protected]> wrote: > > 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": >
Laszlo, FYI -mms-bitfields was to force EFI ABI compatibility, not to enabled bit fields per say. I can't remember why we used ms-bitfields vs. ms-struct? Thanks, Andrew Fish > https://clang.llvm.org/docs/ClangCommandLineReference.html > <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 > > <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] <mailto:[email protected]> > https://lists.01.org/mailman/listinfo/edk2-devel > <https://lists.01.org/mailman/listinfo/edk2-devel> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
