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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

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

> @@ -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?

> --- 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?

Jan


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