| Message ID | 20250504095230.2932860-30-ardb+git@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [tip:,x86/boot] x86/sev: Move instruction decoder into separate source file | expand |
On 5/4/25 04:52, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > As a first step towards disentangling the SEV #VC handling code -which > is shared between the decompressor and the core kernel- from the SEV > startup code, move the decompressor's copy of the instruction decoder > into a separate source file. > > Code movement only - no functional change intended. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/x86/boot/compressed/Makefile | 6 +-- > arch/x86/boot/compressed/misc.h | 7 +++ > arch/x86/boot/compressed/sev-handle-vc.c | 51 ++++++++++++++++++++ > arch/x86/boot/compressed/sev.c | 39 +-------------- > 4 files changed, 62 insertions(+), 41 deletions(-) > > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile > index 0fcad7b7e007..f4f7b22d8113 100644 > --- a/arch/x86/boot/compressed/Makefile > +++ b/arch/x86/boot/compressed/Makefile > @@ -44,10 +44,10 @@ KBUILD_CFLAGS += -D__DISABLE_EXPORTS > KBUILD_CFLAGS += $(call cc-option,-Wa$(comma)-mrelax-relocations=no) > KBUILD_CFLAGS += -include $(srctree)/include/linux/hidden.h > > -# sev.c indirectly includes inat-table.h which is generated during > +# sev-decode-insn.c indirectly includes inat-table.c which is generated during did you mean sev-handle-vc.c ? Thanks, Tom > # compilation and stored in $(objtree). Add the directory to the includes so > # that the compiler finds it even with out-of-tree builds (make O=/some/path). > -CFLAGS_sev.o += -I$(objtree)/arch/x86/lib/ > +CFLAGS_sev-handle-vc.o += -I$(objtree)/arch/x86/lib/ > > KBUILD_AFLAGS := $(KBUILD_CFLAGS) -D__ASSEMBLY__ > > @@ -96,7 +96,7 @@ ifdef CONFIG_X86_64 > vmlinux-objs-y += $(obj)/idt_64.o $(obj)/idt_handlers_64.o > vmlinux-objs-$(CONFIG_AMD_MEM_ENCRYPT) += $(obj)/mem_encrypt.o > vmlinux-objs-y += $(obj)/pgtable_64.o > - vmlinux-objs-$(CONFIG_AMD_MEM_ENCRYPT) += $(obj)/sev.o > + vmlinux-objs-$(CONFIG_AMD_MEM_ENCRYPT) += $(obj)/sev.o $(obj)/sev-handle-vc.o > endif > > vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
On Mon, 5 May 2025 at 16:48, Tom Lendacky <thomas.lendacky@amd.com> wrote: > > On 5/4/25 04:52, Ard Biesheuvel wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > > > As a first step towards disentangling the SEV #VC handling code -which > > is shared between the decompressor and the core kernel- from the SEV > > startup code, move the decompressor's copy of the instruction decoder > > into a separate source file. > > > > Code movement only - no functional change intended. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > arch/x86/boot/compressed/Makefile | 6 +-- > > arch/x86/boot/compressed/misc.h | 7 +++ > > arch/x86/boot/compressed/sev-handle-vc.c | 51 ++++++++++++++++++++ > > arch/x86/boot/compressed/sev.c | 39 +-------------- > > 4 files changed, 62 insertions(+), 41 deletions(-) > > > > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile > > index 0fcad7b7e007..f4f7b22d8113 100644 > > --- a/arch/x86/boot/compressed/Makefile > > +++ b/arch/x86/boot/compressed/Makefile > > @@ -44,10 +44,10 @@ KBUILD_CFLAGS += -D__DISABLE_EXPORTS > > KBUILD_CFLAGS += $(call cc-option,-Wa$(comma)-mrelax-relocations=no) > > KBUILD_CFLAGS += -include $(srctree)/include/linux/hidden.h > > > > -# sev.c indirectly includes inat-table.h which is generated during > > +# sev-decode-insn.c indirectly includes inat-table.c which is generated during > > did you mean sev-handle-vc.c ? > Ah yes - I renamed that at some point and forgot to update the comment.
On Sun, May 04, 2025 at 11:52:35AM +0200, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > As a first step towards disentangling the SEV #VC handling code -which > is shared between the decompressor and the core kernel- from the SEV > startup code, move the decompressor's copy of the instruction decoder > into a separate source file. Why? I'm still unclear why that happens. I'd like to read some blurb in those commit messages which explains the big picture: the insn decoder bits are going to be in the bla mapping because... , and because... and this is wonderful because ... Thx.
On Wed, 7 May 2025 at 11:58, Borislav Petkov <bp@alien8.de> wrote: > > On Sun, May 04, 2025 at 11:52:35AM +0200, Ard Biesheuvel wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > > > As a first step towards disentangling the SEV #VC handling code -which > > is shared between the decompressor and the core kernel- from the SEV > > startup code, move the decompressor's copy of the instruction decoder > > into a separate source file. > > Why? > > I'm still unclear why that happens. > > I'd like to read some blurb in those commit messages which explains the big > picture: the insn decoder bits are going to be in the bla mapping because... > , and because... and this is wonderful because ... > Sure, I can add some more prose. I'll add something along the lines of "Some of the SEV code that is shared between the decompressor and the kernel proper runs very early in the latter, and therefore needs to be built in a special way. This does not apply to all of that shared code, though - some is used both by the decompressor, and by the kernel proper but at a much later stage. That code can be built as ordinary, position dependent code with instrumentations enabled etc etc. The #VC handling machinery and the associated instruction decoder are conceptually separate from the SEV initialization code, and are never used on the early startup path in the core kernel. So start separating it from the SEV startup code, by moving the decompressor's copy of the instruction decoder to a separate source file. In a subsequent patch, the shared #VC handling code will be moved into a separate shared source file, which will be included here too and no longer into sev.c. That way, it no longer gets included into the early SEV startup code, and can be built in the ordinary way." Does that help?
On Wed, May 07, 2025 at 01:49:19PM +0200, Ard Biesheuvel wrote: > Sure, I can add some more prose. I'll add something along the lines of > > "Some of the SEV code that is shared between the decompressor and the > kernel proper runs very early in the latter, and therefore needs to be > built in a special way. This does not apply to all of that shared > code, though - some is used both by the decompressor, and by the > kernel proper but at a much later stage. That code can be built as > ordinary, position dependent code with instrumentations enabled etc > etc. > > The #VC handling machinery and the associated instruction decoder are > conceptually separate from the SEV initialization code, and are never > used on the early startup path in the core kernel. So start separating > it from the SEV startup code, by moving the decompressor's copy of the > instruction decoder to a separate source file. In a subsequent patch, > the shared #VC handling code will be moved into a separate shared > source file, which will be included here too and no longer into sev.c. > That way, it no longer gets included into the early SEV startup code, > and can be built in the ordinary way." > > Does that help? Yap, definitely. So the logic is, everything in startup/ and everything that startup/ *includes* is going to end up being PIC and the rest is ordinary. I guess that's one rule to separate it on. Thanks!
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index 0fcad7b7e007..f4f7b22d8113 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -44,10 +44,10 @@ KBUILD_CFLAGS += -D__DISABLE_EXPORTS KBUILD_CFLAGS += $(call cc-option,-Wa$(comma)-mrelax-relocations=no) KBUILD_CFLAGS += -include $(srctree)/include/linux/hidden.h -# sev.c indirectly includes inat-table.h which is generated during +# sev-decode-insn.c indirectly includes inat-table.c which is generated during # compilation and stored in $(objtree). Add the directory to the includes so # that the compiler finds it even with out-of-tree builds (make O=/some/path). -CFLAGS_sev.o += -I$(objtree)/arch/x86/lib/ +CFLAGS_sev-handle-vc.o += -I$(objtree)/arch/x86/lib/ KBUILD_AFLAGS := $(KBUILD_CFLAGS) -D__ASSEMBLY__ @@ -96,7 +96,7 @@ ifdef CONFIG_X86_64 vmlinux-objs-y += $(obj)/idt_64.o $(obj)/idt_handlers_64.o vmlinux-objs-$(CONFIG_AMD_MEM_ENCRYPT) += $(obj)/mem_encrypt.o vmlinux-objs-y += $(obj)/pgtable_64.o - vmlinux-objs-$(CONFIG_AMD_MEM_ENCRYPT) += $(obj)/sev.o + vmlinux-objs-$(CONFIG_AMD_MEM_ENCRYPT) += $(obj)/sev.o $(obj)/sev-handle-vc.o endif vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h index 450d27d0f449..ccd3f4257bcd 100644 --- a/arch/x86/boot/compressed/misc.h +++ b/arch/x86/boot/compressed/misc.h @@ -133,6 +133,9 @@ static inline void console_init(void) #endif #ifdef CONFIG_AMD_MEM_ENCRYPT +struct es_em_ctxt; +struct insn; + void sev_enable(struct boot_params *bp); void snp_check_features(void); void sev_es_shutdown_ghcb(void); @@ -140,6 +143,10 @@ extern bool sev_es_check_ghcb_fault(unsigned long address); void snp_set_page_private(unsigned long paddr); void snp_set_page_shared(unsigned long paddr); void sev_prep_identity_maps(unsigned long top_level_pgt); + +enum es_result vc_decode_insn(struct es_em_ctxt *ctxt); +bool insn_has_rep_prefix(struct insn *insn); +void sev_insn_decode_init(void); #else static inline void sev_enable(struct boot_params *bp) { diff --git a/arch/x86/boot/compressed/sev-handle-vc.c b/arch/x86/boot/compressed/sev-handle-vc.c new file mode 100644 index 000000000000..b1aa073b732c --- /dev/null +++ b/arch/x86/boot/compressed/sev-handle-vc.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include "misc.h" + +#include <linux/kernel.h> +#include <linux/string.h> +#include <asm/insn.h> +#include <asm/pgtable_types.h> +#include <asm/ptrace.h> +#include <asm/sev.h> + +#define __BOOT_COMPRESSED + +/* Basic instruction decoding support needed */ +#include "../../lib/inat.c" +#include "../../lib/insn.c" + +/* + * Copy a version of this function here - insn-eval.c can't be used in + * pre-decompression code. + */ +bool insn_has_rep_prefix(struct insn *insn) +{ + insn_byte_t p; + int i; + + insn_get_prefixes(insn); + + for_each_insn_prefix(insn, i, p) { + if (p == 0xf2 || p == 0xf3) + return true; + } + + return false; +} + +enum es_result vc_decode_insn(struct es_em_ctxt *ctxt) +{ + char buffer[MAX_INSN_SIZE]; + int ret; + + memcpy(buffer, (unsigned char *)ctxt->regs->ip, MAX_INSN_SIZE); + + ret = insn_decode(&ctxt->insn, buffer, MAX_INSN_SIZE, INSN_MODE_64); + if (ret < 0) + return ES_DECODE_FAILED; + + return ES_OK; +} + +extern void sev_insn_decode_init(void) __alias(inat_init_tables); diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c index bc52c0aa96d4..5cd029c4f36d 100644 --- a/arch/x86/boot/compressed/sev.c +++ b/arch/x86/boot/compressed/sev.c @@ -29,25 +29,6 @@ static struct ghcb boot_ghcb_page __aligned(PAGE_SIZE); struct ghcb *boot_ghcb; -/* - * Copy a version of this function here - insn-eval.c can't be used in - * pre-decompression code. - */ -static bool insn_has_rep_prefix(struct insn *insn) -{ - insn_byte_t p; - int i; - - insn_get_prefixes(insn); - - for_each_insn_prefix(insn, i, p) { - if (p == 0xf2 || p == 0xf3) - return true; - } - - return false; -} - /* * Only a dummy for insn_get_seg_base() - Early boot-code is 64bit only and * doesn't use segments. @@ -74,20 +55,6 @@ static inline void sev_es_wr_ghcb_msr(u64 val) boot_wrmsr(MSR_AMD64_SEV_ES_GHCB, &m); } -static enum es_result vc_decode_insn(struct es_em_ctxt *ctxt) -{ - char buffer[MAX_INSN_SIZE]; - int ret; - - memcpy(buffer, (unsigned char *)ctxt->regs->ip, MAX_INSN_SIZE); - - ret = insn_decode(&ctxt->insn, buffer, MAX_INSN_SIZE, INSN_MODE_64); - if (ret < 0) - return ES_DECODE_FAILED; - - return ES_OK; -} - static enum es_result vc_write_mem(struct es_em_ctxt *ctxt, void *dst, char *buf, size_t size) { @@ -122,10 +89,6 @@ static bool fault_in_kernel_space(unsigned long address) #define __BOOT_COMPRESSED -/* Basic instruction decoding support needed */ -#include "../../lib/inat.c" -#include "../../lib/insn.c" - extern struct svsm_ca *boot_svsm_caa; extern u64 boot_svsm_caa_pa; @@ -230,7 +193,7 @@ static bool early_setup_ghcb(void) boot_ghcb = &boot_ghcb_page; /* Initialize lookup tables for the instruction decoder */ - inat_init_tables(); + sev_insn_decode_init(); /* SNP guest requires the GHCB GPA must be registered */ if (sev_snp_enabled())