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 621BE2095B9E6 for ; Wed, 23 Aug 2017 13:39:08 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2091E267E0; Wed, 23 Aug 2017 20:41:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2091E267E0 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.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 5959E5D753; Wed, 23 Aug 2017 20:41:40 +0000 (UTC) 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> From: Laszlo Ersek Message-ID: <9713645a-bfc0-0dc9-c11b-0b154265c941@redhat.com> Date: Wed, 23 Aug 2017 22:41:39 +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: <1503490967-5559-8-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 23 Aug 2017 20:41:42 +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:39:08 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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.. > + > + @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 > @@ -166,7 +180,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 e5881d537f09..e6279159f8ba 100644 > --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h > +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h > @@ -115,7 +115,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 644ec65e1788..67458e56231b 100644 > --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c > +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c > @@ -181,11 +181,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 89ccac8c1c04..ef9a00710668 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..5cb003161207 100644 > --- a/OvmfPkg/VirtioGpuDxe/Commands.c > +++ b/OvmfPkg/VirtioGpuDxe/Commands.c > @@ -132,7 +132,11 @@ VirtioGpuInit ( > if (EFI_ERROR (Status)) { > goto Failed; > } > - Status = VgpuDev->VirtIo->SetQueueAddress (VgpuDev->VirtIo, &VgpuDev->Ring); > + Status = VgpuDev->VirtIo->SetQueueAddress ( > + VgpuDev->VirtIo, > + &VgpuDev->Ring, > + 0 > + ); > 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 bd912cca9b29..b52060d13d97 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; > } >