Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Michael Kelley <mikelley@microsoft.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] KVM: x86: Hyper-V invariant TSC control
In-Reply-To: <Yx85fuFWR/X097SL@google.com>
References: <20220831085009.1627523-1-vkuznets@redhat.com>
 <20220831085009.1627523-2-vkuznets@redhat.com>
 <Yx85fuFWR/X097SL@google.com>
Date:   Tue, 13 Sep 2022 11:51:56 +0200
Message-ID: <877d27r48z.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Aug 31, 2022, Vitaly Kuznetsov wrote:
>> Normally, genuine Hyper-V doesn't expose architectural invariant TSC
>> (CPUID.80000007H:EDX[8]) to its guests by default. A special PV MSR
>> (HV_X64_MSR_TSC_INVARIANT_CONTROL, 0x40000118) and corresponding CPUID
>> feature bit (CPUID.0x40000003.EAX[15]) were introduced. When bit 0 of the
>> PV MSR is set, invariant TSC bit starts to show up in CPUID. When the
>> feature is exposed to Hyper-V guests, reenlightenment becomes unneeded.
>> 
>> Add the feature to KVM. Keep CPUID output intact when the feature
>> wasn't exposed to L1 and implement the required logic for hiding
>> invariant TSC when the feature was exposed and invariant TSC control
>> MSR wasn't written to. Copy genuine Hyper-V behavior and forbid to
>> disable the feature once it was enabled.
>> 
>> For the reference, for linux guests, support for the feature was added
>> in commit dce7cd62754b ("x86/hyperv: Allow guests to enable InvariantTSC").
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h |  1 +
>>  arch/x86/kvm/cpuid.c            |  7 +++++++
>>  arch/x86/kvm/hyperv.c           | 19 +++++++++++++++++++
>>  arch/x86/kvm/hyperv.h           | 15 +++++++++++++++
>>  arch/x86/kvm/x86.c              |  4 +++-
>>  5 files changed, 45 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 2c96c43c313a..9098187e13aa 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1021,6 +1021,7 @@ struct kvm_hv {
>>  	u64 hv_reenlightenment_control;
>>  	u64 hv_tsc_emulation_control;
>>  	u64 hv_tsc_emulation_status;
>> +	u64 hv_invtsc;
>
> For consistency with the other fields, should this be hv_tsc_invariant_control?

Yep.

>>  
>>  	/* How many vCPUs have VP index != vCPU index */
>>  	atomic_t num_mismatched_vp_indexes;
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 75dcf7a72605..8ccd45fd66a9 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1444,6 +1444,13 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>>  			    (data & TSX_CTRL_CPUID_CLEAR))
>>  				*ebx &= ~(F(RTM) | F(HLE));
>>  		}
>> +		/*
>> +		 * Filter out invariant TSC (CPUID.80000007H:EDX[8]) for Hyper-V
>> +		 * guests if needed.
>> +		 */
>> +		if (function == 0x80000007 && kvm_hv_invtsc_filtered(vcpu))
>
> This can be an else-if.  Kinda weird, but it could be written as
>
> 		else if (function = 0x80000007) {
> 			if (kvm_hv_invtsc_filtered(vcpu))
> 				*edx &= ~SF(CONSTANT_TSC)
> 		}
>
> to make it a pure function+index check.
>
>> +			*edx &= ~(1 << 8);
>
> Ugh, scattered.  Can you add a kvm_only_cpuid_leafs entry so that the bit doesn't
> have to be open coded?

Sure.

>
>> +
>>  	} else {
>>  		*eax = *ebx = *ecx = *edx = 0;
>>  		/*
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index ed804447589c..df90cd7501b9 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -991,6 +991,7 @@ static bool kvm_hv_msr_partition_wide(u32 msr)
>>  	case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
>>  	case HV_X64_MSR_TSC_EMULATION_CONTROL:
>>  	case HV_X64_MSR_TSC_EMULATION_STATUS:
>> +	case HV_X64_MSR_TSC_INVARIANT_CONTROL:
>>  	case HV_X64_MSR_SYNDBG_OPTIONS:
>>  	case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
>>  		r = true;
>> @@ -1275,6 +1276,9 @@ static bool hv_check_msr_access(struct kvm_vcpu_hv *hv_vcpu, u32 msr)
>>  	case HV_X64_MSR_TSC_EMULATION_STATUS:
>>  		return hv_vcpu->cpuid_cache.features_eax &
>>  			HV_ACCESS_REENLIGHTENMENT;
>> +	case HV_X64_MSR_TSC_INVARIANT_CONTROL:
>> +		return hv_vcpu->cpuid_cache.features_eax &
>> +			HV_ACCESS_TSC_INVARIANT;
>>  	case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
>>  	case HV_X64_MSR_CRASH_CTL:
>>  		return hv_vcpu->cpuid_cache.features_edx &
>> @@ -1402,6 +1406,17 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
>>  		if (!host)
>>  			return 1;
>>  		break;
>> +	case HV_X64_MSR_TSC_INVARIANT_CONTROL:
>> +		/* Only bit 0 is supported */
>> +		if (data & ~BIT_ULL(0))
>
> Can a #define be added instead of open coding bit 0?
>

Yes, and then we can avoid open coding it in Linux-on-Hyper-V code too
as it looks like

arch/x86/kernel/cpu/mshyperv.c:         wrmsrl(HV_X64_MSR_TSC_INVARIANT_CONTROL, 0x1);

today.


>> +			return 1;
>> +
>
> Doesn't the host CPUID need to be honored on writes from the guest?
>

You mean INVTSC itself (CPUID.80000007H:EDX[8])? That's a good
question. Genuine Hyper-V will never expose HV_ACCESS_TSC_INVARIANT
without it but a misbehaving KVM VMM can. In case we treat the feature
as a 'filter' only, we don't need to check for the architectural bit.

>> +		/* The feature can't be disabled from the guest */
>> +		if (!host && hv->hv_invtsc && !data)
>> +			return 1;
>> +
>> +		hv->hv_invtsc = data;
>> +		break;
>>  	case HV_X64_MSR_SYNDBG_OPTIONS:
>>  	case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
>>  		return syndbg_set_msr(vcpu, msr, data, host);
>> @@ -1577,6 +1592,9 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
>>  	case HV_X64_MSR_TSC_EMULATION_STATUS:
>>  		data = hv->hv_tsc_emulation_status;
>>  		break;
>> +	case HV_X64_MSR_TSC_INVARIANT_CONTROL:
>> +		data = hv->hv_invtsc;
>> +		break;
>>  	case HV_X64_MSR_SYNDBG_OPTIONS:
>>  	case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
>>  		return syndbg_get_msr(vcpu, msr, pdata, host);
>> @@ -2497,6 +2515,7 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>>  			ent->eax |= HV_MSR_REFERENCE_TSC_AVAILABLE;
>>  			ent->eax |= HV_ACCESS_FREQUENCY_MSRS;
>>  			ent->eax |= HV_ACCESS_REENLIGHTENMENT;
>> +			ent->eax |= HV_ACCESS_TSC_INVARIANT;
>>  
>>  			ent->ebx |= HV_POST_MESSAGES;
>>  			ent->ebx |= HV_SIGNAL_EVENTS;
>> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
>> index da2737f2a956..1a6316ab55eb 100644
>> --- a/arch/x86/kvm/hyperv.h
>> +++ b/arch/x86/kvm/hyperv.h
>> @@ -133,6 +133,21 @@ static inline bool kvm_hv_has_stimer_pending(struct kvm_vcpu *vcpu)
>>  			     HV_SYNIC_STIMER_COUNT);
>>  }
>>  
>> +/*
>> + * With HV_ACCESS_TSC_INVARIANT feature, invariant TSC (CPUID.80000007H:EDX[8])
>> + * is only observed after HV_X64_MSR_TSC_INVARIANT_CONTROL was written to.
>> + */
>> +static inline bool kvm_hv_invtsc_filtered(struct kvm_vcpu *vcpu)
>
> Can this be more strongly worded, e.g. maybe kvm_hv_is_invtsc_disabled()?  "Filtered"
> doesn't strictly mean disabled and makes it sound like there's something else that
> needs to act on the "filtering"
>

"Hidden"? :-) I'm OK with kvm_hv_is_invtsc_disabled() too.

>> +{
>> +	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
>> +	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
>> +
>> +	if (hv_vcpu && hv_vcpu->cpuid_cache.features_eax & HV_ACCESS_TSC_INVARIANT)
>
> Ah, I almost missed the inner check.  Can you write this as:
>
> 	if (!hv_vcpu)
> 		return false;
>
> so that the potentially postive/happy path is at the end?  I.e. follow the common
> pattern of:
>
> 	if (!something)
> 		return -ERRNO;
>
> 	return 0;
>

Sure.

>> +		return !hv->hv_invtsc;
>
> Kinda silly, but I think it's worth checking the exact bit here.  I don't see how
> the TSC can get more invariant, but if another bit is added, this could silently
> break.  And probably no need to grab to_kvm_v() locally.
>
> 	return to_kvm_hv(vcpu->kvm)->hv_invtsc;
>

Sure.

>
>> +
>> +	return false;
>
> Shouldn't this be "return true" if HyperV is enabled but doesn't have the CPUID
> bit set?  I assume the expectation is that host userspace won't set the common
> INVTSC flag without also setting HV_ACCESS_TSC_INVARIANT, but it's confusing logic
> as is.
>
> All in all, I think this?
>
> 	if (!hv_vcpu)
> 		return false;
>
> 	return hv_vcpu->cpuid_cache.features_eax & HV_ACCESS_TSC_INVARIANT &&
> 	       to_kvm_hv(vcpu->kvm)->hv_invtsc & BIT(0);
>

Actually yes, there might be some configurations out there which expose
INVTSC to Hyper-V guests without this new PV feature, no need to break them.

>> +}
>> +
>>  void kvm_hv_process_stimers(struct kvm_vcpu *vcpu);
>>  
>>  void kvm_hv_setup_tsc_page(struct kvm *kvm,
>

-- 
Vitaly