kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* KVM: x86: Update HWCR virtualization
@ 2023-09-28 18:51 Jim Mattson
  2023-09-28 18:51 ` [PATCH v3 1/3] KVM: x86: Allow HWCR.McStatusWrEn to be cleared once set Jim Mattson
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Jim Mattson @ 2023-09-28 18:51 UTC (permalink / raw)
  To: kvm, 'Sean Christopherson ', 'Paolo Bonzini '; +Cc: Jim Mattson

Allow HWCR.McStatusWrEn to be cleared once set, and allow
HWCR.TscFreqSel to be set from userspace.

v1 -> v2: KVM no longer sets HWCR.TscFreqSel
          HWCR.TscFreqSel can be cleared from userspace
          Selftest modified accordingly
v2 -> v3: kvm_set_msr_common() changes simplified

Jim Mattson (3):
  KVM: x86: Allow HWCR.McStatusWrEn to be cleared once set
  KVM: x86: Virtualize HWCR.TscFreqSel[bit 24]
  KVM: selftests: Test behavior of HWCR

 arch/x86/kvm/x86.c                            | 22 ++++++--
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../selftests/kvm/x86_64/hwcr_msr_test.c      | 52 +++++++++++++++++++
 3 files changed, 71 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c

-- 
2.42.0.582.g8ccd20d70d-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 1/3] KVM: x86: Allow HWCR.McStatusWrEn to be cleared once set
  2023-09-28 18:51 KVM: x86: Update HWCR virtualization Jim Mattson
@ 2023-09-28 18:51 ` Jim Mattson
  2023-09-28 18:51 ` [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24] Jim Mattson
  2023-09-28 18:51 ` [PATCH v3 3/3] KVM: selftests: Test behavior of HWCR Jim Mattson
  2 siblings, 0 replies; 8+ messages in thread
From: Jim Mattson @ 2023-09-28 18:51 UTC (permalink / raw)
  To: kvm, 'Sean Christopherson ', 'Paolo Bonzini '; +Cc: Jim Mattson

When HWCR is set to 0, store 0 in vcpu->arch.msr_hwcr.

Fixes: 191c8137a939 ("x86/kvm: Implement HWCR support")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/x86.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9f18b06bbda6..1a323cae219c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3701,12 +3701,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		data &= ~(u64)0x8;	/* ignore TLB cache disable */
 
 		/* Handle McStatusWrEn */
-		if (data == BIT_ULL(18)) {
-			vcpu->arch.msr_hwcr = data;
-		} else if (data != 0) {
+		if (data & ~BIT_ULL(18)) {
 			kvm_pr_unimpl_wrmsr(vcpu, msr, data);
 			return 1;
 		}
+		vcpu->arch.msr_hwcr = data;
 		break;
 	case MSR_FAM10H_MMIO_CONF_BASE:
 		if (data != 0) {
-- 
2.42.0.582.g8ccd20d70d-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24]
  2023-09-28 18:51 KVM: x86: Update HWCR virtualization Jim Mattson
  2023-09-28 18:51 ` [PATCH v3 1/3] KVM: x86: Allow HWCR.McStatusWrEn to be cleared once set Jim Mattson
@ 2023-09-28 18:51 ` Jim Mattson
  2023-09-29  0:56   ` Sean Christopherson
  2023-09-28 18:51 ` [PATCH v3 3/3] KVM: selftests: Test behavior of HWCR Jim Mattson
  2 siblings, 1 reply; 8+ messages in thread
From: Jim Mattson @ 2023-09-28 18:51 UTC (permalink / raw)
  To: kvm, 'Sean Christopherson ', 'Paolo Bonzini '; +Cc: Jim Mattson

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.
+		 */
+		if (!msr_info->host_initiated)
+			data &= ~BIT_ULL(24);
+
+		/*
+		 * Allow McStatusWrEn and (from the host) TscFreqSel.
+		 */
+		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.
+		 */
+		if (!msr_info->host_initiated)
+			data |= vcpu->arch.msr_hwcr & BIT_ULL(24);
 		vcpu->arch.msr_hwcr = data;
 		break;
 	case MSR_FAM10H_MMIO_CONF_BASE:
-- 
2.42.0.582.g8ccd20d70d-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v3 3/3] KVM: selftests: Test behavior of HWCR
  2023-09-28 18:51 KVM: x86: Update HWCR virtualization Jim Mattson
  2023-09-28 18:51 ` [PATCH v3 1/3] KVM: x86: Allow HWCR.McStatusWrEn to be cleared once set Jim Mattson
  2023-09-28 18:51 ` [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24] Jim Mattson
@ 2023-09-28 18:51 ` Jim Mattson
  2023-09-29 17:06   ` Sean Christopherson
  2 siblings, 1 reply; 8+ messages in thread
From: Jim Mattson @ 2023-09-28 18:51 UTC (permalink / raw)
  To: kvm, 'Sean Christopherson ', 'Paolo Bonzini '; +Cc: Jim Mattson

Verify the following:
* Attempts to set bits 3, 6, or 8 are ignored
* Bits 18 and 24 are the only bits that can be set
* Any bit that can be set can also be cleared

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../selftests/kvm/x86_64/hwcr_msr_test.c      | 52 +++++++++++++++++++
 2 files changed, 53 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index a3bb36fb3cfc..6b0219ca65eb 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -135,6 +135,7 @@ TEST_GEN_PROGS_x86_64 += set_memory_region_test
 TEST_GEN_PROGS_x86_64 += steal_time
 TEST_GEN_PROGS_x86_64 += kvm_binary_stats_test
 TEST_GEN_PROGS_x86_64 += system_counter_offset_test
+TEST_GEN_PROGS_x86_64 += x86_64/hwcr_msr_test
 
 # Compiled outputs used by test targets
 TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
diff --git a/tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c b/tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c
new file mode 100644
index 000000000000..1a6a09791ac3
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023, Google LLC.
+ *
+ * Tests for the K7_HWCR MSR.
+ */
+
+#define _GNU_SOURCE /* for program_invocation_short_name */
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "vmx.h"
+
+void test_hwcr_bit(struct kvm_vcpu *vcpu, unsigned int bit)
+{
+	const unsigned long long ignored = BIT_ULL(3) | BIT_ULL(6) | BIT_ULL(8);
+	const unsigned long long valid = BIT_ULL(18) | BIT_ULL(24);
+	const unsigned long long legal = ignored | valid;
+	uint64_t val = BIT_ULL(bit);
+	uint64_t check;
+	int r;
+
+	r = _vcpu_set_msr(vcpu, MSR_K7_HWCR, val);
+	TEST_ASSERT((r == 1 && (val & legal)) || (r == 0 && !(val & legal)),
+		    "Unexpected result (%d) when setting HWCR[bit %u]", r, bit);
+	check =	vcpu_get_msr(vcpu, MSR_K7_HWCR);
+	if (val & valid) {
+		TEST_ASSERT(check == val,
+			    "Bit %u: unexpected HWCR %lx; expected %lx", bit,
+			    check, val);
+		vcpu_set_msr(vcpu, MSR_K7_HWCR, 0);
+	} else {
+		TEST_ASSERT(!check,
+			    "Bit %u: unexpected HWCR %lx; expected 0", bit,
+			    check);
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vm *vm;
+	struct kvm_vcpu *vcpu;
+	unsigned int bit;
+
+	vm = vm_create_with_one_vcpu(&vcpu, NULL);
+
+	for (bit = 0; bit < BITS_PER_LONG; bit++)
+		test_hwcr_bit(vcpu, bit);
+
+	kvm_vm_free(vm);
+}
-- 
2.42.0.582.g8ccd20d70d-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24]
  2023-09-28 18:51 ` [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24] Jim Mattson
@ 2023-09-29  0:56   ` Sean Christopherson
  2023-09-29 13:18     ` Jim Mattson
  0 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2023-09-29  0:56 UTC (permalink / raw)
  To: Jim Mattson; +Cc: kvm, 'Paolo Bonzini '

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
-- 


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24]
  2023-09-29  0:56   ` Sean Christopherson
@ 2023-09-29 13:18     ` Jim Mattson
  2023-09-29 16:28       ` Sean Christopherson
  0 siblings, 1 reply; 8+ messages in thread
From: Jim Mattson @ 2023-09-29 13:18 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, Paolo Bonzini

On Thu, Sep 28, 2023 at 5:56 PM Sean Christopherson <seanjc@google.com> wrote:
>
> 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.
> +                */

The whining is only about TscFreqSel. KVM actually supports the
functionality of McStatusWrEn (i.e. allows the guest to write to the
MCi_STATUS MSRs).

How about...

/*
* Allow McStatusWrEn and TscFreqSel. (Linux guests from v3.2 through
* at least v6.6 whine if TscFreqSel is clear, depending on F/M/S.
*/

> +               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 */

This isn't true. TscFreqSel is not architectural at all. On Family
10h, per https://www.amd.com/content/dam/amd/en/documents/archived-tech-docs/programmer-references/31116.pdf,
it was R/W and powered on as 0. In Family 15h, one of the "changes
relative to Family 10H Revision D processors," per
https://www.amd.com/content/dam/amd/en/documents/archived-tech-docs/programmer-references/42301_15h_Mod_00h-0Fh_BKDG.pdf,
was:

• MSRC001_0015 [Hardware Configuration (HWCR)]:
  • Dropped TscFreqSel; TSC can no longer be selected to run at NB P0-state.

Despite the "Dropped" above, that same document later describes
HWCR[bit 24] as follows:

TscFreqSel: TSC frequency select. Read-only. Reset: 1. 1=The TSC
increments at the P0 frequency

So, perhaps this block of code can just be dropped? Who really cares
if the guest changes the value (unless it goes on to kexec a new
kernel, which will whine about the bit being clear)?

> +               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
> --
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24]
  2023-09-29 13:18     ` Jim Mattson
@ 2023-09-29 16:28       ` Sean Christopherson
  0 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2023-09-29 16:28 UTC (permalink / raw)
  To: Jim Mattson; +Cc: kvm, Paolo Bonzini

On Fri, Sep 29, 2023, Jim Mattson wrote:
> On Thu, Sep 28, 2023 at 5:56 PM Sean Christopherson <seanjc@google.com> wrote:
> > ---
> >  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.
> > +                */
> 
> The whining is only about TscFreqSel. KVM actually supports the
> functionality of McStatusWrEn (i.e. allows the guest to write to the
> MCi_STATUS MSRs).

Ugh, I missed the usage can_set_mci_status().  Stupid open coded bit numbers.

> How about...
> 
> /*
> * Allow McStatusWrEn and TscFreqSel. (Linux guests from v3.2 through
> * at least v6.6 whine if TscFreqSel is clear, depending on F/M/S.
> */
> 
> > +               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 */
> 
> This isn't true. TscFreqSel is not architectural at all.

Doh, right, the whole source of this mess is the uarch specific nature of the MSR.

> On Family
> 10h, per https://www.amd.com/content/dam/amd/en/documents/archived-tech-docs/programmer-references/31116.pdf,
> it was R/W and powered on as 0. In Family 15h, one of the "changes
> relative to Family 10H Revision D processors," per
> https://www.amd.com/content/dam/amd/en/documents/archived-tech-docs/programmer-references/42301_15h_Mod_00h-0Fh_BKDG.pdf,
> was:
> 
> • MSRC001_0015 [Hardware Configuration (HWCR)]:
>   • Dropped TscFreqSel; TSC can no longer be selected to run at NB P0-state.
> 
> Despite the "Dropped" above, that same document later describes
> HWCR[bit 24] as follows:
> 
> TscFreqSel: TSC frequency select. Read-only. Reset: 1. 1=The TSC
> increments at the P0 frequency
> 
> So, perhaps this block of code can just be dropped? Who really cares
> if the guest changes the value (unless it goes on to kexec a new
> kernel, which will whine about the bit being clear)?

Works for me.  If userspace wants to precisely emulate model specific behavior,
then userspace can darn well intercept writes to the MSR.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 3/3] KVM: selftests: Test behavior of HWCR
  2023-09-28 18:51 ` [PATCH v3 3/3] KVM: selftests: Test behavior of HWCR Jim Mattson
@ 2023-09-29 17:06   ` Sean Christopherson
  0 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2023-09-29 17:06 UTC (permalink / raw)
  To: Jim Mattson; +Cc: kvm, 'Paolo Bonzini '

On Thu, Sep 28, 2023, Jim Mattson wrote:
> Verify the following:
> * Attempts to set bits 3, 6, or 8 are ignored
> * Bits 18 and 24 are the only bits that can be set
> * Any bit that can be set can also be cleared
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |  1 +
>  .../selftests/kvm/x86_64/hwcr_msr_test.c      | 52 +++++++++++++++++++
>  2 files changed, 53 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index a3bb36fb3cfc..6b0219ca65eb 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -135,6 +135,7 @@ TEST_GEN_PROGS_x86_64 += set_memory_region_test
>  TEST_GEN_PROGS_x86_64 += steal_time
>  TEST_GEN_PROGS_x86_64 += kvm_binary_stats_test
>  TEST_GEN_PROGS_x86_64 += system_counter_offset_test
> +TEST_GEN_PROGS_x86_64 += x86_64/hwcr_msr_test

Please place this with the other x86_64 specific tests.

Somewhat of a forward looking question, what do folks think about adding an
"msr_tests" subdirectory?  I really want to effectively replace KUT's msr.c test
with a supserset of functionality in selftests, e.g. KUT can't test userspace
writes.  But I don't want to end up with a single massive msr_test.c.  If we add
a subdirectory, then I think that would make it easier to share "private" APIs
and infrastructure without creating a giant monolithic test.

>  # Compiled outputs used by test targets
>  TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
> diff --git a/tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c b/tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c
> new file mode 100644
> index 000000000000..1a6a09791ac3
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/hwcr_msr_test.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023, Google LLC.
> + *
> + * Tests for the K7_HWCR MSR.
> + */
> +
> +#define _GNU_SOURCE /* for program_invocation_short_name */
> +#include <sys/ioctl.h>
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "vmx.h"
> +
> +void test_hwcr_bit(struct kvm_vcpu *vcpu, unsigned int bit)
> +{
> +	const unsigned long long ignored = BIT_ULL(3) | BIT_ULL(6) | BIT_ULL(8);

uint64_t instead of "unsigned long long".

> +	const unsigned long long valid = BIT_ULL(18) | BIT_ULL(24);
> +	const unsigned long long legal = ignored | valid;
> +	uint64_t val = BIT_ULL(bit);
> +	uint64_t check;
> +	int r;
> +
> +	r = _vcpu_set_msr(vcpu, MSR_K7_HWCR, val);
> +	TEST_ASSERT((r == 1 && (val & legal)) || (r == 0 && !(val & legal)),
> +		    "Unexpected result (%d) when setting HWCR[bit %u]", r, bit);
> +	check =	vcpu_get_msr(vcpu, MSR_K7_HWCR);
> +	if (val & valid) {
> +		TEST_ASSERT(check == val,
> +			    "Bit %u: unexpected HWCR %lx; expected %lx", bit,
> +			    check, val);
> +		vcpu_set_msr(vcpu, MSR_K7_HWCR, 0);
> +	} else {
> +		TEST_ASSERT(!check,
> +			    "Bit %u: unexpected HWCR %lx; expected 0", bit,
> +			    check);
> +	}
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct kvm_vm *vm;
> +	struct kvm_vcpu *vcpu;
> +	unsigned int bit;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, NULL);
> +
> +	for (bit = 0; bit < BITS_PER_LONG; bit++)
> +		test_hwcr_bit(vcpu, bit);
> +
> +	kvm_vm_free(vm);
> +}
> -- 
> 2.42.0.582.g8ccd20d70d-goog
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-09-29 17:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-28 18:51 KVM: x86: Update HWCR virtualization Jim Mattson
2023-09-28 18:51 ` [PATCH v3 1/3] KVM: x86: Allow HWCR.McStatusWrEn to be cleared once set Jim Mattson
2023-09-28 18:51 ` [PATCH v3 2/3] KVM: x86: Virtualize HWCR.TscFreqSel[bit 24] Jim Mattson
2023-09-29  0:56   ` Sean Christopherson
2023-09-29 13:18     ` Jim Mattson
2023-09-29 16:28       ` Sean Christopherson
2023-09-28 18:51 ` [PATCH v3 3/3] KVM: selftests: Test behavior of HWCR Jim Mattson
2023-09-29 17:06   ` Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).