Re: [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24]

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 28, 2023, Jim Mattson wrote:
> On certain CPUs, Linux guests expect HWCR.TscFreqSel[bit 24] to be
> set. If it isn't set, they complain:
> 	[Firmware Bug]: TSC doesn't count with P0 frequency!
> 
> Allow userspace to set this bit in the virtual HWCR to eliminate the
> above complaint.
> 
> Attempts to clear this bit from within the guest are ignored, to match
> the behavior of modern AMD processors.
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/x86.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1a323cae219c..9209fc0d1a51 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3700,11 +3700,26 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		data &= ~(u64)0x100;	/* ignore ignne emulation enable */
>  		data &= ~(u64)0x8;	/* ignore TLB cache disable */
>  
> -		/* Handle McStatusWrEn */
> -		if (data & ~BIT_ULL(18)) {
> +		/*
> +		 * Ignore guest attempts to set TscFreqSel.
> +		 */

No need for a multi-line comment.
/
> +		if (!msr_info->host_initiated)
> +			data &= ~BIT_ULL(24);

There's no need to clear this before the check below.  The (arguably useless)
print will show the "supported" bit, but I can't imagine anyone will care.

> +
> +		/*
> +		 * Allow McStatusWrEn and (from the host) TscFreqSel.

This is unnecessarily confusing IMO, just state that writes to TscFreqSel are
architecturally ignored.  This would also be an opportune time to explain why
KVM allows this stupidity...

> +		 */
> +		if (data & ~(BIT_ULL(18) | BIT_ULL(24))) {
>  			kvm_pr_unimpl_wrmsr(vcpu, msr, data);
>  			return 1;
>  		}
> +
> +		/*
> +		 * TscFreqSel is read-only from within the
> +		 * guest. Attempts to clear it are ignored.

Overly aggressive wrapping.

How about this?

---
 arch/x86/kvm/x86.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 791a644dd481..4dd64d359142 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3700,11 +3700,19 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		data &= ~(u64)0x100;	/* ignore ignne emulation enable */
 		data &= ~(u64)0x8;	/* ignore TLB cache disable */
 
-		/* Handle McStatusWrEn */
-		if (data & ~BIT_ULL(18)) {
+		/*
+		 * Allow McStatusWrEn and TscFreqSel, some guests whine if they
+		 * aren't set.
+		 */
+		if (data & ~(BIT_ULL(18) | BIT_ULL(24))) {
 			kvm_pr_unimpl_wrmsr(vcpu, msr, data);
 			return 1;
 		}
+
+		/* TscFreqSel is architecturally read-only, writes are ignored */
+		if (!msr_info->host_initiated)
+			data = ~(data & BIT_ULL(24)) |
+			       (vcpu->arch.msr_hwcr & BIT_ULL(24));
 		vcpu->arch.msr_hwcr = data;
 		break;
 	case MSR_FAM10H_MMIO_CONF_BASE:

base-commit: 831ee29a0d4a2219d30268f9fc577217d222e339
--