From: Ashish Kalra <[email protected]>
RMP table start and end physical range may not be aligned to 2MB in
the e820 tables causing fatal RMP page faults during kexec boot when
new page allocations are done in the same 2MB page as the RMP table.
Check if RMP table start and 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.
Override e820__memory_setup_default() to check and apply these RMP table
fixups in e820_table before e820_table is used to setup
e280_table_firmware and e820_table_kexec.
Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")
Signed-off-by: Ashish Kalra <[email protected]>
---
arch/x86/virt/svm/sev.c | 52 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index cffe1157a90a..e0d7584df28f 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -65,6 +65,8 @@ static u64 probed_rmp_base, probed_rmp_size;
static struct rmpentry *rmptable __ro_after_init;
static u64 rmptable_max_pfn __ro_after_init;
+static char *__init snp_rmptable_e820_fixup(void);
+
static LIST_HEAD(snp_leaked_pages_list);
static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
@@ -160,9 +162,59 @@ bool snp_probe_rmptable_info(void)
pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
+ /*
+ * Override e820__memory_setup_default() to do any RMP table fixups
+ * for kexec if required.
+ */
+ x86_init.resources.memory_setup = snp_rmptable_e820_fixup;
+
return true;
}
+/*
+ * Override e820__memory_setup_default() to do any RMP table fixups
+ * in e820_table before e820_table_firmware and e820_table_kexec
+ * are setup.
+ */
+static char *__init snp_rmptable_e820_fixup(void)
+{
+ /* Populate e820_table from BIOS-supplied e820 map */
+ char *p = e820__memory_setup_default();
+ u64 pa;
+
+ /*
+ * RMP table start & end physical range may not be aligned to 2MB in the
+ * e820 tables causing fatal RMP page faults during kexec boot when new
+ * page allocations are done in the same 2MB page as the RMP table.
+ * 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.
+ * Need to override e820__memory_setup_default() to check and apply
+ * fixups in e820_table before e820_table is used to setup
+ * e280_table_firmware and e820_table_kexec.
+ */
+ 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);
+ }
+ }
+
+ 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);
+ }
+ }
+
+ return p;
+}
+
/*
* Do the necessary preparations which are verified by the firmware as
* described in the SNP_INIT_EX firmware command description in the SNP
--
2.34.1
On 3/13/2024 12:17 AM, Ashish Kalra wrote:
> From: Ashish Kalra <[email protected]>
>
> RMP table start and end physical range may not be aligned to 2MB in
> the e820 tables causing fatal RMP page faults during kexec boot when
> new page allocations are done in the same 2MB page as the RMP table.
> Check if RMP table start and 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.
>
> Override e820__memory_setup_default() to check and apply these RMP table
> fixups in e820_table before e820_table is used to setup
> e280_table_firmware and e820_table_kexec.
>
> Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")
> Signed-off-by: Ashish Kalra <[email protected]>
> ---
> arch/x86/virt/svm/sev.c | 52 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index cffe1157a90a..e0d7584df28f 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -65,6 +65,8 @@ static u64 probed_rmp_base, probed_rmp_size;
> static struct rmpentry *rmptable __ro_after_init;
> static u64 rmptable_max_pfn __ro_after_init;
>
> +static char *__init snp_rmptable_e820_fixup(void);
> +
> static LIST_HEAD(snp_leaked_pages_list);
> static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
>
> @@ -160,9 +162,59 @@ bool snp_probe_rmptable_info(void)
> pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
> probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
>
> + /*
> + * Override e820__memory_setup_default() to do any RMP table fixups
> + * for kexec if required.
> + */
> + x86_init.resources.memory_setup = snp_rmptable_e820_fixup;
> +
> return true;
> }
>
> +/*
> + * Override e820__memory_setup_default() to do any RMP table fixups
> + * in e820_table before e820_table_firmware and e820_table_kexec
> + * are setup.
> + */
> +static char *__init snp_rmptable_e820_fixup(void)
> +{
> + /* Populate e820_table from BIOS-supplied e820 map */
> + char *p = e820__memory_setup_default();
> + u64 pa;
> +
> + /*
> + * RMP table start & end physical range may not be aligned to 2MB in the
> + * e820 tables causing fatal RMP page faults during kexec boot when new
> + * page allocations are done in the same 2MB page as the RMP table.
> + * 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.
> + * Need to override e820__memory_setup_default() to check and apply
> + * fixups in e820_table before e820_table is used to setup
> + * e280_table_firmware and e820_table_kexec.
> + */
> + 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);
> + }
> + }
> +
> + 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);
> + }
> + }
> +
> + return p;
> +}
> +
> /*
> * Do the necessary preparations which are verified by the firmware as
> * described in the SNP_INIT_EX firmware command description in the SNP
Tested this patch, it fixes the kexec issue reported. Thank you.
Tested-by: Srikanth Aithal <[email protected]>
From: Borislav Petkov <[email protected]>
On Tue, Mar 12, 2024 at 06:47:57PM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <[email protected]>
>
> RMP table start and end physical range may not be aligned to 2MB in
> the e820 tables
This already sounds fishy. Why may the range not be aligned? This is
BIOS, right? And BIOS can be fixed to align them properly.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/2/24 09:45, [email protected] wrote:
> From: Borislav Petkov <[email protected]>
>
> On Tue, Mar 12, 2024 at 06:47:57PM +0000, Ashish Kalra wrote:
>> From: Ashish Kalra <[email protected]>
>>
>> RMP table start and end physical range may not be aligned to 2MB in
>> the e820 tables
>
> This already sounds fishy. Why may the range not be aligned? This is
> BIOS, right? And BIOS can be fixed to align them properly.
There's no requirement from a hardware/RMP usage perspective that
requires a 2MB alignment, so BIOS is not doing anything wrong. The
problem occurs because kexec is initially using 2MB mappings that
overlap the start and/or end of the RMP which then results in an RMP
fault when memory within one of those 2MB mappings, that is not part of
the RMP, is referenced.
Additionally, we have BIOSes out there since Milan that don't do this
2MB alignment. And do you really trust that BIOS will do this properly
all the time?
I think it needs to be checked and mitigated in the kernel.
Thanks,
Tom
>
From: Borislav Petkov <[email protected]>
On Tue, Apr 02, 2024 at 10:54:50AM -0500, Tom Lendacky wrote:
> There's no requirement from a hardware/RMP usage perspective that requires a
> 2MB alignment, so BIOS is not doing anything wrong. The problem occurs
> because kexec is initially using 2MB mappings that overlap the start and/or
> end of the RMP which then results in an RMP fault when memory within one of
> those 2MB mappings, that is not part of the RMP, is referenced.
Then this explanation is misleading. And that whole bla about alignment
is nonsense either.
> Additionally, we have BIOSes out there since Milan that don't do this 2MB
> alignment. And do you really trust that BIOS will do this properly all the
> time?
I don't trust the BIOS to do anything properly.
So why isn't the fix for this simply to reserve the space for the RMP
table to start at 2M page - even if it doesn't - and to cover the last
chunk *also* with a 2M page and be done with it?
Not this silly overriding dance.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/2/2024 11:34 AM, [email protected] wrote:
> From: Borislav Petkov <[email protected]>
>
> On Tue, Apr 02, 2024 at 10:54:50AM -0500, Tom Lendacky wrote:
>> There's no requirement from a hardware/RMP usage perspective that requires a
>> 2MB alignment, so BIOS is not doing anything wrong. The problem occurs
>> because kexec is initially using 2MB mappings that overlap the start and/or
>> end of the RMP which then results in an RMP fault when memory within one of
>> those 2MB mappings, that is not part of the RMP, is referenced.
> Then this explanation is misleading. And that whole bla about alignment
> is nonsense either.
>
>> Additionally, we have BIOSes out there since Milan that don't do this 2MB
>> alignment. And do you really trust that BIOS will do this properly all the
>> time?
> I don't trust the BIOS to do anything properly.
>
> So why isn't the fix for this simply to reserve the space for the RMP
> table to start at 2M page - even if it doesn't - and to cover the last
> chunk *also* with a 2M page and be done with it?
But, it is the BIOS which reserves the space for the RMP table.
And if you mean the reservation in the kernel page tables (directmap)
then that will not help as kexec uses it's own identity mapped page tables.
>
> Not this silly overriding dance.
This overriding dance is required to fixup all the three kexec tables,
as there is no interface/API available to do modifications/fixups in all
the three possible kexec tables.
Thanks, Ashish
> Thx.
>
On 4/2/2024 12:06 PM, Kalra, Ashish wrote:
> On 4/2/2024 11:34 AM, [email protected] wrote:
>
>> From: Borislav Petkov <[email protected]>
>>
>> On Tue, Apr 02, 2024 at 10:54:50AM -0500, Tom Lendacky wrote:
>>> There's no requirement from a hardware/RMP usage perspective that
>>> requires a
>>> 2MB alignment, so BIOS is not doing anything wrong. The problem occurs
>>> because kexec is initially using 2MB mappings that overlap the start
>>> and/or
>>> end of the RMP which then results in an RMP fault when memory within
>>> one of
>>> those 2MB mappings, that is not part of the RMP, is referenced.
>> Then this explanation is misleading. And that whole bla about alignment
>> is nonsense either.
>>
>>> Additionally, we have BIOSes out there since Milan that don't do
>>> this 2MB
>>> alignment. And do you really trust that BIOS will do this properly
>>> all the
>>> time?
>> I don't trust the BIOS to do anything properly.
>>
>> So why isn't the fix for this simply to reserve the space for the RMP
>> table to start at 2M page - even if it doesn't - and to cover the last
>> chunk *also* with a 2M page and be done with it?
>
> But, it is the BIOS which reserves the space for the RMP table.
>
> And if you mean the reservation in the kernel page tables (directmap)
> then that will not help as kexec uses it's own identity mapped page
> tables.
>
>>
>> Not this silly overriding dance.
>
> This overriding dance is required to fixup all the three kexec tables,
> as there is no interface/API available to do modifications/fixups in
> all the three possible kexec tables.
>
I meant the three e820 tables out of which two are used for kexec, the
e820_table_firmware and e820_table_kexec.
Thanks, Ashish
>
>> Thx.
>>
On Tue, Apr 02, 2024 at 12:11:03PM -0500, Kalra, Ashish wrote:
> > And if you mean the reservation in the kernel page tables (directmap)
> > then that will not help as kexec uses it's own identity mapped page
> > tables.
So how hard would it be to *always* reserve the chunk of memory at the
2M boundary where the RMP table starts and up to the 2M boundary where
the RMP table ends?
In *every* kernel.
By default.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/2/2024 12:45 PM, Borislav Petkov wrote:
> On Tue, Apr 02, 2024 at 12:11:03PM -0500, Kalra, Ashish wrote:
>>> And if you mean the reservation in the kernel page tables (directmap)
>>> then that will not help as kexec uses it's own identity mapped page
>>> tables.
> So how hard would it be to *always* reserve the chunk of memory at the
> 2M boundary where the RMP table starts and up to the 2M boundary where
> the RMP table ends?
>
> In *every* kernel.
But, that's what the e820 table fixup is doing, it is reserving the
chunk of memory at the 2M boundary where the RMP table starts and ends
in the e820. Kexec is referring to the e820 table(s) for placing it's
segments in memory for the kexec boot.
Thanks, Ashish
>
> By default.
>
> Thx.
>
On Tue, Apr 02, 2024 at 01:41:05PM -0500, Kalra, Ashish wrote:
> But, that's what the e820 table fixup is doing,
And can it be doing it in snp_probe_rmptable_info() where all the RMP
table stuff is being done?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/2/2024 1:50 PM, Borislav Petkov wrote:
> On Tue, Apr 02, 2024 at 01:41:05PM -0500, Kalra, Ashish wrote:
>> But, that's what the e820 table fixup is doing,
> And can it be doing it in snp_probe_rmptable_info() where all the RMP
> table stuff is being done?
The e820 fixups has to be done on both e820_table_kexec and
e820_table_firmware (to handle both kexec variants) and this has to be
done when e820_table is used to populate both e820_table_kexec and
e820_table_firmware and this is done in e820__memory_setup().
But, snp_probe_rmptable_info() is called as part of early_cpu_init() ->
early_identify_cpu() which is called earlier than e820__memory_setup()
and e820 tables are setup much later e820__memory_setup().
And we can't do this in snp_rmptable_init() as e820_table_firmware can't
be fixed at that point and by that time this table has been mapped into
sysfs (/sys/firmware) which is used by kexec -c variant.
The function e820__memory_setup() has an optional platform quirk
available to override/pick up the firmware/bootloader e820 map, and
that's why i am using this platform specific quirk to override it with
the SNP specific RMP table e820 fixup function which internally calls
e820__memory_setup_default() to pass the firmware/bootloader e820 map
(set in e820_table) and then add RMP table fixups on top of it.
Thanks, Ashish
On Tue, Apr 02, 2024 at 02:33:44PM -0500, Kalra, Ashish wrote:
> And we can't do this in snp_rmptable_init() as e820_table_firmware can't be
> fixed at that point and by that time this table has been mapped into sysfs
> (/sys/firmware) which is used by kexec -c variant.
Well, you have to do something here because if snp_rmptable_init()
late-disables SNP, your RMP table fixups are moot and invalid.
Which means, your RMP table fixups need to happen at the *very* *late*
step after we know that SNP is enabled and won't get disabled anymore.
I.e., in snp_rmptable_init().
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/2/2024 3:21 PM, Borislav Petkov wrote:
> On Tue, Apr 02, 2024 at 02:33:44PM -0500, Kalra, Ashish wrote:
>> And we can't do this in snp_rmptable_init() as e820_table_firmware can't be
>> fixed at that point and by that time this table has been mapped into sysfs
>> (/sys/firmware) which is used by kexec -c variant.
> Well, you have to do something here because if snp_rmptable_init()
> late-disables SNP, your RMP table fixups are moot and invalid.
>
> Which means, your RMP table fixups need to happen at the *very* *late*
> step after we know that SNP is enabled and won't get disabled anymore.
>
> I.e., in snp_rmptable_init().
The main issue with doing that in snp_rmptable_init() is that there is
no e820 API interfaces available to update the e820_table_kexec and
e820_table_firmware and e820_table_firmware has already been exposed to
sysfs.
The e820 API only exports e820__range_update() which *only* fixes
e820_table.
The important point to note here is that in most cases BIOS would have
reserved RMP table start and end aligned to 2M boundary and setup the
e820 table which the BIOS passes to the kernel as such, so even if the
kernel does not enable SNP or disables SNP later, these reservations
will remain aligned as such. So what we are doing here in-kernel fixups
is doing the same alignment fixups which the BIOS would have done. The
summary here is that e820 table adjustments for RMP table done either by
BIOS and/or kernel will exist/remain even if SNP is not enabled by the
kernel.
Thanks, Ashish
On 4/2/2024 4:00 PM, Kalra, Ashish wrote:
>
> On 4/2/2024 3:21 PM, Borislav Petkov wrote:
>> On Tue, Apr 02, 2024 at 02:33:44PM -0500, Kalra, Ashish wrote:
>>> And we can't do this in snp_rmptable_init() as e820_table_firmware
>>> can't be
>>> fixed at that point and by that time this table has been mapped into
>>> sysfs
>>> (/sys/firmware) which is used by kexec -c variant.
>> Well, you have to do something here because if snp_rmptable_init()
>> late-disables SNP, your RMP table fixups are moot and invalid.
>>
>> Which means, your RMP table fixups need to happen at the *very* *late*
>> step after we know that SNP is enabled and won't get disabled anymore.
>>
>> I.e., in snp_rmptable_init().
>
> The main issue with doing that in snp_rmptable_init() is that there is
> no e820 API interfaces available to update the e820_table_kexec and
> e820_table_firmware and e820_table_firmware has already been exposed
> to sysfs.
>
> The e820 API only exports e820__range_update() which *only* fixes
> e820_table.
>
> The important point to note here is that in most cases BIOS would have
> reserved RMP table start and end aligned to 2M boundary and setup the
> e820 table which the BIOS passes to the kernel as such, so even if the
> kernel does not enable SNP or disables SNP later, these reservations
> will remain aligned as such. So what we are doing here in-kernel
> fixups is doing the same alignment fixups which the BIOS would have
> done. The summary here is that e820 table adjustments for RMP table
> done either by BIOS and/or kernel will exist/remain even if SNP is not
> enabled by the kernel.
>
Again, to reiterate here, RMP table memory is reserved by BIOS
regardless of the kernel enabling SNP (and also passed on the e820 map
to ensure that kernel does not map anything in that memory), so any
adjustments/fixups on top of that reserved memory should not matter,
after all we don't free this reserved RMP table memory if kernel does
not enable SNP.
Thanks, Ashish
On Tue, Apr 02, 2024 at 04:00:03PM -0500, Kalra, Ashish wrote:
> The main issue with doing that in snp_rmptable_init() is that there is no
> e820 API interfaces available to update the e820_table_kexec and
> e820_table_firmware and e820_table_firmware has already been exposed to
> sysfs.
And?
You can't change it later? Tried?
> The e820 API only exports e820__range_update() which *only* fixes
> e820_table.
>
> The important point to note here is that in most cases BIOS would have
> reserved RMP table start and end aligned to 2M boundary and setup the e820
> table which the BIOS passes to the kernel as such,
So what was this "RMP table start and end physical range may not be
aligned to 2MB" in your commit message?
/me is completely confused now.
Or does "most cases" mean that there can be 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 there's RMP faults?
And you want to allocate those chunks up to the 2M boundary
unconditionally, regardless of SNP enablement?
Now look at your original commit message and tell me how much of what
came out on this thread, was in it?
Not a lot, I'd say...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/2/2024 4:20 PM, Borislav Petkov wrote:
> On Tue, Apr 02, 2024 at 04:00:03PM -0500, Kalra, Ashish wrote:
>> The main issue with doing that in snp_rmptable_init() is that there is no
>> e820 API interfaces available to update the e820_table_kexec and
>> e820_table_firmware and e820_table_firmware has already been exposed to
>> sysfs.
> And?
>
> You can't change it later? Tried?
The main issue is there is no API interface available to do that, i will
need to add new API interfaces to update the e820_table_kexec and
e820_table_firmware and then will that be acceptable for a use case
which can be handled via a platform specific quirk ?
>> The e820 API only exports e820__range_update() which *only* fixes
>> e820_table.
>>
>> The important point to note here is that in most cases BIOS would have
>> reserved RMP table start and end aligned to 2M boundary and setup the e820
>> table which the BIOS passes to the kernel as such,
> So what was this "RMP table start and end physical range may not be
> aligned to 2MB" in your commit message?
> /me is completely confused now.
>
> Or does "most cases" mean that there can be 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 there's RMP faults?
Yes exactly, that's what the above comment means.
That's why the above commit message says "may".
>
> And you want to allocate those chunks up to the 2M boundary
> unconditionally, regardless of SNP enablement?
My point is that we always keep the RMP table memory reserved regardless
of SNP enablement, so these are simply fixups/adjustments on top of that
reservation.
Thanks, Ashish
> Now look at your original commit message and tell me how much of what
> came out on this thread, was in it?
>
> Not a lot, I'd say...
>
On Tue, Apr 02, 2024 at 04:31:30PM -0500, Kalra, Ashish wrote:
> The main issue is there is no API interface available to do that, i will
> need to add new API interfaces to update the e820_table_kexec and
> e820_table_firmware and then will that be acceptable for a use case which
> can be handled via a platform specific quirk ?
I can't review a patch without seeing it first, sorry.
> Yes exactly, that's what the above comment means.
>
> That's why the above commit message says "may".
Yah, *never* use "may" in commit messages. This is internal speak and
doesn't belong in the kernel. Your commit message need to explain stuff
fully and properly. Otherwise I'll keep asking until it does.
> My point is that we always keep the RMP table memory reserved regardless of
> SNP enablement, so these are simply fixups/adjustments on top of that
> reservation.
Yes.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 3/12/24 13:47, Ashish Kalra wrote:
> From: Ashish Kalra <[email protected]>
>
> RMP table start and end physical range may not be aligned to 2MB in
> the e820 tables causing fatal RMP page faults during kexec boot when
> new page allocations are done in the same 2MB page as the RMP table.
> Check if RMP table start and 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.
>
> Override e820__memory_setup_default() to check and apply these RMP table
> fixups in e820_table before e820_table is used to setup
> e280_table_firmware and e820_table_kexec.
>
> Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")
> Signed-off-by: Ashish Kalra <[email protected]>
> ---
> arch/x86/virt/svm/sev.c | 52 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index cffe1157a90a..e0d7584df28f 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -65,6 +65,8 @@ static u64 probed_rmp_base, probed_rmp_size;
> static struct rmpentry *rmptable __ro_after_init;
> static u64 rmptable_max_pfn __ro_after_init;
>
> +static char *__init snp_rmptable_e820_fixup(void);
> +
> static LIST_HEAD(snp_leaked_pages_list);
> static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
>
> @@ -160,9 +162,59 @@ bool snp_probe_rmptable_info(void)
> pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
> probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
>
> + /*
> + * Override e820__memory_setup_default() to do any RMP table fixups
> + * for kexec if required.
> + */
> + x86_init.resources.memory_setup = snp_rmptable_e820_fixup;
This produces a build warning:
WARNING: modpost: vmlinux: section mismatch in reference: snp_probe_rmptable_info+0x95 (section: .text) -> x86_init (section: .init.data)
WARNING: modpost: vmlinux: section mismatch in reference: snp_probe_rmptable_info+0x99 (section: .text) -> snp_rmptable_e820_fixup (section: .init.text)
Thanks,
Tom
> +
> return true;
> }
On 4/2/2024 5:09 PM, Tom Lendacky wrote:
> On 3/12/24 13:47, Ashish Kalra wrote:
>> From: Ashish Kalra <[email protected]>
>>
>> RMP table start and end physical range may not be aligned to 2MB in
>> the e820 tables causing fatal RMP page faults during kexec boot when
>> new page allocations are done in the same 2MB page as the RMP table.
>> Check if RMP table start and 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.
>>
>> Override e820__memory_setup_default() to check and apply these RMP table
>> fixups in e820_table before e820_table is used to setup
>> e280_table_firmware and e820_table_kexec.
>>
>> Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU
>> feature")
>> Signed-off-by: Ashish Kalra <[email protected]>
>> ---
>> arch/x86/virt/svm/sev.c | 52 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 52 insertions(+)
>>
>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>> index cffe1157a90a..e0d7584df28f 100644
>> --- a/arch/x86/virt/svm/sev.c
>> +++ b/arch/x86/virt/svm/sev.c
>> @@ -65,6 +65,8 @@ static u64 probed_rmp_base, probed_rmp_size;
>> static struct rmpentry *rmptable __ro_after_init;
>> static u64 rmptable_max_pfn __ro_after_init;
>> +static char *__init snp_rmptable_e820_fixup(void);
>> +
>> static LIST_HEAD(snp_leaked_pages_list);
>> static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
>> @@ -160,9 +162,59 @@ bool snp_probe_rmptable_info(void)
>> pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
>> probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
>> + /*
>> + * Override e820__memory_setup_default() to do any RMP table fixups
>> + * for kexec if required.
>> + */
>> + x86_init.resources.memory_setup = snp_rmptable_e820_fixup;
>
> This produces a build warning:
>
> WARNING: modpost: vmlinux: section mismatch in reference:
> snp_probe_rmptable_info+0x95 (section: .text) -> x86_init (section:
> .init.data)
> WARNING: modpost: vmlinux: section mismatch in reference:
> snp_probe_rmptable_info+0x99 (section: .text) ->
> snp_rmptable_e820_fixup (section: .init.text)
>
Oh, so this requires snp_probe_rmptable_info() to be fixed to use the
__init macro.
I believe that snp_probe_rmptable_info() should be anyway using the
__init macro and this fix for snp_probe_rmptable_info() needs to be sent
as a separate patch and regardless of this patch getting merged or not.
Thanks, Ashish
> Thanks,
> Tom
>
>> +
>> return true;
>> }
On 4/2/2024 5:42 PM, Michael Roth wrote:
> On Tue, Apr 02, 2024 at 05:31:09PM -0500, Kalra, Ashish wrote:
>> On 4/2/2024 5:09 PM, Tom Lendacky wrote:
>>> On 3/12/24 13:47, Ashish Kalra wrote:
>>>> From: Ashish Kalra <[email protected]>
>>>>
>>>> RMP table start and end physical range may not be aligned to 2MB in
>>>> the e820 tables causing fatal RMP page faults during kexec boot when
>>>> new page allocations are done in the same 2MB page as the RMP table.
>>>> Check if RMP table start and 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.
>>>>
>>>> Override e820__memory_setup_default() to check and apply these RMP table
>>>> fixups in e820_table before e820_table is used to setup
>>>> e280_table_firmware and e820_table_kexec.
>>>>
>>>> Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU
>>>> feature")
>>>> Signed-off-by: Ashish Kalra <[email protected]>
>>>> ---
>>>> arch/x86/virt/svm/sev.c | 52 +++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 52 insertions(+)
>>>>
>>>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>>>> index cffe1157a90a..e0d7584df28f 100644
>>>> --- a/arch/x86/virt/svm/sev.c
>>>> +++ b/arch/x86/virt/svm/sev.c
>>>> @@ -65,6 +65,8 @@ static u64 probed_rmp_base, probed_rmp_size;
>>>> static struct rmpentry *rmptable __ro_after_init;
>>>> static u64 rmptable_max_pfn __ro_after_init;
>>>> +static char *__init snp_rmptable_e820_fixup(void);
>>>> +
>>>> static LIST_HEAD(snp_leaked_pages_list);
>>>> static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
>>>> @@ -160,9 +162,59 @@ bool snp_probe_rmptable_info(void)
>>>> pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
>>>> probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
>>>> + /*
>>>> + * Override e820__memory_setup_default() to do any RMP table fixups
>>>> + * for kexec if required.
>>>> + */
>>>> + x86_init.resources.memory_setup = snp_rmptable_e820_fixup;
>>> This produces a build warning:
>>>
>>> WARNING: modpost: vmlinux: section mismatch in reference:
>>> snp_probe_rmptable_info+0x95 (section: .text) -> x86_init (section:
>>> .init.data)
>>> WARNING: modpost: vmlinux: section mismatch in reference:
>>> snp_probe_rmptable_info+0x99 (section: .text) -> snp_rmptable_e820_fixup
>>> (section: .init.text)
>>>
>> Oh, so this requires snp_probe_rmptable_info() to be fixed to use the __init
>> macro.
>>
>> I believe that snp_probe_rmptable_info() should be anyway using the __init
>> macro and this fix for snp_probe_rmptable_info() needs to be sent as a
>> separate patch and regardless of this patch getting merged or not.
> I think you'll hit issues with:
>
> bsp_determine_snp() -> //non-__init
> snp_probe_rmptable_info() //__init
>
> and bsp_determine_snp() sticks around as a function pointer assigned to
> cpuinfo_x86 so I don't think you can use __init there.
>
> So might need to just drop __init from snp_rmptable_e820_fixup().
Actually, that will not help as snp_probe_rmptable_info() is *also*
accessing x86_init.resources.memory_setup
Thanks, Ashish
>
>> Thanks, Ashish
>>
>>> Thanks,
>>> Tom
>>>
>>>> +
>>>> return true;
>>>> }
On Wed, Apr 03, 2024 at 04:08:35PM -0500, Kalra, Ashish wrote:
>
> On 4/2/2024 5:42 PM, Michael Roth wrote:
> >On Tue, Apr 02, 2024 at 05:31:09PM -0500, Kalra, Ashish wrote:
> >>On 4/2/2024 5:09 PM, Tom Lendacky wrote:
> >>>On 3/12/24 13:47, Ashish Kalra wrote:
> >>>>From: Ashish Kalra <[email protected]>
> >>>>
> >>>>RMP table start and end physical range may not be aligned to 2MB in
> >>>>the e820 tables causing fatal RMP page faults during kexec boot when
> >>>>new page allocations are done in the same 2MB page as the RMP table.
> >>>>Check if RMP table start and 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.
> >>>>
> >>>>Override e820__memory_setup_default() to check and apply these RMP table
> >>>>fixups in e820_table before e820_table is used to setup
> >>>>e280_table_firmware and e820_table_kexec.
> >>>>
> >>>>Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU
> >>>>feature")
> >>>>Signed-off-by: Ashish Kalra <[email protected]>
> >>>>---
> >>>> arch/x86/virt/svm/sev.c | 52 +++++++++++++++++++++++++++++++++++++++++
> >>>> 1 file changed, 52 insertions(+)
> >>>>
> >>>>diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> >>>>index cffe1157a90a..e0d7584df28f 100644
> >>>>--- a/arch/x86/virt/svm/sev.c
> >>>>+++ b/arch/x86/virt/svm/sev.c
> >>>>@@ -65,6 +65,8 @@ static u64 probed_rmp_base, probed_rmp_size;
> >>>> static struct rmpentry *rmptable __ro_after_init;
> >>>> static u64 rmptable_max_pfn __ro_after_init;
> >>>> +static char *__init snp_rmptable_e820_fixup(void);
> >>>>+
> >>>> static LIST_HEAD(snp_leaked_pages_list);
> >>>> static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
> >>>> @@ -160,9 +162,59 @@ bool snp_probe_rmptable_info(void)
> >>>> pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
> >>>> probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
> >>>> + /*
> >>>>+ * Override e820__memory_setup_default() to do any RMP table fixups
> >>>>+ * for kexec if required.
> >>>>+ */
> >>>>+ x86_init.resources.memory_setup = snp_rmptable_e820_fixup;
> >>>This produces a build warning:
> >>>
> >>>WARNING: modpost: vmlinux: section mismatch in reference:
> >>>snp_probe_rmptable_info+0x95 (section: .text) -> x86_init (section:
> >>>.init.data)
> >>>WARNING: modpost: vmlinux: section mismatch in reference:
> >>>snp_probe_rmptable_info+0x99 (section: .text) -> snp_rmptable_e820_fixup
> >>>(section: .init.text)
> >>>
> >>Oh, so this requires snp_probe_rmptable_info() to be fixed to use the __init
> >>macro.
> >>
> >>I believe that snp_probe_rmptable_info() should be anyway using the __init
> >>macro and this fix for snp_probe_rmptable_info() needs to be sent as a
> >>separate patch and regardless of this patch getting merged or not.
> >I think you'll hit issues with:
> >
> > bsp_determine_snp() -> //non-__init
> > snp_probe_rmptable_info() //__init
> >
> >and bsp_determine_snp() sticks around as a function pointer assigned to
> >cpuinfo_x86 so I don't think you can use __init there.
> >
> >So might need to just drop __init from snp_rmptable_e820_fixup().
>
> Actually, that will not help as snp_probe_rmptable_info() is *also*
> accessing x86_init.resources.memory_setup
>
What if we flipped the whole flow? Borislav is adding a CC_ATTR to indicate
HOST_SEV_SNP support, we don't need to clear X86_FEATURE_SEV_SNP at this phase
(or at all for that matter). snp_probe_rmptable_info() can be done later
during kernel init, once e820 has been parsed.
One way of doing this would be to override x86_init.resources.memory_setup()
to do both snp_probe_rmptable_info() and snp_rmptable_e820_fixup().
What do you think?
Hello Jeremi,
On 4/4/2024 3:17 AM, Jeremi Piotrowski wrote:
> On Wed, Apr 03, 2024 at 04:08:35PM -0500, Kalra, Ashish wrote:
>> On 4/2/2024 5:42 PM, Michael Roth wrote:
>>> On Tue, Apr 02, 2024 at 05:31:09PM -0500, Kalra, Ashish wrote:
>>>> On 4/2/2024 5:09 PM, Tom Lendacky wrote:
>>>>> On 3/12/24 13:47, Ashish Kalra wrote:
>>>>>> From: Ashish Kalra <[email protected]>
>>>>>>
>>>>>> RMP table start and end physical range may not be aligned to 2MB in
>>>>>> the e820 tables causing fatal RMP page faults during kexec boot when
>>>>>> new page allocations are done in the same 2MB page as the RMP table.
>>>>>> Check if RMP table start and 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.
>>>>>>
>>>>>> Override e820__memory_setup_default() to check and apply these RMP table
>>>>>> fixups in e820_table before e820_table is used to setup
>>>>>> e280_table_firmware and e820_table_kexec.
>>>>>>
>>>>>> Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU
>>>>>> feature")
>>>>>> Signed-off-by: Ashish Kalra <[email protected]>
>>>>>> ---
>>>>>> arch/x86/virt/svm/sev.c | 52 +++++++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 52 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>>>>>> index cffe1157a90a..e0d7584df28f 100644
>>>>>> --- a/arch/x86/virt/svm/sev.c
>>>>>> +++ b/arch/x86/virt/svm/sev.c
>>>>>> @@ -65,6 +65,8 @@ static u64 probed_rmp_base, probed_rmp_size;
>>>>>> static struct rmpentry *rmptable __ro_after_init;
>>>>>> static u64 rmptable_max_pfn __ro_after_init;
>>>>>> +static char *__init snp_rmptable_e820_fixup(void);
>>>>>> +
>>>>>> static LIST_HEAD(snp_leaked_pages_list);
>>>>>> static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
>>>>>> @@ -160,9 +162,59 @@ bool snp_probe_rmptable_info(void)
>>>>>> pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
>>>>>> probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
>>>>>> + /*
>>>>>> + * Override e820__memory_setup_default() to do any RMP table fixups
>>>>>> + * for kexec if required.
>>>>>> + */
>>>>>> + x86_init.resources.memory_setup = snp_rmptable_e820_fixup;
>>>>> This produces a build warning:
>>>>>
>>>>> WARNING: modpost: vmlinux: section mismatch in reference:
>>>>> snp_probe_rmptable_info+0x95 (section: .text) -> x86_init (section:
>>>>> .init.data)
>>>>> WARNING: modpost: vmlinux: section mismatch in reference:
>>>>> snp_probe_rmptable_info+0x99 (section: .text) -> snp_rmptable_e820_fixup
>>>>> (section: .init.text)
>>>>>
>>>> Oh, so this requires snp_probe_rmptable_info() to be fixed to use the __init
>>>> macro.
>>>>
>>>> I believe that snp_probe_rmptable_info() should be anyway using the __init
>>>> macro and this fix for snp_probe_rmptable_info() needs to be sent as a
>>>> separate patch and regardless of this patch getting merged or not.
>>> I think you'll hit issues with:
>>>
>>> bsp_determine_snp() -> //non-__init
>>> snp_probe_rmptable_info() //__init
>>>
>>> and bsp_determine_snp() sticks around as a function pointer assigned to
>>> cpuinfo_x86 so I don't think you can use __init there.
>>>
>>> So might need to just drop __init from snp_rmptable_e820_fixup().
>> Actually, that will not help as snp_probe_rmptable_info() is *also*
>> accessing x86_init.resources.memory_setup
>>
> What if we flipped the whole flow? Borislav is adding a CC_ATTR to indicate
> HOST_SEV_SNP support, we don't need to clear X86_FEATURE_SEV_SNP at this phase
> (or at all for that matter). snp_probe_rmptable_info() can be done later
> during kernel init, once e820 has been parsed.
>
> One way of doing this would be to override x86_init.resources.memory_setup()
> to do both snp_probe_rmptable_info() and snp_rmptable_e820_fixup().
>
> What do you think?
I like this idea of overriding x86_init.resources_memory_setup() to do
both snp_probe_rmptable_info() and snp_rmptable_e820_fixup().
This callback seems to be meant for platform specific memory setup and
SNP enabled RMP table platforms seem to be a good candidate to use this
callback to probe RMP table, sanitize it and then apply e820 fixups or
adjustments specific to the RMP table.
Additionally. setup_arch() -> e820__memory_setup() initializes all the
three e820 tables (including the two tables used for kexec -
e820_table_kexec and e820_table_firmware) and then there is *no* e820
API functions available to update e820_table_kexec and
e820_table_firmware. Even if i add new e820 API interfaces to allow e820
memmap updates to e820_table_kexec and e820_table_firmware, before
setup_arch() returns it calls e820__reserve_resources() which exposes
the e820_table_firmware to sysfs (/sys/firmware/memmap) which is then
used directly by kexec-tools.
I don't see any callbacks i can use to call any newly added e820 API to
update e820_table_firmware before it is exposed to sysfs between
setup_arch()->e820__memory_setup() and
setup_arch()->e820__reserve_resources().
For the same reason, i tried to use initcalls to invoke
snp_rmptable_e820_fixup() but then initcalls happen after setup_arch()
and by that time e820_table_firmware has already been exposed to sysfs.
That's why i see x86_init.resources_memory_setup() an ideal callback to
probe RMP table itself and then apply any RMP table platform specific
quirks.
Thanks, Ashish
On Tue, Apr 02, 2024 at 05:31:09PM -0500, Kalra, Ashish wrote:
>
> On 4/2/2024 5:09 PM, Tom Lendacky wrote:
> > On 3/12/24 13:47, Ashish Kalra wrote:
> > > From: Ashish Kalra <[email protected]>
> > >
> > > RMP table start and end physical range may not be aligned to 2MB in
> > > the e820 tables causing fatal RMP page faults during kexec boot when
> > > new page allocations are done in the same 2MB page as the RMP table.
> > > Check if RMP table start and 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.
> > >
> > > Override e820__memory_setup_default() to check and apply these RMP table
> > > fixups in e820_table before e820_table is used to setup
> > > e280_table_firmware and e820_table_kexec.
> > >
> > > Fixes: c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU
> > > feature")
> > > Signed-off-by: Ashish Kalra <[email protected]>
> > > ---
> > > ? arch/x86/virt/svm/sev.c | 52 +++++++++++++++++++++++++++++++++++++++++
> > > ? 1 file changed, 52 insertions(+)
> > >
> > > diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> > > index cffe1157a90a..e0d7584df28f 100644
> > > --- a/arch/x86/virt/svm/sev.c
> > > +++ b/arch/x86/virt/svm/sev.c
> > > @@ -65,6 +65,8 @@ static u64 probed_rmp_base, probed_rmp_size;
> > > ? static struct rmpentry *rmptable __ro_after_init;
> > > ? static u64 rmptable_max_pfn __ro_after_init;
> > > ? +static char *__init snp_rmptable_e820_fixup(void);
> > > +
> > > ? static LIST_HEAD(snp_leaked_pages_list);
> > > ? static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
> > > ? @@ -160,9 +162,59 @@ bool snp_probe_rmptable_info(void)
> > > ????? pr_info("RMP table physical range [0x%016llx - 0x%016llx]\n",
> > > ????????? probed_rmp_base, probed_rmp_base + probed_rmp_size - 1);
> > > ? +??? /*
> > > +???? * Override e820__memory_setup_default() to do any RMP table fixups
> > > +???? * for kexec if required.
> > > +???? */
> > > +??? x86_init.resources.memory_setup = snp_rmptable_e820_fixup;
> >
> > This produces a build warning:
> >
> > WARNING: modpost: vmlinux: section mismatch in reference:
> > snp_probe_rmptable_info+0x95 (section: .text) -> x86_init (section:
> > .init.data)
> > WARNING: modpost: vmlinux: section mismatch in reference:
> > snp_probe_rmptable_info+0x99 (section: .text) -> snp_rmptable_e820_fixup
> > (section: .init.text)
> >
> Oh, so this requires snp_probe_rmptable_info() to be fixed to use the __init
> macro.
>
> I believe that snp_probe_rmptable_info() should be anyway using the __init
> macro and this fix for snp_probe_rmptable_info() needs to be sent as a
> separate patch and regardless of this patch getting merged or not.
I think you'll hit issues with:
bsp_determine_snp() -> //non-__init
snp_probe_rmptable_info() //__init
and bsp_determine_snp() sticks around as a function pointer assigned to
cpuinfo_x86 so I don't think you can use __init there.
So might need to just drop __init from snp_rmptable_e820_fixup().
-Mike
>
> Thanks, Ashish
>
> > Thanks,
> > Tom
> >
> > > +
> > > ????? return true;
> > > ? }