Re: [PATCH v2] x86/sev: Make enc_dec_hypercall() accept a size instead of npages

[Gupta, Pankaj]

On 8/22/2023 12:58 AM, Steve Rutherford wrote:
> enc_dec_hypercall() accepted a page count instead of a size, which
> forced its callers to round up. As a result, non-page aligned
> vaddrs caused pages to be spuriously marked as decrypted via the
> encryption status hypercall, which in turn caused consistent
> corruption of pages during live migration. Live migration requires
> accurate encryption status information to avoid migrating pages
> from the wrong perspective.
>
> Fixes: 064ce6c550a0 ("mm: x86: Invoke hypercall when page encryption status is changed")
> Signed-off-by: Steve Rutherford <srutherford@xxxxxxxxxx>
> ---
>   arch/x86/include/asm/mem_encrypt.h |  6 +++---
>   arch/x86/kernel/kvm.c              |  4 +---
>   arch/x86/mm/mem_encrypt_amd.c      | 13 ++++++-------
>   3 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> index 7f97a8a97e24..473b16d73b47 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -50,8 +50,8 @@ void __init sme_enable(struct boot_params *bp);
>   
>   int __init early_set_memory_decrypted(unsigned long vaddr, unsigned long size);
>   int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size);
> -void __init early_set_mem_enc_dec_hypercall(unsigned long vaddr, int npages,
> -					    bool enc);
> +void __init early_set_mem_enc_dec_hypercall(unsigned long vaddr,
> +					    unsigned long size, bool enc);
>   
>   void __init mem_encrypt_free_decrypted_mem(void);
>   
> @@ -85,7 +85,7 @@ early_set_memory_decrypted(unsigned long vaddr, unsigned long size) { return 0;
>   static inline int __init
>   early_set_memory_encrypted(unsigned long vaddr, unsigned long size) { return 0; }
>   static inline void __init
> -early_set_mem_enc_dec_hypercall(unsigned long vaddr, int npages, bool enc) {}
> +early_set_mem_enc_dec_hypercall(unsigned long vaddr, unsigned long size, bool enc) {}
>   
>   static inline void mem_encrypt_free_decrypted_mem(void) { }
>   
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 6a36db4f79fd..b8ab9ee5896c 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -966,10 +966,8 @@ static void __init kvm_init_platform(void)
>   		 * Ensure that _bss_decrypted section is marked as decrypted in the
>   		 * shared pages list.
>   		 */
> -		nr_pages = DIV_ROUND_UP(__end_bss_decrypted - __start_bss_decrypted,
> -					PAGE_SIZE);
>   		early_set_mem_enc_dec_hypercall((unsigned long)__start_bss_decrypted,
> -						nr_pages, 0);
> +						__end_bss_decrypted - __start_bss_decrypted, 0);
>   
>   		/*
>   		 * If not booted using EFI, enable Live migration support.
> diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
> index 54bbd5163e8d..6faea41e99b6 100644
> --- a/arch/x86/mm/mem_encrypt_amd.c
> +++ b/arch/x86/mm/mem_encrypt_amd.c
> @@ -288,11 +288,10 @@ static bool amd_enc_cache_flush_required(void)
>   	return !cpu_feature_enabled(X86_FEATURE_SME_COHERENT);
>   }
>   
> -static void enc_dec_hypercall(unsigned long vaddr, int npages, bool enc)
> +static void enc_dec_hypercall(unsigned long vaddr, unsigned long size, bool enc)
>   {
>   #ifdef CONFIG_PARAVIRT
> -	unsigned long sz = npages << PAGE_SHIFT;
> -	unsigned long vaddr_end = vaddr + sz;
> +	unsigned long vaddr_end = vaddr + size;
>   
>   	while (vaddr < vaddr_end) {
>   		int psize, pmask, level;
> @@ -342,7 +341,7 @@ static bool amd_enc_status_change_finish(unsigned long vaddr, int npages, bool e
>   		snp_set_memory_private(vaddr, npages);
>   
>   	if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
> -		enc_dec_hypercall(vaddr, npages, enc);
> +		enc_dec_hypercall(vaddr, npages << PAGE_SHIFT, enc);
>   
>   	return true;
>   }
> @@ -466,7 +465,7 @@ static int __init early_set_memory_enc_dec(unsigned long vaddr,
>   
>   	ret = 0;
>   
> -	early_set_mem_enc_dec_hypercall(start, PAGE_ALIGN(size) >> PAGE_SHIFT, enc);
> +	early_set_mem_enc_dec_hypercall(start, size, enc);
>   out:
>   	__flush_tlb_all();
>   	return ret;
> @@ -482,9 +481,9 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)
>   	return early_set_memory_enc_dec(vaddr, size, true);
>   }
>   
> -void __init early_set_mem_enc_dec_hypercall(unsigned long vaddr, int npages, bool enc)
> +void __init early_set_mem_enc_dec_hypercall(unsigned long vaddr, unsigned long size, bool enc)
>   {
> -	enc_dec_hypercall(vaddr, npages, enc);
> +	enc_dec_hypercall(vaddr, size, enc);
>   }
>   
>   void __init sme_early_init(void)

Also had this thought to avoid passing the page boundaries calculation 
with npages in-place of existing size based, but no strong opinions.

This seems even better. Thanks!

Reviewed-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>