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