Re: [PATCH] MdeModulePkg/PciBus: Count multiple hotplug resource paddings

[Laszlo Ersek <lersek@redhat.com>]

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
>