Re: [patch v2 6/8] KVM: x86: Implement Intel processor trace context switch

[Paolo Bonzini]

On 30/10/2017 23:05, Luwei Kang wrote:
> +static void pt_guest_enter(struct vcpu_vmx *vmx)
> +{
> +	u64 ctl;
> +
> +	if (pt_mode == PT_MODE_HOST || pt_mode == PT_MODE_HOST_GUEST) {
> +		rdmsrl(MSR_IA32_RTIT_CTL, ctl);
> +		vmx->pt_desc.host.ctl = ctl;
> +		if (ctl & RTIT_CTL_TRACEEN) {
> +			ctl &= ~RTIT_CTL_TRACEEN;
> +			wrmsrl(MSR_IA32_RTIT_CTL, ctl);
> +		}

This "if" is only needed for PT_MODE_HOST_GUEST, I believe.
PT_MODE_HOST can just use the "load RTIT_CTL" vmentry control to disable
tracing.

> +	}
> +
> +	if (pt_mode == PT_MODE_HOST_GUEST) {
> +		pt_save_msr(&vmx->pt_desc.host, vmx->pt_desc.addr_num);
> +		pt_load_msr(&vmx->pt_desc.guest, vmx->pt_desc.addr_num);
> +	}
> +}
> +
> +static void pt_guest_exit(struct vcpu_vmx *vmx)
> +{
> +	if (pt_mode == PT_MODE_HOST_GUEST) {
> +		pt_save_msr(&vmx->pt_desc.guest, vmx->pt_desc.addr_num);
> +		pt_load_msr(&vmx->pt_desc.host, vmx->pt_desc.addr_num);
> +		wrmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
> +	}
> +
> +	if (pt_mode == PT_MODE_HOST)
> +		wrmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
> +}


Please use an

	if (pt_mode == PT_MODE_HOST || pt_mode == PT_MODE_HOST_GUEST)

for the write to RTIT_CTL, so that pt_guest_exit mirrors pt_guest_entry.

Also, we don't actually need to write the MSR if RTIT_CTL_TRACEEN is
false.  With these changes, the cost of the "host-only" mode is
acceptable, but for host-guest mode it is very expensive to read and
write the MSRs on all vmentries and vmexits is very expensive.

It would be much better to avoid writing the guest state if the guest
RTIT_CTL has TRACEEN=0.  This would require keeping the intercepts until
TRACEEN=1, but a lot of the work would be needed anyway---see my review
of patch 7.

Thanks,

Paolo