From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 325AF81D3E for ; Sun, 27 Nov 2016 18:25:21 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP; 27 Nov 2016 18:25:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,561,1473145200"; d="scan'208";a="36220990" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 27 Nov 2016 18:25:20 -0800 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 27 Nov 2016 18:25:20 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 27 Nov 2016 18:25:20 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.142]) by shsmsx102.ccr.corp.intel.com ([169.254.2.239]) with mapi id 14.03.0248.002; Mon, 28 Nov 2016 10:25:16 +0800 From: "Ni, Ruiyu" To: Ard Biesheuvel , "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" , "Gao, Liming" , "afish@apple.com" , "Kinney, Michael D" , "mw@semihalf.com" , "Tian, Feng" Thread-Topic: [PATCH v4 4/5] MdeModulePkg/NonDiscoverablePciDeviceDxe: add support for non-coherent DMA Thread-Index: AQHSRzKaTGMKLJgyG0agJgo6deY5qaDtqW2A Date: Mon, 28 Nov 2016 02:25:15 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D58E8D058@SHSMSX104.ccr.corp.intel.com> References: <1480088538-21834-1-git-send-email-ard.biesheuvel@linaro.org> <1480088538-21834-5-git-send-email-ard.biesheuvel@linaro.org> In-Reply-To: <1480088538-21834-5-git-send-email-ard.biesheuvel@linaro.org> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v4 4/5] 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: Mon, 28 Nov 2016 02:25:21 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Comments in NonCoherentPciIoAllocateBuffer(). Thanks/Ray > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Friday, November 25, 2016 11:42 PM > To: edk2-devel@lists.01.org; leif.lindholm@linaro.org; Gao, Liming > ; afish@apple.com; Ni, Ruiyu ; > Kinney, Michael D ; mw@semihalf.com; Tian, > Feng > Cc: Ard Biesheuvel > Subject: [PATCH v4 4/5] MdeModulePkg/NonDiscoverablePciDeviceDxe: add > support for non-coherent DMA >=20 > 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. >=20 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- >=20 > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci > DeviceDxe.c | 14 +- >=20 > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci > DeviceDxe.inf | 2 + >=20 > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci > DeviceIo.c | 319 ++++++++++++++++++++ >=20 > MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci > DeviceIo.h | 29 ++ > 4 files changed, 357 insertions(+), 7 deletions(-) >=20 > diff --git > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceDxe.c > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceDxe.c > index ee765d7a5d9c..0fcf2b2ec1bf 100644 > --- > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceDxe.c > +++ > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > Pc > +++ iDeviceDxe.c > @@ -16,6 +16,8 @@ >=20 > #include >=20 > +EFI_CPU_ARCH_PROTOCOL *mCpu; > + > // > // We only support the following device types // @@ -69,14 +71,7 @@ > NonDiscoverablePciDeviceSupported ( > return Status; > } >=20 > - // > - // Restricted to DMA coherent for now > - // > Status =3D EFI_UNSUPPORTED; > - if (Device->DmaType !=3D NonDiscoverableDeviceDmaTypeCoherent) { > - goto CloseProtocol; > - } > - > for (Idx =3D 0; Idx < ARRAY_SIZE (SupportedNonDiscoverableDevices); Id= x++) > { > if (CompareGuid (Device->Type, SupportedNonDiscoverableDevices [Idx]= )) > { > Status =3D EFI_SUCCESS; > @@ -224,6 +219,11 @@ NonDiscoverablePciDeviceDxeEntryPoint ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > + EFI_STATUS Status; > + > + Status =3D gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID > + **)&mCpu); ASSERT_EFI_ERROR(Status); > + > return EfiLibInstallDriverBindingComponentName2 ( > ImageHandle, > SystemTable, > diff --git > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceDxe.inf > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceDxe.inf > index 996fe310e0e3..5faa8945134c 100644 > --- > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceDxe.inf > +++ > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > Pc > +++ iDeviceDxe.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 >=20 > [Guids] > gEdkiiNonDiscoverableAhciDeviceGuid > diff --git > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceIo.c > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceIo.c > index fd59267a8d66..5a4861a17717 100644 > --- > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceIo.c > +++ > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > Pc > +++ iDeviceIo.c > @@ -15,6 +15,8 @@ >=20 > #include "NonDiscoverablePciDeviceIo.h" >=20 > +#include > + > #include >=20 > #include > @@ -523,6 +525,316 @@ CoherentPciIoFreeBuffer ( > return EFI_SUCCESS; > } >=20 > +STATIC > +EFI_STATUS > +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 =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + Found =3D FALSE; > + > + // > + // Find the uncached allocation list entry associated // with this > + allocation // for (Entry =3D Dev->UncachedAllocationList.ForwardLink; > + Entry !=3D &Dev->UncachedAllocationList; > + Entry =3D Entry->ForwardLink) { > + > + Alloc =3D BASE_CR (Entry, > NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION, List); > + if (Alloc->HostAddress =3D=3D HostAddress && Alloc->NumPages =3D=3D = Pages) { > + // > + // We are freeing the exact allocation we were given > + // before by AllocateBuffer() > + // > + Found =3D TRUE; > + break; > + } > + } > + > + if (!Found) { > + ASSERT_EFI_ERROR (EFI_NOT_FOUND); > + return EFI_NOT_FOUND; > + } > + > + RemoveEntryList (&Alloc->List); > + > + Status =3D 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 > +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; > + > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > + > + Status =3D CoherentPciIoAllocateBuffer (This, Type, MemoryType, Pages, > + HostAddress, Attributes); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status =3D gDS->GetMemorySpaceDescriptor ( > + (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, > + &GcdDescriptor); > + if (EFI_ERROR (Status)) { > + goto FreeBuffer; 1. in FreeBuffer, NonCoherentPciIoFreeBuffer() is called. But since the allocation entry hasn't been allocated and inserted to the list, the NonCoherentPciIoFreeBuffer() will assert and return EFI_NOT_FOUND, resulting the memory leak. > + } > + > + if ((GcdDescriptor.Capabilities & (EFI_MEMORY_WC | EFI_MEMORY_UC)) > =3D=3D 0) { > + return EFI_UNSUPPORTED; > + } > + > + // > + // Set the preferred memory attributes // if ((Attributes & > + EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE) !=3D 0 || > + (GcdDescriptor.Capabilities & EFI_MEMORY_UC) =3D=3D 0) { > + // > + // Use write combining if it was requested, or if it is the only > + // type supported by the region. > + // > + MemType =3D EFI_MEMORY_WC; > + } else { > + MemType =3D EFI_MEMORY_UC; > + } > + > + Alloc =3D AllocatePool (sizeof *Alloc); if (Alloc =3D=3D NULL) { > + goto FreeBuffer; 2. Same as #1. Did you mean to call CoherentPciIoFreeBuffer()? > + } > + > + Alloc->HostAddress =3D *HostAddress; > + Alloc->NumPages =3D Pages; > + Alloc->Attributes =3D GcdDescriptor.Attributes; > + > + // > + // Record this allocation in the linked list, so we // can restore > + the memory space attributes later // InsertHeadList > + (&Dev->UncachedAllocationList, &Alloc->List); > + > + Status =3D gDS->SetMemorySpaceAttributes ( > + (EFI_PHYSICAL_ADDRESS)(UINTN)*HostAddress, > + EFI_PAGES_TO_SIZE (Pages), > + MemType); > + if (EFI_ERROR (Status)) { > + goto RemoveList; 3. same as #1. > + } > + > + Status =3D mCpu->FlushDataCache ( > + mCpu, > + (EFI_PHYSICAL_ADDRESS)(UINTN)*HostAddress, > + EFI_PAGES_TO_SIZE (Pages), > + EfiCpuFlushTypeInvalidate); if (EFI_ERROR (Status)) > + { > + goto RemoveList; 4. same as #1. > + } > + > + return EFI_SUCCESS; > + > +RemoveList: > + RemoveEntryList (&Alloc->List); > + FreePool (Alloc); > + > +FreeBuffer: > + 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 > + ) > +{ > + 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 =3D AllocatePool (sizeof *MapInfo); if (MapInfo =3D=3D NULL) = { > + return EFI_OUT_OF_RESOURCES; > + } > + > + MapInfo->HostAddress =3D HostAddress; > + MapInfo->Operation =3D Operation; > + MapInfo->NumberOfBytes =3D *NumberOfBytes; > + > + Dev =3D 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= >=3D > 4 GB. > + // > + Bounce =3D ((Dev->Attributes & > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) =3D=3D 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 =3D mCpu->DmaBufferAlignment - 1; > + if ((((UINTN) HostAddress | *NumberOfBytes) & AlignMask) =3D=3D 0)= { > + break; > + } > + // fall through > + > + case EfiPciIoOperationBusMasterCommonBuffer: > + // > + // Check whether the host address refers to an uncached mapping. > + // > + Status =3D gDS->GetMemorySpaceDescriptor ( > + (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, > + &GcdDescriptor); > + if (EFI_ERROR (Status) || > + (GcdDescriptor.Attributes & > (EFI_MEMORY_WB|EFI_MEMORY_WT)) !=3D 0) { > + Bounce =3D TRUE; > + } > + break; > + > + default: > + ASSERT (FALSE); > + } > + } > + > + if (Bounce) { > + if (Operation =3D=3D EfiPciIoOperationBusMasterCommonBuffer) { > + Status =3D EFI_DEVICE_ERROR; > + goto FreeMapInfo; > + } > + > + Status =3D 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 =3D (EFI_PHYSICAL_ADDRESS)(UINTN)AllocAddress; > + if (Operation =3D=3D EfiPciIoOperationBusMasterRead) { > + gBS->CopyMem (AllocAddress, HostAddress, *NumberOfBytes); > + } > + *DeviceAddress =3D MapInfo->AllocAddress; } else { > + MapInfo->AllocAddress =3D 0; > + *DeviceAddress =3D (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 cleanin= g > + // 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 =3D MapInfo; > + return EFI_SUCCESS; > + > +FreeMapInfo: > + FreePool (MapInfo); > + > + return Status; > +} > + > +STATIC > +EFI_STATUS > +NonCoherentPciIoUnmap ( > + IN EFI_PCI_IO_PROTOCOL *This, > + IN VOID *Mapping > + ) > +{ > + NON_DISCOVERABLE_PCI_DEVICE_MAP_INFO *MapInfo; > + > + if (Mapping =3D=3D NULL) { > + return EFI_DEVICE_ERROR; > + } > + > + MapInfo =3D Mapping; > + if (MapInfo->AllocAddress !=3D 0) { > + // > + // We are using a bounce buffer: copy back the data if necessary, > + // and free the buffer. > + // > + if (MapInfo->Operation =3D=3D 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 =3D=3D EfiPciIoOperationBusMasterWrite) { > + mCpu->FlushDataCache (mCpu, > + (EFI_PHYSICAL_ADDRESS)(UINTN)MapInfo->HostAddress, > + MapInfo->NumberOfBytes, EfiCpuFlushTypeInvalidate); > + } > + } > + FreePool (MapInfo); > + return EFI_SUCCESS; > +} >=20 > STATIC > EFI_STATUS > @@ -715,6 +1027,13 @@ InitializePciIoProtocol ( > // Copy protocol structure > CopyMem(&Dev->PciIo, &PciIoTemplate, sizeof PciIoTemplate); >=20 > + if (Dev->Device->DmaType =3D=3D > NonDiscoverableDeviceDmaTypeNonCoherent) { > + Dev->PciIo.AllocateBuffer =3D NonCoherentPciIoAllocateBuffer; > + Dev->PciIo.FreeBuffer =3D NonCoherentPciIoFreeBuffer; > + Dev->PciIo.Map =3D NonCoherentPciIoMap; > + Dev->PciIo.Unmap =3D NonCoherentPciIoUnmap; > + } > + > if (CompareGuid (Dev->Device->Type, > &gEdkiiNonDiscoverableAhciDeviceGuid)) { > Dev->ConfigSpace.Hdr.ClassCode[0] =3D PCI_IF_MASS_STORAGE_AHCI; > Dev->ConfigSpace.Hdr.ClassCode[1] =3D > PCI_CLASS_MASS_STORAGE_SATADPA; diff --git > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceIo.h > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceIo.h > index bc0a3d3258f9..449614862911 100644 > --- > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > PciDeviceIo.h > +++ > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable > Pc > +++ iDeviceIo.h > @@ -15,6 +15,8 @@ > #ifndef __NON_DISCOVERABLE_PCI_DEVICE_IO_H__ > #define __NON_DISCOVERABLE_PCI_DEVICE_IO_H__ >=20 > +#include > + > #include > #include > #include @@ -25,6 +27,7 @@ >=20 > #include > #include > +#include > #include >=20 > #define NON_DISCOVERABLE_PCI_DEVICE_SIG SIGNATURE_32 ('P', 'P', 'I', > 'D') @@ -38,6 +41,27 @@ >=20 > #define PCI_MAX_BARS 6 >=20 > +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; >=20 > VOID > -- > 2.7.4