[edk2] [PATCH 7/7] OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation hints

Laszlo Ersek posted 7 patches 7 years, 3 months ago
[edk2] [PATCH 7/7] OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation hints
Posted by Laszlo Ersek 7 years, 3 months ago
Parse QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION from the bridges'
conventional config spaces. Translate the fields as follows:

* BusNumbers:
  * 0 -- no reservation;
  * (-1) -- firmware default, i.e. no reservation;
  * otherwise -- reserve the requested value. (NB, bus number reservation
    is not supposed to work before
    <https://bugzilla.tianocore.org/show_bug.cgi?id=656> is fixed.)

* Io:
  * 0 -- no reservation;
  * (-1) -- keep our current default (512B);
  * otherwise -- round up the requested value and reserve that.

* NonPrefetchable32BitMmio:
  * 0 -- no reservation;
  * (-1) -- keep our current default (2MB);
  * otherwise -- round up the requested value and reserve that.

* Prefetchable32BitMmio:
  * 0 -- no reservation, proceed to Prefetchable64BitMmio;
  * (-1) -- firmware default, i.e. no reservation, proceed to
    Prefetchable64BitMmio;
  * otherwise -- round up the requested value and reserve that. (NB, if
    Prefetchable32BitMmio is reserved in addition to
    NonPrefetchable32BitMmio, then PciBusDxe currently runs into an
    assertion failure. Refer to
    <https://bugzilla.tianocore.org/show_bug.cgi?id=720>.)

* Prefetchable64BitMmio:
  * only reached if Prefetchable32BitMmio was not reserved;
  * 0 -- no reservation;
  * (-1) -- firmware default, i.e. no reservation;
  * otherwise -- round up the requested value and reserve that.

If QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION is missing, plus any
time the rounding fails, fall back to the current defaults.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf |   2 +
 OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c   | 371 +++++++++++++++++++-
 2 files changed, 367 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
index e0ec9baae1c2..38043986eb67 100644
--- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
+++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
@@ -26,15 +26,17 @@ [Sources]
 
 [Packages]
   MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
   DebugLib
   DevicePathLib
   MemoryAllocationLib
+  PciLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
 
 [Protocols]
diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
index 39646973794b..177e1a62120d 100644
--- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
+++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
@@ -13,14 +13,16 @@
   WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 **/
 
 #include <IndustryStandard/Acpi10.h>
+#include <IndustryStandard/QemuPciBridgeCapabilities.h>
 
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/PciLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
 #include <Protocol/PciHotPlugInit.h>
 #include <Protocol/PciRootBridgeIo.h>
@@ -245,8 +247,239 @@ HighBitSetRoundUp64 (
   return (HighBit < 64) ? HighBit : -1;
 }
 
 
+/**
+  Read a slice from conventional PCI config space at the given offset, then
+  advance the offset.
+
+  @param[in] PciAddress  The address of the PCI Device -- Bus, Device, Function
+                         -- in UEFI (not PciLib) encoding.
+
+  @param[in,out] Offset  On input, the offset in conventional PCI config space
+                         to start reading from. On output, the offset of the
+                         first byte that was not read.
+
+  @param[in] Size        The number of bytes to read.
+
+  @param[out] Buffer     On output, the bytes read from PCI config space are
+                         stored in this object.
+**/
+STATIC
+VOID
+ReadConfigSpace (
+  IN     CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *PciAddress,
+  IN OUT UINT8                                             *Offset,
+  IN     UINT8                                             Size,
+  OUT    VOID                                              *Buffer
+  )
+{
+  PciReadBuffer (
+    PCI_LIB_ADDRESS (
+      PciAddress->Bus,
+      PciAddress->Device,
+      PciAddress->Function,
+      *Offset
+      ),
+    Size,
+    Buffer
+    );
+  *Offset += Size;
+}
+
+
+/**
+  Convenience wrapper macro for ReadConfigSpace().
+
+  Given the following conditions:
+
+  - HeaderField is the first field in the structure pointed-to by Struct,
+
+  - Struct->HeaderField has been populated from the conventional PCI config
+    space of the PCI device identified by PciAddress,
+
+  - *Offset points one past HeaderField in the conventional PCI config space of
+    the PCI device identified by PciAddress,
+
+  populate the rest of *Struct from conventional PCI config space, starting at
+  *Offset. Finally, increment *Offset so that it point one past *Struct.
+
+  @param[in] PciAddress  The address of the PCI Device -- Bus, Device, Function
+                         -- in UEFI (not PciLib) encoding. Type: pointer to
+                         CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS.
+
+  @param[in,out] Offset  On input, the offset in conventional PCI config space
+                         to start reading from; one past Struct->HeaderField.
+                         On output, the offset of the first byte that was not
+                         read; one past *Struct. Type: pointer to UINT8.
+
+  @param[out] Struct     The structure to complete. Type: pointer to structure
+                         object.
+
+  @param[in] HeaderField The name of the first field in *Struct, after which
+                         *Struct should be populated. Type: structure member
+                         identifier.
+**/
+#define COMPLETE_CONFIG_SPACE_STRUCT(PciAddress, Offset, Struct, HeaderField) \
+          ReadConfigSpace (                                                   \
+            (PciAddress),                                                     \
+            (Offset),                                                         \
+            (UINT8)(sizeof *(Struct) - sizeof ((Struct)->HeaderField)),       \
+            &((Struct)->HeaderField) + 1                                      \
+            )
+
+
+/**
+  Look up the QEMU-specific Resource Reservation capability in the conventional
+  config space of a Hotplug Controller (that is, PCI Bridge).
+
+  This function performs as few config space reads as possible.
+
+  @param[in] HpcPciAddress     The address of the PCI Bridge -- Bus, Device,
+                               Function -- in UEFI (not PciLib) encoding.
+
+  @param[out] ReservationHint  The caller-allocated capability structure to
+                               populate from the PCI Bridge's config space.
+
+  @retval EFI_SUCCESS    The capability has been found, ReservationHint has
+                         been populated.
+
+  @retval EFI_NOT_FOUND  The capability is missing. The contents of
+                         ReservationHint are now indeterminate.
+**/
+STATIC
+EFI_STATUS
+QueryReservationHint (
+  IN  CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *HpcPciAddress,
+  OUT QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION   *ReservationHint
+)
+{
+  UINT16 PciVendorId;
+  UINT16 PciStatus;
+  UINT8  PciCapPtr;
+  UINT8  Offset;
+
+  //
+  // Check the vendor identifier.
+  //
+  PciVendorId = PciRead16 (
+                  PCI_LIB_ADDRESS (
+                    HpcPciAddress->Bus,
+                    HpcPciAddress->Device,
+                    HpcPciAddress->Function,
+                    PCI_VENDOR_ID_OFFSET
+                    )
+                  );
+  if (PciVendorId != QEMU_PCI_BRIDGE_VENDOR_ID_REDHAT) {
+    return EFI_NOT_FOUND;
+  }
+
+  //
+  // Check the Capabilities List bit in the PCI Status Register.
+  //
+  PciStatus = PciRead16 (
+                PCI_LIB_ADDRESS (
+                  HpcPciAddress->Bus,
+                  HpcPciAddress->Device,
+                  HpcPciAddress->Function,
+                  PCI_PRIMARY_STATUS_OFFSET
+                  )
+                );
+  if ((PciStatus & EFI_PCI_STATUS_CAPABILITY) == 0) {
+    return EFI_NOT_FOUND;
+  }
+
+  //
+  // Fetch the start of the Capabilities List.
+  //
+  PciCapPtr = PciRead8 (
+                PCI_LIB_ADDRESS (
+                  HpcPciAddress->Bus,
+                  HpcPciAddress->Device,
+                  HpcPciAddress->Function,
+                  PCI_CAPBILITY_POINTER_OFFSET
+                  )
+                );
+
+  //
+  // Scan the Capabilities List until we find the terminator element, or the
+  // Resource Reservation capability.
+  //
+  for (Offset = PciCapPtr & 0xFC;
+       Offset > 0;
+       Offset = ReservationHint->BridgeHdr.VendorHdr.Hdr.NextItemPtr & 0xFC) {
+    BOOLEAN EnoughRoom;
+
+    //
+    // Check if the Resource Reservation capability would fit into config space
+    // at this offset.
+    //
+    EnoughRoom = (BOOLEAN)(
+                   Offset <= PCI_MAX_CONFIG_OFFSET - sizeof *ReservationHint
+                   );
+
+    //
+    // Read the standard capability header so we can check the capability ID
+    // (if necessary) and advance to the next capability.
+    //
+    ReadConfigSpace (
+      HpcPciAddress,
+      &Offset,
+      (UINT8)sizeof ReservationHint->BridgeHdr.VendorHdr.Hdr,
+      &ReservationHint->BridgeHdr.VendorHdr.Hdr
+      );
+    if (!EnoughRoom ||
+        (ReservationHint->BridgeHdr.VendorHdr.Hdr.CapabilityID !=
+         EFI_PCI_CAPABILITY_ID_VENDOR)) {
+      continue;
+    }
+
+    //
+    // Read the rest of the vendor capability header so we can check the
+    // capability length.
+    //
+    COMPLETE_CONFIG_SPACE_STRUCT (
+      HpcPciAddress,
+      &Offset,
+      &ReservationHint->BridgeHdr.VendorHdr,
+      Hdr
+      );
+    if (ReservationHint->BridgeHdr.VendorHdr.Length !=
+        sizeof *ReservationHint) {
+      continue;
+    }
+
+    //
+    // Read the rest of the QEMU bridge capability header so we can check the
+    // capability type.
+    //
+    COMPLETE_CONFIG_SPACE_STRUCT (
+      HpcPciAddress,
+      &Offset,
+      &ReservationHint->BridgeHdr,
+      VendorHdr
+      );
+    if (ReservationHint->BridgeHdr.Type !=
+        QEMU_PCI_BRIDGE_CAPABILITY_TYPE_RESOURCE_RESERVATION) {
+      continue;
+    }
+
+    //
+    // Read the body of the reservation hint.
+    //
+    COMPLETE_CONFIG_SPACE_STRUCT (
+      HpcPciAddress,
+      &Offset,
+      ReservationHint,
+      BridgeHdr
+      );
+    return EFI_SUCCESS;
+  }
+
+  return EFI_NOT_FOUND;
+}
+
+
 /**
   Returns a list of root Hot Plug Controllers (HPCs) that require
   initialization during the boot process.
 
@@ -401,18 +634,21 @@ GetResourcePadding (
   OUT VOID                           **Padding,
   OUT EFI_HPC_PADDING_ATTRIBUTES     *Attributes
   )
 {
+  EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS     *Address;
   BOOLEAN                                         DefaultIo;
   BOOLEAN                                         DefaultMmio;
   RESOURCE_PADDING                                ReservationRequest;
   EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR               *FirstResource;
+  EFI_STATUS                                      ReservationHintStatus;
+  QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION ReservationHint;
+
+  Address = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddress;
 
   DEBUG_CODE (
-    EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *Address;
     CHAR16                                      *DevicePathString;
 
-    Address = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddress;
     DevicePathString = ConvertDevicePathToText (HpcDevicePath, FALSE, FALSE);
 
     DEBUG ((EFI_D_VERBOSE, "%a: Address=%02x:%02x.%x DevicePath=%s\n",
       __FUNCTION__, Address->Bus, Address->Device, Address->Function,
@@ -439,20 +675,143 @@ GetResourcePadding (
   FirstResource = ReservationRequest.Padding +
                   ARRAY_SIZE (ReservationRequest.Padding);
 
   //
-  // (b) Reserve IO space.
+  // Try to get the QEMU-specific Resource Reservation capability.
   //
+  ReservationHintStatus = QueryReservationHint (Address, &ReservationHint);
+  if (!EFI_ERROR (ReservationHintStatus)) {
+    INTN HighBit;
+
+    DEBUG ((
+      DEBUG_VERBOSE,
+      "%a: BusNumbers=0x%x Io=0x%Lx NonPrefetchable32BitMmio=0x%x\n"
+      "%a: Prefetchable32BitMmio=0x%x Prefetchable64BitMmio=0x%Lx\n",
+      __FUNCTION__,
+      ReservationHint.BusNumbers,
+      ReservationHint.Io,
+      ReservationHint.NonPrefetchable32BitMmio,
+      __FUNCTION__,
+      ReservationHint.Prefetchable32BitMmio,
+      ReservationHint.Prefetchable64BitMmio
+      ));
+
+    //
+    // (a) Reserve bus numbers.
+    //
+    switch (ReservationHint.BusNumbers) {
+    case 0:
+      //
+      // No reservation needed.
+      //
+      break;
+    case MAX_UINT32:
+      //
+      // Firmware default (unspecified). Treat it as "no reservation needed".
+      //
+      break;
+    default:
+      //
+      // Request the specified amount.
+      //
+      --FirstResource;
+      FirstResource->ResType = ACPI_ADDRESS_SPACE_TYPE_BUS;
+      FirstResource->AddrLen = ReservationHint.BusNumbers;
+      break;
+    }
+
+    //
+    // (b) Reserve IO space.
+    //
+    switch (ReservationHint.Io) {
+    case 0:
+      //
+      // No reservation needed, disable our built-in.
+      //
+      DefaultIo = FALSE;
+      break;
+    case MAX_UINT64:
+      //
+      // Firmware default (unspecified). Stick with our built-in.
+      //
+      break;
+    default:
+      //
+      // Round the specified amount up to the next power of two. If rounding is
+      // successful, reserve the rounded value. Fall back to the default
+      // otherwise.
+      //
+      HighBit = HighBitSetRoundUp64 (ReservationHint.Io);
+      if (HighBit != -1) {
+        SetIoPadding (--FirstResource, (UINTN)HighBit);
+        DefaultIo = FALSE;
+      }
+      break;
+    }
+
+    //
+    // (c) Reserve non-prefetchable MMIO space (32-bit only).
+    //
+    switch (ReservationHint.NonPrefetchable32BitMmio) {
+    case 0:
+      //
+      // No reservation needed, disable our built-in.
+      //
+      DefaultMmio = FALSE;
+      break;
+    case MAX_UINT32:
+      //
+      // Firmware default (unspecified). Stick with our built-in.
+      //
+      break;
+    default:
+      //
+      // Round the specified amount up to the next power of two. If rounding is
+      // successful, reserve the rounded value. Fall back to the default
+      // otherwise.
+      //
+      HighBit = HighBitSetRoundUp32 (ReservationHint.NonPrefetchable32BitMmio);
+      if (HighBit != -1) {
+        SetMmioPadding (--FirstResource, FALSE, TRUE, (UINTN)HighBit);
+        DefaultMmio = FALSE;
+      }
+      break;
+    }
+
+    //
+    // (d) Reserve prefetchable MMIO space (either 32-bit or 64-bit, never
+    // both).
+    //
+    // For either space, we treat 0 as "no reservation needed", and the maximum
+    // value as "firmware default". The latter is unspecified, and we interpret
+    // it as the former.
+    //
+    // Otherwise, round the specified amount up to the next power of two. If
+    // rounding is successful, reserve the rounded value. Do not reserve
+    // prefetchable MMIO space otherwise.
+    //
+    if (ReservationHint.Prefetchable32BitMmio > 0 &&
+        ReservationHint.Prefetchable32BitMmio < MAX_UINT32) {
+      HighBit = HighBitSetRoundUp32 (ReservationHint.Prefetchable32BitMmio);
+      if (HighBit != -1) {
+        SetMmioPadding (--FirstResource, TRUE, TRUE, (UINTN)HighBit);
+      }
+    } else if (ReservationHint.Prefetchable64BitMmio > 0 &&
+               ReservationHint.Prefetchable64BitMmio < MAX_UINT64) {
+      HighBit = HighBitSetRoundUp64 (ReservationHint.Prefetchable64BitMmio);
+      if (HighBit != -1) {
+        SetMmioPadding (--FirstResource, TRUE, FALSE, (UINTN)HighBit);
+      }
+    }
+  }
+
   if (DefaultIo) {
     //
     // Request defaults.
     //
     SetIoPadding (--FirstResource, (UINTN)HighBitSetRoundUp64 (512));
   }
 
-  //
-  // (c) Reserve non-prefetchable MMIO space (32-bit only).
-  //
   if (DefaultMmio) {
     //
     // Request defaults.
     //
-- 
2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 7/7] OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation hints
Posted by Jordan Justen 7 years, 2 months ago
On 2017-09-25 12:58:24, Laszlo Ersek wrote:
> Parse QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION from the bridges'
> conventional config spaces. Translate the fields as follows:
> 
> * BusNumbers:
>   * 0 -- no reservation;
>   * (-1) -- firmware default, i.e. no reservation;
>   * otherwise -- reserve the requested value. (NB, bus number reservation
>     is not supposed to work before
>     <https://bugzilla.tianocore.org/show_bug.cgi?id=656> is fixed.)
> 
> * Io:
>   * 0 -- no reservation;
>   * (-1) -- keep our current default (512B);
>   * otherwise -- round up the requested value and reserve that.
> 
> * NonPrefetchable32BitMmio:
>   * 0 -- no reservation;
>   * (-1) -- keep our current default (2MB);
>   * otherwise -- round up the requested value and reserve that.
> 
> * Prefetchable32BitMmio:
>   * 0 -- no reservation, proceed to Prefetchable64BitMmio;
>   * (-1) -- firmware default, i.e. no reservation, proceed to
>     Prefetchable64BitMmio;
>   * otherwise -- round up the requested value and reserve that. (NB, if
>     Prefetchable32BitMmio is reserved in addition to
>     NonPrefetchable32BitMmio, then PciBusDxe currently runs into an
>     assertion failure. Refer to
>     <https://bugzilla.tianocore.org/show_bug.cgi?id=720>.)
> 
> * Prefetchable64BitMmio:
>   * only reached if Prefetchable32BitMmio was not reserved;
>   * 0 -- no reservation;
>   * (-1) -- firmware default, i.e. no reservation;
>   * otherwise -- round up the requested value and reserve that.
> 
> If QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION is missing, plus any
> time the rounding fails, fall back to the current defaults.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf |   2 +
>  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c   | 371 +++++++++++++++++++-
>  2 files changed, 367 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
> index e0ec9baae1c2..38043986eb67 100644
> --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
> +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
> @@ -26,15 +26,17 @@ [Sources]
>  
>  [Packages]
>    MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
>    BaseLib
>    BaseMemoryLib
>    DebugLib
>    DevicePathLib
>    MemoryAllocationLib
> +  PciLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>  
>  [Protocols]
> diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
> index 39646973794b..177e1a62120d 100644
> --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
> +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
> @@ -13,14 +13,16 @@
>    WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  **/
>  
>  #include <IndustryStandard/Acpi10.h>
> +#include <IndustryStandard/QemuPciBridgeCapabilities.h>
>  
>  #include <Library/BaseLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Library/PciLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  
>  #include <Protocol/PciHotPlugInit.h>
>  #include <Protocol/PciRootBridgeIo.h>
> @@ -245,8 +247,239 @@ HighBitSetRoundUp64 (
>    return (HighBit < 64) ? HighBit : -1;
>  }
>  
>  
> +/**
> +  Read a slice from conventional PCI config space at the given offset, then
> +  advance the offset.
> +
> +  @param[in] PciAddress  The address of the PCI Device -- Bus, Device, Function
> +                         -- in UEFI (not PciLib) encoding.
> +
> +  @param[in,out] Offset  On input, the offset in conventional PCI config space
> +                         to start reading from. On output, the offset of the
> +                         first byte that was not read.
> +
> +  @param[in] Size        The number of bytes to read.
> +
> +  @param[out] Buffer     On output, the bytes read from PCI config space are
> +                         stored in this object.
> +**/
> +STATIC
> +VOID
> +ReadConfigSpace (
> +  IN     CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *PciAddress,
> +  IN OUT UINT8                                             *Offset,
> +  IN     UINT8                                             Size,
> +  OUT    VOID                                              *Buffer
> +  )
> +{
> +  PciReadBuffer (
> +    PCI_LIB_ADDRESS (
> +      PciAddress->Bus,
> +      PciAddress->Device,
> +      PciAddress->Function,
> +      *Offset
> +      ),
> +    Size,
> +    Buffer
> +    );
> +  *Offset += Size;
> +}
> +
> +
> +/**
> +  Convenience wrapper macro for ReadConfigSpace().
> +
> +  Given the following conditions:
> +
> +  - HeaderField is the first field in the structure pointed-to by Struct,
> +
> +  - Struct->HeaderField has been populated from the conventional PCI config
> +    space of the PCI device identified by PciAddress,
> +
> +  - *Offset points one past HeaderField in the conventional PCI config space of
> +    the PCI device identified by PciAddress,
> +
> +  populate the rest of *Struct from conventional PCI config space, starting at
> +  *Offset. Finally, increment *Offset so that it point one past *Struct.
> +
> +  @param[in] PciAddress  The address of the PCI Device -- Bus, Device, Function
> +                         -- in UEFI (not PciLib) encoding. Type: pointer to
> +                         CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS.
> +
> +  @param[in,out] Offset  On input, the offset in conventional PCI config space
> +                         to start reading from; one past Struct->HeaderField.
> +                         On output, the offset of the first byte that was not
> +                         read; one past *Struct. Type: pointer to UINT8.
> +
> +  @param[out] Struct     The structure to complete. Type: pointer to structure
> +                         object.
> +
> +  @param[in] HeaderField The name of the first field in *Struct, after which
> +                         *Struct should be populated. Type: structure member
> +                         identifier.
> +**/
> +#define COMPLETE_CONFIG_SPACE_STRUCT(PciAddress, Offset, Struct, HeaderField) \
> +          ReadConfigSpace (                                                   \
> +            (PciAddress),                                                     \
> +            (Offset),                                                         \
> +            (UINT8)(sizeof *(Struct) - sizeof ((Struct)->HeaderField)),       \
> +            &((Struct)->HeaderField) + 1                                      \
> +            )

There is a lot of assumptions about inputs with usage of this macro.
(Even the in/out offset on ReadConfigSpace seems a little unexpected.)

Based on that, I was hoping to come up with some suggestion. The two
thoughts I had seemed more appropriate for a more generic PCI helper
lib than for this driver:

* A 'reader' struct that bundles the state of the pci address, read
  buffer and offset read.

* More generically, a capabilities helper lib that could iterate &
  read the PCI capability structs.

Anyway, for this series:

Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

> +
> +
> +/**
> +  Look up the QEMU-specific Resource Reservation capability in the conventional
> +  config space of a Hotplug Controller (that is, PCI Bridge).
> +
> +  This function performs as few config space reads as possible.
> +
> +  @param[in] HpcPciAddress     The address of the PCI Bridge -- Bus, Device,
> +                               Function -- in UEFI (not PciLib) encoding.
> +
> +  @param[out] ReservationHint  The caller-allocated capability structure to
> +                               populate from the PCI Bridge's config space.
> +
> +  @retval EFI_SUCCESS    The capability has been found, ReservationHint has
> +                         been populated.
> +
> +  @retval EFI_NOT_FOUND  The capability is missing. The contents of
> +                         ReservationHint are now indeterminate.
> +**/
> +STATIC
> +EFI_STATUS
> +QueryReservationHint (
> +  IN  CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *HpcPciAddress,
> +  OUT QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION   *ReservationHint
> +)
> +{
> +  UINT16 PciVendorId;
> +  UINT16 PciStatus;
> +  UINT8  PciCapPtr;
> +  UINT8  Offset;
> +
> +  //
> +  // Check the vendor identifier.
> +  //
> +  PciVendorId = PciRead16 (
> +                  PCI_LIB_ADDRESS (
> +                    HpcPciAddress->Bus,
> +                    HpcPciAddress->Device,
> +                    HpcPciAddress->Function,
> +                    PCI_VENDOR_ID_OFFSET
> +                    )
> +                  );
> +  if (PciVendorId != QEMU_PCI_BRIDGE_VENDOR_ID_REDHAT) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  //
> +  // Check the Capabilities List bit in the PCI Status Register.
> +  //
> +  PciStatus = PciRead16 (
> +                PCI_LIB_ADDRESS (
> +                  HpcPciAddress->Bus,
> +                  HpcPciAddress->Device,
> +                  HpcPciAddress->Function,
> +                  PCI_PRIMARY_STATUS_OFFSET
> +                  )
> +                );
> +  if ((PciStatus & EFI_PCI_STATUS_CAPABILITY) == 0) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  //
> +  // Fetch the start of the Capabilities List.
> +  //
> +  PciCapPtr = PciRead8 (
> +                PCI_LIB_ADDRESS (
> +                  HpcPciAddress->Bus,
> +                  HpcPciAddress->Device,
> +                  HpcPciAddress->Function,
> +                  PCI_CAPBILITY_POINTER_OFFSET
> +                  )
> +                );
> +
> +  //
> +  // Scan the Capabilities List until we find the terminator element, or the
> +  // Resource Reservation capability.
> +  //
> +  for (Offset = PciCapPtr & 0xFC;
> +       Offset > 0;
> +       Offset = ReservationHint->BridgeHdr.VendorHdr.Hdr.NextItemPtr & 0xFC) {
> +    BOOLEAN EnoughRoom;
> +
> +    //
> +    // Check if the Resource Reservation capability would fit into config space
> +    // at this offset.
> +    //
> +    EnoughRoom = (BOOLEAN)(
> +                   Offset <= PCI_MAX_CONFIG_OFFSET - sizeof *ReservationHint
> +                   );
> +
> +    //
> +    // Read the standard capability header so we can check the capability ID
> +    // (if necessary) and advance to the next capability.
> +    //
> +    ReadConfigSpace (
> +      HpcPciAddress,
> +      &Offset,
> +      (UINT8)sizeof ReservationHint->BridgeHdr.VendorHdr.Hdr,
> +      &ReservationHint->BridgeHdr.VendorHdr.Hdr
> +      );
> +    if (!EnoughRoom ||
> +        (ReservationHint->BridgeHdr.VendorHdr.Hdr.CapabilityID !=
> +         EFI_PCI_CAPABILITY_ID_VENDOR)) {
> +      continue;
> +    }
> +
> +    //
> +    // Read the rest of the vendor capability header so we can check the
> +    // capability length.
> +    //
> +    COMPLETE_CONFIG_SPACE_STRUCT (
> +      HpcPciAddress,
> +      &Offset,
> +      &ReservationHint->BridgeHdr.VendorHdr,
> +      Hdr
> +      );
> +    if (ReservationHint->BridgeHdr.VendorHdr.Length !=
> +        sizeof *ReservationHint) {
> +      continue;
> +    }
> +
> +    //
> +    // Read the rest of the QEMU bridge capability header so we can check the
> +    // capability type.
> +    //
> +    COMPLETE_CONFIG_SPACE_STRUCT (
> +      HpcPciAddress,
> +      &Offset,
> +      &ReservationHint->BridgeHdr,
> +      VendorHdr
> +      );
> +    if (ReservationHint->BridgeHdr.Type !=
> +        QEMU_PCI_BRIDGE_CAPABILITY_TYPE_RESOURCE_RESERVATION) {
> +      continue;
> +    }
> +
> +    //
> +    // Read the body of the reservation hint.
> +    //
> +    COMPLETE_CONFIG_SPACE_STRUCT (
> +      HpcPciAddress,
> +      &Offset,
> +      ReservationHint,
> +      BridgeHdr
> +      );
> +    return EFI_SUCCESS;
> +  }
> +
> +  return EFI_NOT_FOUND;
> +}
> +
> +
>  /**
>    Returns a list of root Hot Plug Controllers (HPCs) that require
>    initialization during the boot process.
>  
> @@ -401,18 +634,21 @@ GetResourcePadding (
>    OUT VOID                           **Padding,
>    OUT EFI_HPC_PADDING_ATTRIBUTES     *Attributes
>    )
>  {
> +  EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS     *Address;
>    BOOLEAN                                         DefaultIo;
>    BOOLEAN                                         DefaultMmio;
>    RESOURCE_PADDING                                ReservationRequest;
>    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR               *FirstResource;
> +  EFI_STATUS                                      ReservationHintStatus;
> +  QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION ReservationHint;
> +
> +  Address = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddress;
>  
>    DEBUG_CODE (
> -    EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *Address;
>      CHAR16                                      *DevicePathString;
>  
> -    Address = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddress;
>      DevicePathString = ConvertDevicePathToText (HpcDevicePath, FALSE, FALSE);
>  
>      DEBUG ((EFI_D_VERBOSE, "%a: Address=%02x:%02x.%x DevicePath=%s\n",
>        __FUNCTION__, Address->Bus, Address->Device, Address->Function,
> @@ -439,20 +675,143 @@ GetResourcePadding (
>    FirstResource = ReservationRequest.Padding +
>                    ARRAY_SIZE (ReservationRequest.Padding);
>  
>    //
> -  // (b) Reserve IO space.
> +  // Try to get the QEMU-specific Resource Reservation capability.
>    //
> +  ReservationHintStatus = QueryReservationHint (Address, &ReservationHint);
> +  if (!EFI_ERROR (ReservationHintStatus)) {
> +    INTN HighBit;
> +
> +    DEBUG ((
> +      DEBUG_VERBOSE,
> +      "%a: BusNumbers=0x%x Io=0x%Lx NonPrefetchable32BitMmio=0x%x\n"
> +      "%a: Prefetchable32BitMmio=0x%x Prefetchable64BitMmio=0x%Lx\n",
> +      __FUNCTION__,
> +      ReservationHint.BusNumbers,
> +      ReservationHint.Io,
> +      ReservationHint.NonPrefetchable32BitMmio,
> +      __FUNCTION__,
> +      ReservationHint.Prefetchable32BitMmio,
> +      ReservationHint.Prefetchable64BitMmio
> +      ));
> +
> +    //
> +    // (a) Reserve bus numbers.
> +    //
> +    switch (ReservationHint.BusNumbers) {
> +    case 0:
> +      //
> +      // No reservation needed.
> +      //
> +      break;
> +    case MAX_UINT32:
> +      //
> +      // Firmware default (unspecified). Treat it as "no reservation needed".
> +      //
> +      break;
> +    default:
> +      //
> +      // Request the specified amount.
> +      //
> +      --FirstResource;
> +      FirstResource->ResType = ACPI_ADDRESS_SPACE_TYPE_BUS;
> +      FirstResource->AddrLen = ReservationHint.BusNumbers;
> +      break;
> +    }
> +
> +    //
> +    // (b) Reserve IO space.
> +    //
> +    switch (ReservationHint.Io) {
> +    case 0:
> +      //
> +      // No reservation needed, disable our built-in.
> +      //
> +      DefaultIo = FALSE;
> +      break;
> +    case MAX_UINT64:
> +      //
> +      // Firmware default (unspecified). Stick with our built-in.
> +      //
> +      break;
> +    default:
> +      //
> +      // Round the specified amount up to the next power of two. If rounding is
> +      // successful, reserve the rounded value. Fall back to the default
> +      // otherwise.
> +      //
> +      HighBit = HighBitSetRoundUp64 (ReservationHint.Io);
> +      if (HighBit != -1) {
> +        SetIoPadding (--FirstResource, (UINTN)HighBit);
> +        DefaultIo = FALSE;
> +      }
> +      break;
> +    }
> +
> +    //
> +    // (c) Reserve non-prefetchable MMIO space (32-bit only).
> +    //
> +    switch (ReservationHint.NonPrefetchable32BitMmio) {
> +    case 0:
> +      //
> +      // No reservation needed, disable our built-in.
> +      //
> +      DefaultMmio = FALSE;
> +      break;
> +    case MAX_UINT32:
> +      //
> +      // Firmware default (unspecified). Stick with our built-in.
> +      //
> +      break;
> +    default:
> +      //
> +      // Round the specified amount up to the next power of two. If rounding is
> +      // successful, reserve the rounded value. Fall back to the default
> +      // otherwise.
> +      //
> +      HighBit = HighBitSetRoundUp32 (ReservationHint.NonPrefetchable32BitMmio);
> +      if (HighBit != -1) {
> +        SetMmioPadding (--FirstResource, FALSE, TRUE, (UINTN)HighBit);
> +        DefaultMmio = FALSE;
> +      }
> +      break;
> +    }
> +
> +    //
> +    // (d) Reserve prefetchable MMIO space (either 32-bit or 64-bit, never
> +    // both).
> +    //
> +    // For either space, we treat 0 as "no reservation needed", and the maximum
> +    // value as "firmware default". The latter is unspecified, and we interpret
> +    // it as the former.
> +    //
> +    // Otherwise, round the specified amount up to the next power of two. If
> +    // rounding is successful, reserve the rounded value. Do not reserve
> +    // prefetchable MMIO space otherwise.
> +    //
> +    if (ReservationHint.Prefetchable32BitMmio > 0 &&
> +        ReservationHint.Prefetchable32BitMmio < MAX_UINT32) {
> +      HighBit = HighBitSetRoundUp32 (ReservationHint.Prefetchable32BitMmio);
> +      if (HighBit != -1) {
> +        SetMmioPadding (--FirstResource, TRUE, TRUE, (UINTN)HighBit);
> +      }
> +    } else if (ReservationHint.Prefetchable64BitMmio > 0 &&
> +               ReservationHint.Prefetchable64BitMmio < MAX_UINT64) {
> +      HighBit = HighBitSetRoundUp64 (ReservationHint.Prefetchable64BitMmio);
> +      if (HighBit != -1) {
> +        SetMmioPadding (--FirstResource, TRUE, FALSE, (UINTN)HighBit);
> +      }
> +    }
> +  }
> +
>    if (DefaultIo) {
>      //
>      // Request defaults.
>      //
>      SetIoPadding (--FirstResource, (UINTN)HighBitSetRoundUp64 (512));
>    }
>  
> -  //
> -  // (c) Reserve non-prefetchable MMIO space (32-bit only).
> -  //
>    if (DefaultMmio) {
>      //
>      // Request defaults.
>      //
> -- 
> 2.14.1.3.gb7cf6e02401b
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 7/7] OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation hints
Posted by Laszlo Ersek 7 years, 2 months ago
On 10/02/17 19:43, Jordan Justen wrote:
> On 2017-09-25 12:58:24, Laszlo Ersek wrote:
>> Parse QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION from the bridges'
>> conventional config spaces. Translate the fields as follows:
>>
>> * BusNumbers:
>>   * 0 -- no reservation;
>>   * (-1) -- firmware default, i.e. no reservation;
>>   * otherwise -- reserve the requested value. (NB, bus number reservation
>>     is not supposed to work before
>>     <https://bugzilla.tianocore.org/show_bug.cgi?id=656> is fixed.)
>>
>> * Io:
>>   * 0 -- no reservation;
>>   * (-1) -- keep our current default (512B);
>>   * otherwise -- round up the requested value and reserve that.
>>
>> * NonPrefetchable32BitMmio:
>>   * 0 -- no reservation;
>>   * (-1) -- keep our current default (2MB);
>>   * otherwise -- round up the requested value and reserve that.
>>
>> * Prefetchable32BitMmio:
>>   * 0 -- no reservation, proceed to Prefetchable64BitMmio;
>>   * (-1) -- firmware default, i.e. no reservation, proceed to
>>     Prefetchable64BitMmio;
>>   * otherwise -- round up the requested value and reserve that. (NB, if
>>     Prefetchable32BitMmio is reserved in addition to
>>     NonPrefetchable32BitMmio, then PciBusDxe currently runs into an
>>     assertion failure. Refer to
>>     <https://bugzilla.tianocore.org/show_bug.cgi?id=720>.)
>>
>> * Prefetchable64BitMmio:
>>   * only reached if Prefetchable32BitMmio was not reserved;
>>   * 0 -- no reservation;
>>   * (-1) -- firmware default, i.e. no reservation;
>>   * otherwise -- round up the requested value and reserve that.
>>
>> If QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION is missing, plus any
>> time the rounding fails, fall back to the current defaults.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf |   2 +
>>  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c   | 371 +++++++++++++++++++-
>>  2 files changed, 367 insertions(+), 6 deletions(-)
>>
>> diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
>> index e0ec9baae1c2..38043986eb67 100644
>> --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
>> +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
>> @@ -26,15 +26,17 @@ [Sources]
>>  
>>  [Packages]
>>    MdeModulePkg/MdeModulePkg.dec
>>    MdePkg/MdePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>>  
>>  [LibraryClasses]
>>    BaseLib
>>    BaseMemoryLib
>>    DebugLib
>>    DevicePathLib
>>    MemoryAllocationLib
>> +  PciLib
>>    UefiBootServicesTableLib
>>    UefiDriverEntryPoint
>>  
>>  [Protocols]
>> diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
>> index 39646973794b..177e1a62120d 100644
>> --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
>> +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
>> @@ -13,14 +13,16 @@
>>    WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>  **/
>>  
>>  #include <IndustryStandard/Acpi10.h>
>> +#include <IndustryStandard/QemuPciBridgeCapabilities.h>
>>  
>>  #include <Library/BaseLib.h>
>>  #include <Library/BaseMemoryLib.h>
>>  #include <Library/DebugLib.h>
>>  #include <Library/DevicePathLib.h>
>>  #include <Library/MemoryAllocationLib.h>
>> +#include <Library/PciLib.h>
>>  #include <Library/UefiBootServicesTableLib.h>
>>  
>>  #include <Protocol/PciHotPlugInit.h>
>>  #include <Protocol/PciRootBridgeIo.h>
>> @@ -245,8 +247,239 @@ HighBitSetRoundUp64 (
>>    return (HighBit < 64) ? HighBit : -1;
>>  }
>>  
>>  
>> +/**
>> +  Read a slice from conventional PCI config space at the given offset, then
>> +  advance the offset.
>> +
>> +  @param[in] PciAddress  The address of the PCI Device -- Bus, Device, Function
>> +                         -- in UEFI (not PciLib) encoding.
>> +
>> +  @param[in,out] Offset  On input, the offset in conventional PCI config space
>> +                         to start reading from. On output, the offset of the
>> +                         first byte that was not read.
>> +
>> +  @param[in] Size        The number of bytes to read.
>> +
>> +  @param[out] Buffer     On output, the bytes read from PCI config space are
>> +                         stored in this object.
>> +**/
>> +STATIC
>> +VOID
>> +ReadConfigSpace (
>> +  IN     CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *PciAddress,
>> +  IN OUT UINT8                                             *Offset,
>> +  IN     UINT8                                             Size,
>> +  OUT    VOID                                              *Buffer
>> +  )
>> +{
>> +  PciReadBuffer (
>> +    PCI_LIB_ADDRESS (
>> +      PciAddress->Bus,
>> +      PciAddress->Device,
>> +      PciAddress->Function,
>> +      *Offset
>> +      ),
>> +    Size,
>> +    Buffer
>> +    );
>> +  *Offset += Size;
>> +}
>> +
>> +
>> +/**
>> +  Convenience wrapper macro for ReadConfigSpace().
>> +
>> +  Given the following conditions:
>> +
>> +  - HeaderField is the first field in the structure pointed-to by Struct,
>> +
>> +  - Struct->HeaderField has been populated from the conventional PCI config
>> +    space of the PCI device identified by PciAddress,
>> +
>> +  - *Offset points one past HeaderField in the conventional PCI config space of
>> +    the PCI device identified by PciAddress,
>> +
>> +  populate the rest of *Struct from conventional PCI config space, starting at
>> +  *Offset. Finally, increment *Offset so that it point one past *Struct.
>> +
>> +  @param[in] PciAddress  The address of the PCI Device -- Bus, Device, Function
>> +                         -- in UEFI (not PciLib) encoding. Type: pointer to
>> +                         CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS.
>> +
>> +  @param[in,out] Offset  On input, the offset in conventional PCI config space
>> +                         to start reading from; one past Struct->HeaderField.
>> +                         On output, the offset of the first byte that was not
>> +                         read; one past *Struct. Type: pointer to UINT8.
>> +
>> +  @param[out] Struct     The structure to complete. Type: pointer to structure
>> +                         object.
>> +
>> +  @param[in] HeaderField The name of the first field in *Struct, after which
>> +                         *Struct should be populated. Type: structure member
>> +                         identifier.
>> +**/
>> +#define COMPLETE_CONFIG_SPACE_STRUCT(PciAddress, Offset, Struct, HeaderField) \
>> +          ReadConfigSpace (                                                   \
>> +            (PciAddress),                                                     \
>> +            (Offset),                                                         \
>> +            (UINT8)(sizeof *(Struct) - sizeof ((Struct)->HeaderField)),       \
>> +            &((Struct)->HeaderField) + 1                                      \
>> +            )
> 
> There is a lot of assumptions about inputs with usage of this macro.
> (Even the in/out offset on ReadConfigSpace seems a little unexpected.)
> 
> Based on that, I was hoping to come up with some suggestion. The two
> thoughts I had seemed more appropriate for a more generic PCI helper
> lib than for this driver:
> 
> * A 'reader' struct that bundles the state of the pci address, read
>   buffer and offset read.
> 
> * More generically, a capabilities helper lib that could iterate &
>   read the PCI capability structs.

*Absolutely* this ^^^

I've been much missing a canned function that just searches the config
space (conventional or extended, as requested) for a capability, and
returns an offset. Possibly taking an expected size parameter too, for
safety. PciBusDxe has such function(s), but they are not factored out.

I wondered if such a function (or a family of functions) should be part
of the PciLib class, but seeing the number of PciLib implementations, I
didn't even try. :/

I don't know if a standalone PciCapabilityLib class would be acceptable,
if it had (say) 3-4 functions in total. The IoFifo experience doesn't
seem to support that -- the IoFifo functions were ultimately added to
the IoLib class.

Also, I think I've implemented two config space scanning functions in my
career :) ; we'd probably want someone with more such experience to
*design* the lib class.

> 
> Anyway, for this series:
> 
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thank you!

Laszlo

>> +
>> +
>> +/**
>> +  Look up the QEMU-specific Resource Reservation capability in the conventional
>> +  config space of a Hotplug Controller (that is, PCI Bridge).
>> +
>> +  This function performs as few config space reads as possible.
>> +
>> +  @param[in] HpcPciAddress     The address of the PCI Bridge -- Bus, Device,
>> +                               Function -- in UEFI (not PciLib) encoding.
>> +
>> +  @param[out] ReservationHint  The caller-allocated capability structure to
>> +                               populate from the PCI Bridge's config space.
>> +
>> +  @retval EFI_SUCCESS    The capability has been found, ReservationHint has
>> +                         been populated.
>> +
>> +  @retval EFI_NOT_FOUND  The capability is missing. The contents of
>> +                         ReservationHint are now indeterminate.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +QueryReservationHint (
>> +  IN  CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *HpcPciAddress,
>> +  OUT QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION   *ReservationHint
>> +)
>> +{
>> +  UINT16 PciVendorId;
>> +  UINT16 PciStatus;
>> +  UINT8  PciCapPtr;
>> +  UINT8  Offset;
>> +
>> +  //
>> +  // Check the vendor identifier.
>> +  //
>> +  PciVendorId = PciRead16 (
>> +                  PCI_LIB_ADDRESS (
>> +                    HpcPciAddress->Bus,
>> +                    HpcPciAddress->Device,
>> +                    HpcPciAddress->Function,
>> +                    PCI_VENDOR_ID_OFFSET
>> +                    )
>> +                  );
>> +  if (PciVendorId != QEMU_PCI_BRIDGE_VENDOR_ID_REDHAT) {
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  //
>> +  // Check the Capabilities List bit in the PCI Status Register.
>> +  //
>> +  PciStatus = PciRead16 (
>> +                PCI_LIB_ADDRESS (
>> +                  HpcPciAddress->Bus,
>> +                  HpcPciAddress->Device,
>> +                  HpcPciAddress->Function,
>> +                  PCI_PRIMARY_STATUS_OFFSET
>> +                  )
>> +                );
>> +  if ((PciStatus & EFI_PCI_STATUS_CAPABILITY) == 0) {
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  //
>> +  // Fetch the start of the Capabilities List.
>> +  //
>> +  PciCapPtr = PciRead8 (
>> +                PCI_LIB_ADDRESS (
>> +                  HpcPciAddress->Bus,
>> +                  HpcPciAddress->Device,
>> +                  HpcPciAddress->Function,
>> +                  PCI_CAPBILITY_POINTER_OFFSET
>> +                  )
>> +                );
>> +
>> +  //
>> +  // Scan the Capabilities List until we find the terminator element, or the
>> +  // Resource Reservation capability.
>> +  //
>> +  for (Offset = PciCapPtr & 0xFC;
>> +       Offset > 0;
>> +       Offset = ReservationHint->BridgeHdr.VendorHdr.Hdr.NextItemPtr & 0xFC) {
>> +    BOOLEAN EnoughRoom;
>> +
>> +    //
>> +    // Check if the Resource Reservation capability would fit into config space
>> +    // at this offset.
>> +    //
>> +    EnoughRoom = (BOOLEAN)(
>> +                   Offset <= PCI_MAX_CONFIG_OFFSET - sizeof *ReservationHint
>> +                   );
>> +
>> +    //
>> +    // Read the standard capability header so we can check the capability ID
>> +    // (if necessary) and advance to the next capability.
>> +    //
>> +    ReadConfigSpace (
>> +      HpcPciAddress,
>> +      &Offset,
>> +      (UINT8)sizeof ReservationHint->BridgeHdr.VendorHdr.Hdr,
>> +      &ReservationHint->BridgeHdr.VendorHdr.Hdr
>> +      );
>> +    if (!EnoughRoom ||
>> +        (ReservationHint->BridgeHdr.VendorHdr.Hdr.CapabilityID !=
>> +         EFI_PCI_CAPABILITY_ID_VENDOR)) {
>> +      continue;
>> +    }
>> +
>> +    //
>> +    // Read the rest of the vendor capability header so we can check the
>> +    // capability length.
>> +    //
>> +    COMPLETE_CONFIG_SPACE_STRUCT (
>> +      HpcPciAddress,
>> +      &Offset,
>> +      &ReservationHint->BridgeHdr.VendorHdr,
>> +      Hdr
>> +      );
>> +    if (ReservationHint->BridgeHdr.VendorHdr.Length !=
>> +        sizeof *ReservationHint) {
>> +      continue;
>> +    }
>> +
>> +    //
>> +    // Read the rest of the QEMU bridge capability header so we can check the
>> +    // capability type.
>> +    //
>> +    COMPLETE_CONFIG_SPACE_STRUCT (
>> +      HpcPciAddress,
>> +      &Offset,
>> +      &ReservationHint->BridgeHdr,
>> +      VendorHdr
>> +      );
>> +    if (ReservationHint->BridgeHdr.Type !=
>> +        QEMU_PCI_BRIDGE_CAPABILITY_TYPE_RESOURCE_RESERVATION) {
>> +      continue;
>> +    }
>> +
>> +    //
>> +    // Read the body of the reservation hint.
>> +    //
>> +    COMPLETE_CONFIG_SPACE_STRUCT (
>> +      HpcPciAddress,
>> +      &Offset,
>> +      ReservationHint,
>> +      BridgeHdr
>> +      );
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  return EFI_NOT_FOUND;
>> +}
>> +
>> +
>>  /**
>>    Returns a list of root Hot Plug Controllers (HPCs) that require
>>    initialization during the boot process.
>>  
>> @@ -401,18 +634,21 @@ GetResourcePadding (
>>    OUT VOID                           **Padding,
>>    OUT EFI_HPC_PADDING_ATTRIBUTES     *Attributes
>>    )
>>  {
>> +  EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS     *Address;
>>    BOOLEAN                                         DefaultIo;
>>    BOOLEAN                                         DefaultMmio;
>>    RESOURCE_PADDING                                ReservationRequest;
>>    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR               *FirstResource;
>> +  EFI_STATUS                                      ReservationHintStatus;
>> +  QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION ReservationHint;
>> +
>> +  Address = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddress;
>>  
>>    DEBUG_CODE (
>> -    EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *Address;
>>      CHAR16                                      *DevicePathString;
>>  
>> -    Address = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddress;
>>      DevicePathString = ConvertDevicePathToText (HpcDevicePath, FALSE, FALSE);
>>  
>>      DEBUG ((EFI_D_VERBOSE, "%a: Address=%02x:%02x.%x DevicePath=%s\n",
>>        __FUNCTION__, Address->Bus, Address->Device, Address->Function,
>> @@ -439,20 +675,143 @@ GetResourcePadding (
>>    FirstResource = ReservationRequest.Padding +
>>                    ARRAY_SIZE (ReservationRequest.Padding);
>>  
>>    //
>> -  // (b) Reserve IO space.
>> +  // Try to get the QEMU-specific Resource Reservation capability.
>>    //
>> +  ReservationHintStatus = QueryReservationHint (Address, &ReservationHint);
>> +  if (!EFI_ERROR (ReservationHintStatus)) {
>> +    INTN HighBit;
>> +
>> +    DEBUG ((
>> +      DEBUG_VERBOSE,
>> +      "%a: BusNumbers=0x%x Io=0x%Lx NonPrefetchable32BitMmio=0x%x\n"
>> +      "%a: Prefetchable32BitMmio=0x%x Prefetchable64BitMmio=0x%Lx\n",
>> +      __FUNCTION__,
>> +      ReservationHint.BusNumbers,
>> +      ReservationHint.Io,
>> +      ReservationHint.NonPrefetchable32BitMmio,
>> +      __FUNCTION__,
>> +      ReservationHint.Prefetchable32BitMmio,
>> +      ReservationHint.Prefetchable64BitMmio
>> +      ));
>> +
>> +    //
>> +    // (a) Reserve bus numbers.
>> +    //
>> +    switch (ReservationHint.BusNumbers) {
>> +    case 0:
>> +      //
>> +      // No reservation needed.
>> +      //
>> +      break;
>> +    case MAX_UINT32:
>> +      //
>> +      // Firmware default (unspecified). Treat it as "no reservation needed".
>> +      //
>> +      break;
>> +    default:
>> +      //
>> +      // Request the specified amount.
>> +      //
>> +      --FirstResource;
>> +      FirstResource->ResType = ACPI_ADDRESS_SPACE_TYPE_BUS;
>> +      FirstResource->AddrLen = ReservationHint.BusNumbers;
>> +      break;
>> +    }
>> +
>> +    //
>> +    // (b) Reserve IO space.
>> +    //
>> +    switch (ReservationHint.Io) {
>> +    case 0:
>> +      //
>> +      // No reservation needed, disable our built-in.
>> +      //
>> +      DefaultIo = FALSE;
>> +      break;
>> +    case MAX_UINT64:
>> +      //
>> +      // Firmware default (unspecified). Stick with our built-in.
>> +      //
>> +      break;
>> +    default:
>> +      //
>> +      // Round the specified amount up to the next power of two. If rounding is
>> +      // successful, reserve the rounded value. Fall back to the default
>> +      // otherwise.
>> +      //
>> +      HighBit = HighBitSetRoundUp64 (ReservationHint.Io);
>> +      if (HighBit != -1) {
>> +        SetIoPadding (--FirstResource, (UINTN)HighBit);
>> +        DefaultIo = FALSE;
>> +      }
>> +      break;
>> +    }
>> +
>> +    //
>> +    // (c) Reserve non-prefetchable MMIO space (32-bit only).
>> +    //
>> +    switch (ReservationHint.NonPrefetchable32BitMmio) {
>> +    case 0:
>> +      //
>> +      // No reservation needed, disable our built-in.
>> +      //
>> +      DefaultMmio = FALSE;
>> +      break;
>> +    case MAX_UINT32:
>> +      //
>> +      // Firmware default (unspecified). Stick with our built-in.
>> +      //
>> +      break;
>> +    default:
>> +      //
>> +      // Round the specified amount up to the next power of two. If rounding is
>> +      // successful, reserve the rounded value. Fall back to the default
>> +      // otherwise.
>> +      //
>> +      HighBit = HighBitSetRoundUp32 (ReservationHint.NonPrefetchable32BitMmio);
>> +      if (HighBit != -1) {
>> +        SetMmioPadding (--FirstResource, FALSE, TRUE, (UINTN)HighBit);
>> +        DefaultMmio = FALSE;
>> +      }
>> +      break;
>> +    }
>> +
>> +    //
>> +    // (d) Reserve prefetchable MMIO space (either 32-bit or 64-bit, never
>> +    // both).
>> +    //
>> +    // For either space, we treat 0 as "no reservation needed", and the maximum
>> +    // value as "firmware default". The latter is unspecified, and we interpret
>> +    // it as the former.
>> +    //
>> +    // Otherwise, round the specified amount up to the next power of two. If
>> +    // rounding is successful, reserve the rounded value. Do not reserve
>> +    // prefetchable MMIO space otherwise.
>> +    //
>> +    if (ReservationHint.Prefetchable32BitMmio > 0 &&
>> +        ReservationHint.Prefetchable32BitMmio < MAX_UINT32) {
>> +      HighBit = HighBitSetRoundUp32 (ReservationHint.Prefetchable32BitMmio);
>> +      if (HighBit != -1) {
>> +        SetMmioPadding (--FirstResource, TRUE, TRUE, (UINTN)HighBit);
>> +      }
>> +    } else if (ReservationHint.Prefetchable64BitMmio > 0 &&
>> +               ReservationHint.Prefetchable64BitMmio < MAX_UINT64) {
>> +      HighBit = HighBitSetRoundUp64 (ReservationHint.Prefetchable64BitMmio);
>> +      if (HighBit != -1) {
>> +        SetMmioPadding (--FirstResource, TRUE, FALSE, (UINTN)HighBit);
>> +      }
>> +    }
>> +  }
>> +
>>    if (DefaultIo) {
>>      //
>>      // Request defaults.
>>      //
>>      SetIoPadding (--FirstResource, (UINTN)HighBitSetRoundUp64 (512));
>>    }
>>  
>> -  //
>> -  // (c) Reserve non-prefetchable MMIO space (32-bit only).
>> -  //
>>    if (DefaultMmio) {
>>      //
>>      // Request defaults.
>>      //
>> -- 
>> 2.14.1.3.gb7cf6e02401b
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel