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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 09/04/2013 10:50 AM, Jan Beulich wrote:
> > > > On 04.09.13 at 16:39, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 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

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