From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x232.google.com (mail-wm0-x232.google.com [IPv6:2a00:1450:400c:c09::232]) (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 0757781D36 for ; Wed, 2 Nov 2016 09:17:56 -0700 (PDT) Received: by mail-wm0-x232.google.com with SMTP id p190so279015133wmp.1 for ; Wed, 02 Nov 2016 09:17:57 -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=eFAQUWD47m0zujLXDGb3ECCTlxqf28PQFpELuxGsPhc=; b=J2ehS0hLHWMaKJ92aMMU95Qee2kwlL0Awm278KqMn97FmrlfrtCURu0jZ2hwmMHCCO jxpSBtFZzqN+NhyDXc2F+YouPy5z+MoNurYTCG1VQ1EC8x86Ba3+yYkZnLJYlSToob02 ire+8lssMtLVtX56VUHOD/5+wm4hCjBajIuXI= 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=eFAQUWD47m0zujLXDGb3ECCTlxqf28PQFpELuxGsPhc=; b=DsfJo+putkAJ45GW48zVieRbo4/908FFRQ8Odp9O7zYPWJ9FKBPqWhMjQW9rr1JIy1 R7dNnnVll+aE2GBZYRdupahvgb3p+XTiMg3x75AfQznnhXj+epDTftuWsAy0MBRnWXFz q673wxymToS9tUzEDU8KMO/v4/nCXU64oDTja+9fJCwC6DltQ96+vWGVd8BA5foKGtpI RdTubFDhNgNCqmDgX5MUYC295SjYc/U1xqJWBKh7DuEqvwDOXWCtRWNIZ2Gspc3wfhnp ulEI/McwhGPbrIX5q8ms4JIxdchQ1rgyGq91UWqrOZ20vJTOILgfl3bjMP1N8icy+bQv eRRw== X-Gm-Message-State: ABUngveEwPwVurmE99U6S55ssMqctk6k0juk12tMFdlEb7tbDMdCU6OCg55xLq37+tdCeUst X-Received: by 10.195.17.165 with SMTP id gf5mr3547947wjd.114.1478103476125; Wed, 02 Nov 2016 09:17:56 -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 pd2sm3591103wjb.31.2016.11.02.09.17.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 Nov 2016 09:17:54 -0700 (PDT) Date: Wed, 2 Nov 2016 16:17:53 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel-01 Message-ID: <20161102161753.GE27644@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> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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 16:17:56 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Nov 02, 2016 at 01:43:42PM +0000, Ard Biesheuvel wrote: > 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 Ah, that makes a lot more sense then, thanks. I also confusingly added the comment here, rather than 15 lines earlier where I was intending to. / Leif > > 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. Right, as mCpu that makes complete sense. > >> // > >> // 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 > >>