Re: [PATCH edk2-platforms v4 4/9] Silicon/Broadcom/BcmGenetDxe: avoid uncached memory for streaming DMA

["Andrei Warkentin" <awarkentin@vmware.com>]

[-- Attachment #1: Type: text/plain, Size: 7112 bytes --]

Is 1500 the max MTU that UEFI uses? Wasn't clear from the spec.

Otherwise looks good - esp optimizing the allocations instead of wasting half page on every alloc 🙂.

LGTM

Reviewed-by: Andrei Warkentin <andrey.warkentin@gmail.com>

________________________________
From: Ard Biesheuvel <ard.biesheuvel@arm.com>
Sent: Monday, May 11, 2020 9:55 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Pete Batard <pete@akeo.ie>; Jared McNeill <jmcneill@invisible.ca>; Andrei Warkentin <awarkentin@vmware.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
Subject: [PATCH edk2-platforms v4 4/9] Silicon/Broadcom/BcmGenetDxe: avoid uncached memory for streaming DMA

The non-coherent version of DmaAllocateBuffer () returns uncached
memory, to ensure that the CPU and the device see the same data,
even we they are accessing the buffer at the same time.

This is not really necessary for our RX ring: the CPU never accesses
the buffer while it is mapped for writing by the device, and so we
can simply use the streaming DMA model, which uses ordinary cached
buffers, but issues a cache invalidate at DMA unmap time.

While at it, reduce the max packet size to 1536 bytes, and allocate
the entire buffer array using a single call.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf |  4 +++
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h     |  6 ++--
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c     | 36 +++++++++-----------
 Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c |  2 +-
 4 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf
index 1f1aeca7dd6b..248164249c6e 100644
--- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf
@@ -51,3 +51,7 @@ [Protocols]
   gBcmGenetPlatformDeviceProtocolGuid         ## TO_START
   gEfiDevicePathProtocolGuid                  ## BY_START
   gEfiSimpleNetworkProtocolGuid               ## BY_START
+
+[FixedPcd]
+  gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
+  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit
diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h
index b491ea4665b0..ddfbc0806c07 100644
--- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h
@@ -50,7 +50,7 @@
 #define BRGPHY_SHADOW_1C_GTXCLK_EN       0x0200

 #define GENET_VERSION                           0x0a
-#define GENET_MAX_PACKET_SIZE                   2048
+#define GENET_MAX_PACKET_SIZE                   1536

 #define GENET_SYS_REV_CTRL                      0x000
 #define  SYS_REV_MAJOR                          (BIT27|BIT26|BIT25|BIT24)
@@ -217,7 +217,7 @@ typedef struct {
   UINT16                              TxConsIndex;
   UINT16                              TxProdIndex;

-  UINT8                               *RxBuffer[GENET_DMA_DESC_COUNT];
+  EFI_PHYSICAL_ADDRESS                RxBuffer;
   GENET_MAP_INFO                      RxBufferMap[GENET_DMA_DESC_COUNT];
   UINT16                              RxConsIndex;
   UINT16                              RxProdIndex;
@@ -235,6 +235,8 @@ extern CONST EFI_SIMPLE_NETWORK_PROTOCOL      gGenetSimpleNetworkTemplate;
 #define GENET_DRIVER_SIGNATURE                SIGNATURE_32('G', 'N', 'E', 'T')
 #define GENET_PRIVATE_DATA_FROM_SNP_THIS(a)   CR(a, GENET_PRIVATE_DATA, Snp, GENET_DRIVER_SIGNATURE)

+#define GENET_RX_BUFFER(g, idx)               ((UINT8 *)(UINTN)(g)->RxBuffer + GENET_MAX_PACKET_SIZE * (idx))
+
 EFI_STATUS
 EFIAPI
 GenetPhyRead (
diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c
index 71c659e7f882..94b578a10aa1 100644
--- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c
@@ -19,6 +19,10 @@

 #define GENET_PHY_RETRY     1000

+STATIC CONST
+EFI_PHYSICAL_ADDRESS   mDmaAddressLimit = FixedPcdGet64 (PcdDmaDeviceLimit) -
+                                          FixedPcdGet64 (PcdDmaDeviceOffset);
+
 /**
   Read a memory-mapped device CSR.

@@ -605,19 +609,17 @@ GenetDmaAlloc (
   IN GENET_PRIVATE_DATA *Genet
   )
 {
-  EFI_STATUS  Status;
-  UINTN       Idx;
+  EFI_STATUS              Status;

-  for (Idx = 0; Idx < GENET_DMA_DESC_COUNT; Idx++) {
-    Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (GENET_MAX_PACKET_SIZE), (VOID **)&Genet->RxBuffer[Idx]);
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "%a: Failed to allocate RX buffer: %r\n", __FUNCTION__, Status));
-      GenetDmaFree (Genet);
-      return Status;
-    }
+  Genet->RxBuffer = mDmaAddressLimit;
+  Status = gBS->AllocatePages (AllocateMaxAddress, EfiBootServicesData,
+                  EFI_SIZE_TO_PAGES (GENET_MAX_PACKET_SIZE * GENET_DMA_DESC_COUNT),
+                  &Genet->RxBuffer);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: Failed to allocate RX buffer: %r\n", __FUNCTION__, Status));
   }
-
-  return EFI_SUCCESS;
+  return Status;
 }

 /**
@@ -640,11 +642,11 @@ GenetDmaMapRxDescriptor (
   UINTN         DmaNumberOfBytes;

   ASSERT (Genet->RxBufferMap[DescIndex].Mapping == NULL);
-  ASSERT (Genet->RxBuffer[DescIndex] != NULL);
+  ASSERT (Genet->RxBuffer != 0);

   DmaNumberOfBytes = GENET_MAX_PACKET_SIZE;
   Status = DmaMap (MapOperationBusMasterWrite,
-             (VOID *)Genet->RxBuffer[DescIndex],
+             GENET_RX_BUFFER (Genet, DescIndex),
              &DmaNumberOfBytes,
              &Genet->RxBufferMap[DescIndex].PhysAddress,
              &Genet->RxBufferMap[DescIndex].Mapping);
@@ -697,13 +699,9 @@ GenetDmaFree (

   for (Idx = 0; Idx < GENET_DMA_DESC_COUNT; Idx++) {
     GenetDmaUnmapRxDescriptor (Genet, Idx);
-
-    if (Genet->RxBuffer[Idx] != NULL) {
-      DmaFreeBuffer (EFI_SIZE_TO_PAGES (GENET_MAX_PACKET_SIZE),
-        Genet->RxBuffer[Idx]);
-      Genet->RxBuffer[Idx] = NULL;
-    }
   }
+  gBS->FreePages (Genet->RxBuffer,
+         EFI_SIZE_TO_PAGES (GENET_MAX_PACKET_SIZE * GENET_DMA_DESC_COUNT));
 }

 /**
diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c
index 72ab55619b0e..9746f210d1f2 100644
--- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c
@@ -730,7 +730,7 @@ GenetSimpleNetworkReceive (

   GenetDmaUnmapRxDescriptor (Genet, DescIndex);

-  Frame = Genet->RxBuffer[DescIndex];
+  Frame = GENET_RX_BUFFER (Genet, DescIndex);

   if (FrameLength > 2 + Genet->SnpMode.MediaHeaderSize) {
     // Received frame has 2 bytes of padding at the start
--
2.17.1


[-- Attachment #2: Type: text/html, Size: 13137 bytes --]