Re: [RFC PATCH v2 15/20] iommu/amd: AMD IOMMU support for memory encryption

[Borislav Petkov]

On Thu, Sep 15, 2016 at 11:57:41AM -0500, Tom Lendacky wrote:
> If I do that, then I could put an #ifdef in the header to include the
> asm/mem_encrypt.h if the memory encryption is configured, else set the
> value to zero.

Yeah, something along those lines...

> I'll look into this. One immediate question becomes do we keep the
> name very specific vs. making it more generic, sme_me_mask vs me_mask,
> etc.

No strong opinion either way from me. My angle is that whatever it is,
we can always rename it later if we decide to.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v2 15/20] iommu/amd: AMD IOMMU support for memory encryption

[Tom Lendacky]

On 09/14/2016 09:41 AM, Borislav Petkov wrote:
> On Wed, Sep 14, 2016 at 08:45:44AM -0500, Tom Lendacky wrote:
>> Currently, mem_encrypt.h only lives in the arch/x86 directory so it
>> wouldn't be able to be included here without breaking other archs.
> 
> I'm wondering if it would be simpler to move only sme_me_mask to an
> arch-agnostic header just so that we save us all the code duplication.
> 
> Hmmm.

If I do that, then I could put an #ifdef in the header to include the
asm/mem_encrypt.h if the memory encryption is configured, else set the
value to zero.  I'll look into this.  One immediate question becomes do
we keep the name very specific vs. making it more generic, sme_me_mask
vs me_mask, etc.

Thanks,
Tom

> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v2 15/20] iommu/amd: AMD IOMMU support for memory encryption

[Tom Lendacky]

On 09/12/2016 06:45 AM, Borislav Petkov wrote:
> On Mon, Aug 22, 2016 at 05:38:20PM -0500, Tom Lendacky wrote:
>> Add support to the AMD IOMMU driver to set the memory encryption mask if
>> memory encryption is enabled.
>>
>> Signed-off-by: Tom Lendacky 
>> ---
>>  arch/x86/include/asm/mem_encrypt.h |2 ++
>>  arch/x86/mm/mem_encrypt.c  |5 +
>>  drivers/iommu/amd_iommu.c  |   10 ++
>>  3 files changed, 17 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/mem_encrypt.h 
>> b/arch/x86/include/asm/mem_encrypt.h
>> index 384fdfb..e395729 100644
>> --- a/arch/x86/include/asm/mem_encrypt.h
>> +++ b/arch/x86/include/asm/mem_encrypt.h
>> @@ -36,6 +36,8 @@ void __init sme_early_init(void);
>>  /* Architecture __weak replacement functions */
>>  void __init mem_encrypt_init(void);
>>  
>> +unsigned long amd_iommu_get_me_mask(void);
>> +
>>  unsigned long swiotlb_get_me_mask(void);
>>  void swiotlb_set_mem_dec(void *vaddr, unsigned long size);
>>  
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 6b2e8bf..2f28d87 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -185,6 +185,11 @@ void __init mem_encrypt_init(void)
>>  swiotlb_clear_encryption();
>>  }
>>  
>> +unsigned long amd_iommu_get_me_mask(void)
>> +{
>> +return sme_me_mask;
>> +}
>> +
>>  unsigned long swiotlb_get_me_mask(void)
>>  {
>>  return sme_me_mask;
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 96de97a..63995e3 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -166,6 +166,15 @@ struct dma_ops_domain {
>>  static struct iova_domain reserved_iova_ranges;
>>  static struct lock_class_key reserved_rbtree_key;
>>  
>> +/*
>> + * Support for memory encryption. If memory encryption is supported, then an
>> + * override to this function will be provided.
>> + */
>> +unsigned long __weak amd_iommu_get_me_mask(void)
>> +{
>> +return 0;
>> +}
> 
> So instead of adding a function each time which returns sme_me_mask
> for each user it has, why don't you add a single function which
> returns sme_me_mask in mem_encrypt.c and add an inline in the header
> mem_encrypt.h which returns 0 for the !CONFIG_AMD_MEM_ENCRYPT case.

Currently, mem_encrypt.h only lives in the arch/x86 directory so it
wouldn't be able to be included here without breaking other archs.

> 
> This all is still funny because we access sme_me_mask directly for the
> different KERNEL_* masks but then you're adding an accessor function.

Because this lives outside of the arch/x86 I need to use the weak
function.

> 
> So what you should do instead, IMHO, is either hide sme_me_mask
> altogether and use the accessor functions only (not sure if that would
> work in all cases) or expose sme_me_mask unconditionally and have it be
> 0 if CONFIG_AMD_MEM_ENCRYPT is not enabled so that it just works.
> 
> Or is there a third, more graceful variant?

Is there a better way to do this given the support is only in x86?

Thanks,
Tom

> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH v2 15/20] iommu/amd: AMD IOMMU support for memory encryption

[Borislav Petkov]

On Mon, Aug 22, 2016 at 05:38:20PM -0500, Tom Lendacky wrote:
> Add support to the AMD IOMMU driver to set the memory encryption mask if
> memory encryption is enabled.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/include/asm/mem_encrypt.h |2 ++
>  arch/x86/mm/mem_encrypt.c  |5 +
>  drivers/iommu/amd_iommu.c  |   10 ++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/x86/include/asm/mem_encrypt.h 
> b/arch/x86/include/asm/mem_encrypt.h
> index 384fdfb..e395729 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -36,6 +36,8 @@ void __init sme_early_init(void);
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void);
>  
> +unsigned long amd_iommu_get_me_mask(void);
> +
>  unsigned long swiotlb_get_me_mask(void);
>  void swiotlb_set_mem_dec(void *vaddr, unsigned long size);
>  
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 6b2e8bf..2f28d87 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -185,6 +185,11 @@ void __init mem_encrypt_init(void)
>   swiotlb_clear_encryption();
>  }
>  
> +unsigned long amd_iommu_get_me_mask(void)
> +{
> + return sme_me_mask;
> +}
> +
>  unsigned long swiotlb_get_me_mask(void)
>  {
>   return sme_me_mask;
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 96de97a..63995e3 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -166,6 +166,15 @@ struct dma_ops_domain {
>  static struct iova_domain reserved_iova_ranges;
>  static struct lock_class_key reserved_rbtree_key;
>  
> +/*
> + * Support for memory encryption. If memory encryption is supported, then an
> + * override to this function will be provided.
> + */
> +unsigned long __weak amd_iommu_get_me_mask(void)
> +{
> + return 0;
> +}

So instead of adding a function each time which returns sme_me_mask
for each user it has, why don't you add a single function which
returns sme_me_mask in mem_encrypt.c and add an inline in the header
mem_encrypt.h which returns 0 for the !CONFIG_AMD_MEM_ENCRYPT case.

This all is still funny because we access sme_me_mask directly for the
different KERNEL_* masks but then you're adding an accessor function.

So what you should do instead, IMHO, is either hide sme_me_mask
altogether and use the accessor functions only (not sure if that would
work in all cases) or expose sme_me_mask unconditionally and have it be
0 if CONFIG_AMD_MEM_ENCRYPT is not enabled so that it just works.

Or is there a third, more graceful variant?

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH v2 15/20] iommu/amd: AMD IOMMU support for memory encryption

[Tom Lendacky]

Add support to the AMD IOMMU driver to set the memory encryption mask if
memory encryption is enabled.

Signed-off-by: Tom Lendacky 
---
 arch/x86/include/asm/mem_encrypt.h |2 ++
 arch/x86/mm/mem_encrypt.c  |5 +
 drivers/iommu/amd_iommu.c  |   10 ++
 3 files changed, 17 insertions(+)

diff --git a/arch/x86/include/asm/mem_encrypt.h 
b/arch/x86/include/asm/mem_encrypt.h
index 384fdfb..e395729 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -36,6 +36,8 @@ void __init sme_early_init(void);
 /* Architecture __weak replacement functions */
 void __init mem_encrypt_init(void);
 
+unsigned long amd_iommu_get_me_mask(void);
+
 unsigned long swiotlb_get_me_mask(void);
 void swiotlb_set_mem_dec(void *vaddr, unsigned long size);
 
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 6b2e8bf..2f28d87 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -185,6 +185,11 @@ void __init mem_encrypt_init(void)
swiotlb_clear_encryption();
 }
 
+unsigned long amd_iommu_get_me_mask(void)
+{
+   return sme_me_mask;
+}
+
 unsigned long swiotlb_get_me_mask(void)
 {
return sme_me_mask;
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 96de97a..63995e3 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -166,6 +166,15 @@ struct dma_ops_domain {
 static struct iova_domain reserved_iova_ranges;
 static struct lock_class_key reserved_rbtree_key;
 
+/*
+ * Support for memory encryption. If memory encryption is supported, then an
+ * override to this function will be provided.
+ */
+unsigned long __weak amd_iommu_get_me_mask(void)
+{
+   return 0;
+}
+
 /
  *
  * Helper functions
@@ -2302,6 +2311,7 @@ static dma_addr_t __map_single(struct device *dev,
 
prot = dir2prot(direction);
 
+   paddr |= amd_iommu_get_me_mask();
start = address;
for (i = 0; i < pages; ++i) {
ret = iommu_map_page(_dom->domain, start, paddr,

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu