Re: [PATCH] lighweight VM Exit

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Dong, Eddie wrote:
> OK, how about this patch which further reduce the light weight VM Exit
> MSR save/restore?
>
>
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index 1288cff..ef96fae 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -1596,6 +1596,30 @@ void kvm_resched(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_resched);
>  
> +void load_msrs_select(struct vmx_msr_entry *e, int bitmap)
> +{
> +	unsigned long nr;
> +
> +	while (bitmap) {
> +		nr = __ffs(bitmap);
> +		clear_bit(nr,&bitmap);
> +		wrmsrl(e[nr].index, e[nr].data);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(load_msrs_select);
> +
> +void save_msrs_select(struct vmx_msr_entry *e, int bitmap)
> +{
> +	unsigned long nr;
> +
> +	while (bitmap) {
> +		nr = __ffs(bitmap);
> +		clear_bit(nr,&bitmap);
> +		rdmsrl(e[nr].index, e[nr].data);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(save_msrs_select);
> +
>   

__clear_bit() is faster here (no LOCK prefix).  But maybe we can avoid 
the entire thing by having a vcpu->active_msr_list (array of struct 
vmx_msr_entry) which is re-constructed every time the mode changes 
(instead of constructing the bitmap).  vmx_get_msr() can first look at 
the active msr list and then at the regular msr list.

>  
>  /*
> @@ -469,35 +466,51 @@ static void vmx_inject_gp(struct kvm_vcpu *vcpu,
> unsigned error_code)
>   */
>  static void setup_msrs(struct kvm_vcpu *vcpu)
>  {
> -	int nr_skip, nr_good_msrs;
> -
> -	if (is_long_mode(vcpu))
> -		nr_skip = NR_BAD_MSRS;
> -	else
> -		nr_skip = NR_64BIT_MSRS;
> -	nr_good_msrs = vcpu->nmsrs - nr_skip;
> +	int index,save_msrs;
>   

space after comma

>  
> -	/*
> -	 * MSR_K6_STAR is only needed on long mode guests, and only
> -	 * if efer.sce is enabled.
> -	 */
> -	if (find_msr_entry(vcpu, MSR_K6_STAR)) {
> -		--nr_good_msrs;
> -#ifdef CONFIG_X86_64
> -		if (is_long_mode(vcpu) && (vcpu->shadow_efer &
> EFER_SCE))
> -			++nr_good_msrs;
> +	vcpu->smsrs_bitmap = 0;
> +	if (is_long_mode(vcpu)) {
> +		if ((index=__find_msr_index(vcpu, MSR_SYSCALL_MASK)) >=
> 0) {
> +			set_bit(index, &vcpu->smsrs_bitmap);
> +		}
>   

Assignment outside if (), spaces around =, please.  Single statements 
without {}.

Also __set_bit() applies here.

> +		/*
> +		 * MSR_K6_STAR is only needed on long mode guests, and
> only
> +		 * if efer.sce is enabled.
> +		 */
> +		if ((index=__find_msr_index(vcpu, MSR_K6_STAR)) >= 0
> +#ifdef X86_64
> +			&& (vcpu->shadow_efer & EFER_SCE)
>  #endif
> +			) {
> +			set_bit(index, &vcpu->smsrs_bitmap);
>   

You're saving MSR_K6_STAR unnecessarily on i386.  Since we don't export 
EFER on i386 (one day we should...), the guest can't use syscall.

> +		}
>  	}
>  
> +	if ((index = __find_msr_index(vcpu, MSR_EFER)) >= 0) {
> +		save_msrs = 1;
> +	}
> +	else {
> +		save_msrs = 0;
> +		index = 0;
> +	}
>   

Why not use hardware autoloading?  Is it slower than software?

Otherwise looks good.  Did you measure performance improvement?  I 
usually use user/test/vmexit.c from kvm-userspace.git.


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


-------------------------------------------------------------------------
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/