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

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

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.