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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 09/04/2013 10:50 AM, Jan Beulich wrote:
>>>> On 04.09.13 at 16:39, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> On 08/23/2013 10:04 AM, Jan Beulich wrote:
>>> @@ -92,25 +97,26 @@ 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
>>>            push %r10
>>>            push %r11
>>>            push %rbx
>>> +        mov  %rax,%rbx
>> Can you continue with using %rax as 'current' pointer below and not save it
>> to %rbx? %rax appears to be a temp register so perhaps you can use, say,
>> %rdi, for that purpose.
> I'm sorry, but I don't understand what you're asking for.
>
> We actually _want_ "current" to be in a register thats callee-saved,
> such that we don't need to reload it after function calls. One goal of
> the patch in fact is to eliminate such redundant reloads. But as I'm
> not sure what you want, this explanation may be entirely off.


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'


>
> And as a side note: At this point I'm not really expecting requests
> for further changes, unless you spot a mistake in the patch. It was
> around for review for long enough, and as said it had two reviews
> already. And further optimization should go in a separate,
> incremental patch.
>


Right, this was more of an observation, not a mistake in your patch. 
Feel free
to ignore it (assuming it's even correct).


-boris