Re: [PATCH v2 4/5] MdePkg/PciSegmentLib: Add instances that consumes PciSegmentInfoLib

[Andrew Fish <afish@apple.com>]

> On Aug 29, 2017, at 1:39 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 08/29/17 20:51, Ard Biesheuvel wrote:
>> Hi,
>> 
>> Some comments below.
>> 
>> On 25 August 2017 at 09:57, Ruiyu Ni <ruiyu.ni@intel.com> 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 <ruiyu.ni@intel.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> ---
>>> .../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
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>