Re: [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't restored sometimes

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, February 01, 2019 4:59 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ray; Wu, Hao A; Bi, Dandan
> Subject: [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't
> restored sometimes
> 
> From: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1505
> 
> When a device under PPB contains option ROM but doesn't require 32bit
> MMIO, ProgrameUpstreamBridgeForRom() cannot correctly restore the
> PPB MEM32 RANGE BAR. It causes the 32bit MMIO conflict which may
> cause system hangs in boot.
> 
> The root cause is when ProgrameUpstreamBridgeForRom() calls
> ProgramPpbApperture() to restore the PPB MEM32 RANGE BAR, the
> ProgramPpbApperture() skips to program the BAR when the resource
> length is 0.
> 
> This patch fixes this issue by not calling ProgramPpbApperture().
> Instead, it directly programs the PPB MEM32 RANGE BAR.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> ---
>  .../Bus/Pci/PciBusDxe/PciResourceSupport.c    | 53 +++++++++----------
>  1 file changed, 24 insertions(+), 29 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index d3cbefbadf..77cdc3e844 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -1,7 +1,7 @@
>  /** @file
>    PCI resouces support functions implemntation for PCI Bus module.
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be found
> at
> @@ -1661,57 +1661,52 @@ ProgrameUpstreamBridgeForRom (

Not directly related to the purpose of the patch, seems to me there is a
typo for the function name:
Programe -> Program

>    IN BOOLEAN         Enable
>    )
>  {
> -  PCI_IO_DEVICE     *Parent;
> -  PCI_RESOURCE_NODE Node;
> -  UINT64            Base;
> -  UINT64            Length;
> +  PCI_IO_DEVICE       *Parent;
> +  EFI_PCI_IO_PROTOCOL *PciIo;
> +  UINT16              Base;
> +  UINT16              Limit;
>    //
>    // For root bridge, just return.
>    //
>    Parent = PciDevice->Parent;
> -  ZeroMem (&Node, sizeof (Node));
>    while (Parent != NULL) {
>      if (!IS_PCI_BRIDGE (&Parent->Pci)) {
>        break;
>      }
> 
> -    Node.PciDev     = Parent;
> -    Node.Alignment  = 0;
> -    Node.Bar        = PPB_MEM32_RANGE;
> -    Node.ResType    = PciBarTypeMem32;
> -    Node.Offset     = 0;
> +    PciIo = &Parent->PciIo;
> 
>      //
>      // Program PPB to only open a single <= 16MB apperture

Also not directly related:
apperture -> aperture

There seems lots of 'apperture' within this driver.
I would suggest to propose another patch to address those typos.

>      //
>      if (Enable) {
> -      //
> -      // Save the original PPB_MEM32_RANGE BAR.
> -      // The values will be changed by ProgramPpbApperture().
> -      //
> -      Base   = Parent->PciBar[Node.Bar].BaseAddress;
> -      Length = Parent->PciBar[Node.Bar].Length;
> -
>        //
>        // Only cover MMIO for Option ROM.
>        //
> -      Node.Length     = PciDevice->RomSize;
> -      ProgramPpbApperture (OptionRomBase, &Node);
> -
> -      //
> -      // Restore the original PPB_MEM32_RANGE BAR.
> -      // So the MEM32 RANGE BAR register can be restored when disable the
> decoding.
> -      //
> -      Parent->PciBar[Node.Bar].BaseAddress = Base;
> -      Parent->PciBar[Node.Bar].Length      = Length;
> +      Base  = (UINT16) (OptionRomBase >> 16);
> +      Limit = (UINT16) ((OptionRomBase + PciDevice->RomSize - 1) >> 16);
> +      PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYPE01,
> Bridge.MemoryBase),  1, &Base);
> +      PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYPE01,
> Bridge.MemoryLimit), 1, &Limit);
> 
>        PCI_ENABLE_COMMAND_REGISTER (Parent,
> EFI_PCI_COMMAND_MEMORY_SPACE);
>      } else {
>        //
>        // Cover 32bit MMIO for devices below the bridge.
>        //
> -      Node.Length     = Parent->PciBar[Node.Bar].Length;
> -      ProgramPpbApperture (Parent->PciBar[Node.Bar].BaseAddress, &Node);
> +      if (Parent->PciBar[PPB_MEM32_RANGE].Length == 0) {
> +        //
> +        // When devices under the bridge contains Option ROM and doesn't
> require 32bit MMIO.
> +        //
> +        Base  = (UINT16) gAllOne;
> +        Limit = (UINT16) gAllZero;
> +      } else {
> +        Base  = (UINT16) (Parent->PciBar[PPB_MEM32_RANGE].BaseAddress >>
> 16);
> +        Limit = (UINT16)
> +                    ((Parent->PciBar[PPB_MEM32_RANGE].BaseAddress + Parent-
> >PciBar[PPB_MEM32_RANGE].Length - 1) >> 16);
> +      }
> +      PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYPE01,
> Bridge.MemoryBase),  1, &Base);
> +      PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16, OFFSET_OF (PCI_TYPE01,
> Bridge.MemoryLimit), 1, &Limit);
> +

Patch itself seems good to me,
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu 

>        PCI_DISABLE_COMMAND_REGISTER (Parent,
> EFI_PCI_COMMAND_MEMORY_SPACE);
>      }
> 
> --
> 2.20.1.windows.1