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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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