[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 19 Jan 2021 16:48:56 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=CdPtzvGWC8qpvHiI0CIgRulB5H1G0rOJ/MVp8eO5SKM=; b=ZC5waszaVXN8z7qiNIdrfeQt+Lf7lkm7ifLW2jOCwJCRGtCy+kH73/K3pTzgxtdJn3CW2ajOCDqMiq0tvTP+FMm/aArCnOt2o5EkfGE6qBxXMwMEzDaRo/QMb9TmdPt0NZCuJHhUrMy8Fqi5fV76XbKuSfHVXlqie2SvIsHxf0am0dqQVkfI+NMpXve8F6Gz/hvdRIxOyyypVrc5+T/qEhL35vIUKq/qRpPSNSy06NsojpRehEaXb1dT1UjeREckTiGYwHMhyPDb4DYxjiVRWxHzNUiUBlV5zt97ckk5oq/MZGjPdF9OVmWRHCzaqukT4BaEmlaIEXVW5gCusFCKUw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=GLmVW8OKI/fb1aGdqWD9KDlRpoxSM4JJHK7jAuSZDButhi65mCdopio4RiFqAtCQXpqEtX6MmuhK+cgWnpFiSAeVnXTykuSomqCV2Jant5ZgtRMt/nV1k/bpIa/kyj8dPAXl7hbGIqdzGhOGLw5qe0VTk81aCYvKAB/mew/8GzjXENtIX9hS5KLlZTx5rIu42VCAJt2h2BduoBO/h/cEmCrrqusYxcr1PnszYeIaYECSah71SBYHMWrOvOKO5bosxfetTKFgXKWSNv6YJrGn94fuHk5aAzSAhdR3L6LP1CBg2KLukwuYb753n0LSivuRQPrqz4EQlqu5fLCgXVPDWA==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Norbert Kamiński <norbert.kaminski@xxxxxxxxx>, Marek Kasiewicz <marek.kasiewicz@xxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, Michal Zygowski <michal.zygowski@xxxxxxxxx>, Piotr Krol <piotr.krol@xxxxxxxx>, Krystian Hebel <krystian.hebel@xxxxxxxxx>, "Daniel P . Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, Rich Persaud <persaur@xxxxxxxxx>, Christopher Clark <christopher.w.clark@xxxxxxxxx>
  • Delivery-date: Tue, 19 Jan 2021 15:49:30 +0000
  • Ironport-sdr: Wsh5yXwvtcMExerPvH6Ro4TgcWS2IJ8ykInBObMYoKeA0ux/4bb955LcvspiKVH4HK1Pv5pQ8m eHsAr8m4kwTILa7nUgT6azLyBM5mMXI5LNPswLRsXO6UX4ZJPG0cLrJ0BXLlX0A3n3PlHHvnHf 7SQuOmd76RwfGkqsnOpEVxD+TEoJpaauBS/P0cM2H4i2IDDNJE32lAYu2jnK/2++sDbWWyhcQo mptLWBZRqqxXQBAXze9ncKPZ/XvYVo7M7Blyi5XK6AeI55+CpcFQtjrAhR8+m1bPnsAWVph/Aj Zz8=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.