[edk2,5/5] EmbeddedPkg/PlatformPciIoDxe: add support for non-coherent DMA

Message ID 1477937590-10361-6-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Oct. 31, 2016, 6:13 p.m.
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

Comments

Leif Lindholm Nov. 1, 2016, 10:43 p.m. | #1
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
Ard Biesheuvel Nov. 2, 2016, 1:43 p.m. | #2
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
Leif Lindholm Nov. 2, 2016, 4:17 p.m. | #3
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

Patch

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