Re: [PATCH v2 07/12] x86/sev: Setup code to park APs in the AP Jump Table
On Wed, Nov 10, 2021 at 05:37:32PM +0100, Borislav Petkov wrote: > On Mon, Sep 13, 2021 at 05:55:58PM +0200, Joerg Roedel wrote: > > extern unsigned char real_mode_blob[]; > > diff --git a/arch/x86/include/asm/sev-ap-jumptable.h > > b/arch/x86/include/asm/sev-ap-jumptable.h > > new file mode 100644 > > index ..1c8b2ce779e2 > > --- /dev/null > > +++ b/arch/x86/include/asm/sev-ap-jumptable.h > > Why a separate header? arch/x86/include/asm/sev.h looks small enough. The header is included in assembly, so I made a separate one. > > +void __init sev_es_setup_ap_jump_table_data(void *base, u32 pa) > > Why is this a separate function? > > It is all part of the jump table setup. Right, but the sev_es_setup_ap_jump_table_blob() function is already pretty big and I wanted to keep things readable. > > > + return 0; > > Why are you returning 0 here and below? This is in an initcall and it returns just 0 when the environment is not ready to setup the ap jump table. Returning non-zero would create a warning message in the caller for something that is not a bug in the kernel. > > + * This file contains the source code for the binary blob which gets > > copied to > > + * the SEV-ES AP Jumptable to park APs while offlining CPUs or booting a > > new > > I've seen "Jumptable", "Jump Table" and "jump table" at least. I'd say, do > the last one everywhere pls. Fair, sorry for my english being too german :) I changed everything to 'jump table'. > > + /* Reset remaining registers */ > > + movl$0, %esp > > + movl$0, %eax > > + movl$0, %ebx > > + movl$0, %edx > > All 4: use xor XOR changes EFLAGS, can not use them here. > > + > > + /* Reset CR0 to get out of protected mode */ > > + movl$0x6010, %ecx > > Another magic naked number. This is the CR0 reset value. I have updated the comment to make this more clear. Thanks, -- Jörg Rödel jroe...@suse.de SUSE Software Solutions Germany GmbH Maxfeldstr. 5 90409 Nürnberg Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 07/12] x86/sev: Setup code to park APs in the AP Jump Table
On Mon, Sep 13, 2021 at 05:55:58PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel
>
> The AP Jump Table under SEV-ES contains the reset vector where non-boot
> CPUs start executing when coming out of reset. This means that a CPU
> coming out of the AP-reset-hold VMGEXIT also needs to start executing at
> the reset vector stored in the AP Jump Table.
>
> The problem is to find a safe place to put the real-mode code which
> executes the VMGEXIT and jumps to the reset vector. The code can not be
> in kernel memory, because after kexec that memory is owned by the new
> kernel and the code might have been overwritten.
>
> Fortunately the AP Jump Table itself is a safe place, because the
> memory is not owned by the OS and will not be overwritten by a new
> kernel started through kexec. The table is 4k in size and only the
> first 4 bytes are used for the reset vector. This leaves enough space
> for some 16-bit code to do the job and even a small stack.
"The AP jump table must be 4K in size, in encrypted memory and it must
be 4K (page) aligned. There can only be one AP jump table and it should
reside in memory that has been marked as reserved by UEFI."
I think we need to state in the spec that some of that space can be used
by the OS so that future changes to the spec do not cause trouble.
> Install 16-bit code into the AP Jump Table under SEV-ES after the APs
> have been brought up. The code will do an AP-reset-hold VMGEXIT and jump
> to the reset vector after being woken up.
>
> Signed-off-by: Joerg Roedel
> ---
> arch/x86/include/asm/realmode.h | 2 +
> arch/x86/include/asm/sev-ap-jumptable.h | 25 +
> arch/x86/kernel/sev.c | 105 +++
> arch/x86/realmode/Makefile | 9 +-
> arch/x86/realmode/rmpiggy.S | 6 ++
> arch/x86/realmode/sev/Makefile | 41
> arch/x86/realmode/sev/ap_jump_table.S | 130
> arch/x86/realmode/sev/ap_jump_table.lds | 24 +
> 8 files changed, 341 insertions(+), 1 deletion(-)
> create mode 100644 arch/x86/include/asm/sev-ap-jumptable.h
> create mode 100644 arch/x86/realmode/sev/Makefile
> create mode 100644 arch/x86/realmode/sev/ap_jump_table.S
> create mode 100644 arch/x86/realmode/sev/ap_jump_table.lds
>
> diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
> index 5db5d083c873..29590a4ddf24 100644
> --- a/arch/x86/include/asm/realmode.h
> +++ b/arch/x86/include/asm/realmode.h
> @@ -62,6 +62,8 @@ extern unsigned long initial_gs;
> extern unsigned long initial_stack;
> #ifdef CONFIG_AMD_MEM_ENCRYPT
> extern unsigned long initial_vc_handler;
> +extern unsigned char rm_ap_jump_table_blob[];
> +extern unsigned char rm_ap_jump_table_blob_end[];
> #endif
>
> extern unsigned char real_mode_blob[];
> diff --git a/arch/x86/include/asm/sev-ap-jumptable.h
> b/arch/x86/include/asm/sev-ap-jumptable.h
> new file mode 100644
> index ..1c8b2ce779e2
> --- /dev/null
> +++ b/arch/x86/include/asm/sev-ap-jumptable.h
Why a separate header? arch/x86/include/asm/sev.h looks small enough.
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * AMD Encrypted Register State Support
> + *
> + * Author: Joerg Roedel
> + */
> +#ifndef __ASM_SEV_AP_JUMPTABLE_H
> +#define __ASM_SEV_AP_JUMPTABLE_H
> +
> +#define SEV_APJT_CS16 0x8
> +#define SEV_APJT_DS16 0x10
> +
> +#define SEV_APJT_ENTRY 0x10
> +
> +#ifndef __ASSEMBLY__
> +
> +struct sev_ap_jump_table_header {
> + u16 reset_ip;
> + u16 reset_cs;
> + u16 gdt_offset;
I guess you should state that the first two members are as the spec
mandates and cannot be moved around or changed or so.
Also, this gdt_offset thing looks like it wants to be ap_jumptable_gdt,
no?
> +};
> +
> +#endif /* !__ASSEMBLY__ */
> +
> +#endif /* __ASM_SEV_AP_JUMPTABLE_H */
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index eedba56b6bac..a98eab926682 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -19,6 +19,7 @@
> #include
> #include
>
> +#include
> #include
> #include
> #include
> @@ -45,6 +46,9 @@ static struct ghcb __initdata *boot_ghcb;
> /* Cached AP Jump Table Address */
> static phys_addr_t sev_es_jump_table_pa;
>
> +/* Whether the AP Jump Table blob was successfully installed */
> +static bool sev_ap_jumptable_blob_installed __ro_after_init;
> +
> /* #VC handler runtime per-CPU data */
> struct sev_es_runtime_data {
> struct ghcb ghcb_page;
> @@ -749,6 +753,107 @@ static void __init sev_es_setup_play_dead(void)
> static inline void sev_es_setup_play_dead(void) { }
> #endif
>
> +/*
> + * This function make the necessary runtime changes to the AP Jump Table
> blob.
s/This function make/Make/
Ditto for the other "This function" below.
> + * For now this only sets up the GDT used while the code executes. The GDT
> needs
> + * to contain 16-bit code and data segments
