[PATCH/RFC] Let gcc to choose which registers to save

[PATCH/RFC] Let gcc to choose which registers to save

[Laurent Vivier]

As x86_64 ABI defines some registers saved by the calling function, it is not
needed to save all registers in the called function when switching to VCPU.
(see http://www.x86-64.org/documentation/abi.pdf, chapter 3.2.1)

The best way to do that is to inform GCC which registers we use and let
it to save only needed registers.

Signed-off-by: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
---
 drivers/kvm/vmx.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index 2c6b64a..d6c91ac 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -2243,16 +2243,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	asm(
 		/* Store host registers */
 #ifdef CONFIG_X86_64
-		"push %%rax; push %%rbx; push %%rdx;"
-		"push %%rsi; push %%rdi; push %%rbp;"
-		"push %%r8;  push %%r9;  push %%r10; push %%r11;"
-		"push %%r12; push %%r13; push %%r14; push %%r15;"
+		"push %%rdx; push %%rbp;"
 		"push %%rcx \n\t"
-		ASM_VMX_VMWRITE_RSP_RDX "\n\t"
 #else
 		"pusha; push %%ecx \n\t"
-		ASM_VMX_VMWRITE_RSP_RDX "\n\t"
 #endif
+		ASM_VMX_VMWRITE_RSP_RDX "\n\t"
 		/* Check if vmlaunch of vmresume is needed */
 		"cmp $0, %1 \n\t"
 		/* Load guest registers.  Don't clobber flags. */
@@ -2311,12 +2307,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		"mov %%r15, %c[r15](%3) \n\t"
 		"mov %%cr2, %%rax   \n\t"
 		"mov %%rax, %c[cr2](%3) \n\t"
-		"mov (%%rsp), %3 \n\t"
 
-		"pop  %%rcx; pop  %%r15; pop  %%r14; pop  %%r13; pop  %%r12;"
-		"pop  %%r11; pop  %%r10; pop  %%r9;  pop  %%r8;"
-		"pop  %%rbp; pop  %%rdi; pop  %%rsi;"
-		"pop  %%rdx; pop  %%rbx; pop  %%rax \n\t"
+		"pop  %%rcx; pop  %%rbp; pop  %%rdx \n\t"
 #else
 		"xchg %3, (%%esp) \n\t"
 		"mov %%eax, %c[rax](%3) \n\t"
@@ -2354,7 +2346,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		[r15]"i"(offsetof(struct kvm_vcpu, regs[VCPU_REGS_R15])),
 #endif
 		[cr2]"i"(offsetof(struct kvm_vcpu, cr2))
-	      : "cc", "memory");
+	      : "cc", "memory",
+#ifdef CONFIG_X86_64
+		"rbx", "rdi", "rsi",
+		"r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
+#endif
+	      );
 
 	vcpu->interrupt_window_open =
 		(vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0;
-- 
1.5.2.4


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH/RFC] Let gcc to choose which registers to save

[Avi Kivity]

Laurent Vivier wrote:
> As x86_64 ABI defines some registers saved by the calling function, it is not
> needed to save all registers in the called function when switching to VCPU.
> (see http://www.x86-64.org/documentation/abi.pdf, chapter 3.2.1)
>
> The best way to do that is to inform GCC which registers we use and let
> it to save only needed registers.
>
>   

Strange, yesterday I started to do the same thing but dropped it after I 
got discouraged by reload errors from gcc.
> diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
> index 2c6b64a..d6c91ac 100644
> --- a/drivers/kvm/vmx.c
> +++ b/drivers/kvm/vmx.c
> @@ -2243,16 +2243,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	asm(
>  		/* Store host registers */
>  #ifdef CONFIG_X86_64
> -		"push %%rax; push %%rbx; push %%rdx;"
> -		"push %%rsi; push %%rdi; push %%rbp;"
> -		"push %%r8;  push %%r9;  push %%r10; push %%r11;"
> -		"push %%r12; push %%r13; push %%r14; push %%r15;"
> +		"push %%rdx; push %%rbp;"
>  		"push %%rcx \n\t"
> -		ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>  #else
>  		"pusha; push %%ecx \n\t"
> -		ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>  #endif
> +		ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>  		/* Check if vmlaunch of vmresume is needed */
>  		"cmp $0, %1 \n\t"
>  		/* Load guest registers.  Don't clobber flags. */
> @@ -2311,12 +2307,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		"mov %%r15, %c[r15](%3) \n\t"
>  		"mov %%cr2, %%rax   \n\t"
>  		"mov %%rax, %c[cr2](%3) \n\t"
> -		"mov (%%rsp), %3 \n\t"
>  
> -		"pop  %%rcx; pop  %%r15; pop  %%r14; pop  %%r13; pop  %%r12;"
> -		"pop  %%r11; pop  %%r10; pop  %%r9;  pop  %%r8;"
> -		"pop  %%rbp; pop  %%rdi; pop  %%rsi;"
> -		"pop  %%rdx; pop  %%rbx; pop  %%rax \n\t"
> +		"pop  %%rcx; pop  %%rbp; pop  %%rdx \n\t"
>  #else
>  		"xchg %3, (%%esp) \n\t"
>  		"mov %%eax, %c[rax](%3) \n\t"
> @@ -2354,7 +2346,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		[r15]"i"(offsetof(struct kvm_vcpu, regs[VCPU_REGS_R15])),
>  #endif
>  		[cr2]"i"(offsetof(struct kvm_vcpu, cr2))
> -	      : "cc", "memory");
> +	      : "cc", "memory",
> +#ifdef CONFIG_X86_64
> +		"rbx", "rdi", "rsi",
> +		"r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
> +#endif
> +	      );
>  

The comma after "memory" worries me.  Can you compile-test on i386?

Other than that the patch is very welcome -- the excessive register 
saving is very annoying to me.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH/RFC] Let gcc to choose which registers to save

[Laurent Vivier]

Avi Kivity a écrit :
> Laurent Vivier wrote:
>> As x86_64 ABI defines some registers saved by the calling function, it 
>> is not
>> needed to save all registers in the called function when switching to 
>> VCPU.
>> (see http://www.x86-64.org/documentation/abi.pdf, chapter 3.2.1)
>>
>> The best way to do that is to inform GCC which registers we use and let
>> it to save only needed registers.
>>
>>   
> 
> Strange, yesterday I started to do the same thing but dropped it after I 
> got discouraged by reload errors from gcc.

In french, we say "Les beaux esprits se rencontrent" (Voltaire) ;-)
("Great minds think alike")

>> diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
>> index 2c6b64a..d6c91ac 100644
>> --- a/drivers/kvm/vmx.c
>> +++ b/drivers/kvm/vmx.c
>> @@ -2243,16 +2243,12 @@ static void vmx_vcpu_run(struct kvm_vcpu 
>> *vcpu, struct kvm_run *kvm_run)
>>      asm(
>>          /* Store host registers */
>>  #ifdef CONFIG_X86_64
>> -        "push %%rax; push %%rbx; push %%rdx;"
>> -        "push %%rsi; push %%rdi; push %%rbp;"
>> -        "push %%r8;  push %%r9;  push %%r10; push %%r11;"
>> -        "push %%r12; push %%r13; push %%r14; push %%r15;"
>> +        "push %%rdx; push %%rbp;"
>>          "push %%rcx \n\t"
>> -        ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>>  #else
>>          "pusha; push %%ecx \n\t"
>> -        ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>>  #endif
>> +        ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>>          /* Check if vmlaunch of vmresume is needed */
>>          "cmp $0, %1 \n\t"
>>          /* Load guest registers.  Don't clobber flags. */
>> @@ -2311,12 +2307,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, 
>> struct kvm_run *kvm_run)
>>          "mov %%r15, %c[r15](%3) \n\t"
>>          "mov %%cr2, %%rax   \n\t"
>>          "mov %%rax, %c[cr2](%3) \n\t"
>> -        "mov (%%rsp), %3 \n\t"
>>  
>> -        "pop  %%rcx; pop  %%r15; pop  %%r14; pop  %%r13; pop  %%r12;"
>> -        "pop  %%r11; pop  %%r10; pop  %%r9;  pop  %%r8;"
>> -        "pop  %%rbp; pop  %%rdi; pop  %%rsi;"
>> -        "pop  %%rdx; pop  %%rbx; pop  %%rax \n\t"
>> +        "pop  %%rcx; pop  %%rbp; pop  %%rdx \n\t"
>>  #else
>>          "xchg %3, (%%esp) \n\t"
>>          "mov %%eax, %c[rax](%3) \n\t"
>> @@ -2354,7 +2346,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu, 
>> struct kvm_run *kvm_run)
>>          [r15]"i"(offsetof(struct kvm_vcpu, regs[VCPU_REGS_R15])),
>>  #endif
>>          [cr2]"i"(offsetof(struct kvm_vcpu, cr2))
>> -          : "cc", "memory");
>> +          : "cc", "memory",
>> +#ifdef CONFIG_X86_64
>> +        "rbx", "rdi", "rsi",
>> +        "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
>> +#endif
>> +          );
>>  
> 
> The comma after "memory" worries me.  Can you compile-test on i386?

You're right, I thought I've corrected this. I rework this and test on i386.

> Other than that the patch is very welcome -- the excessive register 
> saving is very annoying to me.

I think we can do the same thing with svm.c, but I can't test it.

Regards,
Laurent
-- 
---------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  -----------------
"Given enough eyeballs, all bugs are shallow" E. S. Raymond


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH/RFC] Let gcc to choose which registers to save

[Avi Kivity]

Laurent Vivier wrote:
>   
>> Other than that the patch is very welcome -- the excessive register 
>> saving is very annoying to me.
>>     
>
> I think we can do the same thing with svm.c, but I can't test it.
>
>   

I can test it for you (but a separate patch please -- these are likely 
to cause trouble with different compilers and options, so I want them to 
be easy to revert).


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH/RFC] Let gcc to choose which registers to save

[Laurent Vivier]

Avi Kivity a écrit :
> Laurent Vivier wrote:
>>  
>>> Other than that the patch is very welcome -- the excessive register 
>>> saving is very annoying to me.
>>>     
>>
>> I think we can do the same thing with svm.c, but I can't test it.
>>
>>   
> 
> I can test it for you (but a separate patch please -- these are likely 
> to cause trouble with different compilers and options, so I want them to 
> be easy to revert).
> 
> 

OK, thank you

Laurent
-- 
---------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  -----------------
"Given enough eyeballs, all bugs are shallow" E. S. Raymond


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH/RFC] Let gcc to choose which registers to save

[Alexander Graf]

On Oct 22, 2007, at 5:24 PM, Laurent Vivier wrote:

> Avi Kivity a écrit :
>> Laurent Vivier wrote:
>>> As x86_64 ABI defines some registers saved by the calling  
>>> function, it
>>> is not
>>> needed to save all registers in the called function when  
>>> switching to
>>> VCPU.
>>> (see http://www.x86-64.org/documentation/abi.pdf, chapter 3.2.1)
>>>
>>> The best way to do that is to inform GCC which registers we use  
>>> and let
>>> it to save only needed registers.
>>>
>>>
>>
>> Strange, yesterday I started to do the same thing but dropped it  
>> after I
>> got discouraged by reload errors from gcc.
>
> In french, we say "Les beaux esprits se rencontrent" (Voltaire) ;-)
> ("Great minds think alike")
>
>>> diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
>>> index 2c6b64a..d6c91ac 100644
>>> --- a/drivers/kvm/vmx.c
>>> +++ b/drivers/kvm/vmx.c
>>> @@ -2243,16 +2243,12 @@ static void vmx_vcpu_run(struct kvm_vcpu
>>> *vcpu, struct kvm_run *kvm_run)
>>>      asm(
>>>          /* Store host registers */
>>>  #ifdef CONFIG_X86_64
>>> -        "push %%rax; push %%rbx; push %%rdx;"
>>> -        "push %%rsi; push %%rdi; push %%rbp;"
>>> -        "push %%r8;  push %%r9;  push %%r10; push %%r11;"
>>> -        "push %%r12; push %%r13; push %%r14; push %%r15;"
>>> +        "push %%rdx; push %%rbp;"
>>>          "push %%rcx \n\t"
>>> -        ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>>>  #else
>>>          "pusha; push %%ecx \n\t"
>>> -        ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>>>  #endif
>>> +        ASM_VMX_VMWRITE_RSP_RDX "\n\t"
>>>          /* Check if vmlaunch of vmresume is needed */
>>>          "cmp $0, %1 \n\t"
>>>          /* Load guest registers.  Don't clobber flags. */
>>> @@ -2311,12 +2307,8 @@ static void vmx_vcpu_run(struct kvm_vcpu  
>>> *vcpu,
>>> struct kvm_run *kvm_run)
>>>          "mov %%r15, %c[r15](%3) \n\t"
>>>          "mov %%cr2, %%rax   \n\t"
>>>          "mov %%rax, %c[cr2](%3) \n\t"
>>> -        "mov (%%rsp), %3 \n\t"
>>>
>>> -        "pop  %%rcx; pop  %%r15; pop  %%r14; pop  %%r13; pop  %% 
>>> r12;"
>>> -        "pop  %%r11; pop  %%r10; pop  %%r9;  pop  %%r8;"
>>> -        "pop  %%rbp; pop  %%rdi; pop  %%rsi;"
>>> -        "pop  %%rdx; pop  %%rbx; pop  %%rax \n\t"
>>> +        "pop  %%rcx; pop  %%rbp; pop  %%rdx \n\t"
>>>  #else
>>>          "xchg %3, (%%esp) \n\t"
>>>          "mov %%eax, %c[rax](%3) \n\t"
>>> @@ -2354,7 +2346,12 @@ static void vmx_vcpu_run(struct kvm_vcpu  
>>> *vcpu,
>>> struct kvm_run *kvm_run)
>>>          [r15]"i"(offsetof(struct kvm_vcpu, regs[VCPU_REGS_R15])),
>>>  #endif
>>>          [cr2]"i"(offsetof(struct kvm_vcpu, cr2))
>>> -          : "cc", "memory");
>>> +          : "cc", "memory",
>>> +#ifdef CONFIG_X86_64
>>> +        "rbx", "rdi", "rsi",
>>> +        "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
>>> +#endif
>>> +          );
>>>
>>
>> The comma after "memory" worries me.  Can you compile-test on i386?
>
> You're right, I thought I've corrected this. I rework this and test  
> on i386.
>
>> Other than that the patch is very welcome -- the excessive register
>> saving is very annoying to me.
>
> I think we can do the same thing with svm.c, but I can't test it.
>

Actually you can. Recently I implemented SVM emulation capabilities  
in qemu, so when you use the most current qemu cvs x86_64 emulator,  
there should be no problem running kvm inside of that. Please mind  
that it's neither bug-free nor feature-complete but it ran kvm-36  
without any problems.

Cheers,

Alexander Graf
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/