* [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't restored sometimes @ 2019-02-01 8:59 Ray Ni 2019-02-07 2:17 ` Wu, Hao A 0 siblings, 1 reply; 3+ messages in thread From: Ray Ni @ 2019-02-01 8:59 UTC (permalink / raw) To: edk2-devel; +Cc: Ruiyu Ni, Hao Wu, Dandan Bi 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 ( 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 // 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); + PCI_DISABLE_COMMAND_REGISTER (Parent, EFI_PCI_COMMAND_MEMORY_SPACE); } -- 2.20.1.windows.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't restored sometimes 2019-02-01 8:59 [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't restored sometimes Ray Ni @ 2019-02-07 2:17 ` Wu, Hao A 2019-02-11 1:25 ` Wu, Hao A 0 siblings, 1 reply; 3+ messages in thread From: Wu, Hao A @ 2019-02-07 2:17 UTC (permalink / raw) To: Ni, Ray, edk2-devel@lists.01.org; +Cc: Bi, Dandan > -----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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't restored sometimes 2019-02-07 2:17 ` Wu, Hao A @ 2019-02-11 1:25 ` Wu, Hao A 0 siblings, 0 replies; 3+ messages in thread From: Wu, Hao A @ 2019-02-11 1:25 UTC (permalink / raw) To: Wu, Hao A, Ni, Ray, edk2-devel@lists.01.org; +Cc: Bi, Dandan > -----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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-02-11 1:26 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-02-01 8:59 [PATCH] MdeModulePkg/PciBus: Fix a bug PPB MEM32 BAR isn't restored sometimes Ray Ni 2019-02-07 2:17 ` Wu, Hao A 2019-02-11 1:25 ` Wu, Hao A
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox