* Re: [edk2-devel] [PATCH v2 2/2] Fix bug on SRIOV ReservedBusNum when ARI enable.
2022-10-12 2:36 ` [PATCH v2 2/2] Fix bug on SRIOV ReservedBusNum when ARI enable Foster Nong
@ 2022-10-14 3:03 ` Ni, Ray
2022-10-14 6:26 ` Ni, Ray
1 sibling, 0 replies; 5+ messages in thread
From: Ni, Ray @ 2022-10-14 3:03 UTC (permalink / raw)
To: devel@edk2.groups.io, Nong, Foster
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Foster Nong
> Sent: Wednesday, October 12, 2022 10:37 AM
> To: devel@edk2.groups.io
> Cc: Nong, Foster <foster.nong@intel.com>
> Subject: [edk2-devel] [PATCH v2 2/2] Fix bug on SRIOV ReservedBusNum when ARI enable.
>
> If a device which support both features SR-IOV/ARI has multi
> functions, which maybe support 8-255. After enable ARI forwarding in
> the root port and ARI Capable Hierarchy in the SR-IOV PF0.
> The device will support and expose multi functions(0-255) with ARI ID routing.
> In next device loop in below for() code, actually it still be in the
> same SR-IOV device, and just some PF which is over 8 or higher
> one(n*8), PciAllocateBusNumber() will allocate bus
> number(ReservedBusNum - TempReservedBusNum)) for this PF. if reset
> TempReservedBusNum as 0 in this case,it will allocate wrong bus number
> for this PF because TempReservedBusNum should be total previous PF's
> reserved bus numbers.
>
> code:
> for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) {
> TempReservedBusNum = 0;
> for (Func = 0; Func <= PCI_MAX_FUNC; Func++) {
> //
> // Check to see whether a pci device is present
> //
> Status = PciDevicePresent (
> PciRootBridgeIo,
> &Pci,
> StartBusNumber,
> Device,
> Func
> );
> ...
> Status = PciAllocateBusNumber (PciDevice, *SubBusNumber,
> (UINT8)(PciDevice->ReservedBusNum - TempReservedBusNum), SubBusNumber);
>
> The solution is add a new flag IsAriEnabled to help handle this case.
> if ARI is enabled, then TempReservedBusNum will not be reset again
> during all functions(1-255) scan with checking flag IsAriEnabled.
>
> Signed-off-by: Foster Nong <foster.nong@intel.com>
> ---
> MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 1 +
> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 19 ++++++++++++++++++-
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 1 +
> 3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> index bc20da1f38..8eca859695 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> @@ -2286,6 +2286,7 @@ CreatePciIoDevice (
> &Data32
>
> );
>
> if ((Data32 & EFI_PCIE_CAPABILITY_DEVICE_CAPABILITIES_2_ARI_FORWARDING) != 0) {
>
> + PciIoDevice->IsAriEnabled = TRUE;
>
> //
>
> // ARI forward support in bridge, so enable it.
>
> //
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index d5e3ef4d3f..3a57c05755 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -1106,6 +1106,7 @@ PciScanBus (
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo;
>
> BOOLEAN BusPadding;
>
> UINT32 TempReservedBusNum;
>
> + BOOLEAN IsAriEnabled;
>
>
>
> PciRootBridgeIo = Bridge->PciRootBridgeIo;
>
> SecondBus = 0;
>
> @@ -1116,9 +1117,12 @@ PciScanBus (
> BusPadding = FALSE;
>
> PciDevice = NULL;
>
> PciAddress = 0;
>
> + IsAriEnabled = FALSE;
>
>
>
> for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) {
>
> - TempReservedBusNum = 0;
>
> + if (!IsAriEnabled) {
>
> + TempReservedBusNum = 0;
>
> + }
>
> for (Func = 0; Func <= PCI_MAX_FUNC; Func++) {
>
> //
>
> // Check to see whether a pci device is present
>
> @@ -1157,6 +1161,19 @@ PciScanBus (
> if (EFI_ERROR (Status)) {
>
> continue;
>
> }
>
> + //
>
> + // Per Pcie spec ARI Extended Capability
>
> + // This capability must be implemented by each function in an ARI device.
>
> + // It is not applicable to a Root Port, a Switch Downstream Port, an RCiEP, or a Root Complex Event Collector
>
> + //
>
> + if (((Device == 0) && (Func == 0)) && (PciDevice->IsAriEnabled)) {
>
> + IsAriEnabled = TRUE;
>
> + }
>
> + if (PciDevice->IsAriEnabled != IsAriEnabled) {
>
> + DEBUG ((DEBUG_ERROR, "ERROR: %02x:%02x:%02x device ARI Feature(%x) is not consistent with others Function\n",
>
> + StartBusNumber, Device, Func, PciDevice->IsAriEnabled));
>
> + return EFI_DEVICE_ERROR;
>
> + }
>
>
>
> PciAddress = EFI_PCI_ADDRESS (StartBusNumber, Device, Func, 0);
>
>
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index 4b58c3ea9b..ca5c06204d 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -262,6 +262,7 @@ struct _PCI_IO_DEVICE {
> EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *BusNumberRanges;
>
>
>
> BOOLEAN IsPciExp;
>
> + BOOLEAN IsAriEnabled;
>
> //
>
> // For SR-IOV
>
> //
>
> --
> 2.37.1.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#95106): https://edk2.groups.io/g/devel/message/95106
> Mute This Topic: https://groups.io/mt/94282086/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [edk2-devel] [PATCH v2 2/2] Fix bug on SRIOV ReservedBusNum when ARI enable.
2022-10-12 2:36 ` [PATCH v2 2/2] Fix bug on SRIOV ReservedBusNum when ARI enable Foster Nong
2022-10-14 3:03 ` [edk2-devel] " Ni, Ray
@ 2022-10-14 6:26 ` Ni, Ray
1 sibling, 0 replies; 5+ messages in thread
From: Ni, Ray @ 2022-10-14 6:26 UTC (permalink / raw)
To: devel@edk2.groups.io, Nong, Foster
PR failed.
https://github.com/tianocore/edk2/pull/3475/checks?check_run_id=8885693723
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Foster Nong
> Sent: Wednesday, October 12, 2022 10:37 AM
> To: devel@edk2.groups.io
> Cc: Nong, Foster <foster.nong@intel.com>
> Subject: [edk2-devel] [PATCH v2 2/2] Fix bug on SRIOV ReservedBusNum when ARI enable.
>
> If a device which support both features SR-IOV/ARI has multi
> functions, which maybe support 8-255. After enable ARI forwarding in
> the root port and ARI Capable Hierarchy in the SR-IOV PF0.
> The device will support and expose multi functions(0-255) with ARI ID routing.
> In next device loop in below for() code, actually it still be in the
> same SR-IOV device, and just some PF which is over 8 or higher
> one(n*8), PciAllocateBusNumber() will allocate bus
> number(ReservedBusNum - TempReservedBusNum)) for this PF. if reset
> TempReservedBusNum as 0 in this case,it will allocate wrong bus number
> for this PF because TempReservedBusNum should be total previous PF's
> reserved bus numbers.
>
> code:
> for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) {
> TempReservedBusNum = 0;
> for (Func = 0; Func <= PCI_MAX_FUNC; Func++) {
> //
> // Check to see whether a pci device is present
> //
> Status = PciDevicePresent (
> PciRootBridgeIo,
> &Pci,
> StartBusNumber,
> Device,
> Func
> );
> ...
> Status = PciAllocateBusNumber (PciDevice, *SubBusNumber,
> (UINT8)(PciDevice->ReservedBusNum - TempReservedBusNum), SubBusNumber);
>
> The solution is add a new flag IsAriEnabled to help handle this case.
> if ARI is enabled, then TempReservedBusNum will not be reset again
> during all functions(1-255) scan with checking flag IsAriEnabled.
>
> Signed-off-by: Foster Nong <foster.nong@intel.com>
> ---
> MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 1 +
> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 19 ++++++++++++++++++-
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 1 +
> 3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> index bc20da1f38..8eca859695 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> @@ -2286,6 +2286,7 @@ CreatePciIoDevice (
> &Data32
>
> );
>
> if ((Data32 & EFI_PCIE_CAPABILITY_DEVICE_CAPABILITIES_2_ARI_FORWARDING) != 0) {
>
> + PciIoDevice->IsAriEnabled = TRUE;
>
> //
>
> // ARI forward support in bridge, so enable it.
>
> //
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index d5e3ef4d3f..3a57c05755 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -1106,6 +1106,7 @@ PciScanBus (
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo;
>
> BOOLEAN BusPadding;
>
> UINT32 TempReservedBusNum;
>
> + BOOLEAN IsAriEnabled;
>
>
>
> PciRootBridgeIo = Bridge->PciRootBridgeIo;
>
> SecondBus = 0;
>
> @@ -1116,9 +1117,12 @@ PciScanBus (
> BusPadding = FALSE;
>
> PciDevice = NULL;
>
> PciAddress = 0;
>
> + IsAriEnabled = FALSE;
>
>
>
> for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) {
>
> - TempReservedBusNum = 0;
>
> + if (!IsAriEnabled) {
>
> + TempReservedBusNum = 0;
>
> + }
>
> for (Func = 0; Func <= PCI_MAX_FUNC; Func++) {
>
> //
>
> // Check to see whether a pci device is present
>
> @@ -1157,6 +1161,19 @@ PciScanBus (
> if (EFI_ERROR (Status)) {
>
> continue;
>
> }
>
> + //
>
> + // Per Pcie spec ARI Extended Capability
>
> + // This capability must be implemented by each function in an ARI device.
>
> + // It is not applicable to a Root Port, a Switch Downstream Port, an RCiEP, or a Root Complex Event Collector
>
> + //
>
> + if (((Device == 0) && (Func == 0)) && (PciDevice->IsAriEnabled)) {
>
> + IsAriEnabled = TRUE;
>
> + }
>
> + if (PciDevice->IsAriEnabled != IsAriEnabled) {
>
> + DEBUG ((DEBUG_ERROR, "ERROR: %02x:%02x:%02x device ARI Feature(%x) is not consistent with others Function\n",
>
> + StartBusNumber, Device, Func, PciDevice->IsAriEnabled));
>
> + return EFI_DEVICE_ERROR;
>
> + }
>
>
>
> PciAddress = EFI_PCI_ADDRESS (StartBusNumber, Device, Func, 0);
>
>
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index 4b58c3ea9b..ca5c06204d 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -262,6 +262,7 @@ struct _PCI_IO_DEVICE {
> EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *BusNumberRanges;
>
>
>
> BOOLEAN IsPciExp;
>
> + BOOLEAN IsAriEnabled;
>
> //
>
> // For SR-IOV
>
> //
>
> --
> 2.37.1.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#95106): https://edk2.groups.io/g/devel/message/95106
> Mute This Topic: https://groups.io/mt/94282086/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 5+ messages in thread