Re: [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB

[Leif Lindholm <leif.lindholm@linaro.org>]

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
>