Re: [rfc] dma-mapping: preallocate unencrypted DMA atomic pool
On Fri, 17 Jan 2020, Tom Lendacky wrote:
> On 12/31/19 7:54 PM, David Rientjes wrote:
> > Christoph, Thomas, is something like this (without the diagnosic
> > information included in this patch) acceptable for these allocations?
> > Adding expansion support when the pool is half depleted wouldn't be *that*
> > hard.
>
> Sorry for the delay in responding... Overall, I think this will work
> well. If you want the default size to be adjustable, you can always go
> with a Kconfig setting or a command line parameter or both (I realize the
> command line parameter is not optimal).
>
Ok, so we've determined that we don't only have a dependency on GFP_DMA
memory through the DMA API in a non-blocking context that needs to be
unencrypted, but also GFP_KERNEL. We don't have a dependency on GFP_DMA32
memory (yet) but should likely provision for it.
So my patch would change by actually providing three separate pools, one
for ZONE_DMA, one for ZONE_DMA32, and one for ZONE_NORMAL. The ZONE_DMA
already exists in the form of the atomic_pool, so it would add two
additional pools that likely start out at the same size and dynamically
expand with a kworker when its usage approaches the limitatios of the
pool. I don't necessarily like needing three separate pools, but I can't
think of a better way to provide unencrypted memory for non-blocking
allocations that work for all possible device gfp masks.
My idea right now is to create all three pools instead of the single
atomic_pool, all 256KB in size, and anytime their usage reaches half their
limit, we kick off some background work to double the size of the pool
with GFP_KERNEL | __GFP_NORETRY. Our experimentation suggests that a
kworker would actually respond in time for this.
Any objections to this approach? If so, an alternative suggestion would
be appreciated :) I plan on all atomic pools to be unencrypted at the
time the allocation is successful unless there is some strange need for
non-blocking atomic allocations through the DMA API that should *not* be
encrypted.
> Just a couple of overall comments about the use of variable names and
> messages using both unencrypted and encrypted, I think the use should be
> consistent throughout the patch.
>
> Thanks,
> Tom
>
> >
> > Or are there alternatives we should consider? Thanks!
> >
> >
> >
> >
> > When AMD SEV is enabled in the guest, all allocations through
> > dma_pool_alloc_page() must call set_memory_decrypted() for unencrypted
> > DMA. This includes dma_pool_alloc() and dma_direct_alloc_pages(). These
> > calls may block which is not allowed in atomic allocation contexts such as
> > from the NVMe driver.
> >
> > Preallocate a complementary unecrypted DMA atomic pool that is initially
> > 4MB in size. This patch does not contain dynamic expansion, but that
> > could be added if necessary.
> >
> > In our stress testing, our peak unecrypted DMA atomic allocation
> > requirements is ~1.4MB, so 4MB is plenty. This pool is similar to the
> > existing DMA atomic pool but is unencrypted.
> >
> > Signed-off-by: David Rientjes
> > ---
> > Based on v5.4 HEAD.
> >
> > This commit contains diagnostic information and is not intended for use
> > in a production environment.
> >
> > arch/x86/Kconfig| 1 +
> > drivers/iommu/dma-iommu.c | 5 +-
> > include/linux/dma-mapping.h | 7 ++-
> > kernel/dma/direct.c | 16 -
> > kernel/dma/remap.c | 116 ++--
> > 5 files changed, 108 insertions(+), 37 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1530,6 +1530,7 @@ config X86_CPA_STATISTICS
> > config AMD_MEM_ENCRYPT
> > bool "AMD Secure Memory Encryption (SME) support"
> > depends on X86_64 && CPU_SUP_AMD
> > + select DMA_DIRECT_REMAP
> > select DYNAMIC_PHYSICAL_MASK
> > select ARCH_USE_MEMREMAP_PROT
> > select ARCH_HAS_FORCE_DMA_UNENCRYPTED
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -928,7 +928,7 @@ static void __iommu_dma_free(struct device *dev, size_t
> > size, void *cpu_addr)
> >
> > /* Non-coherent atomic allocation? Easy */
> > if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
> > - dma_free_from_pool(cpu_addr, alloc_size))
> > + dma_free_from_pool(dev, cpu_addr, alloc_size))
> > return;
> >
> > if (IS_ENABLED(CONFIG_DMA_REMAP) && is_vmalloc_addr(cpu_addr)) {
> > @@ -1011,7 +1011,8 @@ static void *iommu_dma_alloc(struct device *dev,
> > size_t size,
> >
> > if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
> > !gfpflags_allow_blocking(gfp) && !coherent)
> > - cpu_addr = dma_alloc_from_pool(PAGE_ALIGN(size), , gfp);
> > + cpu_addr = dma_alloc_from_pool(dev, PAGE_ALIGN(size), ,
> > + gfp);
Re: [rfc] dma-mapping: preallocate unencrypted DMA atomic pool
On 12/31/19 7:54 PM, David Rientjes wrote:
> Christoph, Thomas, is something like this (without the diagnosic
> information included in this patch) acceptable for these allocations?
> Adding expansion support when the pool is half depleted wouldn't be *that*
> hard.
Sorry for the delay in responding... Overall, I think this will work
well. If you want the default size to be adjustable, you can always go
with a Kconfig setting or a command line parameter or both (I realize the
command line parameter is not optimal).
Just a couple of overall comments about the use of variable names and
messages using both unencrypted and encrypted, I think the use should be
consistent throughout the patch.
Thanks,
Tom
>
> Or are there alternatives we should consider? Thanks!
>
>
>
>
> When AMD SEV is enabled in the guest, all allocations through
> dma_pool_alloc_page() must call set_memory_decrypted() for unencrypted
> DMA. This includes dma_pool_alloc() and dma_direct_alloc_pages(). These
> calls may block which is not allowed in atomic allocation contexts such as
> from the NVMe driver.
>
> Preallocate a complementary unecrypted DMA atomic pool that is initially
> 4MB in size. This patch does not contain dynamic expansion, but that
> could be added if necessary.
>
> In our stress testing, our peak unecrypted DMA atomic allocation
> requirements is ~1.4MB, so 4MB is plenty. This pool is similar to the
> existing DMA atomic pool but is unencrypted.
>
> Signed-off-by: David Rientjes
> ---
> Based on v5.4 HEAD.
>
> This commit contains diagnostic information and is not intended for use
> in a production environment.
>
> arch/x86/Kconfig| 1 +
> drivers/iommu/dma-iommu.c | 5 +-
> include/linux/dma-mapping.h | 7 ++-
> kernel/dma/direct.c | 16 -
> kernel/dma/remap.c | 116 ++--
> 5 files changed, 108 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1530,6 +1530,7 @@ config X86_CPA_STATISTICS
> config AMD_MEM_ENCRYPT
> bool "AMD Secure Memory Encryption (SME) support"
> depends on X86_64 && CPU_SUP_AMD
> + select DMA_DIRECT_REMAP
> select DYNAMIC_PHYSICAL_MASK
> select ARCH_USE_MEMREMAP_PROT
> select ARCH_HAS_FORCE_DMA_UNENCRYPTED
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -928,7 +928,7 @@ static void __iommu_dma_free(struct device *dev, size_t
> size, void *cpu_addr)
>
> /* Non-coherent atomic allocation? Easy */
> if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
> - dma_free_from_pool(cpu_addr, alloc_size))
> + dma_free_from_pool(dev, cpu_addr, alloc_size))
> return;
>
> if (IS_ENABLED(CONFIG_DMA_REMAP) && is_vmalloc_addr(cpu_addr)) {
> @@ -1011,7 +1011,8 @@ static void *iommu_dma_alloc(struct device *dev, size_t
> size,
>
> if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
> !gfpflags_allow_blocking(gfp) && !coherent)
> - cpu_addr = dma_alloc_from_pool(PAGE_ALIGN(size), , gfp);
> + cpu_addr = dma_alloc_from_pool(dev, PAGE_ALIGN(size), ,
> +gfp);
> else
> cpu_addr = iommu_dma_alloc_pages(dev, size, , gfp, attrs);
> if (!cpu_addr)
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -629,9 +629,10 @@ void *dma_common_pages_remap(struct page **pages, size_t
> size,
> pgprot_t prot, const void *caller);
> void dma_common_free_remap(void *cpu_addr, size_t size);
>
> -bool dma_in_atomic_pool(void *start, size_t size);
> -void *dma_alloc_from_pool(size_t size, struct page **ret_page, gfp_t flags);
> -bool dma_free_from_pool(void *start, size_t size);
> +bool dma_in_atomic_pool(struct device *dev, void *start, size_t size);
> +void *dma_alloc_from_pool(struct device *dev, size_t size,
> + struct page **ret_page, gfp_t flags);
> +bool dma_free_from_pool(struct device *dev, void *start, size_t size);
>
> int
> dma_common_get_sgtable(struct device *dev, struct sg_table *sgt, void
> *cpu_addr,
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -131,6 +132,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t
> size,
> struct page *page;
> void *ret;
>
> + if (!gfpflags_allow_blocking(gfp) && force_dma_unencrypted(dev)) {
> + ret = dma_alloc_from_pool(dev, size, , gfp);
> + if (!ret)
> + return NULL;
> + goto done;
> + }
> +
> page =
Re: [rfc] dma-mapping: preallocate unencrypted DMA atomic pool
On Thu, 9 Jan 2020, Christoph Hellwig wrote: > > I'll rely on Thomas to chime in if this doesn't make sense for the SEV > > usecase. > > > > I think the sizing of the single atomic pool needs to be determined. Our > > peak usage that we have measured from NVMe is ~1.4MB and atomic_pool is > > currently sized to 256KB by default. I'm unsure at this time if we need > > to be able to dynamically expand this pool with a kworker. > > > > Maybe when CONFIG_AMD_MEM_ENCRYPT is enabled this atomic pool should be > > sized to 2MB or so and then when it reaches half capacity we schedule some > > background work to dynamically increase it? That wouldn't be hard unless > > the pool can be rapidly depleted. > > > > Note that a non-coherent architecture with the same workload would need > the same size. > > > Do we want to increase the atomic pool size by default and then do > > background dynamic expansion? > > For now I'd just scale with system memory size. > Thanks Christoph and Robin for the help, we're running some additional stress tests to double check that our required amount of memory from this pool is accurate. Once that's done, I'll refresh the patch with th suggestions and propose it formally. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [rfc] dma-mapping: preallocate unencrypted DMA atomic pool
On Tue, Jan 07, 2020 at 11:57:24AM -0800, David Rientjes wrote: > I'll rely on Thomas to chime in if this doesn't make sense for the SEV > usecase. > > I think the sizing of the single atomic pool needs to be determined. Our > peak usage that we have measured from NVMe is ~1.4MB and atomic_pool is > currently sized to 256KB by default. I'm unsure at this time if we need > to be able to dynamically expand this pool with a kworker. > > Maybe when CONFIG_AMD_MEM_ENCRYPT is enabled this atomic pool should be > sized to 2MB or so and then when it reaches half capacity we schedule some > background work to dynamically increase it? That wouldn't be hard unless > the pool can be rapidly depleted. > Note that a non-coherent architecture with the same workload would need the same size. > Do we want to increase the atomic pool size by default and then do > background dynamic expansion? For now I'd just scale with system memory size. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [rfc] dma-mapping: preallocate unencrypted DMA atomic pool
On Tue, 7 Jan 2020, Christoph Hellwig wrote: > > On 01/01/2020 1:54 am, David Rientjes via iommu wrote: > >> Christoph, Thomas, is something like this (without the diagnosic > >> information included in this patch) acceptable for these allocations? > >> Adding expansion support when the pool is half depleted wouldn't be *that* > >> hard. > >> > >> Or are there alternatives we should consider? Thanks! > > > > Are there any platforms which require both non-cacheable remapping *and* > > unencrypted remapping for distinct subsets of devices? > > > > If not (and I'm assuming there aren't, because otherwise this patch is > > incomplete in covering only 2 of the 3 possible combinations), then > > couldn't we keep things simpler by just attributing both properties to the > > single "atomic pool" on the basis that one or the other will always be a > > no-op? In other words, basically just tweaking the existing "!coherent" > > tests to "!coherent || force_dma_unencrypted()" and doing > > set_dma_unencrypted() unconditionally in atomic_pool_init(). > > I think that would make most sense. > I'll rely on Thomas to chime in if this doesn't make sense for the SEV usecase. I think the sizing of the single atomic pool needs to be determined. Our peak usage that we have measured from NVMe is ~1.4MB and atomic_pool is currently sized to 256KB by default. I'm unsure at this time if we need to be able to dynamically expand this pool with a kworker. Maybe when CONFIG_AMD_MEM_ENCRYPT is enabled this atomic pool should be sized to 2MB or so and then when it reaches half capacity we schedule some background work to dynamically increase it? That wouldn't be hard unless the pool can be rapidly depleted. Do we want to increase the atomic pool size by default and then do background dynamic expansion? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [rfc] dma-mapping: preallocate unencrypted DMA atomic pool
On Mon, Jan 06, 2020 at 05:34:00PM +, Robin Murphy wrote: > On 01/01/2020 1:54 am, David Rientjes via iommu wrote: >> Christoph, Thomas, is something like this (without the diagnosic >> information included in this patch) acceptable for these allocations? >> Adding expansion support when the pool is half depleted wouldn't be *that* >> hard. >> >> Or are there alternatives we should consider? Thanks! > > Are there any platforms which require both non-cacheable remapping *and* > unencrypted remapping for distinct subsets of devices? > > If not (and I'm assuming there aren't, because otherwise this patch is > incomplete in covering only 2 of the 3 possible combinations), then > couldn't we keep things simpler by just attributing both properties to the > single "atomic pool" on the basis that one or the other will always be a > no-op? In other words, basically just tweaking the existing "!coherent" > tests to "!coherent || force_dma_unencrypted()" and doing > set_dma_unencrypted() unconditionally in atomic_pool_init(). I think that would make most sense. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [rfc] dma-mapping: preallocate unencrypted DMA atomic pool
On 01/01/2020 1:54 am, David Rientjes via iommu wrote:
Christoph, Thomas, is something like this (without the diagnosic
information included in this patch) acceptable for these allocations?
Adding expansion support when the pool is half depleted wouldn't be *that*
hard.
Or are there alternatives we should consider? Thanks!
Are there any platforms which require both non-cacheable remapping *and*
unencrypted remapping for distinct subsets of devices?
If not (and I'm assuming there aren't, because otherwise this patch is
incomplete in covering only 2 of the 3 possible combinations), then
couldn't we keep things simpler by just attributing both properties to
the single "atomic pool" on the basis that one or the other will always
be a no-op? In other words, basically just tweaking the existing
"!coherent" tests to "!coherent || force_dma_unencrypted()" and doing
set_dma_unencrypted() unconditionally in atomic_pool_init().
Robin.
When AMD SEV is enabled in the guest, all allocations through
dma_pool_alloc_page() must call set_memory_decrypted() for unencrypted
DMA. This includes dma_pool_alloc() and dma_direct_alloc_pages(). These
calls may block which is not allowed in atomic allocation contexts such as
from the NVMe driver.
Preallocate a complementary unecrypted DMA atomic pool that is initially
4MB in size. This patch does not contain dynamic expansion, but that
could be added if necessary.
In our stress testing, our peak unecrypted DMA atomic allocation
requirements is ~1.4MB, so 4MB is plenty. This pool is similar to the
existing DMA atomic pool but is unencrypted.
Signed-off-by: David Rientjes
---
Based on v5.4 HEAD.
This commit contains diagnostic information and is not intended for use
in a production environment.
arch/x86/Kconfig| 1 +
drivers/iommu/dma-iommu.c | 5 +-
include/linux/dma-mapping.h | 7 ++-
kernel/dma/direct.c | 16 -
kernel/dma/remap.c | 116 ++--
5 files changed, 108 insertions(+), 37 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1530,6 +1530,7 @@ config X86_CPA_STATISTICS
config AMD_MEM_ENCRYPT
bool "AMD Secure Memory Encryption (SME) support"
depends on X86_64 && CPU_SUP_AMD
+ select DMA_DIRECT_REMAP
select DYNAMIC_PHYSICAL_MASK
select ARCH_USE_MEMREMAP_PROT
select ARCH_HAS_FORCE_DMA_UNENCRYPTED
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -928,7 +928,7 @@ static void __iommu_dma_free(struct device *dev, size_t
size, void *cpu_addr)
/* Non-coherent atomic allocation? Easy */
if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
- dma_free_from_pool(cpu_addr, alloc_size))
+ dma_free_from_pool(dev, cpu_addr, alloc_size))
return;
if (IS_ENABLED(CONFIG_DMA_REMAP) && is_vmalloc_addr(cpu_addr)) {
@@ -1011,7 +1011,8 @@ static void *iommu_dma_alloc(struct device *dev, size_t
size,
if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
!gfpflags_allow_blocking(gfp) && !coherent)
- cpu_addr = dma_alloc_from_pool(PAGE_ALIGN(size), , gfp);
+ cpu_addr = dma_alloc_from_pool(dev, PAGE_ALIGN(size), ,
+ gfp);
else
cpu_addr = iommu_dma_alloc_pages(dev, size, , gfp, attrs);
if (!cpu_addr)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -629,9 +629,10 @@ void *dma_common_pages_remap(struct page **pages, size_t
size,
pgprot_t prot, const void *caller);
void dma_common_free_remap(void *cpu_addr, size_t size);
-bool dma_in_atomic_pool(void *start, size_t size);
-void *dma_alloc_from_pool(size_t size, struct page **ret_page, gfp_t flags);
-bool dma_free_from_pool(void *start, size_t size);
+bool dma_in_atomic_pool(struct device *dev, void *start, size_t size);
+void *dma_alloc_from_pool(struct device *dev, size_t size,
+ struct page **ret_page, gfp_t flags);
+bool dma_free_from_pool(struct device *dev, void *start, size_t size);
int
dma_common_get_sgtable(struct device *dev, struct sg_table *sgt, void
*cpu_addr,
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -131,6 +132,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t
size,
struct page *page;
void *ret;
+ if (!gfpflags_allow_blocking(gfp) && force_dma_unencrypted(dev)) {
+ ret = dma_alloc_from_pool(dev, size, , gfp);
+ if (!ret)
+ return NULL;
+ goto done;
+ }
+
