From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
To: Andrei Warkentin <awarkentin@vmware.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: Pete Batard <pete@akeo.ie>, Jared McNeill <jmcneill@invisible.ca>,
Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>,
Jeremy Linton <jeremy.linton@arm.com>
Subject: Re: [PATCH edk2-platforms v4 4/9] Silicon/Broadcom/BcmGenetDxe: avoid uncached memory for streaming DMA
Date: Mon, 11 May 2020 18:53:55 +0200 [thread overview]
Message-ID: <a2eff688-be85-e13d-7d31-482ffbe306ab@arm.com> (raw)
In-Reply-To: <BN6PR05MB34119A2B8E6206544521C00EB9A10@BN6PR05MB3411.namprd05.prod.outlook.com>
On 5/11/20 6:42 PM, Andrei Warkentin wrote:
> 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>
>
Thanks.
I couldn't disentangle that mess either, but I *think* we should
probably be using 1500 for SnpMode.MaxPacketSize, since MNP adds the
VLAN tag lan and Ethernet FCS size to that. Currently, we have
Genet->SnpMode.MaxPacketSize = GENET_MAX_PACKET_SIZE;
which is definitely wrong.
(1536 is a multiple of 512 so it is guaranteed to be cacheline aligned,
which helps the non-coherent DMA lib map the buffers without having to
do cache maintenance)
> ------------------------------------------------------------------------
> *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
>
next prev parent reply other threads:[~2020-05-11 16:54 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-11 14:55 [PATCH edk2-platforms v4 0/9] BCM genet fixes Ard Biesheuvel
2020-05-11 14:55 ` [PATCH edk2-platforms v4 1/9] Silicon/Broadcom/BcmGenetDxe: whitespace/cosmetic cleanup Ard Biesheuvel
2020-05-11 16:36 ` Samer El-Haj-Mahmoud
2020-05-11 21:14 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-05-11 14:55 ` [PATCH edk2-platforms v4 2/9] Silicon/Broadcom/BcmGenetDxe: add support for broadcast filtering Ard Biesheuvel
2020-05-11 16:44 ` Andrei Warkentin
2020-05-11 20:34 ` Jeremy Linton
2020-05-11 21:21 ` Ard Biesheuvel
2020-05-11 14:55 ` [PATCH edk2-platforms v4 3/9] Silicon/Broadcom/BcmGenetDxe: fix multicast/broadcast handling Ard Biesheuvel
2020-05-11 14:55 ` [PATCH edk2-platforms v4 4/9] Silicon/Broadcom/BcmGenetDxe: avoid uncached memory for streaming DMA Ard Biesheuvel
2020-05-11 16:42 ` Andrei Warkentin
2020-05-11 16:53 ` Ard Biesheuvel [this message]
2020-05-11 14:55 ` [PATCH edk2-platforms v4 5/9] Silicon/Broadcom/BcmGenetDxe: shut down devices on ExitBootServices() Ard Biesheuvel
2020-05-11 16:32 ` Andrei Warkentin
2020-05-11 14:55 ` [PATCH edk2-platforms v4 6/9] Silicon/Broadcom/BcmGenetDxe: keep TX buffer mapped during DMA transfer Ard Biesheuvel
2020-05-11 16:19 ` Andrei Warkentin
2020-05-11 14:55 ` [PATCH edk2-platforms v4 7/9] Silicon/Broadcom/BcmGenetDxe: use MemoryFence() for MMIO write ordering Ard Biesheuvel
2020-05-11 16:55 ` [edk2-devel] " Andrei Warkentin
2020-05-11 21:11 ` Philippe Mathieu-Daudé
2020-05-11 14:55 ` [PATCH edk2-platforms v4 8/9] Silicon/Broadcom/BcmGenetDxe: add unload support Ard Biesheuvel
2020-05-11 16:17 ` Andrei Warkentin
2020-05-11 14:55 ` [PATCH edk2-platforms v4 9/9] Platform/RaspberryPi4: remove ASIX 88772b driver Ard Biesheuvel
2020-05-11 16:06 ` Andrei Warkentin
2020-05-11 16:24 ` Samer El-Haj-Mahmoud
2020-05-11 23:20 ` [PATCH edk2-platforms v4 0/9] BCM genet fixes Jeremy Linton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a2eff688-be85-e13d-7d31-482ffbe306ab@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox