From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web10.17307.1594639522355965932 for ; Mon, 13 Jul 2020 04:25:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=x7ROp7k9; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.65, mailfrom: pete@akeo.ie) Received: by mail-wr1-f65.google.com with SMTP id j4so15985018wrp.10 for ; Mon, 13 Jul 2020 04:25:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=p6WlLc8e0hlnqbE05MVEUML5YAW88n00qWc4tjRPYtQ=; b=x7ROp7k9r3n/4SqDrqKGawcmQODFrQWXFUIz4jY7IU9urbkky1yK59WX6sPwx0WCBu o0wUgebjx0d/9ptrAwU+aaiqxYqe94yIEdBYxj4cJSnHFzcfyLnw22a5HDWV7PXsKFH+ jZEO3g55SUeFbn+iJIuHyRzj4E7//RT0wvuuJrhhytwOw/wewNzXKkFZN2iPr6gQyczM GWFlIIJkdDP1+QUdsOQDInOXzYzFInvKi1HNVXF97wgLNJbAqW/23DdtjADPLhCTpj0s PK+nY3KrjLR4kXeidaRLE7pU1DU4PzMpTUK5CjfW/5koAkTYeGKxpuusVLjp2P/hu+6y 220A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=p6WlLc8e0hlnqbE05MVEUML5YAW88n00qWc4tjRPYtQ=; b=CSQ8RxO2qyDLttFEwhjIlpqo9U6EFAv+/YkPxivd0dJI+LUYS+8cZYoRscmSNK8UnW t/r5P5cXW/wP3oTvielNmHM8pwjxVI4NK95+d3CP85ML+3SRNBCnRbKQs2iF2MKsLlui 2Qsp2ts7LfYPY//llhs35M2hMXa1MH1sRscUPu5r9BvgGp/M1yVuo9hnR1qI2+JLBp+v yRQud+EgAaLYFUEf8SrTukLVAApcq+prGkFmAeeuin5URYGgidAyFO5UHKMqMoQpOsir 4wC2u8cNCDaTXHO8Srcw3zXx6rzN1zUs+pyO62yuBOphYOtVEDkexuCd5dV0EHzqA0DC +m1A== X-Gm-Message-State: AOAM5307XwSOm8uNotSwaXCJv58wxPcoGfqgpG2CKfDAlb8y4+2ujaXu AxwhD90RGZbRxi1qMJa/D7MXiA== X-Google-Smtp-Source: ABdhPJyxT0MIk5XVSONG6VnxnVF2kf3JJ2takRQ1lQCRFgAdeMRZ0qeqw3LVhgbd2w982ebnUgq3/g== X-Received: by 2002:adf:f452:: with SMTP id f18mr78562854wrp.78.1594639520795; Mon, 13 Jul 2020 04:25:20 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.73.23]) by smtp.googlemail.com with ESMTPSA id j6sm22859777wma.25.2020.07.13.04.25.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 13 Jul 2020 04:25:20 -0700 (PDT) Subject: Re: [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied To: Andrei Warkentin , devel@edk2.groups.io Cc: ard.biesheuvel@arm.com, leif@nuviainc.com, philmd@redhat.com References: <20200712042850.117715-1-andrey.warkentin@gmail.com> From: "Pete Batard" Message-ID: Date: Mon, 13 Jul 2020 12:25:18 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200712042850.117715-1-andrey.warkentin@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit One very minor formatting issue inline (that can be sorted during integration): On 2020.07.12 05:28, Andrei Warkentin wrote: > This was originally a bit sloppy, and could hypothetically under heavy > load result in a buffer being overwritten by hardware before the received > buffer is copied. > > Signed-off-by: Andrei Warkentin > --- > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h | 15 +++++ > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c | 59 +++++++++++++++----- > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c | 26 ++++++--- > 3 files changed, 77 insertions(+), 23 deletions(-) > > diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h > index 1a117b25..b39a1326 100644 > --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h > +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h > @@ -358,6 +358,16 @@ GenetTxIntr ( > OUT VOID **TxBuf > ); > > +UINT32 > +GenetRxPending ( > + IN GENET_PRIVATE_DATA *Genet > + ); > + > +UINT32 > +GenetTxPending ( > + IN GENET_PRIVATE_DATA *Genet > + ); > + > EFI_STATUS > GenetRxIntr ( > IN GENET_PRIVATE_DATA *Genet, > @@ -365,4 +375,9 @@ GenetRxIntr ( > OUT UINTN *FrameLength > ); > > +VOID > +GenetRxComplete ( > + IN GENET_PRIVATE_DATA *Genet > + ); > + > #endif /* GENET_UTIL_H__ */ > diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c > index 1c4c8527..a0097b0d 100644 > --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c > +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c > @@ -661,6 +661,7 @@ GenetDmaMapRxDescriptor ( > Genet->RxBufferMap[DescIndex].PhysAddress & 0xFFFFFFFF); > GenetMmioWrite (Genet, GENET_RX_DESC_ADDRESS_HI (DescIndex), > (Genet->RxBufferMap[DescIndex].PhysAddress >> 32) & 0xFFFFFFFF); > + GenetMmioWrite (Genet, GENET_RX_DESC_STATUS (DescIndex), 0); > > return EFI_SUCCESS; > } > @@ -753,12 +754,9 @@ GenetTxIntr ( > OUT VOID **TxBuf > ) > { > - UINT32 ConsIndex, Total; > + UINT32 Total; > > - ConsIndex = GenetMmioRead (Genet, > - GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > - > - Total = (ConsIndex - Genet->TxConsIndex) & 0xFFFF; > + Total = GenetTxPending (Genet); > if (Genet->TxQueued > 0 && Total > 0) { > DmaUnmap (Genet->TxBufferMap[Genet->TxNext]); > *TxBuf = Genet->TxBuffer[Genet->TxNext]; > @@ -770,6 +768,46 @@ GenetTxIntr ( > } > } > > +UINT32 > +GenetRxPending ( > + IN GENET_PRIVATE_DATA *Genet > + ) > +{ > + UINT32 ProdIndex; > + UINT32 ConsIndex; > + > + ConsIndex = GenetMmioRead (Genet, > + GENET_RX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > + ASSERT (ConsIndex == Genet->RxConsIndex); > + > + ProdIndex = GenetMmioRead (Genet, > + GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > + return (ProdIndex - Genet->RxConsIndex) & 0xFFFF; > +} > + > +UINT32 > +GenetTxPending ( > + IN GENET_PRIVATE_DATA *Genet > + ) > +{ > + UINT32 ConsIndex; > + > + ConsIndex = GenetMmioRead (Genet, > + GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; If we want to be pedantic about EDK2 formatting, this should be indented to start after the "Ge" of GenetMmioRead above, like the previous calls. > + > + return (ConsIndex - Genet->TxConsIndex) & 0xFFFF; > +} > + > +VOID > +GenetRxComplete ( > + IN GENET_PRIVATE_DATA *Genet > + ) > +{ > + Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0xFFFF; > + GenetMmioWrite (Genet, GENET_RX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE), > + Genet->RxConsIndex); > +} > + > /** > Simulate an "RX interrupt", returning the index of a completed RX buffer and > corresponding frame length. > @@ -790,21 +828,14 @@ GenetRxIntr ( > ) > { > EFI_STATUS Status; > - UINT32 ProdIndex, Total; > + UINT32 Total; > UINT32 DescStatus; > > - ProdIndex = GenetMmioRead (Genet, > - GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > - > - Total = (ProdIndex - Genet->RxConsIndex) & 0xFFFF; > + Total = GenetRxPending (Genet); > if (Total > 0) { > *DescIndex = Genet->RxConsIndex % GENET_DMA_DESC_COUNT; > DescStatus = GenetMmioRead (Genet, GENET_RX_DESC_STATUS (*DescIndex)); > *FrameLength = SHIFTOUT (DescStatus, GENET_RX_DESC_STATUS_BUFLEN); > - > - Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0xFFFF; > - GenetMmioWrite (Genet, GENET_RX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE), > - Genet->RxConsIndex); > Status = EFI_SUCCESS; > } else { > Status = EFI_NOT_READY; > diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > index 371216ca..1bda18f1 100644 > --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > @@ -502,9 +502,19 @@ GenetSimpleNetworkGetStatus ( > Genet->SnpMode.MediaPresent = FALSE; > } else { > Genet->SnpMode.MediaPresent = TRUE; > + } > + > + if (TxBuf != NULL) { > + GenetTxIntr (Genet, TxBuf); > + } > > - if (TxBuf != NULL) { > - GenetTxIntr (Genet, TxBuf); > + if (InterruptStatus != NULL) { > + *InterruptStatus = 0; > + if (GenetRxPending (Genet) > 0) { > + *InterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; > + } > + if (GenetTxPending (Genet) > 0) { > + *InterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; > } > } > > @@ -741,13 +751,8 @@ GenetSimpleNetworkReceive ( > DEBUG ((DEBUG_ERROR, > "%a: Buffer size (0x%X) is too small for frame (0x%X)\n", > __FUNCTION__, *BufferSize, FrameLength)); > - Status = GenetDmaMapRxDescriptor (Genet, DescIndex); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "%a: Failed to remap RX descriptor!\n", > - __FUNCTION__)); > - } > - EfiReleaseLock (&Genet->Lock); > - return EFI_BUFFER_TOO_SMALL; > + Status = EFI_BUFFER_TOO_SMALL; > + goto out; > } > > if (DestAddr != NULL) { > @@ -773,11 +778,14 @@ GenetSimpleNetworkReceive ( > Status = EFI_NOT_READY; > } > > +out: > Status = GenetDmaMapRxDescriptor (Genet, DescIndex); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "%a: Failed to remap RX descriptor!\n", __FUNCTION__)); > } > > + GenetRxComplete (Genet); > + > EfiReleaseLock (&Genet->Lock); > return Status; > } > Reviewed-by: Pete Batard