[RFC v1 0/2] Enable IOMMU support for pseries Secure VMs

[RFC v1 0/2] Enable IOMMU support for pseries Secure VMs

[Ram Pai]

This patch series enables IOMMU support for pseries Secure VMs.


Tested using QEMU command line option:

 "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
 	iommu_platform=on,disable-modern=off,disable-legacy=on"
 and 

 "-device virtio-blk-pci,scsi=off,bus=pci.0,
 	addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
 	iommu_platform=on,disable-modern=off,disable-legacy=on"

Ram Pai (2):
  powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
  powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.

 arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.

[Ram Pai]

The hypervisor needs to access the contents of the page holding the TCE
entries while setting up the TCE entries in the IOMMU's TCE table. For
SecureVMs, since this page is encrypted, the hypervisor cannot access
valid entries. Share the page with the hypervisor. This ensures that the
hypervisor sees the valid entries.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 8d9c2b1..07f0847 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -37,6 +37,7 @@
 #include <asm/mmzone.h>
 #include <asm/plpar_wrappers.h>
 #include <asm/svm.h>
+#include <asm/ultravisor.h>
 
 #include "pseries.h"
 
@@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
 
 static DEFINE_PER_CPU(__be64 *, tce_page);
 
+/*
+ * Allocate a tce page.  If secure VM, share the page with the hypervisor.
+ */
+static __be64 *alloc_tce_page(void)
+{
+	__be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
+
+	if (tcep && is_secure_guest())
+		uv_share_page(PHYS_PFN(__pa(tcep)), 1);
+
+	return tcep;
+}
+
 static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
 				     long npages, unsigned long uaddr,
 				     enum dma_data_direction direction,
@@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
 	 * from iommu_alloc{,_sg}()
 	 */
 	if (!tcep) {
-		tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
-		/* If allocation fails, fall back to the loop implementation */
+		tcep = alloc_tce_page();
 		if (!tcep) {
 			local_irq_restore(flags);
 			return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
@@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
 	return rc;
 }
 
+
 static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
 					unsigned long num_pfn, const void *arg)
 {
@@ -405,7 +419,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
 	tcep = __this_cpu_read(tce_page);
 
 	if (!tcep) {
-		tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
+		tcep = alloc_tce_page();
 		if (!tcep) {
 			local_irq_enable();
 			return -ENOMEM;
-- 
1.8.3.1

[RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.

[Ram Pai]

This enables IOMMU support for pseries Secure VMs.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/platforms/pseries/iommu.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 07f0847..189717b 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
 	of_reconfig_notifier_register(&iommu_reconfig_nb);
 	register_memory_notifier(&iommu_mem_nb);
 
-	/*
-	 * Secure guest memory is inacessible to devices so regular DMA isn't
-	 * possible.
-	 *
-	 * In that case keep devices' dma_map_ops as NULL so that the generic
-	 * DMA code path will use SWIOTLB to bounce buffers for DMA.
-	 */
-	if (!is_secure_guest())
-		set_pci_dma_ops(&dma_iommu_ops);
+	set_pci_dma_ops(&dma_iommu_ops);
 }
 
 static int __init disable_multitce(char *str)
-- 
1.8.3.1

Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.

[Alexey Kardashevskiy]

On 05/11/2019 08:28, Ram Pai wrote:
> The hypervisor needs to access the contents of the page holding the TCE
> entries while setting up the TCE entries in the IOMMU's TCE table. For
> SecureVMs, since this page is encrypted, the hypervisor cannot access
> valid entries. Share the page with the hypervisor. This ensures that the
> hypervisor sees the valid entries.
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 8d9c2b1..07f0847 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -37,6 +37,7 @@
>  #include <asm/mmzone.h>
>  #include <asm/plpar_wrappers.h>
>  #include <asm/svm.h>
> +#include <asm/ultravisor.h>
>  
>  #include "pseries.h"
>  
> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>  
>  static DEFINE_PER_CPU(__be64 *, tce_page);
>  
> +/*
> + * Allocate a tce page.  If secure VM, share the page with the hypervisor.
> + */
> +static __be64 *alloc_tce_page(void)
> +{
> +	__be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> +
> +	if (tcep && is_secure_guest())
> +		uv_share_page(PHYS_PFN(__pa(tcep)), 1);


There is no matching unshare in this patch.


> +
> +	return tcep;
> +}
> +
>  static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>  				     long npages, unsigned long uaddr,
>  				     enum dma_data_direction direction,
> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>  	 * from iommu_alloc{,_sg}()
>  	 */
>  	if (!tcep) {
> -		tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> -		/* If allocation fails, fall back to the loop implementation */
> +		tcep = alloc_tce_page();
>  		if (!tcep) {
>  			local_irq_restore(flags);
>  			return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
>  	return rc;
>  }
>  
> +

Unrelated.


>  static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
>  					unsigned long num_pfn, const void *arg)
>  {
> @@ -405,7 +419,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
>  	tcep = __this_cpu_read(tce_page);
>  
>  	if (!tcep) {
> -		tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> +		tcep = alloc_tce_page();
>  		if (!tcep) {
>  			local_irq_enable();
>  			return -ENOMEM;
> 

-- 
Alexey

Re: [RFC v1 0/2] Enable IOMMU support for pseries Secure VMs

[Alexey Kardashevskiy]

On 05/11/2019 08:28, Ram Pai wrote:
> This patch series enables IOMMU support for pseries Secure VMs.
> 
> 
> Tested using QEMU command line option:
> 
>  "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
>  	iommu_platform=on,disable-modern=off,disable-legacy=on"
>  and 
> 
>  "-device virtio-blk-pci,scsi=off,bus=pci.0,
>  	addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
>  	iommu_platform=on,disable-modern=off,disable-legacy=on"


Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.

> 
> Ram Pai (2):
>   powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
>   powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
> 
>  arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 

-- 
Alexey

RE: [RFC v1 0/2] Enable IOMMU support for pseries Secure VMs

[Ram Pai]

On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 05/11/2019 08:28, Ram Pai wrote:
> > This patch series enables IOMMU support for pseries Secure VMs.
> > 
> > 
> > Tested using QEMU command line option:
> > 
> >  "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> >  	iommu_platform=on,disable-modern=off,disable-legacy=on"
> >  and 
> > 
> >  "-device virtio-blk-pci,scsi=off,bus=pci.0,
> >  	addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> >  	iommu_platform=on,disable-modern=off,disable-legacy=on"
> 
> 
> Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.

I did see SLOF complaining, but that did not stop it from booting the guest.
My boot device was a virtio device, with iommu platform flag
enabled.

So I am not sure, under what conditions SLOF will fail to boot.   Any
idea?

RP

RE: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.

[Ram Pai]

On Wed, Nov 06, 2019 at 12:58:50PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 05/11/2019 08:28, Ram Pai wrote:
> > The hypervisor needs to access the contents of the page holding the TCE
> > entries while setting up the TCE entries in the IOMMU's TCE table. For
> > SecureVMs, since this page is encrypted, the hypervisor cannot access
> > valid entries. Share the page with the hypervisor. This ensures that the
> > hypervisor sees the valid entries.
> > 
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > index 8d9c2b1..07f0847 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -37,6 +37,7 @@
> >  #include <asm/mmzone.h>
> >  #include <asm/plpar_wrappers.h>
> >  #include <asm/svm.h>
> > +#include <asm/ultravisor.h>
> >  
> >  #include "pseries.h"
> >  
> > @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
> >  
> >  static DEFINE_PER_CPU(__be64 *, tce_page);
> >  
> > +/*
> > + * Allocate a tce page.  If secure VM, share the page with the hypervisor.
> > + */
> > +static __be64 *alloc_tce_page(void)
> > +{
> > +	__be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> > +
> > +	if (tcep && is_secure_guest())
> > +		uv_share_page(PHYS_PFN(__pa(tcep)), 1);
> 
> 
> There is no matching unshare in this patch.

The page is allocated and shared, and stays that way for the life of the
kernel. It is not explicitly unshared or freed.  It is however
implicitly unshared by the guest kernel, through a UV_UNSHARE_ALL_PAGES ucall
when the guest kernel reboots. And it also gets implicitly unshared by
the Ultravisor/Hypervisor, if the SVM abruptly terminates.

> 
> 
> > +
> > +	return tcep;
> > +}
> > +
> >  static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> >  				     long npages, unsigned long uaddr,
> >  				     enum dma_data_direction direction,
> > @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> >  	 * from iommu_alloc{,_sg}()
> >  	 */
> >  	if (!tcep) {
> > -		tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> > -		/* If allocation fails, fall back to the loop implementation */
> > +		tcep = alloc_tce_page();
> >  		if (!tcep) {
> >  			local_irq_restore(flags);
> >  			return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> > @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> >  	return rc;
> >  }
> >  
> > +
> 
> Unrelated.

yes. will fix it.

Thanks,
RP

Re: [RFC v1 0/2] Enable IOMMU support for pseries Secure VMs

[Michael S. Tsirkin]

On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 05/11/2019 08:28, Ram Pai wrote:
> > This patch series enables IOMMU support for pseries Secure VMs.
> > 
> > 
> > Tested using QEMU command line option:
> > 
> >  "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> >  	iommu_platform=on,disable-modern=off,disable-legacy=on"
> >  and 
> > 
> >  "-device virtio-blk-pci,scsi=off,bus=pci.0,
> >  	addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> >  	iommu_platform=on,disable-modern=off,disable-legacy=on"
> 
> 
> Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.

Shouldn't be hard to support: set up the iommu to allow everything
and ack the feature. Right?

> > 
> > Ram Pai (2):
> >   powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
> >   powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
> > 
> >  arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
> >  1 file changed, 18 insertions(+), 12 deletions(-)
> > 
> 
> -- 
> Alexey

Re: [RFC v1 0/2] Enable IOMMU support for pseries Secure VMs

[Michael Roth]

Quoting Michael S. Tsirkin (2019-11-06 12:06:37)
> On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
> > 
> > 
> > On 05/11/2019 08:28, Ram Pai wrote:
> > > This patch series enables IOMMU support for pseries Secure VMs.
> > > 
> > > 
> > > Tested using QEMU command line option:
> > > 
> > >  "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> > >     iommu_platform=on,disable-modern=off,disable-legacy=on"
> > >  and 
> > > 
> > >  "-device virtio-blk-pci,scsi=off,bus=pci.0,
> > >     addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> > >     iommu_platform=on,disable-modern=off,disable-legacy=on"
> > 
> > 
> > Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
> 
> Shouldn't be hard to support: set up the iommu to allow everything
> and ack the feature. Right?

It's not a static/linear mapping in this case so we need calls to map DMA
buffers as-needed. I've gotten it to boot with virtio-blk, but the patches
have some hacks and need cleanup, hoping to post them soon.

I'm a bit perplexed how we would manage to boot a guest without those
changes though, this is what I get with qemu 4.1.0:

  qemu-system-ppc64 -M pseries,ic-mode=xics -m 512M -bios /home/mdroth/w/build/qemu-4.1.0-build/pc-bios/slof.bin -device virtio-blk-pci,drive=drive0,id=blk0,disable-modern=off,disable-legacy=on,iommu_platform=on -drive file=/home/mdroth/vm/bionic-server-cloudimg-ppc64el.img,if=none,id=drive0 -trace enable=spapr_iommu\*,file=trace.out -monitor unix:/tmp/mon.sock,server,nowait -vga none -nographic
  qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
  qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
  qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
  
  
  SLOF **********************************************************************
  QEMU Starting
   Build Date = Jul  3 2019 12:26:14
   FW Version = git-ba1ab360eebe6338
   Press "s" to enter Open Firmware.
  
  Populating /vdevice methods
  Populating /vdevice/vty@71000000
  Populating /vdevice/nvram@71000001
  Populating /vdevice/l-lan@71000002
  Populating /vdevice/v-scsi@71000003
         SCSI: Looking for devices
            8200000000000000 CD-ROM   : "QEMU     QEMU CD-ROM      2.5+"
  Populating /pci@800000020000000
                       00 0000 (D) : 1af4 1042    virtio [ block ]
  No NVRAM common partition, re-initializing...
  Scanning USB
  Using default console: /vdevice/vty@71000000
  
    Welcome to Open Firmware
  
    Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
    This program and the accompanying materials are made available
    under the terms of the BSD License available at
    http://www.opensource.org/licenses/bsd-license.php
  
  
  Trying to load:  from: /pci@800000020000000/scsi@0 ... virtioblk_init: failed
  virtioblk_transfer: Access beyond end of device!

And then it hangs. This is with TCG so maybe it behaves differently with
KVM, but that's the result I would expect with the current SLOF code.

> 
> > > 
> > > Ram Pai (2):
> > >   powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
> > >   powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
> > > 
> > >  arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
> > >  1 file changed, 18 insertions(+), 12 deletions(-)
> > > 
> > 
> > -- 
> > Alexey
> 

RE: [RFC v1 0/2] Enable IOMMU support for pseries Secure VMs

[Ram Pai]

On Wed, Nov 06, 2019 at 12:52:02PM -0600, Michael Roth wrote:
> Quoting Michael S. Tsirkin (2019-11-06 12:06:37)
> > On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
> > > 
> > > 
> > > On 05/11/2019 08:28, Ram Pai wrote:
> > > > This patch series enables IOMMU support for pseries Secure VMs.
> > > > 
> > > > 
> > > > Tested using QEMU command line option:
> > > > 
> > > >  "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
> > > >     iommu_platform=on,disable-modern=off,disable-legacy=on"
> > > >  and 
> > > > 
> > > >  "-device virtio-blk-pci,scsi=off,bus=pci.0,
> > > >     addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
> > > >     iommu_platform=on,disable-modern=off,disable-legacy=on"
> > > 
> > > 
> > > Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
> > 
> > Shouldn't be hard to support: set up the iommu to allow everything
> > and ack the feature. Right?
> 
> It's not a static/linear mapping in this case so we need calls to map DMA
> buffers as-needed. I've gotten it to boot with virtio-blk, but the patches
> have some hacks and need cleanup, hoping to post them soon.
> 
> I'm a bit perplexed how we would manage to boot a guest without those
> changes though, this is what I get with qemu 4.1.0:
> 
>   qemu-system-ppc64 -M pseries,ic-mode=xics -m 512M -bios /home/mdroth/w/build/qemu-4.1.0-build/pc-bios/slof.bin -device virtio-blk-pci,drive=drive0,id=blk0,disable-modern=off,disable-legacy=on,iommu_platform=on -drive file=/home/mdroth/vm/bionic-server-cloudimg-ppc64el.img,if=none,id=drive0 -trace enable=spapr_iommu\*,file=trace.out -monitor unix:/tmp/mon.sock,server,nowait -vga none -nographic
>   qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
>   qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
>   qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
>   
>   
>   SLOF **********************************************************************
>   QEMU Starting
>    Build Date = Jul  3 2019 12:26:14
>    FW Version = git-ba1ab360eebe6338
>    Press "s" to enter Open Firmware.
>   
>   Populating /vdevice methods
>   Populating /vdevice/vty@71000000
>   Populating /vdevice/nvram@71000001
>   Populating /vdevice/l-lan@71000002
>   Populating /vdevice/v-scsi@71000003
>          SCSI: Looking for devices
>             8200000000000000 CD-ROM   : "QEMU     QEMU CD-ROM      2.5+"
>   Populating /pci@800000020000000
>                        00 0000 (D) : 1af4 1042    virtio [ block ]
>   No NVRAM common partition, re-initializing...
>   Scanning USB
>   Using default console: /vdevice/vty@71000000
>   
>     Welcome to Open Firmware
>   
>     Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
>     This program and the accompanying materials are made available
>     under the terms of the BSD License available at
>     https://urldefense.proofpoint.com/v2/url?u=http-3A__www.opensource.org_licenses_bsd-2Dlicense.php&d=DwIFaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=m-UrKChQVkZtnPpjbF6YY99NbT8FBByQ-E-ygV8luxw&m=-xG4gaWE7BANKGOFx0wmF5ZgZVd8A1r-tsN44n4JUW4&s=QcpPgRqeZAk1pICsA-kk2gNKKiMJLASiPVi-hPinur0&e= 
>   
>   
>   Trying to load:  from: /pci@800000020000000/scsi@0 ... virtioblk_init: failed
>   virtioblk_transfer: Access beyond end of device!
> 
> And then it hangs. This is with TCG so maybe it behaves differently with
> KVM, but that's the result I would expect with the current SLOF code.

you are right. In my case, I was providing the kernel image
on the qemu command line. So there was no need for SLOF to read the
virtio disk. Hence it continued without hanging. SLOF needs to be enhanced
to support IOMMU.


RP

Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.

[Alexey Kardashevskiy]

On 07/11/2019 04:01, Ram Pai wrote:
> On Wed, Nov 06, 2019 at 12:58:50PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 05/11/2019 08:28, Ram Pai wrote:
>>> The hypervisor needs to access the contents of the page holding the TCE
>>> entries while setting up the TCE entries in the IOMMU's TCE table. For
>>> SecureVMs, since this page is encrypted, the hypervisor cannot access
>>> valid entries. Share the page with the hypervisor. This ensures that the
>>> hypervisor sees the valid entries.
>>>
>>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>>> ---
>>>  arch/powerpc/platforms/pseries/iommu.c | 20 +++++++++++++++++---
>>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index 8d9c2b1..07f0847 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -37,6 +37,7 @@
>>>  #include <asm/mmzone.h>
>>>  #include <asm/plpar_wrappers.h>
>>>  #include <asm/svm.h>
>>> +#include <asm/ultravisor.h>
>>>  
>>>  #include "pseries.h"
>>>  
>>> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>>  
>>>  static DEFINE_PER_CPU(__be64 *, tce_page);
>>>  
>>> +/*
>>> + * Allocate a tce page.  If secure VM, share the page with the hypervisor.
>>> + */
>>> +static __be64 *alloc_tce_page(void)
>>> +{
>>> +	__be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
>>> +
>>> +	if (tcep && is_secure_guest())
>>> +		uv_share_page(PHYS_PFN(__pa(tcep)), 1);
>>
>>
>> There is no matching unshare in this patch.
> 
> The page is allocated and shared, and stays that way for the life of the
> kernel. It is not explicitly unshared or freed.


Ah, fair enough, I missed that, strange that we do not free it but ok. Thanks,


>  It is however
> implicitly unshared by the guest kernel, through a UV_UNSHARE_ALL_PAGES ucall
> when the guest kernel reboots. And it also gets implicitly unshared by
> the Ultravisor/Hypervisor, if the SVM abruptly terminates.
> 
>>
>>
>>> +
>>> +	return tcep;
>>> +}
>>> +
>>>  static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>>  				     long npages, unsigned long uaddr,
>>>  				     enum dma_data_direction direction,
>>> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>>>  	 * from iommu_alloc{,_sg}()
>>>  	 */
>>>  	if (!tcep) {
>>> -		tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
>>> -		/* If allocation fails, fall back to the loop implementation */
>>> +		tcep = alloc_tce_page();
>>>  		if (!tcep) {
>>>  			local_irq_restore(flags);
>>>  			return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
>>> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
>>>  	return rc;
>>>  }
>>>  
>>> +
>>
>> Unrelated.
> 
> yes. will fix it.
> 
> Thanks,
> RP
> 

-- 
Alexey

Re: [RFC v1 0/2] Enable IOMMU support for pseries Secure VMs

[Alexey Kardashevskiy]

On 07/11/2019 05:06, Michael S. Tsirkin wrote:
> On Wed, Nov 06, 2019 at 12:59:50PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 05/11/2019 08:28, Ram Pai wrote:
>>> This patch series enables IOMMU support for pseries Secure VMs.
>>>
>>>
>>> Tested using QEMU command line option:
>>>
>>>  "-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4,
>>>  	iommu_platform=on,disable-modern=off,disable-legacy=on"
>>>  and 
>>>
>>>  "-device virtio-blk-pci,scsi=off,bus=pci.0,
>>>  	addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,
>>>  	iommu_platform=on,disable-modern=off,disable-legacy=on"
>>
>>
>> Worth mentioning that SLOF won't boot with such devices as SLOF does not know about iommu_platform=on.
> 
> Shouldn't be hard to support: set up the iommu to allow everything
> and ack the feature. Right?

With the defaults we have (32bit window limited by 1GB and SLOF residing close to the end of the second GB), it is not
straight forward; also SLOF's XHCI and E1000 already use IOMMU so we should just do this for virtio as well, I am
hacking it now. Thanks,



> 
>>>
>>> Ram Pai (2):
>>>   powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.
>>>   powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.
>>>
>>>  arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++++++++------------
>>>  1 file changed, 18 insertions(+), 12 deletions(-)
>>>
>>
>> -- 
>> Alexey
> 

-- 
Alexey

Re: [RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:
> This enables IOMMU support for pseries Secure VMs.

Can you give us some more explanation please?

This is basically a revert of commit:
  edea902c1c1e ("powerpc/pseries/iommu: Don't use dma_iommu_ops on secure guests")

But neglects to remove the now unnecessary include of svm.h.

> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 07f0847..189717b 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
>  	of_reconfig_notifier_register(&iommu_reconfig_nb);
>  	register_memory_notifier(&iommu_mem_nb);
>  
> -	/*
> -	 * Secure guest memory is inacessible to devices so regular DMA isn't
> -	 * possible.
> -	 *
> -	 * In that case keep devices' dma_map_ops as NULL so that the generic
> -	 * DMA code path will use SWIOTLB to bounce buffers for DMA.

Please explain what has changed to make this no longer necessary.

cheers

> -	 */
> -	if (!is_secure_guest())
> -		set_pci_dma_ops(&dma_iommu_ops);
> +	set_pci_dma_ops(&dma_iommu_ops);
>  }
>  
>  static int __init disable_multitce(char *str)
> -- 
> 1.8.3.1

Re: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:
> The hypervisor needs to access the contents of the page holding the TCE
> entries while setting up the TCE entries in the IOMMU's TCE table. For
> SecureVMs, since this page is encrypted, the hypervisor cannot access
> valid entries. Share the page with the hypervisor. This ensures that the
> hypervisor sees the valid entries.

Can you please give people some explanation of why this is safe. After
all the point of the Ultravisor is to protect the guest from a malicious
hypervisor. Giving the hypervisor access to a page of TCEs sounds
dangerous, so please explain why it's not.

cheers

> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 8d9c2b1..07f0847 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -37,6 +37,7 @@
>  #include <asm/mmzone.h>
>  #include <asm/plpar_wrappers.h>
>  #include <asm/svm.h>
> +#include <asm/ultravisor.h>
>  
>  #include "pseries.h"
>  
> @@ -179,6 +180,19 @@ static int tce_build_pSeriesLP(struct iommu_table *tbl, long tcenum,
>  
>  static DEFINE_PER_CPU(__be64 *, tce_page);
>  
> +/*
> + * Allocate a tce page.  If secure VM, share the page with the hypervisor.
> + */
> +static __be64 *alloc_tce_page(void)
> +{
> +	__be64 *tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> +
> +	if (tcep && is_secure_guest())
> +		uv_share_page(PHYS_PFN(__pa(tcep)), 1);
> +
> +	return tcep;
> +}
> +
>  static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>  				     long npages, unsigned long uaddr,
>  				     enum dma_data_direction direction,
> @@ -206,8 +220,7 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
>  	 * from iommu_alloc{,_sg}()
>  	 */
>  	if (!tcep) {
> -		tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> -		/* If allocation fails, fall back to the loop implementation */
> +		tcep = alloc_tce_page();
>  		if (!tcep) {
>  			local_irq_restore(flags);
>  			return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
> @@ -391,6 +404,7 @@ static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
>  	return rc;
>  }
>  
> +
>  static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
>  					unsigned long num_pfn, const void *arg)
>  {
> @@ -405,7 +419,7 @@ static int tce_setrange_multi_pSeriesLP(unsigned long start_pfn,
>  	tcep = __this_cpu_read(tce_page);
>  
>  	if (!tcep) {
> -		tcep = (__be64 *)__get_free_page(GFP_ATOMIC);
> +		tcep = alloc_tce_page();
>  		if (!tcep) {
>  			local_irq_enable();
>  			return -ENOMEM;
> -- 
> 1.8.3.1

RE: [RFC v1 2/2] powerpc/pseries/iommu: Use dma_iommu_ops for Secure VMs aswell.

[Ram Pai]

On Thu, Nov 07, 2019 at 09:26:28PM +1100, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > This enables IOMMU support for pseries Secure VMs.
> 
> Can you give us some more explanation please?

Yes. Will do. 

The simple explanation is -- it was a mistake. We should 
not have disabled IOMMU ops for secure guests. Though it enabled
us to use virtio devices, with the help of some additional patches to
the virtio subsystem; in hindsight, we should not have disabled IOMMU
ops for secure VMs  :-(. 

RP



> 
> This is basically a revert of commit:
>   edea902c1c1e ("powerpc/pseries/iommu: Don't use dma_iommu_ops on secure guests")
> 
> But neglects to remove the now unnecessary include of svm.h.
> 
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > index 07f0847..189717b 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -1333,15 +1333,7 @@ void iommu_init_early_pSeries(void)
> >  	of_reconfig_notifier_register(&iommu_reconfig_nb);
> >  	register_memory_notifier(&iommu_mem_nb);
> >  
> > -	/*
> > -	 * Secure guest memory is inacessible to devices so regular DMA isn't
> > -	 * possible.
> > -	 *
> > -	 * In that case keep devices' dma_map_ops as NULL so that the generic
> > -	 * DMA code path will use SWIOTLB to bounce buffers for DMA.
> 
> Please explain what has changed to make this no longer necessary.
> 
> cheers
> 
> > -	 */
> > -	if (!is_secure_guest())
> > -		set_pci_dma_ops(&dma_iommu_ops);
> > +	set_pci_dma_ops(&dma_iommu_ops);
> >  }
> >  
> >  static int __init disable_multitce(char *str)
> > -- 
> > 1.8.3.1

-- 
Ram Pai

RE: [RFC v1 1/2] powerpc/pseries/iommu: Share the per-cpu TCE page with the hypervisor.

[Ram Pai]

On Thu, Nov 07, 2019 at 09:29:54PM +1100, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > The hypervisor needs to access the contents of the page holding the TCE
> > entries while setting up the TCE entries in the IOMMU's TCE table. For
> > SecureVMs, since this page is encrypted, the hypervisor cannot access
> > valid entries. Share the page with the hypervisor. This ensures that the
> > hypervisor sees the valid entries.
> 
> Can you please give people some explanation of why this is safe. After
> all the point of the Ultravisor is to protect the guest from a malicious
> hypervisor. Giving the hypervisor access to a page of TCEs sounds
> dangerous, so please explain why it's not.

Yes. will do, in my next version of the patch.

BTW: this page, which is shareed with the hypervisor contains nothing
but TCE entries. The hypervisor has a need to see those entries, so that it
can update the TCE table with correct entires.

Yes, a malicious hypervisor may try to update the TCE table with entries
that point to incorrect memory location.  But doing so will not help the
hypervisor to steal any data from those memory location, because those
memory location; if accessed by the hypervisor, will only fetch
encrypted data.

At most it can lead to denial of service, but not stolen data.

RP