[PATCH 0/5] Fix HVM domain featuresets when BIOS sets max_leaf limit

[PATCH 0/5] Fix HVM domain featuresets when BIOS sets max_leaf limit

[Andrew Cooper]

This patch series fixes an observed regression between Xen 4.1 and Xen 4.4 on
two particular IvyBridge DT systems, where windows 8.1 will work fine under
Xen 4.1, but unconditionally BSOD under Xen 4.4

The problem turns out to be specific to the BIOS settings on these two boxes.
When IA32_MISC_ENABLE[22] is set, the CPUID max leaf is limited to 3 for
legacy compatibility reasons.  Xen knows how to disable this limit and does
so, but only after trying to fix the CPUID.7[ECX=0] feature flags, which are
ignored base on a max_leaf test.  This causes the cpu_has_XXX tests for these
features to return false despite the feature actually being available.

This regression was introduced as a side effect of c/s 44e24f8567 "x86: don't
call generic_identify() redundantly" where the redundant call actually
resampled CPUID.7[ECX=0] properly to obtain the feature flags.

>From Windows point of view, sees the FSGSBASE feature but gets a #GP fault
when attempting to set it in CR4, as the Xen cpu_has_fsgsbase check fails.
(Windows then appears to forget that it failed to enable FSGSBASE, and BSODs
when it gets a further #GP fault the first time it uses the new instructions.)

Patch 1 is a misc newline corruption I discovered during debugging of the
issue, and is otherwise unrelated to the series.

Patch 2 is the main bugfix.

Patches 3 and 4 are improvements to hvmloader, as prerequisites to Patch 5
which introduces an hvmloader test for the WRFSBASE instruction.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

Andrew Cooper (5):
  x86/cpu: Newline on 'invalid siblings' warning
  x86/cpu: Undo BIOS CPUID max_leaf limit before querying for features.
  hvmloader/tests: use .code64 in 64bit snippets
  hvmloader: Introduce cpuid_count() helper function
  hvmloader/tests: Introduce WRFSBASE test

 tools/firmware/hvmloader/tests.c |   76 +++++++++++++++++++++++++++++++++++---
 tools/firmware/hvmloader/util.c  |    9 -----
 tools/firmware/hvmloader/util.h  |   17 +++++++--
 xen/arch/x86/cpu/common.c        |    9 +++--
 4 files changed, 91 insertions(+), 20 deletions(-)

-- 
1.7.10.4

[PATCH 1/5] x86/cpu: Newline on 'invalid siblings' warning

[Andrew Cooper]

Reindent to keep the line within 80 characters.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpu/common.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 0741696..56c552c 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -444,7 +444,9 @@ void __cpuinit detect_ht(struct cpuinfo_x86 *c)
 	} else if (c->x86_num_siblings > 1 ) {
 
 		if (c->x86_num_siblings > nr_cpu_ids) {
-			printk(KERN_WARNING "CPU: Unsupported number of the siblings %d", c->x86_num_siblings);
+			printk(KERN_WARNING
+			       "CPU: Unsupported number of the siblings %d\n",
+			       c->x86_num_siblings);
 			c->x86_num_siblings = 1;
 			return;
 		}
-- 
1.7.10.4

Re: [PATCH 1/5] x86/cpu: Newline on 'invalid siblings' warning

[Jan Beulich]

>>> On 29.07.14 at 16:29, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -444,7 +444,9 @@ void __cpuinit detect_ht(struct cpuinfo_x86 *c)
>  	} else if (c->x86_num_siblings > 1 ) {
>  
>  		if (c->x86_num_siblings > nr_cpu_ids) {
> -			printk(KERN_WARNING "CPU: Unsupported number of the siblings %d", c->x86_num_siblings);
> +			printk(KERN_WARNING
> +			       "CPU: Unsupported number of the siblings %d\n",
> +			       c->x86_num_siblings);

Since you have to fiddle with this anyway, can you limit the printing
to BP or opt_cpu_info? That said, it's rather bogus a check anyway -
if one built a hypervisor for just 2 CPUs (let's assume for a minute
that nr_cpu_ids then won't be larger than 2) and ran on a 4-fold
HT CPU, this would trigger for no good reason. Hence the whole
conditional could as well go away imo (and current Linux indeed
doesn't have it or any equivalent anymore).

As to indentation - if you want to keep it, how about moving it up
as a middle "else if" prior to the enclosing one?

Jan

Re: [PATCH 1/5] x86/cpu: Newline on 'invalid siblings' warning

[Andrew Cooper]

On 29/07/14 16:10, Jan Beulich wrote:
>>>> On 29.07.14 at 16:29, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -444,7 +444,9 @@ void __cpuinit detect_ht(struct cpuinfo_x86 *c)
>>  	} else if (c->x86_num_siblings > 1 ) {
>>  
>>  		if (c->x86_num_siblings > nr_cpu_ids) {
>> -			printk(KERN_WARNING "CPU: Unsupported number of the siblings %d", c->x86_num_siblings);
>> +			printk(KERN_WARNING
>> +			       "CPU: Unsupported number of the siblings %d\n",
>> +			       c->x86_num_siblings);
> Since you have to fiddle with this anyway, can you limit the printing
> to BP or opt_cpu_info? That said, it's rather bogus a check anyway -
> if one built a hypervisor for just 2 CPUs (let's assume for a minute
> that nr_cpu_ids then won't be larger than 2) and ran on a 4-fold
> HT CPU, this would trigger for no good reason. Hence the whole
> conditional could as well go away imo (and current Linux indeed
> doesn't have it or any equivalent anymore).
>
> As to indentation - if you want to keep it, how about moving it up
> as a middle "else if" prior to the enclosing one?
>
> Jan
>

Hmm - I thought it was somewhat silly but opted for not changing what
was there.

Given that it is clearly bogus, I will just drop the entire clause.

~Andrew

[PATCH 2/5] x86/cpu: Undo BIOS CPUID max_leaf limit before querying for features.

[Andrew Cooper]

If IA32_MISC_ENABLE[22] is set by the BIOS, CPUID.0.EAX will be limited to 3.
Lift this limit before considering whether to query CPUID.7[ECX=0].EBX for
features.

Without this change, dom0 is able to see this feature leaf (as the limit was
subsequently lifted), and will set features appropriately in HVM domain cpuid
policies.

The specific bug XenServer observed was the advertisement of the FSGSBASE
feature, but an inability to set CR4.FSGSBASE as Xen considered the bit to be
reserved as cpu_has_fsgsbase incorrectly evaluated as false.

This is a regression introduced by c/s 44e24f8567 "x86: don't call
generic_identify() redundantly" where the redundant call actually resampled
CPUID.7[ECX=0] properly to obtain the feature flags.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>

---

This fix should be backported as far as Xen 4.2
---
 xen/arch/x86/cpu/common.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 56c552c..a3bc5fd 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -250,14 +250,15 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 			paddr_bits = cpuid_eax(0x80000008) & 0xff;
 	}
 
+	/* Might lift BIOS max_leaf=3 limit. */
+	early_intel_workaround(c);
+
 	/* Intel-defined flags: level 0x00000007 */
 	if ( c->cpuid_level >= 0x00000007 ) {
 		u32 dummy;
 		cpuid_count(0x00000007, 0, &dummy, &ebx, &dummy, &dummy);
 		c->x86_capability[X86_FEATURE_FSGSBASE / 32] = ebx;
 	}
-
-	early_intel_workaround(c);
 }
 
 /*
-- 
1.7.10.4

[PATCH 3/5] hvmloader/tests: use .code64 in 64bit snippets

[Andrew Cooper]

No functional change, but makes the code rather more legible.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/firmware/hvmloader/tests.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c
index 52772aa..37d16f8 100644
--- a/tools/firmware/hvmloader/tests.c
+++ b/tools/firmware/hvmloader/tests.c
@@ -170,12 +170,14 @@ static int shadow_gs_test(void)
         /* Push LRETQ stack frame. */
         "pushl $0; pushl $"STR(SEL_CODE32)"; pushl $0; pushl $2f; "
         /* Jump to 64-bit mode. */
-        "ljmp $"STR(SEL_CODE64)",$1f; 1: "
+        "ljmp $"STR(SEL_CODE64)",$1f; "
+        ".code64; 1: "
         /* Swap GS_BASE and SHADOW_GS_BASE */
-        ".byte 0x0f,0x01,0xf8; " /* SWAPGS */
+        "swapgs; "
         /* Jump to 32-bit mode. */
-        ".byte 0x89, 0xe4; "     /* MOV ESP,ESP */
-        ".byte 0x48, 0xcb; 2: "  /* LRETQ */
+        "movl %%esp,%%esp; "
+        "lretq; "
+        ".code32; 2:"
         /* Read SHADOW_GS_BASE: should now contain 2 */
         "mov $0xc0000102,%%ecx; rdmsr; mov %%eax,%%ebx; "
         /* CR0.PG=0 */
-- 
1.7.10.4

Re: [PATCH 3/5] hvmloader/tests: use .code64 in 64bit snippets

[Jan Beulich]

>>> On 29.07.14 at 16:29, <andrew.cooper3@citrix.com> wrote:
> No functional change, but makes the code rather more legible.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH 4/5] hvmloader: Introduce cpuid_count() helper function

[Andrew Cooper]

Also promote the regular cpuid() function to a static inline in util.h, as
this offers far better register in the generated code than an unconditional
call into a separate translation unit.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/firmware/hvmloader/util.c |    9 ---------
 tools/firmware/hvmloader/util.h |   17 ++++++++++++++---
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 80d822f..29da0d5 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -306,15 +306,6 @@ memcmp(const void *s1, const void *s2, unsigned n)
     return 0;
 }
 
-void
-cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
-{
-    asm volatile (
-        "cpuid"
-        : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
-        : "0" (idx) );
-}
-
 static const char hex_digits[] = "0123456789abcdef";
 
 /* Write a two-character hex representation of 'byte' to digits[].
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index a70e4aa..b3853e3 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -97,9 +97,20 @@ int uart_exists(uint16_t uart_base);
 int lpt_exists(uint16_t lpt_base);
 int hpet_exists(unsigned long hpet_base);
 
-/* Do cpuid instruction, with operation 'idx' */
-void cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx,
-           uint32_t *ecx, uint32_t *edx);
+static inline void cpuid(uint32_t idx, uint32_t *eax,
+                         uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
+{
+    asm volatile ("cpuid" : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
+                  : "0" (idx));
+}
+
+static inline void cpuid_count(uint32_t idx, uint32_t subleaf, uint32_t *eax,
+                               uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
+{
+    asm volatile ("cpuid" : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
+                  : "0" (idx), "2" (subleaf));
+}
+
 
 /* Read the TSC register. */
 static inline uint64_t rdtsc(void)
-- 
1.7.10.4

[PATCH 5/5] hvmloader/tests: Introduce WRFSBASE test

[Andrew Cooper]

Without the bugfix in c/s <INSERT REFERENCE TO PATCH 2>, the first move to cr4
may fail despite the feature being advertised.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

If this patch is acceptable, could the committer insert an appropriate
reference to patch 2 above?
---
 tools/firmware/hvmloader/tests.c |   66 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c
index 37d16f8..a30e738 100644
--- a/tools/firmware/hvmloader/tests.c
+++ b/tools/firmware/hvmloader/tests.c
@@ -192,6 +192,70 @@ static int shadow_gs_test(void)
     return (ebx == 2) ? TEST_PASS : TEST_FAIL;
 }
 
+static int wrfsbase_test(void)
+{
+    uint64_t *pd = (uint64_t *)PD_START;
+    uint32_t i, eax, ebx, ecx, edx;
+
+    /* Skip this test if the CPU does not support long mode. */
+    cpuid(0x80000000, &eax, &ebx, &ecx, &edx);
+    if ( eax < 0x80000001 )
+        return TEST_SKIP;
+    cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
+    if ( !(edx & (1u<<29)) )
+        return TEST_SKIP;
+    /* Skip this test if the CPU does not support fsgsbase. */
+    cpuid(0, &eax, &ebx, &ecx, &edx);
+    if ( eax < 7 )
+        return TEST_SKIP;
+    cpuid_count(7, 0, &eax, &ebx, &ecx, &edx);
+    if ( !(ebx & (1u<<0)) )
+        return TEST_SKIP;
+
+    /* Long mode pagetable setup: Identity map 0-16MB with 2MB mappings. */
+    *pd = (unsigned long)pd + 0x1007; /* Level 4 */
+    pd += 512;
+    *pd = (unsigned long)pd + 0x1007; /* Level 3 */
+    pd += 512;
+    for ( i = 0; i < 8; i++ )         /* Level 2 */
+        *pd++ = (i << 21) + 0x1e3;
+
+    asm volatile (
+        /* CR4.{FSGSBASE,PAE}=1 */
+        "mov $0x10020,%%ebx; "
+        "mov %%ebx,%%cr4; "
+        /* CR3 */
+        "mov %%eax,%%cr3; "
+        /* EFER.LME=1 */
+        "mov $0xc0000080,%%ecx; rdmsr; btsl $8,%%eax; wrmsr; "
+        /* CR0.PG=1 */
+        "mov %%cr0,%%eax; btsl $31,%%eax; mov %%eax,%%cr0; "
+        "jmp 1f; 1: "
+        /* Push LRETQ stack frame. */
+        "pushl $0; pushl $"STR(SEL_CODE32)"; pushl $0; pushl $2f; "
+        /* Jump to 64-bit mode. */
+        "ljmp $"STR(SEL_CODE64)",$1f; "
+        ".code64; 1: "
+        "movl $0x234,%%eax; "
+        ".byte 0xf3, 0x48, 0x0f, 0xae, 0xd0; " /* WRFSBASE %rax */
+        /* Jump to 32-bit mode. */
+        "movl %%esp,%%esp; "
+        "lretq; "
+        ".code32; 2:"
+        /* Read FS_BASE: should now contain 0x234 */
+        "mov $0xc0000100,%%ecx; rdmsr; mov %%eax,%%ebx; "
+        /* CR0.PG=0 */
+        "mov %%cr0,%%eax; btcl $31,%%eax; mov %%eax,%%cr0; "
+        "jmp 1f; 1:"
+        /* EFER.LME=0 */
+        "mov $0xc0000080,%%ecx; rdmsr; btcl $8,%%eax; wrmsr; "
+        /* CR4.PAE=0 */
+        "xor %%eax,%%eax; mov %%eax,%%cr4; "
+        : "=b" (ebx) : "a" (PD_START) : "ecx", "edx", "memory" );
+
+    return (ebx == 0x234) ? TEST_PASS : TEST_FAIL;
+}
+
 void perform_tests(void)
 {
     int i, passed, skipped;
@@ -202,6 +266,7 @@ void perform_tests(void)
     } tests[] = {
         { rep_io_test, "REP INSB across page boundaries" },
         { shadow_gs_test, "GS base MSRs and SWAPGS" },
+        { wrfsbase_test, "FS base MSR and WRFSBASE" },
         { NULL, NULL }
     };
 
@@ -244,7 +309,6 @@ void perform_tests(void)
         BUG();
     }
 }
-
 /*
  * Local variables:
  * mode: C
-- 
1.7.10.4

Re: [PATCH 5/5] hvmloader/tests: Introduce WRFSBASE test

[Jan Beulich]

>>> On 29.07.14 at 16:30, <andrew.cooper3@citrix.com> wrote:
> Without the bugfix in c/s <INSERT REFERENCE TO PATCH 2>, the first move to cr4
> may fail despite the feature being advertised.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> ---
> 
> If this patch is acceptable, could the committer insert an appropriate
> reference to patch 2 above?
> ---
>  tools/firmware/hvmloader/tests.c |   66 
> +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/tests.c 
> b/tools/firmware/hvmloader/tests.c
> index 37d16f8..a30e738 100644
> --- a/tools/firmware/hvmloader/tests.c
> +++ b/tools/firmware/hvmloader/tests.c
> @@ -192,6 +192,70 @@ static int shadow_gs_test(void)
>      return (ebx == 2) ? TEST_PASS : TEST_FAIL;
>  }
>  
> +static int wrfsbase_test(void)
> +{
> +    uint64_t *pd = (uint64_t *)PD_START;
> +    uint32_t i, eax, ebx, ecx, edx;
> +
> +    /* Skip this test if the CPU does not support long mode. */
> +    cpuid(0x80000000, &eax, &ebx, &ecx, &edx);
> +    if ( eax < 0x80000001 )
> +        return TEST_SKIP;
> +    cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
> +    if ( !(edx & (1u<<29)) )
> +        return TEST_SKIP;
> +    /* Skip this test if the CPU does not support fsgsbase. */
> +    cpuid(0, &eax, &ebx, &ecx, &edx);
> +    if ( eax < 7 )
> +        return TEST_SKIP;
> +    cpuid_count(7, 0, &eax, &ebx, &ecx, &edx);
> +    if ( !(ebx & (1u<<0)) )
> +        return TEST_SKIP;
> +
> +    /* Long mode pagetable setup: Identity map 0-16MB with 2MB mappings. */
> +    *pd = (unsigned long)pd + 0x1007; /* Level 4 */
> +    pd += 512;
> +    *pd = (unsigned long)pd + 0x1007; /* Level 3 */
> +    pd += 512;
> +    for ( i = 0; i < 8; i++ )         /* Level 2 */
> +        *pd++ = (i << 21) + 0x1e3;
> +
> +    asm volatile (
> +        /* CR4.{FSGSBASE,PAE}=1 */
> +        "mov $0x10020,%%ebx; "
> +        "mov %%ebx,%%cr4; "
> +        /* CR3 */
> +        "mov %%eax,%%cr3; "
> +        /* EFER.LME=1 */
> +        "mov $0xc0000080,%%ecx; rdmsr; btsl $8,%%eax; wrmsr; "
> +        /* CR0.PG=1 */
> +        "mov %%cr0,%%eax; btsl $31,%%eax; mov %%eax,%%cr0; "
> +        "jmp 1f; 1: "
> +        /* Push LRETQ stack frame. */
> +        "pushl $0; pushl $"STR(SEL_CODE32)"; pushl $0; pushl $2f; "
> +        /* Jump to 64-bit mode. */
> +        "ljmp $"STR(SEL_CODE64)",$1f; "
> +        ".code64; 1: "
> +        "movl $0x234,%%eax; "
> +        ".byte 0xf3, 0x48, 0x0f, 0xae, 0xd0; " /* WRFSBASE %rax */
> +        /* Jump to 32-bit mode. */
> +        "movl %%esp,%%esp; "
> +        "lretq; "
> +        ".code32; 2:"
> +        /* Read FS_BASE: should now contain 0x234 */
> +        "mov $0xc0000100,%%ecx; rdmsr; mov %%eax,%%ebx; "

I don't think it's generally safe to boot an arbitrary OS with FS.base
non-zero. Just reload %fs before exiting the function.

> +        /* CR0.PG=0 */
> +        "mov %%cr0,%%eax; btcl $31,%%eax; mov %%eax,%%cr0; "
> +        "jmp 1f; 1:"
> +        /* EFER.LME=0 */
> +        "mov $0xc0000080,%%ecx; rdmsr; btcl $8,%%eax; wrmsr; "
> +        /* CR4.PAE=0 */
> +        "xor %%eax,%%eax; mov %%eax,%%cr4; "
> +        : "=b" (ebx) : "a" (PD_START) : "ecx", "edx", "memory" );

%ebx gets modified by other than the last instruction and hence
should, even if it doesn't seem to matter right now, be marked as
early-clobber. Similarly %eax gets modified and hence should be
only and input.

> @@ -244,7 +309,6 @@ void perform_tests(void)
>          BUG();
>      }
>  }
> -
>  /*
>   * Local variables:
>   * mode: C

???

Jan

Re: [PATCH 5/5] hvmloader/tests: Introduce WRFSBASE test

[Andrew Cooper]

On 29/07/14 16:22, Jan Beulich wrote:
>>>> On 29.07.14 at 16:30, <andrew.cooper3@citrix.com> wrote:
>> Without the bugfix in c/s <INSERT REFERENCE TO PATCH 2>, the first move to cr4
>> may fail despite the feature being advertised.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>
>> ---
>>
>> If this patch is acceptable, could the committer insert an appropriate
>> reference to patch 2 above?
>> ---
>>  tools/firmware/hvmloader/tests.c |   66 
>> +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/firmware/hvmloader/tests.c 
>> b/tools/firmware/hvmloader/tests.c
>> index 37d16f8..a30e738 100644
>> --- a/tools/firmware/hvmloader/tests.c
>> +++ b/tools/firmware/hvmloader/tests.c
>> @@ -192,6 +192,70 @@ static int shadow_gs_test(void)
>>      return (ebx == 2) ? TEST_PASS : TEST_FAIL;
>>  }
>>  
>> +static int wrfsbase_test(void)
>> +{
>> +    uint64_t *pd = (uint64_t *)PD_START;
>> +    uint32_t i, eax, ebx, ecx, edx;
>> +
>> +    /* Skip this test if the CPU does not support long mode. */
>> +    cpuid(0x80000000, &eax, &ebx, &ecx, &edx);
>> +    if ( eax < 0x80000001 )
>> +        return TEST_SKIP;
>> +    cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
>> +    if ( !(edx & (1u<<29)) )
>> +        return TEST_SKIP;
>> +    /* Skip this test if the CPU does not support fsgsbase. */
>> +    cpuid(0, &eax, &ebx, &ecx, &edx);
>> +    if ( eax < 7 )
>> +        return TEST_SKIP;
>> +    cpuid_count(7, 0, &eax, &ebx, &ecx, &edx);
>> +    if ( !(ebx & (1u<<0)) )
>> +        return TEST_SKIP;
>> +
>> +    /* Long mode pagetable setup: Identity map 0-16MB with 2MB mappings. */
>> +    *pd = (unsigned long)pd + 0x1007; /* Level 4 */
>> +    pd += 512;
>> +    *pd = (unsigned long)pd + 0x1007; /* Level 3 */
>> +    pd += 512;
>> +    for ( i = 0; i < 8; i++ )         /* Level 2 */
>> +        *pd++ = (i << 21) + 0x1e3;
>> +
>> +    asm volatile (
>> +        /* CR4.{FSGSBASE,PAE}=1 */
>> +        "mov $0x10020,%%ebx; "
>> +        "mov %%ebx,%%cr4; "
>> +        /* CR3 */
>> +        "mov %%eax,%%cr3; "
>> +        /* EFER.LME=1 */
>> +        "mov $0xc0000080,%%ecx; rdmsr; btsl $8,%%eax; wrmsr; "
>> +        /* CR0.PG=1 */
>> +        "mov %%cr0,%%eax; btsl $31,%%eax; mov %%eax,%%cr0; "
>> +        "jmp 1f; 1: "
>> +        /* Push LRETQ stack frame. */
>> +        "pushl $0; pushl $"STR(SEL_CODE32)"; pushl $0; pushl $2f; "
>> +        /* Jump to 64-bit mode. */
>> +        "ljmp $"STR(SEL_CODE64)",$1f; "
>> +        ".code64; 1: "
>> +        "movl $0x234,%%eax; "
>> +        ".byte 0xf3, 0x48, 0x0f, 0xae, 0xd0; " /* WRFSBASE %rax */
>> +        /* Jump to 32-bit mode. */
>> +        "movl %%esp,%%esp; "
>> +        "lretq; "
>> +        ".code32; 2:"
>> +        /* Read FS_BASE: should now contain 0x234 */
>> +        "mov $0xc0000100,%%ecx; rdmsr; mov %%eax,%%ebx; "
> I don't think it's generally safe to boot an arbitrary OS with FS.base
> non-zero. Just reload %fs before exiting the function.

The large asm block at the top of hvmloader.c reloads all segment
registers with SEL_DATA16 while in 32bit mode, before dropping into 16
bit mode and zeroing them again.  We are completely safe as long as none
of the upper 32bits get set while in 64bit mode, which is indeed the case.

>
>> +        /* CR0.PG=0 */
>> +        "mov %%cr0,%%eax; btcl $31,%%eax; mov %%eax,%%cr0; "
>> +        "jmp 1f; 1:"
>> +        /* EFER.LME=0 */
>> +        "mov $0xc0000080,%%ecx; rdmsr; btcl $8,%%eax; wrmsr; "
>> +        /* CR4.PAE=0 */
>> +        "xor %%eax,%%eax; mov %%eax,%%cr4; "
>> +        : "=b" (ebx) : "a" (PD_START) : "ecx", "edx", "memory" );
> %ebx gets modified by other than the last instruction and hence
> should, even if it doesn't seem to matter right now, be marked as
> early-clobber. Similarly %eax gets modified and hence should be
> only and input.

This will want to be fixed in the gs test as well.  I will fix both.

>
>> @@ -244,7 +309,6 @@ void perform_tests(void)
>>          BUG();
>>      }
>>  }
>> -
>>  /*
>>   * Local variables:
>>   * mode: C
> ???
>
> Jan
>

Oops - I think some debugging slipped in.  That was unintentional.

~Andrew

Re: [PATCH 5/5] hvmloader/tests: Introduce WRFSBASE test

[Jan Beulich]

>>> On 29.07.14 at 17:34, <andrew.cooper3@citrix.com> wrote:
> On 29/07/14 16:22, Jan Beulich wrote:
>>>>> On 29.07.14 at 16:30, <andrew.cooper3@citrix.com> wrote:
>>> Without the bugfix in c/s <INSERT REFERENCE TO PATCH 2>, the first move to cr4
>>> may fail despite the feature being advertised.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Keir Fraser <keir@xen.org>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Tim Deegan <tim@xen.org>
>>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>
>>> ---
>>>
>>> If this patch is acceptable, could the committer insert an appropriate
>>> reference to patch 2 above?
>>> ---
>>>  tools/firmware/hvmloader/tests.c |   66 
>>> +++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 65 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/firmware/hvmloader/tests.c 
>>> b/tools/firmware/hvmloader/tests.c
>>> index 37d16f8..a30e738 100644
>>> --- a/tools/firmware/hvmloader/tests.c
>>> +++ b/tools/firmware/hvmloader/tests.c
>>> @@ -192,6 +192,70 @@ static int shadow_gs_test(void)
>>>      return (ebx == 2) ? TEST_PASS : TEST_FAIL;
>>>  }
>>>  
>>> +static int wrfsbase_test(void)
>>> +{
>>> +    uint64_t *pd = (uint64_t *)PD_START;
>>> +    uint32_t i, eax, ebx, ecx, edx;
>>> +
>>> +    /* Skip this test if the CPU does not support long mode. */
>>> +    cpuid(0x80000000, &eax, &ebx, &ecx, &edx);
>>> +    if ( eax < 0x80000001 )
>>> +        return TEST_SKIP;
>>> +    cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
>>> +    if ( !(edx & (1u<<29)) )
>>> +        return TEST_SKIP;
>>> +    /* Skip this test if the CPU does not support fsgsbase. */
>>> +    cpuid(0, &eax, &ebx, &ecx, &edx);
>>> +    if ( eax < 7 )
>>> +        return TEST_SKIP;
>>> +    cpuid_count(7, 0, &eax, &ebx, &ecx, &edx);
>>> +    if ( !(ebx & (1u<<0)) )
>>> +        return TEST_SKIP;
>>> +
>>> +    /* Long mode pagetable setup: Identity map 0-16MB with 2MB mappings. */
>>> +    *pd = (unsigned long)pd + 0x1007; /* Level 4 */
>>> +    pd += 512;
>>> +    *pd = (unsigned long)pd + 0x1007; /* Level 3 */
>>> +    pd += 512;
>>> +    for ( i = 0; i < 8; i++ )         /* Level 2 */
>>> +        *pd++ = (i << 21) + 0x1e3;
>>> +
>>> +    asm volatile (
>>> +        /* CR4.{FSGSBASE,PAE}=1 */
>>> +        "mov $0x10020,%%ebx; "
>>> +        "mov %%ebx,%%cr4; "
>>> +        /* CR3 */
>>> +        "mov %%eax,%%cr3; "
>>> +        /* EFER.LME=1 */
>>> +        "mov $0xc0000080,%%ecx; rdmsr; btsl $8,%%eax; wrmsr; "
>>> +        /* CR0.PG=1 */
>>> +        "mov %%cr0,%%eax; btsl $31,%%eax; mov %%eax,%%cr0; "
>>> +        "jmp 1f; 1: "
>>> +        /* Push LRETQ stack frame. */
>>> +        "pushl $0; pushl $"STR(SEL_CODE32)"; pushl $0; pushl $2f; "
>>> +        /* Jump to 64-bit mode. */
>>> +        "ljmp $"STR(SEL_CODE64)",$1f; "
>>> +        ".code64; 1: "
>>> +        "movl $0x234,%%eax; "
>>> +        ".byte 0xf3, 0x48, 0x0f, 0xae, 0xd0; " /* WRFSBASE %rax */
>>> +        /* Jump to 32-bit mode. */
>>> +        "movl %%esp,%%esp; "
>>> +        "lretq; "
>>> +        ".code32; 2:"
>>> +        /* Read FS_BASE: should now contain 0x234 */
>>> +        "mov $0xc0000100,%%ecx; rdmsr; mov %%eax,%%ebx; "
>> I don't think it's generally safe to boot an arbitrary OS with FS.base
>> non-zero. Just reload %fs before exiting the function.
> 
> The large asm block at the top of hvmloader.c reloads all segment
> registers with SEL_DATA16 while in 32bit mode, before dropping into 16
> bit mode and zeroing them again.  We are completely safe as long as none
> of the upper 32bits get set while in 64bit mode, which is indeed the case.

Ah, good. And the upper bits don't matter either afaik (loading a
selector register will reload the full base address, not just the low
32 bits).

>>> +        /* CR0.PG=0 */
>>> +        "mov %%cr0,%%eax; btcl $31,%%eax; mov %%eax,%%cr0; "
>>> +        "jmp 1f; 1:"
>>> +        /* EFER.LME=0 */
>>> +        "mov $0xc0000080,%%ecx; rdmsr; btcl $8,%%eax; wrmsr; "
>>> +        /* CR4.PAE=0 */
>>> +        "xor %%eax,%%eax; mov %%eax,%%cr4; "
>>> +        : "=b" (ebx) : "a" (PD_START) : "ecx", "edx", "memory" );
>> %ebx gets modified by other than the last instruction and hence
>> should, even if it doesn't seem to matter right now, be marked as
>> early-clobber. Similarly %eax gets modified and hence should be
>> only and input.
> 
> This will want to be fixed in the gs test as well.  I will fix both.

Yes, I noticed this too the other day, which is why I paid particular
attention here.

Jan