> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-boun...@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...@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...@intel.com>
> > Cc: Hao Wu <hao.a...@intel.com>
> > Cc: Dandan Bi <dandan...@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...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
