Update the PciExpressLib to enable CCIX port as PCIe root host by
validating the PCIe addresses and introducing corresponding PCD
entries.

Change-Id: I0d1167b86e53a3781f59c4d68a3b2e61add4317e
Signed-off-by: Deepak Pandey <Deepak.Pandey@arm.com>
Signed-off-by: Khasim Syed Mohammed <khasim.mohammed@arm.com>
---
 .../PciExpressLib.c                           | 127 ++++++++++++------
 .../PciExpressLib.inf                         |   7 +-
 Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec   |   5 +-
 3 files changed, 94 insertions(+), 45 deletions(-)

diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
index bb0246b4a9..3abe0a2d6b 100644
--- a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
+++ b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
@@ -20,7 +20,7 @@
   The description of the workarounds included for these limitations can
   be found in the comments below.
 
-  Copyright (c) 2020, ARM Limited. All rights reserved.
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -36,15 +36,29 @@
 #include <Library/PcdLib.h>
 #include <NeoverseN1Soc.h>
 
+#define BUS_OFFSET      20
+#define DEV_OFFSET      15
+#define FUNC_OFFSET     12
+#define REG_OFFSET      4096
+
 /**
-  Assert the validity of a PCI address. A valid PCI address should contain 1's
-  only in the low 28 bits.
+  Assert the validity of a PCI address. A valid PCI address should contain 1's.
 
   @param  A The address to validate.
 
 **/
 #define ASSERT_INVALID_PCI_ADDRESS(A) \
-  ASSERT (((A) & ~0xfffffff) == 0)
+  ASSERT (((A) & ~0xffffffff) == 0)
+
+#define EFI_PCIE_ADDRESS(bus, dev, func, reg) \
+  (UINT64) ( \
+  (((UINTN) bus)   << BUS_OFFSET)  | \
+  (((UINTN) dev)   << DEV_OFFSET)  | \
+  (((UINTN) func)  << FUNC_OFFSET) | \
+  (((UINTN) (reg)) <  REG_OFFSET ?   \
+   ((UINTN) (reg)) : (UINT64) (LShiftU64 ((UINT64) (reg), 32))))
+
+#define GET_PCIE_BASE_ADDRESS(Address)  (Address & 0xF8000000)
 
 /* Root port Entry, BDF Entries Count */
 #define BDF_TABLE_ENTRY_SIZE    4
@@ -53,6 +67,7 @@
 
 /* BDF table offsets for PCIe */
 #define PCIE_BDF_TABLE_OFFSET   0
+#define CCIX_BDF_TABLE_OFFSET   (16 * 1024)
 
 #define GET_BUS_NUM(Address)    (((Address) >> 20) & 0x7F)
 #define GET_DEV_NUM(Address)    (((Address) >> 15) & 0x1F)
@@ -113,49 +128,64 @@ PciExpressRegisterForRuntimeAccess (
 }
 
 /**
-  Check if the requested PCI address can be safely accessed.
+  Check if the requested PCI address is a valid BDF address.
 
-  SCP performs the initial bus scan, prepares a table of valid BDF addresses
-  and shares them through non-trusted SRAM. This function validates if the
-  requested PCI address belongs to a valid BDF by checking the table of valid
-  entries. If not, this function will return false. This is a workaround to
-  avoid bus fault that occurs when accessing unavailable PCI device due to
-  hardware bug.
+  SCP performs the initial bus scan and prepares a table of valid BDF addresses
+  and shares them through non-trusted SRAM. This function validates if the PCI
+  address from any PCI request falls within the table of valid entries. If not,
+  this function will return 0xFFFFFFFF. This is a workaround to avoid bus fault
+  that happens when accessing unavailable PCI device due to RTL bug.
 
   @param  Address The address that encodes the PCI Bus, Device, Function and
                   Register.
 
-  @return TRUE    BDF can be accessed, valid.
-  @return FALSE   BDF should not be accessed, invalid.
+  @return The base address of PCI Express.
 
 **/
 STATIC
-BOOLEAN
+UINTN
 IsBdfValid (
-  IN      UINTN                     Address
+  IN UINTN Address
   )
 {
+  UINT8 Bus;
+  UINT8 Device;
+  UINT8 Function;
   UINTN BdfCount;
   UINTN BdfValue;
-  UINTN BdfEntry;
   UINTN Count;
   UINTN TableBase;
-  UINTN ConfigBase;
+  UINTN PciAddress;
+
+  Bus      = GET_BUS_NUM (Address);
+  Device   = GET_DEV_NUM (Address);
+  Function = GET_FUNC_NUM (Address);
+
+  PciAddress = EFI_PCIE_ADDRESS (Bus, Device, Function, 0);
+
+  if (GET_PCIE_BASE_ADDRESS (Address) ==
+        FixedPcdGet64 (PcdPcieExpressBaseAddress)) {
+    TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
+  } else {
+    TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + CCIX_BDF_TABLE_OFFSET;
+  }
 
-  ConfigBase = Address & ~0xFFF;
-  TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
   BdfCount = MmioRead32 (TableBase + BDF_TABLE_ENTRY_SIZE);
-  BdfEntry = TableBase + BDF_TABLE_HEADER_SIZE;
-
-  /* Skip the header & check remaining entry */
-  for (Count = 0; Count < BdfCount; Count++, BdfEntry += BDF_TABLE_ENTRY_SIZE) {
-    BdfValue = MmioRead32 (BdfEntry);
-    if (BdfValue == ConfigBase) {
-      return TRUE;
-    }
+
+  /* Start from the second entry */
+  for (Count = BDF_TABLE_HEADER_COUNT;
+       Count < (BdfCount + BDF_TABLE_HEADER_COUNT);
+       Count++) {
+    BdfValue = MmioRead32 (TableBase + (Count * BDF_TABLE_ENTRY_SIZE));
+    if (BdfValue == PciAddress)
+      break;
   }
 
-  return FALSE;
+  if (Count == (BdfCount + BDF_TABLE_HEADER_COUNT)) {
+    return mDummyConfigData;
+  } else {
+    return PciAddress;
+  }
 }
 
 /**
@@ -186,20 +216,35 @@ GetPciExpressAddress (
   IN      UINTN                     Address
   )
 {
-  UINT8 Bus, Device, Function;
-  UINTN ConfigAddress;
-
-  Bus = GET_BUS_NUM (Address);
-  Device = GET_DEV_NUM (Address);
+  UINT8  Bus;
+  UINT8  Device;
+  UINT8  Function;
+  UINT16 Register;
+  UINTN  ConfigAddress;
+
+  // Get the EFI notation
+  Bus      = GET_BUS_NUM (Address);
+  Device   = GET_DEV_NUM (Address);
   Function = GET_FUNC_NUM (Address);
-
-  if ((Bus == 0) && (Device == 0) && (Function == 0)) {
-    ConfigAddress = PcdGet32 (PcdPcieRootPortConfigBaseAddress) + Address;
-  } else {
-    ConfigAddress = PcdGet64 (PcdPciExpressBaseAddress) + Address;
-    if (!IsBdfValid(Address)) {
-      ConfigAddress = (UINTN)&mDummyConfigData;
-    }
+  Register = GET_REG_NUM (Address);
+
+  ConfigAddress = (UINTN)
+                  ((GET_PCIE_BASE_ADDRESS (Address) ==
+                    FixedPcdGet64 (PcdPcieExpressBaseAddress)) ?
+                      (((Bus == 0) && (Device == 0) && (Function == 0)) ?
+                        PcdGet32 (PcdPcieRootPortConfigBaseAddress
+                        + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)):
+                        PcdGet64 (PcdPcieExpressBaseAddress
+                        + EFI_PCIE_ADDRESS (Bus, Device, Function, Register))) :
+                      (((Bus == 0) && (Device == 0) && (Function == 0)) ?
+                        PcdGet32 (PcdCcixRootPortConfigBaseAddress
+                        + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)):
+                        PcdGet32 (PcdCcixExpressBaseAddress
+                        + EFI_PCIE_ADDRESS (Bus, Device, Function, Register))));
+
+  if (!((Bus == 0) && (Device == 0) && (Function == 0))) {
+    if (IsBdfValid (Address) == mDummyConfigData)
+      ConfigAddress = (UINTN) &mDummyConfigData;
   }
 
   return (VOID *)ConfigAddress;
diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
index acb6fb6219..eac981e460 100644
--- a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
+++ b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
@@ -21,7 +21,7 @@
 #    2. Root port ECAM space is not capable of 8bit/16bit writes.
 #  This library includes workaround for these limitations as well.
 #
-#  Copyright (c) 2020, ARM Limited. All rights reserved.
+#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -43,6 +43,8 @@
   Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
 
 [FixedPcd]
+  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseAddress
+  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseSize
   gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseAddress
   gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseSize
 
@@ -53,4 +55,5 @@
   PcdLib
 
 [Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress  ## CONSUMES
+  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress  ## CONSUMES
+  gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress
diff --git a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
index eea2d58402..5ec3c32539 100644
--- a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
+++ b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
@@ -46,6 +46,7 @@
   gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64MaxBase|0x28FFFFFFFF|UINT64|0x00000010
   gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Size|0x2000000000|UINT64|0x00000011
   gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Translation|0x0|UINT64|0x00000012
+  gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress|0x70000000|UINT64|0x00000013
 
   # CCIX
   gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusCount|18|UINT32|0x00000016
@@ -53,8 +54,8 @@
   gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusMin|0|UINT32|0x00000018
   gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress|0x68000000|UINT32|0x00000019
   gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoBase|0x0|UINT32|0x0000001A
-  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x01FFFF|UINT32|0x0000001B
-  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x020000|UINT32|0x0000001C
+  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x00FFFFFF|UINT32|0x0000001B
+  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x01000000|UINT32|0x0000001C
   gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoTranslation|0x6D200000|UINT32|0x00000001D
   gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32Base|0x69200000|UINT32|0x0000001E
   gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32MaxBase|0x6D1FFFFF|UINT32|0x00000001F
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83940): https://edk2.groups.io/g/devel/message/83940
Mute This Topic: https://groups.io/mt/87257273/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Khasim,

It seems that:
-PciHostBridgeDxe requires the PciSegmentLib.
-The MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf implementation requires the PciLib.
-The MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf requires the PciExpressLib, wich is implemented in Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/
-Since the patch 2 of the serie is now implementing a PciSegmentLib, isn't it possible to move the PCI hacks from Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/ to this new library ? The Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/ library would then not be needed anymore and could be removed.

A question about the mDummyConfigData value in Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c, shouldn't it be a const ?

I still made some comments inline in this patch.

Regards,

Pierre


On 11/23/21 1:26 PM, Khasim Mohammed via groups.io wrote:
> Update the PciExpressLib to enable CCIX port as PCIe root host by
> validating the PCIe addresses and introducing corresponding PCD
> entries.
>
> Change-Id: I0d1167b86e53a3781f59c4d68a3b2e61add4317e
> Signed-off-by: Deepak Pandey <Deepak.Pandey@arm.com>
> Signed-off-by: Khasim Syed Mohammed <khasim.mohammed@arm.com>
> ---
>  .../PciExpressLib.c                           | 127 ++++++++++++------
>  .../PciExpressLib.inf                         |   7 +-
>  Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec   |   5 +-
>  3 files changed, 94 insertions(+), 45 deletions(-)
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
> index bb0246b4a9..3abe0a2d6b 100644
> --- a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
> @@ -20,7 +20,7 @@
>    The description of the workarounds included for these limitations can
>    be found in the comments below.
>  
> -  Copyright (c) 2020, ARM Limited. All rights reserved.
> +  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -36,15 +36,29 @@
>  #include <Library/PcdLib.h>
>  #include <NeoverseN1Soc.h>
>  
> +#define BUS_OFFSET      20
> +#define DEV_OFFSET      15
> +#define FUNC_OFFSET     12
> +#define REG_OFFSET      4096
> +
>  /**
> -  Assert the validity of a PCI address. A valid PCI address should contain 1's
> -  only in the low 28 bits.
> +  Assert the validity of a PCI address. A valid PCI address should contain 1's.
>  
>    @param  A The address to validate.
>  
>  **/
>  #define ASSERT_INVALID_PCI_ADDRESS(A) \
> -  ASSERT (((A) & ~0xfffffff) == 0)
> +  ASSERT (((A) & ~0xffffffff) == 0)
> +
> +#define EFI_PCIE_ADDRESS(bus, dev, func, reg) \
> +  (UINT64) ( \
> +  (((UINTN) bus)   << BUS_OFFSET)  | \
> +  (((UINTN) dev)   << DEV_OFFSET)  | \
> +  (((UINTN) func)  << FUNC_OFFSET) | \
> +  (((UINTN) (reg)) <  REG_OFFSET ?   \
> +   ((UINTN) (reg)) : (UINT64) (LShiftU64 ((UINT64) (reg), 32))))
> +
> +#define GET_PCIE_BASE_ADDRESS(Address)  (Address & 0xF8000000)
>  
>  /* Root port Entry, BDF Entries Count */
>  #define BDF_TABLE_ENTRY_SIZE    4
> @@ -53,6 +67,7 @@
>  
>  /* BDF table offsets for PCIe */
>  #define PCIE_BDF_TABLE_OFFSET   0
> +#define CCIX_BDF_TABLE_OFFSET   (16 * 1024)
>  
>  #define GET_BUS_NUM(Address)    (((Address) >> 20) & 0x7F)
>  #define GET_DEV_NUM(Address)    (((Address) >> 15) & 0x1F)
> @@ -113,49 +128,64 @@ PciExpressRegisterForRuntimeAccess (
>  }
>  
>  /**
> -  Check if the requested PCI address can be safely accessed.
> +  Check if the requested PCI address is a valid BDF address.
>  
> -  SCP performs the initial bus scan, prepares a table of valid BDF addresses
> -  and shares them through non-trusted SRAM. This function validates if the
> -  requested PCI address belongs to a valid BDF by checking the table of valid
> -  entries. If not, this function will return false. This is a workaround to
> -  avoid bus fault that occurs when accessing unavailable PCI device due to
> -  hardware bug.
> +  SCP performs the initial bus scan and prepares a table of valid BDF addresses
> +  and shares them through non-trusted SRAM. This function validates if the PCI
> +  address from any PCI request falls within the table of valid entries. If not,
> +  this function will return 0xFFFFFFFF. This is a workaround to avoid bus fault
> +  that happens when accessing unavailable PCI device due to RTL bug.
>  
>    @param  Address The address that encodes the PCI Bus, Device, Function and
>                    Register.
>  
> -  @return TRUE    BDF can be accessed, valid.
> -  @return FALSE   BDF should not be accessed, invalid.
> +  @return The base address of PCI Express.
>  
>  **/
>  STATIC
> -BOOLEAN
> +UINTN
>  IsBdfValid (
> -  IN      UINTN                     Address
> +  IN UINTN Address
>    )
>  {
> +  UINT8 Bus;
> +  UINT8 Device;
> +  UINT8 Function;
>    UINTN BdfCount;
>    UINTN BdfValue;
> -  UINTN BdfEntry;
>    UINTN Count;
>    UINTN TableBase;
> -  UINTN ConfigBase;
> +  UINTN PciAddress;
> +
> +  Bus      = GET_BUS_NUM (Address);
> +  Device   = GET_DEV_NUM (Address);
> +  Function = GET_FUNC_NUM (Address);
> +
> +  PciAddress = EFI_PCIE_ADDRESS (Bus, Device, Function, 0);
> +
> +  if (GET_PCIE_BASE_ADDRESS (Address) ==
> +        FixedPcdGet64 (PcdPcieExpressBaseAddress)) {
> +    TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
> +  } else {
> +    TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + CCIX_BDF_TABLE_OFFSET;
> +  }
>  
> -  ConfigBase = Address & ~0xFFF;
> -  TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
>    BdfCount = MmioRead32 (TableBase + BDF_TABLE_ENTRY_SIZE);
> -  BdfEntry = TableBase + BDF_TABLE_HEADER_SIZE;
> -
> -  /* Skip the header & check remaining entry */
> -  for (Count = 0; Count < BdfCount; Count++, BdfEntry += BDF_TABLE_ENTRY_SIZE) {
> -    BdfValue = MmioRead32 (BdfEntry);
> -    if (BdfValue == ConfigBase) {
> -      return TRUE;
> -    }
> +
> +  /* Start from the second entry */
> +  for (Count = BDF_TABLE_HEADER_COUNT;
> +       Count < (BdfCount + BDF_TABLE_HEADER_COUNT);
> +       Count++) {
> +    BdfValue = MmioRead32 (TableBase + (Count * BDF_TABLE_ENTRY_SIZE));
> +    if (BdfValue == PciAddress)
> +      break;
>    }
>  
> -  return FALSE;
> +  if (Count == (BdfCount + BDF_TABLE_HEADER_COUNT)) {
> +    return mDummyConfigData;
> +  } else {
> +    return PciAddress;
> +  }
>  }
>  
>  /**
> @@ -186,20 +216,35 @@ GetPciExpressAddress (
>    IN      UINTN                     Address
>    )
>  {
> -  UINT8 Bus, Device, Function;
> -  UINTN ConfigAddress;
> -
> -  Bus = GET_BUS_NUM (Address);
> -  Device = GET_DEV_NUM (Address);
> +  UINT8  Bus;
> +  UINT8  Device;
> +  UINT8  Function;
> +  UINT16 Register;
> +  UINTN  ConfigAddress;
> +
> +  // Get the EFI notation
> +  Bus      = GET_BUS_NUM (Address);
> +  Device   = GET_DEV_NUM (Address);
>    Function = GET_FUNC_NUM (Address);
> -
> -  if ((Bus == 0) && (Device == 0) && (Function == 0)) {
> -    ConfigAddress = PcdGet32 (PcdPcieRootPortConfigBaseAddress) + Address;
> -  } else {
> -    ConfigAddress = PcdGet64 (PcdPciExpressBaseAddress) + Address;
> -    if (!IsBdfValid(Address)) {
> -      ConfigAddress = (UINTN)&mDummyConfigData;
> -    }
> +  Register = GET_REG_NUM (Address);
> +
> +  ConfigAddress = (UINTN)
> +                  ((GET_PCIE_BASE_ADDRESS (Address) ==
> +                    FixedPcdGet64 (PcdPcieExpressBaseAddress)) ?
> +                      (((Bus == 0) && (Device == 0) && (Function == 0)) ?
> +                        PcdGet32 (PcdPcieRootPortConfigBaseAddress
> +                        + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)):
> +                        PcdGet64 (PcdPcieExpressBaseAddress
> +                        + EFI_PCIE_ADDRESS (Bus, Device, Function, Register))) :
> +                      (((Bus == 0) && (Device == 0) && (Function == 0)) ?
> +                        PcdGet32 (PcdCcixRootPortConfigBaseAddress
> +                        + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)):
> +                        PcdGet32 (PcdCcixExpressBaseAddress
> +                        + EFI_PCIE_ADDRESS (Bus, Device, Function, Register))));

Is it possible to split it in multiple statement ? Since the case of the root bridge is special and checked multiple times, we could store the result of  "(Bus == 0) && (Device == 0) && (Function == 0)"

I m not sure why this is valid, but shouldn't the brackets of:

PcdGet64 (PcdPcieExpressBaseAddress + EFI_PCIE_ADDRESS (Bus, Device, Function, Register))

be like:

PcdGet64 (PcdPcieExpressBaseAddress) + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)

(Same question for other PcdGet64() calls in the assignement)

> +
> +  if (!((Bus == 0) && (Device == 0) && (Function == 0))) {
> +    if (IsBdfValid (Address) == mDummyConfigData)
> +      ConfigAddress = (UINTN) &mDummyConfigData;
>    }
>  
>    return (VOID *)ConfigAddress;
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
> index acb6fb6219..eac981e460 100644
> --- a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
> @@ -21,7 +21,7 @@
>  #    2. Root port ECAM space is not capable of 8bit/16bit writes.
>  #  This library includes workaround for these limitations as well.
>  #
> -#  Copyright (c) 2020, ARM Limited. All rights reserved.
> +#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -43,6 +43,8 @@
>    Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>  
>  [FixedPcd]
> +  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseAddress
> +  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseSize
>    gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseAddress
>    gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseSize
>  
> @@ -53,4 +55,5 @@
>    PcdLib
>  
>  [Pcd]
> -  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress  ## CONSUMES
> +  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress  ## CONSUMES
> +  gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress
> diff --git a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> index eea2d58402..5ec3c32539 100644
> --- a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> +++ b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> @@ -46,6 +46,7 @@
>    gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64MaxBase|0x28FFFFFFFF|UINT64|0x00000010
>    gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Size|0x2000000000|UINT64|0x00000011
>    gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Translation|0x0|UINT64|0x00000012
> +  gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress|0x70000000|UINT64|0x00000013
>  
>    # CCIX
>    gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusCount|18|UINT32|0x00000016
> @@ -53,8 +54,8 @@
>    gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusMin|0|UINT32|0x00000018
>    gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress|0x68000000|UINT32|0x00000019
>    gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoBase|0x0|UINT32|0x0000001A
> -  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x01FFFF|UINT32|0x0000001B
> -  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x020000|UINT32|0x0000001C
> +  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x00FFFFFF|UINT32|0x0000001B
> +  gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x01000000|UINT32|0x0000001C
>    gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoTranslation|0x6D200000|UINT32|0x00000001D
>    gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32Base|0x69200000|UINT32|0x0000001E
>    gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32MaxBase|0x6D1FFFFF|UINT32|0x00000001F


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84165): https://edk2.groups.io/g/devel/message/84165
Mute This Topic: https://groups.io/mt/87257273/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Pierre,

I have addressed all the suggested changes and posted v3 version of patches.

On Tue, Nov 30, 2021 at 09:10 AM, PierreGondois wrote:

> 
> Hi Khasim,
> 
> It seems that:
> -PciHostBridgeDxe requires the PciSegmentLib.
> -The MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> implementation requires the PciLib.
> -The MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf requires
> the PciExpressLib, wich is implemented in
> Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/
> -Since the patch 2 of the serie is now implementing a PciSegmentLib, isn't
> it possible to move the PCI hacks from
> Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/ to this new
> library ? The
> Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/ library
> would then not be needed anymore and could be removed.

I tried to do this, but unfortunately wasn't successful. We are reusing the standard version of PciExpressLib.c and modifying the GetPci functionality to return the updated base address. If we remove the inf entry from custom implementation then it will leverage the default one and it will fail to get fixed PCIe address.

There is similar patches posted for Morello SOC,
https://edk2.groups.io/g/devel/message/84344?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Cchandni%2C20%2C2%2C0%2C87497024
https://edk2.groups.io/g/devel/message/84165?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Ckhasim%2C20%2C2%2C0%2C87257273

> 
> A question about the mDummyConfigData value in
> Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c,
> shouldn't it be a const ?

Have fixed this in v3 version of patches.

> 
> I still made some comments inline in this patch.
> 
> Regards,
> 
> Pierre
> 
> 
> On 11/23/21 1:26 PM, Khasim Mohammed via groups.io wrote:
> 
>> Update the PciExpressLib to enable CCIX port as PCIe root host by
>> validating the PCIe addresses and introducing corresponding PCD
>> entries.
>> 
>> Change-Id: I0d1167b86e53a3781f59c4d68a3b2e61add4317e
>> Signed-off-by: Deepak Pandey <Deepak.Pandey@arm.com>
>> Signed-off-by: Khasim Syed Mohammed <khasim.mohammed@arm.com>
>> ---
>> .../PciExpressLib.c | 127 ++++++++++++------
>> .../PciExpressLib.inf | 7 +-
>> Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec | 5 +-
>> 3 files changed, 94 insertions(+), 45 deletions(-)
>> 
>> diff --git
>> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
>> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
>> 
>> index bb0246b4a9..3abe0a2d6b 100644
>> ---
>> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
>> 
>> +++
>> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
>> 
>> @@ -20,7 +20,7 @@
>> The description of the workarounds included for these limitations can
>> be found in the comments below.
>> 
>> - Copyright (c) 2020, ARM Limited. All rights reserved.
>> + Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.<BR>
>> 
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>> 
>> @@ -36,15 +36,29 @@
>> #include <Library/PcdLib.h>
>> #include <NeoverseN1Soc.h>
>> 
>> +#define BUS_OFFSET 20
>> +#define DEV_OFFSET 15
>> +#define FUNC_OFFSET 12
>> +#define REG_OFFSET 4096
>> +
>> /**
>> - Assert the validity of a PCI address. A valid PCI address should contain
>> 1's
>> - only in the low 28 bits.
>> + Assert the validity of a PCI address. A valid PCI address should contain
>> 1's.
>> 
>> @param A The address to validate.
>> 
>> **/
>> #define ASSERT_INVALID_PCI_ADDRESS(A) \
>> - ASSERT (((A) & ~0xfffffff) == 0)
>> + ASSERT (((A) & ~0xffffffff) == 0)
>> +
>> +#define EFI_PCIE_ADDRESS(bus, dev, func, reg) \
>> + (UINT64) ( \
>> + (((UINTN) bus) << BUS_OFFSET) | \
>> + (((UINTN) dev) << DEV_OFFSET) | \
>> + (((UINTN) func) << FUNC_OFFSET) | \
>> + (((UINTN) (reg)) < REG_OFFSET ? \
>> + ((UINTN) (reg)) : (UINT64) (LShiftU64 ((UINT64) (reg), 32))))
>> +
>> +#define GET_PCIE_BASE_ADDRESS(Address) (Address & 0xF8000000)
>> 
>> /* Root port Entry, BDF Entries Count */
>> #define BDF_TABLE_ENTRY_SIZE 4
>> @@ -53,6 +67,7 @@
>> 
>> /* BDF table offsets for PCIe */
>> #define PCIE_BDF_TABLE_OFFSET 0
>> +#define CCIX_BDF_TABLE_OFFSET (16 * 1024)
>> 
>> #define GET_BUS_NUM(Address) (((Address) >> 20) & 0x7F)
>> #define GET_DEV_NUM(Address) (((Address) >> 15) & 0x1F)
>> @@ -113,49 +128,64 @@ PciExpressRegisterForRuntimeAccess (
>> }
>> 
>> /**
>> - Check if the requested PCI address can be safely accessed.
>> + Check if the requested PCI address is a valid BDF address.
>> 
>> - SCP performs the initial bus scan, prepares a table of valid BDF
>> addresses
>> - and shares them through non-trusted SRAM. This function validates if the
>> 
>> - requested PCI address belongs to a valid BDF by checking the table of
>> valid
>> - entries. If not, this function will return false. This is a workaround
>> to
>> - avoid bus fault that occurs when accessing unavailable PCI device due to
>> 
>> - hardware bug.
>> + SCP performs the initial bus scan and prepares a table of valid BDF
>> addresses
>> + and shares them through non-trusted SRAM. This function validates if the
>> PCI
>> + address from any PCI request falls within the table of valid entries. If
>> not,
>> + this function will return 0xFFFFFFFF. This is a workaround to avoid bus
>> fault
>> + that happens when accessing unavailable PCI device due to RTL bug.
>> 
>> @param Address The address that encodes the PCI Bus, Device, Function and
>> Register.
>> 
>> - @return TRUE BDF can be accessed, valid.
>> - @return FALSE BDF should not be accessed, invalid.
>> + @return The base address of PCI Express.
>> 
>> **/
>> STATIC
>> -BOOLEAN
>> +UINTN
>> IsBdfValid (
>> - IN UINTN Address
>> + IN UINTN Address
>> )
>> {
>> + UINT8 Bus;
>> + UINT8 Device;
>> + UINT8 Function;
>> UINTN BdfCount;
>> UINTN BdfValue;
>> - UINTN BdfEntry;
>> UINTN Count;
>> UINTN TableBase;
>> - UINTN ConfigBase;
>> + UINTN PciAddress;
>> +
>> + Bus = GET_BUS_NUM (Address);
>> + Device = GET_DEV_NUM (Address);
>> + Function = GET_FUNC_NUM (Address);
>> +
>> + PciAddress = EFI_PCIE_ADDRESS (Bus, Device, Function, 0);
>> +
>> + if (GET_PCIE_BASE_ADDRESS (Address) ==
>> + FixedPcdGet64 (PcdPcieExpressBaseAddress)) {
>> + TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
>> + } else {
>> + TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + CCIX_BDF_TABLE_OFFSET;
>> + }
>> 
>> - ConfigBase = Address & ~0xFFF;
>> - TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
>> BdfCount = MmioRead32 (TableBase + BDF_TABLE_ENTRY_SIZE);
>> - BdfEntry = TableBase + BDF_TABLE_HEADER_SIZE;
>> -
>> - /* Skip the header & check remaining entry */
>> - for (Count = 0; Count < BdfCount; Count++, BdfEntry +=
>> BDF_TABLE_ENTRY_SIZE) {
>> - BdfValue = MmioRead32 (BdfEntry);
>> - if (BdfValue == ConfigBase) {
>> - return TRUE;
>> - }
>> +
>> + /* Start from the second entry */
>> + for (Count = BDF_TABLE_HEADER_COUNT;
>> + Count < (BdfCount + BDF_TABLE_HEADER_COUNT);
>> + Count++) {
>> + BdfValue = MmioRead32 (TableBase + (Count * BDF_TABLE_ENTRY_SIZE));
>> + if (BdfValue == PciAddress)
>> + break;
>> }
>> 
>> - return FALSE;
>> + if (Count == (BdfCount + BDF_TABLE_HEADER_COUNT)) {
>> + return mDummyConfigData;
>> + } else {
>> + return PciAddress;
>> + }
>> }
>> 
>> /**
>> @@ -186,20 +216,35 @@ GetPciExpressAddress (
>> IN UINTN Address
>> )
>> {
>> - UINT8 Bus, Device, Function;
>> - UINTN ConfigAddress;
>> -
>> - Bus = GET_BUS_NUM (Address);
>> - Device = GET_DEV_NUM (Address);
>> + UINT8 Bus;
>> + UINT8 Device;
>> + UINT8 Function;
>> + UINT16 Register;
>> + UINTN ConfigAddress;
>> +
>> + // Get the EFI notation
>> + Bus = GET_BUS_NUM (Address);
>> + Device = GET_DEV_NUM (Address);
>> Function = GET_FUNC_NUM (Address);
>> -
>> - if ((Bus == 0) && (Device == 0) && (Function == 0)) {
>> - ConfigAddress = PcdGet32 (PcdPcieRootPortConfigBaseAddress) + Address;
>> - } else {
>> - ConfigAddress = PcdGet64 (PcdPciExpressBaseAddress) + Address;
>> - if (!IsBdfValid(Address)) {
>> - ConfigAddress = (UINTN)&mDummyConfigData;
>> - }
>> + Register = GET_REG_NUM (Address);
>> +
>> + ConfigAddress = (UINTN)
>> + ((GET_PCIE_BASE_ADDRESS (Address) ==
>> + FixedPcdGet64 (PcdPcieExpressBaseAddress)) ?
>> + (((Bus == 0) && (Device == 0) && (Function == 0)) ?
>> + PcdGet32 (PcdPcieRootPortConfigBaseAddress
>> + + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)):
>> + PcdGet64 (PcdPcieExpressBaseAddress
>> + + EFI_PCIE_ADDRESS (Bus, Device, Function, Register))) :
>> + (((Bus == 0) && (Device == 0) && (Function == 0)) ?
>> + PcdGet32 (PcdCcixRootPortConfigBaseAddress
>> + + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)):
>> + PcdGet32 (PcdCcixExpressBaseAddress
>> + + EFI_PCIE_ADDRESS (Bus, Device, Function, Register))));
> 
> Is it possible to split it in multiple statement ? Since the case of the
> root bridge is special and checked multiple times, we could store the
> result of  "(Bus == 0) && (Device == 0) && (Function == 0)"

Have spit this functionality in v3 version of patches.

> 
> I m not sure why this is valid, but shouldn't the brackets of:
> 
> PcdGet64 (PcdPcieExpressBaseAddress + EFI_PCIE_ADDRESS (Bus, Device,
> Function, Register))
> 
> be like:
> 
> PcdGet64 (PcdPcieExpressBaseAddress) + EFI_PCIE_ADDRESS (Bus, Device,
> Function, Register)
> 
> (Same question for other PcdGet64() calls in the assignement)

Have fixed this.

> 
> 
>> +
>> + if (!((Bus == 0) && (Device == 0) && (Function == 0))) {
>> + if (IsBdfValid (Address) == mDummyConfigData)
>> + ConfigAddress = (UINTN) &mDummyConfigData;
>> }
>> 
>> return (VOID *)ConfigAddress;
>> diff --git
>> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
>> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
>> 
>> index acb6fb6219..eac981e460 100644
>> ---
>> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
>> 
>> +++
>> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
>> 
>> @@ -21,7 +21,7 @@
>> # 2. Root port ECAM space is not capable of 8bit/16bit writes.
>> # This library includes workaround for these limitations as well.
>> #
>> -# Copyright (c) 2020, ARM Limited. All rights reserved.
>> +# Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.<BR>
>> #
>> # SPDX-License-Identifier: BSD-2-Clause-Patent
>> #
>> @@ -43,6 +43,8 @@
>> Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>> 
>> [FixedPcd]
>> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseAddress
>> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseSize
>> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseAddress
>> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseSize
>> 
>> @@ -53,4 +55,5 @@
>> PcdLib
>> 
>> [Pcd]
>> - gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress ## CONSUMES
>> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress ## CONSUMES
>> + gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress
>> diff --git a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>> b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>> index eea2d58402..5ec3c32539 100644
>> --- a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>> +++ b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>> @@ -46,6 +46,7 @@
>> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64MaxBase|0x28FFFFFFFF|UINT64|0x00000010
>> 
>> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Size|0x2000000000|UINT64|0x00000011
>> 
>> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Translation|0x0|UINT64|0x00000012
>> 
>> +
>> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress|0x70000000|UINT64|0x00000013
>> 
>> 
>> # CCIX
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusCount|18|UINT32|0x00000016
>> @@ -53,8 +54,8 @@
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusMin|0|UINT32|0x00000018
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress|0x68000000|UINT32|0x00000019
>> 
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoBase|0x0|UINT32|0x0000001A
>> -
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x01FFFF|UINT32|0x0000001B
>> 
>> - gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x020000|UINT32|0x0000001C
>> 
>> +
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x00FFFFFF|UINT32|0x0000001B
>> 
>> +
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x01000000|UINT32|0x0000001C
>> 
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoTranslation|0x6D200000|UINT32|0x00000001D
>> 
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32Base|0x69200000|UINT32|0x0000001E
>> 
>> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32MaxBase|0x6D1FFFFF|UINT32|0x00000001F
>> 
> 
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84398): https://edk2.groups.io/g/devel/message/84398
Mute This Topic: https://groups.io/mt/87257273/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-