* [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation
@ 2016-09-12 10:01 Ard Biesheuvel
2016-09-12 10:01 ` [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB Ard Biesheuvel
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2016-09-12 10:01 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
On ARM/Linux systems, the PCI layer is usually reconfigured from scratch,
given the embedded legacy of ARM systems. However, when booting arm64/Linux
in ACPI mode, such reconfiguration can be avoided, since ACPI implies UEFI
implies firmware (as opposed to DT mode, which could be booted via QEMU's
~10 instruction bootloader)
In this case, it is important for the firmware to leave the PCI configuration
in a reasonable state, and one of the things that UEFI does by default, and
which makes no sense at all on an arm64 system, is to degrade 64-bit MMIO BARs
to 32-bit in the presence of a ROM BAR on the same device. (It does makes sense
on an Intel system running legacy option ROMs under a CSM)
Fortunately, we have a way of influencing this policy without having to hack
the generic PCI bus driver: we can install the IncompatiblePciDeviceSupport
protocol, and the PCI bus driver will invoke it for each PCI device it
configures.
Our implementation for ArmPkg is simply the OVMF version with the bits ripped
out that care about CSMs and legacy BIOSes
Ard Biesheuvel (3):
ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4
GB
ArmVirtPkg/FdtPciPcdProducerLib: add discovery of PcdPciMmio64Size
ArmVirtPkg/ArmVirtQemu: add IncompatiblePciDeviceSupportDxe
ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c | 223 ++++++++++++++++++++
ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf | 49 +++++
ArmVirtPkg/ArmVirtQemu.dsc | 5 +
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 +
ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 31 +--
ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 1 +
7 files changed, 302 insertions(+), 13 deletions(-)
create mode 100644 ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
create mode 100644 ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB
2016-09-12 10:01 [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Ard Biesheuvel
@ 2016-09-12 10:01 ` Ard Biesheuvel
2016-09-12 10:23 ` Leif Lindholm
2016-09-12 10:01 ` [PATCH 2/3] ArmVirtPkg/FdtPciPcdProducerLib: add discovery of PcdPciMmio64Size Ard Biesheuvel
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2016-09-12 10:01 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
By default, the generic PciBusDxe driver uses the presence of a ROM BAR
as a hint that all MMIO BARs should be allocated in the 32-bit region,
even the ones that could be allocated above 4 GB. The reason is that
such a ROM BAR may contain code that runs in the context of a legacy
BIOS (i.e., a CSM), and allocating the 64-bit BARs above 4 GB may put
them out of reach.
Of course, none of this matters on ARM, and so we can unconditionally
override this decision. So take the OVMF implementation of the
IncompatiblePciDeviceSupportDxe driver, rip out the bits that care
about the presence of a legacy BIOS, and wire it up to the ArmPkg PCI
PCDs.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c | 223 ++++++++++++++++++++
ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf | 49 +++++
2 files changed, 272 insertions(+)
diff --git a/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c b/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
new file mode 100644
index 000000000000..3f2869f8f3c2
--- /dev/null
+++ b/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
@@ -0,0 +1,223 @@
+/** @file
+ A simple DXE_DRIVER that causes the PCI Bus UEFI_DRIVER to allocate 64-bit
+ MMIO BARs above 4 GB, regardless of option ROM availability, conserving
+ 32-bit MMIO aperture for 32-bit BARs.
+
+ Copyright (C) 2016, Red Hat, Inc.
+ Copyright (C) 2016, Linaro, Ltd.
+
+ 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
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+#include <IndustryStandard/Acpi10.h>
+#include <IndustryStandard/Pci22.h>
+
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/IncompatiblePciDeviceSupport.h>
+
+//
+// The protocol interface this driver produces.
+//
+STATIC
+EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL mIncompatiblePciDeviceSupport;
+
+//
+// Configuration template for the CheckDevice() protocol member function.
+//
+// Refer to Table 20 "ACPI 2.0 & 3.0 QWORD Address Space Descriptor Usage" in
+// the Platform Init 1.4a Spec, Volume 5.
+//
+// This structure is interpreted by the UpdatePciInfo() function in the edk2
+// PCI Bus UEFI_DRIVER.
+//
+#pragma pack (1)
+typedef struct {
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR AddressSpaceDesc;
+ EFI_ACPI_END_TAG_DESCRIPTOR EndDesc;
+} MMIO64_PREFERENCE;
+#pragma pack ()
+
+STATIC CONST MMIO64_PREFERENCE mConfiguration = {
+ //
+ // AddressSpaceDesc
+ //
+ {
+ ACPI_ADDRESS_SPACE_DESCRIPTOR, // Desc
+ (UINT16)( // Len
+ sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
+ OFFSET_OF (
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
+ ResType
+ )
+ ),
+ ACPI_ADDRESS_SPACE_TYPE_MEM, // ResType
+ PCI_ACPI_UNUSED, // GenFlag
+ PCI_ACPI_UNUSED, // SpecificFlag
+ 64, // AddrSpaceGranularity:
+ // aperture selection hint
+ // for BAR allocation
+ PCI_ACPI_UNUSED, // AddrRangeMin
+ PCI_BAR_OLD_ALIGN, // AddrRangeMax:
+ // no special alignment
+ // for affected BARs
+ PCI_BAR_ALL, // AddrTranslationOffset:
+ // hint covers all
+ // eligible BARs
+ PCI_BAR_NOCHANGE // AddrLen:
+ // use probed BAR size
+ },
+ //
+ // EndDesc
+ //
+ {
+ ACPI_END_TAG_DESCRIPTOR, // Desc
+ 0 // Checksum: to be ignored
+ }
+};
+
+
+/**
+ Returns a list of ACPI resource descriptors that detail the special resource
+ configuration requirements for an incompatible PCI device.
+
+ Prior to bus enumeration, the PCI bus driver will look for the presence of
+ the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL. Only one instance of this
+ protocol can be present in the system. For each PCI device that the PCI bus
+ driver discovers, the PCI bus driver calls this function with the device's
+ vendor ID, device ID, revision ID, subsystem vendor ID, and subsystem device
+ ID. If the VendorId, DeviceId, RevisionId, SubsystemVendorId, or
+ SubsystemDeviceId value is set to (UINTN)-1, that field will be ignored. The
+ ID values that are not (UINTN)-1 will be used to identify the current device.
+
+ This function will only return EFI_SUCCESS. However, if the device is an
+ incompatible PCI device, a list of ACPI resource descriptors will be returned
+ in Configuration. Otherwise, NULL will be returned in Configuration instead.
+ The PCI bus driver does not need to allocate memory for Configuration.
+ However, it is the PCI bus driver's responsibility to free it. The PCI bus
+ driver then can configure this device with the information that is derived
+ from this list of resource nodes, rather than the result of BAR probing.
+
+ Only the following two resource descriptor types from the ACPI Specification
+ may be used to describe the incompatible PCI device resource requirements:
+ - QWORD Address Space Descriptor (ACPI 2.0, section 6.4.3.5.1; also ACPI 3.0)
+ - End Tag (ACPI 2.0, section 6.4.2.8; also ACPI 3.0)
+
+ The QWORD Address Space Descriptor can describe memory, I/O, and bus number
+ ranges for dynamic or fixed resources. The configuration of a PCI root bridge
+ is described with one or more QWORD Address Space Descriptors, followed by an
+ End Tag. See the ACPI Specification for details on the field values.
+
+ @param[in] This Pointer to the
+ EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL
+ instance.
+
+ @param[in] VendorId A unique ID to identify the manufacturer of
+ the PCI device. See the Conventional PCI
+ Specification 3.0 for details.
+
+ @param[in] DeviceId A unique ID to identify the particular PCI
+ device. See the Conventional PCI
+ Specification 3.0 for details.
+
+ @param[in] RevisionId A PCI device-specific revision identifier.
+ See the Conventional PCI Specification 3.0
+ for details.
+
+ @param[in] SubsystemVendorId Specifies the subsystem vendor ID. See the
+ Conventional PCI Specification 3.0 for
+ details.
+
+ @param[in] SubsystemDeviceId Specifies the subsystem device ID. See the
+ Conventional PCI Specification 3.0 for
+ details.
+
+ @param[out] Configuration A list of ACPI resource descriptors that
+ detail the configuration requirement.
+
+ @retval EFI_SUCCESS The function always returns EFI_SUCCESS.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+CheckDevice (
+ IN EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL *This,
+ IN UINTN VendorId,
+ IN UINTN DeviceId,
+ IN UINTN RevisionId,
+ IN UINTN SubsystemVendorId,
+ IN UINTN SubsystemDeviceId,
+ OUT VOID **Configuration
+ )
+{
+ //
+ // Unlike the general description of this protocol member suggests, there is
+ // nothing incompatible about the PCI devices that we'll match here. We'll
+ // match all PCI devices, and generate exactly one QWORD Address Space
+ // Descriptor for each. That descriptor will instruct the PCI Bus UEFI_DRIVER
+ // not to degrade 64-bit MMIO BARs for the device, even if a PCI option ROM
+ // BAR is present on the device.
+ //
+
+ //
+ // This member function is mis-specified actually: it is supposed to allocate
+ // memory, but as specified, it could not return an error status. Thankfully,
+ // the edk2 PCI Bus UEFI_DRIVER actually handles error codes; see the
+ // UpdatePciInfo() function.
+ //
+ *Configuration = AllocateCopyPool (sizeof mConfiguration, &mConfiguration);
+ if (*Configuration == NULL) {
+ DEBUG ((EFI_D_WARN,
+ "%a: 64-bit MMIO BARs may be degraded for PCI 0x%04x:0x%04x (rev %d)\n",
+ __FUNCTION__, (UINT32)VendorId, (UINT32)DeviceId, (UINT8)RevisionId));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ return EFI_SUCCESS;
+}
+
+
+/**
+ Entry point for this driver.
+
+ @param[in] ImageHandle Image handle of this driver.
+ @param[in] SystemTable Pointer to SystemTable.
+
+ @retval EFI_SUCESS Driver has loaded successfully.
+ @retval EFI_UNSUPPORTED PCI resource allocation has been disabled.
+ @retval EFI_UNSUPPORTED There is no 64-bit PCI MMIO aperture.
+ @return Error codes from lower level functions.
+
+**/
+EFI_STATUS
+EFIAPI
+DriverInitialize (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ //
+ // If the PCI Bus driver is not supposed to allocate resources, then it makes
+ // no sense to install a protocol that influences the resource allocation.
+ //
+ // Similarly, if there is no 64-bit PCI MMIO aperture, then 64-bit MMIO BARs
+ // have to be allocated under 4 GB unconditionally.
+ //
+ if (PcdGetBool (PcdPciDisableBusEnumeration) ||
+ PcdGet64 (PcdPciMmio64Size) == 0) {
+ return EFI_UNSUPPORTED;
+ }
+
+ mIncompatiblePciDeviceSupport.CheckDevice = CheckDevice;
+ return gBS->InstallMultipleProtocolInterfaces (&ImageHandle,
+ &gEfiIncompatiblePciDeviceSupportProtocolGuid,
+ &mIncompatiblePciDeviceSupport, NULL);
+}
diff --git a/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf b/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
new file mode 100644
index 000000000000..0bda20199d51
--- /dev/null
+++ b/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
@@ -0,0 +1,49 @@
+## @file
+# A simple DXE_DRIVER that causes the PCI Bus UEFI_DRIVER to allocate 64-bit
+# MMIO BARs above 4 GB, regardless of option ROM availability, conserving
+# 32-bit MMIO aperture for 32-bit BARs.
+#
+# Copyright (C) 2016, Red Hat, Inc.
+# Copyright (C) 2016, Linaro, Ltd.
+#
+# 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
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = IncompatiblePciDeviceSupportDxe
+ FILE_GUID = 18BCB238-844E-446B-AA2D-3DF38A25FA0F
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = DriverInitialize
+
+[Sources]
+ IncompatiblePciDeviceSupport.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ DebugLib
+ MemoryAllocationLib
+ PcdLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Protocols]
+ gEfiIncompatiblePciDeviceSupportProtocolGuid ## PRODUCES
+
+[Pcd]
+ gArmTokenSpaceGuid.PcdPciMmio64Size ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration ## CONSUMES
+
+[Depex]
+ TRUE
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] ArmVirtPkg/FdtPciPcdProducerLib: add discovery of PcdPciMmio64Size
2016-09-12 10:01 [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Ard Biesheuvel
2016-09-12 10:01 ` [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB Ard Biesheuvel
@ 2016-09-12 10:01 ` Ard Biesheuvel
2016-09-12 10:01 ` [PATCH 3/3] ArmVirtPkg/ArmVirtQemu: add IncompatiblePciDeviceSupportDxe Ard Biesheuvel
2016-09-12 11:57 ` [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Laszlo Ersek
3 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2016-09-12 10:01 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
In preparation of adding IncompatibleDeviceSupportDxe to ArmVirtQemu,
in order to lure the PCI code into allocating all 64-bit BARs in the
64-bit region, update FdtPciPcdProducerLib so it sets the PcdPciMmio64Size
based on the DT description of the PCIe root complex.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 31 ++++++++++++--------
ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 1 +
4 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index a3beb4654072..fa2b547ac486 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -206,6 +206,7 @@ [PcdsDynamicDefault.common]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xFFFFFFFFFFFFFFFF
gArmTokenSpaceGuid.PcdPciIoTranslation|0x0
+ gArmTokenSpaceGuid.PcdPciMmio64Size|0x0
#
# Set video resolution for boot options and for text setup.
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index e0dcf4300338..d80079c45c28 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -202,6 +202,7 @@ [PcdsDynamicDefault.common]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xFFFFFFFFFFFFFFFF
gArmTokenSpaceGuid.PcdPciIoTranslation|0x0
+ gArmTokenSpaceGuid.PcdPciMmio64Size|0x0
#
# Set video resolution for boot options and for text setup.
diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
index ea27cda7b77c..ea35c6df2546 100644
--- a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
+++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
@@ -44,11 +44,12 @@ typedef struct {
#define DTB_PCI_HOST_RANGE_TYPEMASK (BIT31 | BIT30 | BIT29 | BIT25 | BIT24)
STATIC
-RETURN_STATUS
-GetPciIoTranslation (
+VOID
+GetPciIoTranslationAndMmio64Size (
IN FDT_CLIENT_PROTOCOL *FdtClient,
IN INT32 Node,
- OUT UINT64 *IoTranslation
+ OUT UINT64 *IoTranslation,
+ OUT UINT64 *Mmio64Size
)
{
UINT32 RecordIdx;
@@ -64,24 +65,25 @@ GetPciIoTranslation (
if (EFI_ERROR (Status) || Len == 0 ||
Len % sizeof (DTB_PCI_HOST_RANGE_RECORD) != 0) {
DEBUG ((EFI_D_ERROR, "%a: 'ranges' not found or invalid\n", __FUNCTION__));
- return RETURN_PROTOCOL_ERROR;
+ return;
}
for (RecordIdx = 0; RecordIdx < Len / sizeof (DTB_PCI_HOST_RANGE_RECORD);
++RecordIdx) {
CONST DTB_PCI_HOST_RANGE_RECORD *Record;
- UINT32 Type;
Record = (CONST DTB_PCI_HOST_RANGE_RECORD *)Prop + RecordIdx;
- Type = SwapBytes32 (Record->Type) & DTB_PCI_HOST_RANGE_TYPEMASK;
- if (Type == DTB_PCI_HOST_RANGE_IO) {
+ switch (SwapBytes32 (Record->Type) & DTB_PCI_HOST_RANGE_TYPEMASK) {
+ case DTB_PCI_HOST_RANGE_IO:
IoBase = SwapBytes64 (Record->ChildBase);
*IoTranslation = SwapBytes64 (Record->CpuBase) - IoBase;
+ break;
- return RETURN_SUCCESS;
+ case DTB_PCI_HOST_RANGE_MMIO64:
+ *Mmio64Size = SwapBytes64 (Record->Size);
+ break;
}
}
- return RETURN_NOT_FOUND;
}
RETURN_STATUS
@@ -96,8 +98,8 @@ FdtPciPcdProducerLibConstructor (
UINT32 RegSize;
EFI_STATUS Status;
INT32 Node;
- RETURN_STATUS RetStatus;
UINT64 IoTranslation;
+ UINT64 Mmio64Size;
PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);
if (PciExpressBaseAddress != MAX_UINT64) {
@@ -128,9 +130,11 @@ FdtPciPcdProducerLibConstructor (
PcdSetBool (PcdPciDisableBusEnumeration, FALSE);
- IoTranslation = 0;
- RetStatus = GetPciIoTranslation (FdtClient, Node, &IoTranslation);
- if (!RETURN_ERROR (RetStatus)) {
+ IoTranslation = MAX_UINT64;
+ Mmio64Size = 0;
+ GetPciIoTranslationAndMmio64Size (FdtClient, Node, &IoTranslation,
+ &Mmio64Size);
+ if (IoTranslation != MAX_UINT64) {
PcdSet64 (PcdPciIoTranslation, IoTranslation);
} else {
//
@@ -142,6 +146,7 @@ FdtPciPcdProducerLibConstructor (
"%a: 'pci-host-ecam-generic' device encountered with no I/O range\n",
__FUNCTION__));
}
+ PcdSet64 (PcdPciMmio64Size, Mmio64Size);
}
}
diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
index cd138fa1aa6e..d29bcb0a801b 100644
--- a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
@@ -42,6 +42,7 @@ [Protocols]
[Pcd]
gArmTokenSpaceGuid.PcdPciIoTranslation ## PRODUCES
+ gArmTokenSpaceGuid.PcdPciMmio64Size ## PRODUCES
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress ## PRODUCES
gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration ## PRODUCES
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] ArmVirtPkg/ArmVirtQemu: add IncompatiblePciDeviceSupportDxe
2016-09-12 10:01 [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Ard Biesheuvel
2016-09-12 10:01 ` [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB Ard Biesheuvel
2016-09-12 10:01 ` [PATCH 2/3] ArmVirtPkg/FdtPciPcdProducerLib: add discovery of PcdPciMmio64Size Ard Biesheuvel
@ 2016-09-12 10:01 ` Ard Biesheuvel
2016-09-12 11:57 ` [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Laszlo Ersek
3 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2016-09-12 10:01 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
To prevent the generic PCI bus driver from allocating 64-bit MMIO BARs in
the 32-bit region for no good reason (since no good reasons exist on ARM),
add the IncompatiblePciDeviceSupportDxe driver to the build, which will
force override the policy to do so in the presence of a ROM BAR.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/ArmVirtQemu.dsc | 4 ++++
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ++++
3 files changed, 9 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index fa2b547ac486..47a77aaf87fa 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -370,6 +370,10 @@ [Components.common]
<LibraryClasses>
NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
}
+ ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf {
+ <LibraryClasses>
+ NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+ }
OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
OvmfPkg/Virtio10Dxe/Virtio10.inf
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index 2571884b20ac..18350d83b90d 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -159,6 +159,7 @@ [FV.FvMain]
INF ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
INF MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+ INF ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
INF OvmfPkg/Virtio10Dxe/Virtio10.inf
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index d80079c45c28..b6a54b81aa34 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -359,6 +359,10 @@ [Components.common]
<LibraryClasses>
NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
}
+ ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf {
+ <LibraryClasses>
+ NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+ }
OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
OvmfPkg/Virtio10Dxe/Virtio10.inf
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB
2016-09-12 10:01 ` [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB Ard Biesheuvel
@ 2016-09-12 10:23 ` Leif Lindholm
2016-09-12 12:29 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Leif Lindholm @ 2016-09-12 10:23 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek
On Mon, Sep 12, 2016 at 11:01:17AM +0100, Ard Biesheuvel wrote:
> By default, the generic PciBusDxe driver uses the presence of a ROM BAR
> as a hint that all MMIO BARs should be allocated in the 32-bit region,
> even the ones that could be allocated above 4 GB. The reason is that
> such a ROM BAR may contain code that runs in the context of a legacy
> BIOS (i.e., a CSM), and allocating the 64-bit BARs above 4 GB may put
> them out of reach.
>
> Of course, none of this matters on ARM, and so we can unconditionally
> override this decision. So take the OVMF implementation of the
> IncompatiblePciDeviceSupportDxe driver, rip out the bits that care
> about the presence of a legacy BIOS, and wire it up to the ArmPkg PCI
> PCDs.
While I agree this solves a real problem, we're now at a point where
we're adding multiple new implementations of a library to give
standards-compliant platforms the ability to circumvent legacy
workarounds in core code.
Should this not be the other way around?
And if not, is that not a policy decision that suggests the workaround
belongs as a core library anyway?
/
Leif
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c | 223 ++++++++++++++++++++
> ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf | 49 +++++
> 2 files changed, 272 insertions(+)
>
> diff --git a/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c b/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
> new file mode 100644
> index 000000000000..3f2869f8f3c2
> --- /dev/null
> +++ b/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
> @@ -0,0 +1,223 @@
> +/** @file
> + A simple DXE_DRIVER that causes the PCI Bus UEFI_DRIVER to allocate 64-bit
> + MMIO BARs above 4 GB, regardless of option ROM availability, conserving
> + 32-bit MMIO aperture for 32-bit BARs.
> +
> + Copyright (C) 2016, Red Hat, Inc.
> + Copyright (C) 2016, Linaro, Ltd.
> +
> + 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
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +**/
> +
> +#include <IndustryStandard/Acpi10.h>
> +#include <IndustryStandard/Pci22.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#include <Protocol/IncompatiblePciDeviceSupport.h>
> +
> +//
> +// The protocol interface this driver produces.
> +//
> +STATIC
> +EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL mIncompatiblePciDeviceSupport;
> +
> +//
> +// Configuration template for the CheckDevice() protocol member function.
> +//
> +// Refer to Table 20 "ACPI 2.0 & 3.0 QWORD Address Space Descriptor Usage" in
> +// the Platform Init 1.4a Spec, Volume 5.
> +//
> +// This structure is interpreted by the UpdatePciInfo() function in the edk2
> +// PCI Bus UEFI_DRIVER.
> +//
> +#pragma pack (1)
> +typedef struct {
> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR AddressSpaceDesc;
> + EFI_ACPI_END_TAG_DESCRIPTOR EndDesc;
> +} MMIO64_PREFERENCE;
> +#pragma pack ()
> +
> +STATIC CONST MMIO64_PREFERENCE mConfiguration = {
> + //
> + // AddressSpaceDesc
> + //
> + {
> + ACPI_ADDRESS_SPACE_DESCRIPTOR, // Desc
> + (UINT16)( // Len
> + sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
> + OFFSET_OF (
> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
> + ResType
> + )
> + ),
> + ACPI_ADDRESS_SPACE_TYPE_MEM, // ResType
> + PCI_ACPI_UNUSED, // GenFlag
> + PCI_ACPI_UNUSED, // SpecificFlag
> + 64, // AddrSpaceGranularity:
> + // aperture selection hint
> + // for BAR allocation
> + PCI_ACPI_UNUSED, // AddrRangeMin
> + PCI_BAR_OLD_ALIGN, // AddrRangeMax:
> + // no special alignment
> + // for affected BARs
> + PCI_BAR_ALL, // AddrTranslationOffset:
> + // hint covers all
> + // eligible BARs
> + PCI_BAR_NOCHANGE // AddrLen:
> + // use probed BAR size
> + },
> + //
> + // EndDesc
> + //
> + {
> + ACPI_END_TAG_DESCRIPTOR, // Desc
> + 0 // Checksum: to be ignored
> + }
> +};
> +
> +
> +/**
> + Returns a list of ACPI resource descriptors that detail the special resource
> + configuration requirements for an incompatible PCI device.
> +
> + Prior to bus enumeration, the PCI bus driver will look for the presence of
> + the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL. Only one instance of this
> + protocol can be present in the system. For each PCI device that the PCI bus
> + driver discovers, the PCI bus driver calls this function with the device's
> + vendor ID, device ID, revision ID, subsystem vendor ID, and subsystem device
> + ID. If the VendorId, DeviceId, RevisionId, SubsystemVendorId, or
> + SubsystemDeviceId value is set to (UINTN)-1, that field will be ignored. The
> + ID values that are not (UINTN)-1 will be used to identify the current device.
> +
> + This function will only return EFI_SUCCESS. However, if the device is an
> + incompatible PCI device, a list of ACPI resource descriptors will be returned
> + in Configuration. Otherwise, NULL will be returned in Configuration instead.
> + The PCI bus driver does not need to allocate memory for Configuration.
> + However, it is the PCI bus driver's responsibility to free it. The PCI bus
> + driver then can configure this device with the information that is derived
> + from this list of resource nodes, rather than the result of BAR probing.
> +
> + Only the following two resource descriptor types from the ACPI Specification
> + may be used to describe the incompatible PCI device resource requirements:
> + - QWORD Address Space Descriptor (ACPI 2.0, section 6.4.3.5.1; also ACPI 3.0)
> + - End Tag (ACPI 2.0, section 6.4.2.8; also ACPI 3.0)
> +
> + The QWORD Address Space Descriptor can describe memory, I/O, and bus number
> + ranges for dynamic or fixed resources. The configuration of a PCI root bridge
> + is described with one or more QWORD Address Space Descriptors, followed by an
> + End Tag. See the ACPI Specification for details on the field values.
> +
> + @param[in] This Pointer to the
> + EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL
> + instance.
> +
> + @param[in] VendorId A unique ID to identify the manufacturer of
> + the PCI device. See the Conventional PCI
> + Specification 3.0 for details.
> +
> + @param[in] DeviceId A unique ID to identify the particular PCI
> + device. See the Conventional PCI
> + Specification 3.0 for details.
> +
> + @param[in] RevisionId A PCI device-specific revision identifier.
> + See the Conventional PCI Specification 3.0
> + for details.
> +
> + @param[in] SubsystemVendorId Specifies the subsystem vendor ID. See the
> + Conventional PCI Specification 3.0 for
> + details.
> +
> + @param[in] SubsystemDeviceId Specifies the subsystem device ID. See the
> + Conventional PCI Specification 3.0 for
> + details.
> +
> + @param[out] Configuration A list of ACPI resource descriptors that
> + detail the configuration requirement.
> +
> + @retval EFI_SUCCESS The function always returns EFI_SUCCESS.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +CheckDevice (
> + IN EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL *This,
> + IN UINTN VendorId,
> + IN UINTN DeviceId,
> + IN UINTN RevisionId,
> + IN UINTN SubsystemVendorId,
> + IN UINTN SubsystemDeviceId,
> + OUT VOID **Configuration
> + )
> +{
> + //
> + // Unlike the general description of this protocol member suggests, there is
> + // nothing incompatible about the PCI devices that we'll match here. We'll
> + // match all PCI devices, and generate exactly one QWORD Address Space
> + // Descriptor for each. That descriptor will instruct the PCI Bus UEFI_DRIVER
> + // not to degrade 64-bit MMIO BARs for the device, even if a PCI option ROM
> + // BAR is present on the device.
> + //
> +
> + //
> + // This member function is mis-specified actually: it is supposed to allocate
> + // memory, but as specified, it could not return an error status. Thankfully,
> + // the edk2 PCI Bus UEFI_DRIVER actually handles error codes; see the
> + // UpdatePciInfo() function.
> + //
> + *Configuration = AllocateCopyPool (sizeof mConfiguration, &mConfiguration);
> + if (*Configuration == NULL) {
> + DEBUG ((EFI_D_WARN,
> + "%a: 64-bit MMIO BARs may be degraded for PCI 0x%04x:0x%04x (rev %d)\n",
> + __FUNCTION__, (UINT32)VendorId, (UINT32)DeviceId, (UINT8)RevisionId));
> + return EFI_OUT_OF_RESOURCES;
> + }
> + return EFI_SUCCESS;
> +}
> +
> +
> +/**
> + Entry point for this driver.
> +
> + @param[in] ImageHandle Image handle of this driver.
> + @param[in] SystemTable Pointer to SystemTable.
> +
> + @retval EFI_SUCESS Driver has loaded successfully.
> + @retval EFI_UNSUPPORTED PCI resource allocation has been disabled.
> + @retval EFI_UNSUPPORTED There is no 64-bit PCI MMIO aperture.
> + @return Error codes from lower level functions.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +DriverInitialize (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + //
> + // If the PCI Bus driver is not supposed to allocate resources, then it makes
> + // no sense to install a protocol that influences the resource allocation.
> + //
> + // Similarly, if there is no 64-bit PCI MMIO aperture, then 64-bit MMIO BARs
> + // have to be allocated under 4 GB unconditionally.
> + //
> + if (PcdGetBool (PcdPciDisableBusEnumeration) ||
> + PcdGet64 (PcdPciMmio64Size) == 0) {
> + return EFI_UNSUPPORTED;
> + }
> +
> + mIncompatiblePciDeviceSupport.CheckDevice = CheckDevice;
> + return gBS->InstallMultipleProtocolInterfaces (&ImageHandle,
> + &gEfiIncompatiblePciDeviceSupportProtocolGuid,
> + &mIncompatiblePciDeviceSupport, NULL);
> +}
> diff --git a/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf b/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
> new file mode 100644
> index 000000000000..0bda20199d51
> --- /dev/null
> +++ b/ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
> @@ -0,0 +1,49 @@
> +## @file
> +# A simple DXE_DRIVER that causes the PCI Bus UEFI_DRIVER to allocate 64-bit
> +# MMIO BARs above 4 GB, regardless of option ROM availability, conserving
> +# 32-bit MMIO aperture for 32-bit BARs.
> +#
> +# Copyright (C) 2016, Red Hat, Inc.
> +# Copyright (C) 2016, Linaro, Ltd.
> +#
> +# 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
> +# http://opensource.org/licenses/bsd-license.php
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = IncompatiblePciDeviceSupportDxe
> + FILE_GUID = 18BCB238-844E-446B-AA2D-3DF38A25FA0F
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT = DriverInitialize
> +
> +[Sources]
> + IncompatiblePciDeviceSupport.c
> +
> +[Packages]
> + ArmPkg/ArmPkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> + DebugLib
> + MemoryAllocationLib
> + PcdLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
> +
> +[Protocols]
> + gEfiIncompatiblePciDeviceSupportProtocolGuid ## PRODUCES
> +
> +[Pcd]
> + gArmTokenSpaceGuid.PcdPciMmio64Size ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration ## CONSUMES
> +
> +[Depex]
> + TRUE
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation
2016-09-12 10:01 [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Ard Biesheuvel
` (2 preceding siblings ...)
2016-09-12 10:01 ` [PATCH 3/3] ArmVirtPkg/ArmVirtQemu: add IncompatiblePciDeviceSupportDxe Ard Biesheuvel
@ 2016-09-12 11:57 ` Laszlo Ersek
2016-09-26 12:54 ` Ard Biesheuvel
3 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2016-09-12 11:57 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm
On 09/12/16 12:01, Ard Biesheuvel wrote:
> On ARM/Linux systems, the PCI layer is usually reconfigured from scratch,
> given the embedded legacy of ARM systems. However, when booting arm64/Linux
> in ACPI mode, such reconfiguration can be avoided, since ACPI implies UEFI
> implies firmware (as opposed to DT mode, which could be booted via QEMU's
> ~10 instruction bootloader)
>
> In this case, it is important for the firmware to leave the PCI configuration
> in a reasonable state, and one of the things that UEFI does by default, and
> which makes no sense at all on an arm64 system, is to degrade 64-bit MMIO BARs
> to 32-bit in the presence of a ROM BAR on the same device. (It does makes sense
> on an Intel system running legacy option ROMs under a CSM)
>
> Fortunately, we have a way of influencing this policy without having to hack
> the generic PCI bus driver: we can install the IncompatiblePciDeviceSupport
> protocol, and the PCI bus driver will invoke it for each PCI device it
> configures.
>
> Our implementation for ArmPkg is simply the OVMF version with the bits ripped
> out that care about CSMs and legacy BIOSes
>
> Ard Biesheuvel (3):
> ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4
> GB
> ArmVirtPkg/FdtPciPcdProducerLib: add discovery of PcdPciMmio64Size
> ArmVirtPkg/ArmVirtQemu: add IncompatiblePciDeviceSupportDxe
>
> ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c | 223 ++++++++++++++++++++
> ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf | 49 +++++
> ArmVirtPkg/ArmVirtQemu.dsc | 5 +
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 +
> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 31 +--
> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 1 +
> 7 files changed, 302 insertions(+), 13 deletions(-)
> create mode 100644 ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
> create mode 100644 ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>
Formatting / reviewing the first patch with "--find-copies-harder" works
wonders, so I recommend that flag for similar future posts.
Series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB
2016-09-12 10:23 ` Leif Lindholm
@ 2016-09-12 12:29 ` Laszlo Ersek
2016-09-12 13:06 ` Ard Biesheuvel
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2016-09-12 12:29 UTC (permalink / raw)
To: Leif Lindholm, Ard Biesheuvel; +Cc: edk2-devel
On 09/12/16 12:23, Leif Lindholm wrote:
> On Mon, Sep 12, 2016 at 11:01:17AM +0100, Ard Biesheuvel wrote:
>> By default, the generic PciBusDxe driver uses the presence of a ROM BAR
>> as a hint that all MMIO BARs should be allocated in the 32-bit region,
>> even the ones that could be allocated above 4 GB. The reason is that
>> such a ROM BAR may contain code that runs in the context of a legacy
>> BIOS (i.e., a CSM), and allocating the 64-bit BARs above 4 GB may put
>> them out of reach.
>>
>> Of course, none of this matters on ARM, and so we can unconditionally
>> override this decision. So take the OVMF implementation of the
>> IncompatiblePciDeviceSupportDxe driver, rip out the bits that care
>> about the presence of a legacy BIOS, and wire it up to the ArmPkg PCI
>> PCDs.
>
> While I agree this solves a real problem, we're now at a point where
> we're adding multiple new implementations of a library
s/library/protocol/
> to give
> standards-compliant platforms the ability to circumvent legacy
> workarounds in core code.
>
> Should this not be the other way around?
Yes, it should be.
... IMO, the specific degradation logic in the PCI Bus driver is
simplistic; for example, if it finds a PCI oprom for the device, it
could parse it to see if there's a legacy BIOS image (too) in that
oprom, before summarily degrading the resource.
But, implementing that in PciBusDxe now, or even eliminating the quirk
completely, would require all downstreams to make up for it in their
IncompatiblePciDeviceSupportDxe drivers. Getting shortcuts into core
code is easy (initally), getting them out is not so much.
So, I completely agree with you, especially that we're abusing this
protocol as a resource allocation hint -- as the patches themselves say,
there's nothing "incompatible" about the devices we handle. But,
changing the status quo at this stage looks really impractical.
> And if not, is that not a policy decision that suggests the workaround
> belongs as a core library anyway?
Based on the PI spec (1.4a, vol 5, "11.5 Incompatible PCI Device Support
Protocol"), the protocol should be provided by a platform driver.
Now, PciHostBridgeDriverDxe is also a platform driver. I think it's
different from this case because PciHostBridgeDriverDxe has so much
common / shared logic that it could be meaningfully split into a
PciHostBridgeLib class, and a common driver base (linking against the
lib class). For IncompatiblePciDeviceSupportDxe however, at least on the
level that we work with it, there's zero common logic to factor out.
(Okay, yes, you could centralize one or two PcdGet() calls, and perhaps
one of the PCDs could be moved to a central .DEC file.)
I don't like this situation either, but it's how ABI specs designed for
the proprietary world work in practice, I guess.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB
2016-09-12 12:29 ` Laszlo Ersek
@ 2016-09-12 13:06 ` Ard Biesheuvel
0 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2016-09-12 13:06 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Leif Lindholm, edk2-devel@lists.01.org
On 12 September 2016 at 13:29, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/12/16 12:23, Leif Lindholm wrote:
>> On Mon, Sep 12, 2016 at 11:01:17AM +0100, Ard Biesheuvel wrote:
>>> By default, the generic PciBusDxe driver uses the presence of a ROM BAR
>>> as a hint that all MMIO BARs should be allocated in the 32-bit region,
>>> even the ones that could be allocated above 4 GB. The reason is that
>>> such a ROM BAR may contain code that runs in the context of a legacy
>>> BIOS (i.e., a CSM), and allocating the 64-bit BARs above 4 GB may put
>>> them out of reach.
>>>
>>> Of course, none of this matters on ARM, and so we can unconditionally
>>> override this decision. So take the OVMF implementation of the
>>> IncompatiblePciDeviceSupportDxe driver, rip out the bits that care
>>> about the presence of a legacy BIOS, and wire it up to the ArmPkg PCI
>>> PCDs.
>>
>> While I agree this solves a real problem, we're now at a point where
>> we're adding multiple new implementations of a library
>
> s/library/protocol/
>
>> to give
>> standards-compliant platforms the ability to circumvent legacy
>> workarounds in core code.
>>
>> Should this not be the other way around?
>
> Yes, it should be.
>
> ... IMO, the specific degradation logic in the PCI Bus driver is
> simplistic; for example, if it finds a PCI oprom for the device, it
> could parse it to see if there's a legacy BIOS image (too) in that
> oprom, before summarily degrading the resource.
>
> But, implementing that in PciBusDxe now, or even eliminating the quirk
> completely, would require all downstreams to make up for it in their
> IncompatiblePciDeviceSupportDxe drivers. Getting shortcuts into core
> code is easy (initally), getting them out is not so much.
>
True, but I have sent out a patch nonetheless. Let's at least have the
discussion.
> So, I completely agree with you, especially that we're abusing this
> protocol as a resource allocation hint -- as the patches themselves say,
> there's nothing "incompatible" about the devices we handle. But,
> changing the status quo at this stage looks really impractical.
>
>> And if not, is that not a policy decision that suggests the workaround
>> belongs as a core library anyway?
>
> Based on the PI spec (1.4a, vol 5, "11.5 Incompatible PCI Device Support
> Protocol"), the protocol should be provided by a platform driver.
>
> Now, PciHostBridgeDriverDxe is also a platform driver. I think it's
> different from this case because PciHostBridgeDriverDxe has so much
> common / shared logic that it could be meaningfully split into a
> PciHostBridgeLib class, and a common driver base (linking against the
> lib class). For IncompatiblePciDeviceSupportDxe however, at least on the
> level that we work with it, there's zero common logic to factor out.
> (Okay, yes, you could centralize one or two PcdGet() calls, and perhaps
> one of the PCDs could be moved to a central .DEC file.)
>
> I don't like this situation either, but it's how ABI specs designed for
> the proprietary world work in practice, I guess.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation
2016-09-12 11:57 ` [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Laszlo Ersek
@ 2016-09-26 12:54 ` Ard Biesheuvel
0 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2016-09-26 12:54 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Leif Lindholm
On 12 September 2016 at 04:57, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/12/16 12:01, Ard Biesheuvel wrote:
>> On ARM/Linux systems, the PCI layer is usually reconfigured from scratch,
>> given the embedded legacy of ARM systems. However, when booting arm64/Linux
>> in ACPI mode, such reconfiguration can be avoided, since ACPI implies UEFI
>> implies firmware (as opposed to DT mode, which could be booted via QEMU's
>> ~10 instruction bootloader)
>>
>> In this case, it is important for the firmware to leave the PCI configuration
>> in a reasonable state, and one of the things that UEFI does by default, and
>> which makes no sense at all on an arm64 system, is to degrade 64-bit MMIO BARs
>> to 32-bit in the presence of a ROM BAR on the same device. (It does makes sense
>> on an Intel system running legacy option ROMs under a CSM)
>>
>> Fortunately, we have a way of influencing this policy without having to hack
>> the generic PCI bus driver: we can install the IncompatiblePciDeviceSupport
>> protocol, and the PCI bus driver will invoke it for each PCI device it
>> configures.
>>
>> Our implementation for ArmPkg is simply the OVMF version with the bits ripped
>> out that care about CSMs and legacy BIOSes
>>
>> Ard Biesheuvel (3):
>> ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4
>> GB
>> ArmVirtPkg/FdtPciPcdProducerLib: add discovery of PcdPciMmio64Size
>> ArmVirtPkg/ArmVirtQemu: add IncompatiblePciDeviceSupportDxe
>>
>> ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c | 223 ++++++++++++++++++++
>> ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf | 49 +++++
>> ArmVirtPkg/ArmVirtQemu.dsc | 5 +
>> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 +
>> ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 +
>> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 31 +--
>> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 1 +
>> 7 files changed, 302 insertions(+), 13 deletions(-)
>> create mode 100644 ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
>> create mode 100644 ArmPkg/Drivers/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>>
>
> Formatting / reviewing the first patch with "--find-copies-harder" works
> wonders, so I recommend that flag for similar future posts.
>
> Series
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
Thanks all, but this series is no longer needed now that the PCI
64-bit MMIO BAR degradation quirk has been made X86-only
So dropping this series.
--
Ard.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-26 12:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-12 10:01 [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Ard Biesheuvel
2016-09-12 10:01 ` [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB Ard Biesheuvel
2016-09-12 10:23 ` Leif Lindholm
2016-09-12 12:29 ` Laszlo Ersek
2016-09-12 13:06 ` Ard Biesheuvel
2016-09-12 10:01 ` [PATCH 2/3] ArmVirtPkg/FdtPciPcdProducerLib: add discovery of PcdPciMmio64Size Ard Biesheuvel
2016-09-12 10:01 ` [PATCH 3/3] ArmVirtPkg/ArmVirtQemu: add IncompatiblePciDeviceSupportDxe Ard Biesheuvel
2016-09-12 11:57 ` [PATCH 0/3] ArmPkg ArmVirtPkg: prevent 64-bit MMIO BAR degradation Laszlo Ersek
2016-09-26 12:54 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox