Re: [Xen-devel] [PATCH RFC v2 05/12] x86: don't access saved user regs via rsp in trap handlers

[Juergen Gross <jgross@xxxxxxxx>]

On 30/01/18 15:49, Jan Beulich wrote:
>>>> On 22.01.18 at 13:32, <jgross@xxxxxxxx> wrote:
>> In order to support switching stacks when entering the hypervisor for
>> support of page table isolation, don't use %rsp for accessing the
>> saved user registers, but do that via %rdi.
> 
> If this really turns out to be necessary ...
> 
>> @@ -58,20 +58,24 @@ compat_test_guest_events:
>>          jmp   compat_test_all_events
>>  
>>          ALIGN
>> -/* %rbx: struct vcpu */
>> +/* %rbx: struct vcpu, %rdi: user_regs */
>>  compat_process_softirqs:
>>          sti
>> +        pushq %rdi
>>          call  do_softirq
>> +        popq  %rdi
>>          jmp   compat_test_all_events
> 
> ... to avoid changes like this one (which unduly affect stack
> alignment) you will want to consider using e.g. %r12 instead.

Right. I have this already on my agenda for the next version of the
patches.

> But concerning specifically the compat entry code, it's unclear to
> me why you'd need to switch stacks there too.

That was just for consistency. I can drop that if you prefer.

> 
>> @@ -211,13 +218,15 @@ ENTRY(cstar_enter)
>>          testl $~3,%esi
>>          leal  (,%rcx,TBF_INTERRUPT),%ecx
>>  UNLIKELY_START(z, compat_syscall_gpf)
>> -        movq  VCPU_trap_ctxt(%rbx),%rdi
>> -        movl  $TRAP_gp_fault,UREGS_entry_vector(%rsp)
>> -        subl  $2,UREGS_rip(%rsp)
>> +        pushq %rcx
>> +        movq  VCPU_trap_ctxt(%rbx),%rcx
>> +        movl  $TRAP_gp_fault,UREGS_entry_vector(%rdi)
>> +        subl  $2,UREGS_rip(%rdi)
>>          movl  $0,TRAPBOUNCE_error_code(%rdx)
>> -        movl  TRAP_gp_fault * TRAPINFO_sizeof + TRAPINFO_eip(%rdi),%eax
>> -        movzwl TRAP_gp_fault * TRAPINFO_sizeof + TRAPINFO_cs(%rdi),%esi
>> -        testb $4,TRAP_gp_fault * TRAPINFO_sizeof + TRAPINFO_flags(%rdi)
>> +        movl  TRAP_gp_fault * TRAPINFO_sizeof + TRAPINFO_eip(%rcx),%eax
>> +        movzwl TRAP_gp_fault * TRAPINFO_sizeof + TRAPINFO_cs(%rcx),%esi
>> +        testb $4,TRAP_gp_fault * TRAPINFO_sizeof + TRAPINFO_flags(%rcx)
>> +        popq  %rcx
> 
> Is there really no register available, requiring you to push/pop
> %rcx here?

With switching from %rdi to e.g. %r12 this is no longer an issue.

> 
>> --- a/xen/include/asm-x86/current.h
>> +++ b/xen/include/asm-x86/current.h
>> @@ -95,9 +95,13 @@ unsigned long get_stack_dump_bottom (unsigned long sp);
>>      ({                                                                  \
>>          __asm__ __volatile__ (                                          \
>>              "mov %0,%%"__OP"sp;"                                        \
>> -            CHECK_FOR_LIVEPATCH_WORK                                      \
>> -             "jmp %c1"                                                  \
>> -            : : "r" (guest_cpu_user_regs()), "i" (__fn) : "memory" );   \
>> +            "mov %1,%%"__OP"di;"                                        \
>> +            "pushq %%"__OP"di;"                                         \
>> +            CHECK_FOR_LIVEPATCH_WORK                                    \
>> +            "popq %%"__OP"di;"                                          \
>> +            "jmp %c2"                                                   \
>> +            : : "r" (get_cpu_info()), "r" (guest_cpu_user_regs()),      \
>> +                "i" (__fn) : "memory" );                                \
>>          unreachable();                                                  \
>>      })
> 
> If you want guest_cpu_user_regs() in %rdi, why don't you use
> "D" as constraint? Why do you need to restore %rdi prior to the
> final JMP? And why do you need the value in %rdi before calling
> check_for_livepatch_work(), when the function takes no arguments?

Will change.


Juergen


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