| Message ID | 20200714120917.11253-71-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: SEV-ES Guest Support | expand |
On Tue, Jul 14, 2020 at 02:09:12PM +0200, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > The APs are not ready to handle exceptions when verify_cpu() is called > in secondary_startup_64. Eek, no. MSR_IA32_MISC_ENABLE_XD_DISABLE needs to be cleared very early during CPU startup; this can't just be skipped. Also, is UNWIND_HINT_EMPTY needed for the new target? -Kees > > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > arch/x86/include/asm/realmode.h | 1 + > arch/x86/kernel/head_64.S | 1 + > arch/x86/realmode/init.c | 6 ++++++ > 3 files changed, 8 insertions(+) > > diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h > index 6590394af309..5c97807c38a4 100644 > --- a/arch/x86/include/asm/realmode.h > +++ b/arch/x86/include/asm/realmode.h > @@ -69,6 +69,7 @@ extern unsigned char startup_32_smp[]; > extern unsigned char boot_gdt[]; > #else > extern unsigned char secondary_startup_64[]; > +extern unsigned char secondary_startup_64_no_verify[]; > #endif > > static inline size_t real_mode_size_needed(void) > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S > index 5b577d6bce7a..8b43ed0592e8 100644 > --- a/arch/x86/kernel/head_64.S > +++ b/arch/x86/kernel/head_64.S > @@ -165,6 +165,7 @@ SYM_CODE_START(secondary_startup_64) > /* Sanitize CPU configuration */ > call verify_cpu > > +SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) > /* > * Retrieve the modifier (SME encryption mask if SME is active) to be > * added to the initial pgdir entry that will be programmed into CR3. > diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c > index 61a52b925d15..df701f87ddef 100644 > --- a/arch/x86/realmode/init.c > +++ b/arch/x86/realmode/init.c > @@ -46,6 +46,12 @@ static void sme_sev_setup_real_mode(struct trampoline_header *th) > th->flags |= TH_FLAGS_SME_ACTIVE; > > if (sev_es_active()) { > + /* > + * Skip the call to verify_cpu() in secondary_startup_64 as it > + * will cause #VC exceptions when the AP can't handle them yet. > + */ > + th->start = (u64) secondary_startup_64_no_verify; > + > if (sev_es_setup_ap_jump_table(real_mode_header)) > panic("Failed to update SEV-ES AP Jump Table"); > } > -- > 2.27.0 >
Hi Kees, thanks for your reviews! On Tue, Jul 14, 2020 at 06:40:30PM -0700, Kees Cook wrote: > Eek, no. MSR_IA32_MISC_ENABLE_XD_DISABLE needs to be cleared very early > during CPU startup; this can't just be skipped. That MSR is Intel-only, right? The boot-path installed here is only used for SEV-ES guests, running on AMD systems, so this MSR is not even accessed during boot on those VMs. The alternative is to set up exception handling prior to calling verify_cpu, including segments, stack and IDT. Given that verify_cpu() does not add much value to SEV-ES guests, I'd like to avoid adding this complexity. > Also, is UNWIND_HINT_EMPTY needed for the new target? Yes, I think it is, will add it in the next version. Regards, Joerg
On Wed, Jul 15, 2020 at 11:26:38AM +0200, Joerg Roedel wrote: > Hi Kees, > > thanks for your reviews! > > On Tue, Jul 14, 2020 at 06:40:30PM -0700, Kees Cook wrote: > > Eek, no. MSR_IA32_MISC_ENABLE_XD_DISABLE needs to be cleared very early > > during CPU startup; this can't just be skipped. > > That MSR is Intel-only, right? The boot-path installed here is only used > for SEV-ES guests, running on AMD systems, so this MSR is not even > accessed during boot on those VMs. Oh, hrm, yes, that's true. If other x86 maintainers are comfortable with this, then okay. My sense is that changing the early CPU startup paths will cause trouble down the line. > The alternative is to set up exception handling prior to calling > verify_cpu, including segments, stack and IDT. Given that verify_cpu() > does not add much value to SEV-ES guests, I'd like to avoid adding this > complexity. So, going back to the requirements here ... what things in verify_cpu() can cause exceptions? AFAICT, cpuid is safely handled (i.e. it is detected and only run in a way to avoid exceptions and the MSR reads/writes are similarly bound by CPU family/id range checks). I must be missing something. :) > > > Also, is UNWIND_HINT_EMPTY needed for the new target? > > Yes, I think it is, will add it in the next version. > > Regards, > > Joerg
Hi Kees, as a general note: With SEV-ES the guest kernel will get #VC exceptions for events that, without SEV-ES, would just cause a #VMEXIT to the hypervisor. On Wed, Jul 15, 2020 at 08:26:14AM -0700, Kees Cook wrote: > On Wed, Jul 15, 2020 at 11:26:38AM +0200, Joerg Roedel wrote: > > That MSR is Intel-only, right? The boot-path installed here is only used > > for SEV-ES guests, running on AMD systems, so this MSR is not even > > accessed during boot on those VMs. > > Oh, hrm, yes, that's true. If other x86 maintainers are comfortable with > this, then okay. My sense is that changing the early CPU startup paths > will cause trouble down the line. The AP startup path does not change for non SEV-ES guests. But under SEV-ES everything that might cause a #VC exception must be avoided until the kernel is ready to handle them. With the current patches this happens when the AP runs in 64bit long-mode and loaded TSS and IDT. Therefore a slightly different AP boot-path is needed for SEV-ES guests. > So, going back to the requirements here ... what things in verify_cpu() > can cause exceptions? AFAICT, cpuid is safely handled (i.e. it is > detected and only run in a way to avoid exceptions and the MSR > reads/writes are similarly bound by CPU family/id range checks). I must > be missing something. :) It is actually the CPUID instructions that cause #VC exceptions. The MSRs that are accessed on AMD processors are not intercepted in the hypervisors this code has been tested on, so these will not cause #VC exceptions. Regards, Joerg
On Wed, Jul 15, 2020 at 05:48:56PM +0200, Joerg Roedel wrote: > It is actually the CPUID instructions that cause #VC exceptions. The > MSRs that are accessed on AMD processors are not intercepted in the > hypervisors this code has been tested on, so these will not cause #VC > exceptions. Aaah. I see. Thanks for the details there. So ... can you add a bunch more comments about why/when the new entry path is being used? I really don't want to accidentally discover some unrelated refactoring down the road (in months, years, unrelated to SEV, etc) starts to also skip verify_cpu() on Intel systems. There had been a lot of BIOSes that set this MSR to disable NX, and I don't want to repeat that pain: Linux must never start an Intel CPU with that MSR set. :P
On Wed, Jul 15, 2020 at 12:49:23PM -0700, Kees Cook wrote: > Aaah. I see. Thanks for the details there. So ... can you add a bunch > more comments about why/when the new entry path is being used? I really > don't want to accidentally discover some unrelated refactoring down > the road (in months, years, unrelated to SEV, etc) starts to also skip > verify_cpu() on Intel systems. There had been a lot of BIOSes that set > this MSR to disable NX, and I don't want to repeat that pain: Linux must > never start an Intel CPU with that MSR set. :P Understood :) I added a comment above the label explaining why it is only used for SEV-ES guests and pointing out the importance of running verify_cpu() on all other systems, especially if they are Intel based. Regards, Joerg
diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h index 6590394af309..5c97807c38a4 100644 --- a/arch/x86/include/asm/realmode.h +++ b/arch/x86/include/asm/realmode.h @@ -69,6 +69,7 @@ extern unsigned char startup_32_smp[]; extern unsigned char boot_gdt[]; #else extern unsigned char secondary_startup_64[]; +extern unsigned char secondary_startup_64_no_verify[]; #endif static inline size_t real_mode_size_needed(void) diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S index 5b577d6bce7a..8b43ed0592e8 100644 --- a/arch/x86/kernel/head_64.S +++ b/arch/x86/kernel/head_64.S @@ -165,6 +165,7 @@ SYM_CODE_START(secondary_startup_64) /* Sanitize CPU configuration */ call verify_cpu +SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) /* * Retrieve the modifier (SME encryption mask if SME is active) to be * added to the initial pgdir entry that will be programmed into CR3. diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c index 61a52b925d15..df701f87ddef 100644 --- a/arch/x86/realmode/init.c +++ b/arch/x86/realmode/init.c @@ -46,6 +46,12 @@ static void sme_sev_setup_real_mode(struct trampoline_header *th) th->flags |= TH_FLAGS_SME_ACTIVE; if (sev_es_active()) { + /* + * Skip the call to verify_cpu() in secondary_startup_64 as it + * will cause #VC exceptions when the AP can't handle them yet. + */ + th->start = (u64) secondary_startup_64_no_verify; + if (sev_es_setup_ap_jump_table(real_mode_header)) panic("Failed to update SEV-ES AP Jump Table"); }