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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>> +{
>> +    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.

>> +
>> +    /*
>> +     * 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.

With this codepath added, we've got everywhere it is legitimate to use.

>> +}
>> +
>>  /*
>>   * 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?

Of course.  (I also need to complete my series allowing us to move boot
time microcode loading far earlier.)

>
>> +
>>      /* 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.

I did it like this, with an expectation of how the TXT logic is likely
to mesh in, without unnecessary churn.

~Andrew