[PATCH v7 3/5] x86/mm: Make tdx_enc_status_changed() vmalloc address compatible

Kuppuswamy Sathyanarayanan posted 5 patches 3 years, 8 months ago
There is a newer version of this series
[PATCH v7 3/5] x86/mm: Make tdx_enc_status_changed() vmalloc address compatible
Posted by Kuppuswamy Sathyanarayanan 3 years, 8 months ago
set_memory_*crypted() APIs are used to change encryption or decryption
page attributes for the given address. It also by default support the
conversion for the vmalloc'ed memory address.

In TDX Guest, tdx_enc_status_changed() function is triggered by
set_memory_*crypted() APIs when converting memory from/to shared or
private. Internally this function uses __pa() for physical address
conversion, which breaks the vmalloc address compatibility of the
set_memory_*crypted() APIs.

So add support to fix the vmalloc'ed address compatibility issue.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/tdx/tdx.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index b49211994864..37d58675ccf1 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -15,6 +15,7 @@
 #include <asm/idtentry.h>
 #include <asm/irq_regs.h>
 #include <asm/desc.h>
+#include <asm/io.h>
 
 /* TDX module Call Leaf IDs */
 #define TDX_GET_INFO			1
@@ -680,8 +681,14 @@ static bool try_accept_one(phys_addr_t *start, unsigned long len,
  */
 static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
 {
-	phys_addr_t start = __pa(vaddr);
-	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
+	phys_addr_t start, end;
+
+	if (is_vmalloc_addr((void *)vaddr))
+		start = vmalloc_to_pfn((void *) vaddr) << PAGE_SHIFT;
+	else
+		start = __pa(vaddr);
+
+	end = start + numpages * PAGE_SIZE;
 
 	if (!enc) {
 		/* Set the shared (decrypted) bits: */
-- 
2.25.1
Re: [PATCH v7 3/5] x86/mm: Make tdx_enc_status_changed() vmalloc address compatible
Posted by Kai Huang 3 years, 8 months ago
On Mon, 2022-05-23 at 21:05 -0700, Kuppuswamy Sathyanarayanan wrote:
> set_memory_*crypted() APIs are used to change encryption or decryption
> page attributes for the given address. It also by default support the
> conversion for the vmalloc'ed memory address.
> 
> In TDX Guest, tdx_enc_status_changed() function is triggered by
> set_memory_*crypted() APIs when converting memory from/to shared or
> private. Internally this function uses __pa() for physical address
> conversion, which breaks the vmalloc address compatibility of the
> set_memory_*crypted() APIs.
> 
> So add support to fix the vmalloc'ed address compatibility issue.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  arch/x86/coco/tdx/tdx.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index b49211994864..37d58675ccf1 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -15,6 +15,7 @@
>  #include <asm/idtentry.h>
>  #include <asm/irq_regs.h>
>  #include <asm/desc.h>
> +#include <asm/io.h>
>  
>  /* TDX module Call Leaf IDs */
>  #define TDX_GET_INFO			1
> @@ -680,8 +681,14 @@ static bool try_accept_one(phys_addr_t *start, unsigned long len,
>   */
>  static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>  {
> -	phys_addr_t start = __pa(vaddr);
> -	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
> +	phys_addr_t start, end;
> +
> +	if (is_vmalloc_addr((void *)vaddr))
> +		start = vmalloc_to_pfn((void *) vaddr) << PAGE_SHIFT;
> +	else
> +		start = __pa(vaddr);
> +
> +	end = start + numpages * PAGE_SIZE;
>  
>  	if (!enc) {
>  		/* Set the shared (decrypted) bits: */

AMD uses lookup_address() which doesn't require the vaddr being vmap() address.
Shouldn't TDX use the same way?

-- 
Thanks,
-Kai
Re: [PATCH v7 3/5] x86/mm: Make tdx_enc_status_changed() vmalloc address compatible
Posted by Sathyanarayanan Kuppuswamy 3 years, 8 months ago

On 5/30/22 3:47 AM, Kai Huang wrote:
> On Mon, 2022-05-23 at 21:05 -0700, Kuppuswamy Sathyanarayanan wrote:
>> set_memory_*crypted() APIs are used to change encryption or decryption
>> page attributes for the given address. It also by default support the
>> conversion for the vmalloc'ed memory address.
>>
>> In TDX Guest, tdx_enc_status_changed() function is triggered by
>> set_memory_*crypted() APIs when converting memory from/to shared or
>> private. Internally this function uses __pa() for physical address
>> conversion, which breaks the vmalloc address compatibility of the
>> set_memory_*crypted() APIs.
>>
>> So add support to fix the vmalloc'ed address compatibility issue.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   arch/x86/coco/tdx/tdx.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index b49211994864..37d58675ccf1 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -15,6 +15,7 @@
>>   #include <asm/idtentry.h>
>>   #include <asm/irq_regs.h>
>>   #include <asm/desc.h>
>> +#include <asm/io.h>
>>   
>>   /* TDX module Call Leaf IDs */
>>   #define TDX_GET_INFO			1
>> @@ -680,8 +681,14 @@ static bool try_accept_one(phys_addr_t *start, unsigned long len,
>>    */
>>   static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>>   {
>> -	phys_addr_t start = __pa(vaddr);
>> -	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
>> +	phys_addr_t start, end;
>> +
>> +	if (is_vmalloc_addr((void *)vaddr))
>> +		start = vmalloc_to_pfn((void *) vaddr) << PAGE_SHIFT;
>> +	else
>> +		start = __pa(vaddr);
>> +
>> +	end = start + numpages * PAGE_SIZE;
>>   
>>   	if (!enc) {
>>   		/* Set the shared (decrypted) bits: */
> 
> AMD uses lookup_address() which doesn't require the vaddr being vmap() address.
> Shouldn't TDX use the same way?

AMD uses it to add some additional checks for address validation and
missing PTE entry. But in our case, reaching here means address is valid
and there is a valid PTE entry. So there is no need to lookup again and
convert it. It is easier to use above method.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Re: [PATCH v7 3/5] x86/mm: Make tdx_enc_status_changed() vmalloc address compatible
Posted by Wander Lairson Costa 3 years, 8 months ago
On Mon, May 23, 2022 at 09:05:15PM -0700, Kuppuswamy Sathyanarayanan wrote:
> set_memory_*crypted() APIs are used to change encryption or decryption
> page attributes for the given address. It also by default support the
> conversion for the vmalloc'ed memory address.
> 
> In TDX Guest, tdx_enc_status_changed() function is triggered by
> set_memory_*crypted() APIs when converting memory from/to shared or
> private. Internally this function uses __pa() for physical address
> conversion, which breaks the vmalloc address compatibility of the
> set_memory_*crypted() APIs.
> 
> So add support to fix the vmalloc'ed address compatibility issue.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  arch/x86/coco/tdx/tdx.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index b49211994864..37d58675ccf1 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -15,6 +15,7 @@
>  #include <asm/idtentry.h>
>  #include <asm/irq_regs.h>
>  #include <asm/desc.h>
> +#include <asm/io.h>
>  
>  /* TDX module Call Leaf IDs */
>  #define TDX_GET_INFO			1
> @@ -680,8 +681,14 @@ static bool try_accept_one(phys_addr_t *start, unsigned long len,
>   */
>  static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>  {
> -	phys_addr_t start = __pa(vaddr);
> -	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
> +	phys_addr_t start, end;
> +
> +	if (is_vmalloc_addr((void *)vaddr))
> +		start = vmalloc_to_pfn((void *) vaddr) << PAGE_SHIFT;
> +	else
> +		start = __pa(vaddr);
> +
> +	end = start + numpages * PAGE_SIZE;
>  
>  	if (!enc) {
>  		/* Set the shared (decrypted) bits: */
> -- 
> 2.25.1
> 
> 

Acked-by: Wander Lairson Costa <wander@redhat.com>