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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.07.14 at 17:34, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/07/14 16:22, Jan Beulich wrote:
>>>>> 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.
> 
> 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

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