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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.07.14 at 16:30, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> 
> ---
> 
> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel