[PATCH 0/3] x86: Initial Trenchboot/SKINIT support

[PATCH 0/3] x86: Initial Trenchboot/SKINIT support

[Andrew Cooper]

The Trenchboot project[0][1] project aims to develop and upstream support for
TCG DRTM support into various open source projects in the ecosystem, to
improve boot security.  Trenchboot has been discussed at previous
XenSummits[2], and the work across the ecosystem is extensively blogged
about[3].

This series forms the start of the work within Xen, and for now simply covers
the ability to boot in the AMD SKINIT/Secure Startup environment.

Full details are available in AMD APM Vol2 15.27 "Secure Startup with SKINIT"

Future work will cover related support for Intel TXT in a non-tboot system,
and the common logic to interact with the TPM and extend the hardware measured
chain of trust into dom0.

[0] https://trenchboot.org/
[1] https://github.com/TrenchBoot/documentation
[2] https://www.youtube.com/watch?v=SwByVrw7-08&list=PLYyw7IQjL-zFYmEoZEYswoVuXrHvXAWxj&index=13
[3] https://blog.3mdeb.com/tags/trenchboot/

Andrew Cooper (2):
  x86/smpboot: Re-position the call to tboot_wake_ap()
  x86/smpboot: Allow making an INIT IPI conditional

Norbert Kamiński (1):
  x86: Support booting under Secure Startup via SKINIT

 xen/arch/x86/cpu/common.c        | 32 +++++++++++++
 xen/arch/x86/smpboot.c           | 98 ++++++++++++++++++++++++----------------
 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, 99 insertions(+), 39 deletions(-)

-- 
2.11.0

[PATCH 1/3] x86/smpboot: Re-position the call to tboot_wake_ap()

[Andrew Cooper]

So all the moving parts are in one function.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
CC: Michal Zygowski <michal.zygowski@3mdeb.com>
CC: Piotr Krol <piotr.krol@3mdeb.co>
CC: Krystian Hebel <krystian.hebel@3mdeb.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Rich Persaud <persaur@gmail.com>
CC: Christopher Clark <christopher.w.clark@gmail.com>
---
 xen/arch/x86/smpboot.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 67e727cebd..9eca452ce1 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -426,6 +426,13 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
     int maxlvt, timeout, i;
 
     /*
+     * Some versions of tboot might be able to handle the entire wake sequence
+     * on our behalf.
+     */
+    if ( tboot_in_measured_env() && tboot_wake_ap(phys_apicid, start_eip) )
+        return 0;
+
+    /*
      * Be paranoid about clearing APIC errors.
      */
     apic_write(APIC_ESR, 0);
@@ -570,8 +577,7 @@ static int do_boot_cpu(int apicid, int cpu)
     set_cpu_state(CPU_STATE_INIT);
 
     /* Starting actual IPI sequence... */
-    if ( !tboot_in_measured_env() || tboot_wake_ap(apicid, start_eip) )
-        boot_error = wakeup_secondary_cpu(apicid, start_eip);
+    boot_error = wakeup_secondary_cpu(apicid, start_eip);
 
     if ( !boot_error )
     {
-- 
2.11.0

[PATCH 2/3] x86/smpboot: Allow making an INIT IPI conditional

[Andrew Cooper]

A subsequent change is going to introduce SKINIT support, wherein the APs will
be already be in the wait-for-SIPI state, and an INIT must not be sent.

Introduce a send_INIT boolean, so we can control sending an INIT IPI
separately from sending SIPIs.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
CC: Michal Zygowski <michal.zygowski@3mdeb.com>
CC: Piotr Krol <piotr.krol@3mdeb.co>
CC: Krystian Hebel <krystian.hebel@3mdeb.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Rich Persaud <persaur@gmail.com>
CC: Christopher Clark <christopher.w.clark@gmail.com>
---
 xen/arch/x86/smpboot.c | 78 ++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 37 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 9eca452ce1..195e3681b4 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -424,6 +424,7 @@ 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;
 
     /*
      * Some versions of tboot might be able to handle the entire wake sequence
@@ -438,49 +439,52 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
     apic_write(APIC_ESR, 0);
     apic_read(APIC_ESR);
 
-    Dprintk("Asserting INIT.\n");
+    if ( send_INIT )
+    {
+        Dprintk("Asserting INIT.\n");
 
-    /*
-     * Turn INIT on target chip via IPI
-     */
-    apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT,
-                   phys_apicid);
+        /*
+         * Turn INIT on target chip via IPI
+         */
+        apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT,
+                       phys_apicid);
 
-    if ( !x2apic_enabled )
-    {
-        Dprintk("Waiting for send to finish...\n");
-        timeout = 0;
-        do {
-            Dprintk("+");
-            udelay(100);
-            send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
-        } while ( send_status && (timeout++ < 1000) );
+        if ( !x2apic_enabled )
+        {
+            Dprintk("Waiting for send to finish...\n");
+            timeout = 0;
+            do {
+                Dprintk("+");
+                udelay(100);
+                send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
+            } while ( send_status && (timeout++ < 1000) );
 
-        mdelay(10);
+            mdelay(10);
 
-        Dprintk("Deasserting INIT.\n");
+            Dprintk("Deasserting INIT.\n");
 
-        apic_icr_write(APIC_INT_LEVELTRIG | APIC_DM_INIT, phys_apicid);
+            apic_icr_write(APIC_INT_LEVELTRIG | APIC_DM_INIT, phys_apicid);
 
-        Dprintk("Waiting for send to finish...\n");
-        timeout = 0;
-        do {
-            Dprintk("+");
-            udelay(100);
-            send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
-        } while ( send_status && (timeout++ < 1000) );
-    }
-    else if ( tboot_in_measured_env() )
-    {
-        /*
-         * With tboot AP is actually spinning in a mini-guest before
-         * receiving INIT. Upon receiving INIT ipi, AP need time to VMExit,
-         * update VMCS to tracking SIPIs and VMResume.
-         *
-         * While AP is in root mode handling the INIT the CPU will drop
-         * any SIPIs
-         */
-        udelay(10);
+            Dprintk("Waiting for send to finish...\n");
+            timeout = 0;
+            do {
+                Dprintk("+");
+                udelay(100);
+                send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
+            } while ( send_status && (timeout++ < 1000) );
+        }
+        else if ( tboot_in_measured_env() )
+        {
+            /*
+             * With tboot AP is actually spinning in a mini-guest before
+             * receiving INIT. Upon receiving INIT ipi, AP need time to VMExit,
+             * update VMCS to tracking SIPIs and VMResume.
+             *
+             * While AP is in root mode handling the INIT the CPU will drop
+             * any SIPIs
+             */
+            udelay(10);
+        }
     }
 
     maxlvt = get_maxlvt();
-- 
2.11.0

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

[Andrew Cooper]

From: Norbert Kamiński <norbert.kaminski@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.

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@3mdeb.com>
Signed-off-by: Norbert Kamiński <norbert.kaminski@3mdeb.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
CC: Michal Zygowski <michal.zygowski@3mdeb.com>
CC: Piotr Krol <piotr.krol@3mdeb.co>
CC: Krystian Hebel <krystian.hebel@3mdeb.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Rich Persaud <persaur@gmail.com>
CC: Christopher Clark <christopher.w.clark@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)
+{
+	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) )
+		return;
+
+	ap_boot_method = AP_BOOT_SKINIT;
+
+	/*
+	 * 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" );
+}
+
 /*
  * 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();
+
 	/* 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;
 
     /*
      * Some versions of tboot might be able to handle the entire wake sequence
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index ad3d84bdde..f62e526a96 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -76,6 +76,7 @@
 #define cpu_has_svm             boot_cpu_has(X86_FEATURE_SVM)
 #define cpu_has_sse4a           boot_cpu_has(X86_FEATURE_SSE4A)
 #define cpu_has_xop             boot_cpu_has(X86_FEATURE_XOP)
+#define cpu_has_skinit          boot_cpu_has(X86_FEATURE_SKINIT)
 #define cpu_has_fma4            boot_cpu_has(X86_FEATURE_FMA4)
 #define cpu_has_tbm             boot_cpu_has(X86_FEATURE_TBM)
 
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index ff583cf0ed..1f5a5d0e38 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -117,6 +117,7 @@
 #define  PASID_VALID                        (_AC(1, ULL) << 31)
 
 #define MSR_K8_VM_CR                        0xc0010114
+#define  VM_CR_INIT_REDIRECTION             (_AC(1, ULL) <<  1)
 #define  VM_CR_SVM_DISABLE                  (_AC(1, ULL) <<  4)
 
 /*
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 9acb80fdcd..d5f467d245 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -631,6 +631,12 @@ static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model,
 extern int8_t opt_tsx, cpu_has_tsx_ctrl;
 void tsx_init(void);
 
+enum ap_boot_method {
+    AP_BOOT_NORMAL,
+    AP_BOOT_SKINIT,
+};
+extern enum ap_boot_method ap_boot_method;
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_X86_PROCESSOR_H */
-- 
2.11.0

Re: [PATCH 1/3] x86/smpboot: Re-position the call to tboot_wake_ap()

[Roger Pau Monné]

On Fri, Jan 15, 2021 at 11:10:44PM +0000, Andrew Cooper wrote:
> So all the moving parts are in one function.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
> CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
> CC: Michal Zygowski <michal.zygowski@3mdeb.com>
> CC: Piotr Krol <piotr.krol@3mdeb.co>
> CC: Krystian Hebel <krystian.hebel@3mdeb.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Rich Persaud <persaur@gmail.com>
> CC: Christopher Clark <christopher.w.clark@gmail.com>
> ---
>  xen/arch/x86/smpboot.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 67e727cebd..9eca452ce1 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -426,6 +426,13 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
>      int maxlvt, timeout, i;
>  
>      /*
> +     * Some versions of tboot might be able to handle the entire wake sequence
> +     * on our behalf.
> +     */
> +    if ( tboot_in_measured_env() && tboot_wake_ap(phys_apicid, start_eip) )

I think you are missing a ! in front of tboot_wake_ap?

Thanks, Roger.

Re: [PATCH 1/3] x86/smpboot: Re-position the call to tboot_wake_ap()

[Andrew Cooper]

On 19/01/2021 14:38, Roger Pau Monné wrote:
> On Fri, Jan 15, 2021 at 11:10:44PM +0000, Andrew Cooper wrote:
>> So all the moving parts are in one function.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
>> CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
>> CC: Michal Zygowski <michal.zygowski@3mdeb.com>
>> CC: Piotr Krol <piotr.krol@3mdeb.co>
>> CC: Krystian Hebel <krystian.hebel@3mdeb.com>
>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>> CC: Rich Persaud <persaur@gmail.com>
>> CC: Christopher Clark <christopher.w.clark@gmail.com>
>> ---
>>  xen/arch/x86/smpboot.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
>> index 67e727cebd..9eca452ce1 100644
>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -426,6 +426,13 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
>>      int maxlvt, timeout, i;
>>  
>>      /*
>> +     * Some versions of tboot might be able to handle the entire wake sequence
>> +     * on our behalf.
>> +     */
>> +    if ( tboot_in_measured_env() && tboot_wake_ap(phys_apicid, start_eip) )
> I think you are missing a ! in front of tboot_wake_ap?

Oh - so I am.  That function is totally backwards.

Fixed.

~Andrew

Re: [PATCH 2/3] x86/smpboot: Allow making an INIT IPI conditional

[Roger Pau Monné]

On Fri, Jan 15, 2021 at 11:10:45PM +0000, Andrew Cooper wrote:
> A subsequent change is going to introduce SKINIT support, wherein the APs will
> be already be in the wait-for-SIPI state, and an INIT must not be sent.
> 
> Introduce a send_INIT boolean, so we can control sending an INIT IPI
> separately from sending SIPIs.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'm not opposed to introduce this, but maybe it would be better to
move it to a separate helper? send_init(unsigned int apicid); or some
such?

Would reduce one level of indentation.

Thanks, Roger.

Re: [PATCH 2/3] x86/smpboot: Allow making an INIT IPI conditional

[Andrew Cooper]

On 19/01/2021 14:58, Roger Pau Monné wrote:
> On Fri, Jan 15, 2021 at 11:10:45PM +0000, Andrew Cooper wrote:
>> A subsequent change is going to introduce SKINIT support, wherein the APs will
>> be already be in the wait-for-SIPI state, and an INIT must not be sent.
>>
>> Introduce a send_INIT boolean, so we can control sending an INIT IPI
>> separately from sending SIPIs.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I'm not opposed to introduce this, but maybe it would be better to
> move it to a separate helper? send_init(unsigned int apicid); or some
> such?
>
> Would reduce one level of indentation.

I've got a lot of cleanup planned for 4.16, but splitting this up
INIT-SIPI-SIPI is specifically not one of them.

This will get more complicated with Intel TXT Intel, and I also want to
integrate it more nicely with the virtualised AP boot logic.  I suspect
we'll end up with a function pointer per platform&configuration, but
that's too much work at this point in 4.15.

~Andrew

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

[Roger Pau Monné]

On Fri, Jan 15, 2021 at 11:10:46PM +0000, Andrew Cooper wrote:
> From: Norbert Kamiński <norbert.kaminski@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).

> 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@3mdeb.com>
> Signed-off-by: Norbert Kamiński <norbert.kaminski@3mdeb.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
> CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
> CC: Michal Zygowski <michal.zygowski@3mdeb.com>
> CC: Piotr Krol <piotr.krol@3mdeb.co>
> CC: Krystian Hebel <krystian.hebel@3mdeb.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Rich Persaud <persaur@gmail.com>
> CC: Christopher Clark <christopher.w.clark@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?

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

Re: [PATCH 2/3] x86/smpboot: Allow making an INIT IPI conditional

[Roger Pau Monné]

On Tue, Jan 19, 2021 at 03:05:38PM +0000, Andrew Cooper wrote:
> On 19/01/2021 14:58, Roger Pau Monné wrote:
> > On Fri, Jan 15, 2021 at 11:10:45PM +0000, Andrew Cooper wrote:
> >> A subsequent change is going to introduce SKINIT support, wherein the APs will
> >> be already be in the wait-for-SIPI state, and an INIT must not be sent.
> >>
> >> Introduce a send_INIT boolean, so we can control sending an INIT IPI
> >> separately from sending SIPIs.
> >>
> >> No functional change.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > I'm not opposed to introduce this, but maybe it would be better to
> > move it to a separate helper? send_init(unsigned int apicid); or some
> > such?
> >
> > Would reduce one level of indentation.
> 
> I've got a lot of cleanup planned for 4.16, but splitting this up
> INIT-SIPI-SIPI is specifically not one of them.
> 
> This will get more complicated with Intel TXT Intel, and I also want to
> integrate it more nicely with the virtualised AP boot logic.  I suspect
> we'll end up with a function pointer per platform&configuration, but
> that's too much work at this point in 4.15.

Ack, I'm fine with this, albeit I would also be fine with dropping the
variable and just open-coding the ap_boot_method check (but I guess
there will be further use of this variable in newer patches):

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

[Andrew Cooper]

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@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.kasiewicz@3mdeb.com>
>> Signed-off-by: Norbert Kamiński <norbert.kaminski@3mdeb.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
>> CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
>> CC: Michal Zygowski <michal.zygowski@3mdeb.com>
>> CC: Piotr Krol <piotr.krol@3mdeb.co>
>> CC: Krystian Hebel <krystian.hebel@3mdeb.com>
>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>> CC: Rich Persaud <persaur@gmail.com>
>> CC: Christopher Clark <christopher.w.clark@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.

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

Re: [PATCH 1/3] x86/smpboot: Re-position the call to tboot_wake_ap()

[Jan Beulich]

On 19.01.2021 15:55, Andrew Cooper wrote:
> On 19/01/2021 14:38, Roger Pau Monné wrote:
>> On Fri, Jan 15, 2021 at 11:10:44PM +0000, Andrew Cooper wrote:
>>> So all the moving parts are in one function.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Wei Liu <wl@xen.org>
>>> CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
>>> CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
>>> CC: Michal Zygowski <michal.zygowski@3mdeb.com>
>>> CC: Piotr Krol <piotr.krol@3mdeb.co>
>>> CC: Krystian Hebel <krystian.hebel@3mdeb.com>
>>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> CC: Rich Persaud <persaur@gmail.com>
>>> CC: Christopher Clark <christopher.w.clark@gmail.com>
>>> ---
>>>  xen/arch/x86/smpboot.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
>>> index 67e727cebd..9eca452ce1 100644
>>> --- a/xen/arch/x86/smpboot.c
>>> +++ b/xen/arch/x86/smpboot.c
>>> @@ -426,6 +426,13 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip)
>>>      int maxlvt, timeout, i;
>>>  
>>>      /*
>>> +     * Some versions of tboot might be able to handle the entire wake sequence
>>> +     * on our behalf.
>>> +     */
>>> +    if ( tboot_in_measured_env() && tboot_wake_ap(phys_apicid, start_eip) )
>> I think you are missing a ! in front of tboot_wake_ap?
> 
> Oh - so I am.  That function is totally backwards.
> 
> Fixed.

And then

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Jan Beulich]

On 16.01.2021 00:10, Andrew Cooper wrote:
> --- 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)
> +{
> +	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) )
> +		return;
> +
> +	ap_boot_method = AP_BOOT_SKINIT;
> +
> +	/*
> +	 * 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);

Why wrmsr_safe() without checking its return value? If the write
faults, we're hosed anyway, aren't we, so we may as well crash on
the offending WRMSR rather than some time later?

Jan

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

[Roger Pau Monné]

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@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.kasiewicz@3mdeb.com>
> >> Signed-off-by: Norbert Kamiński <norbert.kaminski@3mdeb.com>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Wei Liu <wl@xen.org>
> >> CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
> >> CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
> >> CC: Michal Zygowski <michal.zygowski@3mdeb.com>
> >> CC: Piotr Krol <piotr.krol@3mdeb.co>
> >> CC: Krystian Hebel <krystian.hebel@3mdeb.com>
> >> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> >> CC: Rich Persaud <persaur@gmail.com>
> >> CC: Christopher Clark <christopher.w.clark@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.

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

[Andrew Cooper]

On 20/01/2021 09:19, Jan Beulich wrote:
> On 16.01.2021 00:10, Andrew Cooper wrote:
>> --- 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)
>> +{
>> +	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) )
>> +		return;
>> +
>> +	ap_boot_method = AP_BOOT_SKINIT;
>> +
>> +	/*
>> +	 * 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);
> Why wrmsr_safe() without checking its return value? If the write
> faults, we're hosed anyway, aren't we, so we may as well crash on
> the offending WRMSR rather than some time later?

Paranoia.

Xen's old MSR behaviour would have leaked INIT_REDIRECTION into guest
context but discarded writes, and there are usecases to keep
INIT_REDIRECTION enabled (if you're willing to sacrifice PV guests to
avoid #SX-over-the-syscall-gap or back-to-back-INIT-on-IST shaped
security holes).

I can make it unconditional if you'd prefer.  At the moment, all this is
is a best-effort attempt to get back into the old state, so development
can continue more easily.

~Andrew

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

[Jan Beulich]

On 28.01.2021 21:26, Andrew Cooper wrote:
> On 20/01/2021 09:19, Jan Beulich wrote:
>> On 16.01.2021 00:10, Andrew Cooper wrote:
>>> --- 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)
>>> +{
>>> +	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) )
>>> +		return;
>>> +
>>> +	ap_boot_method = AP_BOOT_SKINIT;
>>> +
>>> +	/*
>>> +	 * 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);
>> Why wrmsr_safe() without checking its return value? If the write
>> faults, we're hosed anyway, aren't we, so we may as well crash on
>> the offending WRMSR rather than some time later?
> 
> Paranoia.
> 
> Xen's old MSR behaviour would have leaked INIT_REDIRECTION into guest
> context but discarded writes,

In which case there wouldn't have been any fault to catch and
ignore.

> and there are usecases to keep
> INIT_REDIRECTION enabled (if you're willing to sacrifice PV guests to
> avoid #SX-over-the-syscall-gap or back-to-back-INIT-on-IST shaped
> security holes).
> 
> I can make it unconditional if you'd prefer.  At the moment, all this is
> is a best-effort attempt to get back into the old state, so development
> can continue more easily.

I'm not sure which variant is strictly better, but if you stick
to wrmsr_safe(), may I ask that you say this is out of paranoia
in the comment, so future readers will not wonder like I did?

Jan

[PATCH 2.5/3] x86/svm: Reimplement VMRUN/STGI/CLGI with new asm-defns.h infrastructure

[Andrew Cooper]

... in order to reuse stgi elsewhere.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/hvm/svm/entry.S    | 10 +++-------
 xen/include/asm-x86/asm-defns.h | 12 ++++++++++++
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index 1d2df08e89..e208a4b32a 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -22,10 +22,6 @@
 #include <asm/asm_defns.h>
 #include <asm/page.h>
 
-#define VMRUN  .byte 0x0F,0x01,0xD8
-#define STGI   .byte 0x0F,0x01,0xDC
-#define CLGI   .byte 0x0F,0x01,0xDD
-
 ENTRY(svm_asm_do_resume)
         GET_CURRENT(bx)
 .Lsvm_do_resume:
@@ -82,9 +78,9 @@ __UNLIKELY_END(nsvm_hap)
         pop  %rsi
         pop  %rdi
 
-        CLGI
+        clgi
         sti
-        VMRUN
+        vmrun
 
         SAVE_ALL
 
@@ -93,7 +89,7 @@ __UNLIKELY_END(nsvm_hap)
         SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
-        STGI
+        stgi
 GLOBAL(svm_stgi_label)
         mov  %rsp,%rdi
         call svm_vmexit_handler
diff --git a/xen/include/asm-x86/asm-defns.h b/xen/include/asm-x86/asm-defns.h
index 43f4868d40..2e3ec0ac01 100644
--- a/xen/include/asm-x86/asm-defns.h
+++ b/xen/include/asm-x86/asm-defns.h
@@ -8,6 +8,18 @@
 .endm
 #endif
 
+.macro vmrun
+    .byte 0x0f, 0x01, 0xd8
+.endm
+
+.macro stgi
+    .byte 0x0f, 0x01, 0xdc
+.endm
+
+.macro clgi
+    .byte 0x0f, 0x01, 0xdd
+.endm
+
 .macro INDIRECT_BRANCH insn:req arg:req
 /*
  * Create an indirect branch.  insn is one of call/jmp, arg is a single
-- 
2.11.0

Re: [PATCH 2.5/3] x86/svm: Reimplement VMRUN/STGI/CLGI with new asm-defns.h infrastructure

[Jan Beulich]

On 29.01.2021 11:45, Andrew Cooper wrote:
> ... in order to reuse stgi elsewhere.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Andrew Cooper]

From: Norbert Kamiński <norbert.kaminski@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.

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@3mdeb.com>
Signed-off-by: Norbert Kamiński <norbert.kaminski@3mdeb.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com>
CC: Norbert Kamiński <norbert.kaminski@3mdeb.com>
CC: Michal Zygowski <michal.zygowski@3mdeb.com>
CC: Piotr Krol <piotr.krol@3mdeb.com>
CC: Krystian Hebel <krystian.hebel@3mdeb.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Rich Persaud <persaur@gmail.com>
CC: Christopher Clark <christopher.w.clark@gmail.com>

v2:
 * Rebase over 'stgi' cleanup
 * Don't use wrmsr_safe().
---
 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..cca97e4e03 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)
+{
+	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) )
+		return;
+
+	ap_boot_method = AP_BOOT_SKINIT;
+
+	/*
+	 * 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(MSR_K8_VM_CR, val & ~VM_CR_INIT_REDIRECTION);
+	asm volatile ( "stgi" ::: "memory" );
+}
+
 /*
  * 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();
+
 	/* 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 61ce923189..82c1012e89 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;
 
     /*
      * Some versions of tboot might be able to handle the entire wake sequence
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index ad3d84bdde..f62e526a96 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -76,6 +76,7 @@
 #define cpu_has_svm             boot_cpu_has(X86_FEATURE_SVM)
 #define cpu_has_sse4a           boot_cpu_has(X86_FEATURE_SSE4A)
 #define cpu_has_xop             boot_cpu_has(X86_FEATURE_XOP)
+#define cpu_has_skinit          boot_cpu_has(X86_FEATURE_SKINIT)
 #define cpu_has_fma4            boot_cpu_has(X86_FEATURE_FMA4)
 #define cpu_has_tbm             boot_cpu_has(X86_FEATURE_TBM)
 
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index ff583cf0ed..1f5a5d0e38 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -117,6 +117,7 @@
 #define  PASID_VALID                        (_AC(1, ULL) << 31)
 
 #define MSR_K8_VM_CR                        0xc0010114
+#define  VM_CR_INIT_REDIRECTION             (_AC(1, ULL) <<  1)
 #define  VM_CR_SVM_DISABLE                  (_AC(1, ULL) <<  4)
 
 /*
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 9acb80fdcd..d5f467d245 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -631,6 +631,12 @@ static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model,
 extern int8_t opt_tsx, cpu_has_tsx_ctrl;
 void tsx_init(void);
 
+enum ap_boot_method {
+    AP_BOOT_NORMAL,
+    AP_BOOT_SKINIT,
+};
+extern enum ap_boot_method ap_boot_method;
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_X86_PROCESSOR_H */
-- 
2.11.0

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

[Jan Beulich]

On 29.01.2021 12:45, Andrew Cooper wrote:
> From: Norbert Kamiński <norbert.kaminski@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.
> 
> 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@3mdeb.com>
> Signed-off-by: Norbert Kamiński <norbert.kaminski@3mdeb.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>