Xen upstream #4

Asiderr · 2020-12-23T17:23:16Z

No description provided.

krystian-hebel · 2020-12-23T17:35:19Z

xen/arch/x86/cpu/common.c

+ * initialized with AMD SKINIT. Also funtion clears the INIT_R bit, which
+ * indicates that the CPU was initialized with AMD SKINIT.


INIT_R is not cleared in this commit, so the comment does not reflect the code.

I've merged first to commits.

krystian-hebel · 2020-12-23T17:47:18Z

xen/arch/x86/cpu/common.c

+	 * 12th bit in ECX register for CPUID function numbered 8000_0001h
+	 * indicates support for SKINIT.
+	 */
+	if (cpuid_ecx(0x80000001) & 0x1000)


I think you should also check if this is AMD processor. On Intel this bit is marked as reserved in current SDM, but they may decide to use it at some point in the future.

I added the AuthenticAMD check.

krystian-hebel · 2020-12-23T17:52:42Z

xen/include/asm-x86/processor.h

@@ -586,6 +586,8 @@ static inline void enable_nmis(void)
                     [cs] "r" (__HYPERVISOR_CS) );
 }

+void clr_initr_stgi(void);


This probably won't be called outside of common.c, so maybe make it static there and don't declare it here?

krystian-hebel · 2020-12-23T17:54:22Z

xen/arch/x86/smpboot.c

@@ -413,6 +415,8 @@ void start_secondary(void *unused)

    /* We can take interrupts now: we're officially "up". */
    local_irq_enable();
+    if (__xen_run_by_SKINIT)


Shouldn't this be __cpu_SKINIT?

krystian-hebel · 2020-12-23T17:57:13Z

xen/arch/x86/smpboot.c

@@ -431,49 +435,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 (!__cpu_SKINIT)


Please add a comment that after SKINIT APs are already in wait-for-SIPI state and sending another INIT would break security, and something similar in the commit message.

https://www.amd.com/system/files/TechDocs/24593.pdf section 15.27.8:

A Startup IPI normally causes an AP to start execution at a location provided by the IPI. To support secure MP startup, each AP responds to a startup IPI by additionally clearing its GIF and setting the DPD, R_INIT and DIS_A20M flags in the VM_CR registerif, and only if, the BSP has indicated that it has executed an SKINIT. All other aspects of Startup IPI behavior remain unchanged.

and then a bit lower in AP Startup Sequence:

Care must also be taken to avoid issuing another INIT IPI from any processor after the BSP executes SKINIT and before all APs have received a Startup IPI, as this could compromise the integrity of AP initialization.

I added the comment and the commit message.

andyhhp · 2020-12-30T17:59:35Z

xen/arch/x86/smpboot.c

@@ -413,6 +415,9 @@ void start_secondary(void *unused)

    /* We can take interrupts now: we're officially "up". */
    local_irq_enable();
+    /* If cpu was run with SKINIT, set global interrupts flag (GIF). */
+    if (__cpu_SKINIT)
+        svm_stgi();


cpu_init() is already called earlier in start_secondary(). No need for any SKINIT logic here

andyhhp · 2020-12-30T18:04:02Z

xen/arch/x86/cpu/common.c

+		rdmsrl(MSR_K8_VM_CR, msr_content);
+		if (msr_content & K8_VMCR_R_INIT)
+		{
+			/* Clear INIT_R*/


This comment needs extending. It is strictly necessary to clear R_INIT before stgi, so a pending INIT doesn't turn into #SX and we panic with an unknown exception.

andyhhp · 2020-12-30T18:21:43Z

xen/arch/x86/cpu/common.c

+	char id[12];
+	uint32_t eax = 0;
+	uint64_t msr_content;
+
+	/*
+	 * 12th bit in ECX register for CPUID function numbered 8000_0001h
+	 * indicates support for SKINIT.
+	 */
+	cpuid(0, &eax, (uint32_t *)&id[0], (uint32_t *)&id[8],
+		(uint32_t *)&id[4]);
+	if ((memcmp(id, "AuthenticAMD", 12) == 0)
+		&& (cpuid_ecx(0x80000001) & 0x1000))


Use this hunk:

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)

to wrap Xen's existing CPUID infrastructure, then use simply if ( cpu_has_skinit ) here. However, if you use it in its opposite form, you reduce the quantity of indentation in the rest of the function.

Amongst other things, you don't want to rule out Hygon CPUs which I believe also have SKINIT support.

andyhhp · 2020-12-30T18:24:07Z

xen/arch/x86/cpu/common.c

+		&& (cpuid_ecx(0x80000001) & 0x1000))
+	{
+		/* Check is R_INIT bit set to determinate if xen was run by SKINIT */
+		rdmsrl(MSR_K8_VM_CR, msr_content);


It's probably best to use rdmsr_safe() and wrmsr_safe() lower down. I don't trust virtual environments to get this emulation correct ;)

andyhhp · 2020-12-30T18:36:19Z

xen/arch/x86/cpu/common.c

+	 * When CPU is initialized by AMD SKINIT, set GIF before before enabling
+	 * NMIs. Otherwise CPU will not be able to drop into NMI context.


Its not really about enabling NMIs - that was merely the first thing which broke. Its about enabling all interrupt sources. How about /* When the CPU is initialised by AMD SKINIT, GIF is clear to prevent external interrupts interfering with secure startup. Re-enable all interrupts now that we are suitably set up. */

andyhhp · 2020-12-30T18:41:31Z

xen/include/asm-x86/msr-index.h

@@ -321,6 +321,8 @@
 /* MSR_K8_VM_CR bits: */
 #define _K8_VMCR_SVME_DISABLE		4
 #define K8_VMCR_SVME_DISABLE		(1 << _K8_VMCR_SVME_DISABLE)
+#define _K8_VMCR_R_INIT		1
+#define K8_VMCR_R_INIT		(1 << _K8_VMCR_R_INIT)


We're attempting to drop redundant symbols. Nothing uses the shift value, so just define this as (_AC(1, ULL) << 1). I'll do a patch to clean up the adjacent SVME_DISABLE.

Cleanup patch https://lore.kernel.org/xen-devel/20201230193525.12290-1-andrew.cooper3@citrix.com/T/#u. On top of this patch, you'll want

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) /*

Asiderr · 2021-01-07T11:22:21Z

@andyhhp I added suggestions and fixes to the commits.

dozylynx · 2021-01-07T21:48:57Z

Successful test report: I have booted into Xen 4.14.1-pre with the two patches from this PR applied on a PC Engines APU2 with the current latest firmware, and it shows PCRs 17 and 18 populated. Bootable image built using meta-trenchboot and with this patch applied to grub:
3mdeb/grub#13
3mdeb/grub@529ecde

I have no-real-mode on the Xen command line, as per the wic partition-building script in meta-trenchboot, and used Linux 5.5.3 from the 3mdeb repository for the dom0 kernel. I hope that it will be possible to update to use a modern 5.10 kernel with it soon.

Thanks for this work and best of luck with integrating it upstream.

andyhhp

One other thing to say. It's getting close to the Xen 4.15 last-post deadline. If you want these fixes in Xen 4.15, v1 of them need posting to xen-devel within the next week.

In addition to these changes, are there any other bugs which need addressing? no-real-mode isn't something we can do in the timeframe (and at least there is a workaround for that), but I suspect Xen blindly assuming that the BDA exists in frame 0 probably wants fixing.

andyhhp · 2021-01-11T19:01:49Z

xen/arch/x86/cpu/common.c

+	 * 12th bit in ECX register for CPUID function numbered 8000_0001h
+	 * indicates support for SKINIT.
+	 */
+	if (cpu_has_skinit)


if ( !cpu_has_skinit ) return;

This reduces the indentation later in the function. Note the spaces as well inside the outermost brackes (yes - Xen style is a little weird, and it is further complicated here as you're modifying files which use different styles). Also, no need for the comment, seeing as there are no magic numbers in the if condition.

I added your style suggestions (Xen style is sometimes confusing) and I deleted the comment.

andyhhp · 2021-01-11T19:22:24Z

xen/arch/x86/cpu/common.c

+		/* Check is R_INIT bit set to determinate if xen was run by SKINIT */
+		rdmsr_safe(MSR_K8_VM_CR, msr_content);
+		if (msr_content & VM_CR_INIT_REDIRECTION)
+		{


Need to check the return value from rdmsr_safe().

if ( rdmsr_safe(MSR_K8_VM_CR, msr_content) || !(msr_content & VM_CR_INIT_REDIRECTION) ) return;

Also, all R_INIT's in comments need updating per the change to the VM_CR_INIT_REDIRECTION constant

andyhhp · 2021-01-11T19:31:43Z

xen/include/asm-x86/hvm/svm/svm.h

+static inline void svm_stgi(void)
+{
+    asm volatile (
+        ".byte 0x0f,0x01,0xdc" /* STGI */
+        : : : "memory" );
+}
+


Actually, please just write this inline, rather than making a helper.

For one, we don't want to include lots of HVM internals into the common boot code, and there are no other places in C to legitimately use STGI. Also, feel free to collapse it into one line, rather than 3.

andyhhp · 2021-01-11T19:32:52Z

xen/arch/x86/smpboot.c

@@ -45,10 +45,12 @@
 #include <asm/spec_ctrl.h>
 #include <asm/time.h>
 #include <asm/tboot.h>
+#include <asm/hvm/svm/svm.h>


Stale import now.

andyhhp · 2021-01-11T19:39:20Z

xen/arch/x86/smpboot.c

 #include <irq_vectors.h>
 #include <mach_apic.h>

 unsigned long __read_mostly trampoline_phys;
+extern bool __cpu_SKINIT;


We've got 3 different AP startup sequences. I think it would be better to have

enum ap_boot_method { AP_BOOT_NORMAL, AP_BOOT_TXT, AP_BOOT_SKINIT, }; extern enum ap_boot_method ap_boot_method;

in xen/include/asm-x86/processor.h (probably), with

enum ap_boot_method ap_boot_method = AP_BOOT_NORMAL;

in smpboot.c

I created switch.

andyhhp · 2021-01-11T19:42:08Z

xen/arch/x86/smpboot.c

-                   phys_apicid);
-
-    if ( !x2apic_enabled )
+    if (!__cpu_SKINIT)


This wants to turn into switch ( ap_boot_method ) (per the above enum suggestion) rather than being a nested if-else chain.

It would probably be better to split this patch into two - the first which introduces the enum, and converts the existing path (finding an appropriate point in the tboot code to set ap_boot_method = AP_BOOT_TXT rather than constantly calling tboot_in_measured_env()), and a second patch which adds the new AP_BOOT_SKINIT case.

[Edit] Depending on preference, you could keep the series as two patches. One cleanup patch for ap_boot_method and TXT, and the second patch which does both the AP_BOOT_SKINIT case and STGI in cpu_init(). This has the advantage that you introduce and set AP_BOOT_SKINIT together, rather than split across two patches.

I created the ap_boot_method for AP boot sequences and I placed ap_boot_method = AP_BOOT_TXT in tboot.c. I replaced tboot_in_measured_env() with ap_boot_method == AP_BOOT_TXT.

pietrushnic · 2021-01-11T20:48:10Z

@dozylynx thank you very much for testing and @andyhhp for review.

x86/tboot: Replace tboot_in_measured_env with AP_BOOT_TXT compare There are three different AP startup sequences which are declared in ap_boot_method. Each method could be used to recognise startup sequence. This patch replace tboot_in_measured_env is replaced with AP_BOOT_TXT comparison. Signed-off-by: Norbert Kamiński <norbert.kaminski@3mdeb.com>

krystian-hebel · 2021-01-12T20:08:26Z

xen/arch/x86/cpu/common.c

+
+	if ( !cpu_has_skinit )
+		return;
+	if ( cpu_has_skinit )


Drop this if, it is redundant

x86/cpu: Clear R_INIT bit and set AP_BOOT_SKINIT When CPU is initialized with AMD SKINIT, GIF is clear to prevent external interrupts interfering with secure startup. STGI re-enable all interrupts, when the CPU is suitably set up. R_INIT bit indicates that CPU was initialized with SKINIT. This patch clears R_INIT bit to prevent turning INIT into security exception (#SX). It would cause a panic with an unknown exception. The AP_BOOT_SKINIT ap_boot_method replace R_INIT bit. Signed-off-by: Marek Kasiewicz <marek.kasiewicz@3mdeb.com> Signed-off-by: Norbert Kamiński <norbert.kaminski@3mdeb.com>

krystian-hebel · 2021-01-15T16:13:56Z

I have no-real-mode on the Xen command line, as per the wic partition-building script in meta-trenchboot, and used Linux 5.5.3 from the 3mdeb repository for the dom0 kernel. I hope that it will be possible to update to use a modern 5.10 kernel with it soon.

FYI, you don't need a special kernel for dom0 (at least for now, when only AMD is supported), unmodified one will do just fine - every measurement happens earlier, in LZ, thanks to condensed and easily parsable format of Multiboot2. It almost seems as if it was developed with this use case in mind, though I doubt its creators were aware of it back then 😃

Asiderr requested review from macpijan and krystian-hebel December 23, 2020 17:23

Asiderr changed the title ~~Xen upstream~~ WIP: Xen upstream Dec 23, 2020

Asiderr force-pushed the xen_upstream branch 2 times, most recently from e978ba3 to 148e83a Compare December 23, 2020 17:40

krystian-hebel requested changes Dec 23, 2020

View reviewed changes

Asiderr force-pushed the xen_upstream branch 3 times, most recently from 79e1012 to c94cccd Compare December 28, 2020 15:32

Asiderr changed the title ~~WIP: Xen upstream~~ Xen upstream Dec 28, 2020

Asiderr force-pushed the xen_upstream branch from c94cccd to f8f4891 Compare December 30, 2020 00:41

andyhhp reviewed Dec 30, 2020

View reviewed changes

Asiderr force-pushed the xen_upstream branch from f8f4891 to e2434ff Compare January 7, 2021 11:17

andyhhp reviewed Jan 11, 2021

View reviewed changes

Asiderr force-pushed the xen_upstream branch from e2434ff to 7ebb1f8 Compare January 12, 2021 20:00

krystian-hebel reviewed Jan 12, 2021

View reviewed changes

Asiderr force-pushed the xen_upstream branch from 7ebb1f8 to efa0c02 Compare January 12, 2021 20:15

Asiderr force-pushed the xen_upstream branch from efa0c02 to 03e0e8c Compare January 12, 2021 20:27

		* initialized with AMD SKINIT. Also funtion clears the INIT_R bit, which
		* indicates that the CPU was initialized with AMD SKINIT.

		* When CPU is initialized by AMD SKINIT, set GIF before before enabling
		* NMIs. Otherwise CPU will not be able to drop into NMI context.

Xen upstream #4

Are you sure you want to change the base?

Xen upstream #4

Conversation

Asiderr commented Dec 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Asiderr Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krystian-hebel Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Asiderr Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyhhp Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Asiderr commented Jan 7, 2021

dozylynx commented Jan 7, 2021 • edited Loading

andyhhp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyhhp Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyhhp Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietrushnic commented Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krystian-hebel commented Jan 15, 2021

Asiderr Dec 28, 2020 •

edited

Loading

krystian-hebel Dec 23, 2020 •

edited

Loading

Asiderr Dec 28, 2020 •

edited

Loading

andyhhp Dec 30, 2020 •

edited

Loading

dozylynx commented Jan 7, 2021 •

edited

Loading

andyhhp Jan 11, 2021 •

edited

Loading

andyhhp Jan 11, 2021 •

edited

Loading

pietrushnic commented Jan 11, 2021 •

edited

Loading