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

[Juergen Gross <jgross@xxxxxxxx>]

On 08/03/18 15:24, Jan Beulich wrote:
>>>> 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.

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel