[PATCH v2 0/3] HVMLoader test improvements

[PATCH v2 0/3] HVMLoader test improvements

[Andrew Cooper]

These patches are the remainer of the ones from a previous series where the
main bugfix has already been committed.

Patch is improvements to the shadow_gs_test() in HVMLoader.  Patch 2 is a
prerequisite for Patch 3, which was the test I developed to spot the
regression (fixed by c/s a1ac4cf52) between Xen 4.1 and 4.4

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>

[PATCH v2 1/3] hvmloader/tests: Improvements to shadow_gs_test()

[Andrew Cooper]

Use .code64 and .code32, and real mnemonics instead of bare opcodes.  No
functional change to the test.

Correct register constraints.  PD_START doesn't necessarily need to be in eax,
but eax does need to identified as clobbered.

As GAS is unable to express a register as an input and also clobbered, opt for
an explicit eax clobber and allow some choice for the register used to pass
PD_START (and indeed, objdump indicates that esi is chosen).  ebx gains an
early clobber to remove it from the candidate list for PD_START.

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>

---
v2: Include register constraint fixes as suggested by Jan
---
 tools/firmware/hvmloader/tests.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c
index 52772aa..931ed2f 100644
--- a/tools/firmware/hvmloader/tests.c
+++ b/tools/firmware/hvmloader/tests.c
@@ -135,7 +135,7 @@ static int rep_io_test(void)
 static int shadow_gs_test(void)
 {
     uint64_t *pd = (uint64_t *)PD_START;
-    uint32_t i, eax, ebx, ecx, edx;
+    uint32_t i, eax, ebx, ecx, edx, dummy;
 
     /* Skip this test if the CPU does not support long mode. */
     cpuid(0x80000000, &eax, &ebx, &ecx, &edx);
@@ -158,7 +158,7 @@ static int shadow_gs_test(void)
         "mov $0x20,%%ebx; "
         "mov %%ebx,%%cr4; "
         /* CR3 */
-        "mov %%eax,%%cr3; "
+        "mov %0,%%cr3; "
         /* EFER.LME=1 */
         "mov $0xc0000080,%%ecx; rdmsr; btsl $8,%%eax; wrmsr; "
         /* CR0.PG=1 */
@@ -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 */
@@ -185,7 +187,9 @@ static int shadow_gs_test(void)
         "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" );
+        : "=r" (dummy), "=&b" (ebx)
+        : "0" (PD_START)
+        : "eax", "ecx", "edx", "memory" );
 
     return (ebx == 2) ? TEST_PASS : TEST_FAIL;
 }
-- 
1.7.10.4

Re: [PATCH v2 1/3] hvmloader/tests: Improvements to shadow_gs_test()

[Jan Beulich]

>>> On 29.07.14 at 19:49, <andrew.cooper3@citrix.com> wrote:
> @@ -158,7 +158,7 @@ static int shadow_gs_test(void)
>          "mov $0x20,%%ebx; "
>          "mov %%ebx,%%cr4; "
>          /* CR3 */
> -        "mov %%eax,%%cr3; "
> +        "mov %0,%%cr3; "
>          /* EFER.LME=1 */
>          "mov $0xc0000080,%%ecx; rdmsr; btsl $8,%%eax; wrmsr; "
>          /* CR0.PG=1 */
> @@ -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 */
> @@ -185,7 +187,9 @@ static int shadow_gs_test(void)
>          "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" );
> +        : "=r" (dummy), "=&b" (ebx)
> +        : "0" (PD_START)
> +        : "eax", "ecx", "edx", "memory" );

This is all rather inconsistent now: The clobbered registers can't be
picked for operand 0 now. I think "=&a" (eax) would be better than
the dummy you use, even more so since the way you changed it
this register doesn't really get modified (i.e. you wouldn't need an
output). Or alternatively, when you relax the PD_START operand
you could also relax the one currently in %ebx - there's nothing
requiring it to be in that specific register afaict, and then specify its
initializer also in the asm() operands rather than its first instruction.
Same thing as for %eax would go for %ecx perhaps; only %edx is
truly just being clobbered.

Jan

[PATCH v2 2/3] 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 v2 3/3] hvmloader/tests: Introduce WRFSBASE test

[Andrew Cooper]

Without the bugfix in c/s a1ac4cf52 "x86/cpu: undo BIOS CPUID max_leaf limit
before querying for features", 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>

---
v2: Drop spurious whitespace change.  Rebase on top of changes to shadow_gs_test()
---
 tools/firmware/hvmloader/tests.c |   67 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c
index 931ed2f..0e70032 100644
--- a/tools/firmware/hvmloader/tests.c
+++ b/tools/firmware/hvmloader/tests.c
@@ -194,6 +194,72 @@ 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, dummy;
+
+    /* 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 %0,%%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; "
+        : "=r" (dummy), "=&b" (ebx)
+        : "0" (PD_START)
+        : "eax", "ecx", "edx", "memory" );
+
+    return (ebx == 0x234) ? TEST_PASS : TEST_FAIL;
+}
+
 void perform_tests(void)
 {
     int i, passed, skipped;
@@ -204,6 +270,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 }
     };
 
-- 
1.7.10.4