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 51E2381CE2 for ; Tue, 1 Nov 2016 15:43:44 -0700 (PDT) Received: by mail-wm0-x232.google.com with SMTP id 79so41766789wmy.0 for ; Tue, 01 Nov 2016 15:43:45 -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=nSB6w1ZP9pemT6llvwwjC+ifK2X7LoqKbXS98aZE1ZE=; b=e++MCdOaqGYJdXFZb1SglZu3x0unZb0U1K7P1xOHFCi1Wa8rvpBF2Tq/e8HfjTXU31 aVykOqN0Ew0vAueo+Mets9fb6Pv5BAIvtyD/ZqmzwXlN4xRCb+Q/XidqbRw+UCAaGzvR f5IBhbxMa9+zPULCuYlGKYE1KkQB7ke+Hw0QI= 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=nSB6w1ZP9pemT6llvwwjC+ifK2X7LoqKbXS98aZE1ZE=; b=F3DYUvOS1070C2f8Hs6bkWIuB02ZLuuSm7UbBc7eT2qXzoNf1PNQpM7daU8gLWy1xD 6xHaOrB+Z07hnIKtTV0T9HfPNi8gnoSgp+bWdj6Pqc1MWLdeROVSNtpU8XSdc7Z+OVgo H+9il9+ftPIF8OeSpIVLqEEXWi97llHY4VXZ9aWa6YqJVwqHQ8e9xi8ff677XTknszFK ueuX3CBFW8CJIx+CUT4aymo0DEEkNcjqFi7+fATe6rumAoJiQnmdp7UHyf0KyFY4cyI1 zhcv+cKVk25DOuTCrF6+6u7cK9WbsJI8y5oPLSbW+vYOeY2S/GiBhezJzcqf4WrEPocp rQPA== X-Gm-Message-State: ABUngvcYIoeTTqySxBidegoy7Aviw+0BOZwL5S7t4pitr5hDwh1hKsu4jL6qW8Lto4/hZlpd X-Received: by 10.28.5.207 with SMTP id 198mr258819wmf.22.1478040224282; Tue, 01 Nov 2016 15:43:44 -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 r1sm37953917wjc.43.2016.11.01.15.43.43 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 01 Nov 2016 15:43:43 -0700 (PDT) Date: Tue, 1 Nov 2016 22:43:42 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org Message-ID: <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> MIME-Version: 1.0 In-Reply-To: <1477937590-10361-6-git-send-email-ard.biesheuvel@linaro.org> 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: Tue, 01 Nov 2016 22:43:44 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. 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? 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? > // > // 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 >