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 7962121E87979 for ; Wed, 13 Sep 2017 02:11:11 -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 972DF4ACBB; Wed, 13 Sep 2017 09:14:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 972DF4ACBB Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.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 27B6117CD6; Wed, 13 Sep 2017 09:14:06 +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-4-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 13 Sep 2017 11:14:06 +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-4-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.38]); Wed, 13 Sep 2017 09:14:08 +0000 (UTC) Subject: Re: [PATCH v2 3/8] OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages() 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 09:11:11 -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, VirtioNetDxe is required to use the > device address in bus master operations. RxBuf is allocated using > AllocatePool() which returns the system physical address. > > The patch uses VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() to allocate > the RxBuf and map with VirtioMapAllBytesInSharedBuffer() so that we can > obtain the device address for RxBuf. > > 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 | 4 + > OvmfPkg/VirtioNetDxe/SnpInitialize.c | 77 +++++++++++++++----- > OvmfPkg/VirtioNetDxe/SnpReceive.c | 5 +- > OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +- > OvmfPkg/VirtioNetDxe/TechNotes.txt | 2 +- > 5 files changed, 74 insertions(+), 21 deletions(-) > > diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h > index 6762fc9d1d6e..7d5f33b01dc8 100644 > --- a/OvmfPkg/VirtioNetDxe/VirtioNet.h > +++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h > @@ -4,6 +4,7 @@ > Protocol instances for virtio-net devices. > > Copyright (C) 2013, Red Hat, Inc. > + Copyright (c) 2017, AMD Inc, All rights reserved.
> > This program and the accompanying materials are licensed and made available > under the terms and conditions of the BSD License which accompanies this > @@ -86,6 +87,8 @@ typedef struct { > // VirtioNetInitRing > UINT8 *RxBuf; // VirtioNetInitRx > UINT16 RxLastUsed; // VirtioNetInitRx > + UINTN RxBufNrPages; // VirtioNetInitRx > + VOID *RxBufMap; // VirtioNetInitRx > > VRING TxRing; // VirtioNetInitRing > VOID *TxRingMap; // VirtioRingMap and > @@ -95,6 +98,7 @@ typedef struct { > UINT16 *TxFreeStack; // VirtioNetInitTx > VIRTIO_1_0_NET_REQ TxSharedReq; // VirtioNetInitTx > UINT16 TxLastUsed; // VirtioNetInitTx > + EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx (1) For clarity, please move the new "RxBufDeviceBase" field between "RxBufNrPages" and "RxBufMap". > } VNET_DEV; > > > diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > index 8eabdbff6f5e..54c808c501bf 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c > +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > @@ -242,8 +242,10 @@ VirtioNetInitTx ( > @param[in,out] Dev The VNET_DEV driver instance about to enter the > EfiSimpleNetworkInitialized state. > > - @retval EFI_OUT_OF_RESOURCES Failed to allocate RX destination area. > - @return Status codes from VIRTIO_CFG_WRITE(). > + @retval EFI_OUT_OF_RESOURCES Failed to allocate or map RX destination area. > + @return Status codes from VIRTIO_CFG_WRITE() or > + VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages or > + VirtioMapAllBytesInSharedBuffer(). > @retval EFI_SUCCESS RX setup successful. The device is live and may > already be writing to the receive area. > */ The update to "@return ..." is fine. (2) But, please remove the "@retval EFI_OUT_OF_RESOURCES" line entirely; we no longer return this status code explicitly. > @@ -255,13 +257,15 @@ VirtioNetInitRx ( > IN OUT VNET_DEV *Dev > ) > { > - EFI_STATUS Status; > - UINTN VirtioNetReqSize; > - UINTN RxBufSize; > - UINT16 RxAlwaysPending; > - UINTN PktIdx; > - UINT16 DescIdx; > - UINT8 *RxPtr; > + EFI_STATUS Status; > + UINTN VirtioNetReqSize; > + UINTN RxBufSize; > + UINT16 RxAlwaysPending; > + UINTN PktIdx; > + UINT16 DescIdx; > + UINTN NumBytes; > + EFI_PHYSICAL_ADDRESS RxBufDeviceAddress; > + VOID *RxBuffer; > > // > // In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on > @@ -286,11 +290,37 @@ VirtioNetInitRx ( > // > RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, VNET_MAX_PENDING); > > - Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize); > - if (Dev->RxBuf == NULL) { > - return EFI_OUT_OF_RESOURCES; > + // > + // The RxBuf is shared between guest and hypervisor, use > + // AllocateSharedPages() to allocate this memory region and map it with > + // BusMasterCommonBuffer so that it can be accessed by both guest and > + // hypervisor. > + // > + NumBytes = RxAlwaysPending * RxBufSize; > + Dev->RxBufNrPages = EFI_SIZE_TO_PAGES (NumBytes); > + Status = Dev->VirtIo->AllocateSharedPages ( > + Dev->VirtIo, > + Dev->RxBufNrPages, > + &RxBuffer > + ); > + if (EFI_ERROR (Status)) { > + return Status; > } > (3) In this spot, please add: ZeroMem (RxBuffer, NumBytes); The rest looks good. Thank you, Laszlo > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + RxBuffer, > + NumBytes, > + &Dev->RxBufDeviceBase, > + &Dev->RxBufMap > + ); > + if (EFI_ERROR (Status)) { > + goto FreeSharedBuffer; > + } > + > + Dev->RxBuf = RxBuffer; > + > // > // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device > // > @@ -310,7 +340,7 @@ VirtioNetInitRx ( > // link each chain into (from) the available ring as well > // > DescIdx = 0; > - RxPtr = Dev->RxBuf; > + RxBufDeviceAddress = Dev->RxBufDeviceBase; > for (PktIdx = 0; PktIdx < RxAlwaysPending; ++PktIdx) { > // > // virtio-0.9.5, 2.4.1.2 Updating the Available Ring > @@ -321,16 +351,16 @@ VirtioNetInitRx ( > // > // virtio-0.9.5, 2.4.1.1 Placing Buffers into the Descriptor Table > // > - Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr; > + Dev->RxRing.Desc[DescIdx].Addr = RxBufDeviceAddress; > Dev->RxRing.Desc[DescIdx].Len = (UINT32) VirtioNetReqSize; > Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE | VRING_DESC_F_NEXT; > Dev->RxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1); > - RxPtr += Dev->RxRing.Desc[DescIdx++].Len; > + RxBufDeviceAddress += Dev->RxRing.Desc[DescIdx++].Len; > > - Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr; > + Dev->RxRing.Desc[DescIdx].Addr = RxBufDeviceAddress; > Dev->RxRing.Desc[DescIdx].Len = (UINT32) (RxBufSize - VirtioNetReqSize); > Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE; > - RxPtr += Dev->RxRing.Desc[DescIdx++].Len; > + RxBufDeviceAddress += Dev->RxRing.Desc[DescIdx++].Len; > } > > // > @@ -351,10 +381,21 @@ VirtioNetInitRx ( > Status = Dev->VirtIo->SetQueueNotify (Dev->VirtIo, VIRTIO_NET_Q_RX); > if (EFI_ERROR (Status)) { > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > - FreePool (Dev->RxBuf); > + goto UnmapSharedBuffer; > } > > return Status; > + > +UnmapSharedBuffer: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap); > + > +FreeSharedBuffer: > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + Dev->RxBufNrPages, > + RxBuffer > + ); > + return Status; > } > > > diff --git a/OvmfPkg/VirtioNetDxe/SnpReceive.c b/OvmfPkg/VirtioNetDxe/SnpReceive.c > index 99abd7ebe454..c42489636ea0 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpReceive.c > +++ b/OvmfPkg/VirtioNetDxe/SnpReceive.c > @@ -82,6 +82,7 @@ VirtioNetReceive ( > UINT8 *RxPtr; > UINT16 AvailIdx; > EFI_STATUS NotifyStatus; > + UINTN RxBufOffset; > > if (This == NULL || BufferSize == NULL || Buffer == NULL) { > return EFI_INVALID_PARAMETER; > @@ -143,7 +144,9 @@ VirtioNetReceive ( > *HeaderSize = Dev->Snm.MediaHeaderSize; > } > > - RxPtr = (UINT8 *)(UINTN) Dev->RxRing.Desc[DescIdx + 1].Addr; > + RxBufOffset = (UINTN)(Dev->RxRing.Desc[DescIdx + 1].Addr - > + Dev->RxBufDeviceBase); > + RxPtr = Dev->RxBuf + RxBufOffset; > CopyMem (Buffer, RxPtr, RxLen); > > if (DestAddr != NULL) { > diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > index 57c7395848bd..ee4f9ed36ecd 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > +++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > @@ -39,7 +39,12 @@ VirtioNetShutdownRx ( > IN OUT VNET_DEV *Dev > ) > { > - FreePool (Dev->RxBuf); > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap); > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + Dev->RxBufNrPages, > + Dev->RxBuf > + ); > } > > > diff --git a/OvmfPkg/VirtioNetDxe/TechNotes.txt b/OvmfPkg/VirtioNetDxe/TechNotes.txt > index 0891e8210489..f39426fb13e4 100644 > --- a/OvmfPkg/VirtioNetDxe/TechNotes.txt > +++ b/OvmfPkg/VirtioNetDxe/TechNotes.txt > @@ -247,7 +247,7 @@ In VirtioNetInitRx, the guest allocates the fixed size Receive Destination > Area, which accommodates all packets delivered asynchronously by the host. To > each packet, a slice of this area is dedicated; each slice is further > subdivided into virtio-net request header and network packet data. The > -(guest-physical) addresses of these sub-slices are denoted with A2, A3, A4 and > +(device-physical) addresses of these sub-slices are denoted with A2, A3, A4 and > so on. Importantly, an even-subscript "A" always belongs to a virtio-net > request header, while an odd-subscript "A" always belongs to a packet > sub-slice. >