Topics

[Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe: Support PCIe Resizable BAR Capability


Heng Luo
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D313

Add PcdPcieResizableBarSupport to enable/disable PCIe Resizable
BAR Capability fearture.
Program the Resizable BAR Register if the device suports PCIe Resizable
BAR Capability and PcdPcieResizableBarSupport is TRUE.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Heng Luo <heng.luo@intel.com>
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 4 +++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 3 ++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 27 ++++++++++++++=
++++++++++++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h | 12 +++++++++++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 185 ++++++++++++++=
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++=
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++=
+++++++--------------
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h | 22 ++++++++++++++=
+++++++-
MdeModulePkg/MdeModulePkg.dec | 8 +++++++-
7 files changed, 241 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h b/MdeModulePkg/Bus/Pci=
/PciBusDxe/PciBus.h
index d4113993c8..a619a68526 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
@@ -1,7 +1,7 @@
/** @file=0D
Header files and data structures needed by PCI Bus module.=0D
=0D
-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
@@ -280,6 +280,8 @@ struct _PCI_IO_DEVICE {
// This field is used to support this case.=0D
//=0D
UINT16 BridgeIoAlignment;=0D
+ UINT32 ResizableBarOffset;=0D
+ UINT32 ResizableBarNumber;=0D
};=0D
=0D
#define PCI_IO_DEVICE_FROM_PCI_IO_THIS(a) \=0D
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bu=
s/Pci/PciBusDxe/PciBusDxe.inf
index 9284998f36..e317169d9c 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
@@ -2,7 +2,7 @@
# The PCI bus driver will probe all PCI devices and allocate MMIO and IO =
space for these devices.=0D
# Please use PCD feature flag PcdPciBusHotplugDeviceSupport to enable hot=
plug supporting.=0D
#=0D
-# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>=
=0D
+# Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>=
=0D
#=0D
# SPDX-License-Identifier: BSD-2-Clause-Patent=0D
#=0D
@@ -106,6 +106,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## CONSUME=
S=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport ## CONSUME=
S=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration ## SOMETIM=
ES_CONSUMES=0D
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport ## CONSUME=
S=0D
=0D
[UserExtensions.TianoCore."ExtraFiles"]=0D
PciBusDxeExtra.uni=0D
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeMod=
ulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index 6c68a97d4e..1b64924b7b 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -1,7 +1,7 @@
/** @file=0D
PCI emumeration support functions implementation for PCI Bus module.=0D
=0D
-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>=0D
(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
@@ -2426,6 +2426,31 @@ CreatePciIoDevice (
}=0D
}=0D
=0D
+ PciIoDevice->ResizableBarOffset =3D 0;=0D
+ if (PcdGetBool (PcdPcieResizableBarSupport)) {=0D
+ Status =3D LocatePciExpressCapabilityRegBlock (=0D
+ PciIoDevice,=0D
+ PCI_EXPRESS_EXTENDED_CAPABILITY_RESIZABLE_BAR_ID,=0D
+ &PciIoDevice->ResizableBarOffset,=0D
+ NULL=0D
+ );=0D
+ if (!EFI_ERROR (Status)) {=0D
+ PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_CONTROL ResizableBar=
Control;=0D
+ UINT32 Offset;=0D
+ Offset =3D PciIoDevice->ResizableBarOffset + sizeof (PCI_EXPRESS_EXT=
ENDED_CAPABILITIES_HEADER)=0D
+ + sizeof (PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_=
CAPABILITY),=0D
+ PciIo->Pci.Read (=0D
+ PciIo,=0D
+ EfiPciIoWidthUint8,=0D
+ Offset,=0D
+ sizeof (PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_CONT=
ROL),=0D
+ &ResizableBarControl=0D
+ );=0D
+ PciIoDevice->ResizableBarNumber =3D ResizableBarControl.Bits.Resizab=
leBarNumber;=0D
+ PciProgramResizableBar (PciIoDevice, PciResizableBarMax);=0D
+ }=0D
+ }=0D
+=0D
//=0D
// Initialize the reserved resource list=0D
//=0D
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h b/MdeMod=
ulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
index d76606c7df..4581b270c9 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
@@ -1,7 +1,7 @@
/** @file=0D
PCI enumeration support functions declaration for PCI Bus module.=0D
=0D
-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
@@ -467,4 +467,14 @@ DumpPpbPaddingResource (
IN PCI_BAR_TYPE ResourceType=0D
);=0D
=0D
+/**=0D
+ Dump the PCI BAR information.=0D
+=0D
+ @param PciIoDevice PCI IO instance.=0D
+**/=0D
+VOID=0D
+DumpPciBars (=0D
+ IN PCI_IO_DEVICE *PciIoDevice=0D
+ );=0D
+=0D
#endif=0D
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci=
/PciBusDxe/PciLib.c
index 72690ab647..6bba283671 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -1,7 +1,7 @@
/** @file=0D
Internal library implementation for PCI Bus module.=0D
=0D
-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>=0D
(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
@@ -377,6 +377,60 @@ DumpResourceMap (
}=0D
}=0D
=0D
+/**=0D
+ Adjust the Devices' BAR size to minimum value if it support Resizeable B=
AR capability.=0D
+=0D
+ @param RootBridgeDev Pointer to instance of PCI_IO_DEVICE..=0D
+=0D
+ @return TRUE if BAR size is adjusted.=0D
+=0D
+**/=0D
+BOOLEAN=0D
+AdjustPciDeviceBarSize (=0D
+ IN PCI_IO_DEVICE *RootBridgeDev=0D
+ )=0D
+{=0D
+ PCI_IO_DEVICE *PciIoDevice;=0D
+ LIST_ENTRY *CurrentLink;=0D
+ BOOLEAN Adjusted;=0D
+ UINTN Offset;=0D
+ UINTN BarIndex;=0D
+=0D
+ Adjusted =3D FALSE;=0D
+ CurrentLink =3D RootBridgeDev->ChildList.ForwardLink;=0D
+=0D
+ while (CurrentLink !=3D NULL && CurrentLink !=3D &RootBridgeDev->ChildLi=
st) {=0D
+ PciIoDevice =3D PCI_IO_DEVICE_FROM_LINK (CurrentLink);=0D
+=0D
+ if (IS_PCI_BRIDGE (&PciIoDevice->Pci)) {=0D
+ if (AdjustPciDeviceBarSize (PciIoDevice)) {=0D
+ Adjusted =3D TRUE;=0D
+ }=0D
+ } else {=0D
+ if (PciIoDevice->ResizableBarOffset !=3D 0) {=0D
+ DEBUG ((=0D
+ DEBUG_ERROR,=0D
+ "PciBus: [%02x|%02x|%02x] Adjust Pci Device Bar Size\n",=0D
+ PciIoDevice->BusNumber, PciIoDevice->DeviceNumber, PciIoDevice->=
FunctionNumber=0D
+ ));=0D
+ PciProgramResizableBar (PciIoDevice, PciResizableBarMin);=0D
+ //=0D
+ // Start to parse the bars=0D
+ //=0D
+ for (Offset =3D 0x10, BarIndex =3D 0; Offset <=3D 0x24 && BarIndex=
< PCI_MAX_BAR; BarIndex++) {=0D
+ Offset =3D PciParseBar (PciIoDevice, Offset, BarIndex);=0D
+ }=0D
+ Adjusted =3D TRUE;=0D
+ DEBUG_CODE (DumpPciBars (PciIoDevice););=0D
+ }=0D
+ }=0D
+=0D
+ CurrentLink =3D CurrentLink->ForwardLink;=0D
+ }=0D
+=0D
+ return Adjusted;=0D
+}=0D
+=0D
/**=0D
Submits the I/O and memory resource requirements for the specified PCI H=
ost Bridge.=0D
=0D
@@ -422,6 +476,10 @@ PciHostBridgeResourceAllocator (
PCI_RESOURCE_NODE PMem64Pool;=0D
EFI_DEVICE_HANDLE_EXTENDED_DATA_PAYLOAD HandleExtendedData;=0D
EFI_RESOURCE_ALLOC_FAILURE_ERROR_DATA_PAYLOAD AllocFailExtendedData;=0D
+ BOOLEAN ResizableBarNeedAdjust;=0D
+ BOOLEAN ResizableBarAdjusted;=0D
+=0D
+ ResizableBarNeedAdjust =3D PcdGetBool (PcdPcieResizableBarSupport);=0D
=0D
//=0D
// It may try several times if the resource allocation fails=0D
@@ -703,19 +761,30 @@ PciHostBridgeResourceAllocator (
sizeof (AllocFailExtendedData)=0D
);=0D
=0D
- Status =3D PciHostBridgeAdjustAllocation (=0D
- &IoPool,=0D
- &Mem32Pool,=0D
- &PMem32Pool,=0D
- &Mem64Pool,=0D
- &PMem64Pool,=0D
- IoResStatus,=0D
- Mem32ResStatus,=0D
- PMem32ResStatus,=0D
- Mem64ResStatus,=0D
- PMem64ResStatus=0D
- );=0D
-=0D
+ //=0D
+ // When resource conflict happens, adjust the BAR size first.=0D
+ // Only when adjusting BAR size doesn't help or BAR size cannot be ad=
justed,=0D
+ // reject the device who requests largest resource that causes confli=
ct.=0D
+ //=0D
+ ResizableBarAdjusted =3D FALSE;=0D
+ if (ResizableBarNeedAdjust) {=0D
+ ResizableBarAdjusted =3D AdjustPciDeviceBarSize (RootBridgeDev);=0D
+ ResizableBarNeedAdjust =3D FALSE;=0D
+ }=0D
+ if (!ResizableBarAdjusted) {=0D
+ Status =3D PciHostBridgeAdjustAllocation (=0D
+ &IoPool,=0D
+ &Mem32Pool,=0D
+ &PMem32Pool,=0D
+ &Mem64Pool,=0D
+ &PMem64Pool,=0D
+ IoResStatus,=0D
+ Mem32ResStatus,=0D
+ PMem32ResStatus,=0D
+ Mem64ResStatus,=0D
+ PMem64ResStatus=0D
+ );=0D
+ }=0D
//=0D
// Destroy all the resource tree=0D
//=0D
@@ -1651,3 +1720,91 @@ PciHostBridgeEnumerator (
=0D
return EFI_SUCCESS;=0D
}=0D
+=0D
+/**=0D
+ This function is used to program the Resizable BAR Register.=0D
+=0D
+ @param PciIoDevice A pointer to the PCI_IO_DEVICE.=0D
+ @param ResizableBarOp PciResizableBarMax: Set BAR to max size=0D
+ PciResizableBarMin: set BAR to min size.=0D
+=0D
+ @retval EFI_SUCCESS Successfully enumerated the host bridge.=0D
+ @retval other Some error occurred when enumerating the h=
ost bridge.=0D
+=0D
+**/=0D
+EFI_STATUS=0D
+PciProgramResizableBar (=0D
+ IN PCI_IO_DEVICE *PciIoDevice,=0D
+ IN PCI_RESIZABLE_BAR_OPERATION ResizableBarOp=0D
+ )=0D
+{=0D
+ EFI_PCI_IO_PROTOCOL *PciIo;=0D
+ UINT64 Capabilities;=0D
+ UINT32 Index;=0D
+ UINT32 Offset;=0D
+ INTN Bit;=0D
+ UINTN ResizableBarNumber;=0D
+ EFI_STATUS Status;=0D
+ PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_ENTRY Entries[PCI_MAX_=
BAR];=0D
+=0D
+ ASSERT (PciIoDevice->ResizableBarOffset !=3D 0);=0D
+=0D
+ DEBUG ((DEBUG_INFO, " Programs Resizable BAR register, offset: 0x%08x,=
number: %d\n",=0D
+ PciIoDevice->ResizableBarOffset, PciIoDevice->ResizableBarNumber))=
;=0D
+=0D
+ ResizableBarNumber =3D MIN (PciIoDevice->ResizableBarNumber, PCI_MAX_BAR=
);=0D
+ PciIo =3D &PciIoDevice->PciIo;=0D
+ Status =3D PciIo->Pci.Read (=0D
+ PciIo,=0D
+ EfiPciIoWidthUint8,=0D
+ PciIoDevice->ResizableBarOffset + sizeof (PCI_EXPRESS_EXTENDED_C=
APABILITIES_HEADER),=0D
+ sizeof (PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_ENTRY) *=
ResizableBarNumber,=0D
+ (VOID *)(&Entries)=0D
+ );=0D
+ ASSERT_EFI_ERROR (Status);=0D
+=0D
+ for (Index =3D 0; Index < ResizableBarNumber; Index++) {=0D
+=0D
+ //=0D
+ // When the bit of Capabilities Set, indicates that the Function suppo=
rts=0D
+ // operating with the BAR sized to (2^Bit) MB.=0D
+ // Example:=0D
+ // Bit 0 is set: supports operating with the BAR sized to 1 MB=0D
+ // Bit 1 is set: supports operating with the BAR sized to 2 MB=0D
+ // Bit n is set: supports operating with the BAR sized to (2^n) MB=0D
+ //=0D
+ Capabilities =3D LShiftU64(Entries[Index].ResizableBarControl.Bits.Bar=
SizeCapability, 28)=0D
+ | Entries[Index].ResizableBarCapability.Bits.BarSizeCapa=
bility;=0D
+=0D
+ if (ResizableBarOp =3D=3D PciResizableBarMax) {=0D
+ Bit =3D HighBitSet64(Capabilities);=0D
+ } else if (ResizableBarOp =3D=3D PciResizableBarMin) {=0D
+ Bit =3D LowBitSet64(Capabilities);=0D
+ } else {=0D
+ ASSERT ((ResizableBarOp =3D=3D PciResizableBarMax) || (ResizableBarO=
p =3D=3D PciResizableBarMin));=0D
+ }=0D
+=0D
+ ASSERT (Bit >=3D 0);=0D
+=0D
+ Offset =3D PciIoDevice->ResizableBarOffset + sizeof (PCI_EXPRESS_EXTEN=
DED_CAPABILITIES_HEADER)=0D
+ + Index * sizeof (PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_=
BAR_ENTRY)=0D
+ + OFFSET_OF (PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_E=
NTRY, ResizableBarControl);=0D
+=0D
+ Entries[Index].ResizableBarControl.Bits.BarSize =3D (UINT32) Bit;=0D
+ DEBUG ((=0D
+ DEBUG_INFO,=0D
+ " Resizable Bar: Offset =3D 0x%x, Bar Size Capability =3D 0x%016lx=
, New Bar Size =3D 0x%lx\n",=0D
+ OFFSET_OF (PCI_TYPE00, Device.Bar[Entries[Index].ResizableBarControl=
.Bits.BarIndex]),=0D
+ Capabilities, LShiftU64 (SIZE_1MB, Bit)=0D
+ ));=0D
+ PciIo->Pci.Write (=0D
+ PciIo,=0D
+ EfiPciIoWidthUint32,=0D
+ Offset,=0D
+ 1,=0D
+ &Entries[Index].ResizableBarControl.Uint32=0D
+ );=0D
+ }=0D
+=0D
+ return EFI_SUCCESS;=0D
+}=0D
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h b/MdeModulePkg/Bus/Pci=
/PciBusDxe/PciLib.h
index 10b435d146..aeec6d6b6d 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h
@@ -1,7 +1,7 @@
/** @file=0D
Internal library declaration for PCI Bus module.=0D
=0D
-Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
@@ -24,6 +24,10 @@ typedef struct {
UINT8 *AllocRes;=0D
} EFI_RESOURCE_ALLOC_FAILURE_ERROR_DATA_PAYLOAD;=0D
=0D
+typedef enum {=0D
+ PciResizableBarMin =3D 0x00,=0D
+ PciResizableBarMax =3D 0xFF=0D
+} PCI_RESIZABLE_BAR_OPERATION;=0D
=0D
/**=0D
Retrieve the PCI Card device BAR information via PciIo interface.=0D
@@ -156,4 +160,20 @@ PciHostBridgeEnumerator (
IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL *PciResAlloc=0D
);=0D
=0D
+/**=0D
+ This function is used to program the Resizable BAR Register.=0D
+=0D
+ @param PciIoDevice A pointer to the PCI_IO_DEVICE.=0D
+ @param ResizableBarOp PciResizableBarMax: Set BAR to max size=0D
+ PciResizableBarMin: set BAR to min size.=0D
+=0D
+ @retval EFI_SUCCESS Successfully enumerated the host bridge.=0D
+ @retval other Some error occurred when enumerating the h=
ost bridge.=0D
+=0D
+**/=0D
+EFI_STATUS=0D
+PciProgramResizableBar (=0D
+ IN PCI_IO_DEVICE *PciIoDevice,=0D
+ IN PCI_RESIZABLE_BAR_OPERATION ResizableBarOp=0D
+ );=0D
#endif=0D
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 9b52b34494..9173fdef83 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -4,7 +4,7 @@
# and libraries instances, which are used for those modules.=0D
#=0D
# Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.=0D
-# Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>=0D
+# Copyright (c) 2007 - 2021, Intel Corporation. All rights reserved.<BR>=0D
# Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>=0D
# (C) Copyright 2016 - 2019 Hewlett Packard Enterprise Development LP<BR>=
=0D
# Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>=0D
@@ -2043,6 +2043,12 @@
# @Prompt Enable StatusCode via memory.=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|FALSE|BOOLEAN|0x00=
010023=0D
=0D
+ ## Indicates if the PCIe Resizable BAR Capability Supported.<BR><BR>=0D
+ # TRUE - PCIe Resizable BAR Capability is supported.<BR>=0D
+ # FALSE - PCIe Resizable BAR Capability is not supported.<BR>=0D
+ # @Prompt Enable PCIe Resizable BAR Capability support.=0D
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport|TRUE|BOOLEAN|0=
x10000024=0D
+=0D
[PcdsPatchableInModule]=0D
## Specify memory size with page number for PEI code when=0D
# Loading Module at Fixed Address feature is enabled.=0D
--=20
2.24.0.windows.2


Ni, Ray
 

Reviewed-by: Ray Ni <ray.ni@intel.com>

-----Original Message-----
From: Luo, Heng <heng.luo@intel.com>
Sent: Monday, January 4, 2021 3:00 PM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe: Support PCIe
Resizable BAR Capability

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=313

Add PcdPcieResizableBarSupport to enable/disable PCIe Resizable
BAR Capability fearture.
Program the Resizable BAR Register if the device suports PCIe Resizable
BAR Capability and PcdPcieResizableBarSupport is TRUE.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Heng Luo <heng.luo@intel.com>
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 4 +++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 3 ++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 27
++++++++++++++++++++++++++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h | 12
+++++++++++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 185
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+++++++++++++++++++++++++++++++++++++++++++--------------
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h | 22
+++++++++++++++++++++-
MdeModulePkg/MdeModulePkg.dec | 8 +++++++-
7 files changed, 241 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
index d4113993c8..a619a68526 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
@@ -1,7 +1,7 @@
/** @file

Header files and data structures needed by PCI Bus module.



-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>

+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent



**/

@@ -280,6 +280,8 @@ struct _PCI_IO_DEVICE {
// This field is used to support this case.

//

UINT16 BridgeIoAlignment;

+ UINT32 ResizableBarOffset;

+ UINT32 ResizableBarNumber;

};



#define PCI_IO_DEVICE_FROM_PCI_IO_THIS(a) \

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
index 9284998f36..e317169d9c 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
@@ -2,7 +2,7 @@
# The PCI bus driver will probe all PCI devices and allocate MMIO and IO
space for these devices.

# Please use PCD feature flag PcdPciBusHotplugDeviceSupport to enable hot
plug supporting.

#

-# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>

+# Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>

#

# SPDX-License-Identifier: BSD-2-Clause-Patent

#

@@ -106,6 +106,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## CONSUMES

gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport ##
CONSUMES

gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration ##
SOMETIMES_CONSUMES

+ gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport ##
CONSUMES



[UserExtensions.TianoCore."ExtraFiles"]

PciBusDxeExtra.uni

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index 6c68a97d4e..1b64924b7b 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -1,7 +1,7 @@
/** @file

PCI emumeration support functions implementation for PCI Bus module.



-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>

+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>

(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent



@@ -2426,6 +2426,31 @@ CreatePciIoDevice (
}

}



+ PciIoDevice->ResizableBarOffset = 0;

+ if (PcdGetBool (PcdPcieResizableBarSupport)) {

+ Status = LocatePciExpressCapabilityRegBlock (

+ PciIoDevice,

+ PCI_EXPRESS_EXTENDED_CAPABILITY_RESIZABLE_BAR_ID,

+ &PciIoDevice->ResizableBarOffset,

+ NULL

+ );

+ if (!EFI_ERROR (Status)) {

+ PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_CONTROL
ResizableBarControl;

+ UINT32 Offset;

+ Offset = PciIoDevice->ResizableBarOffset + sizeof
(PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER)

+ + sizeof
(PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_CAPABILITY),

+ PciIo->Pci.Read (

+ PciIo,

+ EfiPciIoWidthUint8,

+ Offset,

+ sizeof
(PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_CONTROL),

+ &ResizableBarControl

+ );

+ PciIoDevice->ResizableBarNumber =
ResizableBarControl.Bits.ResizableBarNumber;

+ PciProgramResizableBar (PciIoDevice, PciResizableBarMax);

+ }

+ }

+

//

// Initialize the reserved resource list

//

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
index d76606c7df..4581b270c9 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
@@ -1,7 +1,7 @@
/** @file

PCI enumeration support functions declaration for PCI Bus module.



-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>

+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent



**/

@@ -467,4 +467,14 @@ DumpPpbPaddingResource (
IN PCI_BAR_TYPE ResourceType

);



+/**

+ Dump the PCI BAR information.

+

+ @param PciIoDevice PCI IO instance.

+**/

+VOID

+DumpPciBars (

+ IN PCI_IO_DEVICE *PciIoDevice

+ );

+

#endif

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index 72690ab647..6bba283671 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -1,7 +1,7 @@
/** @file

Internal library implementation for PCI Bus module.



-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>

+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>

(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent



@@ -377,6 +377,60 @@ DumpResourceMap (
}

}



+/**

+ Adjust the Devices' BAR size to minimum value if it support Resizeable BAR
capability.

+

+ @param RootBridgeDev Pointer to instance of PCI_IO_DEVICE..

+

+ @return TRUE if BAR size is adjusted.

+

+**/

+BOOLEAN

+AdjustPciDeviceBarSize (

+ IN PCI_IO_DEVICE *RootBridgeDev

+ )

+{

+ PCI_IO_DEVICE *PciIoDevice;

+ LIST_ENTRY *CurrentLink;

+ BOOLEAN Adjusted;

+ UINTN Offset;

+ UINTN BarIndex;

+

+ Adjusted = FALSE;

+ CurrentLink = RootBridgeDev->ChildList.ForwardLink;

+

+ while (CurrentLink != NULL && CurrentLink != &RootBridgeDev->ChildList) {

+ PciIoDevice = PCI_IO_DEVICE_FROM_LINK (CurrentLink);

+

+ if (IS_PCI_BRIDGE (&PciIoDevice->Pci)) {

+ if (AdjustPciDeviceBarSize (PciIoDevice)) {

+ Adjusted = TRUE;

+ }

+ } else {

+ if (PciIoDevice->ResizableBarOffset != 0) {

+ DEBUG ((

+ DEBUG_ERROR,

+ "PciBus: [%02x|%02x|%02x] Adjust Pci Device Bar Size\n",

+ PciIoDevice->BusNumber, PciIoDevice->DeviceNumber, PciIoDevice-
FunctionNumber
+ ));

+ PciProgramResizableBar (PciIoDevice, PciResizableBarMin);

+ //

+ // Start to parse the bars

+ //

+ for (Offset = 0x10, BarIndex = 0; Offset <= 0x24 && BarIndex <
PCI_MAX_BAR; BarIndex++) {

+ Offset = PciParseBar (PciIoDevice, Offset, BarIndex);

+ }

+ Adjusted = TRUE;

+ DEBUG_CODE (DumpPciBars (PciIoDevice););

+ }

+ }

+

+ CurrentLink = CurrentLink->ForwardLink;

+ }

+

+ return Adjusted;

+}

+

/**

Submits the I/O and memory resource requirements for the specified PCI
Host Bridge.



@@ -422,6 +476,10 @@ PciHostBridgeResourceAllocator (
PCI_RESOURCE_NODE PMem64Pool;

EFI_DEVICE_HANDLE_EXTENDED_DATA_PAYLOAD HandleExtendedData;

EFI_RESOURCE_ALLOC_FAILURE_ERROR_DATA_PAYLOAD
AllocFailExtendedData;

+ BOOLEAN ResizableBarNeedAdjust;

+ BOOLEAN ResizableBarAdjusted;

+

+ ResizableBarNeedAdjust = PcdGetBool (PcdPcieResizableBarSupport);



//

// It may try several times if the resource allocation fails

@@ -703,19 +761,30 @@ PciHostBridgeResourceAllocator (
sizeof (AllocFailExtendedData)

);



- Status = PciHostBridgeAdjustAllocation (

- &IoPool,

- &Mem32Pool,

- &PMem32Pool,

- &Mem64Pool,

- &PMem64Pool,

- IoResStatus,

- Mem32ResStatus,

- PMem32ResStatus,

- Mem64ResStatus,

- PMem64ResStatus

- );

-

+ //

+ // When resource conflict happens, adjust the BAR size first.

+ // Only when adjusting BAR size doesn't help or BAR size cannot be
adjusted,

+ // reject the device who requests largest resource that causes conflict.

+ //

+ ResizableBarAdjusted = FALSE;

+ if (ResizableBarNeedAdjust) {

+ ResizableBarAdjusted = AdjustPciDeviceBarSize (RootBridgeDev);

+ ResizableBarNeedAdjust = FALSE;

+ }

+ if (!ResizableBarAdjusted) {

+ Status = PciHostBridgeAdjustAllocation (

+ &IoPool,

+ &Mem32Pool,

+ &PMem32Pool,

+ &Mem64Pool,

+ &PMem64Pool,

+ IoResStatus,

+ Mem32ResStatus,

+ PMem32ResStatus,

+ Mem64ResStatus,

+ PMem64ResStatus

+ );

+ }

//

// Destroy all the resource tree

//

@@ -1651,3 +1720,91 @@ PciHostBridgeEnumerator (


return EFI_SUCCESS;

}

+

+/**

+ This function is used to program the Resizable BAR Register.

+

+ @param PciIoDevice A pointer to the PCI_IO_DEVICE.

+ @param ResizableBarOp PciResizableBarMax: Set BAR to max size

+ PciResizableBarMin: set BAR to min size.

+

+ @retval EFI_SUCCESS Successfully enumerated the host bridge.

+ @retval other Some error occurred when enumerating the host
bridge.

+

+**/

+EFI_STATUS

+PciProgramResizableBar (

+ IN PCI_IO_DEVICE *PciIoDevice,

+ IN PCI_RESIZABLE_BAR_OPERATION ResizableBarOp

+ )

+{

+ EFI_PCI_IO_PROTOCOL *PciIo;

+ UINT64 Capabilities;

+ UINT32 Index;

+ UINT32 Offset;

+ INTN Bit;

+ UINTN ResizableBarNumber;

+ EFI_STATUS Status;

+ PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_ENTRY
Entries[PCI_MAX_BAR];

+

+ ASSERT (PciIoDevice->ResizableBarOffset != 0);

+

+ DEBUG ((DEBUG_INFO, " Programs Resizable BAR register, offset: 0x%08x,
number: %d\n",

+ PciIoDevice->ResizableBarOffset, PciIoDevice->ResizableBarNumber));

+

+ ResizableBarNumber = MIN (PciIoDevice->ResizableBarNumber,
PCI_MAX_BAR);

+ PciIo = &PciIoDevice->PciIo;

+ Status = PciIo->Pci.Read (

+ PciIo,

+ EfiPciIoWidthUint8,

+ PciIoDevice->ResizableBarOffset + sizeof
(PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER),

+ sizeof (PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_ENTRY)
* ResizableBarNumber,

+ (VOID *)(&Entries)

+ );

+ ASSERT_EFI_ERROR (Status);

+

+ for (Index = 0; Index < ResizableBarNumber; Index++) {

+

+ //

+ // When the bit of Capabilities Set, indicates that the Function supports

+ // operating with the BAR sized to (2^Bit) MB.

+ // Example:

+ // Bit 0 is set: supports operating with the BAR sized to 1 MB

+ // Bit 1 is set: supports operating with the BAR sized to 2 MB

+ // Bit n is set: supports operating with the BAR sized to (2^n) MB

+ //

+ Capabilities =
LShiftU64(Entries[Index].ResizableBarControl.Bits.BarSizeCapability, 28)

+ | Entries[Index].ResizableBarCapability.Bits.BarSizeCapability;

+

+ if (ResizableBarOp == PciResizableBarMax) {

+ Bit = HighBitSet64(Capabilities);

+ } else if (ResizableBarOp == PciResizableBarMin) {

+ Bit = LowBitSet64(Capabilities);

+ } else {

+ ASSERT ((ResizableBarOp == PciResizableBarMax) || (ResizableBarOp ==
PciResizableBarMin));

+ }

+

+ ASSERT (Bit >= 0);

+

+ Offset = PciIoDevice->ResizableBarOffset + sizeof
(PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER)

+ + Index * sizeof
(PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_ENTRY)

+ + OFFSET_OF
(PCI_EXPRESS_EXTENDED_CAPABILITIES_RESIZABLE_BAR_ENTRY,
ResizableBarControl);

+

+ Entries[Index].ResizableBarControl.Bits.BarSize = (UINT32) Bit;

+ DEBUG ((

+ DEBUG_INFO,

+ " Resizable Bar: Offset = 0x%x, Bar Size Capability = 0x%016lx, New Bar
Size = 0x%lx\n",

+ OFFSET_OF (PCI_TYPE00,
Device.Bar[Entries[Index].ResizableBarControl.Bits.BarIndex]),

+ Capabilities, LShiftU64 (SIZE_1MB, Bit)

+ ));

+ PciIo->Pci.Write (

+ PciIo,

+ EfiPciIoWidthUint32,

+ Offset,

+ 1,

+ &Entries[Index].ResizableBarControl.Uint32

+ );

+ }

+

+ return EFI_SUCCESS;

+}

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h
index 10b435d146..aeec6d6b6d 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h
@@ -1,7 +1,7 @@
/** @file

Internal library declaration for PCI Bus module.



-Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR>

+Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent



**/

@@ -24,6 +24,10 @@ typedef struct {
UINT8 *AllocRes;

} EFI_RESOURCE_ALLOC_FAILURE_ERROR_DATA_PAYLOAD;



+typedef enum {

+ PciResizableBarMin = 0x00,

+ PciResizableBarMax = 0xFF

+} PCI_RESIZABLE_BAR_OPERATION;



/**

Retrieve the PCI Card device BAR information via PciIo interface.

@@ -156,4 +160,20 @@ PciHostBridgeEnumerator (
IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL
*PciResAlloc

);



+/**

+ This function is used to program the Resizable BAR Register.

+

+ @param PciIoDevice A pointer to the PCI_IO_DEVICE.

+ @param ResizableBarOp PciResizableBarMax: Set BAR to max size

+ PciResizableBarMin: set BAR to min size.

+

+ @retval EFI_SUCCESS Successfully enumerated the host bridge.

+ @retval other Some error occurred when enumerating the host
bridge.

+

+**/

+EFI_STATUS

+PciProgramResizableBar (

+ IN PCI_IO_DEVICE *PciIoDevice,

+ IN PCI_RESIZABLE_BAR_OPERATION ResizableBarOp

+ );

#endif

diff --git a/MdeModulePkg/MdeModulePkg.dec
b/MdeModulePkg/MdeModulePkg.dec
index 9b52b34494..9173fdef83 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -4,7 +4,7 @@
# and libraries instances, which are used for those modules.

#

# Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.

-# Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>

+# Copyright (c) 2007 - 2021, Intel Corporation. All rights reserved.<BR>

# Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>

# (C) Copyright 2016 - 2019 Hewlett Packard Enterprise Development LP<BR>

# Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>

@@ -2043,6 +2043,12 @@
# @Prompt Enable StatusCode via memory.


gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|FALSE|BOOLE
AN|0x00010023



+ ## Indicates if the PCIe Resizable BAR Capability Supported.<BR><BR>

+ # TRUE - PCIe Resizable BAR Capability is supported.<BR>

+ # FALSE - PCIe Resizable BAR Capability is not supported.<BR>

+ # @Prompt Enable PCIe Resizable BAR Capability support.

+
gEfiMdeModulePkgTokenSpaceGuid.PcdPcieResizableBarSupport|TRUE|BOOLE
AN|0x10000024

+

[PcdsPatchableInModule]

## Specify memory size with page number for PEI code when

# Loading Module at Fixed Address feature is enabled.

--
2.24.0.windows.2


Laszlo Ersek
 

Hi,

On 01/04/21 07:59, Heng Luo wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=313

Add PcdPcieResizableBarSupport to enable/disable PCIe Resizable
BAR Capability fearture.
Program the Resizable BAR Register if the device suports PCIe
Resizable BAR Capability and PcdPcieResizableBarSupport is TRUE.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Heng Luo <heng.luo@intel.com>
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 4 +++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 3 ++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 27 ++++++++++++++++++++++++++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h | 12 +++++++++++-
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.h | 22 +++++++++++++++++++++-
MdeModulePkg/MdeModulePkg.dec | 8 +++++++-
7 files changed, 241 insertions(+), 20 deletions(-)
I have brought this feature to the attention of our PCI experts.

First of all, let me say that the bugzilla ticket (3138), and the commit
message on this patch, are utterly useless. (To add insult to injury,
the bugzilla reference in the commit message even has a typo -- it
misses the last digit "8".)

So, given the lack of information in the bugzilla and the commit
message, I could only attempt to decipher the goal / use case myself.
This was my interpretation:

It seems like the max BAR size is selected first, but if there's a
"resource conflict" (running out of a particular resource type
aperture), then the minimum BAR size is selected. I don't know what
set of devices and/or resizable BARs this logic applies to, if there
are multiple of them.
*IF* my interpretation of the code is correct -- which is admittedly a
"big if" --, then the logic seems misguided. This is the feedback I got
internally:

Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the control
register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor should the
minimum size be considered the default. In fact, [we] tested various
handoff BAR sizes for [a particular] GPU and found that Windows didn't
like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel support
for resizeable BARs indicates that FPGA devices might implement the
REBAR capability as part of their standard PCI wrapper ([our]
interpretation), but the BAR usage would be determined by the actual
bitstream written to the device, therefore there might be a full
bitmask for the BAR sizes supported by the device.

[1] https://lists.freedesktop.org/archives/dri-devel/2021-January/thread.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of assumptions
about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default, smaller
compatibility size expanding to some representation that allows direct
DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the default value
of "PcdPcieResizableBarSupport" should be set to FALSE, as the policy
for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of policy in
core edk2, please at least *document* the policy somewhere. It's
unacceptable to have to decipher the source code for such a possibly
impactful change in the core. There is no need for a wiki page or an
RFC, but a sane bugzilla ticket and a sane commit message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)

Thanks
Laszlo


Laszlo Ersek
 

Hi Heng,

On 01/11/21 20:38, Laszlo Ersek wrote:

General request for the future: if you implement some kind of policy in
core edk2, please at least *document* the policy somewhere. It's
unacceptable to have to decipher the source code for such a possibly
impactful change in the core. There is no need for a wiki page or an
RFC, but a sane bugzilla ticket and a sane commit message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)
Wow, I've even found the following regression in the bug tracker:

https://bugzilla.tianocore.org/show_bug.cgi?id=3158

Your approach in

https://bugzilla.tianocore.org/show_bug.cgi?id=3158#c1

is entirely wrong. It is not the burden of platform owners to fix up the
regression you have introduced. It is your responsibility to *not break*
existent use cases. You can offer the feature, and platforms that want
the feature may enable it (or you can enable it by default if you can
prove or test that no platform breaks in response).

Please revert this patch immediately, or flip the PCD default to FALSE
immediately.

Laszlo


Ni, Ray
 

It seems like the max BAR size is selected first, but if there's a
"resource conflict" (running out of a particular resource type
aperture), then the minimum BAR size is selected. I don't know what
set of devices and/or resizable BARs this logic applies to, if there
are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the control
register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor should the
minimum size be considered the default. In fact, [we] tested various
handoff BAR sizes for [a particular] GPU and found that Windows didn't
like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel support
for resizeable BARs indicates that FPGA devices might implement the
REBAR capability as part of their standard PCI wrapper ([our]
interpretation), but the BAR usage would be determined by the actual
bitstream written to the device, therefore there might be a full
bitmask for the BAR sizes supported by the device.

[1] https://lists.freedesktop.org/archives/dri-devel/2021-January/thread.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of assumptions
about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default, smaller
compatibility size expanding to some representation that allows direct
DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the default value
of "PcdPcieResizableBarSupport" should be set to FALSE, as the policy
for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of policy in
core edk2, please at least *document* the policy somewhere. It's
unacceptable to have to decipher the source code for such a possibly
impactful change in the core. There is no need for a wiki page or an
RFC, but a sane bugzilla ticket and a sane commit message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)


Your understanding is correct. Original idea is to let platform supply the policy about
what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code for such policy because
we thought it's a burden for platform to supply the policy data.

I agree that we set the PCD default value as disabled and after a period of study, we will
understand whether a platform policy is really needed.

Thanks,
Ray


Heng Luo
 

Thank Ray and Laszlo! I have sent 2 patches for this, please help to review.
https://edk2.groups.io/g/devel/message/70139
https://edk2.groups.io/g/devel/message/70140

Thanks,
Heng

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, January 12, 2021 10:28 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo, Heng
<heng.luo@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

It seems like the max BAR size is selected first, but if there's a
"resource conflict" (running out of a particular resource type
aperture), then the minimum BAR size is selected. I don't know what
set of devices and/or resizable BARs this logic applies to, if there
are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the control
register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor should
the minimum size be considered the default. In fact, [we] tested
various handoff BAR sizes for [a particular] GPU and found that
Windows didn't like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel support
for resizeable BARs indicates that FPGA devices might implement the
REBAR capability as part of their standard PCI wrapper ([our]
interpretation), but the BAR usage would be determined by the actual
bitstream written to the device, therefore there might be a full
bitmask for the BAR sizes supported by the device.

[1]
https://lists.freedesktop.org/archives/dri-devel/2021-January/thread
.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of assumptions
about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default, smaller
compatibility size expanding to some representation that allows
direct DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the default
value of "PcdPcieResizableBarSupport" should be set to FALSE, as the
policy for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of policy
in core edk2, please at least *document* the policy somewhere. It's
unacceptable to have to decipher the source code for such a possibly
impactful change in the core. There is no need for a wiki page or an
RFC, but a sane bugzilla ticket and a sane commit message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)


Your understanding is correct. Original idea is to let platform supply the
policy about what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code for such
policy because we thought it's a burden for platform to supply the policy
data.

I agree that we set the PCD default value as disabled and after a period of
study, we will understand whether a platform policy is really needed.

Thanks,
Ray


Wu, Hao A
 

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, January 12, 2021 10:28 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo, Heng
<heng.luo@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

It seems like the max BAR size is selected first, but if there's a
"resource conflict" (running out of a particular resource type
aperture), then the minimum BAR size is selected. I don't know what
set of devices and/or resizable BARs this logic applies to, if there
are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the control
register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor should
the minimum size be considered the default. In fact, [we] tested
various handoff BAR sizes for [a particular] GPU and found that
Windows didn't like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel support
for resizeable BARs indicates that FPGA devices might implement the
REBAR capability as part of their standard PCI wrapper ([our]
interpretation), but the BAR usage would be determined by the actual
bitstream written to the device, therefore there might be a full
bitmask for the BAR sizes supported by the device.

[1]
https://lists.freedesktop.org/archives/dri-devel/2021-January/thread
.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of assumptions
about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default, smaller
compatibility size expanding to some representation that allows
direct DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the default
value of "PcdPcieResizableBarSupport" should be set to FALSE, as the
policy for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of policy
in core edk2, please at least *document* the policy somewhere. It's
unacceptable to have to decipher the source code for such a possibly
impactful change in the core. There is no need for a wiki page or an
RFC, but a sane bugzilla ticket and a sane commit message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)


Your understanding is correct. Original idea is to let platform supply the policy
about what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code for such
policy because we thought it's a burden for platform to supply the policy data.

I agree that we set the PCD default value as disabled and after a period of
study, we will understand whether a platform policy is really needed.

Hello Laszlo and Ray,

I saw Heng's patch series to
1) Set the PCD default value to FALSE: https://edk2.groups.io/g/devel/message/70139
2) Update the UNI file: https://edk2.groups.io/g/devel/message/70140
has got Reviewed-by/Acked-by tags from reviewers.

Do you have further comments for the series?
If not, I will merge this change in the next 24 hours.

Thanks in advance.

Best Regards,
Hao Wu



Thanks,
Ray


Ni, Ray
 

I've given R-b to the two patches. No comments from my side.

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Wednesday, January 13, 2021 2:00 PM
To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, January 12, 2021 10:28 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo, Heng
<heng.luo@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

It seems like the max BAR size is selected first, but if there's a
"resource conflict" (running out of a particular resource type
aperture), then the minimum BAR size is selected. I don't know what
set of devices and/or resizable BARs this logic applies to, if there
are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the control
register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor should
the minimum size be considered the default. In fact, [we] tested
various handoff BAR sizes for [a particular] GPU and found that
Windows didn't like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel support
for resizeable BARs indicates that FPGA devices might implement the
REBAR capability as part of their standard PCI wrapper ([our]
interpretation), but the BAR usage would be determined by the actual
bitstream written to the device, therefore there might be a full
bitmask for the BAR sizes supported by the device.

[1]
https://lists.freedesktop.org/archives/dri-devel/2021-January/thread
.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of assumptions
about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default, smaller
compatibility size expanding to some representation that allows
direct DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the default
value of "PcdPcieResizableBarSupport" should be set to FALSE, as the
policy for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of policy
in core edk2, please at least *document* the policy somewhere. It's
unacceptable to have to decipher the source code for such a possibly
impactful change in the core. There is no need for a wiki page or an
RFC, but a sane bugzilla ticket and a sane commit message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)


Your understanding is correct. Original idea is to let platform supply the
policy
about what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code for such
policy because we thought it's a burden for platform to supply the policy data.

I agree that we set the PCD default value as disabled and after a period of
study, we will understand whether a platform policy is really needed.

Hello Laszlo and Ray,

I saw Heng's patch series to
1) Set the PCD default value to FALSE:
https://edk2.groups.io/g/devel/message/70139
2) Update the UNI file: https://edk2.groups.io/g/devel/message/70140
has got Reviewed-by/Acked-by tags from reviewers.

Do you have further comments for the series?
If not, I will merge this change in the next 24 hours.

Thanks in advance.

Best Regards,
Hao Wu



Thanks,
Ray


Heng Luo
 

Hi Hao,
Please hold on merging patch now, we are still waiting for some inputs, I will let you know when we reach agreement.

Thanks,
Heng

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Wednesday, January 13, 2021 2:07 PM
To: Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

I've given R-b to the two patches. No comments from my side.

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Wednesday, January 13, 2021 2:00 PM
To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, January 12, 2021 10:28 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo,
Heng <heng.luo@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

It seems like the max BAR size is selected first, but if there's
a "resource conflict" (running out of a particular resource type
aperture), then the minimum BAR size is selected. I don't know
what set of devices and/or resizable BARs this logic applies to,
if there are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the
control register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor
should the minimum size be considered the default. In fact,
[we] tested various handoff BAR sizes for [a particular] GPU and
found that Windows didn't like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel
support for resizeable BARs indicates that FPGA devices might
implement the REBAR capability as part of their standard PCI
wrapper ([our] interpretation), but the BAR usage would be
determined by the actual bitstream written to the device,
therefore there might be a full bitmask for the BAR sizes supported
by the device.

[1]
https://lists.freedesktop.org/archives/dri-devel/2021-January/th
read
.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of
assumptions about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default,
smaller compatibility size expanding to some representation that
allows direct DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the default
value of "PcdPcieResizableBarSupport" should be set to FALSE, as
the policy for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of
policy in core edk2, please at least *document* the policy
somewhere. It's unacceptable to have to decipher the source code
for such a possibly impactful change in the core. There is no need
for a wiki page or an RFC, but a sane bugzilla ticket and a sane commit
message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)


Your understanding is correct. Original idea is to let platform
supply the
policy
about what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code
for such policy because we thought it's a burden for platform to supply
the policy data.

I agree that we set the PCD default value as disabled and after a
period of study, we will understand whether a platform policy is really
needed.


Hello Laszlo and Ray,

I saw Heng's patch series to
1) Set the PCD default value to FALSE:
https://edk2.groups.io/g/devel/message/70139
2) Update the UNI file: https://edk2.groups.io/g/devel/message/70140
has got Reviewed-by/Acked-by tags from reviewers.

Do you have further comments for the series?
If not, I will merge this change in the next 24 hours.

Thanks in advance.

Best Regards,
Hao Wu



Thanks,
Ray


Wu, Hao A
 

-----Original Message-----
From: Luo, Heng <heng.luo@intel.com>
Sent: Wednesday, January 13, 2021 2:15 PM
To: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Laszlo
Ersek <lersek@redhat.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

Hi Hao,
Please hold on merging patch now, we are still waiting for some inputs, I will
let you know when we reach agreement.

Got it. Thanks Heng.

Best Regards,
Hao Wu



Thanks,
Heng

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Wednesday, January 13, 2021 2:07 PM
To: Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

I've given R-b to the two patches. No comments from my side.

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Wednesday, January 13, 2021 2:00 PM
To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, January 12, 2021 10:28 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo,
Heng <heng.luo@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

It seems like the max BAR size is selected first, but if
there's a "resource conflict" (running out of a particular
resource type aperture), then the minimum BAR size is
selected. I don't know what set of devices and/or resizable
BARs this logic applies to, if there are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the
control register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor
should the minimum size be considered the default. In fact,
[we] tested various handoff BAR sizes for [a particular] GPU
and found that Windows didn't like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel
support for resizeable BARs indicates that FPGA devices might
implement the REBAR capability as part of their standard PCI
wrapper ([our] interpretation), but the BAR usage would be
determined by the actual bitstream written to the device,
therefore there might be a full bitmask for the BAR sizes
supported
by the device.

[1]
https://lists.freedesktop.org/archives/dri-devel/2021-January/
th
read
.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of
assumptions about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default,
smaller compatibility size expanding to some representation
that allows direct DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the
default value of "PcdPcieResizableBarSupport" should be set to
FALSE, as the policy for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of
policy in core edk2, please at least *document* the policy
somewhere. It's unacceptable to have to decipher the source code
for such a possibly impactful change in the core. There is no
need for a wiki page or an RFC, but a sane bugzilla ticket and a
sane commit
message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at
all.)


Your understanding is correct. Original idea is to let platform
supply the
policy
about what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code
for such policy because we thought it's a burden for platform to
supply
the policy data.

I agree that we set the PCD default value as disabled and after a
period of study, we will understand whether a platform policy is
really
needed.


Hello Laszlo and Ray,

I saw Heng's patch series to
1) Set the PCD default value to FALSE:
https://edk2.groups.io/g/devel/message/70139
2) Update the UNI file:
https://edk2.groups.io/g/devel/message/70140
has got Reviewed-by/Acked-by tags from reviewers.

Do you have further comments for the series?
If not, I will merge this change in the next 24 hours.

Thanks in advance.

Best Regards,
Hao Wu



Thanks,
Ray


Laszlo Ersek
 

On 01/13/21 07:15, Luo, Heng wrote:
Hi Hao,
Please hold on merging patch now, we are still waiting for some inputs, I will let you know when we reach agreement.
I disagree with waiting. The original patch caused a regression. The
currently pending patch fixes the regression. Any further input
("agreement") should be processed *after* we have mitigated the regression.

The tree is currently in a wrong state. The fix has been reviewed. Hao,
please proceed with merging the fix as soon as you can.

Thanks
Laszlo


Thanks,
Heng

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Wednesday, January 13, 2021 2:07 PM
To: Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

I've given R-b to the two patches. No comments from my side.

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Wednesday, January 13, 2021 2:00 PM
To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, January 12, 2021 10:28 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo,
Heng <heng.luo@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

It seems like the max BAR size is selected first, but if there's
a "resource conflict" (running out of a particular resource type
aperture), then the minimum BAR size is selected. I don't know
what set of devices and/or resizable BARs this logic applies to,
if there are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the
control register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor
should the minimum size be considered the default. In fact,
[we] tested various handoff BAR sizes for [a particular] GPU and
found that Windows didn't like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel
support for resizeable BARs indicates that FPGA devices might
implement the REBAR capability as part of their standard PCI
wrapper ([our] interpretation), but the BAR usage would be
determined by the actual bitstream written to the device,
therefore there might be a full bitmask for the BAR sizes supported
by the device.

[1]
https://lists.freedesktop.org/archives/dri-devel/2021-January/th
read
.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of
assumptions about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default,
smaller compatibility size expanding to some representation that
allows direct DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the default
value of "PcdPcieResizableBarSupport" should be set to FALSE, as
the policy for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of
policy in core edk2, please at least *document* the policy
somewhere. It's unacceptable to have to decipher the source code
for such a possibly impactful change in the core. There is no need
for a wiki page or an RFC, but a sane bugzilla ticket and a sane commit
message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)


Your understanding is correct. Original idea is to let platform
supply the
policy
about what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code
for such policy because we thought it's a burden for platform to supply
the policy data.

I agree that we set the PCD default value as disabled and after a
period of study, we will understand whether a platform policy is really
needed.


Hello Laszlo and Ray,

I saw Heng's patch series to
1) Set the PCD default value to FALSE:
https://edk2.groups.io/g/devel/message/70139
2) Update the UNI file: https://edk2.groups.io/g/devel/message/70140
has got Reviewed-by/Acked-by tags from reviewers.

Do you have further comments for the series?
If not, I will merge this change in the next 24 hours.

Thanks in advance.

Best Regards,
Hao Wu



Thanks,
Ray


Heng Luo
 

Hi Laszlo,
Sorry for the delay. I have gotten the feedback, we can merged them now.

Hi Hao,
Could you help to merge the patches.

Thanks,
Heng

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, January 13, 2021 5:09 PM
To: Luo, Heng <heng.luo@intel.com>; Ni, Ray <ray.ni@intel.com>; Wu, Hao A
<hao.a.wu@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

On 01/13/21 07:15, Luo, Heng wrote:
Hi Hao,
Please hold on merging patch now, we are still waiting for some inputs, I
will let you know when we reach agreement.

I disagree with waiting. The original patch caused a regression. The currently
pending patch fixes the regression. Any further input
("agreement") should be processed *after* we have mitigated the regression.

The tree is currently in a wrong state. The fix has been reviewed. Hao, please
proceed with merging the fix as soon as you can.

Thanks
Laszlo


Thanks,
Heng

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Wednesday, January 13, 2021 2:07 PM
To: Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

I've given R-b to the two patches. No comments from my side.

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Wednesday, January 13, 2021 2:00 PM
To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, January 12, 2021 10:28 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo,
Heng <heng.luo@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

It seems like the max BAR size is selected first, but if there's
a "resource conflict" (running out of a particular resource type
aperture), then the minimum BAR size is selected. I don't know
what set of devices and/or resizable BARs this logic applies to,
if there are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what the
optimal size is for the resource, and programs that size via the BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the
control register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor should
the minimum size be considered the default. In fact, [we] tested
various handoff BAR sizes for [a particular] GPU and found that
Windows didn't like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel
support for resizeable BARs indicates that FPGA devices might
implement the REBAR capability as part of their standard PCI
wrapper ([our] interpretation), but the BAR usage would be
determined by the actual bitstream written to the device,
therefore there might be a full bitmask for the BAR sizes
supported
by the device.

[1]
https://lists.freedesktop.org/archives/dri-devel/2021-January/th
read
.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of
assumptions about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default,
smaller compatibility size expanding to some representation that
allows direct DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the default
value of "PcdPcieResizableBarSupport" should be set to FALSE, as
the policy for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of
policy in core edk2, please at least *document* the policy
somewhere. It's unacceptable to have to decipher the source code
for such a possibly impactful change in the core. There is no need
for a wiki page or an RFC, but a sane bugzilla ticket and a sane
commit
message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at all.)


Your understanding is correct. Original idea is to let platform
supply the
policy
about what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code
for such policy because we thought it's a burden for platform to
supply
the policy data.

I agree that we set the PCD default value as disabled and after a
period of study, we will understand whether a platform policy is
really
needed.


Hello Laszlo and Ray,

I saw Heng's patch series to
1) Set the PCD default value to FALSE:
https://edk2.groups.io/g/devel/message/70139
2) Update the UNI file:
https://edk2.groups.io/g/devel/message/70140
has got Reviewed-by/Acked-by tags from reviewers.

Do you have further comments for the series?
If not, I will merge this change in the next 24 hours.

Thanks in advance.

Best Regards,
Hao Wu



Thanks,
Ray




Wu, Hao A
 

Sure, I will create PR to merge the series.

Best Regards,
Hao Wu

-----Original Message-----
From: Luo, Heng <heng.luo@intel.com>
Sent: Thursday, January 14, 2021 8:45 AM
To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>;
Wu, Hao A <hao.a.wu@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2] MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

Hi Laszlo,
Sorry for the delay. I have gotten the feedback, we can merged them now.

Hi Hao,
Could you help to merge the patches.

Thanks,
Heng

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, January 13, 2021 5:09 PM
To: Luo, Heng <heng.luo@intel.com>; Ni, Ray <ray.ni@intel.com>; Wu,
Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

On 01/13/21 07:15, Luo, Heng wrote:
Hi Hao,
Please hold on merging patch now, we are still waiting for some
inputs, I
will let you know when we reach agreement.

I disagree with waiting. The original patch caused a regression. The
currently pending patch fixes the regression. Any further input
("agreement") should be processed *after* we have mitigated the
regression.

The tree is currently in a wrong state. The fix has been reviewed.
Hao, please proceed with merging the fix as soon as you can.

Thanks
Laszlo


Thanks,
Heng

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Wednesday, January 13, 2021 2:07 PM
To: Wu, Hao A <hao.a.wu@intel.com>; Laszlo Ersek
<lersek@redhat.com>; devel@edk2.groups.io; Luo, Heng
<heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

I've given R-b to the two patches. No comments from my side.

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Wednesday, January 13, 2021 2:00 PM
To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io; Luo, Heng <heng.luo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, January 12, 2021 10:28 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Luo,
Heng <heng.luo@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch V3 2/2]
MdeModulePkg/Bus/Pci/PciBusDxe:
Support PCIe Resizable BAR Capability

It seems like the max BAR size is selected first, but if
there's a "resource conflict" (running out of a particular
resource type aperture), then the minimum BAR size is selected.
I don't know what set of devices and/or resizable BARs this
logic applies to, if there are multiple of them.
Per the PCIe specification (revision 5.0, version 0.9) 7.8.6:

Software determines, through a proprietary mechanism, what
the
optimal size is for the resource, and programs that size via the
BAR
Size field of the Resizable BAR Control register.

Furthermore, Table 7-114 defines the Bar Size field of the
control register stating:

The default value of this field is equal to the default size of the
address space that the BAR resource is requesting via the BAR's
read-only bits.

Therefore the maximum size is not necessarily optimal, nor
should the minimum size be considered the default. In fact,
[we] tested various handoff BAR sizes for [a particular] GPU
and found that Windows didn't like the maximum BAR size.

Elsewhere in the discussion [1] the AMD author of the kernel
support for resizeable BARs indicates that FPGA devices might
implement the REBAR capability as part of their standard PCI
wrapper ([our] interpretation), but the BAR usage would be
determined by the actual bitstream written to the device,
therefore there might be a full bitmask for the BAR sizes
supported
by the device.

[1]
https://lists.freedesktop.org/archives/dri-devel/2021-January/t
h
read
.html

It would certainly make sense for the firmware to take REBAR
capabilities into account when sizing bridge apertures, but to
generically enable extended BAR sizes would make lots of
assumptions about the device usage and compatibility.

[...] At least for GPUs the expectation would be a default,
smaller compatibility size expanding to some representation
that allows direct DMA to the entire memory of the card.
So this patch should either be reverted; or minimally, the
default value of "PcdPcieResizableBarSupport" should be set to
FALSE, as the policy for BAR sizing doesn't look robust or portable.


General request for the future: if you implement some kind of
policy in core edk2, please at least *document* the policy
somewhere. It's unacceptable to have to decipher the source code
for such a possibly impactful change in the core. There is no
need for a wiki page or an RFC, but a sane bugzilla ticket and a
sane commit
message are required.

(The documentation of the PCD in the "MdeModulePkg.dec" file is
unsatisfactory too, and the UNI file has not been updated at
all.)


Your understanding is correct. Original idea is to let platform
supply the
policy
about what the optimal BAR size is for each resizable BAR.
The current implementation is a try to avoid asking platform code
for such policy because we thought it's a burden for platform to
supply
the policy data.

I agree that we set the PCD default value as disabled and after a
period of study, we will understand whether a platform policy is
really
needed.


Hello Laszlo and Ray,

I saw Heng's patch series to
1) Set the PCD default value to FALSE:
https://edk2.groups.io/g/devel/message/70139
2) Update the UNI file:
https://edk2.groups.io/g/devel/message/70140
has got Reviewed-by/Acked-by tags from reviewers.

Do you have further comments for the series?
If not, I will merge this change in the next 24 hours.

Thanks in advance.

Best Regards,
Hao Wu



Thanks,
Ray