Re: [Xen-devel] [PATCH RFC v2 09/12] x86: enhance syscall stub to work in per-domain mapping

[Juergen Gross <jgross@xxxxxxxx>]

On 30/01/18 16:11, Jan Beulich wrote:
>>>> On 22.01.18 at 13:32, <jgross@xxxxxxxx> wrote:
>> --- a/xen/arch/x86/x86_64/traps.c
>> +++ b/xen/arch/x86/x86_64/traps.c
>> @@ -260,10 +260,11 @@ void do_double_fault(struct cpu_user_regs *regs)
>>      panic("DOUBLE FAULT -- system shutdown");
>>  }
>>  
>> -static unsigned int write_stub_trampoline(
>> -    unsigned char *stub, unsigned long stub_va,
>> -    unsigned long stack_bottom, unsigned long target_va)
>> +void write_stub_trampoline(unsigned char *stub, unsigned long stub_va,
>> +                           unsigned long stack_bottom, unsigned long 
>> target_va)
> 
> Why does the static go away?

I'll need it in patch 10.

> 
>> @@ -282,24 +283,32 @@ static unsigned int write_stub_trampoline(
>>      /* pushq %rax */
>>      stub[23] = 0x50;
>>  
>> -    /* jmp target_va */
>> -    stub[24] = 0xe9;
>> -    *(int32_t *)&stub[25] = target_va - (stub_va + 29);
>> -
>> -    /* Round up to a multiple of 16 bytes. */
>> -    return 32;
>> +    target_diff = target_va - (stub_va + 29);
>> +    if ( target_diff >> 31 == target_diff >> 63 )
>> +    {
>> +        /* jmp target_va */
>> +        stub[24] = 0xe9;
>> +        *(int32_t *)&stub[25] = target_diff;
>> +    }
>> +    else
>> +    {
>> +        /* movabs target_va, %rax */
>> +        stub[24] = 0x48;
>> +        stub[25] = 0xb8;
>> +        *(uint64_t *)&stub[26] = target_va;
>> +        /* jmpq *%rax */
>> +        stub[34] = 0xff;
>> +        stub[35] = 0xe0;
>> +    }
> 
> This clearly needs another solution, as you'd have to go through a
> thunk now, and the thunk would be unreachable too.

Aah, right. So maybe it would be better not to share the code for
writing the stub page with XPTI.

I'll replace this patch with one adding a new function for XPTI.


Juergen

> 
>>  }
>>  
>>  DEFINE_PER_CPU(struct stubs, stubs);
>> -void lstar_enter(void);
>> -void cstar_enter(void);
> 
> Why do these move into a header?
> 
>> @@ -312,10 +321,9 @@ void subarch_percpu_traps_init(void)
>>       * start of the stubs.
>>       */
>>      wrmsrl(MSR_LSTAR, stub_va);
>> -    offset = write_stub_trampoline(stub_page + (stub_va & ~PAGE_MASK),
>> -                                   stub_va, stack_bottom,
>> -                                   (unsigned long)lstar_enter);
>> -    stub_va += offset;
>> +    write_stub_trampoline(stub_page + (stub_va & ~PAGE_MASK), stub_va,
>> +                          stack_bottom, (unsigned long)lstar_enter);
>> +    stub_va += STUB_TRAMPOLINE_SIZE_PERCPU;
> 
> The function may have written more than 32 bytes now; you'd
> notice the breakage if you put a suitable BUILD_BUG_ON() into
> the function. Otherwise I recommend you stick to the current
> "return number of bytes written" model.
> 
>> @@ -328,12 +336,11 @@ void subarch_percpu_traps_init(void)
>>  
>>      /* Trampoline for SYSCALL entry from compatibility mode. */
>>      wrmsrl(MSR_CSTAR, stub_va);
>> -    offset += write_stub_trampoline(stub_page + (stub_va & ~PAGE_MASK),
>> -                                    stub_va, stack_bottom,
>> -                                    (unsigned long)cstar_enter);
>> +    write_stub_trampoline(stub_page + (stub_va & ~PAGE_MASK), stub_va,
>> +                          stack_bottom, (unsigned long)cstar_enter);
>>  
>>      /* Don't consume more than half of the stub space here. */
>> -    ASSERT(offset <= STUB_BUF_SIZE / 2);
>> +    ASSERT(2 * STUB_TRAMPOLINE_SIZE_PERCPU <= STUB_BUF_SIZE / 2);
> 
> BUILD_BUG_ON() for compile time constants.
> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel