.../Bus/Pci/PciBusDxe/PciResourceSupport.c | 67 +++++++--------------- 1 file changed, 21 insertions(+), 46 deletions(-)
The current implementation assumes there is only one hotplug resource
padding for each resource type. It's not true considering
DegradeResource(): MEM64 resource could be degraded to MEM32
resource.
The patch treat the resource paddings using the same logic as
treating typical/actual resources and the total resource of a bridge
is set to the MAX of typical/actual resources and resource paddings.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
.../Bus/Pci/PciBusDxe/PciResourceSupport.c | 67 +++++++---------------
1 file changed, 21 insertions(+), 46 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index e93134613b..f086b1732d 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -343,14 +343,9 @@ CalculateResourceAperture (
IN PCI_RESOURCE_NODE *Bridge
)
{
- UINT64 Aperture;
+ UINT64 Aperture[2];
LIST_ENTRY *CurrentLink;
PCI_RESOURCE_NODE *Node;
- UINT64 PaddingAperture;
- UINT64 Offset;
-
- Aperture = 0;
- PaddingAperture = 0;
if (Bridge == NULL) {
return ;
@@ -362,6 +357,8 @@ CalculateResourceAperture (
return ;
}
+ Aperture[PciResUsageTypical] = 0;
+ Aperture[PciResUsagePadding] = 0;
//
// Assume the bridge is aligned
//
@@ -369,58 +366,30 @@ CalculateResourceAperture (
; !IsNull (&Bridge->ChildList, CurrentLink)
; CurrentLink = GetNextNode (&Bridge->ChildList, CurrentLink)
) {
-
Node = RESOURCE_NODE_FROM_LINK (CurrentLink);
- if (Node->ResourceUsage == PciResUsagePadding) {
- ASSERT (PaddingAperture == 0);
- PaddingAperture = Node->Length;
- continue;
- }
//
- // Apply padding resource if available
+ // It's possible for a bridge to contain multiple padding resource
+ // nodes due to DegradeResource().
//
- Offset = Aperture & (Node->Alignment);
-
- if (Offset != 0) {
-
- Aperture = Aperture + (Node->Alignment + 1) - Offset;
-
- }
-
+ ASSERT ((Node->ResourceUsage == PciResUsageTypical) ||
+ (Node->ResourceUsage == PciResUsagePadding));
+ ASSERT (Node->ResourceUsage < ARRAY_SIZE (Aperture));
//
// Recode current aperture as a offset
- // this offset will be used in future real allocation
+ // Apply padding resource to meet alignment requirement
+ // Node offset will be used in future real allocation
//
- Node->Offset = Aperture;
+ Node->Offset = ALIGN_VALUE (Aperture[Node->ResourceUsage], Node->Alignment + 1);
//
- // Increment aperture by the length of node
+ // Record the total aperture.
//
- Aperture += Node->Length;
- }
-
- //
- // At last, adjust the aperture with the bridge's
- // alignment
- //
- Offset = Aperture & (Bridge->Alignment);
- if (Offset != 0) {
- Aperture = Aperture + (Bridge->Alignment + 1) - Offset;
+ Aperture[Node->ResourceUsage] = Node->Offset + Node->Length;
}
//
- // If the bridge has already padded the resource and the
- // amount of padded resource is larger, then keep the
- // padded resource
- //
- if (Bridge->Length < Aperture) {
- Bridge->Length = Aperture;
- }
-
- //
- // Adjust the bridge's alignment to the first child's alignment
- // if the bridge has at least one child
+ // Adjust the bridge's alignment to the MAX (first) alignment of all children.
//
CurrentLink = Bridge->ChildList.ForwardLink;
if (CurrentLink != &Bridge->ChildList) {
@@ -431,10 +400,16 @@ CalculateResourceAperture (
}
//
+ // At last, adjust the aperture with the bridge's alignment
+ //
+ Aperture[PciResUsageTypical] = ALIGN_VALUE (Aperture[PciResUsageTypical], Bridge->Alignment + 1);
+ Aperture[PciResUsagePadding] = ALIGN_VALUE (Aperture[PciResUsagePadding], Bridge->Alignment + 1);
+
+ //
// Hotplug controller needs padding resources.
// Use the larger one between the padding resource and actual occupied resource.
//
- Bridge->Length = MAX (Bridge->Length, PaddingAperture);
+ Bridge->Length = MAX (Aperture[PciResUsageTypical], Aperture[PciResUsagePadding]);
}
/**
--
2.12.2.windows.2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo,
Please check whether this patch can resolve the hang issue you reported.
I don't use your suggestion to use the MAX resource paddings.
Instead, I count all the resource paddings.
If it can work, I will submit another patch to clean up the code in CalculateApertureIo16().
That function could be similar to the CalculateAperture().
Thanks/Ray
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu
> Ni
> Sent: Saturday, September 30, 2017 1:11 PM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [PATCH] MdeModulePkg/PciBus: Count multiple hotplug
> resource paddings
>
> The current implementation assumes there is only one hotplug resource padding
> for each resource type. It's not true considering
> DegradeResource(): MEM64 resource could be degraded to MEM32 resource.
>
> The patch treat the resource paddings using the same logic as treating
> typical/actual resources and the total resource of a bridge is set to the MAX of
> typical/actual resources and resource paddings.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 67 +++++++---------------
> 1 file changed, 21 insertions(+), 46 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index e93134613b..f086b1732d 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -343,14 +343,9 @@ CalculateResourceAperture (
> IN PCI_RESOURCE_NODE *Bridge
> )
> {
> - UINT64 Aperture;
> + UINT64 Aperture[2];
> LIST_ENTRY *CurrentLink;
> PCI_RESOURCE_NODE *Node;
> - UINT64 PaddingAperture;
> - UINT64 Offset;
> -
> - Aperture = 0;
> - PaddingAperture = 0;
>
> if (Bridge == NULL) {
> return ;
> @@ -362,6 +357,8 @@ CalculateResourceAperture (
> return ;
> }
>
> + Aperture[PciResUsageTypical] = 0;
> + Aperture[PciResUsagePadding] = 0;
> //
> // Assume the bridge is aligned
> //
> @@ -369,58 +366,30 @@ CalculateResourceAperture (
> ; !IsNull (&Bridge->ChildList, CurrentLink)
> ; CurrentLink = GetNextNode (&Bridge->ChildList, CurrentLink)
> ) {
> -
> Node = RESOURCE_NODE_FROM_LINK (CurrentLink);
> - if (Node->ResourceUsage == PciResUsagePadding) {
> - ASSERT (PaddingAperture == 0);
> - PaddingAperture = Node->Length;
> - continue;
> - }
>
> //
> - // Apply padding resource if available
> + // It's possible for a bridge to contain multiple padding resource
> + // nodes due to DegradeResource().
> //
> - Offset = Aperture & (Node->Alignment);
> -
> - if (Offset != 0) {
> -
> - Aperture = Aperture + (Node->Alignment + 1) - Offset;
> -
> - }
> -
> + ASSERT ((Node->ResourceUsage == PciResUsageTypical) ||
> + (Node->ResourceUsage == PciResUsagePadding));
> + ASSERT (Node->ResourceUsage < ARRAY_SIZE (Aperture));
> //
> // Recode current aperture as a offset
> - // this offset will be used in future real allocation
> + // Apply padding resource to meet alignment requirement
> + // Node offset will be used in future real allocation
> //
> - Node->Offset = Aperture;
> + Node->Offset = ALIGN_VALUE (Aperture[Node->ResourceUsage],
> + Node->Alignment + 1);
>
> //
> - // Increment aperture by the length of node
> + // Record the total aperture.
> //
> - Aperture += Node->Length;
> - }
> -
> - //
> - // At last, adjust the aperture with the bridge's
> - // alignment
> - //
> - Offset = Aperture & (Bridge->Alignment);
> - if (Offset != 0) {
> - Aperture = Aperture + (Bridge->Alignment + 1) - Offset;
> + Aperture[Node->ResourceUsage] = Node->Offset + Node->Length;
> }
>
> //
> - // If the bridge has already padded the resource and the
> - // amount of padded resource is larger, then keep the
> - // padded resource
> - //
> - if (Bridge->Length < Aperture) {
> - Bridge->Length = Aperture;
> - }
> -
> - //
> - // Adjust the bridge's alignment to the first child's alignment
> - // if the bridge has at least one child
> + // Adjust the bridge's alignment to the MAX (first) alignment of all children.
> //
> CurrentLink = Bridge->ChildList.ForwardLink;
> if (CurrentLink != &Bridge->ChildList) { @@ -431,10 +400,16 @@
> CalculateResourceAperture (
> }
>
> //
> + // At last, adjust the aperture with the bridge's alignment //
> + Aperture[PciResUsageTypical] = ALIGN_VALUE
> + (Aperture[PciResUsageTypical], Bridge->Alignment + 1);
> + Aperture[PciResUsagePadding] = ALIGN_VALUE
> + (Aperture[PciResUsagePadding], Bridge->Alignment + 1);
> +
> + //
> // Hotplug controller needs padding resources.
> // Use the larger one between the padding resource and actual occupied
> resource.
> //
> - Bridge->Length = MAX (Bridge->Length, PaddingAperture);
> + Bridge->Length = MAX (Aperture[PciResUsageTypical],
> + Aperture[PciResUsagePadding]);
> }
>
> /**
> --
> 2.12.2.windows.2
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 09/30/17 07:12, Ni, Ruiyu wrote:
> Laszlo,
> Please check whether this patch can resolve the hang issue you reported.
> I don't use your suggestion to use the MAX resource paddings.
> Instead, I count all the resource paddings.
>
> If it can work, I will submit another patch to clean up the code in CalculateApertureIo16().
> That function could be similar to the CalculateAperture().
Thank you very much for the quick patch, it works!
Tested-by: Laszlo Ersek <lersek@redhat.com>
Can you add a reference to
<https://bugzilla.tianocore.org/show_bug.cgi?id=720> to the commit message?
Thanks!
Laszlo
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu
>> Ni
>> Sent: Saturday, September 30, 2017 1:11 PM
>> To: edk2-devel@lists.01.org
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Subject: [edk2] [PATCH] MdeModulePkg/PciBus: Count multiple hotplug
>> resource paddings
>>
>> The current implementation assumes there is only one hotplug resource padding
>> for each resource type. It's not true considering
>> DegradeResource(): MEM64 resource could be degraded to MEM32 resource.
>>
>> The patch treat the resource paddings using the same logic as treating
>> typical/actual resources and the total resource of a bridge is set to the MAX of
>> typical/actual resources and resource paddings.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> ---
>> .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 67 +++++++---------------
>> 1 file changed, 21 insertions(+), 46 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> index e93134613b..f086b1732d 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> @@ -343,14 +343,9 @@ CalculateResourceAperture (
>> IN PCI_RESOURCE_NODE *Bridge
>> )
>> {
>> - UINT64 Aperture;
>> + UINT64 Aperture[2];
>> LIST_ENTRY *CurrentLink;
>> PCI_RESOURCE_NODE *Node;
>> - UINT64 PaddingAperture;
>> - UINT64 Offset;
>> -
>> - Aperture = 0;
>> - PaddingAperture = 0;
>>
>> if (Bridge == NULL) {
>> return ;
>> @@ -362,6 +357,8 @@ CalculateResourceAperture (
>> return ;
>> }
>>
>> + Aperture[PciResUsageTypical] = 0;
>> + Aperture[PciResUsagePadding] = 0;
>> //
>> // Assume the bridge is aligned
>> //
>> @@ -369,58 +366,30 @@ CalculateResourceAperture (
>> ; !IsNull (&Bridge->ChildList, CurrentLink)
>> ; CurrentLink = GetNextNode (&Bridge->ChildList, CurrentLink)
>> ) {
>> -
>> Node = RESOURCE_NODE_FROM_LINK (CurrentLink);
>> - if (Node->ResourceUsage == PciResUsagePadding) {
>> - ASSERT (PaddingAperture == 0);
>> - PaddingAperture = Node->Length;
>> - continue;
>> - }
>>
>> //
>> - // Apply padding resource if available
>> + // It's possible for a bridge to contain multiple padding resource
>> + // nodes due to DegradeResource().
>> //
>> - Offset = Aperture & (Node->Alignment);
>> -
>> - if (Offset != 0) {
>> -
>> - Aperture = Aperture + (Node->Alignment + 1) - Offset;
>> -
>> - }
>> -
>> + ASSERT ((Node->ResourceUsage == PciResUsageTypical) ||
>> + (Node->ResourceUsage == PciResUsagePadding));
>> + ASSERT (Node->ResourceUsage < ARRAY_SIZE (Aperture));
>> //
>> // Recode current aperture as a offset
>> - // this offset will be used in future real allocation
>> + // Apply padding resource to meet alignment requirement
>> + // Node offset will be used in future real allocation
>> //
>> - Node->Offset = Aperture;
>> + Node->Offset = ALIGN_VALUE (Aperture[Node->ResourceUsage],
>> + Node->Alignment + 1);
>>
>> //
>> - // Increment aperture by the length of node
>> + // Record the total aperture.
>> //
>> - Aperture += Node->Length;
>> - }
>> -
>> - //
>> - // At last, adjust the aperture with the bridge's
>> - // alignment
>> - //
>> - Offset = Aperture & (Bridge->Alignment);
>> - if (Offset != 0) {
>> - Aperture = Aperture + (Bridge->Alignment + 1) - Offset;
>> + Aperture[Node->ResourceUsage] = Node->Offset + Node->Length;
>> }
>>
>> //
>> - // If the bridge has already padded the resource and the
>> - // amount of padded resource is larger, then keep the
>> - // padded resource
>> - //
>> - if (Bridge->Length < Aperture) {
>> - Bridge->Length = Aperture;
>> - }
>> -
>> - //
>> - // Adjust the bridge's alignment to the first child's alignment
>> - // if the bridge has at least one child
>> + // Adjust the bridge's alignment to the MAX (first) alignment of all children.
>> //
>> CurrentLink = Bridge->ChildList.ForwardLink;
>> if (CurrentLink != &Bridge->ChildList) { @@ -431,10 +400,16 @@
>> CalculateResourceAperture (
>> }
>>
>> //
>> + // At last, adjust the aperture with the bridge's alignment //
>> + Aperture[PciResUsageTypical] = ALIGN_VALUE
>> + (Aperture[PciResUsageTypical], Bridge->Alignment + 1);
>> + Aperture[PciResUsagePadding] = ALIGN_VALUE
>> + (Aperture[PciResUsagePadding], Bridge->Alignment + 1);
>> +
>> + //
>> // Hotplug controller needs padding resources.
>> // Use the larger one between the padding resource and actual occupied
>> resource.
>> //
>> - Bridge->Length = MAX (Bridge->Length, PaddingAperture);
>> + Bridge->Length = MAX (Aperture[PciResUsageTypical],
>> + Aperture[PciResUsagePadding]);
>> }
>>
>> /**
>> --
>> 2.12.2.windows.2
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Saturday, September 30, 2017 1:11 PM
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>
Subject: [edk2] [PATCH] MdeModulePkg/PciBus: Count multiple hotplug resource paddings
The current implementation assumes there is only one hotplug resource padding for each resource type. It's not true considering
DegradeResource(): MEM64 resource could be degraded to MEM32 resource.
The patch treat the resource paddings using the same logic as treating typical/actual resources and the total resource of a bridge is set to the MAX of typical/actual resources and resource paddings.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
.../Bus/Pci/PciBusDxe/PciResourceSupport.c | 67 +++++++---------------
1 file changed, 21 insertions(+), 46 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index e93134613b..f086b1732d 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -343,14 +343,9 @@ CalculateResourceAperture (
IN PCI_RESOURCE_NODE *Bridge
)
{
- UINT64 Aperture;
+ UINT64 Aperture[2];
LIST_ENTRY *CurrentLink;
PCI_RESOURCE_NODE *Node;
- UINT64 PaddingAperture;
- UINT64 Offset;
-
- Aperture = 0;
- PaddingAperture = 0;
if (Bridge == NULL) {
return ;
@@ -362,6 +357,8 @@ CalculateResourceAperture (
return ;
}
+ Aperture[PciResUsageTypical] = 0;
+ Aperture[PciResUsagePadding] = 0;
//
// Assume the bridge is aligned
//
@@ -369,58 +366,30 @@ CalculateResourceAperture (
; !IsNull (&Bridge->ChildList, CurrentLink)
; CurrentLink = GetNextNode (&Bridge->ChildList, CurrentLink)
) {
-
Node = RESOURCE_NODE_FROM_LINK (CurrentLink);
- if (Node->ResourceUsage == PciResUsagePadding) {
- ASSERT (PaddingAperture == 0);
- PaddingAperture = Node->Length;
- continue;
- }
//
- // Apply padding resource if available
+ // It's possible for a bridge to contain multiple padding resource
+ // nodes due to DegradeResource().
//
- Offset = Aperture & (Node->Alignment);
-
- if (Offset != 0) {
-
- Aperture = Aperture + (Node->Alignment + 1) - Offset;
-
- }
-
+ ASSERT ((Node->ResourceUsage == PciResUsageTypical) ||
+ (Node->ResourceUsage == PciResUsagePadding));
+ ASSERT (Node->ResourceUsage < ARRAY_SIZE (Aperture));
//
// Recode current aperture as a offset
- // this offset will be used in future real allocation
+ // Apply padding resource to meet alignment requirement
+ // Node offset will be used in future real allocation
//
- Node->Offset = Aperture;
+ Node->Offset = ALIGN_VALUE (Aperture[Node->ResourceUsage],
+ Node->Alignment + 1);
//
- // Increment aperture by the length of node
+ // Record the total aperture.
//
- Aperture += Node->Length;
- }
-
- //
- // At last, adjust the aperture with the bridge's
- // alignment
- //
- Offset = Aperture & (Bridge->Alignment);
- if (Offset != 0) {
- Aperture = Aperture + (Bridge->Alignment + 1) - Offset;
+ Aperture[Node->ResourceUsage] = Node->Offset + Node->Length;
}
//
- // If the bridge has already padded the resource and the
- // amount of padded resource is larger, then keep the
- // padded resource
- //
- if (Bridge->Length < Aperture) {
- Bridge->Length = Aperture;
- }
-
- //
- // Adjust the bridge's alignment to the first child's alignment
- // if the bridge has at least one child
+ // Adjust the bridge's alignment to the MAX (first) alignment of all children.
//
CurrentLink = Bridge->ChildList.ForwardLink;
if (CurrentLink != &Bridge->ChildList) { @@ -431,10 +400,16 @@ CalculateResourceAperture (
}
//
+ // At last, adjust the aperture with the bridge's alignment //
+ Aperture[PciResUsageTypical] = ALIGN_VALUE
+ (Aperture[PciResUsageTypical], Bridge->Alignment + 1);
+ Aperture[PciResUsagePadding] = ALIGN_VALUE
+ (Aperture[PciResUsagePadding], Bridge->Alignment + 1);
+
+ //
// Hotplug controller needs padding resources.
// Use the larger one between the padding resource and actual occupied resource.
//
- Bridge->Length = MAX (Bridge->Length, PaddingAperture);
+ Bridge->Length = MAX (Aperture[PciResUsageTypical],
+ Aperture[PciResUsagePadding]);
}
/**
--
2.12.2.windows.2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.