[PATCH 0/5] KVM: VMX: PT (processor trace) optimizations and fixes

[PATCH 0/5] KVM: VMX: PT (processor trace) optimizations and fixes

[Xiaoyao Li]

Patch 1 and 2 are optimizations and cleanup.

Patch 3 - 5 are fixes for PT handling.

Xiaoyao Li (5):
  KVM: VMX: Restore host's MSR_IA32_RTIT_CTL when it's not zero
  KVM: VMX: Use cached vmx->pt_desc.addr_range
  KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit
  KVM: VMX: Disallow PT MSRs accessing if PT is not exposed to guest
  KVM: VMX: Check Intel PT related CPUID leaves

 arch/x86/kvm/cpuid.c   | 25 +++++++++++++++++++++++++
 arch/x86/kvm/vmx/vmx.c | 34 +++++++++++++++++++++-------------
 2 files changed, 46 insertions(+), 13 deletions(-)

-- 
2.27.0

[PATCH 1/5] KVM: VMX: Restore host's MSR_IA32_RTIT_CTL when it's not zero

[Xiaoyao Li]

A minor optimation to WRMSR MSR_IA32_RTIT_CTL when necessary.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fada1055f325..e0a9460e4dab 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1075,7 +1075,8 @@ static void pt_guest_exit(struct vcpu_vmx *vmx)
 	}
 
 	/* Reload host state (IA32_RTIT_CTL will be cleared on VM exit). */
-	wrmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
+	if (vmx->pt_desc.host.ctl)
+		wrmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
 }
 
 void vmx_set_host_fs_gs(struct vmcs_host_state *host, u16 fs_sel, u16 gs_sel,
-- 
2.27.0

Re: [PATCH 1/5] KVM: VMX: Restore host's MSR_IA32_RTIT_CTL when it's not zero

[Sean Christopherson]

On Tue, Aug 24, 2021, Xiaoyao Li wrote:
> A minor optimation to WRMSR MSR_IA32_RTIT_CTL when necessary.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index fada1055f325..e0a9460e4dab 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1075,7 +1075,8 @@ static void pt_guest_exit(struct vcpu_vmx *vmx)
>  	}
>  
>  	/* Reload host state (IA32_RTIT_CTL will be cleared on VM exit). */

Could you opportunistically update the comment to call out that KVM requires
VM_EXIT_CLEAR_IA32_RTIT_CTL to expose PT to the guest?  E.g. something like

	/*
	 * KVM's requires VM_EXIT_CLEAR_IA32_RTIT_CTL to expose PT to the guest,
	 * i.e. RTIT_CTL is always cleared on VM-Exit.  Restore it if necessary.
	 */

With that,

Reviewed-by: Sean Christopherson <seanjc@google.com> 

> -	wrmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
> +	if (vmx->pt_desc.host.ctl)
> +		wrmsrl(MSR_IA32_RTIT_CTL, vmx->pt_desc.host.ctl);
>  }
>  
>  void vmx_set_host_fs_gs(struct vmcs_host_state *host, u16 fs_sel, u16 gs_sel,
> -- 
> 2.27.0
> 

[PATCH 2/5] KVM: VMX: Use cached vmx->pt_desc.addr_range

[Xiaoyao Li]

The number of guest's valid PT ADDR MSRs is cached in
vmx->pt_desc.addr_range. Use it instead of calculating it again.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e0a9460e4dab..7ed96c460661 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2202,8 +2202,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (!pt_can_write_msr(vmx))
 			return 1;
 		index = msr_info->index - MSR_IA32_RTIT_ADDR0_A;
-		if (index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps,
-						       PT_CAP_num_address_ranges))
+		if (index >= 2 * vmx->pt_desc.addr_range)
 			return 1;
 		if (is_noncanonical_address(data, vcpu))
 			return 1;
-- 
2.27.0

Re: [PATCH 2/5] KVM: VMX: Use cached vmx->pt_desc.addr_range

[Sean Christopherson]

On Tue, Aug 24, 2021, Xiaoyao Li wrote:
> The number of guest's valid PT ADDR MSRs is cached in

Can you do s/cached/precomputed in the shortlog and changelog?  Explanation below.

> vmx->pt_desc.addr_range. Use it instead of calculating it again.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index e0a9460e4dab..7ed96c460661 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2202,8 +2202,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		if (!pt_can_write_msr(vmx))
>  			return 1;
>  		index = msr_info->index - MSR_IA32_RTIT_ADDR0_A;
> -		if (index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps,
> -						       PT_CAP_num_address_ranges))
> +		if (index >= 2 * vmx->pt_desc.addr_range)

Ugh, "validate" is a lie, a better name would be intel_pt_get_cap() or so.  There
is no validation, the helper is simply extracting the requested cap from the
passed in array of capabilities.

That matters in this case because the number of address ranges exposed to the
guest is not bounded by the number of address ranges present in hardware, i.e.
it's not "validated".  And that matters because KVM uses vmx->pt_desc.addr_range
to pass through the ADDRn_{A,B} MSRs when tracing enabled.  In other words,
userspace can expose MSRs to the guest that do not exist.

The bug shouldn't be a security issue, so long as Intel CPUs are bug free and
aren't doing silly things with MSR indexes.  The number of possible address ranges
is encoded in three bits, thus the theoretical max is 8 ranges.  So userspace can't
get access to arbitrary MSRs, just ADDR0_A -> ADDR7_B.

And since KVM would be modifying the "validated" value, it's more than just a
cache, hence the request to use "precomputed".

Finally, vmx_get_msr() should use the precomputed value as well.

P.S. If you want to introduce a bit of churn, s/addr_range/nr_addr_ranges would
     be a welcome change as well.

>  			return 1;
>  		if (is_noncanonical_address(data, vcpu))
>  			return 1;
> -- 
> 2.27.0
> 

Re: [PATCH 2/5] KVM: VMX: Use cached vmx->pt_desc.addr_range

[Xiaoyao Li]

On 8/24/2021 11:24 PM, Sean Christopherson wrote:
> On Tue, Aug 24, 2021, Xiaoyao Li wrote:
>> The number of guest's valid PT ADDR MSRs is cached in
> 
> Can you do s/cached/precomputed in the shortlog and changelog?  Explanation below.

OK.

>> vmx->pt_desc.addr_range. Use it instead of calculating it again.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index e0a9460e4dab..7ed96c460661 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -2202,8 +2202,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   		if (!pt_can_write_msr(vmx))
>>   			return 1;
>>   		index = msr_info->index - MSR_IA32_RTIT_ADDR0_A;
>> -		if (index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps,
>> -						       PT_CAP_num_address_ranges))
>> +		if (index >= 2 * vmx->pt_desc.addr_range)
> 
> Ugh, "validate" is a lie, a better name would be intel_pt_get_cap() or so.  There
> is no validation, the helper is simply extracting the requested cap from the
> passed in array of capabilities.
> 
> That matters in this case because the number of address ranges exposed to the
> guest is not bounded by the number of address ranges present in hardware, i.e.
> it's not "validated".  And that matters because KVM uses vmx->pt_desc.addr_range
> to pass through the ADDRn_{A,B} MSRs when tracing enabled.  In other words,
> userspace can expose MSRs to the guest that do not exist.

That's why I provided patch 5.

> The bug shouldn't be a security issue, so long as Intel CPUs are bug free and
> aren't doing silly things with MSR indexes.  The number of possible address ranges
> is encoded in three bits, thus the theoretical max is 8 ranges.  So userspace can't
> get access to arbitrary MSRs, just ADDR0_A -> ADDR7_B.
> 
> And since KVM would be modifying the "validated" value, it's more than just a
> cache, hence the request to use "precomputed".
> 
> Finally, vmx_get_msr() should use the precomputed value as well.

Argh, I missed it.

> P.S. If you want to introduce a bit of churn, s/addr_range/nr_addr_ranges would
>       be a welcome change as well.

In a separate patch?

>>   			return 1;
>>   		if (is_noncanonical_address(data, vcpu))
>>   			return 1;
>> -- 
>> 2.27.0
>>

[PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit

[Xiaoyao Li]

Per Intel SDM, RTIT_CTL_BRANCH_EN bit has no dependency on any CPUID
leaf 0x14.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 7ed96c460661..4a70a6d2f442 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7116,7 +7116,8 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
 
 	/* Initialize and clear the no dependency bits */
 	vmx->pt_desc.ctl_bitmask = ~(RTIT_CTL_TRACEEN | RTIT_CTL_OS |
-			RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC);
+			RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC |
+			RTIT_CTL_BRANCH_EN);
 
 	/*
 	 * If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set otherwise
@@ -7134,12 +7135,11 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
 				RTIT_CTL_CYC_THRESH | RTIT_CTL_PSB_FREQ);
 
 	/*
-	 * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn BranchEn and
-	 * MTCFreq can be set
+	 * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn and MTCFreq can be set
 	 */
 	if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_mtc))
 		vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_MTC_EN |
-				RTIT_CTL_BRANCH_EN | RTIT_CTL_MTC_RANGE);
+					      RTIT_CTL_MTC_RANGE);
 
 	/* If CPUID.(EAX=14H,ECX=0):EBX[4]=1 FUPonPTW and PTWEn can be set */
 	if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_ptwrite))
-- 
2.27.0

Re: [PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit

[Like Xu]

+Alexander

On 24/8/2021 7:07 pm, Xiaoyao Li wrote:
> Per Intel SDM, RTIT_CTL_BRANCH_EN bit has no dependency on any CPUID
> leaf 0x14.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   arch/x86/kvm/vmx/vmx.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 7ed96c460661..4a70a6d2f442 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7116,7 +7116,8 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>   
>   	/* Initialize and clear the no dependency bits */
>   	vmx->pt_desc.ctl_bitmask = ~(RTIT_CTL_TRACEEN | RTIT_CTL_OS |
> -			RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC);
> +			RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC |
> +			RTIT_CTL_BRANCH_EN);
>   
>   	/*
>   	 * If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set otherwise
> @@ -7134,12 +7135,11 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>   				RTIT_CTL_CYC_THRESH | RTIT_CTL_PSB_FREQ);
>   
>   	/*
> -	 * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn BranchEn and
> -	 * MTCFreq can be set
> +	 * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn and MTCFreq can be set

If CPUID.(EAX=14H,ECX=0):EBX[3]=1,

	"indicates support of MTC timing packet and suppression of COFI-based packets."

Per 31.2.5.4 Branch Enable (BranchEn),

	"If BranchEn is not set, then relevant COFI packets (TNT, TIP*, FUP, MODE.*) 
are suppressed."

I think if the COFI capability is suppressed, the software can't set the 
BranchEn bit, right ?

>   	 */
>   	if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_mtc))
>   		vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_MTC_EN |
> -				RTIT_CTL_BRANCH_EN | RTIT_CTL_MTC_RANGE);
> +					      RTIT_CTL_MTC_RANGE);
>   
>   	/* If CPUID.(EAX=14H,ECX=0):EBX[4]=1 FUPonPTW and PTWEn can be set */
>   	if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_ptwrite))
> 

Re: [PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit

[Xiaoyao Li]

On 8/25/2021 11:30 AM, Like Xu wrote:
> +Alexander
> 
> On 24/8/2021 7:07 pm, Xiaoyao Li wrote:
>> Per Intel SDM, RTIT_CTL_BRANCH_EN bit has no dependency on any CPUID
>> leaf 0x14.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 7ed96c460661..4a70a6d2f442 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -7116,7 +7116,8 @@ static void update_intel_pt_cfg(struct kvm_vcpu 
>> *vcpu)
>>       /* Initialize and clear the no dependency bits */
>>       vmx->pt_desc.ctl_bitmask = ~(RTIT_CTL_TRACEEN | RTIT_CTL_OS |
>> -            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC);
>> +            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC |
>> +            RTIT_CTL_BRANCH_EN);
>>       /*
>>        * If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set otherwise
>> @@ -7134,12 +7135,11 @@ static void update_intel_pt_cfg(struct 
>> kvm_vcpu *vcpu)
>>                   RTIT_CTL_CYC_THRESH | RTIT_CTL_PSB_FREQ);
>>       /*
>> -     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn BranchEn and
>> -     * MTCFreq can be set
>> +     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn and MTCFreq can be set
> 
> If CPUID.(EAX=14H,ECX=0):EBX[3]=1,
> 
>      "indicates support of MTC timing packet and suppression of 
> COFI-based packets."

I think it's a mistake of SDM in CPUID instruction.

If you read 31.3.1, table 31-11 of SDM 325462-075US,

It just says CPUID(0x14, 0):EBX[3]: MTC supprted.
It doesn't talk anything about COFI packets suppression.

Further as below.

> Per 31.2.5.4 Branch Enable (BranchEn),
> 
>      "If BranchEn is not set, then relevant COFI packets (TNT, TIP*, 
> FUP, MODE.*) are suppressed."
> 
> I think if the COFI capability is suppressed, the software can't set the 
> BranchEn bit, right ?

Based on your understanding, isn't it that

1. if CPUID.(EAX=14H,ECX=0):EBX[3]=0, it doesn't support "suppression of 
COFI-based packets".
2. if it doesn't support "suppression of COFI-based packets", then it 
doens't support "If BranchEn is not set, then relevant COFI packets 
(TNT, TIP*, FUP, MODE.*) are suppressed", i.e. BranchEn must be 1.

Anyway, I think it's just a mistake on CPUID instruction document of SDM.

CPUD.(EAX=14H,ECX=0):EBX[3] should only indicates the MTC support.

BranchEn should be always supported if PT is available. Per "31.2.7.2 
IA32_RTIT_CTL MSR" on SDM:
When BranchEn is 1, it enables COFI-based packets.
When BranchEn is 0, it disables COFI-based packtes. i.e., COFI packets 
are suppressed.

>>        */
>>       if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_mtc))
>>           vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_MTC_EN |
>> -                RTIT_CTL_BRANCH_EN | RTIT_CTL_MTC_RANGE);
>> +                          RTIT_CTL_MTC_RANGE);
>>       /* If CPUID.(EAX=14H,ECX=0):EBX[4]=1 FUPonPTW and PTWEn can be 
>> set */
>>       if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_ptwrite))
>>

Re: [PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit

[Like Xu]

On 25/8/2021 12:19 pm, Xiaoyao Li wrote:
> On 8/25/2021 11:30 AM, Like Xu wrote:
>> +Alexander
>>
>> On 24/8/2021 7:07 pm, Xiaoyao Li wrote:
>>> Per Intel SDM, RTIT_CTL_BRANCH_EN bit has no dependency on any CPUID
>>> leaf 0x14.
>>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> ---
>>>   arch/x86/kvm/vmx/vmx.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>> index 7ed96c460661..4a70a6d2f442 100644
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -7116,7 +7116,8 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>>>       /* Initialize and clear the no dependency bits */
>>>       vmx->pt_desc.ctl_bitmask = ~(RTIT_CTL_TRACEEN | RTIT_CTL_OS |
>>> -            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC);
>>> +            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC |
>>> +            RTIT_CTL_BRANCH_EN);
>>>       /*
>>>        * If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set otherwise
>>> @@ -7134,12 +7135,11 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>>>                   RTIT_CTL_CYC_THRESH | RTIT_CTL_PSB_FREQ);
>>>       /*
>>> -     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn BranchEn and
>>> -     * MTCFreq can be set
>>> +     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn and MTCFreq can be set
>>
>> If CPUID.(EAX=14H,ECX=0):EBX[3]=1,
>>
>>      "indicates support of MTC timing packet and suppression of COFI-based 
>> packets."
> 
> I think it's a mistake of SDM in CPUID instruction.
> 
> If you read 31.3.1, table 31-11 of SDM 325462-075US,
> 
> It just says CPUID(0x14, 0):EBX[3]: MTC supprted.
> It doesn't talk anything about COFI packets suppression.
> 
> Further as below.
> 
>> Per 31.2.5.4 Branch Enable (BranchEn),
>>
>>      "If BranchEn is not set, then relevant COFI packets (TNT, TIP*, FUP, 
>> MODE.*) are suppressed."
>>
>> I think if the COFI capability is suppressed, the software can't set the 
>> BranchEn bit, right ?
> 
> Based on your understanding, isn't it that
> 
> 1. if CPUID.(EAX=14H,ECX=0):EBX[3]=0, it doesn't support "suppression of 
> COFI-based packets".
> 2. if it doesn't support "suppression of COFI-based packets", then it doens't 
> support "If BranchEn is not set, then relevant COFI packets (TNT, TIP*, FUP, 
> MODE.*) are suppressed", i.e. BranchEn must be 1.

That's it.

> 
> Anyway, I think it's just a mistake on CPUID instruction document of SDM.

Is this an ambiguity rather than a mistake ?

> 
> CPUD.(EAX=14H,ECX=0):EBX[3] should only indicates the MTC support.

Please do not make assertions that you do not confirm with hw.

> 
> BranchEn should be always supported if PT is available. Per "31.2.7.2 

Check d35869ba348d3f1ff3e6d8214fe0f674bb0e404e.

> IA32_RTIT_CTL MSR" on SDM:
> When BranchEn is 1, it enables COFI-based packets.
> When BranchEn is 0, it disables COFI-based packtes. i.e., COFI packets are 
> suppressed.
> 
>>>        */
>>>       if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_mtc))
>>>           vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_MTC_EN |
>>> -                RTIT_CTL_BRANCH_EN | RTIT_CTL_MTC_RANGE);
>>> +                          RTIT_CTL_MTC_RANGE);
>>>       /* If CPUID.(EAX=14H,ECX=0):EBX[4]=1 FUPonPTW and PTWEn can be set */
>>>       if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_ptwrite))
>>>
> 

Re: [PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit

[Xiaoyao Li]

On 8/25/2021 2:08 PM, Like Xu wrote:
> On 25/8/2021 12:19 pm, Xiaoyao Li wrote:
>> On 8/25/2021 11:30 AM, Like Xu wrote:
>>> +Alexander
>>>
>>> On 24/8/2021 7:07 pm, Xiaoyao Li wrote:
>>>> Per Intel SDM, RTIT_CTL_BRANCH_EN bit has no dependency on any CPUID
>>>> leaf 0x14.
>>>>
>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>> ---
>>>>   arch/x86/kvm/vmx/vmx.c | 8 ++++----
>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>>> index 7ed96c460661..4a70a6d2f442 100644
>>>> --- a/arch/x86/kvm/vmx/vmx.c
>>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>>> @@ -7116,7 +7116,8 @@ static void update_intel_pt_cfg(struct 
>>>> kvm_vcpu *vcpu)
>>>>       /* Initialize and clear the no dependency bits */
>>>>       vmx->pt_desc.ctl_bitmask = ~(RTIT_CTL_TRACEEN | RTIT_CTL_OS |
>>>> -            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC);
>>>> +            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC |
>>>> +            RTIT_CTL_BRANCH_EN);
>>>>       /*
>>>>        * If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set 
>>>> otherwise
>>>> @@ -7134,12 +7135,11 @@ static void update_intel_pt_cfg(struct 
>>>> kvm_vcpu *vcpu)
>>>>                   RTIT_CTL_CYC_THRESH | RTIT_CTL_PSB_FREQ);
>>>>       /*
>>>> -     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn BranchEn and
>>>> -     * MTCFreq can be set
>>>> +     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn and MTCFreq can be set
>>>
>>> If CPUID.(EAX=14H,ECX=0):EBX[3]=1,
>>>
>>>      "indicates support of MTC timing packet and suppression of 
>>> COFI-based packets."
>>
>> I think it's a mistake of SDM in CPUID instruction.
>>
>> If you read 31.3.1, table 31-11 of SDM 325462-075US,
>>
>> It just says CPUID(0x14, 0):EBX[3]: MTC supprted.
>> It doesn't talk anything about COFI packets suppression.
>>
>> Further as below.
>>
>>> Per 31.2.5.4 Branch Enable (BranchEn),
>>>
>>>      "If BranchEn is not set, then relevant COFI packets (TNT, TIP*, 
>>> FUP, MODE.*) are suppressed."
>>>
>>> I think if the COFI capability is suppressed, the software can't set 
>>> the BranchEn bit, right ?
>>
>> Based on your understanding, isn't it that
>>
>> 1. if CPUID.(EAX=14H,ECX=0):EBX[3]=0, it doesn't support "suppression 
>> of COFI-based packets".
>> 2. if it doesn't support "suppression of COFI-based packets", then it 
>> doens't support "If BranchEn is not set, then relevant COFI packets 
>> (TNT, TIP*, FUP, MODE.*) are suppressed", i.e. BranchEn must be 1.
> 
> That's it.
> 
>>
>> Anyway, I think it's just a mistake on CPUID instruction document of SDM.
> 
> Is this an ambiguity rather than a mistake ?
> 
>>
>> CPUD.(EAX=14H,ECX=0):EBX[3] should only indicates the MTC support.
> 
> Please do not make assertions that you do not confirm with hw.
> 
>>
>> BranchEn should be always supported if PT is available. Per "31.2.7.2 
> 
> Check d35869ba348d3f1ff3e6d8214fe0f674bb0e404e.

This commit shows BranchEn is supported on BDW, and must be enabled on 
BDW. This doesn't conflict the description above that BranchEn should be 
always supported.

>> IA32_RTIT_CTL MSR" on SDM:
>> When BranchEn is 1, it enables COFI-based packets.
>> When BranchEn is 0, it disables COFI-based packtes. i.e., COFI packets 
>> are suppressed.
>>
>>>>        */
>>>>       if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_mtc))
>>>>           vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_MTC_EN |
>>>> -                RTIT_CTL_BRANCH_EN | RTIT_CTL_MTC_RANGE);
>>>> +                          RTIT_CTL_MTC_RANGE);
>>>>       /* If CPUID.(EAX=14H,ECX=0):EBX[4]=1 FUPonPTW and PTWEn can be 
>>>> set */
>>>>       if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_ptwrite))
>>>>
>>

Re: [PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit

[Like Xu]

On 25/8/2021 2:33 pm, Xiaoyao Li wrote:
> On 8/25/2021 2:08 PM, Like Xu wrote:
>> On 25/8/2021 12:19 pm, Xiaoyao Li wrote:
>>> On 8/25/2021 11:30 AM, Like Xu wrote:
>>>> +Alexander
>>>>
>>>> On 24/8/2021 7:07 pm, Xiaoyao Li wrote:
>>>>> Per Intel SDM, RTIT_CTL_BRANCH_EN bit has no dependency on any CPUID
>>>>> leaf 0x14.
>>>>>
>>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>> ---
>>>>>   arch/x86/kvm/vmx/vmx.c | 8 ++++----
>>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>>>> index 7ed96c460661..4a70a6d2f442 100644
>>>>> --- a/arch/x86/kvm/vmx/vmx.c
>>>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>>>> @@ -7116,7 +7116,8 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>>>>>       /* Initialize and clear the no dependency bits */
>>>>>       vmx->pt_desc.ctl_bitmask = ~(RTIT_CTL_TRACEEN | RTIT_CTL_OS |
>>>>> -            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC);
>>>>> +            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC |
>>>>> +            RTIT_CTL_BRANCH_EN);
>>>>>       /*
>>>>>        * If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set otherwise
>>>>> @@ -7134,12 +7135,11 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>>>>>                   RTIT_CTL_CYC_THRESH | RTIT_CTL_PSB_FREQ);
>>>>>       /*
>>>>> -     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn BranchEn and
>>>>> -     * MTCFreq can be set
>>>>> +     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn and MTCFreq can be set
>>>>
>>>> If CPUID.(EAX=14H,ECX=0):EBX[3]=1,
>>>>
>>>>      "indicates support of MTC timing packet and suppression of COFI-based 
>>>> packets."
>>>
>>> I think it's a mistake of SDM in CPUID instruction.
>>>
>>> If you read 31.3.1, table 31-11 of SDM 325462-075US,
>>>
>>> It just says CPUID(0x14, 0):EBX[3]: MTC supprted.
>>> It doesn't talk anything about COFI packets suppression.
>>>
>>> Further as below.
>>>
>>>> Per 31.2.5.4 Branch Enable (BranchEn),
>>>>
>>>>      "If BranchEn is not set, then relevant COFI packets (TNT, TIP*, FUP, 
>>>> MODE.*) are suppressed."
>>>>
>>>> I think if the COFI capability is suppressed, the software can't set the 
>>>> BranchEn bit, right ?
>>>
>>> Based on your understanding, isn't it that
>>>
>>> 1. if CPUID.(EAX=14H,ECX=0):EBX[3]=0, it doesn't support "suppression of 
>>> COFI-based packets".
>>> 2. if it doesn't support "suppression of COFI-based packets", then it doens't 
>>> support "If BranchEn is not set, then relevant COFI packets (TNT, TIP*, FUP, 
>>> MODE.*) are suppressed", i.e. BranchEn must be 1.
>>
>> That's it.
>>
>>>
>>> Anyway, I think it's just a mistake on CPUID instruction document of SDM.
>>
>> Is this an ambiguity rather than a mistake ?
>>
>>>
>>> CPUD.(EAX=14H,ECX=0):EBX[3] should only indicates the MTC support.
>>
>> Please do not make assertions that you do not confirm with hw.
>>
>>>
>>> BranchEn should be always supported if PT is available. Per "31.2.7.2 
>>
>> Check d35869ba348d3f1ff3e6d8214fe0f674bb0e404e.
> 
> This commit shows BranchEn is supported on BDW, and must be enabled on BDW. This 
> doesn't conflict the description above that BranchEn should be always supported.

Per Vol. 4 Table 2-34. Additional MSRs Common to Processors Based the
Broadwell Microarchitectures, the BranchEn bit 13 is:

	"Reserved; writing 0 will #GP if also setting TraceEn"

on the Intel® Core™ M Processors.

My point is that we, especially software developers from hardware vendors,
should really focus on real hardware and fix real problems.

<EOM>

> 
>>> IA32_RTIT_CTL MSR" on SDM:
>>> When BranchEn is 1, it enables COFI-based packets.
>>> When BranchEn is 0, it disables COFI-based packtes. i.e., COFI packets are 
>>> suppressed.
>>>
>>>>>        */
>>>>>       if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_mtc))
>>>>>           vmx->pt_desc.ctl_bitmask &= ~(RTIT_CTL_MTC_EN |
>>>>> -                RTIT_CTL_BRANCH_EN | RTIT_CTL_MTC_RANGE);
>>>>> +                          RTIT_CTL_MTC_RANGE);
>>>>>       /* If CPUID.(EAX=14H,ECX=0):EBX[4]=1 FUPonPTW and PTWEn can be set */
>>>>>       if (intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_ptwrite))
>>>>>
>>>
> 

Re: [PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit

[Xiaoyao Li]

On 8/25/2021 4:14 PM, Like Xu wrote:
> On 25/8/2021 2:33 pm, Xiaoyao Li wrote:
>> On 8/25/2021 2:08 PM, Like Xu wrote:
>>> On 25/8/2021 12:19 pm, Xiaoyao Li wrote:
>>>> On 8/25/2021 11:30 AM, Like Xu wrote:
>>>>> +Alexander
>>>>>
>>>>> On 24/8/2021 7:07 pm, Xiaoyao Li wrote:
>>>>>> Per Intel SDM, RTIT_CTL_BRANCH_EN bit has no dependency on any CPUID
>>>>>> leaf 0x14.
>>>>>>
>>>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>>> ---
>>>>>>   arch/x86/kvm/vmx/vmx.c | 8 ++++----
>>>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>>>>> index 7ed96c460661..4a70a6d2f442 100644
>>>>>> --- a/arch/x86/kvm/vmx/vmx.c
>>>>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>>>>> @@ -7116,7 +7116,8 @@ static void update_intel_pt_cfg(struct 
>>>>>> kvm_vcpu *vcpu)
>>>>>>       /* Initialize and clear the no dependency bits */
>>>>>>       vmx->pt_desc.ctl_bitmask = ~(RTIT_CTL_TRACEEN | RTIT_CTL_OS |
>>>>>> -            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC);
>>>>>> +            RTIT_CTL_USR | RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC |
>>>>>> +            RTIT_CTL_BRANCH_EN);
>>>>>>       /*
>>>>>>        * If CPUID.(EAX=14H,ECX=0):EBX[0]=1 CR3Filter can be set 
>>>>>> otherwise
>>>>>> @@ -7134,12 +7135,11 @@ static void update_intel_pt_cfg(struct 
>>>>>> kvm_vcpu *vcpu)
>>>>>>                   RTIT_CTL_CYC_THRESH | RTIT_CTL_PSB_FREQ);
>>>>>>       /*
>>>>>> -     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn BranchEn and
>>>>>> -     * MTCFreq can be set
>>>>>> +     * If CPUID.(EAX=14H,ECX=0):EBX[3]=1 MTCEn and MTCFreq can be 
>>>>>> set
>>>>>
>>>>> If CPUID.(EAX=14H,ECX=0):EBX[3]=1,
>>>>>
>>>>>      "indicates support of MTC timing packet and suppression of 
>>>>> COFI-based packets."
>>>>
>>>> I think it's a mistake of SDM in CPUID instruction.
>>>>
>>>> If you read 31.3.1, table 31-11 of SDM 325462-075US,
>>>>
>>>> It just says CPUID(0x14, 0):EBX[3]: MTC supprted.
>>>> It doesn't talk anything about COFI packets suppression.
>>>>
>>>> Further as below.
>>>>
>>>>> Per 31.2.5.4 Branch Enable (BranchEn),
>>>>>
>>>>>      "If BranchEn is not set, then relevant COFI packets (TNT, 
>>>>> TIP*, FUP, MODE.*) are suppressed."
>>>>>
>>>>> I think if the COFI capability is suppressed, the software can't 
>>>>> set the BranchEn bit, right ?
>>>>
>>>> Based on your understanding, isn't it that
>>>>
>>>> 1. if CPUID.(EAX=14H,ECX=0):EBX[3]=0, it doesn't support 
>>>> "suppression of COFI-based packets".
>>>> 2. if it doesn't support "suppression of COFI-based packets", then 
>>>> it doens't support "If BranchEn is not set, then relevant COFI 
>>>> packets (TNT, TIP*, FUP, MODE.*) are suppressed", i.e. BranchEn must 
>>>> be 1.
>>>
>>> That's it.
>>>
>>>>
>>>> Anyway, I think it's just a mistake on CPUID instruction document of 
>>>> SDM.
>>>
>>> Is this an ambiguity rather than a mistake ?
>>>
>>>>
>>>> CPUD.(EAX=14H,ECX=0):EBX[3] should only indicates the MTC support.
>>>
>>> Please do not make assertions that you do not confirm with hw.
>>>
>>>>
>>>> BranchEn should be always supported if PT is available. Per "31.2.7.2 
>>>
>>> Check d35869ba348d3f1ff3e6d8214fe0f674bb0e404e.
>>
>> This commit shows BranchEn is supported on BDW, and must be enabled on 
>> BDW. This doesn't conflict the description above that BranchEn should 
>> be always supported.
> 
> Per Vol. 4 Table 2-34. Additional MSRs Common to Processors Based the
> Broadwell Microarchitectures, the BranchEn bit 13 is:
> 
>      "Reserved; writing 0 will #GP if also setting TraceEn"
> 
> on the Intel® Core™ M Processors.
> 
> My point is that we, especially software developers from hardware vendors,
> should really focus on real hardware and fix real problems.

Isn't this patch fixing real problem? Without it, it forbids guest to 
enable BranchEn if PT_MTC_cap not exposed to guest.

Re: [PATCH 3/5] KVM: VMX: RTIT_CTL_BRANCH_EN has no dependency on other CPUID bit

[Alexander Shishkin]

Xiaoyao Li <xiaoyao.li@intel.com> writes:

> On 8/25/2021 2:08 PM, Like Xu wrote:
>> On 25/8/2021 12:19 pm, Xiaoyao Li wrote:
>>> On 8/25/2021 11:30 AM, Like Xu wrote:
>>> BranchEn should be always supported if PT is available. Per "31.2.7.2 
>> 
>> Check d35869ba348d3f1ff3e6d8214fe0f674bb0e404e.
>
> This commit shows BranchEn is supported on BDW, and must be enabled on 
> BDW. This doesn't conflict the description above that BranchEn should be 
> always supported.

It's the *not* setting BranchEn that's not supported on BDW. The point
of BranchEn is to allow the user to not set it and filter out all the
branch trace related packets. The main point of PT, however, is the
branch trace, so in the first implementation BranchEn was reserved as
1.

IOW, it's always available, doesn't depend on CPUID, but on BDW,
BranchEn==0 should throw a #GP, if I remember right. Check BDM106 for
details.

Regards,
--
Alex

[PATCH 4/5] KVM: VMX: Disallow PT MSRs accessing if PT is not exposed to guest

[Xiaoyao Li]

Per SDM, it triggers #GP for all the accessing of PT MSRs, if
X86_FEATURE_INTEL_PT is not available.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4a70a6d2f442..1bbc4d84c623 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1010,9 +1010,16 @@ static unsigned long segment_base(u16 selector)
 static inline bool pt_can_write_msr(struct vcpu_vmx *vmx)
 {
 	return vmx_pt_mode_is_host_guest() &&
+	       guest_cpuid_has(&vmx->vcpu, X86_FEATURE_INTEL_PT) &&
 	       !(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN);
 }
 
+static inline bool pt_can_read_msr(struct kvm_vcpu *vcpu)
+{
+	return vmx_pt_mode_is_host_guest() &&
+	       guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT);
+}
+
 static inline bool pt_output_base_valid(struct kvm_vcpu *vcpu, u64 base)
 {
 	/* The base must be 128-byte aligned and a legal physical address. */
@@ -1849,24 +1856,24 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 							&msr_info->data);
 		break;
 	case MSR_IA32_RTIT_CTL:
-		if (!vmx_pt_mode_is_host_guest())
+		if (!pt_can_read_msr(vcpu))
 			return 1;
 		msr_info->data = vmx->pt_desc.guest.ctl;
 		break;
 	case MSR_IA32_RTIT_STATUS:
-		if (!vmx_pt_mode_is_host_guest())
+		if (!pt_can_read_msr(vcpu))
 			return 1;
 		msr_info->data = vmx->pt_desc.guest.status;
 		break;
 	case MSR_IA32_RTIT_CR3_MATCH:
-		if (!vmx_pt_mode_is_host_guest() ||
+		if (!pt_can_read_msr(vcpu) ||
 			!intel_pt_validate_cap(vmx->pt_desc.caps,
 						PT_CAP_cr3_filtering))
 			return 1;
 		msr_info->data = vmx->pt_desc.guest.cr3_match;
 		break;
 	case MSR_IA32_RTIT_OUTPUT_BASE:
-		if (!vmx_pt_mode_is_host_guest() ||
+		if (!pt_can_read_msr(vcpu) ||
 			(!intel_pt_validate_cap(vmx->pt_desc.caps,
 					PT_CAP_topa_output) &&
 			 !intel_pt_validate_cap(vmx->pt_desc.caps,
@@ -1875,7 +1882,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = vmx->pt_desc.guest.output_base;
 		break;
 	case MSR_IA32_RTIT_OUTPUT_MASK:
-		if (!vmx_pt_mode_is_host_guest() ||
+		if (!pt_can_read_msr(vcpu) ||
 			(!intel_pt_validate_cap(vmx->pt_desc.caps,
 					PT_CAP_topa_output) &&
 			 !intel_pt_validate_cap(vmx->pt_desc.caps,
@@ -1885,7 +1892,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
 		index = msr_info->index - MSR_IA32_RTIT_ADDR0_A;
-		if (!vmx_pt_mode_is_host_guest() ||
+		if (!pt_can_read_msr(vcpu) ||
 			(index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps,
 					PT_CAP_num_address_ranges)))
 			return 1;
@@ -2154,6 +2161,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		return vmx_set_vmx_msr(vcpu, msr_index, data);
 	case MSR_IA32_RTIT_CTL:
 		if (!vmx_pt_mode_is_host_guest() ||
+		    !guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT) ||
 			vmx_rtit_ctl_check(vcpu, data) ||
 			vmx->nested.vmxon)
 			return 1;
-- 
2.27.0

Re: [PATCH 4/5] KVM: VMX: Disallow PT MSRs accessing if PT is not exposed to guest

[Sean Christopherson]

On Tue, Aug 24, 2021, Xiaoyao Li wrote:
> Per SDM, it triggers #GP for all the accessing of PT MSRs, if
> X86_FEATURE_INTEL_PT is not available.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 4a70a6d2f442..1bbc4d84c623 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1010,9 +1010,16 @@ static unsigned long segment_base(u16 selector)
>  static inline bool pt_can_write_msr(struct vcpu_vmx *vmx)
>  {
>  	return vmx_pt_mode_is_host_guest() &&
> +	       guest_cpuid_has(&vmx->vcpu, X86_FEATURE_INTEL_PT) &&
>  	       !(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN);
>  }
>  
> +static inline bool pt_can_read_msr(struct kvm_vcpu *vcpu)
> +{
> +	return vmx_pt_mode_is_host_guest() &&
> +	       guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT);
> +}
> +
>  static inline bool pt_output_base_valid(struct kvm_vcpu *vcpu, u64 base)
>  {
>  	/* The base must be 128-byte aligned and a legal physical address. */
> @@ -1849,24 +1856,24 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  							&msr_info->data);
>  		break;
>  	case MSR_IA32_RTIT_CTL:
> -		if (!vmx_pt_mode_is_host_guest())
> +		if (!pt_can_read_msr(vcpu))

These all need to provide exemptions for accesses from the host.  KVM allows
access to MSRs that are not exposed to the guest so long as all the other checks
pass.  Same for the next patch.

Easiest thing is probably to pass in @msr_info to the helpers and do the check
there.

>  			return 1;
>  		msr_info->data = vmx->pt_desc.guest.ctl;
>  		break;

Re: [PATCH 4/5] KVM: VMX: Disallow PT MSRs accessing if PT is not exposed to guest

[Xiaoyao Li]

On 8/24/2021 10:20 PM, Sean Christopherson wrote:
> On Tue, Aug 24, 2021, Xiaoyao Li wrote:
>> Per SDM, it triggers #GP for all the accessing of PT MSRs, if
>> X86_FEATURE_INTEL_PT is not available.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 4a70a6d2f442..1bbc4d84c623 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -1010,9 +1010,16 @@ static unsigned long segment_base(u16 selector)
>>   static inline bool pt_can_write_msr(struct vcpu_vmx *vmx)
>>   {
>>   	return vmx_pt_mode_is_host_guest() &&
>> +	       guest_cpuid_has(&vmx->vcpu, X86_FEATURE_INTEL_PT) &&
>>   	       !(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN);
>>   }
>>   
>> +static inline bool pt_can_read_msr(struct kvm_vcpu *vcpu)
>> +{
>> +	return vmx_pt_mode_is_host_guest() &&
>> +	       guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT);
>> +}
>> +
>>   static inline bool pt_output_base_valid(struct kvm_vcpu *vcpu, u64 base)
>>   {
>>   	/* The base must be 128-byte aligned and a legal physical address. */
>> @@ -1849,24 +1856,24 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   							&msr_info->data);
>>   		break;
>>   	case MSR_IA32_RTIT_CTL:
>> -		if (!vmx_pt_mode_is_host_guest())
>> +		if (!pt_can_read_msr(vcpu))
> 
> These all need to provide exemptions for accesses from the host.  KVM allows
> access to MSRs that are not exposed to the guest so long as all the other checks
> pass. 

Not all the MSRs are allowed to be accessed from host regardless of 
whether it's exposed to guest. e.g., MSR_IA32_TSC_ADJUST, it checks 
guest CPUID first.

For me, for those PT MSRs, I cannot think of any reason that 
host/userspace would access them without PT being exposed to guest.

On the other hand, since this patch indeed breaks the existing userspace 
VMM who accesses those MSRs without checking guest CPUID.

So I will follow your advice to allow the host_initiated case in next 
version.

> Same for the next patch.

Sorry, I don't know how it matters next patch.

> Easiest thing is probably to pass in @msr_info to the helpers and do the check
> there.
> 
>>   			return 1;
>>   		msr_info->data = vmx->pt_desc.guest.ctl;
>>   		break;

Re: [PATCH 4/5] KVM: VMX: Disallow PT MSRs accessing if PT is not exposed to guest

[Sean Christopherson]

On Tue, Aug 24, 2021, Xiaoyao Li wrote:
> On 8/24/2021 10:20 PM, Sean Christopherson wrote:
> > On Tue, Aug 24, 2021, Xiaoyao Li wrote:
> > > Per SDM, it triggers #GP for all the accessing of PT MSRs, if
> > > X86_FEATURE_INTEL_PT is not available.
> > > 
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > ---
> > >   arch/x86/kvm/vmx/vmx.c | 20 ++++++++++++++------
> > >   1 file changed, 14 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 4a70a6d2f442..1bbc4d84c623 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -1010,9 +1010,16 @@ static unsigned long segment_base(u16 selector)
> > >   static inline bool pt_can_write_msr(struct vcpu_vmx *vmx)
> > >   {
> > >   	return vmx_pt_mode_is_host_guest() &&
> > > +	       guest_cpuid_has(&vmx->vcpu, X86_FEATURE_INTEL_PT) &&
> > >   	       !(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN);
> > >   }
> > > +static inline bool pt_can_read_msr(struct kvm_vcpu *vcpu)
> > > +{
> > > +	return vmx_pt_mode_is_host_guest() &&
> > > +	       guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT);
> > > +}
> > > +
> > >   static inline bool pt_output_base_valid(struct kvm_vcpu *vcpu, u64 base)
> > >   {
> > >   	/* The base must be 128-byte aligned and a legal physical address. */
> > > @@ -1849,24 +1856,24 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > >   							&msr_info->data);
> > >   		break;
> > >   	case MSR_IA32_RTIT_CTL:
> > > -		if (!vmx_pt_mode_is_host_guest())
> > > +		if (!pt_can_read_msr(vcpu))
> > 
> > These all need to provide exemptions for accesses from the host.  KVM allows
> > access to MSRs that are not exposed to the guest so long as all the other checks
> > pass.
> 
> Not all the MSRs are allowed to be accessed from host regardless of whether
> it's exposed to guest. e.g., MSR_IA32_TSC_ADJUST, it checks guest CPUID
> first.
> 
> For me, for those PT MSRs, I cannot think of any reason that host/userspace
> would access them without PT being exposed to guest.

Order of operations.  Userspace is allowed to do KVM_GET/SET_MSR before
KVM_SET_CPUID2.

> On the other hand, since this patch indeed breaks the existing userspace VMM
> who accesses those MSRs without checking guest CPUID.
> 
> So I will follow your advice to allow the host_initiated case in next
> version.
> 
> > Same for the next patch.
> 
> Sorry, I don't know how it matters next patch.

Me either.  Ignore that comment. :-)

[PATCH 5/5] KVM: VMX: Check Intel PT related CPUID leaves

[Xiaoyao Li]

CPUID 0XD leaves reports the capabilities of Intel PT and decides which
bits are valid to be set in MSR_IA32_RTIT_CTL.

KVM needs to check the guest CPUID values set by userspace doesn't
enable any bit which is not supported by bare metal. Otherwise, it
allows guest to set corresponding bit in MSR_IA32_RTIT_CTL and it will
trigger vm-entry failure if unsupported bit is exposed to guest and
set by guest.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
There is bit 31 of CPUID(0xD, 0).ECX that doesn't restrict any bit in
MSR_IA32_RTIT_CTL. If guest has different value than host, it won't
cause any vm-entry failure, but guest will parse the PT packet with
wrong format.

I also check it to be same as host to ensure the virtualization correctness.
---
 arch/x86/kvm/cpuid.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 739be5da3bca..0c8e06a24156 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -76,6 +76,7 @@ static inline struct kvm_cpuid_entry2 *cpuid_entry2_find(
 static int kvm_check_cpuid(struct kvm_cpuid_entry2 *entries, int nent)
 {
 	struct kvm_cpuid_entry2 *best;
+	u32 eax, ebx, ecx, edx;
 
 	/*
 	 * The existing code assumes virtual address is 48-bit or 57-bit in the
@@ -89,6 +90,30 @@ static int kvm_check_cpuid(struct kvm_cpuid_entry2 *entries, int nent)
 			return -EINVAL;
 	}
 
+	/*
+	 * CPUID 0xD leaves tell Intel PT capabilities, which decides
+	 * pt_desc.ctl_bitmask in later update_intel_pt_cfg().
+	 *
+	 * pt_desc.ctl_bitmask decides the legal value for guest
+	 * MSR_IA32_RTIT_CTL. KVM cannot support PT capabilities beyond native,
+	 * otherwise it will trigger vm-entry failure if guest sets native
+	 * unsupported bits in MSR_IA32_RTIT_CTL.
+	 */
+	best = cpuid_entry2_find(entries, nent, 0xD, 0);
+	if (best) {
+		cpuid_count(0xD, 0, &eax, &ebx, &ecx, &edx);
+		if (best->ebx & ~ebx || best->ecx & ~ecx)
+			return -EINVAL;
+	}
+	best = cpuid_entry2_find(entries, nent, 0xD, 1);
+	if (best) {
+		cpuid_count(0xD, 0, &eax, &ebx, &ecx, &edx);
+		if (((best->eax & 0x7) > (eax & 0x7)) ||
+		    ((best->eax & ~eax) >> 16) ||
+		    (best->ebx & ~ebx))
+			return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.27.0