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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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