Re: [Xen-devel] [PATCH v2 5/6] xen/x86: use flag byte for decision whether xen_cr3 is valid

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.03.18 at 09:14, <jgross@xxxxxxxx> 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.

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

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

> @@ -831,6 +820,7 @@ handle_ist_exception:
>           * and copy the context to stack bottom.
>           */
>          xor   %r15, %r15
> +        xor   %bl, %bl

Same here.

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

Jan


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