On 08/03/18 15:24, Jan Beulich wrote: >>>> On 02.03.18 at 09:14, <jgr...@suse.com> wrote: >> This reduces the number of branches in interrupt handling and results >> in better performance (e.g. parallel make of the Xen hypervisor on my >> system was using about 3% less system time). > > 3% seems an awful lot for a single conditional branch on each of the > three affected entry paths.
I measured it multiple times because I couldn't believe it. >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -1698,6 +1698,7 @@ void context_switch(struct vcpu *prev, struct vcpu >> *next) >> ASSERT(local_irq_is_enabled()); >> >> get_cpu_info()->xen_cr3 = 0; >> + get_cpu_info()->use_xen_cr3 = false; > > Don't you need this to be the other way around _and_ a barrier() in > between? As the context above shows, interrupts are enabled here > (and NMI/#MC can occur at any time anyway), so with the order > above it seems to me as if restore_all_xen might write zero into CR3. > While the ordering appears to be right elsewhere, the barrier() part > may apply to changes further down as well. Yes, you are right. Thanks for catching this bug. > >> @@ -523,18 +516,17 @@ ENTRY(common_interrupt) >> >> .Lintr_cr3_start: >> mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx >> + mov STACK_CPUINFO_FIELD(use_xen_cr3)(%r14), %bl >> mov %rcx, %r15 >> - neg %rcx >> + test %rcx, %rcx >> jz .Lintr_cr3_okay >> - jns .Lintr_cr3_load >> - mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14) >> - neg %rcx >> -.Lintr_cr3_load: >> + movb $0, STACK_CPUINFO_FIELD(use_xen_cr3)(%r14) >> mov %rcx, %cr3 >> xor %ecx, %ecx >> mov %rcx, STACK_CPUINFO_FIELD(xen_cr3)(%r14) >> testb $3, UREGS_cs(%rsp) >> cmovnz %rcx, %r15 >> + cmovnz %cx, %bx > > 32-bit operation please. Okay. > >> @@ -831,6 +820,7 @@ handle_ist_exception: >> * and copy the context to stack bottom. >> */ >> xor %r15, %r15 >> + xor %bl, %bl > > Same here. Okay. > >> @@ -68,6 +65,12 @@ struct cpu_info { >> */ >> bool root_pgt_changed; >> >> + /* >> + * use_xen_cr3 is set in case the value of xen_cr3 is to be written into >> + * CR3 when entering the hypervisor. >> + */ >> + bool use_xen_cr3; > > When entering the hypervisor? Afaics the flag is evaluated only to > trigger the unlikely code in restore_all_xen, which is an exit path (as > the comment portion you remove from xen_cr3 also says). Sorry, you are right, of course. Will change the comment. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel