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.kamin...@3mdeb.com>
> >>
> >> 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.kasiew...@3mdeb.com>
> >> Signed-off-by: Norbert Kamiński <norbert.kamin...@3mdeb.com>
> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> >> ---
> >> CC: Jan Beulich <jbeul...@suse.com>
> >> CC: Roger Pau Monné <roger....@citrix.com>
> >> CC: Wei Liu <w...@xen.org>
> >> CC: Marek Kasiewicz <marek.kasiew...@3mdeb.com>
> >> CC: Norbert Kamiński <norbert.kamin...@3mdeb.com>
> >> CC: Michal Zygowski <michal.zygow...@3mdeb.com>
> >> CC: Piotr Krol <piotr.k...@3mdeb.co>
> >> CC: Krystian Hebel <krystian.he...@3mdeb.com>
> >> CC: Daniel P. Smith <dpsm...@apertussolutions.com>
> >> CC: Rich Persaud <pers...@gmail.com>
> >> CC: Christopher Clark <christopher.w.cl...@gmail.com>
> >> ---
> >> 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.
