Re: [PATCH 8/9] KVM, VMX: Add support for guest/host-only profiling

[Avi Kivity <avi@redhat.com>]

On 10/03/2011 03:49 PM, Gleb Natapov wrote:
> Support guest/host-only profiling by switch perf msrs on
> a guest entry if needed.
>
> @@ -6052,6 +6056,26 @@ static void vmx_cancel_injection(struct kvm_vcpu *vcpu)
>   	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);
>   }
>
> +static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
> +{
> +#ifdef CONFIG_PERF_EVENTS

No need for #ifdef (if you also define perf_guest_get_msrs() when 
!CONFIG_PERF_EVENTS).

>
> +	int i;
> +
> +	if (!cpu_has_arch_perfmon || vmx->perf_msrs_cnt<= 0)
> +		return;

Why the first check?

>
> +
> +	perf_guest_get_msrs(vmx->perf_msrs_cnt, vmx->perf_msrs);
> +	for (i = 0; i<  vmx->perf_msrs_cnt; i++) {
> +		struct perf_guest_switch_msr *msr =&vmx->perf_msrs[i];
> +		if (msr->host == msr->guest)
> +			clear_atomic_switch_msr(vmx, msr->msr);
> +		else
> +			add_atomic_switch_msr(vmx, msr->msr, msr->guest,
> +					msr->host);

This generates a lot of VMWRITEs even if nothing changes, just to re-set 
bits in the VMCS to their existing values.  Need to add something like this:

    if (loaded_vmcs->msr[i].host == msr->host
&& loaded_vmcs->msr[i].guest == msr->guest)
           continue;

btw, shouldn't the msr autoload list be part of loaded_vmcs as well?

>
> +	}
> +#endif
> +}
> +
>   #ifdef CONFIG_X86_64
>   #define R "r"
>   #define Q "q"
> @@ -6101,6 +6125,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>   	if (vcpu->guest_debug&  KVM_GUESTDBG_SINGLESTEP)
>   		vmx_set_interrupt_shadow(vcpu, 0);
>
> +	atomic_switch_perf_msrs(vmx);
> +
>   	vmx->__launched = vmx->loaded_vmcs->launched;
>   	asm(
>   		/* Store host registers */
> @@ -6306,6 +6332,15 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>   	vmx->nested.current_vmptr = -1ull;
>   	vmx->nested.current_vmcs12 = NULL;
>
> +	vmx->perf_msrs_cnt = perf_guest_get_msrs_count();
> +	if (vmx->perf_msrs_cnt>  0) {
> +		vmx->perf_msrs = kmalloc(vmx->perf_msrs_cnt *
> +				sizeof(struct perf_guest_switch_msr),
> +				GFP_KERNEL);
> +		if (!vmx->perf_msrs)
> +			goto free_vmcs;
> +	}
> +

Do we really need a private buffer?  Perhaps perf_guest_get_msrs() can 
return a perf-internal buffer (but then, we will need to copy it for the 
optimization above, but that's a separate issue).

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