From: "Kalra, Ashish" <ashish.kalra@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: thomas.lendacky@amd.com, michael.roth@amd.com, x86@kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.
Date: Wed, 24 Apr 2024 18:48:08 -0500 [thread overview]
Message-ID: <ed4cb373-e626-4b79-b692-df5ea2ca8899@amd.com> (raw)
In-Reply-To: <20240420130533.GNZiO9nShSxjxB-FQn@fat_crate.local>
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
next prev parent reply other threads:[~2024-04-24 23:48 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-15 21:08 [PATCH v2 0/2] Apply RMP table fixups for kexec Ashish Kalra
2024-04-15 21:09 ` [PATCH v2 1/2] x86/e820: Expose API to update e820 kexec and firmware tables externally Ashish Kalra
2024-04-20 11:20 ` Borislav Petkov
2024-04-15 21:09 ` [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec Ashish Kalra
2024-04-20 13:05 ` Borislav Petkov
2024-04-24 23:48 ` Kalra, Ashish [this message]
2024-04-26 12:58 ` Borislav Petkov
2024-04-26 14:56 ` Kalra, Ashish
2024-04-20 11:08 ` [PATCH v2 0/2] Apply " Borislav Petkov
2024-04-24 19:10 ` Kalra, Ashish
2024-04-26 12:49 ` Borislav Petkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ed4cb373-e626-4b79-b692-df5ea2ca8899@amd.com \
--to=ashish.kalra@amd.com \
--cc=bp@alien8.de \
--cc=linux-kernel@vger.kernel.org \
--cc=michael.roth@amd.com \
--cc=thomas.lendacky@amd.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).