Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

["Kalra, Ashish" <ashish.kalra@amd.com>]

Hello Boris,

> Subject: Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.
>
> From: Ashish Kalra<ashish.kalra@amd.com>

<snip>
> Why do you keep explaining in your commit messages what a patch does?
I will fix the commit message.
>> Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>   arch/x86/include/asm/sev.h |  2 ++
>>   arch/x86/mm/mem_encrypt.c  |  3 +++
>>   arch/x86/virt/svm/sev.c    | 44 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 49 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index 7f57382afee4..6600ac467cf9 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -269,6 +269,7 @@ int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immut
>>   int rmp_make_shared(u64 pfn, enum pg_level level);
>>   void snp_leak_pages(u64 pfn, unsigned int npages);
>>   void kdump_sev_callback(void);
>> +void snp_rmptable_e820_fixup(void);
>>   #else
>>   static inline bool snp_probe_rmptable_info(void) { return false; }
>>   static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; }
>> @@ -282,6 +283,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 as
>>   static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; }
>>   static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
>>   static inline void kdump_sev_callback(void) { }
>> +static inline void snp_rmptable_e820_fixup(void) {}
>>   #endif
>>   
>>   #endif
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 6f3b3e028718..765ce94e4b89 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -102,6 +102,9 @@ void __init mem_encrypt_setup_arch(void)
>>   	phys_addr_t total_mem = memblock_phys_mem_size();
>>   	unsigned long size;
>>   
>> +	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
> We use CC_ATTR_HOST_SEV_SNP for host SNP support checks, including RMP
> table viability.
Ok.
>
> Also, why isn't this called in snp_init()?
>
> If there's a reason why (I think there is) put that reason as a comment
> above it why this thing needs to be called here exactly.

This callback needs to be invoked as part of setup_arch() as it needs 
e820 table to be setup in e820__memory_setup() before the callback is 
invoked and snp_init() is called from sme_enable() in kernel/head_64.S 
(startup_64), which is much before start_kernel() -> setup_arch() is 
invoked.

I will add the comment here.

>> +		snp_rmptable_e820_fixup();
> IOW, point to the comment above that function's definition.
>
>> +
>>   	if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
>>   		return;
>>   
>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>> index ab0e8448bb6e..d999ff7f1671 100644
>> --- a/arch/x86/virt/svm/sev.c
>> +++ b/arch/x86/virt/svm/sev.c
>> @@ -163,6 +163,50 @@ bool snp_probe_rmptable_info(void)
>>   	return true;
>>   }
>>   
>> +/*
>> + * Callback to do any RMP table fixups, needs to be called
>> + * after e820__memory_setup(), after the e820 tables are
>> + * setup/populated and before e820__reserve_resources(), before
>> + * the e820 map has been converted to the standard Linux memory
>> + * resources and e820 map is no longer used and modifying it
>> + * has no effect.
>> + */
>> +void __init snp_rmptable_e820_fixup(void)
>> +{
>> +	u64 pa;
>> +
>> +	/*
>> +	 * Handle cases where the RMP table placement in the BIOS is not 2M aligned
>> +	 * and then the kexec kernel could try to allocate from within that chunk
>> +	 * and that causes a fatal RMP fault.
> Merge this comment with the one above the function and put it all there.
Ok.
>> Check if RMP table start & end
>> +	 * physical range in e820_table is not aligned to 2MB and in that case use
>> +	 * e820__range_update() to map this range to reserved, e820__range_update()
>> +	 * nicely handles partial range update and also merges any consecutive
>> +	 * ranges of the same type.
>> +	 */
> This comment talks about what this does and is kinda obvious but then
> talks about e820__range_update() and not the other ones. Just put the
> gist of what this is supposed to do and do not explain the code step by
> step.
>
> What is really missing here and what is not really trivial is why all
> three e820 tables need updating.
I will add all these details.
>
>> +	pa = probed_rmp_base;
>> +	if (!IS_ALIGNED(pa, PMD_SIZE)) {
>> +		pa = ALIGN_DOWN(pa, PMD_SIZE);
>> +		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
>> +			pr_info("Reserving start of RMP table on a 2MB boundary [0x%016llx]\n", pa);
>> +			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
>> +			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
>> +			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
>> +		}
>> +	}
>> +
>> +	pa = probed_rmp_base + probed_rmp_size;
>> +	if (!IS_ALIGNED(pa, PMD_SIZE)) {
>> +		pa = ALIGN_DOWN(pa, PMD_SIZE);
>> +		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
>> +			pr_info("Reserving end of RMP table on a 2MB boundary [0x%016llx]\n", pa);
>> +			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
>> +			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
>> +			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
>> +		}
>> +	}
>> +}
> Ontop for less duplication:
>
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index be17661fee9b..118dfe61f80e 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -163,6 +163,21 @@ bool snp_probe_rmptable_info(void)
>   	return true;
>   }
>   
> +static void __init __snp_e820_tables_fixup(u64 pa)
> +{
> +	if (IS_ALIGNED(pa, PMD_SIZE))
> +		return;
> +
> +	pa = ALIGN_DOWN(pa, PMD_SIZE);
> +	if (!e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM))
> +		return;
> +
> +	pr_info("Reserving chunk of RMP table on a 2MB boundary [0x%016llx]\n", pa);
> +	e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> +	e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> +	e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> +}
> +
>   /*
>    * Callback to do any RMP table fixups, needs to be called
>    * after e820__memory_setup(), after the e820 tables are
> @@ -173,8 +188,6 @@ bool snp_probe_rmptable_info(void)
>    */
>   void __init snp_rmptable_e820_fixup(void)
>   {
> -	u64 pa;
> -
>   	/*
>   	 * Handle cases where the RMP table placement in the BIOS is not 2M aligned
>   	 * and then the kexec kernel could try to allocate from within that chunk
> @@ -184,27 +197,8 @@ void __init snp_rmptable_e820_fixup(void)
>   	 * nicely handles partial range update and also merges any consecutive
>   	 * ranges of the same type.
>   	 */
> -	pa = probed_rmp_base;
> -	if (!IS_ALIGNED(pa, PMD_SIZE)) {
> -		pa = ALIGN_DOWN(pa, PMD_SIZE);
> -		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
> -			pr_info("Reserving start of RMP table on a 2MB boundary [0x%016llx]\n", pa);
> -			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> -			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> -			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> -		}
> -	}
> -
> -	pa = probed_rmp_base + probed_rmp_size;
> -	if (!IS_ALIGNED(pa, PMD_SIZE)) {
> -		pa = ALIGN_DOWN(pa, PMD_SIZE);
> -		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
> -			pr_info("Reserving end of RMP table on a 2MB boundary [0x%016llx]\n", pa);
> -			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> -			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> -			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
> -		}
> -	}
> +	__snp_e820_tables_fixup(probed_rmp_base);
> +	__snp_e820_tables_fixup(probed_rmp_base + probed_rmp_size);
>   }
>   
Thanks, Ashish