From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 97D8621E70D59 for ; Tue, 29 Aug 2017 13:37:02 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D5C9FC047B99; Tue, 29 Aug 2017 20:39:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D5C9FC047B99 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-166.phx2.redhat.com [10.3.116.166]) by smtp.corp.redhat.com (Postfix) with ESMTP id A5CDA6C419; Tue, 29 Aug 2017 20:39:41 +0000 (UTC) To: Ard Biesheuvel , Ruiyu Ni Cc: "edk2-devel@lists.01.org" , Liming Gao References: <20170825085723.396044-1-ruiyu.ni@intel.com> <20170825085723.396044-5-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <0a441b5d-3183-03b0-c5a4-fda92d7b01c1@redhat.com> Date: Tue, 29 Aug 2017 22:39:40 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 29 Aug 2017 20:39:43 +0000 (UTC) Subject: Re: [PATCH v2 4/5] MdePkg/PciSegmentLib: Add instances that consumes PciSegmentInfoLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Aug 2017 20:37:02 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/29/17 20:51, Ard Biesheuvel wrote: > Hi, > > Some comments below. > > On 25 August 2017 at 09:57, Ruiyu Ni 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 >> Cc: Liming Gao >> --- >> .../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.
>> + 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