* Re: [PATCH kvm-unit-tests] x86: do not overwrite bits 7 and 12 of MSR_IA32_MISC_ENABLE
2022-05-20 18:32 [PATCH kvm-unit-tests] x86: do not overwrite bits 7 and 12 of MSR_IA32_MISC_ENABLE Paolo Bonzini
@ 2022-06-16 18:30 ` Sean Christopherson
0 siblings, 0 replies; 2+ messages in thread
From: Sean Christopherson @ 2022-06-16 18:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, likexu
On Fri, May 20, 2022, Paolo Bonzini wrote:
> Bits 7 and 12 of MSR_IA32_MISC_ENABLE represent the configuration of the
> vPMU, and latest KVM does not allow the guest to modify them. Adjust
> kvm-unit-tests to avoid failures.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> x86/msr.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/x86/msr.c b/x86/msr.c
> index 44fbb3b..2eb928c 100644
> --- a/x86/msr.c
> +++ b/x86/msr.c
> @@ -19,6 +19,7 @@ struct msr_info {
> bool is_64bit_only;
> const char *name;
> unsigned long long value;
> + unsigned long long keep;
> };
>
>
> @@ -27,6 +28,8 @@ struct msr_info {
>
> #define MSR_TEST(msr, val, only64) \
> { .index = msr, .name = #msr, .value = val, .is_64bit_only = only64 }
> +#define MSR_TEST_RO_BITS(msr, val, only64, ro) \
Heh, I wrote pretty much this exact patch before I saw your version.
> + { .index = msr, .name = #msr, .value = val, .is_64bit_only = only64, .keep = ro }
What if we omit @only64 and hardcode it false, and add separate macros for the
"64-bit only" MSRS? Then there are fewer magic booleans in the code. Prep patch
at the bottom. With that, this becomes:
#define MSR_TEST_RO_BITS(msr, val, ro) __MSR_TEST(msr, val, false, ro)
> struct msr_info msr_info[] =
> {
> @@ -34,7 +37,8 @@ struct msr_info msr_info[] =
> MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul, false),
> MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul, false),
> // reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
> - MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889, false),
> + // read-only: 7, 12
> + MSR_TEST_RO_BITS(MSR_IA32_MISC_ENABLE, 0x400c50809, false, 0x1080),
Bit 11, "BTS Unavailable", is also read-only. I would also strongly prefer that
this use BIT(7) | BIT(11) | BIT(12). Maybe someday we can pull in msr-index.h...
> MSR_TEST(MSR_IA32_CR_PAT, 0x07070707, false),
> MSR_TEST(MSR_FS_BASE, addr_64, true),
> MSR_TEST(MSR_GS_BASE, addr_64, true),
> @@ -59,6 +63,8 @@ static void test_msr_rw(struct msr_info *msr, unsigned long long val)
> */
> if (msr->index == MSR_EFER)
> val |= orig;
> + else
> + val = (val & ~msr->keep) | (orig & msr->keep);
Use MSR_TEST_RO_BITS() for MSR_EFER and the special case goes away.
MSR_TEST_RO_BITS(MSR_EFER, EFER_SCE, ~EFER_SCE),
--
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 10 Jun 2022 12:44:01 -0700
Subject: [PATCH] x86/msr: Add dedicated macros to handle MSRs that are 64-bit
only
Add a separate macro for handling 64-bit only MSRs to minimize churn and
copy+paste in a future commit that will add support for read-only bits.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
x86/msr.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/x86/msr.c b/x86/msr.c
index 44fbb3b2..5f2ad8d6 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -25,24 +25,27 @@ struct msr_info {
#define addr_64 0x0000123456789abcULL
#define addr_ul (unsigned long)addr_64
-#define MSR_TEST(msr, val, only64) \
+#define __MSR_TEST(msr, val, only64) \
{ .index = msr, .name = #msr, .value = val, .is_64bit_only = only64 }
+#define MSR_TEST(msr, val) __MSR_TEST(msr, val, false)
+#define MSR_TEST_ONLY64(msr, val) __MSR_TEST(msr, val, true)
+
struct msr_info msr_info[] =
{
- MSR_TEST(MSR_IA32_SYSENTER_CS, 0x1234, false),
- MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul, false),
- MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul, false),
+ MSR_TEST(MSR_IA32_SYSENTER_CS, 0x1234),
+ MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul),
+ MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul),
// reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
- MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889, false),
- MSR_TEST(MSR_IA32_CR_PAT, 0x07070707, false),
- MSR_TEST(MSR_FS_BASE, addr_64, true),
- MSR_TEST(MSR_GS_BASE, addr_64, true),
- MSR_TEST(MSR_KERNEL_GS_BASE, addr_64, true),
- MSR_TEST(MSR_EFER, EFER_SCE, false),
- MSR_TEST(MSR_LSTAR, addr_64, true),
- MSR_TEST(MSR_CSTAR, addr_64, true),
- MSR_TEST(MSR_SYSCALL_MASK, 0xffffffff, true),
+ MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889),
+ MSR_TEST(MSR_IA32_CR_PAT, 0x07070707),
+ MSR_TEST_ONLY64(MSR_FS_BASE, addr_64),
+ MSR_TEST_ONLY64(MSR_GS_BASE, addr_64),
+ MSR_TEST_ONLY64(MSR_KERNEL_GS_BASE, addr_64),
+ MSR_TEST(MSR_EFER, EFER_SCE),
+ MSR_TEST_ONLY64(MSR_LSTAR, addr_64),
+ MSR_TEST_ONLY64(MSR_CSTAR, addr_64),
+ MSR_TEST_ONLY64(MSR_SYSCALL_MASK, 0xffffffff),
// MSR_IA32_DEBUGCTLMSR needs svm feature LBRV
// MSR_VM_HSAVE_PA only AMD host
};
base-commit: 2eed0bf1096077144cc3a0dd9974689487f9511a
--
^ permalink raw reply related [flat|nested] 2+ messages in thread