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 4A81620945C16 for ; Wed, 13 Sep 2017 07:29:02 -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 C71667E420; Wed, 13 Sep 2017 14:31:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C71667E420 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.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 4647A18ABE; Wed, 13 Sep 2017 14:31:58 +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-5-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 13 Sep 2017 16:31:56 +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-5-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.27]); Wed, 13 Sep 2017 14:31:59 +0000 (UTC) Subject: Re: [PATCH v2 4/8] 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, 13 Sep 2017 14:29:02 -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: > 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 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. > > 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/SnpInitialize.c | 64 +++++++++++++++++--- > OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +++ > 3 files changed, 65 insertions(+), 9 deletions(-) > > diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h > index 7d5f33b01dc8..3f48bcc6b67c 100644 > --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h > +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h > @@ -96,7 +96,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; // VirtioNetInitTx > UINT16 TxLastUsed; // VirtioNetInitTx > EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx > } VNET_DEV; > diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > index 54c808c501bf..6cedb406a172 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c > +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > @@ -18,6 +18,7 @@ > **/ > > #include > +#include > #include > #include > > @@ -147,6 +148,9 @@ ReleaseQueue: > > @retval EFI_OUT_OF_RESOURCES Failed to allocate the stack to track the heads > of free descriptor chains. > + @return Status codes from VIRTIO_DEVICE_PROTOCOL. > + AllocateSharedPages() or > + VirtioMapAllBytesInSharedBuffer() > @retval EFI_SUCCESS TX setup successful. > */ > > @@ -157,8 +161,11 @@ VirtioNetInitTx ( > IN OUT VNET_DEV *Dev > ) > { > - UINTN TxSharedReqSize; > - UINTN PktIdx; > + UINTN TxSharedReqSize; > + UINTN PktIdx; > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *TxSharedReqBuffer; > > Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2, > VNET_MAX_PENDING); > @@ -170,12 +177,42 @@ 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)) { > + goto FreeTxFreeStack; > + } > + > + ZeroMem (TxSharedReqBuffer, sizeof *Dev->TxSharedReq); > + > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + TxSharedReqBuffer, > + sizeof *(Dev->TxSharedReq), > + &DeviceAddress, > + &Dev->TxSharedReqMap > + ); > + if (EFI_ERROR (Status)) { > + goto FreeTxSharedReqBuffer; > + } > + > + Dev->TxSharedReq = TxSharedReqBuffer; > + > + > + // > // 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; > > for (PktIdx = 0; PktIdx < Dev->TxMaxPending; ++PktIdx) { > UINT16 DescIdx; > @@ -187,7 +224,7 @@ VirtioNetInitTx ( > // 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 = DeviceAddress; > Dev->TxRing.Desc[DescIdx].Len = (UINT32) TxSharedReqSize; > Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT; > Dev->TxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1); > @@ -202,13 +239,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 > @@ -223,6 +260,17 @@ VirtioNetInitTx ( > *Dev->TxRing.Avail.Flags = (UINT16) VRING_AVAIL_F_NO_INTERRUPT; > > return EFI_SUCCESS; > + > +FreeTxSharedReqBuffer: > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)), > + TxSharedReqBuffer > + ); > +FreeTxFreeStack: > + FreePool (Dev->TxFreeStack); > + > + return Status; > } > > > diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > index ee4f9ed36ecd..2ec3dc385a9f 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > @@ -54,6 +54,13 @@ 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 > + ); > + > FreePool (Dev->TxFreeStack); > } > > (1) Regarding the last argument of FreeSharedPages(), from my previous review at http://mid.mail-archive.com/ea935749-2a4f-bedd-44ee-05b59524ea07@redhat.com you missed: On 09/06/17 11:11, Laszlo Ersek wrote: > (18) The (VOID*) cast is unneeded. The rest is good. Thanks! Laszlo