Re: [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
> Add a library class, and a BASE lib instance, that are layered on top of
> PciCapLib, and allow clients to plug a PciSegmentLib backend into
> PciCapLib, for config space access.
>
> (Side note:
>

For the record: I am fine with keeping this side note in the commit log.

> The "MaxDomain" parameter is provided because, in practice, platforms
> exist where a PCI Express device may show up on a root bridge such that
> the root bridge doesn't support access to extended config space. Earlier
> the same issue was handled for MdeModulePkg/PciHostBridgeDxe in commit
> 014b472053ae3. However, that solution does not apply to the PciSegmentLib
> class, because:
>
> (1) The config space accessor functions of the PciSegmentLib class, such
>     as PciSegmentReadBuffer(), have no way of informing the caller whether
>     access to extended config space actually succeeds.
>
>     (For example, in the UefiPciSegmentLibPciRootBridgeIo instace, which
>     could in theory benefit from commit 014b472053ae3, the
>     EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read() status code is explicitly
>     ignored, because there's no way for the lib instance to propagate it
>     to the PciSegmentLib caller. If the
>     EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read() call fails, then
>     DxePciSegmentLibPciRootBridgeIoReadWorker() returns Data with
>     indeterminate value.)
>
> (2) There is no *general* way for any firmware platform to provide, or
>     use, a PciSegmentLib instance in which access to extended config space
>     always succeeds.
>
> In brief, on a platform where config space may be limited to 256 bytes,
> access to extended config space through PciSegmentLib may invoke undefined
> behavior; therefore PciCapPciSegmentLib must give platforms a way to
> prevent such access.)
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     v2:
>     - move library from MdePkg to OvmfPkg, for initial introduction
>
>  OvmfPkg/OvmfPkg.dec                                                 |   5 +
>  OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf |  35 +++
>  OvmfPkg/Include/Library/PciCapPciSegmentLib.h                       |  82 +++++++
>  OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h   |  47 ++++
>  OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c   | 226 ++++++++++++++++++++
>  5 files changed, 395 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 74818a2e2a19..fbec1cfe4a8e 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -35,6 +35,11 @@ [LibraryClasses]
>    #                  config space.
>    PciCapLib|Include/Library/PciCapLib.h
>
> +  ##  @libraryclass  Layered on top of PciCapLib, allows clients to plug a
> +  #                  PciSegmentLib backend into PciCapLib, for config space
> +  #                  access.
> +  PciCapPciSegmentLib|Include/Library/PciCapPciSegmentLib.h
> +
>    ##  @libraryclass  Access QEMU's firmware configuration interface
>    #
>    QemuFwCfgLib|Include/Library/QemuFwCfgLib.h
> diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> new file mode 100644
> index 000000000000..e3cf5de49b15
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> @@ -0,0 +1,35 @@
> +## @file
> +# Plug a PciSegmentLib backend into PciCapLib, for config space access.
> +#
> +# Copyright (C) 2018, Red Hat, Inc.
> +#
> +# 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.
> +##
> +
> +[Defines]
> +  INF_VERSION    = 1.27
> +  BASE_NAME      = BasePciCapPciSegmentLib
> +  FILE_GUID      = ED011855-AA31-43B9-ACC0-BF45A05C5985
> +  MODULE_TYPE    = BASE
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = PciCapPciSegmentLib
> +
> +[Sources]
> +  BasePciCapPciSegmentLib.h
> +  BasePciCapPciSegmentLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  MemoryAllocationLib
> +  PciSegmentLib
> diff --git a/OvmfPkg/Include/Library/PciCapPciSegmentLib.h b/OvmfPkg/Include/Library/PciCapPciSegmentLib.h
> new file mode 100644
> index 000000000000..6b6930288d16
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/PciCapPciSegmentLib.h
> @@ -0,0 +1,82 @@
> +/** @file
> +  Library class layered on top of PciCapLib that allows clients to plug a
> +  PciSegmentLib backend into PciCapLib, for config space access.
> +
> +  Copyright (C) 2018, Red Hat, Inc.
> +
> +  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.
> +**/
> +
> +#ifndef __PCI_CAP_PCI_SEGMENT_LIB_H__
> +#define __PCI_CAP_PCI_SEGMENT_LIB_H__
> +
> +#include <Library/PciCapLib.h>
> +
> +
> +/**
> +  Create a PCI_CAP_DEV object from the PCI Segment:Bus:Device.Function
> +  quadruplet. The config space accessors are based upon PciSegmentLib.
> +
> +  @param[in] MaxDomain   If MaxDomain is PciCapExtended, then
> +                         PciDevice->ReadConfig() and PciDevice->WriteConfig()
> +                         will delegate extended config space accesses too to
> +                         PciSegmentReadBuffer() and PciSegmentWriteBuffer(),
> +                         respectively. Otherwise, PciDevice->ReadConfig() and
> +                         PciDevice->WriteConfig() will reject accesses to
> +                         extended config space with RETURN_UNSUPPORTED, without
> +                         calling PciSegmentReadBuffer() or
> +                         PciSegmentWriteBuffer(). By setting MaxDomain to
> +                         PciCapNormal, the platform can prevent undefined
> +                         PciSegmentLib behavior when the PCI root bridge under
> +                         the PCI device at Segment:Bus:Device.Function doesn't
> +                         support extended config space.
> +
> +  @param[in] Segment     16-bit wide segment number.
> +
> +  @param[in] Bus         8-bit wide bus number.
> +
> +  @param[in] Device      5-bit wide device number.
> +
> +  @param[in] Function    3-bit wide function number.
> +
> +  @param[out] PciDevice  The PCI_CAP_DEV object constructed as described above.
> +                         PciDevice can be passed to the PciCapLib APIs.
> +
> +  @retval RETURN_SUCCESS            PciDevice has been constructed and output.
> +
> +  @retval RETURN_INVALID_PARAMETER  Device or Function does not fit in the
> +                                    permitted number of bits.
> +
> +  @retval RETURN_OUT_OF_RESOURCES   Memory allocation failed.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapPciSegmentDeviceInit (
> +  IN  PCI_CAP_DOMAIN MaxDomain,
> +  IN  UINT16         Segment,
> +  IN  UINT8          Bus,
> +  IN  UINT8          Device,
> +  IN  UINT8          Function,
> +  OUT PCI_CAP_DEV    **PciDevice
> +  );
> +
> +
> +/**
> +  Free the resources used by PciDevice.
> +
> +  @param[in] PciDevice  The PCI_CAP_DEV object to free, originally produced by
> +                        PciCapPciSegmentDeviceInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapPciSegmentDeviceUninit (
> +  IN PCI_CAP_DEV *PciDevice
> +  );
> +
> +#endif // __PCI_CAP_PCI_SEGMENT_LIB_H__
> diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
> new file mode 100644
> index 000000000000..3ce15fe0fb57
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
> @@ -0,0 +1,47 @@
> +/** @file
> +  Plug a PciSegmentLib backend into PciCapLib, for config space access --
> +  internal macro and type definitions.
> +
> +  Copyright (C) 2018, Red Hat, Inc.
> +
> +  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.
> +**/
> +
> +#ifndef __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
> +#define __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
> +
> +#include <Library/DebugLib.h>
> +
> +#include <Library/PciCapPciSegmentLib.h>
> +
> +#define SEGMENT_DEV_SIG SIGNATURE_64 ('P', 'C', 'P', 'S', 'G', 'M', 'N', 'T')
> +
> +typedef struct {
> +  //
> +  // Signature identifying the derived class.
> +  //
> +  UINT64 Signature;
> +  //
> +  // Members added by the derived class, specific to the use of PciSegmentLib.
> +  //
> +  PCI_CAP_DOMAIN MaxDomain;
> +  UINT16         SegmentNr;
> +  UINT8          BusNr;
> +  UINT8          DeviceNr;
> +  UINT8          FunctionNr;
> +  //
> +  // Base class.
> +  //
> +  PCI_CAP_DEV BaseDevice;
> +} SEGMENT_DEV;
> +
> +#define SEGMENT_DEV_FROM_PCI_CAP_DEV(PciDevice) \
> +  CR (PciDevice, SEGMENT_DEV, BaseDevice, SEGMENT_DEV_SIG)
> +
> +#endif // __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
> diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
> new file mode 100644
> index 000000000000..57eb6b625b56
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
> @@ -0,0 +1,226 @@
> +/** @file
> +  Plug a PciSegmentLib backend into PciCapLib, for config space access.
> +
> +  Copyright (C) 2018, Red Hat, Inc.
> +
> +  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 <IndustryStandard/Pci23.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PciSegmentLib.h>
> +
> +#include "BasePciCapPciSegmentLib.h"
> +
> +
> +/**
> +  Read the config space of a given PCI device (both normal and extended).
> +
> +  SegmentDevReadConfig() performs as few config space accesses as possible
> +  (without attempting 64-bit wide accesses).
> +
> +  @param[in] PciDevice           Implementation-specific unique representation
> +                                 of the PCI device in the PCI hierarchy.
> +
> +  @param[in] SourceOffset        Source offset in the config space of the PCI
> +                                 device to start reading from.
> +
> +  @param[out] DestinationBuffer  Buffer to store the read data to.
> +
> +  @param[in] Size                The number of bytes to transfer.
> +
> +  @retval RETURN_SUCCESS      Size bytes have been transferred from config
> +                              space to DestinationBuffer.
> +
> +  @retval RETURN_UNSUPPORTED  Accessing Size bytes from SourceOffset exceeds
> +                              the config space limit of the PCI device.
> +                              Although PCI_CAP_DEV_READ_CONFIG allows reading
> +                              fewer than Size bytes in this case,
> +                              SegmentDevReadConfig() will read none.
> +**/
> +STATIC
> +RETURN_STATUS
> +EFIAPI
> +SegmentDevReadConfig (
> +  IN  PCI_CAP_DEV *PciDevice,
> +  IN  UINT16      SourceOffset,
> +  OUT VOID        *DestinationBuffer,
> +  IN  UINT16      Size
> +  )
> +{
> +  SEGMENT_DEV *SegmentDev;
> +  UINT16      ConfigSpaceSize;
> +  UINT64      SourceAddress;
> +
> +  SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
> +  ConfigSpaceSize = (SegmentDev->MaxDomain == PciCapNormal ?
> +                     PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET);
> +  //
> +  // Note that all UINT16 variables below are promoted to INT32, and the
> +  // addition and the comparison is carried out in INT32.
> +  //
> +  if (SourceOffset + Size > ConfigSpaceSize) {
> +    return RETURN_UNSUPPORTED;
> +  }
> +  SourceAddress = PCI_SEGMENT_LIB_ADDRESS (SegmentDev->SegmentNr,
> +                    SegmentDev->BusNr, SegmentDev->DeviceNr,
> +                    SegmentDev->FunctionNr, SourceOffset);
> +  PciSegmentReadBuffer (SourceAddress, Size, DestinationBuffer);
> +  return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> +  Write the config space of a given PCI device (both normal and extended).
> +
> +  SegmentDevWriteConfig() performs as few config space accesses as possible
> +  (without attempting 64-bit wide accesses).
> +
> +  @param[in] PciDevice          Implementation-specific unique representation
> +                                of the PCI device in the PCI hierarchy.
> +
> +  @param[in] DestinationOffset  Destination offset in the config space of the
> +                                PCI device to start writing at.
> +
> +  @param[in] SourceBuffer       Buffer to read the data to be stored from.
> +
> +  @param[in] Size               The number of bytes to transfer.
> +
> +  @retval RETURN_SUCCESS      Size bytes have been transferred from
> +                              SourceBuffer to config space.
> +
> +  @retval RETURN_UNSUPPORTED  Accessing Size bytes at DestinationOffset exceeds
> +                              the config space limit of the PCI device.
> +                              Although PCI_CAP_DEV_WRITE_CONFIG allows writing
> +                              fewer than Size bytes in this case,
> +                              SegmentDevWriteConfig() will write none.
> +**/
> +STATIC
> +RETURN_STATUS
> +EFIAPI
> +SegmentDevWriteConfig (
> +  IN PCI_CAP_DEV *PciDevice,
> +  IN UINT16      DestinationOffset,
> +  IN VOID        *SourceBuffer,
> +  IN UINT16      Size
> +  )
> +{
> +  SEGMENT_DEV *SegmentDev;
> +  UINT16      ConfigSpaceSize;
> +  UINT64      DestinationAddress;
> +
> +  SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
> +  ConfigSpaceSize = (SegmentDev->MaxDomain == PciCapNormal ?
> +                     PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET);
> +  //
> +  // Note that all UINT16 variables below are promoted to INT32, and the
> +  // addition and the comparison is carried out in INT32.
> +  //
> +  if (DestinationOffset + Size > ConfigSpaceSize) {
> +    return RETURN_UNSUPPORTED;
> +  }
> +  DestinationAddress = PCI_SEGMENT_LIB_ADDRESS (SegmentDev->SegmentNr,
> +                         SegmentDev->BusNr, SegmentDev->DeviceNr,
> +                         SegmentDev->FunctionNr, DestinationOffset);
> +  PciSegmentWriteBuffer (DestinationAddress, Size, SourceBuffer);
> +  return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> +  Create a PCI_CAP_DEV object from the PCI Segment:Bus:Device.Function
> +  quadruplet. The config space accessors are based upon PciSegmentLib.
> +
> +  @param[in] MaxDomain   If MaxDomain is PciCapExtended, then
> +                         PciDevice->ReadConfig() and PciDevice->WriteConfig()
> +                         will delegate extended config space accesses too to
> +                         PciSegmentReadBuffer() and PciSegmentWriteBuffer(),
> +                         respectively. Otherwise, PciDevice->ReadConfig() and
> +                         PciDevice->WriteConfig() will reject accesses to
> +                         extended config space with RETURN_UNSUPPORTED, without
> +                         calling PciSegmentReadBuffer() or
> +                         PciSegmentWriteBuffer(). By setting MaxDomain to
> +                         PciCapNormal, the platform can prevent undefined
> +                         PciSegmentLib behavior when the PCI root bridge under
> +                         the PCI device at Segment:Bus:Device.Function doesn't
> +                         support extended config space.
> +
> +  @param[in] Segment     16-bit wide segment number.
> +
> +  @param[in] Bus         8-bit wide bus number.
> +
> +  @param[in] Device      5-bit wide device number.
> +
> +  @param[in] Function    3-bit wide function number.
> +
> +  @param[out] PciDevice  The PCI_CAP_DEV object constructed as described above.
> +                         PciDevice can be passed to the PciCapLib APIs.
> +
> +  @retval RETURN_SUCCESS            PciDevice has been constructed and output.
> +
> +  @retval RETURN_INVALID_PARAMETER  Device or Function does not fit in the
> +                                    permitted number of bits.
> +
> +  @retval RETURN_OUT_OF_RESOURCES   Memory allocation failed.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapPciSegmentDeviceInit (
> +  IN  PCI_CAP_DOMAIN MaxDomain,
> +  IN  UINT16         Segment,
> +  IN  UINT8          Bus,
> +  IN  UINT8          Device,
> +  IN  UINT8          Function,
> +  OUT PCI_CAP_DEV    **PciDevice
> +  )
> +{
> +  SEGMENT_DEV *SegmentDev;
> +
> +  if (Device > PCI_MAX_DEVICE || Function > PCI_MAX_FUNC) {
> +    return RETURN_INVALID_PARAMETER;
> +  }
> +
> +  SegmentDev = AllocatePool (sizeof *SegmentDev);
> +  if (SegmentDev == NULL) {
> +    return RETURN_OUT_OF_RESOURCES;
> +  }
> +
> +  SegmentDev->Signature              = SEGMENT_DEV_SIG;
> +  SegmentDev->MaxDomain              = MaxDomain;
> +  SegmentDev->SegmentNr              = Segment;
> +  SegmentDev->BusNr                  = Bus;
> +  SegmentDev->DeviceNr               = Device;
> +  SegmentDev->FunctionNr             = Function;
> +  SegmentDev->BaseDevice.ReadConfig  = SegmentDevReadConfig;
> +  SegmentDev->BaseDevice.WriteConfig = SegmentDevWriteConfig;
> +
> +  *PciDevice = &SegmentDev->BaseDevice;
> +  return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> +  Free the resources used by PciDevice.
> +
> +  @param[in] PciDevice  The PCI_CAP_DEV object to free, originally produced by
> +                        PciCapPciSegmentDeviceInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapPciSegmentDeviceUninit (
> +  IN PCI_CAP_DEV *PciDevice
> +  )
> +{
> +  SEGMENT_DEV *SegmentDev;
> +
> +  SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
> +  FreePool (SegmentDev);
> +}
> --
> 2.14.1.3.gb7cf6e02401b
>
>