Re: [PATCH 3/3] x86: Support booting under Secure Startup via SKINIT

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 19, 2021 at 05:23:25PM +0000, Andrew Cooper wrote:
> On 19/01/2021 15:48, Roger Pau Monné wrote:
> > 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).
> 
> The SK/SLB is implemented by https://github.com/TrenchBoot/landing-zone/
> which does all the low level platform stuff.  It is the logical
> equivalent of the Intel-provided TXT ACM which is a blob as far as the
> world is concerned.
> 
> From a "system boot" perspective, the plan is something like this:
> 
> * Grub puts xen/kernel/initrd in memory
> * A final stanza line of "secure_launch" changes the exit of grub to be
> a DRTM DLE (Dynamic Launch Event).
> ** For Intel TXT, this is the SENTER instruction, provided that the
> firmware loaded the TXT ACM properly
> ** For AMD/Hygon, this is additionally loading landing-zone into memory,
> and using the SKINIT instruction.
> * TXT blob, or Landing Zone, do low level things.
> * They enter xen/Linux/other via a common protocol.
> 
> With this patch series in place, Xen's Multiboot2 entrypoint works just
> fine as far as "invoke the next thing" goes.  Linux has a
> work-in-progress extension to their zero-page protocol.
> 
> >> 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?
> 
> Hygon will get sad...
> 
> It's deliberately not in AMD-specific code.

Hygon already calls into AMD specific functions in amd.c
(early_init_amd, amd_log_freq), so it wouldn't seem that weird to
place it in amd.c and share with other AMD derivatives. Likely not
worth arguing about.

> >> +{
> >> +  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);
> 
> At the moment, it really doesn't matter.  This change is to simply let
> Xen boot.
> 
> Later changes which do the TPM and attestation work is going to need to
> know details like this, but it will be elsewhere on boot, and one
> recovery option is going to be "please just boot and let be get back
> into the system", in which case a crosscheck is not wanted.

I still think printing a message might be helpful to know the AP has
been started in an unexpected state.

> >> +
> >> +  /*
> >> +   * 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.
> 
> I deliberately didn't.  SGTI is not something we should have a helper
> for - it's not safe for general use.

Oh, I wasn't thinking of a helper, but rather a define for the
open-coded .byte directive (ie: like the one in svm/entry.S) that's
shared between both users. No big deal anyway.

Thanks, Roger.