From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x236.google.com (mail-io0-x236.google.com [IPv6:2607:f8b0:4001:c06::236]) (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 41DB581C51 for ; Wed, 14 Dec 2016 01:15:05 -0800 (PST) Received: by mail-io0-x236.google.com with SMTP id d9so29984820ioe.0 for ; Wed, 14 Dec 2016 01:15:05 -0800 (PST) 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=HorOEQ5ZfUu4iLSTd6PeW10kSwgKkMpKdCSJU9MXUEc=; b=NmO4WeSgajmRvIjxFEXG3ozw0pRQfBFpNr5yyf45Eu5OgNaKFuFyo4CatePJPj3h6W twlBN9Y2Cu3K26RTW1GXLqB+WK/bNMUWzHq1xFgkAWAx+lI0CTm6LqcODZ0d/e979AZX bcJsvJ3LTeFmPi80KWwNpRcum6byM73/d6TyI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=HorOEQ5ZfUu4iLSTd6PeW10kSwgKkMpKdCSJU9MXUEc=; b=sl3LNSHghbibo/O51s7V3OqpS70/KdcAiLfB8/vwprpLY7jNu6+gGw9jN3S7E0ra2A zz8CZd/Y6B006pfWq6fcOl/DZmD5o3jMvCIzsx3zAieyiXKMQjoMXhVFFE0uS3Uj80sN Hv/XDJfkFOea+kqzRhTWpN1QEdvhKZOVW7ZykJDStECc7NqxOXfrLEtlKelUxKvXeztn oW0K/dJcVb3LUWSG4ZwOxGf09S0myOJe+mJmkYFLKot2Y2OxmD87ckhXweRVyzDQ8J90 U3vHxw/eSh0A9VSNskRat3hVl+KVpEm7pNoAYOrv5t3mMpssYHJXdAh2volHWtoUxExN B8aw== X-Gm-Message-State: AKaTC01GjFTa+ISNC/cVhYfSVi3DbfJXhkBpW4QQ8TbLw8L0me0jgPT/uNUQRIV+wWHCi9I3fxpkoeO+U2Fks3Qn X-Received: by 10.107.2.8 with SMTP id 8mr80981734ioc.83.1481706904086; Wed, 14 Dec 2016 01:15:04 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.198.67 with HTTP; Wed, 14 Dec 2016 01:15:03 -0800 (PST) In-Reply-To: <1481295875-32499-2-git-send-email-ard.biesheuvel@linaro.org> References: <1481295875-32499-1-git-send-email-ard.biesheuvel@linaro.org> <1481295875-32499-2-git-send-email-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Wed, 14 Dec 2016 09:15:03 +0000 Message-ID: To: "edk2-devel@lists.01.org" , Leif Lindholm , Ruiyu Ni Cc: "Tian, Feng" , "Gao, Liming" , Ard Biesheuvel Subject: Re: [PATCH v6 1/2] MdeModulePkg/NonDiscoverablePciDeviceDxe: 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, 14 Dec 2016 09:15:05 -0000 Content-Type: text/plain; charset=UTF-8 Ray, Do you have any comments on this version? Thanks, Ard. On 9 December 2016 at 15:04, Ard Biesheuvel wrote: > Add support for non-coherent DMA, either by performing explicit cache > maintenance when DMA mappings are aligned to the CPU's DMA buffer alignment, > or by bounce buffering via uncached mappings otherwise. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > Tested-by: Marcin Wojtas > --- > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 14 +- > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf | 2 + > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 329 ++++++++++++++++++++ > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h | 29 ++ > 4 files changed, 367 insertions(+), 7 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > index ee765d7a5d9c..0fcf2b2ec1bf 100644 > --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c > @@ -16,6 +16,8 @@ > > #include > > +EFI_CPU_ARCH_PROTOCOL *mCpu; > + > // > // We only support the following device types > // > @@ -69,14 +71,7 @@ NonDiscoverablePciDeviceSupported ( > return Status; > } > > - // > - // Restricted to DMA coherent for now > - // > Status = EFI_UNSUPPORTED; > - if (Device->DmaType != NonDiscoverableDeviceDmaTypeCoherent) { > - goto CloseProtocol; > - } > - > for (Idx = 0; Idx < ARRAY_SIZE (SupportedNonDiscoverableDevices); Idx++) { > if (CompareGuid (Device->Type, SupportedNonDiscoverableDevices [Idx])) { > Status = EFI_SUCCESS; > @@ -224,6 +219,11 @@ NonDiscoverablePciDeviceDxeEntryPoint ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > + EFI_STATUS Status; > + > + Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu); > + ASSERT_EFI_ERROR(Status); > + > return EfiLibInstallDriverBindingComponentName2 ( > ImageHandle, > SystemTable, > diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf > index 996fe310e0e3..5faa8945134c 100644 > --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf > +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf > @@ -32,6 +32,7 @@ [Packages] > [LibraryClasses] > BaseMemoryLib > DebugLib > + DxeServicesTableLib > MemoryAllocationLib > UefiBootServicesTableLib > UefiDriverEntryPoint > @@ -40,6 +41,7 @@ [LibraryClasses] > [Protocols] > gEfiPciIoProtocolGuid ## BY_START > gEdkiiNonDiscoverableDeviceProtocolGuid ## TO_START > + gEfiCpuArchProtocolGuid ## CONSUMES > > [Guids] > gEdkiiNonDiscoverableAhciDeviceGuid > diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c > index 56482e3353c0..814f7643ae4c 100644 > --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c > +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c > @@ -15,6 +15,8 @@ > > #include "NonDiscoverablePciDeviceIo.h" > > +#include > + > #include > > #include > @@ -537,6 +539,324 @@ CoherentPciIoFreeBuffer ( > return EFI_SUCCESS; > } > > +STATIC > +EFI_STATUS > +EFIAPI > +NonCoherentPciIoFreeBuffer ( > + IN EFI_PCI_IO_PROTOCOL *This, > + IN UINTN Pages, > + IN VOID *HostAddress > + ) > +{ > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + LIST_ENTRY *Entry; > + EFI_STATUS Status; > + NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION *Alloc; > + BOOLEAN Found; > + > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + Found = FALSE; > + > + // > + // Find the uncached allocation list entry associated > + // with this allocation > + // > + for (Entry = Dev->UncachedAllocationList.ForwardLink; > + Entry != &Dev->UncachedAllocationList; > + Entry = Entry->ForwardLink) { > + > + Alloc = BASE_CR (Entry, NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION, List); > + if (Alloc->HostAddress == HostAddress && Alloc->NumPages == Pages) { > + // > + // We are freeing the exact allocation we were given > + // before by AllocateBuffer() > + // > + Found = TRUE; > + break; > + } > + } > + > + if (!Found) { > + ASSERT_EFI_ERROR (EFI_NOT_FOUND); > + return EFI_NOT_FOUND; > + } > + > + RemoveEntryList (&Alloc->List); > + > + Status = gDS->SetMemorySpaceAttributes ( > + (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, > + EFI_PAGES_TO_SIZE (Pages), > + Alloc->Attributes); > + if (EFI_ERROR (Status)) { > + goto FreeAlloc; > + } > + > + // > + // If we fail to restore the original attributes, it is better to leak the > + // memory than to return it to the heap > + // > + FreePages (HostAddress, Pages); > + > +FreeAlloc: > + FreePool (Alloc); > + return Status; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +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 > + ) > +{ > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; > + EFI_STATUS Status; > + UINT64 MemType; > + NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION *Alloc; > + VOID *AllocAddress; > + > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + Status = CoherentPciIoAllocateBuffer (This, Type, MemoryType, Pages, > + &AllocAddress, Attributes); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = gDS->GetMemorySpaceDescriptor ( > + (EFI_PHYSICAL_ADDRESS)(UINTN)AllocAddress, > + &GcdDescriptor); > + if (EFI_ERROR (Status)) { > + goto FreeBuffer; > + } > + > + if ((GcdDescriptor.Capabilities & (EFI_MEMORY_WC | EFI_MEMORY_UC)) == 0) { > + Status = EFI_UNSUPPORTED; > + goto FreeBuffer; > + } > + > + // > + // Set the preferred memory attributes > + // > + if ((Attributes & EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE) != 0 || > + (GcdDescriptor.Capabilities & EFI_MEMORY_UC) == 0) { > + // > + // Use write combining if it was requested, or if it is the only > + // type supported by the region. > + // > + MemType = EFI_MEMORY_WC; > + } else { > + MemType = EFI_MEMORY_UC; > + } > + > + Alloc = AllocatePool (sizeof *Alloc); > + if (Alloc == NULL) { > + goto FreeBuffer; > + } > + > + Alloc->HostAddress = AllocAddress; > + Alloc->NumPages = Pages; > + Alloc->Attributes = GcdDescriptor.Attributes; > + > + // > + // Record this allocation in the linked list, so we > + // can restore the memory space attributes later > + // > + InsertHeadList (&Dev->UncachedAllocationList, &Alloc->List); > + > + Status = gDS->SetMemorySpaceAttributes ( > + (EFI_PHYSICAL_ADDRESS)(UINTN)AllocAddress, > + EFI_PAGES_TO_SIZE (Pages), > + MemType); > + if (EFI_ERROR (Status)) { > + goto RemoveList; > + } > + > + Status = mCpu->FlushDataCache ( > + mCpu, > + (EFI_PHYSICAL_ADDRESS)(UINTN)AllocAddress, > + EFI_PAGES_TO_SIZE (Pages), > + EfiCpuFlushTypeInvalidate); > + if (EFI_ERROR (Status)) { > + goto RemoveList; > + } > + > + *HostAddress = AllocAddress; > + > + return EFI_SUCCESS; > + > +RemoveList: > + RemoveEntryList (&Alloc->List); > + FreePool (Alloc); > + > +FreeBuffer: > + CoherentPciIoFreeBuffer (This, Pages, AllocAddress); > + return Status; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +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 > + ) > +{ > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > + EFI_STATUS Status; > + NON_DISCOVERABLE_PCI_DEVICE_MAP_INFO *MapInfo; > + UINTN AlignMask; > + VOID *AllocAddress; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; > + BOOLEAN Bounce; > + > + MapInfo = AllocatePool (sizeof *MapInfo); > + if (MapInfo == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + MapInfo->HostAddress = HostAddress; > + MapInfo->Operation = Operation; > + MapInfo->NumberOfBytes = *NumberOfBytes; > + > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + // > + // If this device does not support 64-bit DMA addressing, we need to allocate > + // a bounce buffer and copy over the data in case HostAddress >= 4 GB. > + // > + Bounce = ((Dev->Attributes & EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0 && > + (UINTN)HostAddress + *NumberOfBytes > SIZE_4GB); > + > + if (!Bounce) { > + switch (Operation) { > + case EfiPciIoOperationBusMasterRead: > + case EfiPciIoOperationBusMasterWrite: > + // > + // For streaming DMA, it is sufficient if the buffer is aligned to > + // the CPUs DMA buffer alignment. > + // > + AlignMask = mCpu->DmaBufferAlignment - 1; > + if ((((UINTN) HostAddress | *NumberOfBytes) & AlignMask) == 0) { > + break; > + } > + // fall through > + > + case EfiPciIoOperationBusMasterCommonBuffer: > + // > + // Check whether the host address refers to an uncached mapping. > + // > + Status = gDS->GetMemorySpaceDescriptor ( > + (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, > + &GcdDescriptor); > + if (EFI_ERROR (Status) || > + (GcdDescriptor.Attributes & (EFI_MEMORY_WB|EFI_MEMORY_WT)) != 0) { > + Bounce = TRUE; > + } > + break; > + > + default: > + ASSERT (FALSE); > + } > + } > + > + if (Bounce) { > + if (Operation == EfiPciIoOperationBusMasterCommonBuffer) { > + Status = EFI_DEVICE_ERROR; > + goto FreeMapInfo; > + } > + > + Status = NonCoherentPciIoAllocateBuffer (This, AllocateAnyPages, > + EfiBootServicesData, EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes), > + &AllocAddress, EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE); > + 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. > + // > + mCpu->FlushDataCache (mCpu, (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, > + *NumberOfBytes, EfiCpuFlushTypeWriteBack); > + } > + > + *Mapping = MapInfo; > + return EFI_SUCCESS; > + > +FreeMapInfo: > + FreePool (MapInfo); > + > + return Status; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +NonCoherentPciIoUnmap ( > + IN EFI_PCI_IO_PROTOCOL *This, > + IN VOID *Mapping > + ) > +{ > + NON_DISCOVERABLE_PCI_DEVICE_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 *)(UINTN)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) { > + mCpu->FlushDataCache (mCpu, > + (EFI_PHYSICAL_ADDRESS)(UINTN)MapInfo->HostAddress, > + MapInfo->NumberOfBytes, EfiCpuFlushTypeInvalidate); > + } > + } > + FreePool (MapInfo); > + return EFI_SUCCESS; > +} > > STATIC > EFI_STATUS > @@ -726,12 +1046,21 @@ InitializePciIoProtocol ( > EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > INTN Idx; > > + InitializeListHead (&Dev->UncachedAllocationList); > + > Dev->ConfigSpace.Hdr.VendorId = PCI_ID_VENDOR_UNKNOWN; > Dev->ConfigSpace.Hdr.DeviceId = PCI_ID_DEVICE_DONTCARE; > > // Copy protocol structure > CopyMem(&Dev->PciIo, &PciIoTemplate, sizeof PciIoTemplate); > > + if (Dev->Device->DmaType == NonDiscoverableDeviceDmaTypeNonCoherent) { > + Dev->PciIo.AllocateBuffer = NonCoherentPciIoAllocateBuffer; > + Dev->PciIo.FreeBuffer = NonCoherentPciIoFreeBuffer; > + Dev->PciIo.Map = NonCoherentPciIoMap; > + Dev->PciIo.Unmap = NonCoherentPciIoUnmap; > + } > + > if (CompareGuid (Dev->Device->Type, &gEdkiiNonDiscoverableAhciDeviceGuid)) { > Dev->ConfigSpace.Hdr.ClassCode[0] = PCI_IF_MASS_STORAGE_AHCI; > Dev->ConfigSpace.Hdr.ClassCode[1] = PCI_CLASS_MASS_STORAGE_SATADPA; > diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h > index bc0a3d3258f9..449614862911 100644 > --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h > +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.h > @@ -15,6 +15,8 @@ > #ifndef __NON_DISCOVERABLE_PCI_DEVICE_IO_H__ > #define __NON_DISCOVERABLE_PCI_DEVICE_IO_H__ > > +#include > + > #include > #include > #include > @@ -25,6 +27,7 @@ > > #include > #include > +#include > #include > > #define NON_DISCOVERABLE_PCI_DEVICE_SIG SIGNATURE_32 ('P', 'P', 'I', 'D') > @@ -38,6 +41,27 @@ > > #define PCI_MAX_BARS 6 > > +extern EFI_CPU_ARCH_PROTOCOL *mCpu; > + > +typedef struct { > + // > + // The linked-list next pointer > + // > + LIST_ENTRY List; > + // > + // The address of the uncached allocation > + // > + VOID *HostAddress; > + // > + // The number of pages in the allocation > + // > + UINTN NumPages; > + // > + // The attributes of the allocation > + // > + UINT64 Attributes; > +} NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION; > + > typedef struct { > UINT32 Signature; > // > @@ -71,6 +95,11 @@ typedef struct { > // Whether this device has been enabled > // > BOOLEAN Enabled; > + // > + // Linked list to keep track of uncached allocations performed > + // on behalf of this device > + // > + LIST_ENTRY UncachedAllocationList; > } NON_DISCOVERABLE_PCI_DEVICE; > > VOID > -- > 2.7.4 >