[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/3] x86: Support booting under Secure Startup via SKINIT
On Fri, Jan 15, 2021 at 11:10:46PM +0000, Andrew Cooper wrote: > From: Norbert Kamiński <norbert.kaminski@xxxxxxxxx> > > For now, this is simply enough logic to let Xen come up after the bootloader > has executed an SKINIT instruction to begin a Secure Startup. Since I know very little about this, I might as well ask. Reading the PM, SKINIT requires a payload, which is an image to boot. That image must be <= 64KB and needs a special header. In case of booting Xen using SKINIT, what would be that payload? (called SLB in the PM). > During a Secure Startup, the BSP operates with the GIF clear (blocks all > external interrupts, even SMI/NMI), and INIT_REDIRECTION active (converts INIT > IPIs to #SX exceptions, if e.g. the platform needs to scrub secrets before > resetting). To afford APs the same Secure Startup protections as the BSP, the > INIT IPI must be skipped, and SIPI must be the first interrupt seen. > > Full details are available in AMD APM Vol2 15.27 "Secure Startup with SKINIT" > > Introduce skinit_enable_intr() and call it from cpu_init(), next to the > enable_nmis() which performs a related function for tboot startups. > > Also introduce ap_boot_method to control the sequence of actions for AP boot. > > Signed-off-by: Marek Kasiewicz <marek.kasiewicz@xxxxxxxxx> > Signed-off-by: Norbert Kamiński <norbert.kaminski@xxxxxxxxx> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > --- > CC: Jan Beulich <JBeulich@xxxxxxxx> > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx> > CC: Wei Liu <wl@xxxxxxx> > CC: Marek Kasiewicz <marek.kasiewicz@xxxxxxxxx> > CC: Norbert Kamiński <norbert.kaminski@xxxxxxxxx> > CC: Michal Zygowski <michal.zygowski@xxxxxxxxx> > CC: Piotr Krol <piotr.krol@xxxxxxxx> > CC: Krystian Hebel <krystian.hebel@xxxxxxxxx> > CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx> > CC: Rich Persaud <persaur@xxxxxxxxx> > CC: Christopher Clark <christopher.w.clark@xxxxxxxxx> > --- > xen/arch/x86/cpu/common.c | 32 ++++++++++++++++++++++++++++++++ > xen/arch/x86/smpboot.c | 12 +++++++++++- > xen/include/asm-x86/cpufeature.h | 1 + > xen/include/asm-x86/msr-index.h | 1 + > xen/include/asm-x86/processor.h | 6 ++++++ > 5 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c > index a684519a20..d9a103e721 100644 > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -834,6 +834,29 @@ void load_system_tables(void) > BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf)); > } > > +static void skinit_enable_intr(void) Since this is AFAICT AMD specific code, shouldn't it better be in cpu/amd.c instead of cpu/common.c? > +{ > + uint64_t val; > + > + /* > + * If the platform is performing a Secure Launch via SKINIT > + * INIT_REDIRECTION flag will be active. > + */ > + if ( !cpu_has_skinit || rdmsr_safe(MSR_K8_VM_CR, val) || > + !(val & VM_CR_INIT_REDIRECTION) ) I would add some kind of check here to assert that APs are started in the correct state, ie: BUG_ON(ap_boot_method == AP_BOOT_SKINIT); > + return; > + > + ap_boot_method = AP_BOOT_SKINIT; And I would also set ap_boot_method from the BSP context only, there's no need for the APs to set this. > + > + /* > + * We don't yet handle #SX. Disable INIT_REDIRECTION first, before > + * enabling GIF, so a pending INIT resets us, rather than causing a > + * panic due to an unknown exception. > + */ > + wrmsr_safe(MSR_K8_VM_CR, val & ~VM_CR_INIT_REDIRECTION); > + asm volatile ( ".byte 0x0f,0x01,0xdc" /* STGI */ ::: "memory" ); We already have one of those in smv/entry.S, so I would rather not open-code the opcodes here again, but I'm not unsure whether there's a suitable place for those. > +} > + > /* > * cpu_init() initializes state that is per-CPU. Some data is already > * initialized (naturally) in the bootstrap process, such as the GDT > @@ -865,6 +888,15 @@ void cpu_init(void) > write_debugreg(6, X86_DR6_DEFAULT); > write_debugreg(7, X86_DR7_DEFAULT); > > + /* > + * If the platform is performing a Secure Launch via SKINIT, GIF is > + * clear to prevent external interrupts interfering with Secure > + * Startup. Re-enable all interrupts now that we are suitably set up. > + * > + * Refer to AMD APM Vol2 15.27 "Secure Startup with SKINIT". > + */ > + skinit_enable_intr(); As this doesn't seem to be mentioned in the PM, for confirmation, is it fine to do this before updating microcode? > + > /* Enable NMIs. Our loader (e.g. Tboot) may have left them disabled. */ > enable_nmis(); > } > diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c > index 195e3681b4..0f11fea7be 100644 > --- a/xen/arch/x86/smpboot.c > +++ b/xen/arch/x86/smpboot.c > @@ -49,6 +49,7 @@ > #include <mach_apic.h> > > unsigned long __read_mostly trampoline_phys; > +enum ap_boot_method __read_mostly ap_boot_method = AP_BOOT_NORMAL; > > /* representing HT siblings of each logical CPU */ > DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask); > @@ -424,7 +425,16 @@ static int wakeup_secondary_cpu(int phys_apicid, > unsigned long start_eip) > { > unsigned long send_status = 0, accept_status = 0; > int maxlvt, timeout, i; > - bool send_INIT = true; > + > + /* > + * Normal AP startup uses an INIT-SIPI-SIPI sequence. > + * > + * When using SKINIT for Secure Startup, the INIT IPI must be skipped, so > + * that SIPI is the first interrupt the AP sees. > + * > + * Refer to AMD APM Vol2 15.27 "Secure Startup with SKINIT". > + */ > + bool send_INIT = ap_boot_method != AP_BOOT_SKINIT; Since send_INIT is used in a single place in the function I would just use ap_boot_method != AP_BOOT_SKINIT directly instead. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |