| 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
| ||
|
Message-ID: <Yx85fuFWR/X097SL@google.com>
Date: Mon, 12 Sep 2022 13:51:58 +0000
From: Sean Christopherson <seanjc@...gle.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Michael Kelley <mikelley@...rosoft.com>,
Maxim Levitsky <mlevitsk@...hat.com>,
linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] KVM: x86: Hyper-V invariant TSC control
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@...hat.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?
>
> /* 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?
> +
> } 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?
> + return 1;
> +
Doesn't the host CPUID need to be honored on writes from the guest?
> + /* 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"
> +{
> + 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;
> + 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;
> +
> + 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);
> +}
> +
> void kvm_hv_process_stimers(struct kvm_vcpu *vcpu);
>
> void kvm_hv_setup_tsc_page(struct kvm *kvm,
Powered by blists - more mailing lists