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 ________________________________ From: Ard Biesheuvel Sent: Monday, May 11, 2020 9:55 AM To: devel@edk2.groups.io Cc: Ard Biesheuvel ; Pete Batard ; Jared McNeill ; Andrei Warkentin ; Samer El-Haj-Mahmoud ; Jeremy Linton 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 --- 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