From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22e.google.com (mail-it0-x22e.google.com [IPv6:2607:f8b0:4001:c0b::22e]) (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 C8C6381CE2 for ; Wed, 2 Nov 2016 06:43:41 -0700 (PDT) Received: by mail-it0-x22e.google.com with SMTP id f129so17873133itc.1 for ; Wed, 02 Nov 2016 06:43:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=v5Jdc0xadCsrb3G2whYqQxy/BYEM8SZpqY/i+qLAS8I=; b=ZUFw7Up3nqwbVL9WSTWWyiXSCBVgzx9Su1sURIKm3soZhrkgt6Z1YhsVeFU2mc0Y1i Pncp0W1Lxf7ZZWkgnRjBCM7dSNJZkYMncJhhvIpBIpzk92xF7gVgIX6hRL6ZrABsvr8Q fQcSDNpOsr7rjup0FsOv7NzFqGRVb3H0xKmEk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=v5Jdc0xadCsrb3G2whYqQxy/BYEM8SZpqY/i+qLAS8I=; b=gFM750e00DkVTgoCfJgZfuEHZ89Lf8FKNxV5RSy2hpdi+Dw8b58i+uRizqbRj5iAO2 43yMmbNFrz60L95Xt1zmmyw9fqynlTQPs/R6EuO+2MuYuVtTCvXlSVImGRt2vWkjGgX4 aD4Dsz4pwusaADQOlGlQYIczGOtj3Dg+UpntuW5uo1usWpBMt8IFqh+jmpiWhqvVEecU vRquDP0nxEKW9B2905a3VL2R9FE4QEESKquGAnhx+tIEwcSV83wAVEj2nUatUVQ8n6UR cZJa+hqwo/FxzVBkUSpxFV4qb+j3iFpURfGKaSI2llQkDA5qAES3ZU1A5xmlfNSDbOt+ i24A== X-Gm-Message-State: ABUngvdoVGkZY4EVRZTSEYvDR8QlOsf71qp8NljRRJT0ehGaikenZH4DlXkUdswU9+cYxJxxPOCAADNBp6JrQCk6 X-Received: by 10.107.25.11 with SMTP id 11mr4224358ioz.138.1478094222730; Wed, 02 Nov 2016 06:43:42 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.59.147 with HTTP; Wed, 2 Nov 2016 06:43:42 -0700 (PDT) In-Reply-To: <20161101224342.GQ1161@bivouac.eciton.net> References: <1477937590-10361-1-git-send-email-ard.biesheuvel@linaro.org> <1477937590-10361-6-git-send-email-ard.biesheuvel@linaro.org> <20161101224342.GQ1161@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 2 Nov 2016 13:43:42 +0000 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 Subject: Re: [PATCH 5/5] EmbeddedPkg/PlatformPciIoDxe: add support for non-coherent DMA 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: Wed, 02 Nov 2016 13:43:42 -0000 Content-Type: text/plain; charset=UTF-8 On 1 November 2016 at 22:43, Leif Lindholm wrote: > On Mon, Oct 31, 2016 at 06:13:10PM +0000, Ard Biesheuvel wrote: >> Add support for bounce buffering using uncached mappings when DMA mappings >> are not aligned to the CPU's DMA buffer alignment. > > This description does not appear to match the subject line? > And the contents of the patch seems to do both. > Yes, I was a bit sloppy with this one. The subject line needs to be duplicated into the long log. > Anyway, I'll pass on a proper technical review until my head is a bit > clearer, but a few comments/questions below. > >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel >> --- >> EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c | 204 ++++++++++++++++++++ >> EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h | 5 + >> EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c | 17 +- >> EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf | 2 + >> 4 files changed, 221 insertions(+), 7 deletions(-) >> >> diff --git a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c >> index 97ed19353347..658d096c73c1 100644 >> --- a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c >> +++ b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.c >> @@ -15,6 +15,8 @@ >> >> #include "PlatformPciIo.h" >> >> +#include >> + >> #include >> >> typedef struct { >> @@ -454,6 +456,201 @@ CoherentPciIoFreeBuffer ( >> return EFI_SUCCESS; >> } >> >> +STATIC >> +EFI_STATUS >> +NonCoherentPciIoFreeBuffer ( >> + IN EFI_PCI_IO_PROTOCOL *This, >> + IN UINTN Pages, >> + IN VOID *HostAddress >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + Status = gDS->SetMemorySpaceAttributes ( >> + (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, >> + EFI_PAGES_TO_SIZE (Pages), >> + EFI_MEMORY_WB); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + FreePages (HostAddress, Pages); >> + return EFI_SUCCESS; >> +} >> + >> +STATIC >> +EFI_STATUS >> +NonCoherentPciIoAllocateBuffer ( >> + IN EFI_PCI_IO_PROTOCOL *This, >> + IN EFI_ALLOCATE_TYPE Type, >> + IN EFI_MEMORY_TYPE MemoryType, >> + IN UINTN Pages, >> + OUT VOID **HostAddress, >> + IN UINT64 Attributes >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + Status = CoherentPciIoAllocateBuffer (This, Type, MemoryType, Pages, >> + HostAddress, Attributes); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + Status = gDS->SetMemorySpaceAttributes ( >> + (EFI_PHYSICAL_ADDRESS)(UINTN)*HostAddress, >> + EFI_PAGES_TO_SIZE (Pages), >> + EFI_MEMORY_WC); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + Status = gCpu->FlushDataCache ( >> + gCpu, >> + (EFI_PHYSICAL_ADDRESS)(UINTN)*HostAddress, >> + EFI_PAGES_TO_SIZE (Pages), >> + EfiCpuFlushTypeInvalidate); >> + if (EFI_ERROR (Status)) { >> + NonCoherentPciIoFreeBuffer (This, Pages, *HostAddress); >> + } >> + return Status; >> +} >> + >> +STATIC >> +EFI_STATUS >> +NonCoherentPciIoMap ( >> + IN EFI_PCI_IO_PROTOCOL *This, >> + IN EFI_PCI_IO_PROTOCOL_OPERATION Operation, >> + IN VOID *HostAddress, >> + IN OUT UINTN *NumberOfBytes, >> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, >> + OUT VOID **Mapping >> + ) >> +{ >> + PLATFORM_PCI_IO_DEV *Dev; >> + EFI_STATUS Status; >> + PLATFORM_PCI_IO_MAP_INFO *MapInfo; >> + UINTN AlignMask; >> + VOID *AllocAddress; >> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; >> + BOOLEAN UncachedMapping; >> + >> + MapInfo = (PLATFORM_PCI_IO_MAP_INFO *)AllocatePool (sizeof *MapInfo); >> + if (MapInfo == NULL) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + >> + MapInfo->HostAddress = HostAddress; >> + MapInfo->Operation = Operation; >> + MapInfo->NumberOfBytes = *NumberOfBytes; >> + >> + // >> + // Bounce buffering is not possible for consistent mappings, so >> + // check we are mapping a cached buffer for consistent DMA >> + // >> + if (Operation == EfiPciIoOperationBusMasterCommonBuffer) { >> + Status = gDS->GetMemorySpaceDescriptor ( >> + (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, >> + &GcdDescriptor); >> + if (!EFI_ERROR (Status)) { >> + UncachedMapping = (GcdDescriptor.Attributes & EFI_MEMORY_WC) != 0; >> + } else { >> + UncachedMapping = FALSE; >> + } >> + } >> + >> + // >> + // If this device does not support 64-bit DMA addressing, we need to allocate >> + // a bounce buffer and copy over the data if HostAddress >= 4 GB. We also need >> + // to allocate a bounce buffer if the mapping is not aligned to the CPU's >> + // DMA buffer alignment. >> + // >> + Dev = PLATFORM_PCI_IO_DEV_FROM_PCI_IO(This); >> + AlignMask = gCpu->DmaBufferAlignment - 1; >> + if (((Dev->Attributes & EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0 && >> + (UINTN) HostAddress >= SIZE_4GB) || >> + ((((UINTN) HostAddress || *NumberOfBytes) & AlignMask) != 0 && >> + !UncachedMapping)) { >> + >> + Status = NonCoherentPciIoAllocateBuffer (This, AllocateAnyPages, >> + EfiBootServicesData, EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes), >> + &AllocAddress, 0); >> + if (EFI_ERROR (Status)) { >> + goto FreeMapInfo; >> + } >> + MapInfo->AllocAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocAddress; >> + if (Operation == EfiPciIoOperationBusMasterRead) { >> + gBS->CopyMem (AllocAddress, HostAddress, *NumberOfBytes); >> + } >> + *DeviceAddress = MapInfo->AllocAddress; >> + } else { >> + MapInfo->AllocAddress = 0; >> + *DeviceAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress; >> + >> + // >> + // We are not using a bounce buffer: the mapping is sufficiently >> + // aligned to allow us to simply flush the caches. Note that cleaning >> + // the caches is necessary for both data directions: >> + // - for bus master read, we want the latest data to be present >> + // in main memory >> + // - for bus master write, we don't want any stale dirty cachelines that >> + // may be written back unexpectedly, and clobber the data written to >> + // main memory by the device. >> + // >> + gCpu->FlushDataCache (gCpu, (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, >> + *NumberOfBytes, EfiCpuFlushTypeWriteBack); >> + } >> + >> + *Mapping = MapInfo; >> + return EFI_SUCCESS; >> + >> +FreeMapInfo: >> + FreePool (MapInfo); >> + >> + return Status; >> +} >> + >> +STATIC >> +EFI_STATUS >> +NonCoherentPciIoUnmap ( >> + IN EFI_PCI_IO_PROTOCOL *This, >> + IN VOID *Mapping >> + ) >> +{ >> + PLATFORM_PCI_IO_MAP_INFO *MapInfo; >> + >> + if (Mapping == NULL) { >> + return EFI_DEVICE_ERROR; >> + } >> + >> + MapInfo = Mapping; >> + if (MapInfo->AllocAddress != 0) { >> + // >> + // We are using a bounce buffer: copy back the data if necessary, >> + // and free the buffer. >> + // >> + if (MapInfo->Operation == EfiPciIoOperationBusMasterWrite) { >> + gBS->CopyMem (MapInfo->HostAddress, (VOID *)MapInfo->AllocAddress, >> + MapInfo->NumberOfBytes); >> + } >> + NonCoherentPciIoFreeBuffer (This, >> + EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes), >> + (VOID *)(UINTN)MapInfo->AllocAddress); >> + } else { >> + // >> + // We are *not* using a bounce buffer: if this is a bus master write, >> + // we have to invalidate the caches so the CPU will see the uncached >> + // data written by the device. >> + // >> + if (MapInfo->Operation == EfiPciIoOperationBusMasterWrite) { >> + gCpu->FlushDataCache (gCpu, >> + (EFI_PHYSICAL_ADDRESS)(UINTN)MapInfo->HostAddress, >> + MapInfo->NumberOfBytes, EfiCpuFlushTypeInvalidate); >> + } >> + } >> + FreePool (MapInfo); >> + return EFI_SUCCESS; >> +} >> >> STATIC >> EFI_STATUS >> @@ -646,4 +843,11 @@ InitializePciIoProtocol ( >> >> // Copy protocol structure >> CopyMem(&PlatformPciIoDev->PciIo, &PciIoTemplate, sizeof PciIoTemplate); >> + >> + if (PlatformPciIoDev->PlatformPciIo->DmaType == PlatformPciIoDmaNonCoherent) { >> + PlatformPciIoDev->PciIo.AllocateBuffer = NonCoherentPciIoAllocateBuffer; >> + PlatformPciIoDev->PciIo.FreeBuffer = NonCoherentPciIoFreeBuffer; >> + PlatformPciIoDev->PciIo.Map = NonCoherentPciIoMap; >> + PlatformPciIoDev->PciIo.Unmap = NonCoherentPciIoUnmap; >> + } >> } >> diff --git a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h >> index 8fd8dc5e4a11..b7b792b85ae4 100644 >> --- a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h >> +++ b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIo.h >> @@ -12,6 +12,8 @@ >> >> **/ >> >> +#include >> + >> #include >> #include >> #include >> @@ -20,6 +22,7 @@ >> >> #include >> >> +#include >> #include >> #include >> >> @@ -28,6 +31,8 @@ >> #define PLATFORM_PCI_IO_DEV_FROM_PCI_IO(PciIoPointer) \ >> CR (PciIoPointer, PLATFORM_PCI_IO_DEV, PciIo, PLATFORM_PCI_IO_SIG) >> >> +extern EFI_CPU_ARCH_PROTOCOL *gCpu; >> + >> typedef struct { >> UINT32 Signature; >> // >> diff --git a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c >> index 7f3306e7e891..fa4719686a6d 100644 >> --- a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c >> +++ b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.c >> @@ -17,6 +17,8 @@ >> #include >> #include >> >> +EFI_CPU_ARCH_PROTOCOL *gCpu; >> + > > Should this not really be pulled in from some header file? > No. But it should be called mCpu not gCpu > Maybe including it straight from DxeMain.h isn't entirely kosher even > if this code does end up in MdeModulePkg, but surely the declaration > there should also be pulled in from somewhere under UefiCpuPkg/Include? > It is simply a reference we hold to an architectural protocol, so it is no different from any other driver that finds a protocol using LocateProtocol and stashes the pointer. >> // >> // Probe, start and stop functions of this driver, called by the DXE core for >> // specific devices. >> @@ -60,13 +62,9 @@ PlatformPciIoDriverBindingSupported ( >> case PlatformPciIoDeviceEhci: >> case PlatformPciIoDeviceXhci: >> case PlatformPciIoDeviceAhci: >> - // >> - // Restricted to DMA coherent for now >> - // >> - if (PlatformPciIo->DmaType == PlatformPciIoDmaCoherent) { >> - Status = EFI_SUCCESS; >> - break; >> - } >> + Status = EFI_SUCCESS; >> + break; >> + >> default: >> Status = EFI_UNSUPPORTED; >> } >> @@ -257,6 +255,11 @@ PlatformPciIoDxeEntryPoint ( >> IN EFI_SYSTEM_TABLE *SystemTable >> ) >> { >> + EFI_STATUS Status; >> + >> + Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&gCpu); >> + ASSERT_EFI_ERROR(Status); >> + >> return EfiLibInstallDriverBindingComponentName2 ( >> ImageHandle, >> SystemTable, >> diff --git a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf >> index 2b0baf06732c..670dcc5a9ff2 100644 >> --- a/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf >> +++ b/EmbeddedPkg/Drivers/PlatformPciIoDxe/PlatformPciIoDxe.inf >> @@ -31,11 +31,13 @@ [Packages] >> [LibraryClasses] >> BaseMemoryLib >> DebugLib >> + DxeServicesTableLib >> MemoryAllocationLib >> UefiBootServicesTableLib >> UefiDriverEntryPoint >> UefiLib >> >> [Protocols] >> + gEfiCpuArchProtocolGuid ## CONSUMES >> gEfiPciIoProtocolGuid ## BY_START >> gPlatformPciIoProtocolGuid ## TO_START >> -- >> 2.7.4 >>