>>> On 22.01.18 at 13:32, <jgr...@suse.com> 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.
> --- 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?
> call do_early_page_fault
> + movq %rsp, %rdi
> jmp restore_all_xen
Doesn't this belong in an earlier patch?
> --- 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.
> --- 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.
> +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.
> @@ -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.
> -#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.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
