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: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wu,
> Hao A
> Sent: Thursday, February 07, 2019 10:18 AM
> To: Ni, Ray; edk2-devel@lists.01.org
> Cc: Bi, Dandan
> Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR
> isn't restored sometimes
> 
> > -----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);

Sorry for not spotting this earlier.
'PciDevice->RomSize' is of type UINT64 here, RShiftU64() should be used here.

Two more similar cases below.

> > +      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);

Please use RShiftU64() here.

> > +        Limit = (UINT16)
> > +                    ((Parent->PciBar[PPB_MEM32_RANGE].BaseAddress + Parent-
> > >PciBar[PPB_MEM32_RANGE].Length - 1) >> 16);

Please use RShiftU64() here.

Best Regards,
Hao Wu

> > +      }
> > +      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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel