From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CD10421D492C5 for ; Thu, 14 Sep 2017 13:55:59 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 901117F354; Thu, 14 Sep 2017 20:58:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 901117F354 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-80.rdu2.redhat.com [10.10.121.80]) by smtp.corp.redhat.com (Postfix) with ESMTP id B62EC18255; Thu, 14 Sep 2017 20:58:56 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <20170914110822.112540-1-brijesh.singh@amd.com> <20170914110822.112540-8-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Thu, 14 Sep 2017 22:58:55 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170914110822.112540-8-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 14 Sep 2017 20:58:58 +0000 (UTC) Subject: Re: [PATCH v3 7/8] OvmfPkg/VirtioNetDxe: map caller-supplied Tx packet to device-address X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Sep 2017 20:56:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/14/17 13:08, Brijesh Singh wrote: > When device is behind the IOMMU, driver is require to pass the device > address of caller-supplied transmit buffer for the bus master operations. > > The patch uses VirtioNetMapTxBuf() to map caller-supplied Tx packet to a > device-address and enqueue the device address in VRING for transfer and > perform the reverse mapping when transfer is completed so that we can > return the caller-supplied buffer. > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Tom Lendacky > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 41 +++++++++++++++----- > OvmfPkg/VirtioNetDxe/SnpTransmit.c | 27 ++++++++++--- > 2 files changed, 53 insertions(+), 15 deletions(-) > > diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c > index 694940ea1d97..f817b98801fa 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c > +++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c > @@ -61,11 +61,15 @@ VirtioNetGetStatus ( > OUT VOID **TxBuf OPTIONAL > ) > { > - VNET_DEV *Dev; > - EFI_TPL OldTpl; > - EFI_STATUS Status; > - UINT16 RxCurUsed; > - UINT16 TxCurUsed; > + VNET_DEV *Dev; > + EFI_TPL OldTpl; > + EFI_STATUS Status; > + UINT16 RxCurUsed; > + UINT16 TxCurUsed; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + UINT32 LocalInterruptStatus; > + > + LocalInterruptStatus = *InterruptStatus; > > if (This == NULL) { > return EFI_INVALID_PARAMETER; > @@ -113,11 +117,11 @@ VirtioNetGetStatus ( > // > *InterruptStatus = 0; > if (Dev->RxLastUsed != RxCurUsed) { > - *InterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; > + LocalInterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; > } > if (Dev->TxLastUsed != TxCurUsed) { > ASSERT (Dev->TxCurPending > 0); > - *InterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; > + LocalInterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; > } > } > I must have been unclear in my v2 review, sorry about that. My point was that, given the changes that I requested in points v2/3 and v2/4, the handling of "InterruptStatus" was fine as it was. (1) Therefore please undo the v3 "InterruptStatus" changes -- the "LocalInterruptStatus" variable is unnecessary. > @@ -141,17 +145,36 @@ VirtioNetGetStatus ( > ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1)); > > // > - // report buffer address to caller that has been enqueued by caller > + // get the device address that has been enqueued for the caller's > + // transmit buffer > // > - *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr; > + DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr; > > // > // now this descriptor can be used again to enqueue a transmit buffer > // > Dev->TxFreeStack[--Dev->TxCurPending] = (UINT16) DescIdx; > + > + // > + // Unmap the device address and perform the reverse mapping to find the > + // caller buffer address. > + // > + Status = VirtioNetUnmapTxBuf ( > + Dev, > + TxBuf, > + DeviceAddress > + ); > + if (EFI_ERROR (Status)) { > + // > + // VirtioNetUnmapTxBuf should never fail, if we have reached here > + // that means our internal state has been corrupted > + // > + ASSERT (FALSE); The rest of VirtioNetGetStatus() looks good, but here's the other misunderstanding: (2) Please *keep* the ASSERT() that you are adding above, and below it, *add back* what you had in v2: Status = EFI_DEVICE_ERROR; goto Exit; The rest looks good. Can you submit v4 soon please? :) Thanks, Laszlo > + } > } > } > > + *InterruptStatus = LocalInterruptStatus; > Status = EFI_SUCCESS; > > Exit: > diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c > index 7ca40d5d0650..b39226e138b9 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c > +++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c > @@ -73,11 +73,12 @@ VirtioNetTransmit ( > IN UINT16 *Protocol OPTIONAL > ) > { > - VNET_DEV *Dev; > - EFI_TPL OldTpl; > - EFI_STATUS Status; > - UINT16 DescIdx; > - UINT16 AvailIdx; > + VNET_DEV *Dev; > + EFI_TPL OldTpl; > + EFI_STATUS Status; > + UINT16 DescIdx; > + UINT16 AvailIdx; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > > if (This == NULL || BufferSize == 0 || Buffer == NULL) { > return EFI_INVALID_PARAMETER; > @@ -144,10 +145,24 @@ VirtioNetTransmit ( > } > > // > + // Map the transmit buffer system physical address to device address. > + // > + Status = VirtioNetMapTxBuf ( > + Dev, > + Buffer, > + BufferSize, > + &DeviceAddress > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto Exit; > + } > + > + // > // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device > // > DescIdx = Dev->TxFreeStack[Dev->TxCurPending++]; > - Dev->TxRing.Desc[DescIdx + 1].Addr = (UINTN) Buffer; > + Dev->TxRing.Desc[DescIdx + 1].Addr = DeviceAddress; > Dev->TxRing.Desc[DescIdx + 1].Len = (UINT32) BufferSize; > > // >