From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EDEDA1A1E0A for ; Mon, 12 Sep 2016 03:23:59 -0700 (PDT) Received: by mail-wm0-x233.google.com with SMTP id 1so136073466wmz.1 for ; Mon, 12 Sep 2016 03:23:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=sDrFigwR1jAY73ncULzjNornqSJA6nlaEp4P/FfrNyo=; b=RIYGhib9+lDBSsM0SOxTcZBZohXQfEhg86FuChdoC1Mq9lbFYse9dgHXQqgY8oPgEK k17zzZuKxWX9myK4ZdNHpxkCm9KN6l7J5+tvOJFgzq3CNSfHS/fbQugdXj73rMbBZbwc NlIdPjrNN22OidRxzyjIz9JDRO5CMONDaAHGg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=sDrFigwR1jAY73ncULzjNornqSJA6nlaEp4P/FfrNyo=; b=GIzb4d5j/9sLtnjHdwDcPsOVOZAy9WDiUo7BfKWVpbgI4MEyHK8MTDUEbC1KWK8tu3 p6YyoOjv5crEQJHvpkQDMLsdinNwXgj7AV9c2L7P5TzgtxnJT/VYgE3ZN0uLfThMty1r bepCkioXhlyqCnmmwGDel/5L/q+/hF5Iwu9F1h7HKfMQw0RZM7PJ3Otx/Al/AlP821Ac OnSxDU6ONSXK07jG7mEO1LIbOxPxJP8oDwwQQdevEyEdtyXyw238bFXQ+efQ/jmCqkk6 PzEKy9w3W/mEsxyWqivIoqp7av8vym9IE4xJjAR30eJthozawjhacFD/OrlIp4LDu3vs MRZg== X-Gm-Message-State: AE9vXwPTyUNh3LbddJfoghHMYdOSlN48o5eqDirEV3c92H8Wyjd8kX2RzpwHTOjgPJ0zxZff X-Received: by 10.194.97.242 with SMTP id ed18mr6358623wjb.188.1473675838470; Mon, 12 Sep 2016 03:23:58 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id m68sm4312038wmg.6.2016.09.12.03.23.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 12 Sep 2016 03:23:57 -0700 (PDT) Date: Mon, 12 Sep 2016 11:23:56 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, lersek@redhat.com Message-ID: <20160912102356.GS16080@bivouac.eciton.net> References: <1473674479-20207-1-git-send-email-ard.biesheuvel@linaro.org> <1473674479-20207-2-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1473674479-20207-2-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 1/3] ArmPkg: add driver to force 64-bit MMIO BARs to be allocated above 4 GB X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Sep 2016 10:24:00 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 > +#include > + > +#include > +#include > +#include > +#include > + > +#include > + > +// > +// 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 >