arch/x86/virt/svm/sev.c | 45 ++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 23 deletions(-)
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.2On 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
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. >
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
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;
> }
>
> /*
>
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
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. >
© 2016 - 2026 Red Hat, Inc.