[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240502134506.GDZjOY4guvlKH9-73J@fat_crate.local>
Date: Thu, 2 May 2024 15:45:06 +0200
From: Borislav Petkov <bp@...en8.de>
To: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc: adrian.hunter@...el.com, ashish.kalra@....com, bhe@...hat.com,
dave.hansen@...ux.intel.com, elena.reshetova@...el.com,
jun.nakajima@...el.com, kai.huang@...el.com,
kexec@...ts.infradead.org, linux-coco@...ts.linux.dev,
linux-kernel@...r.kernel.org, ltao@...hat.com, mingo@...hat.com,
nik.borisov@...e.com, peterz@...radead.org, rafael@...nel.org,
rick.p.edgecombe@...el.com,
sathyanarayanan.kuppuswamy@...ux.intel.com, seanjc@...gle.com,
tglx@...utronix.de, thomas.lendacky@....com, x86@...nel.org
Subject: Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted
memory for kexec
On Sat, Apr 27, 2024 at 08:06:34PM +0300, Kirill A. Shutemov wrote:
> Subject: Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted memory for kexec
s/Adding/Add/
> AMD SEV and Intel TDX guests allocate shared buffers for performing I/O.
> This is done by allocating pages normally from the buddy allocator and
> then converting them to shared using set_memory_decrypted().
>
> On kexec, the second kernel is unaware of which memory has been
> converted in this manner. It only sees E820_TYPE_RAM. Accessing shared
> memory as private is fatal.
>
> Therefore, the memory state must be reset to its original state before
> starting the new kernel with kexec.
>
> The process of converting shared memory back to private occurs in two
> steps:
>
> - enc_kexec_stop_conversion() stops new conversions.
>
> - enc_kexec_unshare_mem() unshares all existing shared memory, reverting
> it back to private.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@...e.com>x
> Reviewed-by: Kai Huang <kai.huang@...el.com>
> Tested-by: Tao Liu <ltao@...hat.com>
> ---
> arch/x86/include/asm/x86_init.h | 2 ++
> arch/x86/kernel/crash.c | 4 ++++
> arch/x86/kernel/reboot.c | 12 ++++++++++++
> arch/x86/kernel/x86_init.c | 4 ++++
> 4 files changed, 22 insertions(+)
>
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 28ac3cb9b987..c731e6bc4343 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -155,6 +155,8 @@ struct x86_guest {
> int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
> bool (*enc_tlb_flush_required)(bool enc);
> bool (*enc_cache_flush_required)(void);
> + void (*enc_kexec_stop_conversion)(bool crash);
> + void (*enc_kexec_unshare_mem)(void);
This is all fine and dandy but those functions need documentation in the
kernel doc above it.
And if there's a "stop_conversion" function, then I'd expect there to be
a "start conversion" one.
I think you need to give those two better names to denote that they're
related, the order in which they should be called and why they're
separate.
> };
>
> /**
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index e74d0c4286c1..f1b261be78b4 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -128,6 +128,10 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
> #ifdef CONFIG_HPET_TIMER
> hpet_disable();
> #endif
> +
> + x86_platform.guest.enc_kexec_stop_conversion(true);
> + x86_platform.guest.enc_kexec_unshare_mem();
> +
You call them here back-to-back...
> crash_save_cpu(regs, safe_smp_processor_id());
> }
>
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index f3130f762784..c1920ec34f0c 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -12,6 +12,7 @@
> #include <linux/delay.h>
> #include <linux/objtool.h>
> #include <linux/pgtable.h>
> +#include <linux/kexec.h>
> #include <acpi/reboot.h>
> #include <asm/io.h>
> #include <asm/apic.h>
> @@ -716,6 +717,14 @@ static void native_machine_emergency_restart(void)
>
> void native_machine_shutdown(void)
> {
> + /*
> + * Call enc_kexec_stop_conversion() while all CPUs are still active and
> + * interrupts are enabled. This will allow all in-flight memory
> + * conversions to finish cleanly.
> + */
> + if (kexec_in_progress)
> + x86_platform.guest.enc_kexec_stop_conversion(false);
> +
> /* Stop the cpus and apics */
> #ifdef CONFIG_X86_IO_APIC
> /*
> @@ -752,6 +761,9 @@ void native_machine_shutdown(void)
> #ifdef CONFIG_X86_64
> x86_platform.iommu_shutdown();
> #endif
> +
> + if (kexec_in_progress)
> + x86_platform.guest.enc_kexec_unshare_mem();
.. but they're split here.
And I don't know why and nothing tells me...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists