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 4D53E2095B9E6 for ; Wed, 23 Aug 2017 13:40:51 -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 120007E44C; Wed, 23 Aug 2017 20:43:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 120007E44C 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-116-43.phx2.redhat.com [10.3.116.43]) by smtp.corp.redhat.com (Postfix) with ESMTP id B33BE68B06; Wed, 23 Aug 2017 20:43:23 +0000 (UTC) From: Laszlo Ersek To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1503490967-5559-1-git-send-email-brijesh.singh@amd.com> <1503490967-5559-8-git-send-email-brijesh.singh@amd.com> <9713645a-bfc0-0dc9-c11b-0b154265c941@redhat.com> Message-ID: <83cf4f23-e569-d87a-b07b-795f0cf5319d@redhat.com> Date: Wed, 23 Aug 2017 22:43:22 +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: <9713645a-bfc0-0dc9-c11b-0b154265c941@redhat.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.27]); Wed, 23 Aug 2017 20:43:25 +0000 (UTC) Subject: Re: [PATCH v3 07/23] OvmfPkg/Virtio: take RingBaseShift in SetQueueAddress() 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, 23 Aug 2017 20:40:51 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/23/17 22:41, Laszlo Ersek wrote: > On 08/23/17 14:22, Brijesh Singh wrote: >> For the case when an IOMMU is used for translating system physical >> addresses to DMA bus master addresses, the transport-independent >> virtio device drivers will be required to map their VRING areas to >> bus addresses with VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() calls. >> >> VirtioRingMap() maps the ring buffer system physical to a bus address. >> When an IOMMU is used for translating the address then bus address can >> start at a different offset from the system physical address. > > (1) The paragraph that you now have as first paragraph above was my > suggestion, so thank you for picking it up. However, the second > paragraph should have been deleted; I suggested the now-first paragraph > as a replacement for the now-second one. > > I wrote, "to keep our references within the virtio device protocol". > VirtioRingMap() is a VirtioLib function, which is a utility layer on top > of the virtio device protocol. So, as I said, VirtioLib patches may > refer to both VirtioLib and the protocol, but protocol patches should > preferably only refer to the protocol, and not VirtioLib. > > VirtioLib --+ > | ^ | > | | | > | +-------+ > | > v > VirtioDeviceProtocol --+ > ^ | > | | > +--------+ > > This is also consistent with the reordering of the patches that I asked > for (and that you implemented well in v3, thank you for it). > > So, apologies if I wasn't clear enough of this -- it's not a big deal at > all, I can remove the second paragraph when I push this. > > Reviewed-by: Laszlo Ersek > > Thanks! > Laszlo > >> >> - MMIO and legacy virtio transport do not support IOMMU to translate the >> addresses hence RingBaseShift will always be set to zero. >> >> - modern virtio transport supports IOMMU to translate the address, in >> next patch we will update the Virtio10Dxe to use RingBaseShift offset. >> >> Suggested-by: Laszlo Ersek >> 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/Include/Protocol/VirtioDevice.h | 19 +++++++++++++++++-- >> OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h | 3 ++- >> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h | 3 ++- >> OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c | 5 ++++- >> OvmfPkg/Virtio10Dxe/Virtio10.c | 5 ++++- >> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 2 +- >> OvmfPkg/VirtioGpuDxe/Commands.c | 6 +++++- >> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 2 +- >> OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 5 ++++- >> OvmfPkg/VirtioRngDxe/VirtioRng.c | 2 +- >> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 2 +- >> 11 files changed, 42 insertions(+), 12 deletions(-) >> >> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h >> index 9a01932958a2..2e3a6d6edf04 100644 >> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h >> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h >> @@ -156,7 +156,21 @@ EFI_STATUS >> @param[in] This This instance of VIRTIO_DEVICE_PROTOCOL >> >> @param[in] Ring The initialized VRING object to take the >> - addresses from. >> + addresses from. The caller is responsible for >> + ensuring that on input, all Ring->NumPages pages, >> + starting at Ring->Base, have been successfully >> + mapped with a single call to >> + This->MapSharedBuffer() for CommonBuffer bus >> + master operation.. (2) I'll also remove one of the periods. Thanks Laszlo