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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <77e7451c-1e1a-7332-08c2-a5fe8ab930e3@redhat.com>
Date:   Mon, 13 Nov 2017 17:01:36 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Luwei Kang <luwei.kang@...el.com>, kvm@...r.kernel.org
Cc:     rkrcmar@...hat.com, tglx@...utronix.de, mingo@...hat.com,
        hpa@...or.com, x86@...nel.org, linux-kernel@...r.kernel.org,
        Chao Peng <chao.p.peng@...ux.intel.com>
Subject: Re: [patch v2 6/8] KVM: x86: Implement Intel processor trace context
 switch

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