* [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific @ 2016-09-15 14:54 Ard Biesheuvel 2016-09-15 15:28 ` Leif Lindholm 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2016-09-15 14:54 UTC (permalink / raw) To: edk2-devel, liming.gao, star.zeng, feng.tian, ruiyu.ni, leif.lindholm, afish, michael.d.kinney Cc: lersek, jiewen.yao, Ard Biesheuvel The 'universal' PCI bus driver in MdeModulePkg contains a quirk to degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option ROM on the same PCI controller. This quirk is highly specific to not just the X64 architecture in general, but to the PC platform in particular, given that only X64 platforms that require legacy PC BIOS compatibility require it. However, making the quirk dependent on the presence of the legacy BIOS protocol met with resistance, due to the fact that it introduces a dependency on the IntelFrameworkModulePkg package. So instead, make the quirk dependent on whether we are compiling for the X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor directives around it. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Since nobody proposed any alternatives to the solution I proposed, let's just make the quirk disappear on architectures that have no use for it. Note that forcing non-X64 architectures to install a driver that produces the IncompatiblePciDevice protocol only to make the PCI bus driver behave normally (and which, incidentally, can only be produced once per platform, making it difficult to provide a default implementation that can be widely reused) is not acceptable IMO. If anything, this should require an IncompatiblePlatform protocol that informs the PCI bus driver that a special quirk is required, but I am aware that this is difficult to accomplish without breaking backward compatibility. Hence this compromise. MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c index b0632d53b82b..b4771a822d65 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c @@ -1052,6 +1052,7 @@ DegradeResource ( IN PCI_RESOURCE_NODE *PMem64Node ) { +#ifdef MDE_CPU_X64 PCI_IO_DEVICE *PciIoDevice; LIST_ENTRY *ChildDeviceLink; LIST_ENTRY *ChildNodeLink; @@ -1101,6 +1102,7 @@ DegradeResource ( } ChildDeviceLink = ChildDeviceLink->ForwardLink; } +#endif // // If firmware is in 32-bit mode, -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific 2016-09-15 14:54 [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific Ard Biesheuvel @ 2016-09-15 15:28 ` Leif Lindholm 2016-09-15 15:48 ` Andrew Fish 0 siblings, 1 reply; 5+ messages in thread From: Leif Lindholm @ 2016-09-15 15:28 UTC (permalink / raw) To: Ard Biesheuvel Cc: edk2-devel, liming.gao, star.zeng, feng.tian, ruiyu.ni, afish, michael.d.kinney, lersek, jiewen.yao On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote: > The 'universal' PCI bus driver in MdeModulePkg contains a quirk to > degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option > ROM on the same PCI controller. > > This quirk is highly specific to not just the X64 architecture in general, > but to the PC platform in particular, given that only X64 platforms that > require legacy PC BIOS compatibility require it. However, making the > quirk dependent on the presence of the legacy BIOS protocol met with > resistance, due to the fact that it introduces a dependency on the > IntelFrameworkModulePkg package. > > So instead, make the quirk dependent on whether we are compiling for the > X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor > directives around it. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> I'm happy with the change, but it may be worth putting a comment in the code to the extent that: --- In order to reliably support legacy Option ROMs, which may not be 64-bit safe, X64 needs to ensure PCI MMIO BARs are forced down to use only the lower 32 bits. Other architectures, or indeed X64 without legacy option ROM support, can safely leave these in normal operation mode (and may require so in order to function). --- For what it's worth: Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > > Since nobody proposed any alternatives to the solution I proposed, > let's just make the quirk disappear on architectures that have no > use for it. > > Note that forcing non-X64 architectures to install a driver that > produces the IncompatiblePciDevice protocol only to make the PCI > bus driver behave normally (and which, incidentally, can only be > produced once per platform, making it difficult to provide a default > implementation that can be widely reused) is not acceptable IMO. > > If anything, this should require an IncompatiblePlatform protocol > that informs the PCI bus driver that a special quirk is required, > but I am aware that this is difficult to accomplish without breaking > backward compatibility. Hence this compromise. > > MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > index b0632d53b82b..b4771a822d65 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > @@ -1052,6 +1052,7 @@ DegradeResource ( > IN PCI_RESOURCE_NODE *PMem64Node > ) > { > +#ifdef MDE_CPU_X64 > PCI_IO_DEVICE *PciIoDevice; > LIST_ENTRY *ChildDeviceLink; > LIST_ENTRY *ChildNodeLink; > @@ -1101,6 +1102,7 @@ DegradeResource ( > } > ChildDeviceLink = ChildDeviceLink->ForwardLink; > } > +#endif > > // > // If firmware is in 32-bit mode, > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific 2016-09-15 15:28 ` Leif Lindholm @ 2016-09-15 15:48 ` Andrew Fish 2016-09-15 15:51 ` Brian J. Johnson 2016-09-15 15:53 ` Ard Biesheuvel 0 siblings, 2 replies; 5+ messages in thread From: Andrew Fish @ 2016-09-15 15:48 UTC (permalink / raw) To: Leif Lindholm Cc: Ard Biesheuvel, ruiyu.ni, feng.tian, edk2-devel, liming.gao, jiewen.yao, Mike Kinney, lersek, star.zeng > On Sep 15, 2016, at 8:28 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote: > > On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote: >> The 'universal' PCI bus driver in MdeModulePkg contains a quirk to >> degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option >> ROM on the same PCI controller. >> >> This quirk is highly specific to not just the X64 architecture in general, >> but to the PC platform in particular, given that only X64 platforms that >> require legacy PC BIOS compatibility require it. However, making the >> quirk dependent on the presence of the legacy BIOS protocol met with >> resistance, due to the fact that it introduces a dependency on the >> IntelFrameworkModulePkg package. >> >> So instead, make the quirk dependent on whether we are compiling for the >> X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor >> directives around it. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > I'm happy with the change, but it may be worth putting a comment in Do we have to use an #ifdef? Seems like it should be a PCD. Is this for legacy BIOS Option ROM support, or for existing EFI ROMs? Thanks, Andrew Fish > the code to the extent that: > --- > In order to reliably support legacy Option ROMs, which may not be > 64-bit safe, X64 needs to ensure PCI MMIO BARs are forced down to use > only the lower 32 bits. > Other architectures, or indeed X64 without legacy option ROM support, > can safely leave these in normal operation mode (and may require so in > order to function). > --- > > For what it's worth: > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > >> --- >> >> Since nobody proposed any alternatives to the solution I proposed, >> let's just make the quirk disappear on architectures that have no >> use for it. >> >> Note that forcing non-X64 architectures to install a driver that >> produces the IncompatiblePciDevice protocol only to make the PCI >> bus driver behave normally (and which, incidentally, can only be >> produced once per platform, making it difficult to provide a default >> implementation that can be widely reused) is not acceptable IMO. >> >> If anything, this should require an IncompatiblePlatform protocol >> that informs the PCI bus driver that a special quirk is required, >> but I am aware that this is difficult to accomplish without breaking >> backward compatibility. Hence this compromise. >> >> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c >> index b0632d53b82b..b4771a822d65 100644 >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c >> @@ -1052,6 +1052,7 @@ DegradeResource ( >> IN PCI_RESOURCE_NODE *PMem64Node >> ) >> { >> +#ifdef MDE_CPU_X64 >> PCI_IO_DEVICE *PciIoDevice; >> LIST_ENTRY *ChildDeviceLink; >> LIST_ENTRY *ChildNodeLink; >> @@ -1101,6 +1102,7 @@ DegradeResource ( >> } >> ChildDeviceLink = ChildDeviceLink->ForwardLink; >> } >> +#endif >> >> // >> // If firmware is in 32-bit mode, >> -- >> 2.7.4 >> > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific 2016-09-15 15:48 ` Andrew Fish @ 2016-09-15 15:51 ` Brian J. Johnson 2016-09-15 15:53 ` Ard Biesheuvel 1 sibling, 0 replies; 5+ messages in thread From: Brian J. Johnson @ 2016-09-15 15:51 UTC (permalink / raw) To: Andrew Fish, Leif Lindholm Cc: ruiyu.ni, feng.tian, Ard Biesheuvel, edk2-devel, liming.gao, jiewen.yao, Mike Kinney, lersek, star.zeng On 09/15/2016 10:48 AM, Andrew Fish wrote: > >> On Sep 15, 2016, at 8:28 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote: >> >> On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote: >>> The 'universal' PCI bus driver in MdeModulePkg contains a quirk to >>> degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option >>> ROM on the same PCI controller. >>> >>> This quirk is highly specific to not just the X64 architecture in general, >>> but to the PC platform in particular, given that only X64 platforms that >>> require legacy PC BIOS compatibility require it. However, making the >>> quirk dependent on the presence of the legacy BIOS protocol met with >>> resistance, due to the fact that it introduces a dependency on the >>> IntelFrameworkModulePkg package. >>> >>> So instead, make the quirk dependent on whether we are compiling for the >>> X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor >>> directives around it. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> I'm happy with the change, but it may be worth putting a comment in > > Do we have to use an #ifdef? Seems like it should be a PCD. Is this for legacy BIOS Option ROM support, or for existing EFI ROMs? > > Thanks, > > Andrew Fish I was going to say the same thing... this sounds like exactly the sort of thing PCDs are meant for. And as Leif's comment says, not all X64 platforms have legacy support or want BAR degradation (ours don't.) Brian Johnson > >> the code to the extent that: >> --- >> In order to reliably support legacy Option ROMs, which may not be >> 64-bit safe, X64 needs to ensure PCI MMIO BARs are forced down to use >> only the lower 32 bits. >> Other architectures, or indeed X64 without legacy option ROM support, >> can safely leave these in normal operation mode (and may require so in >> order to function). >> --- >> >> For what it's worth: >> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> >> >>> --- >>> >>> Since nobody proposed any alternatives to the solution I proposed, >>> let's just make the quirk disappear on architectures that have no >>> use for it. >>> >>> Note that forcing non-X64 architectures to install a driver that >>> produces the IncompatiblePciDevice protocol only to make the PCI >>> bus driver behave normally (and which, incidentally, can only be >>> produced once per platform, making it difficult to provide a default >>> implementation that can be widely reused) is not acceptable IMO. >>> >>> If anything, this should require an IncompatiblePlatform protocol >>> that informs the PCI bus driver that a special quirk is required, >>> but I am aware that this is difficult to accomplish without breaking >>> backward compatibility. Hence this compromise. >>> >>> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c >>> index b0632d53b82b..b4771a822d65 100644 >>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c >>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c >>> @@ -1052,6 +1052,7 @@ DegradeResource ( >>> IN PCI_RESOURCE_NODE *PMem64Node >>> ) >>> { >>> +#ifdef MDE_CPU_X64 >>> PCI_IO_DEVICE *PciIoDevice; >>> LIST_ENTRY *ChildDeviceLink; >>> LIST_ENTRY *ChildNodeLink; >>> @@ -1101,6 +1102,7 @@ DegradeResource ( >>> } >>> ChildDeviceLink = ChildDeviceLink->ForwardLink; >>> } >>> +#endif >>> >>> // >>> // If firmware is in 32-bit mode, >>> -- >>> 2.7.4 >>> >> _______________________________________________ >> 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 > -- My statements are my own, are not authorized by SGI, and do not necessarily represent SGI’s positions. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific 2016-09-15 15:48 ` Andrew Fish 2016-09-15 15:51 ` Brian J. Johnson @ 2016-09-15 15:53 ` Ard Biesheuvel 1 sibling, 0 replies; 5+ messages in thread From: Ard Biesheuvel @ 2016-09-15 15:53 UTC (permalink / raw) To: Andrew Fish Cc: Leif Lindholm, Ruiyu Ni, Tian, Feng, edk2-devel-01, Gao, Liming, Yao, Jiewen, Mike Kinney, Laszlo Ersek, Zeng, Star On 15 September 2016 at 16:48, Andrew Fish <afish@apple.com> wrote: > >> On Sep 15, 2016, at 8:28 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote: >> >> On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote: >>> The 'universal' PCI bus driver in MdeModulePkg contains a quirk to >>> degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option >>> ROM on the same PCI controller. >>> >>> This quirk is highly specific to not just the X64 architecture in general, >>> but to the PC platform in particular, given that only X64 platforms that >>> require legacy PC BIOS compatibility require it. However, making the >>> quirk dependent on the presence of the legacy BIOS protocol met with >>> resistance, due to the fact that it introduces a dependency on the >>> IntelFrameworkModulePkg package. >>> >>> So instead, make the quirk dependent on whether we are compiling for the >>> X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor >>> directives around it. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> I'm happy with the change, but it may be worth putting a comment in > > Do we have to use an #ifdef? Seems like it should be a PCD. Is this for legacy BIOS Option ROM support, or for existing EFI ROMs? > Strictly for being able to execute a legacy BIOS Option ROM in the context of a CSM. Hence my assertion that this is specific to the 64-bit variant of the PC platform. Getting new PCDs introduced is also met with fierce resistance, to the extent that (as this code testifies), we'd rather keep quirks hidden in the code like this, and enable them by default rather than acknowledging their existence by adding explicit controls to enable/disable them. Others have suggested a dynamic PCD, which can be flicked when the legacy BIOS protocol installed. But since X64 is not my primary interested, I will settle for a feature PCD, and #ifdef or anything else that results in this quirk to be disabled by default on 64-bit ARM. Thanks, Ard. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-15 15:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-15 14:54 [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific Ard Biesheuvel 2016-09-15 15:28 ` Leif Lindholm 2016-09-15 15:48 ` Andrew Fish 2016-09-15 15:51 ` Brian J. Johnson 2016-09-15 15:53 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox