Re: [PATCH 11/13] dma-direct: handle the memory encryption bit in common code
On 3/19/2018 5:39 AM, Christoph Hellwig wrote: > Can you test and review the V3 of the series I just sent out? > We reall should get it into linux-next ASAP. Can do. I'll get back to you on V3 thread with the results. Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 11/13] dma-direct: handle the memory encryption bit in common code
Can you test and review the V3 of the series I just sent out? We reall should get it into linux-next ASAP. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 11/13] dma-direct: handle the memory encryption bit in common code
On 03/13/2018 08:10 AM, Christoph Hellwig wrote:
> On Mon, Mar 12, 2018 at 02:48:51PM -0500, Tom Lendacky wrote:
>> Ok, I found one issue that allows this to work when the IOMMU isn't
>> enabled (see below).
>
> Thanks, folded!
>
>> But the bigger issue is when the IOMMU is enabled. The IOMMU code uses
>> a common mapping routine to create the I/O page tables. This routine
>> assumes that all memory being mapped is encrypted and therefore sets the
>> encryption bit in the I/O page tables. With this patch, the call to
>> dma_alloc_direct() now returns un-encrypted memory which results in an
>> encryption mis-match. I think keeping dma_alloc_direct() as it was prior
>> to this patch is the way to go. It allows SME DMA allocations to remain
>> encrypted and avoids added complexity in the amd_iommu.c file. This
>> would mean that SEV would still have special DMA operations (so that the
>> alloc/free can change the memory to un-encrypted).
>>
>> What do you think?
>
> In terms of logic you are right. I still don't like keeping a just
> slightly tweaked version of dma_alloc_direct around just for this, it
> will be perpetually out of sync in terms of features and bug fixes.
>
> What do you think about this version that does the decision at runtime:
>
>
> http://git.infradead.org/users/hch/misc.git/commitdiff/b89f24dc856595dc7610d672bf077195ab0dabf4
>
> The full tree is available here for testing:
>
> git://git.infradead.org/users/hch/misc.git dma-direct-x86
>
Thanks for the pointer to the tree. I did find one bug in the
allocation routine, that once fixed (see below), worked with SME
for IOMMU on and off and worked with an SEV guest.
I understand the comment about using sev_active() in the dma-direct
code, maybe we can up with something later to address that.
Thanks,
Tom
diff --git a/lib/dma-direct.c b/lib/dma-direct.c
index 856e140..988a3d8 100644
--- a/lib/dma-direct.c
+++ b/lib/dma-direct.c
@@ -82,10 +82,12 @@ void *dma_direct_alloc(struct device *dev, size_t
size, dma_addr_t *dma_handle,
if (!page)
return NULL;
- *dma_handle = __phys_to_dma(dev, page_to_phys(page));
+ *dma_handle = phys_to_dma(dev, page_to_phys(page));
ret = page_address(page);
- if (sev_active())
+ if (sev_active()) {
+ *dma_handle = __phys_to_dma(dev, page_to_phys(page));
set_memory_decrypted((unsigned long)ret, 1 << page_order);
+ }
memset(ret, 0, size);
return ret;
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 11/13] dma-direct: handle the memory encryption bit in common code
On Mon, Mar 12, 2018 at 02:48:51PM -0500, Tom Lendacky wrote: > Ok, I found one issue that allows this to work when the IOMMU isn't > enabled (see below). Thanks, folded! > But the bigger issue is when the IOMMU is enabled. The IOMMU code uses > a common mapping routine to create the I/O page tables. This routine > assumes that all memory being mapped is encrypted and therefore sets the > encryption bit in the I/O page tables. With this patch, the call to > dma_alloc_direct() now returns un-encrypted memory which results in an > encryption mis-match. I think keeping dma_alloc_direct() as it was prior > to this patch is the way to go. It allows SME DMA allocations to remain > encrypted and avoids added complexity in the amd_iommu.c file. This > would mean that SEV would still have special DMA operations (so that the > alloc/free can change the memory to un-encrypted). > > What do you think? In terms of logic you are right. I still don't like keeping a just slightly tweaked version of dma_alloc_direct around just for this, it will be perpetually out of sync in terms of features and bug fixes. What do you think about this version that does the decision at runtime: http://git.infradead.org/users/hch/misc.git/commitdiff/b89f24dc856595dc7610d672bf077195ab0dabf4 The full tree is available here for testing: git://git.infradead.org/users/hch/misc.git dma-direct-x86 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 11/13] dma-direct: handle the memory encryption bit in common code
On 3/12/2018 1:29 PM, Tom Lendacky wrote:
> On 3/5/2018 11:46 AM, Christoph Hellwig wrote:
>> Give the basic phys_to_dma and dma_to_phys helpers a __-prefix and add
>> the memory encryption mask to the non-prefixed versions. Use the
>> __-prefixed versions directly instead of clearing the mask again in
>> various places.
>>
>> With that in place the generic dma-direct routines can be used to
>> allocate non-encrypted bounce buffers, and the x86 SEV case can use
>> the generic swiotlb ops.
>>
>> Signed-off-by: Christoph Hellwig
>
> So this patch results in my system failing to boot when SME is active.
> I'm investigating further to see why. I'll follow up with more details
> as I find them.
Ok, I found one issue that allows this to work when the IOMMU isn't
enabled (see below).
But the bigger issue is when the IOMMU is enabled. The IOMMU code uses
a common mapping routine to create the I/O page tables. This routine
assumes that all memory being mapped is encrypted and therefore sets the
encryption bit in the I/O page tables. With this patch, the call to
dma_alloc_direct() now returns un-encrypted memory which results in an
encryption mis-match. I think keeping dma_alloc_direct() as it was prior
to this patch is the way to go. It allows SME DMA allocations to remain
encrypted and avoids added complexity in the amd_iommu.c file. This
would mean that SEV would still have special DMA operations (so that the
alloc/free can change the memory to un-encrypted).
What do you think?
>
> Additionally, when running with SME (not SEV), this is forcing all DMA
> coherent allocations to be decrypted, when that isn't required with SME
> (as long as the device can perform 48-bit or greater DMA). So it may
> be worth looking at only doing the decrypted allocations for SEV.
>
> Thanks,
> Tom
>
>> ---
>> arch/arm/include/asm/dma-direct.h | 4 +-
>> arch/mips/cavium-octeon/dma-octeon.c | 10 +--
>> .../include/asm/mach-cavium-octeon/dma-coherence.h | 4 +-
>> .../include/asm/mach-loongson64/dma-coherence.h| 10 +--
>> arch/mips/loongson64/common/dma-swiotlb.c | 4 +-
>> arch/powerpc/include/asm/dma-direct.h | 4 +-
>> arch/x86/Kconfig | 2 +-
>> arch/x86/include/asm/dma-direct.h | 25 +---
>> arch/x86/mm/mem_encrypt.c | 73
>> +-
>> arch/x86/pci/sta2x11-fixup.c | 6 +-
>> include/linux/dma-direct.h | 21 ++-
>> lib/dma-direct.c | 21 +--
>> lib/swiotlb.c | 25 +++-
>> 13 files changed, 70 insertions(+), 139 deletions(-)
>>
< ... SNIP ... >
>> diff --git a/lib/dma-direct.c b/lib/dma-direct.c
>> index c9e8e21cb334..84f50b5982fc 100644
>> --- a/lib/dma-direct.c
>> +++ b/lib/dma-direct.c
>> @@ -9,6 +9,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #define DIRECT_MAPPING_ERROR0
>>
>> @@ -35,9 +36,13 @@ check_addr(struct device *dev, dma_addr_t dma_addr,
>> size_t size,
>> return true;
>> }
>>
>> +/*
>> + * Since we will be clearing the encryption bit, check the mask with it
>> already
>> + * cleared.
>> + */
>> static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t
>> size)
>> {
>> -return phys_to_dma(dev, phys) + size - 1 <= dev->coherent_dma_mask;
>> +return __phys_to_dma(dev, phys) + size - 1 <= dev->coherent_dma_mask;
>> }
>>
>> void *dma_direct_alloc(struct device *dev, size_t size, dma_addr_t
>> *dma_handle,
>> @@ -46,6 +51,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
>> dma_addr_t *dma_handle,
>> unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> int page_order = get_order(size);
>> struct page *page = NULL;
>> +void *ret;
>>
>> /* GFP_DMA32 and GFP_DMA are no ops without the corresponding zones: */
>> if (dev->coherent_dma_mask <= DMA_BIT_MASK(ARCH_ZONE_DMA_BITS))
>> @@ -78,10 +84,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
>> dma_addr_t *dma_handle,
>>
>> if (!page)
>> return NULL;
>> -
>> -*dma_handle = phys_to_dma(dev, page_to_phys(page));
>> -memset(page_address(page), 0, size);
>> -return page_address(page);
>> +*dma_handle = __phys_to_dma(dev, page_to_phys(page));
>> +ret = page_address(page);
>> +set_memory_decrypted((unsigned long)ret, page_order);
The second parameter should be 1 << page_order to get the number of
pages.
Thanks,
Tom
>> +memset(ret, 0, size);
>> +return ret;
>> }
>>
>> /*
>> @@ -92,9 +99,11 @@ void dma_direct_free(struct device *dev, size_t size,
>> void *cpu_addr,
>> dma_addr_t dma_addr, unsigned long attrs)
>> {
>> unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> +unsigned int page_order = get_order(size);
>>
>> +set_memory_encrypted((unsigned
Re: [PATCH 11/13] dma-direct: handle the memory encryption bit in common code
On 3/5/2018 11:46 AM, Christoph Hellwig wrote:
> Give the basic phys_to_dma and dma_to_phys helpers a __-prefix and add
> the memory encryption mask to the non-prefixed versions. Use the
> __-prefixed versions directly instead of clearing the mask again in
> various places.
>
> With that in place the generic dma-direct routines can be used to
> allocate non-encrypted bounce buffers, and the x86 SEV case can use
> the generic swiotlb ops.
>
> Signed-off-by: Christoph Hellwig
So this patch results in my system failing to boot when SME is active.
I'm investigating further to see why. I'll follow up with more details
as I find them.
Additionally, when running with SME (not SEV), this is forcing all DMA
coherent allocations to be decrypted, when that isn't required with SME
(as long as the device can perform 48-bit or greater DMA). So it may
be worth looking at only doing the decrypted allocations for SEV.
Thanks,
Tom
> ---
> arch/arm/include/asm/dma-direct.h | 4 +-
> arch/mips/cavium-octeon/dma-octeon.c | 10 +--
> .../include/asm/mach-cavium-octeon/dma-coherence.h | 4 +-
> .../include/asm/mach-loongson64/dma-coherence.h| 10 +--
> arch/mips/loongson64/common/dma-swiotlb.c | 4 +-
> arch/powerpc/include/asm/dma-direct.h | 4 +-
> arch/x86/Kconfig | 2 +-
> arch/x86/include/asm/dma-direct.h | 25 +---
> arch/x86/mm/mem_encrypt.c | 73
> +-
> arch/x86/pci/sta2x11-fixup.c | 6 +-
> include/linux/dma-direct.h | 21 ++-
> lib/dma-direct.c | 21 +--
> lib/swiotlb.c | 25 +++-
> 13 files changed, 70 insertions(+), 139 deletions(-)
>
> diff --git a/arch/arm/include/asm/dma-direct.h
> b/arch/arm/include/asm/dma-direct.h
> index 5b0a8a421894..b67e5fc1fe43 100644
> --- a/arch/arm/include/asm/dma-direct.h
> +++ b/arch/arm/include/asm/dma-direct.h
> @@ -2,13 +2,13 @@
> #ifndef ASM_ARM_DMA_DIRECT_H
> #define ASM_ARM_DMA_DIRECT_H 1
>
> -static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
> +static inline dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
> {
> unsigned int offset = paddr & ~PAGE_MASK;
> return pfn_to_dma(dev, __phys_to_pfn(paddr)) + offset;
> }
>
> -static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t
> dev_addr)
> +static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t
> dev_addr)
> {
> unsigned int offset = dev_addr & ~PAGE_MASK;
> return __pfn_to_phys(dma_to_pfn(dev, dev_addr)) + offset;
> diff --git a/arch/mips/cavium-octeon/dma-octeon.c
> b/arch/mips/cavium-octeon/dma-octeon.c
> index c7bb8a407041..7b335ab21697 100644
> --- a/arch/mips/cavium-octeon/dma-octeon.c
> +++ b/arch/mips/cavium-octeon/dma-octeon.c
> @@ -10,7 +10,7 @@
> * IP32 changes by Ilya.
> * Copyright (C) 2010 Cavium Networks, Inc.
> */
> -#include
> +#include
> #include
> #include
> #include
> @@ -182,7 +182,7 @@ struct octeon_dma_map_ops {
> phys_addr_t (*dma_to_phys)(struct device *dev, dma_addr_t daddr);
> };
>
> -dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
> +dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
> {
> struct octeon_dma_map_ops *ops = container_of(get_dma_ops(dev),
> struct octeon_dma_map_ops,
> @@ -190,9 +190,9 @@ dma_addr_t phys_to_dma(struct device *dev, phys_addr_t
> paddr)
>
> return ops->phys_to_dma(dev, paddr);
> }
> -EXPORT_SYMBOL(phys_to_dma);
> +EXPORT_SYMBOL(__phys_to_dma);
>
> -phys_addr_t dma_to_phys(struct device *dev, dma_addr_t daddr)
> +phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t daddr)
> {
> struct octeon_dma_map_ops *ops = container_of(get_dma_ops(dev),
> struct octeon_dma_map_ops,
> @@ -200,7 +200,7 @@ phys_addr_t dma_to_phys(struct device *dev, dma_addr_t
> daddr)
>
> return ops->dma_to_phys(dev, daddr);
> }
> -EXPORT_SYMBOL(dma_to_phys);
> +EXPORT_SYMBOL(__dma_to_phys);
>
> static struct octeon_dma_map_ops octeon_linear_dma_map_ops = {
> .dma_map_ops = {
> diff --git a/arch/mips/include/asm/mach-cavium-octeon/dma-coherence.h
> b/arch/mips/include/asm/mach-cavium-octeon/dma-coherence.h
> index 138edf6b5b48..6eb1ee548b11 100644
> --- a/arch/mips/include/asm/mach-cavium-octeon/dma-coherence.h
> +++ b/arch/mips/include/asm/mach-cavium-octeon/dma-coherence.h
> @@ -69,8 +69,8 @@ static inline bool dma_capable(struct device *dev,
> dma_addr_t addr, size_t size)
> return addr + size - 1 <= *dev->dma_mask;
> }
>
> -dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr);
> -phys_addr_t dma_to_phys(struct device *dev, dma_addr_t daddr);
> +dma_addr_t __phys_to_dma(struct devic
[PATCH 11/13] dma-direct: handle the memory encryption bit in common code
Give the basic phys_to_dma and dma_to_phys helpers a __-prefix and add
the memory encryption mask to the non-prefixed versions. Use the
__-prefixed versions directly instead of clearing the mask again in
various places.
With that in place the generic dma-direct routines can be used to
allocate non-encrypted bounce buffers, and the x86 SEV case can use
the generic swiotlb ops.
Signed-off-by: Christoph Hellwig
---
arch/arm/include/asm/dma-direct.h | 4 +-
arch/mips/cavium-octeon/dma-octeon.c | 10 +--
.../include/asm/mach-cavium-octeon/dma-coherence.h | 4 +-
.../include/asm/mach-loongson64/dma-coherence.h| 10 +--
arch/mips/loongson64/common/dma-swiotlb.c | 4 +-
arch/powerpc/include/asm/dma-direct.h | 4 +-
arch/x86/Kconfig | 2 +-
arch/x86/include/asm/dma-direct.h | 25 +---
arch/x86/mm/mem_encrypt.c | 73 +-
arch/x86/pci/sta2x11-fixup.c | 6 +-
include/linux/dma-direct.h | 21 ++-
lib/dma-direct.c | 21 +--
lib/swiotlb.c | 25 +++-
13 files changed, 70 insertions(+), 139 deletions(-)
diff --git a/arch/arm/include/asm/dma-direct.h
b/arch/arm/include/asm/dma-direct.h
index 5b0a8a421894..b67e5fc1fe43 100644
--- a/arch/arm/include/asm/dma-direct.h
+++ b/arch/arm/include/asm/dma-direct.h
@@ -2,13 +2,13 @@
#ifndef ASM_ARM_DMA_DIRECT_H
#define ASM_ARM_DMA_DIRECT_H 1
-static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
+static inline dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
{
unsigned int offset = paddr & ~PAGE_MASK;
return pfn_to_dma(dev, __phys_to_pfn(paddr)) + offset;
}
-static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t dev_addr)
+static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t
dev_addr)
{
unsigned int offset = dev_addr & ~PAGE_MASK;
return __pfn_to_phys(dma_to_pfn(dev, dev_addr)) + offset;
diff --git a/arch/mips/cavium-octeon/dma-octeon.c
b/arch/mips/cavium-octeon/dma-octeon.c
index c7bb8a407041..7b335ab21697 100644
--- a/arch/mips/cavium-octeon/dma-octeon.c
+++ b/arch/mips/cavium-octeon/dma-octeon.c
@@ -10,7 +10,7 @@
* IP32 changes by Ilya.
* Copyright (C) 2010 Cavium Networks, Inc.
*/
-#include
+#include
#include
#include
#include
@@ -182,7 +182,7 @@ struct octeon_dma_map_ops {
phys_addr_t (*dma_to_phys)(struct device *dev, dma_addr_t daddr);
};
-dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
+dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
{
struct octeon_dma_map_ops *ops = container_of(get_dma_ops(dev),
struct octeon_dma_map_ops,
@@ -190,9 +190,9 @@ dma_addr_t phys_to_dma(struct device *dev, phys_addr_t
paddr)
return ops->phys_to_dma(dev, paddr);
}
-EXPORT_SYMBOL(phys_to_dma);
+EXPORT_SYMBOL(__phys_to_dma);
-phys_addr_t dma_to_phys(struct device *dev, dma_addr_t daddr)
+phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t daddr)
{
struct octeon_dma_map_ops *ops = container_of(get_dma_ops(dev),
struct octeon_dma_map_ops,
@@ -200,7 +200,7 @@ phys_addr_t dma_to_phys(struct device *dev, dma_addr_t
daddr)
return ops->dma_to_phys(dev, daddr);
}
-EXPORT_SYMBOL(dma_to_phys);
+EXPORT_SYMBOL(__dma_to_phys);
static struct octeon_dma_map_ops octeon_linear_dma_map_ops = {
.dma_map_ops = {
diff --git a/arch/mips/include/asm/mach-cavium-octeon/dma-coherence.h
b/arch/mips/include/asm/mach-cavium-octeon/dma-coherence.h
index 138edf6b5b48..6eb1ee548b11 100644
--- a/arch/mips/include/asm/mach-cavium-octeon/dma-coherence.h
+++ b/arch/mips/include/asm/mach-cavium-octeon/dma-coherence.h
@@ -69,8 +69,8 @@ static inline bool dma_capable(struct device *dev, dma_addr_t
addr, size_t size)
return addr + size - 1 <= *dev->dma_mask;
}
-dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr);
-phys_addr_t dma_to_phys(struct device *dev, dma_addr_t daddr);
+dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr);
+phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t daddr);
struct dma_map_ops;
extern const struct dma_map_ops *octeon_pci_dma_map_ops;
diff --git a/arch/mips/include/asm/mach-loongson64/dma-coherence.h
b/arch/mips/include/asm/mach-loongson64/dma-coherence.h
index b1b575f5c6c1..64fc44dec0a8 100644
--- a/arch/mips/include/asm/mach-loongson64/dma-coherence.h
+++ b/arch/mips/include/asm/mach-loongson64/dma-coherence.h
@@ -25,13 +25,13 @@ static inline bool dma_capable(struct device *dev,
dma_addr_t addr, size_t size)
return addr + size - 1 <= *dev->dma_mask;
}
-extern dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr);
-extern
