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 3F89421EA35BE for ; Tue, 5 Sep 2017 08:03:17 -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 4458D8F511; Tue, 5 Sep 2017 15:06:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4458D8F511 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-185.rdu2.redhat.com [10.10.120.185]) by smtp.corp.redhat.com (Postfix) with ESMTP id 794CF6E46C; Tue, 5 Sep 2017 15:06:03 +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-3-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <959544e1-d15d-7dab-76fb-67c887379f81@redhat.com> Date: Tue, 5 Sep 2017 17:06:02 +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-3-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]); Tue, 05 Sep 2017 15:06:05 +0000 (UTC) Subject: Re: [PATCH 2/5] 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: Tue, 05 Sep 2017 15:03:17 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Looks like I'm slowly getting to the 2nd patch after all: On 09/01/17 13:24, 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.AllocateSharedPage() to allocate > the RxBuf and map with BusMasterCommonBuffer so that we can obtain the > device address for RxBuf. (1) the "AllocateSharedPage" member function is called "AllocateSharedPages" (plural) (2) since we mention mapping, it's probably best to mention the helper function that we use -- VirtioMapAllBytesInSharedBuffer() > > 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 | 75 ++++++++++++++++---- > OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 7 +- > 4 files changed, 77 insertions(+), 14 deletions(-) > > diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h > index d80d441b50a4..7df51bd044f5 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 > @@ -85,6 +86,8 @@ typedef struct { > VOID *RxRingMap; // VirtioRingMap > UINT8 *RxBuf; // VirtioNetInitRx > UINT16 RxLastUsed; // VirtioNetInitRx > + UINTN RxBufNoPages; // VirtioNetInitRx > + VOID *RxBufMap; // VirtioMapSharedBuffer (3) The comment on both new fields should be "VirtioNetInitRx". (4) Please rename "RxBufNoPages" to "RxBufNrPages". ("RxBufNumberOfPages" would be too long for this table, and its "mixed verbosity" wouldn't be consistent with the rest of the fields.) > > VRING TxRing; // VirtioNetInitRing > VOID *TxRingMap; // VirtioRingMap (5) I guess I can add this remark here, about the documentation: I've now re-read "TechNotes.txt", until the end of the Virtio internals -- Rx section (i.e. I stopped just before "Virtio internals -- Tx"). I think that this section needs to be updated in one spot only; namely the expression (guest-physical) addresses of these sub-slices should be replaced with (device-mapped) addresses of these sub-slices Please include such an update in this patch. > diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c > index 6950c4d56df1..0586a82cdf09 100644 > --- a/OvmfPkg/VirtioNetDxe/Events.c > +++ b/OvmfPkg/VirtioNetDxe/Events.c > @@ -95,4 +95,10 @@ VirtioNetExitBoot ( > // > Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->TxRingMap); > Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxRingMap); > + > + // > + // Unmap Rx buffer so that hypervisor will not be able get readable data after > + // device is reset. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap); > } (6) My earlier comment applies; the RX structures are erected in VirtioNetInitialize() --> VirtioNetInitRx(), so they are only available in EfiSimpleNetworkInitialized state. > diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > index 803a38bd4239..6052d40fc6cb 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c > +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > @@ -236,7 +236,7 @@ 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. > + @retval EFI_OUT_OF_RESOURCES Failed to allocate or map RX destination area. > @return Status codes from VIRTIO_CFG_WRITE(). > @retval EFI_SUCCESS RX setup successful. The device is live and may > already be writing to the receive area. > @@ -249,13 +249,17 @@ 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; > + UINT8 *RxPtr; > + UINTN NumBytes; > + EFI_PHYSICAL_ADDRESS RxBufDeviceAddress; > + UINT64 BufBaseShift; > + VOID *RxBuffer; > > // > // In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on > @@ -280,11 +284,40 @@ VirtioNetInitRx ( > // > RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, VNET_MAX_PENDING); > > - Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize); > - if (Dev->RxBuf == NULL) { > + // > + // 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->RxBufNoPages = EFI_SIZE_TO_PAGES (NumBytes); > + Status = Dev->VirtIo->AllocateSharedPages ( > + Dev->VirtIo, > + Dev->RxBufNoPages, > + &RxBuffer > + ); > + if (EFI_ERROR (Status)) { > return EFI_OUT_OF_RESOURCES; > } (7) I suggest propagating Status here, and updating the comment block above accordingly. > > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + RxBuffer, > + NumBytes, > + &RxBufDeviceAddress, > + &Dev->RxBufMap > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_OUT_OF_RESOURCES; (8) Same here, I suggest propagating the status code, and updating the leading comment block. > + goto FreeSharedBuffer; > + } > + > + Dev->RxBuf = RxBuffer; > + > + BufBaseShift = (UINT64) (UINTN)Dev->RxBuf - (UINT64) RxBufDeviceAddress; > + > // > // virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device > // > @@ -306,6 +339,11 @@ VirtioNetInitRx ( > DescIdx = 0; > RxPtr = Dev->RxBuf; > for (PktIdx = 0; PktIdx < RxAlwaysPending; ++PktIdx) { > + UINT64 Address; > + > + Address = (UINTN) RxPtr; > + Address += BufBaseShift; > + (9) Please remove the following local variables: "BufBaseShift" (new), "Address" (new), "RxPtr" (old). (10) In "VirtioNet.h", please introduce a new field as follows: EFI_PHYSICAL_ADDRESS RxBufDeviceBase; // VirtioNetInitRx (11) Rather than the "RxBufDeviceAddress" local variable, the "Dev->RxBufDeviceBase" field should receive the mapped address in this function. (12) The "RxBufDeviceAddress" (new) local variable should be preserved, and it should take the role of the current "RxPtr" variable. Namely, it should be - set to "Dev->RxBufDeviceBase" before the loop, - incremented in the loop, - and assigned to the "Addr" field. (13) And now, the explanation for all of the above: in this patch you missed the VirtioNetReceive() function, in "SnpReceive.c". In that function, we currently have RxPtr = (UINT8 *)(UINTN) Dev->RxRing.Desc[DescIdx + 1].Addr; In this patch, the "Addr" field has to be reverse-mapped, like this: UINTN RxBufOffset; RxBufOffset = (UINTN)(Dev->RxRing.Desc[DescIdx + 1].Addr - Dev->RxBufDeviceBase); RxPtr = Dev->RxBuf + RxBufOffset; (No change to the rest of the code in VirtioNetReceive().) > // > // virtio-0.9.5, 2.4.1.2 Updating the Available Ring > // invisible to the host until we update the Index Field > @@ -315,13 +353,13 @@ 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 = Address; > 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; > > - Dev->RxRing.Desc[DescIdx].Addr = (UINTN) RxPtr; > + Dev->RxRing.Desc[DescIdx].Addr = Address; > Dev->RxRing.Desc[DescIdx].Len = (UINT32) (RxBufSize - VirtioNetReqSize); > Dev->RxRing.Desc[DescIdx].Flags = VRING_DESC_F_WRITE; > RxPtr += Dev->RxRing.Desc[DescIdx++].Len; > @@ -345,10 +383,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->RxBufNoPages, > + (VOID *) Dev->RxBuf > + ); (14) The last argument is incorrect. If you jump here because VirtioMapAllBytesInSharedBuffer() fails, then Dev->RxBuf is not set yet, only RxBuffer. Please say RxBuffer. > + return Status; > } > > > diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c > index 9fedb72fdbd4..4c9d9ece0790 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->RxBufNoPages, > + (VOID *) Dev->RxBuf > + ); > } (15) There's no need for the explicit (VOID*) cast. Thanks! Laszlo