[PATCH][SVM] Only save/restore MSRs when needed

[PATCH][SVM] Only save/restore MSRs when needed

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 325 bytes --]

I've tested this with 32-bit and 64-bit guests on a 64-bit host and with 
32-bit guests on a 32-bit host.

I *think* it's doing the right thing wrt to DEBUGCTL but an extra set of 
eyes would be helpful.

Depending on my machine's mood, we're at somewhere between 3200 and 2700 
cycles per VMEXIT.

Regards,

Anthony Liguori

[-- Attachment #2: svm-lazy-msrs.diff --]
[-- Type: text/x-patch, Size: 3881 bytes --]

From: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [PATCH][SVM] Only save/restore MSRs when needed

  We only have to save/restore MSR_GS_BASE on every VMEXIT.  The rest can be
  saved/restored when we leave the VCPU.  As a special case, MSR_IA32_DEBUGCTL
  only needs to be saved/restored on every exit if debugging is enabled in
  either the host or the guest.

Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Index: kernel/drivers/kvm/svm.c
===================================================================
--- kernel.orig/drivers/kvm/svm.c	2007-04-27 17:52:37.200780944 -0500
+++ kernel/drivers/kvm/svm.c	2007-04-27 18:14:02.098446808 -0500
@@ -611,7 +611,7 @@
 
 static void svm_vcpu_load(struct kvm_vcpu *vcpu)
 {
-	int cpu;
+	int cpu, i;
 
 	cpu = get_cpu();
 	if (unlikely(cpu != vcpu->cpu)) {
@@ -626,10 +626,24 @@
 		vcpu->svm->vmcb->control.tsc_offset += delta;
 		vcpu->cpu = cpu;
 	}
+
+	for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
+		rdmsrl(host_save_user_msrs[i], vcpu->svm->host_user_msrs[i]);
+
+	rdmsrl(MSR_IA32_DEBUGCTLMSR, vcpu->svm->host_ia32_debugctl);
 }
 
 static void svm_vcpu_put(struct kvm_vcpu *vcpu)
 {
+	int i;
+
+	for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
+		wrmsrl(host_save_user_msrs[i], vcpu->svm->host_user_msrs[i]);
+
+	wrmsrl(MSR_IA32_DEBUGCTLMSR, vcpu->svm->host_ia32_debugctl);
+	/* FIXME: LASTBRANCHFROMIP, LASTBRANCHTOIP, LASTINTFROMIP,
+	   LASTINTTOIP */
+
 	rdtscll(vcpu->host_tsc);
 	put_cpu();
 }
@@ -815,18 +829,16 @@
 
 static void load_host_msrs(struct kvm_vcpu *vcpu)
 {
-	int i;
-
-	for ( i = 0; i < NR_HOST_SAVE_MSRS; i++)
-		wrmsrl(host_save_msrs[i], vcpu->svm->host_msrs[i]);
+#ifdef CONFIG_X86_64
+	wrmsrl(MSR_GS_BASE, vcpu->svm->host_gs_base);
+#endif
 }
 
 static void save_host_msrs(struct kvm_vcpu *vcpu)
 {
-	int i;
-
-	for ( i = 0; i < NR_HOST_SAVE_MSRS; i++)
-		rdmsrl(host_save_msrs[i], vcpu->svm->host_msrs[i]);
+#ifdef CONFIG_X86_64
+	rdmsrl(MSR_GS_BASE, vcpu->svm->host_gs_base);
+#endif
 }
 
 static void new_asid(struct kvm_vcpu *vcpu, struct svm_cpu_data *svm_data)
@@ -1498,6 +1510,11 @@
 		load_db_regs(vcpu->svm->db_regs);
 	}
 
+	if ((vcpu->svm->vmcb->save.dr7 & 0xff) ||
+	    (vcpu->svm->host_dr7 & 0xff)) {
+		wrmsrl(MSR_IA32_DEBUGCTLMSR, vcpu->svm->host_ia32_debugctl);
+	}
+
 	if (vcpu->fpu_active) {
 		fx_save(vcpu->host_fx_image);
 		fx_restore(vcpu->guest_fx_image);
@@ -1617,6 +1634,11 @@
 		fx_restore(vcpu->host_fx_image);
 	}
 
+	if ((vcpu->svm->vmcb->save.dr7 & 0xff) ||
+	    (vcpu->svm->host_dr7 & 0xff)) {
+		rdmsrl(MSR_IA32_DEBUGCTLMSR, vcpu->svm->host_ia32_debugctl);
+	}
+
 	if ((vcpu->svm->vmcb->save.dr7 & 0xff))
 		load_db_regs(vcpu->svm->host_db_regs);
 
Index: kernel/drivers/kvm/kvm_svm.h
===================================================================
--- kernel.orig/drivers/kvm/kvm_svm.h	2007-04-27 17:52:37.240774864 -0500
+++ kernel/drivers/kvm/kvm_svm.h	2007-04-27 17:53:42.509852456 -0500
@@ -9,17 +9,15 @@
 #include "svm.h"
 #include "kvm.h"
 
-static const u32 host_save_msrs[] = {
+static const u32 host_save_user_msrs[] = {
 #ifdef CONFIG_X86_64
 	MSR_STAR, MSR_LSTAR, MSR_CSTAR, MSR_SYSCALL_MASK, MSR_KERNEL_GS_BASE,
-	MSR_FS_BASE, MSR_GS_BASE,
+	MSR_FS_BASE,
 #endif
 	MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP,
-	MSR_IA32_DEBUGCTLMSR, /*MSR_IA32_LASTBRANCHFROMIP,
-	MSR_IA32_LASTBRANCHTOIP, MSR_IA32_LASTINTFROMIP,MSR_IA32_LASTINTTOIP,*/
 };
 
-#define NR_HOST_SAVE_MSRS ARRAY_SIZE(host_save_msrs)
+#define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
 #define NUM_DB_REGS 4
 
 struct vcpu_svm {
@@ -32,7 +30,9 @@
 
 	u64 next_rip;
 
-	u64 host_msrs[NR_HOST_SAVE_MSRS];
+	u64 host_user_msrs[NR_HOST_SAVE_USER_MSRS];
+	u64 host_ia32_debugctl;
+	u64 host_gs_base;
 	unsigned long host_cr2;
 	unsigned long host_db_regs[NUM_DB_REGS];
 	unsigned long host_dr6;

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH][SVM] Only save/restore MSRs when needed

[Anthony Liguori]

Anthony Liguori wrote:
> I've tested this with 32-bit and 64-bit guests on a 64-bit host and 
> with 32-bit guests on a 32-bit host.
>
> I *think* it's doing the right thing wrt to DEBUGCTL but an extra set 
> of eyes would be helpful.

I think the follow patch preserves the current behavior but I'm starting 
to believe that the current behavior is wrong.

The current code assumes that the guest DEBUGCTL gets loaded/saved 
during vmrun/vmexit.  By my reading of the spec (I finally printed out 
the new one :-)), DEBUGCTL only gets saved if LBR virtualization is 
enabled.  LBR virtualization also triggers the save/restore of the other 
4 debug registers.  The spec seems to suggest that the host version is 
saved before the guest is loaded too.

I think the right behavior is to do nothing if LBR is available and 
enabled; hw will do it all for us.  If LBR isn't available, I think we 
should be saving/restoring not just DEBUGCTL but the other 4.  We can 
further optimize that path though by doing the same sort of tricks I 
have below.

If that all seems sane, I'll submit another patch to fix that up.

Regards,

Anthony Liguori

> Depending on my machine's mood, we're at somewhere between 3200 and 
> 2700 cycles per VMEXIT.
>
> Regards,
>
> Anthony Liguori
> ------------------------------------------------------------------------
>
> From: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Subject: [PATCH][SVM] Only save/restore MSRs when needed
>
>   We only have to save/restore MSR_GS_BASE on every VMEXIT.  The rest can be
>   saved/restored when we leave the VCPU.  As a special case, MSR_IA32_DEBUGCTL
>   only needs to be saved/restored on every exit if debugging is enabled in
>   either the host or the guest.
>
> Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>
> Index: kernel/drivers/kvm/svm.c
> ===================================================================
> --- kernel.orig/drivers/kvm/svm.c	2007-04-27 17:52:37.200780944 -0500
> +++ kernel/drivers/kvm/svm.c	2007-04-27 18:14:02.098446808 -0500
> @@ -611,7 +611,7 @@
>  
>  static void svm_vcpu_load(struct kvm_vcpu *vcpu)
>  {
> -	int cpu;
> +	int cpu, i;
>  
>  	cpu = get_cpu();
>  	if (unlikely(cpu != vcpu->cpu)) {
> @@ -626,10 +626,24 @@
>  		vcpu->svm->vmcb->control.tsc_offset += delta;
>  		vcpu->cpu = cpu;
>  	}
> +
> +	for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
> +		rdmsrl(host_save_user_msrs[i], vcpu->svm->host_user_msrs[i]);
> +
> +	rdmsrl(MSR_IA32_DEBUGCTLMSR, vcpu->svm->host_ia32_debugctl);
>  }
>  
>  static void svm_vcpu_put(struct kvm_vcpu *vcpu)
>  {
> +	int i;
> +
> +	for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
> +		wrmsrl(host_save_user_msrs[i], vcpu->svm->host_user_msrs[i]);
> +
> +	wrmsrl(MSR_IA32_DEBUGCTLMSR, vcpu->svm->host_ia32_debugctl);
> +	/* FIXME: LASTBRANCHFROMIP, LASTBRANCHTOIP, LASTINTFROMIP,
> +	   LASTINTTOIP */
> +
>  	rdtscll(vcpu->host_tsc);
>  	put_cpu();
>  }
> @@ -815,18 +829,16 @@
>  
>  static void load_host_msrs(struct kvm_vcpu *vcpu)
>  {
> -	int i;
> -
> -	for ( i = 0; i < NR_HOST_SAVE_MSRS; i++)
> -		wrmsrl(host_save_msrs[i], vcpu->svm->host_msrs[i]);
> +#ifdef CONFIG_X86_64
> +	wrmsrl(MSR_GS_BASE, vcpu->svm->host_gs_base);
> +#endif
>  }
>  
>  static void save_host_msrs(struct kvm_vcpu *vcpu)
>  {
> -	int i;
> -
> -	for ( i = 0; i < NR_HOST_SAVE_MSRS; i++)
> -		rdmsrl(host_save_msrs[i], vcpu->svm->host_msrs[i]);
> +#ifdef CONFIG_X86_64
> +	rdmsrl(MSR_GS_BASE, vcpu->svm->host_gs_base);
> +#endif
>  }
>  
>  static void new_asid(struct kvm_vcpu *vcpu, struct svm_cpu_data *svm_data)
> @@ -1498,6 +1510,11 @@
>  		load_db_regs(vcpu->svm->db_regs);
>  	}
>  
> +	if ((vcpu->svm->vmcb->save.dr7 & 0xff) ||
> +	    (vcpu->svm->host_dr7 & 0xff)) {
> +		wrmsrl(MSR_IA32_DEBUGCTLMSR, vcpu->svm->host_ia32_debugctl);
> +	}
> +
>  	if (vcpu->fpu_active) {
>  		fx_save(vcpu->host_fx_image);
>  		fx_restore(vcpu->guest_fx_image);
> @@ -1617,6 +1634,11 @@
>  		fx_restore(vcpu->host_fx_image);
>  	}
>  
> +	if ((vcpu->svm->vmcb->save.dr7 & 0xff) ||
> +	    (vcpu->svm->host_dr7 & 0xff)) {
> +		rdmsrl(MSR_IA32_DEBUGCTLMSR, vcpu->svm->host_ia32_debugctl);
> +	}
> +
>  	if ((vcpu->svm->vmcb->save.dr7 & 0xff))
>  		load_db_regs(vcpu->svm->host_db_regs);
>  
> Index: kernel/drivers/kvm/kvm_svm.h
> ===================================================================
> --- kernel.orig/drivers/kvm/kvm_svm.h	2007-04-27 17:52:37.240774864 -0500
> +++ kernel/drivers/kvm/kvm_svm.h	2007-04-27 17:53:42.509852456 -0500
> @@ -9,17 +9,15 @@
>  #include "svm.h"
>  #include "kvm.h"
>  
> -static const u32 host_save_msrs[] = {
> +static const u32 host_save_user_msrs[] = {
>  #ifdef CONFIG_X86_64
>  	MSR_STAR, MSR_LSTAR, MSR_CSTAR, MSR_SYSCALL_MASK, MSR_KERNEL_GS_BASE,
> -	MSR_FS_BASE, MSR_GS_BASE,
> +	MSR_FS_BASE,
>  #endif
>  	MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP,
> -	MSR_IA32_DEBUGCTLMSR, /*MSR_IA32_LASTBRANCHFROMIP,
> -	MSR_IA32_LASTBRANCHTOIP, MSR_IA32_LASTINTFROMIP,MSR_IA32_LASTINTTOIP,*/
>  };
>  
> -#define NR_HOST_SAVE_MSRS ARRAY_SIZE(host_save_msrs)
> +#define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
>  #define NUM_DB_REGS 4
>  
>  struct vcpu_svm {
> @@ -32,7 +30,9 @@
>  
>  	u64 next_rip;
>  
> -	u64 host_msrs[NR_HOST_SAVE_MSRS];
> +	u64 host_user_msrs[NR_HOST_SAVE_USER_MSRS];
> +	u64 host_ia32_debugctl;
> +	u64 host_gs_base;
>  	unsigned long host_cr2;
>  	unsigned long host_db_regs[NUM_DB_REGS];
>  	unsigned long host_dr6;
>   
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> ------------------------------------------------------------------------
>
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH][SVM] Only save/restore MSRs when needed

[Avi Kivity]

Anthony Liguori wrote:
> Anthony Liguori wrote:
>> I've tested this with 32-bit and 64-bit guests on a 64-bit host and
>> with 32-bit guests on a 32-bit host.
>>
>> I *think* it's doing the right thing wrt to DEBUGCTL but an extra set
>> of eyes would be helpful.
>
> I think the follow patch preserves the current behavior but I'm
> starting to believe that the current behavior is wrong.
>
> The current code assumes that the guest DEBUGCTL gets loaded/saved
> during vmrun/vmexit.  By my reading of the spec (I finally printed out
> the new one :-)), DEBUGCTL only gets saved if LBR virtualization is
> enabled.  LBR virtualization also triggers the save/restore of the
> other 4 debug registers.  The spec seems to suggest that the host
> version is saved before the guest is loaded too.
>
> I think the right behavior is to do nothing if LBR is available and
> enabled; hw will do it all for us.  If LBR isn't available, I think we
> should be saving/restoring not just DEBUGCTL but the other 4.  We can
> further optimize that path though by doing the same sort of tricks I
> have below.
>
> If that all seems sane, I'll submit another patch to fix that up.
>

Some observations from grepping the sources:

- The host never uses the DEBUGCTL msr.  So we only need to restore it
(to zero) if the guest changes it.
- The guest never uses the DEBUGCTL msr, as kvm doesn't handle wrmsr to
DEBUGCTL.

So, doing nothing (in hardware and software) would probably work well
and be quite fast.  If we ever emulate DEBUGCTL, we can conditionally
enable hardware or software save/restore (this is fairly similar to lazy
fpu).  If Linux ever gets DEBUGCTL support, we'd just ask for a hook so
we can conditionally save and restore on the host side.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH][SVM] Only save/restore MSRs when needed

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>   
>> Anthony Liguori wrote:
>>     
>>> I've tested this with 32-bit and 64-bit guests on a 64-bit host and
>>> with 32-bit guests on a 32-bit host.
>>>
>>> I *think* it's doing the right thing wrt to DEBUGCTL but an extra set
>>> of eyes would be helpful.
>>>       
>> I think the follow patch preserves the current behavior but I'm
>> starting to believe that the current behavior is wrong.
>>
>> The current code assumes that the guest DEBUGCTL gets loaded/saved
>> during vmrun/vmexit.  By my reading of the spec (I finally printed out
>> the new one :-)), DEBUGCTL only gets saved if LBR virtualization is
>> enabled.  LBR virtualization also triggers the save/restore of the
>> other 4 debug registers.  The spec seems to suggest that the host
>> version is saved before the guest is loaded too.
>>
>> I think the right behavior is to do nothing if LBR is available and
>> enabled; hw will do it all for us.  If LBR isn't available, I think we
>> should be saving/restoring not just DEBUGCTL but the other 4.  We can
>> further optimize that path though by doing the same sort of tricks I
>> have below.
>>
>> If that all seems sane, I'll submit another patch to fix that up.
>>
>>     
>
> Some observations from grepping the sources:
>
> - The host never uses the DEBUGCTL msr.  So we only need to restore it
> (to zero) if the guest changes it.
> - The guest never uses the DEBUGCTL msr, as kvm doesn't handle wrmsr to
> DEBUGCTL.
>
> So, doing nothing (in hardware and software) would probably work well
> and be quite fast.  If we ever emulate DEBUGCTL, we can conditionally
> enable hardware or software save/restore (this is fairly similar to lazy
> fpu).  

Okay, so I'll resubmit with DEBUGCTL disabled (including lbr_enable = 
0).  I'll also see if I can find something that uses DEBUGCTL.

Regards,

Anthony Liguori

> If Linux ever gets DEBUGCTL support, we'd just ask for a hook so
> we can conditionally save and restore on the host side.
>
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/