Re: [Xen-devel] [PATCH 4/4] SVM: streamline entry.S code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.13 at 18:42, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 09/04/2013 11:20 AM, Jan Beulich wrote:
>>>>> On 04.09.13 at 17:09, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> I meant something like this:
>>>
>>> diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
>>> index 1969629..b362637 100644
>>> --- a/xen/arch/x86/hvm/svm/entry.S
>>> +++ b/xen/arch/x86/hvm/svm/entry.S
>>> @@ -92,10 +92,12 @@ UNLIKELY_END(svm_trace)
>>>
>>>            VMRUN
>>>
>>> +       GET_CURRENT(%rax)
>>>            push %rdi
>>>            push %rsi
>>>            push %rdx
>>>            push %rcx
>>> +       mov  VCPU_svm_vmcb(%rax),%rcx
>>>            push %rax
>>>            push %r8
>>>            push %r9
>>> @@ -108,17 +110,15 @@ UNLIKELY_END(svm_trace)
>>>            push %r14
>>>            push %r15
>>>
>>> -        GET_CURRENT(%rbx)
>>> -        movb $0,VCPU_svm_vmcb_in_sync(%rbx)
>>> -        mov  VCPU_svm_vmcb(%rbx),%rcx
>>> -        mov  VMCB_rax(%rcx),%rax
>>> -        mov  %rax,UREGS_rax(%rsp)
>>> -        mov  VMCB_rip(%rcx),%rax
>>> -        mov  %rax,UREGS_rip(%rsp)
>>> -        mov  VMCB_rsp(%rcx),%rax
>>> -        mov  %rax,UREGS_rsp(%rsp)
>>> -        mov  VMCB_rflags(%rcx),%rax
>>> -        mov  %rax,UREGS_eflags(%rsp)
>>> +        movb $0,VCPU_svm_vmcb_in_sync(%rax)
>>> +        mov  VMCB_rax(%rcx),%rdi
>>> +        mov  %rdi,UREGS_rax(%rsp)
>>> +        mov  VMCB_rip(%rcx),%rdi
>>> +        mov  %rdi,UREGS_rip(%rsp)
>>> +        mov  VMCB_rsp(%rcx),%rdi
>>> +        mov  %rdi,UREGS_rsp(%rsp)
>>> +        mov  VMCB_rflags(%rcx),%rdi
>>> +        mov  %rdi,UREGS_eflags(%rsp)
>>>
>>>    #ifndef NDEBUG
>>>            mov  $0xbeef,%ax
>>> ostr@workbase>
>>>
>>>
>>>
>>> %rax is clobbered anyway by ' mov  VMCB_rax(%rcx),%rax'
>> But the "current" pointer also isn't needed anymore after the clearing
>> of VCPU_svm_vmcb_in_sync and before doing the first function call
>> (it's needed after the function call, but for that it's can't be in %rax).
>>
>> So yes, that clearing of VCPU_svm_vmcb_in_sync could certainly be
>> done using %rax instead of %rbx, but no other code change would
>> seem necessary/desirable. If you agree, I'd be willing to do this one
>> line adjustment.
> 
> Oh, yes, that's right, no reason to change other lines.
> 
> My only reason for suggesting this change though was to eliminate
> 'mov  %rax,%rbx' instruction (in your patch, not in the code above).
> If you want to keep it in though then I don't think the line adjustment
> that you are talking about is needed.

Once again - this is not a question of "want", but a question of
"need": After calling svm_vmexit_handler execution continue on
to .Lsvm_do_resume, specifically skipping the (more expensive)
(re-)load of %rbx done immediately before that label (for only
svm_asm_do_resume's sake).

The extra one line adjustment would be to further hide eventual
latency between loading %rbx and using it in an address
calculation (I left it untouched originally because the latency
from a reg-reg move ought to be much smaller than that from
the original GET_CURRENT(%rbx) involving a memory read).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel