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 A9B1C21E977EA for ; Wed, 6 Sep 2017 02:09:01 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E9A84882FF; Wed, 6 Sep 2017 09:11:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E9A84882FF Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-41.rdu2.redhat.com [10.10.120.41]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4EC3619E03; Wed, 6 Sep 2017 09:11:49 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1504265045-19008-1-git-send-email-brijesh.singh@amd.com> <1504265045-19008-4-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 6 Sep 2017 11:11:48 +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: <1504265045-19008-4-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 06 Sep 2017 09:11:51 +0000 (UTC) Subject: Re: [PATCH 3/5] OvmfPkg/VirtioNetDxe: dynamically alloc transmit header 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, 06 Sep 2017 09:09:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/01/17 13:24, Brijesh Singh wrote: > A network packets are transmitted by placing them into a transmit queue, > each packet is preceded by a VIRTIO_1_0_NET_REQ header. VirtioNetInitTx(), > builds the header once and fills the vring descriptors with the system > physical address of the VIRTIO_1_0_NET_REQ header. (1) Please *replace* this paragraph (emphasis added to avoid misunderstandings) with the following: ---- Each network packet is submitted for transmission by pushing the head descriptor of a two-part descriptor chain to the Available Ring of the TX queue. VirtioNetInitTx() sets up the the descriptor chains for all queueable packets in advance, and points all the head descriptors to the same shared, never modified, VIRTIO_1_0_NET_REQ header object (or its initial VIRTIO_NET_REQ sub-object, dependent on virtio version). VirtioNetInitTx() currently uses the header object's system physical address for populating the head descriptors. ---- > > When device is behind the IOMMU, VirtioNet driver is required to provide > the device address of VIRTIO_1_0_NET_REQ header. In this patch we > dynamically allocate the header using AllocateSharedPages() and map with > BusMasterCommonBuffer so that header can be accessed by both processor > and the device. > > We could map it with BusMasterRead but since the header pointer is > used after it was added into vring hence I choose to dynamically allocate > and map it with BusMasterCommonBuffer to avoid the code complexity. (2) This is a very good choice!, but I'd like the commit message to be clearer. How about this, as a replacement for the last paragraph: ------- We map the header object for CommonBuffer operation because, in order to stick with the current code order, we populate the head descriptors with the header's device address first, and fill in the header itself second second. ------- (And then, we're not even mentioning that the header has to be unmappable at ExitBootServices() without freeing memory. But see the parallel discussion about that anyway...) > > 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/VirtioNet.h | 3 +- > OvmfPkg/VirtioNetDxe/Events.c | 6 ++ > OvmfPkg/VirtioNetDxe/SnpInitialize.c | 65 +++++++++++++++++--- > OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +++ > 4 files changed, 72 insertions(+), 9 deletions(-) > > diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h > index 7df51bd044f5..3efe95a735d8 100644 > --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h > +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h > @@ -94,7 +94,8 @@ typedef struct { > UINT16 TxMaxPending; // VirtioNetInitTx > UINT16 TxCurPending; // VirtioNetInitTx > UINT16 *TxFreeStack; // VirtioNetInitTx > - VIRTIO_1_0_NET_REQ TxSharedReq; // VirtioNetInitTx > + VIRTIO_1_0_NET_REQ *TxSharedReq; // VirtioNetInitTx > + VOID *TxSharedReqMap; // VirtioMapSharedBuffer (3) Consistently with this code, and with my other comments for this patch-set (and inconsistently with the other virtio drivers, sorry about that), please make the comment say "VirtioNetInitTx" for "TxSharedReqMap" as well. > UINT16 TxLastUsed; // VirtioNetInitTx > } VNET_DEV; > > diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c > index 0586a82cdf09..9366aa00a1b3 100644 > --- a/OvmfPkg/VirtioNetDxe/Events.c > +++ b/OvmfPkg/VirtioNetDxe/Events.c > @@ -101,4 +101,10 @@ VirtioNetExitBoot ( > // device is reset. > // > Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap); > + > + // > + // Unmap shared Tx request mapping so that hypervisor will not be able to get > + // readable data after device is reset. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap); > } (4) This too belongs in the "EfiSimpleNetworkInitialized" scope. (Assuming we're going to stick with the unmap-at-ExitBootServices idea at all... See the other discussion. For now, I recommend going ahead with this approach.) > diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > index 6052d40fc6cb..551820e30f36 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c > +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > @@ -18,6 +18,7 @@ > **/ > > #include > +#include > #include > #include > > @@ -151,8 +152,12 @@ VirtioNetInitTx ( > IN OUT VNET_DEV *Dev > ) > { > - UINTN TxSharedReqSize; > - UINTN PktIdx; > + UINTN TxSharedReqSize; > + UINTN PktIdx; > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + UINT64 BufBaseShift; (5) Please drop "BufBaseShift". > + VOID *TxSharedReqBuffer; > > Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2, > VNET_MAX_PENDING); > @@ -164,24 +169,60 @@ VirtioNetInitTx ( > } > > // > + // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it > + // can be accessed equally by both processor and device. > + // > + Status = Dev->VirtIo->AllocateSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)), > + &TxSharedReqBuffer > + ); > + if (EFI_ERROR (Status)) { > + return EFI_OUT_OF_RESOURCES; (6) This is wrong, we would leak "Dev->TxFreeStack" by returning here. Please jump to an error handling label like "FreeTxFreeStack". (7) The Status value should be propagated here, not masked as EFI_OUT_OF_RESOURCES. The error handling label will handle this anyway, but I'm spelling it out so that you please update the leading comment block on the function (= error conditions). > + } > + > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + TxSharedReqBuffer, > + sizeof *(Dev->TxSharedReq), > + &DeviceAddress, > + &Dev->TxSharedReqMap > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_OUT_OF_RESOURCES; (8) Please don't mask the returned Status. > + goto FreeBuffer; (9) For clarity, I suggest "FreeTxSharedReqBuffer" for the label. > + } > + > + Dev->TxSharedReq = TxSharedReqBuffer; > + > + BufBaseShift = (UINT64) (UINTN)Dev->TxSharedReq - (UINT64) DeviceAddress; > + > + ZeroMem ((VOID *) Dev->TxSharedReq, sizeof *(Dev->TxSharedReq)); (10) This ZeroMem() is in the wrong spot; it should be done *between* AllocateSharedPages() and VirtioMapAllBytesInSharedBuffer(). (11) No need for the explicit (VOID*) cast. (12) I have a question about *(Dev->TxSharedReq) Did you write the expression like this because you genuinely find the parens easier to read, or because you did a search-and-replace, and wanted to ensure that the operator bindings stay fine? If you genuinely like it better this way, I can accept that -- I dislike it :) --, but if you just did a search-and-replace, then please replace all occurrences with *Dev->TxSharedReq (this affects other files modified in the patch as well). > + > + // > // In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on > // VIRTIO_NET_F_MRG_RXBUF, which we never negotiate. > // > TxSharedReqSize = (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) ? > - sizeof Dev->TxSharedReq.V0_9_5 : > - sizeof Dev->TxSharedReq; > + sizeof ((Dev->TxSharedReq)->V0_9_5) : > + sizeof *(Dev->TxSharedReq); (13) More of the above, but in this case, I don't feel so lenient; the following expression: (Dev->TxSharedReq)->V0_9_5 simply stumps me :) Please replace it with: Dev->TxSharedReq->V0_9_5 > > for (PktIdx = 0; PktIdx < Dev->TxMaxPending; ++PktIdx) { > UINT16 DescIdx; > + UINT64 Address; (14) Please drop "Address". > > DescIdx = (UINT16) (2 * PktIdx); > Dev->TxFreeStack[PktIdx] = DescIdx; > > + Address = (UINTN) Dev->TxSharedReq; > + Address += BufBaseShift; > + > // > // For each possibly pending packet, lay out the descriptor for the common > // (unmodified by the host) virtio-net request header. > // > - Dev->TxRing.Desc[DescIdx].Addr = (UINTN) &Dev->TxSharedReq; > + Dev->TxRing.Desc[DescIdx].Addr = Address; (15) Simply assign "DeviceAddress". (BTW, your BufBaseShift calculation should be negated anyway, because you want to go from host address to device address. You start with the host address, so your addend, i.e. BusBaseShift, has to *subtract* the host address, and *add* the device address. But it's moot anyway, just assign DeviceAddress to Addr.) > Dev->TxRing.Desc[DescIdx].Len = (UINT32) TxSharedReqSize; > Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT; > Dev->TxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1); > @@ -196,13 +237,13 @@ VirtioNetInitTx ( > // > // virtio-0.9.5, Appendix C, Packet Transmission > // > - Dev->TxSharedReq.V0_9_5.Flags = 0; > - Dev->TxSharedReq.V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE; > + Dev->TxSharedReq->V0_9_5.Flags = 0; > + Dev->TxSharedReq->V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE; > > // > // For VirtIo 1.0 only -- the field exists, but it is unused > // > - Dev->TxSharedReq.NumBuffers = 0; > + Dev->TxSharedReq->NumBuffers = 0; > > // > // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device > @@ -217,6 +258,14 @@ VirtioNetInitTx ( > *Dev->TxRing.Avail.Flags = (UINT16) VRING_AVAIL_F_NO_INTERRUPT; > > return EFI_SUCCESS; > + > +FreeBuffer: > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)), > + (VOID *) Dev->TxSharedReq > + ); (16) The last argument is wrong. If VirtioMapAllBytesInSharedBuffer() fails and you jump here, you won't have assigned "Dev->TxSharedReq" the value of "TxSharedReqBuffer" just yet. So please replace the last argument with "TxSharedReqBuffer". (17) The explicit (VOID*) cast is unnecessary. > + return Status; > } > > > diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > index 4c9d9ece0790..aeb9e6605f0d 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > @@ -54,5 +54,12 @@ VirtioNetShutdownTx ( > IN OUT VNET_DEV *Dev > ) > { > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap); > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)), > + (VOID *) Dev->TxSharedReq > + ); > + (18) The (VOID*) cast is unneeded. > FreePool (Dev->TxFreeStack); > } > Thanks! Laszlo