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 DB5EB21CFA5EE for ; Wed, 16 Aug 2017 14:48:39 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C685778EBD; Wed, 16 Aug 2017 21:51:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C685778EBD 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-59.phx2.redhat.com [10.3.116.59]) by smtp.corp.redhat.com (Postfix) with ESMTP id 72CE967CE3; Wed, 16 Aug 2017 21:51:04 +0000 (UTC) From: Laszlo Ersek To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502710605-8058-1-git-send-email-brijesh.singh@amd.com> <1502710605-8058-14-git-send-email-brijesh.singh@amd.com> Message-ID: <878033db-b6d4-6e4a-5d87-1136e1366697@redhat.com> Date: Wed, 16 Aug 2017 23:51:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 16 Aug 2017 21:51:06 +0000 (UTC) Subject: Re: [PATCH v2 13/23] OvmfPkg/Virtio: take RingBaseShift in VirtioSetQueueAddress() 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, 16 Aug 2017 21:48:40 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/16/17 23:43, Laszlo Ersek wrote: > (1) The "VirtioSetQueueAddress" identifier does not exist in the edk2 > tree as a function or protocol member name, so I suggest at least > replacing "VirtioSetQueueAddress" with "SetQueueAddress". > > On 08/14/17 13:36, Brijesh Singh wrote: >> 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. > > (2) I suggest the following wording in order to keep our references > within the virtio device protocol: > > 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. > >> - MMIO and legacy virtio device does not use IOMMU to translate the >> addresses hence RingBaseShift will always be set to zero. > > (3) s/device does not use/transports do not support/ > >> >> - modern virtio device use IOMMU to translate the address, in next patch > > (4) s/device use/transport supports/ > >> 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 | 5 ++++- >> 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 | 3 ++- >> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 2 +- >> OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 5 ++++- >> OvmfPkg/VirtioRngDxe/VirtioRng.c | 2 +- >> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 2 +- >> 11 files changed, 26 insertions(+), 11 deletions(-) >> >> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h >> index 14f980d7bf0a..25fd73b847a5 100644 >> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h >> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h >> @@ -156,6 +156,8 @@ EFI_STATUS >> @param[in] Ring The initialized VRING object to take the >> addresses from. >> >> + @param[in] RingBaseShift The offset for the Ring Base address. >> + > > OK, so this comment block is critical. > > (5) Please update the documentation of the "Ring" parameter like this: > > @param[in] Ring The initialized VRING object to take the > 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(). (5a) oops, small (but important) omission: please append "for CommonBuffer bus master operation". Thanks Laszlo