[RFC PATCH 0/2] dma_direct_{alloc,free}() for Hyper-V IVMs

[RFC PATCH 0/2] dma_direct_{alloc,free}() for Hyper-V IVMs

[Andrea Parri (Microsoft)]

Through swiotlb_unencrypted_base.

P.S.  I'm on vacation for the next couple of weeks starting next Monday;
Dexuan/Michael should be able to address review feedback in that period.

Andrea Parri (Microsoft) (2):
  swiotlb,dma-direct: Move swiotlb_unencrypted_base to direct.c
  dma-direct: Fix dma_direct_{alloc,free}() for Hyperv-V IVMs

 arch/x86/kernel/cpu/mshyperv.c |  6 +++---
 include/linux/dma-direct.h     |  2 ++
 include/linux/swiotlb.h        |  2 --
 kernel/dma/direct.c            | 38 +++++++++++++++++++++++++++++++++-
 kernel/dma/swiotlb.c           | 12 +++++------
 5 files changed, 47 insertions(+), 13 deletions(-)

-- 
2.25.1

[RFC PATCH 1/2] swiotlb,dma-direct: Move swiotlb_unencrypted_base to direct.c

[Andrea Parri (Microsoft)]

The variable will come in handy to enable dma_direct_{alloc,free}()
for Hyper-V AMD SEV-SNP Isolated VMs.

Rename swiotlb_unencrypted_base to dma_unencrypted_base to indicate
that the notion is not restricted to SWIOTLB.

No functional change.

Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
Yeah, this is in some sense trading the dependency on SWIOTLB for a
dependency on HAS_DMA:

Q1. I'm unable to envision a scenario where SWIOTLB without HAS_DMA
would make sense but I'm also expecting one of the kernel test bots
to try such a nonsensical configuration... should the references to
dma_unencrypted_base in swiotlb.c be protected with HAS_DMA? other?

Q2. Can the #ifdef CONFIG_HAS_DMA in arch/x86/kernel/cpu/mshyperv.c
be removed? can we make HYPERV "depends on HAS_DMA"?

...

 arch/x86/kernel/cpu/mshyperv.c |  6 +++---
 include/linux/dma-direct.h     |  2 ++
 include/linux/swiotlb.h        |  2 --
 kernel/dma/direct.c            |  8 ++++++++
 kernel/dma/swiotlb.c           | 12 +++++-------
 5 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 831613959a92a..47e9cece86ff8 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -18,7 +18,7 @@
 #include <linux/kexec.h>
 #include <linux/i8253.h>
 #include <linux/random.h>
-#include <linux/swiotlb.h>
+#include <linux/dma-direct.h>
 #include <asm/processor.h>
 #include <asm/hypervisor.h>
 #include <asm/hyperv-tlfs.h>
@@ -333,8 +333,8 @@ static void __init ms_hyperv_init_platform(void)
 
 		if (hv_get_isolation_type() == HV_ISOLATION_TYPE_SNP) {
 			static_branch_enable(&isolation_type_snp);
-#ifdef CONFIG_SWIOTLB
-			swiotlb_unencrypted_base = ms_hyperv.shared_gpa_boundary;
+#ifdef CONFIG_HAS_DMA
+			dma_unencrypted_base = ms_hyperv.shared_gpa_boundary;
 #endif
 		}
 		/* Isolation VMs are unenlightened SEV-based VMs, thus this check: */
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 18aade195884d..0b7e4c4b7b34c 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -14,6 +14,8 @@
 
 extern unsigned int zone_dma_bits;
 
+extern phys_addr_t dma_unencrypted_base;
+
 /*
  * Record the mapping of CPU physical to DMA addresses for a given region.
  */
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 7ed35dd3de6e7..fa2e85f21af61 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -190,6 +190,4 @@ static inline bool is_swiotlb_for_alloc(struct device *dev)
 }
 #endif /* CONFIG_DMA_RESTRICTED_POOL */
 
-extern phys_addr_t swiotlb_unencrypted_base;
-
 #endif /* __LINUX_SWIOTLB_H */
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 8d0b68a170422..06b2b901e37a3 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -22,6 +22,14 @@
  */
 unsigned int zone_dma_bits __ro_after_init = 24;
 
+/*
+ * Certain Confidential Computing solutions, such as Hyper-V AMD SEV-SNP
+ * isolated VMs, use dma_unencrypted_base as a watermark: memory addresses
+ * below dma_unencrypted_base are treated as private, while memory above
+ * dma_unencrypted_base is treated as shared.
+ */
+phys_addr_t dma_unencrypted_base;
+
 static inline dma_addr_t phys_to_dma_direct(struct device *dev,
 		phys_addr_t phys)
 {
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index cb50f8d383606..78d4f5294a56c 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -67,8 +67,6 @@ static bool swiotlb_force_disable;
 
 struct io_tlb_mem io_tlb_default_mem;
 
-phys_addr_t swiotlb_unencrypted_base;
-
 static unsigned long default_nslabs = IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT;
 
 static int __init
@@ -142,7 +140,7 @@ static inline unsigned long nr_slots(u64 val)
 
 /*
  * Remap swioltb memory in the unencrypted physical address space
- * when swiotlb_unencrypted_base is set. (e.g. for Hyper-V AMD SEV-SNP
+ * when dma_unencrypted_base is set. (e.g. for Hyper-V AMD SEV-SNP
  * Isolation VMs).
  */
 #ifdef CONFIG_HAS_IOMEM
@@ -150,8 +148,8 @@ static void *swiotlb_mem_remap(struct io_tlb_mem *mem, unsigned long bytes)
 {
 	void *vaddr = NULL;
 
-	if (swiotlb_unencrypted_base) {
-		phys_addr_t paddr = mem->start + swiotlb_unencrypted_base;
+	if (dma_unencrypted_base) {
+		phys_addr_t paddr = mem->start + dma_unencrypted_base;
 
 		vaddr = memremap(paddr, bytes, MEMREMAP_WB);
 		if (!vaddr)
@@ -213,10 +211,10 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
 	}
 
 	/*
-	 * If swiotlb_unencrypted_base is set, the bounce buffer memory will
+	 * If dma_unencrypted_base is set, the bounce buffer memory will
 	 * be remapped and cleared in swiotlb_update_mem_attributes.
 	 */
-	if (swiotlb_unencrypted_base)
+	if (dma_unencrypted_base)
 		return;
 
 	memset(vaddr, 0, bytes);
-- 
2.25.1

[RFC PATCH 2/2] dma-direct: Fix dma_direct_{alloc,free}() for Hyperv-V IVMs

[Andrea Parri (Microsoft)]

In Hyper-V AMD SEV-SNP Isolated VMs, the virtual address returned by
dma_direct_alloc() must map above dma_unencrypted_base because the
memory is shared with the hardware device and must not be encrypted.

Modify dma_direct_alloc() to do the necessary remapping.  In
dma_direct_free(), use the (unmodified) DMA address to derive the
original virtual address and re-encrypt the pages.

Suggested-by: Michael Kelley <mikelley@microsoft.com>
Co-developed-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 kernel/dma/direct.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 06b2b901e37a3..c4ce277687a49 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -13,6 +13,7 @@
 #include <linux/vmalloc.h>
 #include <linux/set_memory.h>
 #include <linux/slab.h>
+#include <linux/io.h> /* for memremap() */
 #include "direct.h"
 
 /*
@@ -305,6 +306,21 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 		ret = page_address(page);
 		if (dma_set_decrypted(dev, ret, size))
 			goto out_free_pages;
+#ifdef CONFIG_HAS_IOMEM
+		/*
+		 * Remap the pages in the unencrypted physical address space
+		 * when dma_unencrypted_base is set (e.g., for Hyper-V AMD
+		 * SEV-SNP isolated guests).
+		 */
+		if (dma_unencrypted_base) {
+			phys_addr_t ret_pa = virt_to_phys(ret);
+
+			ret_pa += dma_unencrypted_base;
+			ret = memremap(ret_pa, size, MEMREMAP_WB);
+			if (!ret)
+				goto out_encrypt_pages;
+		}
+#endif
 	}
 
 	memset(ret, 0, size);
@@ -360,11 +376,23 @@ void dma_direct_free(struct device *dev, size_t size,
 	    dma_free_from_pool(dev, cpu_addr, PAGE_ALIGN(size)))
 		return;
 
-	if (is_vmalloc_addr(cpu_addr)) {
+	/*
+	 * If dma_unencrypted_base is set, the virtual address returned by
+	 * dma_direct_alloc() is in the vmalloc address range.
+	 */
+	if (!dma_unencrypted_base && is_vmalloc_addr(cpu_addr)) {
 		vunmap(cpu_addr);
 	} else {
 		if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_CLEAR_UNCACHED))
 			arch_dma_clear_uncached(cpu_addr, size);
+#ifdef CONFIG_HAS_IOMEM
+		if (dma_unencrypted_base) {
+			memunmap(cpu_addr);
+			/* re-encrypt the pages using the original address */
+			cpu_addr = page_address(pfn_to_page(PHYS_PFN(
+					dma_to_phys(dev, dma_addr))));
+		}
+#endif
 		if (dma_set_encrypted(dev, cpu_addr, size))
 			return;
 	}
-- 
2.25.1

Re: [RFC PATCH 2/2] dma-direct: Fix dma_direct_{alloc,free}() for Hyperv-V IVMs

[Christoph Hellwig]

On Wed, Jul 06, 2022 at 09:50:27PM +0200, Andrea Parri (Microsoft) wrote:
> @@ -305,6 +306,21 @@ void *dma_direct_alloc(struct device *dev, size_t size,
>  		ret = page_address(page);
>  		if (dma_set_decrypted(dev, ret, size))
>  			goto out_free_pages;
> +#ifdef CONFIG_HAS_IOMEM
> +		/*
> +		 * Remap the pages in the unencrypted physical address space
> +		 * when dma_unencrypted_base is set (e.g., for Hyper-V AMD
> +		 * SEV-SNP isolated guests).
> +		 */
> +		if (dma_unencrypted_base) {
> +			phys_addr_t ret_pa = virt_to_phys(ret);
> +
> +			ret_pa += dma_unencrypted_base;
> +			ret = memremap(ret_pa, size, MEMREMAP_WB);
> +			if (!ret)
> +				goto out_encrypt_pages;
> +		}
> +#endif


So:

this needs to move into dma_set_decrypted, otherwise we don't handle
the dma_alloc_pages case (never mind that this is pretty unreadable).

Which then again largely duplicates the code in swiotlb.  So I think
what we need here is a low-level helper that does the
set_memory_decrypted and memremap.  I'm not quite sure where it
should go, but maybe some of the people involved with memory
encryption might have good ideas.  unencrypted_base should go with
it and then both swiotlb and dma-direct can call it.

> +	/*
> +	 * If dma_unencrypted_base is set, the virtual address returned by
> +	 * dma_direct_alloc() is in the vmalloc address range.
> +	 */
> +	if (!dma_unencrypted_base && is_vmalloc_addr(cpu_addr)) {
>  		vunmap(cpu_addr);
>  	} else {
>  		if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_CLEAR_UNCACHED))
>  			arch_dma_clear_uncached(cpu_addr, size);
> +#ifdef CONFIG_HAS_IOMEM
> +		if (dma_unencrypted_base) {
> +			memunmap(cpu_addr);
> +			/* re-encrypt the pages using the original address */
> +			cpu_addr = page_address(pfn_to_page(PHYS_PFN(
> +					dma_to_phys(dev, dma_addr))));
> +		}
> +#endif
>  		if (dma_set_encrypted(dev, cpu_addr, size))

Same on the unmap side.  It might also be worth looking into reordering
the checks in some form instead o that raw dma_unencrypted_base check
before the unmap.

Re: [RFC PATCH 2/2] dma-direct: Fix dma_direct_{alloc,free}() for Hyperv-V IVMs

[Andrea Parri]

> > @@ -305,6 +306,21 @@ void *dma_direct_alloc(struct device *dev, size_t size,
> >  		ret = page_address(page);
> >  		if (dma_set_decrypted(dev, ret, size))
> >  			goto out_free_pages;
> > +#ifdef CONFIG_HAS_IOMEM
> > +		/*
> > +		 * Remap the pages in the unencrypted physical address space
> > +		 * when dma_unencrypted_base is set (e.g., for Hyper-V AMD
> > +		 * SEV-SNP isolated guests).
> > +		 */
> > +		if (dma_unencrypted_base) {
> > +			phys_addr_t ret_pa = virt_to_phys(ret);
> > +
> > +			ret_pa += dma_unencrypted_base;
> > +			ret = memremap(ret_pa, size, MEMREMAP_WB);
> > +			if (!ret)
> > +				goto out_encrypt_pages;
> > +		}
> > +#endif
> 
> 
> So:
> 
> this needs to move into dma_set_decrypted, otherwise we don't handle
> the dma_alloc_pages case (never mind that this is pretty unreadable).
> 
> Which then again largely duplicates the code in swiotlb.  So I think
> what we need here is a low-level helper that does the
> set_memory_decrypted and memremap.  I'm not quite sure where it
> should go, but maybe some of the people involved with memory
> encryption might have good ideas.  unencrypted_base should go with
> it and then both swiotlb and dma-direct can call it.

Agreed, will look into this more  (other people's ideas welcome).


> > +	/*
> > +	 * If dma_unencrypted_base is set, the virtual address returned by
> > +	 * dma_direct_alloc() is in the vmalloc address range.
> > +	 */
> > +	if (!dma_unencrypted_base && is_vmalloc_addr(cpu_addr)) {
> >  		vunmap(cpu_addr);
> >  	} else {
> >  		if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_CLEAR_UNCACHED))
> >  			arch_dma_clear_uncached(cpu_addr, size);
> > +#ifdef CONFIG_HAS_IOMEM
> > +		if (dma_unencrypted_base) {
> > +			memunmap(cpu_addr);
> > +			/* re-encrypt the pages using the original address */
> > +			cpu_addr = page_address(pfn_to_page(PHYS_PFN(
> > +					dma_to_phys(dev, dma_addr))));
> > +		}
> > +#endif
> >  		if (dma_set_encrypted(dev, cpu_addr, size))
> 
> Same on the unmap side.  It might also be worth looking into reordering
> the checks in some form instead o that raw dma_unencrypted_base check
> before the unmap.

Got it.

Thanks,
  Andrea