* Re: [PATCH v2] MdeModulePkg/PciBusDxe: make OPROM BAR degradation configurable
2016-09-19 8:36 [PATCH v2] MdeModulePkg/PciBusDxe: make OPROM BAR degradation configurable Ard Biesheuvel
@ 2016-09-23 8:01 ` Ard Biesheuvel
2016-09-23 8:37 ` Ni, Ruiyu
1 sibling, 0 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2016-09-23 8:01 UTC (permalink / raw)
To: edk2-devel@lists.01.org, Gao, Liming, Zeng, Star, Tian, Feng,
Ruiyu Ni, Leif Lindholm, afish@apple.com, Kinney, Michael D
Cc: Laszlo Ersek, Yao, Jiewen, Ard Biesheuvel
On 19 September 2016 at 09:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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 configurable, by introducing a feature flag PCD
> which defaults to TRUE only for X64.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: followed the suggestion of Andrew Fish to introduce a new feature flag
> PCD that controls the PCI BAR degradation behavior.
>
Ping? Any comments?
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 +
> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 76 ++++++++++----------
> MdeModulePkg/MdeModulePkg.dec | 12 ++++
> 3 files changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index 330ccc8cbffc..fc49f3d9412c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -100,6 +100,7 @@ [FeaturePcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdPciBridgeIoAlignmentProbe ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdUnalignedPciIoEnable ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom ## CONSUMES
>
> [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSrIovSystemPageSize ## SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index b0632d53b82b..37dc03e90358 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -1058,48 +1058,50 @@ DegradeResource (
> LIST_ENTRY *NextChildNodeLink;
> PCI_RESOURCE_NODE *ResourceNode;
>
> - //
> - // If any child device has both option ROM and 64-bit BAR, degrade its PMEM64/MEM64
> - // requests in case that if a legacy option ROM image can not access 64-bit resources.
> - //
> - ChildDeviceLink = Bridge->ChildList.ForwardLink;
> - while (ChildDeviceLink != NULL && ChildDeviceLink != &Bridge->ChildList) {
> - PciIoDevice = PCI_IO_DEVICE_FROM_LINK (ChildDeviceLink);
> - if (PciIoDevice->RomSize != 0) {
> - if (!IsListEmpty (&Mem64Node->ChildList)) {
> - ChildNodeLink = Mem64Node->ChildList.ForwardLink;
> - while (ChildNodeLink != &Mem64Node->ChildList) {
> - ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> - NextChildNodeLink = ChildNodeLink->ForwardLink;
> -
> - if ((ResourceNode->PciDev == PciIoDevice) &&
> - (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed)
> - ) {
> - RemoveEntryList (ChildNodeLink);
> - InsertResourceNode (Mem32Node, ResourceNode);
> + if (FeaturePcdGet (PcdPciDegradeBarsForOptionRom)) {
> + //
> + // If any child device has both option ROM and 64-bit BAR, degrade its PMEM64/MEM64
> + // requests in case that if a legacy option ROM image can not access 64-bit resources.
> + //
> + ChildDeviceLink = Bridge->ChildList.ForwardLink;
> + while (ChildDeviceLink != NULL && ChildDeviceLink != &Bridge->ChildList) {
> + PciIoDevice = PCI_IO_DEVICE_FROM_LINK (ChildDeviceLink);
> + if (PciIoDevice->RomSize != 0) {
> + if (!IsListEmpty (&Mem64Node->ChildList)) {
> + ChildNodeLink = Mem64Node->ChildList.ForwardLink;
> + while (ChildNodeLink != &Mem64Node->ChildList) {
> + ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> + NextChildNodeLink = ChildNodeLink->ForwardLink;
> +
> + if ((ResourceNode->PciDev == PciIoDevice) &&
> + (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed)
> + ) {
> + RemoveEntryList (ChildNodeLink);
> + InsertResourceNode (Mem32Node, ResourceNode);
> + }
> + ChildNodeLink = NextChildNodeLink;
> }
> - ChildNodeLink = NextChildNodeLink;
> - }
> - }
> + }
>
> - if (!IsListEmpty (&PMem64Node->ChildList)) {
> - ChildNodeLink = PMem64Node->ChildList.ForwardLink;
> - while (ChildNodeLink != &PMem64Node->ChildList) {
> - ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> - NextChildNodeLink = ChildNodeLink->ForwardLink;
> -
> - if ((ResourceNode->PciDev == PciIoDevice) &&
> - (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed)
> - ) {
> - RemoveEntryList (ChildNodeLink);
> - InsertResourceNode (PMem32Node, ResourceNode);
> + if (!IsListEmpty (&PMem64Node->ChildList)) {
> + ChildNodeLink = PMem64Node->ChildList.ForwardLink;
> + while (ChildNodeLink != &PMem64Node->ChildList) {
> + ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> + NextChildNodeLink = ChildNodeLink->ForwardLink;
> +
> + if ((ResourceNode->PciDev == PciIoDevice) &&
> + (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed)
> + ) {
> + RemoveEntryList (ChildNodeLink);
> + InsertResourceNode (PMem32Node, ResourceNode);
> + }
> + ChildNodeLink = NextChildNodeLink;
> }
> - ChildNodeLink = NextChildNodeLink;
> - }
> - }
> + }
>
> + }
> + ChildDeviceLink = ChildDeviceLink->ForwardLink;
> }
> - ChildDeviceLink = ChildDeviceLink->ForwardLink;
> }
>
> //
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 8d90f169b26e..d8cb9c119598 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -749,6 +749,18 @@ [PcdsFeatureFlag]
> # @Prompt Turn on PS2 Mouse Extended Verification
> gEfiMdeModulePkgTokenSpaceGuid.PcdPs2MouseExtendedVerification|TRUE|BOOLEAN|0x00010075
>
> +[PcdsFeatureFlag.X64]
> + ## Indicates whether 64-bit PCI MMIO BARs should degrade to 32-bit in the presence of an option ROM
> + # On X64 platforms, Option ROMs may contain code that executes in the context of a legacy BIOS (CSM),
> + # which requires that all PCI MMIO BARs are located below 4 GB
> + # TRUE - All PCI MMIO BARs of a device will be located below 4 GB if it has an option ROM
> + # FALSE - PCI MMIO BARs of a device may be located above 4 GB even if it has an option ROM
> + # @Prompt Degrade 64-bit PCI MMIO BARs for legacy BIOS option ROMs
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom|TRUE|BOOLEAN|0x0001003a
> +
> +[PcdsFeatureFlag.IA32, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom|FALSE|BOOLEAN|0x0001003a
> +
> [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
> ## Indicates if DxeIpl should switch to long mode to enter DXE phase.
> # It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] MdeModulePkg/PciBusDxe: make OPROM BAR degradation configurable
2016-09-19 8:36 [PATCH v2] MdeModulePkg/PciBusDxe: make OPROM BAR degradation configurable Ard Biesheuvel
2016-09-23 8:01 ` Ard Biesheuvel
@ 2016-09-23 8:37 ` Ni, Ruiyu
1 sibling, 0 replies; 3+ messages in thread
From: Ni, Ruiyu @ 2016-09-23 8:37 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, Gao, Liming, Zeng, Star,
Tian, Feng, leif.lindholm@linaro.org, afish@apple.com,
Kinney, Michael D
Cc: lersek@redhat.com, Yao, Jiewen
Ard,
I agree with your patch, except the PCD name. Can you rename it to PcdPciDegradeResourceForOptionRom?
Mike,
What's your opinion?
Thanks/Ray
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, September 19, 2016 4:36 PM
> To: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Tian, Feng <feng.tian@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; leif.lindholm@linaro.org; afish@apple.com; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: lersek@redhat.com; Yao, Jiewen <jiewen.yao@intel.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [PATCH v2] MdeModulePkg/PciBusDxe: make OPROM BAR
> degradation configurable
>
> 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 configurable, by introducing a feature flag PCD
> which defaults to TRUE only for X64.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: followed the suggestion of Andrew Fish to introduce a new feature flag
> PCD that controls the PCI BAR degradation behavior.
>
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 +
> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 76
> ++++++++++----------
> MdeModulePkg/MdeModulePkg.dec | 12 ++++
> 3 files changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index 330ccc8cbffc..fc49f3d9412c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -100,6 +100,7 @@ [FeaturePcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport ##
> CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdPciBridgeIoAlignmentProbe ##
> CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdUnalignedPciIoEnable ##
> CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom
> ##
> + CONSUMES
>
> [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSrIovSystemPageSize ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index b0632d53b82b..37dc03e90358 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -1058,48 +1058,50 @@ DegradeResource (
> LIST_ENTRY *NextChildNodeLink;
> PCI_RESOURCE_NODE *ResourceNode;
>
> - //
> - // If any child device has both option ROM and 64-bit BAR, degrade its
> PMEM64/MEM64
> - // requests in case that if a legacy option ROM image can not access 64-bit
> resources.
> - //
> - ChildDeviceLink = Bridge->ChildList.ForwardLink;
> - while (ChildDeviceLink != NULL && ChildDeviceLink != &Bridge->ChildList) {
> - PciIoDevice = PCI_IO_DEVICE_FROM_LINK (ChildDeviceLink);
> - if (PciIoDevice->RomSize != 0) {
> - if (!IsListEmpty (&Mem64Node->ChildList)) {
> - ChildNodeLink = Mem64Node->ChildList.ForwardLink;
> - while (ChildNodeLink != &Mem64Node->ChildList) {
> - ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> - NextChildNodeLink = ChildNodeLink->ForwardLink;
> -
> - if ((ResourceNode->PciDev == PciIoDevice) &&
> - (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode-
> >Bar].BarTypeFixed)
> - ) {
> - RemoveEntryList (ChildNodeLink);
> - InsertResourceNode (Mem32Node, ResourceNode);
> + if (FeaturePcdGet (PcdPciDegradeBarsForOptionRom)) {
> + //
> + // If any child device has both option ROM and 64-bit BAR, degrade its
> PMEM64/MEM64
> + // requests in case that if a legacy option ROM image can not access 64-bit
> resources.
> + //
> + ChildDeviceLink = Bridge->ChildList.ForwardLink;
> + while (ChildDeviceLink != NULL && ChildDeviceLink != &Bridge->ChildList)
> {
> + PciIoDevice = PCI_IO_DEVICE_FROM_LINK (ChildDeviceLink);
> + if (PciIoDevice->RomSize != 0) {
> + if (!IsListEmpty (&Mem64Node->ChildList)) {
> + ChildNodeLink = Mem64Node->ChildList.ForwardLink;
> + while (ChildNodeLink != &Mem64Node->ChildList) {
> + ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> + NextChildNodeLink = ChildNodeLink->ForwardLink;
> +
> + if ((ResourceNode->PciDev == PciIoDevice) &&
> + (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode-
> >Bar].BarTypeFixed)
> + ) {
> + RemoveEntryList (ChildNodeLink);
> + InsertResourceNode (Mem32Node, ResourceNode);
> + }
> + ChildNodeLink = NextChildNodeLink;
> }
> - ChildNodeLink = NextChildNodeLink;
> - }
> - }
> + }
>
> - if (!IsListEmpty (&PMem64Node->ChildList)) {
> - ChildNodeLink = PMem64Node->ChildList.ForwardLink;
> - while (ChildNodeLink != &PMem64Node->ChildList) {
> - ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> - NextChildNodeLink = ChildNodeLink->ForwardLink;
> -
> - if ((ResourceNode->PciDev == PciIoDevice) &&
> - (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode-
> >Bar].BarTypeFixed)
> - ) {
> - RemoveEntryList (ChildNodeLink);
> - InsertResourceNode (PMem32Node, ResourceNode);
> + if (!IsListEmpty (&PMem64Node->ChildList)) {
> + ChildNodeLink = PMem64Node->ChildList.ForwardLink;
> + while (ChildNodeLink != &PMem64Node->ChildList) {
> + ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> + NextChildNodeLink = ChildNodeLink->ForwardLink;
> +
> + if ((ResourceNode->PciDev == PciIoDevice) &&
> + (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode-
> >Bar].BarTypeFixed)
> + ) {
> + RemoveEntryList (ChildNodeLink);
> + InsertResourceNode (PMem32Node, ResourceNode);
> + }
> + ChildNodeLink = NextChildNodeLink;
> }
> - ChildNodeLink = NextChildNodeLink;
> - }
> - }
> + }
>
> + }
> + ChildDeviceLink = ChildDeviceLink->ForwardLink;
> }
> - ChildDeviceLink = ChildDeviceLink->ForwardLink;
> }
>
> //
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 8d90f169b26e..d8cb9c119598
> 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -749,6 +749,18 @@ [PcdsFeatureFlag]
> # @Prompt Turn on PS2 Mouse Extended Verification
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdPs2MouseExtendedVerification|TR
> UE|BOOLEAN|0x00010075
>
> +[PcdsFeatureFlag.X64]
> + ## Indicates whether 64-bit PCI MMIO BARs should degrade to 32-bit in
> +the presence of an option ROM
> + # On X64 platforms, Option ROMs may contain code that executes in
> +the context of a legacy BIOS (CSM),
> + # which requires that all PCI MMIO BARs are located below 4 GB
> + # TRUE - All PCI MMIO BARs of a device will be located below 4 GB if it has
> an option ROM
> + # FALSE - PCI MMIO BARs of a device may be located above 4 GB even if it
> has an option ROM
> + # @Prompt Degrade 64-bit PCI MMIO BARs for legacy BIOS option ROMs
> +
> +gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom|TR
> UE|BOOLE
> +AN|0x0001003a
> +
> +[PcdsFeatureFlag.IA32, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
> +
> +gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom|FA
> LSE|BOOL
> +EAN|0x0001003a
> +
> [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
> ## Indicates if DxeIpl should switch to long mode to enter DXE phase.
> # It is assumed that 64-bit DxeCore is built in firmware if it is true;
> otherwise 32-bit DxeCore
> --
> 2.7.4
^ permalink raw reply [flat|nested] 3+ messages in thread