From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753515AbdKMQB5 (ORCPT <rfc822;w@1wt.eu>);
        Mon, 13 Nov 2017 11:01:57 -0500
Received: from mx1.redhat.com ([209.132.183.28]:23578 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751521AbdKMQBz (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 13 Nov 2017 11:01:55 -0500
Subject: Re: [patch v2 6/8] KVM: x86: Implement Intel processor trace context
 switch
To: Luwei Kang <luwei.kang@intel.com>, kvm@vger.kernel.org
Cc: rkrcmar@redhat.com, tglx@linutronix.de, mingo@redhat.com,
        hpa@zytor.com, x86@kernel.org, linux-kernel@vger.kernel.org,
        Chao Peng <chao.p.peng@linux.intel.com>
References: <1509401117-15521-1-git-send-email-luwei.kang@intel.com>
 <1509401117-15521-7-git-send-email-luwei.kang@intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <77e7451c-1e1a-7332-08c2-a5fe8ab930e3@redhat.com>
Date: Mon, 13 Nov 2017 17:01:36 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <1509401117-15521-7-git-send-email-luwei.kang@intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 13 Nov 2017 16:01:55 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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