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 00EFC21E8796A for ; Wed, 13 Sep 2017 13:12:25 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B5677C0587DD; Wed, 13 Sep 2017 20:15:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B5677C0587DD Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-116.rdu2.redhat.com [10.10.120.116]) by smtp.corp.redhat.com (Postfix) with ESMTP id E11555D963; Wed, 13 Sep 2017 20:15:20 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <20170911121657.34992-1-brijesh.singh@amd.com> <20170911121657.34992-8-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 13 Sep 2017 22:15:19 +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: <20170911121657.34992-8-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 13 Sep 2017 20:15:22 +0000 (UTC) Subject: Re: [PATCH v2 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: Wed, 13 Sep 2017 20:12:25 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/11/17 14:16, 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 | 30 +++++++++++++---- > OvmfPkg/VirtioNetDxe/SnpTransmit.c | 34 ++++++++++++++++---- > 2 files changed, 50 insertions(+), 14 deletions(-) > > diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c > index 694940ea1d97..9bd62fbe8ec0 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c > +++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c > @@ -61,11 +61,12 @@ 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; > > if (This == NULL) { > return EFI_INVALID_PARAMETER; > @@ -141,9 +142,24 @@ VirtioNetGetStatus ( > ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1)); > > // > - // report buffer address to caller that has been enqueued by caller > + // get the device address to caller that has been enqueued by caller (1) I suggest, "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; > + > + // > + // Unmap the device address and perform the reverse mapping to find the > + // caller buffer address. > + // > + Status = VirtioNetUnmapTxBuf ( > + Dev, > + DescIdx + 1, (2) As discussed, this argument should go away. > + TxBuf, > + DeviceAddress > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto Exit; > + } Now, normally I would request the following: "Because you are introducing a new error path here, it is now possible that *InterruptStatus is modified earlier, but we exit with and error. Please introduce a local variable for InterruptStatus, and only assign *InterruptStatus when everything is fine." However, VirtioNetUnmapTxBuf() should never fail. It can only return an error if "DeviceAddress is not mapped", and that means our internal state has been corrupted somehow. (3) Therefore, please add such a comment, plus an "ASSERT (FALSE)" right above the "Status = EFI_DEVICE_ERROR" assignment. > > // > // now this descriptor can be used again to enqueue a transmit buffer (4) Please move the VirtioNetUnmapTxBuf() call *under* the TxFreeStack manipulation. So that the order of operations is ultimately: (4a) fetch DeviceAddress (4b) put DescIdx back on TxFreeStack (4c) call VirtioNetUnmapTxBuf() as final operation (followed by the error check + ASSERT, as discussed above) > diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c > index 7ca40d5d0650..0be3243b4606 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,29 @@ VirtioNetTransmit ( > } > > // > - // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device > + // Get the free VRING descriptor > // > DescIdx = Dev->TxFreeStack[Dev->TxCurPending++]; > - Dev->TxRing.Desc[DescIdx + 1].Addr = (UINTN) Buffer; > + > + // > + // Map the transmit buffer system physical address to device address. > + // > + Status = VirtioNetMapTxBuf ( > + Dev, > + DescIdx + 1, (5) this argument is going away > + Buffer, > + BufferSize, > + &DeviceAddress > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto Exit; > + } VirtioNetMapTxBuf() can genuinely fail for a number of reasons, and if we exit here like this, we will have leaked a descriptor from "TxFreeStack". (6) Therefore, please don't touch the comment // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device Instead, move the VirtioNetMapTxBuf() call right above that comment. The error handling for VirtioNetMapTxBuf() becomes correct then. Thanks! Laszlo > + > + // > + // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device > + // > + Dev->TxRing.Desc[DescIdx + 1].Addr = DeviceAddress; > Dev->TxRing.Desc[DescIdx + 1].Len = (UINT32) BufferSize; > > // >