| Message ID | 1477937590-10361-6-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 <ard.biesheuvel@linaro.org> > --- > 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 <Library/DxeServicesTableLib.h> > + > #include <Protocol/PciRootBridgeIo.h> > > 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 <PiDxe.h> > + > #include <Library/BaseMemoryLib.h> > #include <Library/DebugLib.h> > #include <Library/MemoryAllocationLib.h> > @@ -20,6 +22,7 @@ > > #include <IndustryStandard/Pci.h> > > +#include <Protocol/Cpu.h> > #include <Protocol/PciIo.h> > #include <Protocol/PlatformPciIo.h> > > @@ -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 <Protocol/ComponentName.h> > #include <Protocol/DriverBinding.h> > > +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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 1 November 2016 at 22:43, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> >> --- >> 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 <Library/DxeServicesTableLib.h> >> + >> #include <Protocol/PciRootBridgeIo.h> >> >> 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 <PiDxe.h> >> + >> #include <Library/BaseMemoryLib.h> >> #include <Library/DebugLib.h> >> #include <Library/MemoryAllocationLib.h> >> @@ -20,6 +22,7 @@ >> >> #include <IndustryStandard/Pci.h> >> >> +#include <Protocol/Cpu.h> >> #include <Protocol/PciIo.h> >> #include <Protocol/PlatformPciIo.h> >> >> @@ -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 <Protocol/ComponentName.h> >> #include <Protocol/DriverBinding.h> >> >> +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 >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Nov 02, 2016 at 01:43:42PM +0000, Ard Biesheuvel wrote: > On 1 November 2016 at 22:43, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> > >> --- > >> 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 <Library/DxeServicesTableLib.h> > >> + > >> #include <Protocol/PciRootBridgeIo.h> > >> > >> 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 <PiDxe.h> > >> + > >> #include <Library/BaseMemoryLib.h> > >> #include <Library/DebugLib.h> > >> #include <Library/MemoryAllocationLib.h> > >> @@ -20,6 +22,7 @@ > >> > >> #include <IndustryStandard/Pci.h> > >> > >> +#include <Protocol/Cpu.h> > >> #include <Protocol/PciIo.h> > >> #include <Protocol/PlatformPciIo.h> > >> > >> @@ -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 <Protocol/ComponentName.h> > >> #include <Protocol/DriverBinding.h> > >> > >> +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 > >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 <Library/DxeServicesTableLib.h> + #include <Protocol/PciRootBridgeIo.h> 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 <PiDxe.h> + #include <Library/BaseMemoryLib.h> #include <Library/DebugLib.h> #include <Library/MemoryAllocationLib.h> @@ -20,6 +22,7 @@ #include <IndustryStandard/Pci.h> +#include <Protocol/Cpu.h> #include <Protocol/PciIo.h> #include <Protocol/PlatformPciIo.h> @@ -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 <Protocol/ComponentName.h> #include <Protocol/DriverBinding.h> +EFI_CPU_ARCH_PROTOCOL *gCpu; + // // 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
Add support for bounce buffering using uncached mappings when DMA mappings are not aligned to the CPU's DMA buffer alignment. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- 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(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel