Re: [Xen-devel] [PATCH RFC v2 11/12] x86: modify interrupt handlers to support stack switching

[Juergen Gross <jgross@xxxxxxxx>]

On 30/01/18 17:07, Jan Beulich wrote:
>>>> On 22.01.18 at 13:32, <jgross@xxxxxxxx> wrote:
>> --- a/xen/arch/x86/x86_64/asm-offsets.c
>> +++ b/xen/arch/x86/x86_64/asm-offsets.c
>> @@ -137,6 +137,10 @@ void __dummy__(void)
>>      OFFSET(CPUINFO_processor_id, struct cpu_info, processor_id);
>>      OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
>>      OFFSET(CPUINFO_cr4, struct cpu_info, cr4);
>> +    OFFSET(CPUINFO_stack_bottom_cpu, struct cpu_info, stack_bottom_cpu);
>> +    OFFSET(CPUINFO_flags, struct cpu_info, flags);
>> +    DEFINE(ASM_ON_VCPUSTACK, ON_VCPUSTACK);
>> +    DEFINE(ASM_VCPUSTACK_ACTIVE, VCPUSTACK_ACTIVE);
> 
> Seeing their uses in asm_defns.h it's not really clear to me why
> you can't use the C constants there, the more that those uses
> are inside C macros (which perhaps would better be assembler
> ones). The latter doesn't even appear to be used in assembly
> code.

I tried using the C constants but this led to rather nasty include
dependencies.

ASM_VCPUSTACK_ACTIVE will be used when %cr3 switching is being added.

> 
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -19,6 +19,7 @@ ENTRY(entry_int82)
>>          movl  $HYPERCALL_VECTOR, 4(%rsp)
>>          SAVE_ALL compat=1 /* DPL1 gate, restricted to 32bit PV guests only. 
>> */
>>          mov   %rsp, %rdi
>> +        SWITCH_FROM_VCPU_STACK
>>          CR4_PV32_RESTORE
> 
> Once again - why for compat mode guests?
> 
>> @@ -615,7 +623,9 @@ ENTRY(early_page_fault)
>>          movl  $TRAP_page_fault,4(%rsp)
>>          SAVE_ALL
>>          movq  %rsp,%rdi
>> +        SWITCH_FROM_VCPU_STACK
> 
> Why, in this context?

Same as before: consistency. I can remove this.

> 
>>          call  do_early_page_fault
>> +        movq  %rsp, %rdi
>>          jmp   restore_all_xen
> 
> Doesn't this belong in an earlier patch?

I have cleaned this up already.

> 
>> --- a/xen/common/wait.c
>> +++ b/xen/common/wait.c
>> @@ -122,10 +122,10 @@ void wake_up_all(struct waitqueue_head *wq)
>>  
>>  static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>>  {
>> -    struct cpu_info *cpu_info = get_cpu_info();
>> +    struct cpu_user_regs *user_regs = guest_cpu_user_regs();
>>      struct vcpu *curr = current;
>>      unsigned long dummy;
>> -    u32 entry_vector = cpu_info->guest_cpu_user_regs.entry_vector;
>> +    u32 entry_vector = user_regs->entry_vector;
>>  
>>      ASSERT(wqv->esp == 0);
>>  
>> @@ -160,7 +160,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>>          "pop %%r11; pop %%r10; pop %%r9;  pop %%r8;"
>>          "pop %%rbp; pop %%rdx; pop %%rbx; pop %%rax"
>>          : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
>> -        : "i" (PAGE_SIZE), "0" (0), "1" (cpu_info), "2" (wqv->stack)
>> +        : "i" (PAGE_SIZE), "0" (0), "1" (user_regs), "2" (wqv->stack)
>>          : "memory" );
>>  
>>      if ( unlikely(wqv->esp == 0) )
>> @@ -169,7 +169,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>>          domain_crash_synchronous();
>>      }
>>  
>> -    cpu_info->guest_cpu_user_regs.entry_vector = entry_vector;
>> +    user_regs->entry_vector = entry_vector;
>>  }
> 
> I don't see how this change is related to the purpose of this patch,
> or why the change is needed. All you do is utilize that
> guest_cpu_user_regs is the first field of struct cpu_info afaics.

guest_cpu_user_regs() might point to either stack, while get_cpu_info()
will always reference the Xen stack and never the per-vcpu one.

> 
>> --- a/xen/include/asm-x86/asm_defns.h
>> +++ b/xen/include/asm-x86/asm_defns.h
>> @@ -116,6 +116,25 @@ void ret_from_intr(void);
>>          GET_STACK_END(reg);                       \
>>          __GET_CURRENT(reg)
>>  
>> +#define SWITCH_FROM_VCPU_STACK                                           \
>> +        GET_STACK_END(ax);                                               \
>> +        testb $ASM_ON_VCPUSTACK, STACK_CPUINFO_FIELD(flags)(%rax);       \
>> +        jz    1f;                                                        \
>> +        movq  STACK_CPUINFO_FIELD(stack_bottom_cpu)(%rax), %rsp;         \
>> +1:
>> +
>> +#define SWITCH_FROM_VCPU_STACK_IST                                       \
>> +        GET_STACK_END(ax);                                               \
>> +        testb $ASM_ON_VCPUSTACK, STACK_CPUINFO_FIELD(flags)(%rax);       \
>> +        jz    1f;                                                        \
>> +        subq  $(CPUINFO_sizeof - 1), %rax;                               \
>> +        addq  CPUINFO_stack_bottom_cpu(%rax), %rsp;                      \
>> +        subq  %rax, %rsp;                                                \
> 
> If I'm not mistaken, %rsp is complete rubbish for on instruction
> here. While quite likely not a problem in practice, it would still
> feel better if you went through an intermediate register. I also
> think the calculation might then end up easier to follow. It'll also
> make analysis of a crash easier if an NMI or #MC hits exactly at
> this boundary.

Okay. Will change.

> 
>> +1:
>> +
>> +#define SWITCH_TO_VCPU_STACK                                             \
>> +        movq  %rdi, %rsp
> 
> For these additions as a whole: At least in new pieces of code
> please avoid insn suffixes when they're redundant with registers
> used.

Okay.

> 
>> @@ -94,9 +95,16 @@ static inline struct cpu_info *get_cpu_info(void)
>>  #define set_processor_id(id)  do {                                      \
>>      struct cpu_info *ci__ = get_cpu_info();                             \
>>      ci__->per_cpu_offset = __per_cpu_offset[ci__->processor_id = (id)]; \
>> +    ci__->flags = 0;                                                    \
>>  } while (0)
> 
> Not here, no. Considering other similar changes by recent patches
> I can see the need for a helper doing that, but this shouldn't be
> hidden in a completely unrelated macro.

Okay.

> 
>> -#define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
>> +#define guest_cpu_user_regs() ({                                        \
>> +    struct cpu_info *info = get_cpu_info();                             \
> 
> Please use a more macro-suitable name, e.g. ci__ as above.

Okay.


Juergen


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