Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib

[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 UEFI_DRIVER lib instance, that are layered on
> top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend
> into PciCapLib, for config space access.
>
> (Side note:
>

Again, please retain the below.

> Although the UEFI spec says that EFI_PCI_IO_PROTOCOL_CONFIG() returns
> EFI_UNSUPPORTED if "[t]he address range specified by Offset, Width, and
> Count is not valid for the PCI configuration header of the PCI
> controller", this patch doesn't directly document the EFI_UNSUPPORTED
> error code, for ProtoDevTransferConfig() and its callers
> ProtoDevReadConfig() and ProtoDevWriteConfig(). Instead, the patch refers
> to "unspecified error codes". The reason is that in edk2, the
> PciIoConfigRead() and PciIoConfigWrite() functions [1] can also return
> EFI_INVALID_PARAMETER for the above situation.
>
> Namely, PciIoConfigRead() and PciIoConfigWrite() first call
> PciIoVerifyConfigAccess(), which indeed produces the standard
> EFI_UNSUPPORTED error code, if the device's config space is exceeded.
> However, if PciIoVerifyConfigAccess() passes, and we reach
> RootBridgeIoPciRead() and RootBridgeIoPciWrite() [2], then
> RootBridgeIoCheckParameter() can still fail, e.g. if the root bridge
> doesn't support extended config space (see commit 014b472053ae3).
>
> For all kinds of Limit violations in IO, MMIO, and config space,
> RootBridgeIoCheckParameter() returns EFI_INVALID_PARAMETER, not
> EFI_UNSUPPORTED. That error code is then propagated up to, and out of,
> PciIoConfigRead() and PciIoConfigWrite().
>
> [1] MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> [2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> )
>
> 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/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf |  36 +++
>  OvmfPkg/Include/Library/PciCapPciIoLib.h                  |  58 +++++
>  OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h   |  44 ++++
>  OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c   | 243 ++++++++++++++++++++
>  5 files changed, 386 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index fbec1cfe4a8e..dc5597db4136 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 an
> +  #                  EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config
> +  #                  space access.
> +  PciCapPciIoLib|Include/Library/PciCapPciIoLib.h
> +
>    ##  @libraryclass  Layered on top of PciCapLib, allows clients to plug a
>    #                  PciSegmentLib backend into PciCapLib, for config space
>    #                  access.
> diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> new file mode 100644
> index 000000000000..2e14acb0ab75
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> @@ -0,0 +1,36 @@
> +## @file
> +# Plug an EFI_PCI_IO_PROTOCOL 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      = UefiPciCapPciIoLib
> +  FILE_GUID      = 4102F4FE-DA10-4F0F-AC18-4982ED506154
> +  MODULE_TYPE    = UEFI_DRIVER
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = PciCapPciIoLib
> +
> +[Sources]
> +  UefiPciCapPciIoLib.h
> +  UefiPciCapPciIoLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  MemoryAllocationLib
> +
> +[Protocols]
> +  gEfiPciIoProtocolGuid ## CONSUMES
> diff --git a/OvmfPkg/Include/Library/PciCapPciIoLib.h b/OvmfPkg/Include/Library/PciCapPciIoLib.h
> new file mode 100644
> index 000000000000..553715fd5080
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/PciCapPciIoLib.h
> @@ -0,0 +1,58 @@
> +/** @file
> +  Library class layered on top of PciCapLib that allows clients to plug an
> +  EFI_PCI_IO_PROTOCOL 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_IO_LIB_H__
> +#define __PCI_CAP_PCI_IO_LIB_H__
> +
> +#include <Protocol/PciIo.h>
> +
> +#include <Library/PciCapLib.h>
> +
> +
> +/**
> +  Create a PCI_CAP_DEV object from an EFI_PCI_IO_PROTOCOL instance. The config
> +  space accessors are based upon EFI_PCI_IO_PROTOCOL.Pci.Read() and
> +  EFI_PCI_IO_PROTOCOL.Pci.Write().
> +
> +  @param[in] PciIo       EFI_PCI_IO_PROTOCOL representation of the PCI device.
> +
> +  @param[out] PciDevice  The PCI_CAP_DEV object constructed as described above.
> +                         PciDevice can be passed to the PciCapLib APIs.
> +
> +  @retval EFI_SUCCESS           PciDevice has been constructed and output.
> +
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation failed.
> +**/
> +EFI_STATUS
> +EFIAPI
> +PciCapPciIoDeviceInit (
> +  IN  EFI_PCI_IO_PROTOCOL *PciIo,
> +  OUT PCI_CAP_DEV         **PciDevice
> +  );
> +
> +
> +/**
> +  Free the resources used by PciDevice.
> +
> +  @param[in] PciDevice  The PCI_CAP_DEV object to free, originally produced by
> +                        PciCapPciIoDeviceInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapPciIoDeviceUninit (
> +  IN PCI_CAP_DEV *PciDevice
> +  );
> +
> +#endif // __PCI_CAP_PCI_IO_LIB_H__
> diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
> new file mode 100644
> index 000000000000..a94f65e5a4e3
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
> @@ -0,0 +1,44 @@
> +/** @file
> +  Plug an EFI_PCI_IO_PROTOCOL 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 __UEFI_PCI_CAP_PCI_IO_LIB_H__
> +#define __UEFI_PCI_CAP_PCI_IO_LIB_H__
> +
> +#include <Library/DebugLib.h>
> +
> +#include <Library/PciCapPciIoLib.h>
> +
> +#define PROTO_DEV_SIG SIGNATURE_64 ('P', 'C', 'P', 'I', 'O', 'P', 'R', 'T')
> +
> +typedef struct {
> +  //
> +  // Signature identifying the derived class.
> +  //
> +  UINT64 Signature;
> +  //
> +  // Members added by the derived class, specific to the use of
> +  // EFI_PCI_IO_PROTOCOL.
> +  //
> +  EFI_PCI_IO_PROTOCOL *PciIo;
> +  //
> +  // Base class.
> +  //
> +  PCI_CAP_DEV BaseDevice;
> +} PROTO_DEV;
> +
> +#define PROTO_DEV_FROM_PCI_CAP_DEV(PciDevice) \
> +  CR (PciDevice, PROTO_DEV, BaseDevice, PROTO_DEV_SIG)
> +
> +#endif // __UEFI_PCI_CAP_PCI_IO_LIB_H__
> diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> new file mode 100644
> index 000000000000..84369e4dc3a8
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> @@ -0,0 +1,243 @@
> +/** @file
> +  Plug an EFI_PCI_IO_PROTOCOL 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 <Library/MemoryAllocationLib.h>
> +
> +#include "UefiPciCapPciIoLib.h"
> +
> +
> +/**
> +  Transfer bytes between the config space of a given PCI device and a memory
> +  buffer.
> +
> +  ProtoDevTransferConfig() performs as few config space accesses as possible
> +  (without attempting 64-bit wide accesses).
> +
> +  @param[in] PciIo             The EFI_PCI_IO_PROTOCOL representation of the
> +                               PCI device.
> +
> +  @param[in] TransferFunction  The EFI_PCI_IO_PROTOCOL_CONFIG function that
> +                               implements the transfer. The direction of the
> +                               transfer is inherent to TransferFunction.
> +                               TransferFunction() is required to return an
> +                               unspecified error if any sub-transfer within
> +                               Size bytes from ConfigOffset exceeds the config
> +                               space limit of the PCI device.
> +
> +  @param[in] ConfigOffset      The offset in the config space of the PCI device
> +                               at which the transfer should commence.
> +
> +  @param[in,out] Buffer        The memory buffer where the transfer should
> +                               occur.
> +
> +  @param[in] Size              The number of bytes to transfer.
> +
> +  @retval EFI_SUCCESS  Size bytes have been transferred between config space
> +                       and Buffer.
> +
> +  @return              Error codes propagated from TransferFunction(). Fewer
> +                       than Size bytes may have been transferred.
> +**/
> +STATIC
> +EFI_STATUS
> +ProtoDevTransferConfig (
> +  IN     EFI_PCI_IO_PROTOCOL        *PciIo,
> +  IN     EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction,
> +  IN     UINT16                     ConfigOffset,
> +  IN OUT UINT8                      *Buffer,
> +  IN     UINT16                     Size
> +  )
> +{
> +  while (Size > 0) {
> +    EFI_PCI_IO_PROTOCOL_WIDTH Width;
> +    UINT16                    Count;
> +    EFI_STATUS                Status;
> +    UINT16                    Progress;
> +
> +    //
> +    // Pick the largest access size that is allowed by the remaining transfer
> +    // Size and by the alignment of ConfigOffset.
> +    //
> +    // When the largest access size is available, transfer as many bytes as
> +    // possible in one iteration of the loop. Otherwise, transfer only one
> +    // unit, to improve the alignment.
> +    //
> +    if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {

Ugh. Just use '4' or sizeof(UINT32).

> +      Width = EfiPciIoWidthUint32;
> +      Count = Size >> Width;
> +    } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
> +      Width = EfiPciIoWidthUint16;
> +      Count = 1;
> +    } else {
> +      Width = EfiPciIoWidthUint8;
> +      Count = 1;
> +    }
> +    Status = TransferFunction (PciIo, Width, ConfigOffset, Count, Buffer);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    Progress      = Count << Width;
> +    ConfigOffset += Progress;
> +    Buffer       += Progress;
> +    Size         -= Progress;
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Read the config space of a given PCI device (both normal and extended).
> +
> +  ProtoDevReadConfig() performs as few config space accesses as possible
> +  (without attempting 64-bit wide accesses).
> +
> +  ProtoDevReadConfig() returns an unspecified error if accessing Size bytes
> +  from SourceOffset exceeds the config space limit of the PCI device. Fewer
> +  than Size bytes may have been read in this case.
> +
> +  @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.
> +
> +  @return                 Error codes propagated from
> +                          EFI_PCI_IO_PROTOCOL.Pci.Read(). Fewer than Size bytes
> +                          may have been read.
> +**/
> +STATIC
> +RETURN_STATUS
> +EFIAPI
> +ProtoDevReadConfig (
> +  IN  PCI_CAP_DEV *PciDevice,
> +  IN  UINT16      SourceOffset,
> +  OUT VOID        *DestinationBuffer,
> +  IN  UINT16      Size
> +  )
> +{
> +  PROTO_DEV *ProtoDev;
> +
> +  ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
> +  return ProtoDevTransferConfig (ProtoDev->PciIo, ProtoDev->PciIo->Pci.Read,
> +           SourceOffset, DestinationBuffer, Size);
> +}
> +
> +
> +/**
> +  Write the config space of a given PCI device (both normal and extended).
> +
> +  ProtoDevWriteConfig() performs as few config space accesses as possible
> +  (without attempting 64-bit wide accesses).
> +
> +  ProtoDevWriteConfig() returns an unspecified error if accessing Size bytes at
> +  DestinationOffset exceeds the config space limit of the PCI device. Fewer
> +  than Size bytes may have been written in this case.
> +
> +  @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.
> +
> +  @return                 Error codes propagated from
> +                          EFI_PCI_IO_PROTOCOL.Pci.Write(). Fewer than Size
> +                          bytes may have been written.
> +**/
> +STATIC
> +RETURN_STATUS
> +EFIAPI
> +ProtoDevWriteConfig (
> +  IN PCI_CAP_DEV *PciDevice,
> +  IN UINT16      DestinationOffset,
> +  IN VOID        *SourceBuffer,
> +  IN UINT16      Size
> +  )
> +{
> +  PROTO_DEV *ProtoDev;
> +
> +  ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
> +  return ProtoDevTransferConfig (ProtoDev->PciIo, ProtoDev->PciIo->Pci.Write,
> +           DestinationOffset, SourceBuffer, Size);
> +}
> +
> +
> +/**
> +  Create a PCI_CAP_DEV object from an EFI_PCI_IO_PROTOCOL instance. The config
> +  space accessors are based upon EFI_PCI_IO_PROTOCOL.Pci.Read() and
> +  EFI_PCI_IO_PROTOCOL.Pci.Write().
> +
> +  @param[in] PciIo       EFI_PCI_IO_PROTOCOL representation of the PCI device.
> +
> +  @param[out] PciDevice  The PCI_CAP_DEV object constructed as described above.
> +                         PciDevice can be passed to the PciCapLib APIs.
> +
> +  @retval EFI_SUCCESS           PciDevice has been constructed and output.
> +
> +  @retval EFI_OUT_OF_RESOURCES  Memory allocation failed.
> +**/
> +EFI_STATUS
> +EFIAPI
> +PciCapPciIoDeviceInit (
> +  IN  EFI_PCI_IO_PROTOCOL *PciIo,
> +  OUT PCI_CAP_DEV         **PciDevice
> +  )
> +{
> +  PROTO_DEV *ProtoDev;
> +
> +  ProtoDev = AllocatePool (sizeof *ProtoDev);
> +  if (ProtoDev == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  ProtoDev->Signature              = PROTO_DEV_SIG;
> +  ProtoDev->PciIo                  = PciIo;
> +  ProtoDev->BaseDevice.ReadConfig  = ProtoDevReadConfig;
> +  ProtoDev->BaseDevice.WriteConfig = ProtoDevWriteConfig;
> +
> +  *PciDevice = &ProtoDev->BaseDevice;
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Free the resources used by PciDevice.
> +
> +  @param[in] PciDevice  The PCI_CAP_DEV object to free, originally produced by
> +                        PciCapPciIoDeviceInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapPciIoDeviceUninit (
> +  IN PCI_CAP_DEV *PciDevice
> +  )
> +{
> +  PROTO_DEV *ProtoDev;
> +
> +  ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
> +  FreePool (ProtoDev);
> +}
> --
> 2.14.1.3.gb7cf6e02401b
>
>