linux-kernel - Re: [PATCH] KVM: SVM: Set HWCR[TscFreqSel] to host's value

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] [day] [month] [year] [list]

Message-ID: <Ynr5L7+OJ23tEowC@google.com>
Date:   Tue, 10 May 2022 23:45:51 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Shannon Zhao <shannon.zhao@...ux.alibaba.com>
Cc:     kvm@...r.kernel.org, pbonzini@...hat.com,
        linux-kernel@...r.kernel.org, yijunzhu@...ux.alibaba.com
Subject: Re: [PATCH] KVM: SVM: Set HWCR[TscFreqSel] to host's value

On Tue, May 10, 2022, Shannon Zhao wrote:
> KVM sets CPUID.80000007H:EDX[8] to 1, but not set HWCR[TscFreqSel].
> This will cause guest kernel printing below log on AMD platform even
> though the hardware TSC exactly counts with P0 frequency.
> "[Firmware Bug]: TSC doesn't count with P0 frequency!"
> 
> Fix it by setting HWCR[TscFreqSel] to host's value to indicate whether
> the TSC increments at the P0 frequency.

I don't think this is safe.  The APM says

  Some HWCR bits are implementation specific, and are described in the BIOS and
  Kernel Developer’s Guide (BKDG) or Processor Programming Reference Manual
  applicable to your product. Implementation specific HWCR bits are not listed below.

and then omits bit 24.  One thought to handle this would be to let userspace
write all the non-architectural bits, then userspace can set the magic,
non-architectural bits based on CPUID and vCPU FMS.

> Signed-off-by: Shannon Zhao <shannon.zhao@...ux.alibaba.com>
> ---
>  arch/x86/kvm/svm/svm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7e45d03..fb4bb51 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1139,6 +1139,11 @@ static void __svm_vcpu_reset(struct kvm_vcpu *vcpu)
>  	svm_init_osvw(vcpu);
>  	vcpu->arch.microcode_version = 0x01000065;
>  	svm->tsc_ratio_msr = kvm_default_tsc_scaling_ratio;
> +	/* 
> +	 * TSC frequency select is HWCR[24], set it to host's value to indicate

This really should get a #define in msr-index.h, but maybe that's "impossible"
because it's not an architectural bit.

> +	 * whether the TSC increments at the P0 frequency. 
> +	 */
> +	vcpu->arch.msr_hwcr = native_read_msr(MSR_K7_HWCR) & BIT_ULL(24);

This will break live save/restore, a.k.a. live migration.  KVM doesn't allow
writes to MSR_K7_HWCR to set anything other than bit 18, even if the write comes
from userspace.

>  
>  	if (sev_es_guest(vcpu->kvm))
>  		sev_es_vcpu_reset(svm);
> -- 
> 1.8.3.1
>