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

[Laszlo Ersek <lersek@redhat.com>]

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":

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