[PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[Tom Lendacky]

The RMP table is probed early in the boot process before max_pfn has been
set, so the logic to check if the RMP covers all of system memory is not
valid.

Move the RMP memory coverage check from snp_probe_rmptable_info() into
snp_rmptable_init(), which is well after max_pfn has been set.

Fixes: 216d106c7ff7 ("x86/sev: Add SEV-SNP host initialization support")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/virt/svm/sev.c | 45 ++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 0ae10535c699..0636fc5279e6 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -61,7 +61,7 @@ struct rmpentry {
 /* Mask to apply to a PFN to get the first PFN of a 2MB page */
 #define PFN_PMD_MASK	GENMASK_ULL(63, PMD_SHIFT - PAGE_SHIFT)
 
-static u64 probed_rmp_base, probed_rmp_size;
+static u64 probed_rmp_base, probed_rmp_end, probed_rmp_size;
 static struct rmpentry *rmptable __ro_after_init;
 static u64 rmptable_max_pfn __ro_after_init;
 
@@ -120,7 +120,7 @@ static __init void snp_enable(void *arg)
 
 bool snp_probe_rmptable_info(void)
 {
-	u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+	u64 rmp_sz, rmp_base, rmp_end;
 
 	rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
 	rdmsrl(MSR_AMD64_RMP_END, rmp_end);
@@ -137,28 +137,12 @@ bool snp_probe_rmptable_info(void)
 
 	rmp_sz = rmp_end - rmp_base + 1;
 
-	/*
-	 * Calculate the amount the memory that must be reserved by the BIOS to
-	 * address the whole RAM, including the bookkeeping area. The RMP itself
-	 * must also be covered.
-	 */
-	max_rmp_pfn = max_pfn;
-	if (PHYS_PFN(rmp_end) > max_pfn)
-		max_rmp_pfn = PHYS_PFN(rmp_end);
-
-	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
-
-	if (calc_rmp_sz > rmp_sz) {
-		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
-		       calc_rmp_sz, rmp_sz);
-		return false;
-	}
-
 	probed_rmp_base = rmp_base;
+	probed_rmp_end  = rmp_end;
 	probed_rmp_size = rmp_sz;
 
 	pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
-		probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
+		probed_rmp_base, probed_rmp_end);
 
 	return true;
 }
@@ -206,9 +190,8 @@ void __init snp_fixup_e820_tables(void)
  */
 static int __init snp_rmptable_init(void)
 {
+	u64 max_rmp_pfn, calc_rmp_sz, rmptable_size, val;
 	void *rmptable_start;
-	u64 rmptable_size;
-	u64 val;
 
 	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return 0;
@@ -219,10 +202,26 @@ static int __init snp_rmptable_init(void)
 	if (!probed_rmp_size)
 		goto nosnp;
 
+	/*
+	 * Calculate the amount the memory that must be reserved by the BIOS to
+	 * address the whole RAM, including the bookkeeping area. The RMP itself
+	 * must also be covered.
+	 */
+	max_rmp_pfn = max_pfn;
+	if (PHYS_PFN(probed_rmp_end) > max_pfn)
+		max_rmp_pfn = PHYS_PFN(probed_rmp_end);
+
+	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
+	if (calc_rmp_sz > probed_rmp_size) {
+		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
+		       calc_rmp_sz, probed_rmp_size);
+		goto nosnp;
+	}
+
 	rmptable_start = memremap(probed_rmp_base, probed_rmp_size, MEMREMAP_WB);
 	if (!rmptable_start) {
 		pr_err("Failed to map RMP table\n");
-		return 1;
+		goto nosnp;
 	}
 
 	/*
-- 
2.43.2

Re: [PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[Liam Merwick]

On 05/06/2024 16:38, Tom Lendacky wrote:
> The RMP table is probed early in the boot process before max_pfn has been
> set, so the logic to check if the RMP covers all of system memory is not
> valid.
> 
> Move the RMP memory coverage check from snp_probe_rmptable_info() into
> snp_rmptable_init(), which is well after max_pfn has been set.
> 
> Fixes: 216d106c7ff7 ("x86/sev: Add SEV-SNP host initialization support")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>

Reviewed-by: Liam Merwick <liam.merwick@oracle.com>


> ---
>   arch/x86/virt/svm/sev.c | 45 ++++++++++++++++++++---------------------
>   1 file changed, 22 insertions(+), 23 deletions(-)
> 

Re: [PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[Borislav Petkov]

On Wed, Jun 05, 2024 at 10:38:37AM -0500, Tom Lendacky wrote:
> -static u64 probed_rmp_base, probed_rmp_size;
> +static u64 probed_rmp_base, probed_rmp_end, probed_rmp_size;

Why do you need _end if you have _size already?

IOW, please compute _end where you need it instead of adding yet another
static.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[Tom Lendacky]

On 6/21/24 08:59, Borislav Petkov wrote:
> On Wed, Jun 05, 2024 at 10:38:37AM -0500, Tom Lendacky wrote:
>> -static u64 probed_rmp_base, probed_rmp_size;
>> +static u64 probed_rmp_base, probed_rmp_end, probed_rmp_size;
> 
> Why do you need _end if you have _size already?
> 
> IOW, please compute _end where you need it instead of adding yet another
> static.

I think it makes the code easier to follow and less of a chance to compute
the value wrong given you have to substract 1 (end = base + size - 1). I
guess I can create a #define or a helper function so that the calculation
is always the same if that is preferred.

Thanks,
Tom

> 
> Thx.
> 

Re: [PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[Borislav Petkov]

On Fri, Jun 21, 2024 at 09:17:35AM -0500, Tom Lendacky wrote:
> I think it makes the code easier to follow and less of a chance to compute
> the value wrong given you have to substract 1 (end = base + size - 1). I
> guess I can create a #define or a helper function so that the calculation
> is always the same if that is preferred.

Why, what's wrong with this variant?

You can always do a separate variable if warranted but right now you need it
at exactly one spot and one spot only...

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 0ae10535c699..8a87c0344833 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -120,7 +120,7 @@ static __init void snp_enable(void *arg)
 
 bool snp_probe_rmptable_info(void)
 {
-	u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+	u64 rmp_sz, rmp_base, rmp_end;
 
 	rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
 	rdmsrl(MSR_AMD64_RMP_END, rmp_end);
@@ -137,28 +137,11 @@ bool snp_probe_rmptable_info(void)
 
 	rmp_sz = rmp_end - rmp_base + 1;
 
-	/*
-	 * Calculate the amount the memory that must be reserved by the BIOS to
-	 * address the whole RAM, including the bookkeeping area. The RMP itself
-	 * must also be covered.
-	 */
-	max_rmp_pfn = max_pfn;
-	if (PHYS_PFN(rmp_end) > max_pfn)
-		max_rmp_pfn = PHYS_PFN(rmp_end);
-
-	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
-
-	if (calc_rmp_sz > rmp_sz) {
-		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
-		       calc_rmp_sz, rmp_sz);
-		return false;
-	}
-
 	probed_rmp_base = rmp_base;
 	probed_rmp_size = rmp_sz;
 
 	pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
-		probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
+		probed_rmp_base, rmp_end);
 
 	return true;
 }
@@ -206,9 +189,8 @@ void __init snp_fixup_e820_tables(void)
  */
 static int __init snp_rmptable_init(void)
 {
+	u64 max_rmp_pfn, calc_rmp_sz, rmptable_size, probed_rmp_end, val;
 	void *rmptable_start;
-	u64 rmptable_size;
-	u64 val;
 
 	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return 0;
@@ -219,10 +201,28 @@ static int __init snp_rmptable_init(void)
 	if (!probed_rmp_size)
 		goto nosnp;
 
+	probed_rmp_end = probed_rmp_base + probed_rmp_size - 1;
+
+	/*
+	 * Calculate the amount the memory that must be reserved by the BIOS to
+	 * address the whole RAM, including the bookkeeping area. The RMP itself
+	 * must also be covered.
+	 */
+	max_rmp_pfn = max_pfn;
+	if (PHYS_PFN(probed_rmp_end) > max_pfn)
+		max_rmp_pfn = PHYS_PFN(probed_rmp_end);
+
+	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
+	if (calc_rmp_sz > probed_rmp_size) {
+		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
+		       calc_rmp_sz, probed_rmp_size);
+		goto nosnp;
+	}
+
 	rmptable_start = memremap(probed_rmp_base, probed_rmp_size, MEMREMAP_WB);
 	if (!rmptable_start) {
 		pr_err("Failed to map RMP table\n");
-		return 1;
+		goto nosnp;
 	}
 
 	/*

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[Tom Lendacky]

On 6/21/24 09:29, Borislav Petkov wrote:
> On Fri, Jun 21, 2024 at 09:17:35AM -0500, Tom Lendacky wrote:
>> I think it makes the code easier to follow and less of a chance to compute
>> the value wrong given you have to substract 1 (end = base + size - 1). I
>> guess I can create a #define or a helper function so that the calculation
>> is always the same if that is preferred.
> 
> Why, what's wrong with this variant?
> 
> You can always do a separate variable if warranted but right now you need it
> at exactly one spot and one spot only...

Ok, I'll remove the new static and resubmit. There is also a logic error
in the original check which should be using PFN_UP instead of PHYS_PFN, so
I'll include that, too.

Do you want a single patch or two patches, one to fix the PHYS_PFN to
PFN_UP and one to move the check?

Thanks,
Tom

> 
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 0ae10535c699..8a87c0344833 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -120,7 +120,7 @@ static __init void snp_enable(void *arg)
>  
>  bool snp_probe_rmptable_info(void)
>  {
> -	u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
> +	u64 rmp_sz, rmp_base, rmp_end;
>  
>  	rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
>  	rdmsrl(MSR_AMD64_RMP_END, rmp_end);
> @@ -137,28 +137,11 @@ bool snp_probe_rmptable_info(void)
>  
>  	rmp_sz = rmp_end - rmp_base + 1;
>  
> -	/*
> -	 * Calculate the amount the memory that must be reserved by the BIOS to
> -	 * address the whole RAM, including the bookkeeping area. The RMP itself
> -	 * must also be covered.
> -	 */
> -	max_rmp_pfn = max_pfn;
> -	if (PHYS_PFN(rmp_end) > max_pfn)
> -		max_rmp_pfn = PHYS_PFN(rmp_end);
> -
> -	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
> -
> -	if (calc_rmp_sz > rmp_sz) {
> -		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
> -		       calc_rmp_sz, rmp_sz);
> -		return false;
> -	}
> -
>  	probed_rmp_base = rmp_base;
>  	probed_rmp_size = rmp_sz;
>  
>  	pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
> -		probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
> +		probed_rmp_base, rmp_end);
>  
>  	return true;
>  }
> @@ -206,9 +189,8 @@ void __init snp_fixup_e820_tables(void)
>   */
>  static int __init snp_rmptable_init(void)
>  {
> +	u64 max_rmp_pfn, calc_rmp_sz, rmptable_size, probed_rmp_end, val;
>  	void *rmptable_start;
> -	u64 rmptable_size;
> -	u64 val;
>  
>  	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
>  		return 0;
> @@ -219,10 +201,28 @@ static int __init snp_rmptable_init(void)
>  	if (!probed_rmp_size)
>  		goto nosnp;
>  
> +	probed_rmp_end = probed_rmp_base + probed_rmp_size - 1;
> +
> +	/*
> +	 * Calculate the amount the memory that must be reserved by the BIOS to
> +	 * address the whole RAM, including the bookkeeping area. The RMP itself
> +	 * must also be covered.
> +	 */
> +	max_rmp_pfn = max_pfn;
> +	if (PHYS_PFN(probed_rmp_end) > max_pfn)
> +		max_rmp_pfn = PHYS_PFN(probed_rmp_end);
> +
> +	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
> +	if (calc_rmp_sz > probed_rmp_size) {
> +		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
> +		       calc_rmp_sz, probed_rmp_size);
> +		goto nosnp;
> +	}
> +
>  	rmptable_start = memremap(probed_rmp_base, probed_rmp_size, MEMREMAP_WB);
>  	if (!rmptable_start) {
>  		pr_err("Failed to map RMP table\n");
> -		return 1;
> +		goto nosnp;
>  	}
>  
>  	/*
> 

Re: [PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[Borislav Petkov]

On Fri, Jun 21, 2024 at 09:37:46AM -0500, Tom Lendacky wrote:
> Ok, I'll remove the new static and resubmit. There is also a logic error
> in the original check which should be using PFN_UP instead of PHYS_PFN, so
> I'll include that, too.

So we said this fix should not go to stable because SNP host is not upstream
yet.
 
> Do you want a single patch or two patches, one to fix the PHYS_PFN to
> PFN_UP and one to move the check?

Since this is snp_rmptable_init() and that is also SNP host then I think
a single patch is fine.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/sev: Do RMP memory coverage check after max_pfn has been set

[Tom Lendacky]

On 6/21/24 09:49, Borislav Petkov wrote:
> On Fri, Jun 21, 2024 at 09:37:46AM -0500, Tom Lendacky wrote:
>> Ok, I'll remove the new static and resubmit. There is also a logic error
>> in the original check which should be using PFN_UP instead of PHYS_PFN, so
>> I'll include that, too.
> 
> So we said this fix should not go to stable because SNP host is not upstream
> yet.

Correct.

>  
>> Do you want a single patch or two patches, one to fix the PHYS_PFN to
>> PFN_UP and one to move the check?
> 
> Since this is snp_rmptable_init() and that is also SNP host then I think
> a single patch is fine.

Will do.

Thanks,
Tom

> 
> Thx.
>