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 608F721CFA5E5 for ; Wed, 16 Aug 2017 14:40:48 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4AD9280F9F; Wed, 16 Aug 2017 21:43:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4AD9280F9F 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 54AB66BF82; Wed, 16 Aug 2017 21:43:12 +0000 (UTC) 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> From: Laszlo Ersek Message-ID: Date: Wed, 16 Aug 2017 23:43:11 +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: <1502710605-8058-14-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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:43:14 +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:40:48 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (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(). (This documentation basically provides the specification for our VirtioRingMap() function.) (6) Then we can simply define RingBaseShift like this: @param[in] RingBaseShift Adding this value using UINT64 arithmetic to the addresses found in Ring translates them from system memory to bus addresses. The caller shall calculate RingBaseShift as (DeviceAddress - (UINT64)(UINTN)HostAddress), where DeviceAddress and HostAddress (i.e., Ring->Base) were output and input parameters of This->MapSharedBuffer(), respectively. > @retval EFI_SUCCESS The data was written successfully. > @retval EFI_UNSUPPORTED The underlying IO device doesn't support the > provided address offset and write size. > @@ -164,7 +166,8 @@ typedef > EFI_STATUS > (EFIAPI *VIRTIO_SET_QUEUE_ADDRESS) ( > IN VIRTIO_DEVICE_PROTOCOL *This, > - IN VRING *Ring > + IN VRING *Ring, > + IN UINT64 RingBaseShift > ); > > /** > diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h > index b69f6d7b7a85..b5cc091fe820 100644 > --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h > +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h > @@ -113,7 +113,8 @@ VirtioMmioSetQueueSel ( > EFI_STATUS > VirtioMmioSetQueueAddress ( > IN VIRTIO_DEVICE_PROTOCOL *This, > - IN VRING *Ring > + IN VRING *Ring, > + IN UINT64 RingBaseShift > ); > > EFI_STATUS > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > index 41df5a98e560..1f0dc45d501e 100644 > --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h > @@ -126,7 +126,8 @@ EFI_STATUS > EFIAPI > VirtioPciSetQueueAddress ( > IN VIRTIO_DEVICE_PROTOCOL *This, > - IN VRING *Ring > + IN VRING *Ring, > + IN UINT64 RingBaseShift > ); > > EFI_STATUS > diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c > index f3f69f324c6c..3d14b1035e91 100644 > --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c > +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c > @@ -183,11 +183,14 @@ VirtioMmioSetQueueSel ( > EFI_STATUS > VirtioMmioSetQueueAddress ( > IN VIRTIO_DEVICE_PROTOCOL *This, > - IN VRING *Ring > + IN VRING *Ring, > + IN UINT64 RingBaseShift > ) > { > VIRTIO_MMIO_DEVICE *Device; > > + ASSERT (RingBaseShift == 0); > + > Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This); > > VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_PFN, > diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c > index 413ffa06cf35..d102e1fd8551 100644 > --- a/OvmfPkg/Virtio10Dxe/Virtio10.c > +++ b/OvmfPkg/Virtio10Dxe/Virtio10.c > @@ -489,7 +489,8 @@ EFI_STATUS > EFIAPI > Virtio10SetQueueAddress ( > IN VIRTIO_DEVICE_PROTOCOL *This, > - IN VRING *Ring > + IN VRING *Ring, > + IN UINT64 RingBaseShift > ) > { > VIRTIO_1_0_DEV *Dev; > @@ -497,6 +498,8 @@ Virtio10SetQueueAddress ( > UINT64 Address; > UINT16 Enable; > > + ASSERT (RingBaseShift == 0); > + > Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This); > > Address = (UINTN)Ring->Desc; > diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > index 61b9cab4ff02..bff15fe3add1 100644 > --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > @@ -745,7 +745,7 @@ VirtioBlkInit ( > // > // step 4c -- Report GPFN (guest-physical frame number) of queue. > // > - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring); > + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0); > if (EFI_ERROR (Status)) { > goto ReleaseQueue; > } > diff --git a/OvmfPkg/VirtioGpuDxe/Commands.c b/OvmfPkg/VirtioGpuDxe/Commands.c > index c2e4d72feb67..df8ded99edff 100644 > --- a/OvmfPkg/VirtioGpuDxe/Commands.c > +++ b/OvmfPkg/VirtioGpuDxe/Commands.c > @@ -132,7 +132,8 @@ VirtioGpuInit ( > if (EFI_ERROR (Status)) { > goto Failed; > } > - Status = VgpuDev->VirtIo->SetQueueAddress (VgpuDev->VirtIo, &VgpuDev->Ring); > + Status = VgpuDev->VirtIo->SetQueueAddress (VgpuDev->VirtIo, > + &VgpuDev->Ring, 0); (7) Please break all arguments to separate lines (and the closing paren to another separate line). The rest looks fine, thanks! Laszlo > if (EFI_ERROR (Status)) { > goto ReleaseQueue; > } > diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > index 6d9b81a9f939..0ecfe044a977 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c > +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c > @@ -96,7 +96,7 @@ VirtioNetInitRing ( > // > // step 4c -- report GPFN (guest-physical frame number) of queue > // > - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring); > + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, 0); > if (EFI_ERROR (Status)) { > goto ReleaseQueue; > } > diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > index 4597095deb78..86f752e1651f 100644 > --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c > @@ -183,11 +183,14 @@ EFI_STATUS > EFIAPI > VirtioPciSetQueueAddress ( > IN VIRTIO_DEVICE_PROTOCOL *This, > - IN VRING *Ring > + IN VRING *Ring, > + IN UINT64 RingBaseShift > ) > { > VIRTIO_PCI_DEVICE *Dev; > > + ASSERT (RingBaseShift == 0); > + > Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This); > > return VirtioPciIoWrite (Dev, VIRTIO_PCI_OFFSET_QUEUE_ADDRESS, sizeof (UINT32), > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c > index e20602ac7225..0abca488e6cd 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c > @@ -298,7 +298,7 @@ VirtioRngInit ( > // > // step 4c -- Report GPFN (guest-physical frame number) of queue. > // > - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring); > + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0); > if (EFI_ERROR (Status)) { > goto ReleaseQueue; > } > diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > index c2f6f412ff40..a983b3df7b9c 100644 > --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > @@ -855,7 +855,7 @@ VirtioScsiInit ( > // > // step 4c -- Report GPFN (guest-physical frame number) of queue. > // > - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring); > + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0); > if (EFI_ERROR (Status)) { > goto ReleaseQueue; > } >