linux-kernel - Re: [patch v2 7/8] KVM: x86: add Intel PT msr RTIT

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]

Date:   Mon, 13 Nov 2017 17:22:03 +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 7/8] KVM: x86: add Intel PT msr RTIT_CTL read/write

On 30/10/2017 23:05, Luwei Kang wrote:
> From: Chao Peng <chao.p.peng@...ux.intel.com>
> 
> L1 hypervisor can't get the capability of "TraceEn can be
> set in VMX operation"(IA32_VMX_MISC[14]=0) and any attempt
> to set IA32_RTIT_CTL.TraceEn in VMX operation using WRMSR
> will cause a general-protection exception if IA32_VMX_MISC[14]
> is 0. So we need to leave out write this msr in L1 VMX
> operation.
> 
> Signed-off-by: Chao Peng <chao.p.peng@...ux.intel.com>
> Signed-off-by: Luwei Kang <luwei.kang@...el.com>
> ---
>  arch/x86/kvm/vmx.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 091120e..8f61a8d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -936,6 +936,7 @@ static void vmx_get_segment(struct kvm_vcpu *vcpu,
>  static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked);
>  static bool nested_vmx_is_page_fault_vmexit(struct vmcs12 *vmcs12,
>  					    u16 error_code);
> +static bool vmx_pt_supported(void);
>  
>  static DEFINE_PER_CPU(struct vmcs *, vmxarea);
>  static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
> @@ -3384,6 +3385,11 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			return 1;
>  		msr_info->data = vcpu->arch.ia32_xss;
>  		break;
> +	case MSR_IA32_RTIT_CTL:
> +		if (!vmx_pt_supported())
> +			return 1;
> +		msr_info->data = vmcs_read64(GUEST_IA32_RTIT_CTL);
> +		break;
>  	case MSR_TSC_AUX:
>  		if (!msr_info->host_initiated &&
>  		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> @@ -3508,6 +3514,11 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		else
>  			clear_atomic_switch_msr(vmx, MSR_IA32_XSS);
>  		break;
> +	case MSR_IA32_RTIT_CTL:
> +		if (!vmx_pt_supported() || to_vmx(vcpu)->nested.vmxon)
> +			return 1;

VMXON must also clear TraceEn bit (see 23.4 in the SDM).

You also need to support R/W of all the other MSRs, in order to make
them accessible to userspace, and add them in msrs_to_save and
kvm_init_msr_list.

Regarding the save/restore of the state, I am now wondering if you could
also use XSAVES and XRSTORS instead of multiple rdmsr/wrmsr in a loop.
The cost is two MSR writes on vmenter (because the guest must run with
IA32_XSS=0) and two on vmexit.

Thanks,

Paolo

> +		vmcs_write64(GUEST_IA32_RTIT_CTL, data);
> +		break;
>  	case MSR_TSC_AUX:
>  		if (!msr_info->host_initiated &&
>  		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
>