Re: [PATCH kvm-unit-tests] x86: do not overwrite bits 7 and 12 of MSR_IA32_MISC_ENABLE

[Sean Christopherson <seanjc@google.com>]

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