[PATCH 0/2] nVMX: Ensure host's PAT stays sane

[PATCH 0/2] nVMX: Ensure host's PAT stays sane

[Sean Christopherson]

Fix a bug(s) where test_load_host_pat clobbers PAT and causes all
subsequent tests to effectively run with all memory mapped as UC.  Xiangfei
first noticed that this caused rdtsc_vmexit_diff_test to fail, but it can
also lead to timeouts if more tests are added, i.e. if more stuff runs after
test_load_host_pat.

Sean Christopherson (2):
  nVMX: Ensure host's PAT is loaded at the end of all VMX tests
  nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR

 x86/vmx.c       |  4 +++-
 x86/vmx_tests.c | 10 +++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)


base-commit: 765b349b6c4b1c06473dfea548a15f799e0fbc2b
-- 
2.45.1.467.gbab1589fc0-goog

[PATCH 1/2] nVMX: Ensure host's PAT is loaded at the end of all VMX tests

[Sean Christopherson]

Load the host's original PAT on VM-Exit by default in all VMX tests, and
manually write PAT with the original value in the test that verifies all
legal PAT values can be loaded via HOST_PAT.  Failure to (re)load the
correct host PAT results in all tests that run after test_load_host_pat()
using UC memtype for all memory.

Opportunistically fix a message goof for the ENT_LOAD_PAT=0 case.

Reported-by: Xiangfei Ma <xiangfeix.ma@intel.com>
Cc: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/vmx.c       | 4 +++-
 x86/vmx_tests.c | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index 9f08c096..c803eaa6 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -1126,6 +1126,8 @@ static void init_vmcs_host(void)
 	vmcs_write(HOST_CR4, read_cr4());
 	vmcs_write(HOST_SYSENTER_EIP, (u64)(&entry_sysenter));
 	vmcs_write(HOST_SYSENTER_CS,  KERNEL_CS);
+	if (ctrl_exit_rev.clr & EXI_LOAD_PAT)
+		vmcs_write(HOST_PAT, rdmsr(MSR_IA32_CR_PAT));
 
 	/* 26.2.3 */
 	vmcs_write(HOST_SEL_CS, KERNEL_CS);
@@ -1247,7 +1249,7 @@ int init_vmcs(struct vmcs **vmcs)
 	/* All settings to pin/exit/enter/cpu
 	   control fields should be placed here */
 	ctrl_pin |= PIN_EXTINT | PIN_NMI | PIN_VIRT_NMI;
-	ctrl_exit = EXI_LOAD_EFER | EXI_HOST_64;
+	ctrl_exit = EXI_LOAD_EFER | EXI_HOST_64 | EXI_LOAD_PAT;
 	ctrl_enter = (ENT_LOAD_EFER | ENT_GUEST_64);
 	/* DIsable IO instruction VMEXIT now */
 	ctrl_cpu[0] &= (~(CPU_IO | CPU_IO_BITMAP));
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 22e8812a..8a17dd90 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7233,6 +7233,7 @@ static void test_guest_efer(void)
 static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
 		     u64 ctrl_bit)
 {
+	u64 pat_msr_saved = rdmsr(MSR_IA32_CR_PAT);
 	u32 ctrl_saved = vmcs_read(ctrl_field);
 	u64 pat_saved = vmcs_read(field);
 	u64 i, val;
@@ -7252,7 +7253,7 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
 				report_prefix_pop();
 
 			} else {	// GUEST_PAT
-				test_guest_state("ENT_LOAD_PAT enabled", false,
+				test_guest_state("ENT_LOAD_PAT disabled", false,
 						 val, "GUEST_PAT");
 			}
 		}
@@ -7274,6 +7275,8 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
 					error = 0;
 
 				test_vmx_vmlaunch(error);
+				wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
+
 				report_prefix_pop();
 
 			} else {	// GUEST_PAT
-- 
2.45.1.467.gbab1589fc0-goog

Re: [PATCH 1/2] nVMX: Ensure host's PAT is loaded at the end of all VMX tests

[Yan Zhao]

On Wed, Jun 05, 2024 at 03:45:26PM -0700, Sean Christopherson wrote:
> Load the host's original PAT on VM-Exit by default in all VMX tests, and
> manually write PAT with the original value in the test that verifies all
> legal PAT values can be loaded via HOST_PAT.  Failure to (re)load the
> correct host PAT results in all tests that run after test_load_host_pat()
> using UC memtype for all memory.
> 
> Opportunistically fix a message goof for the ENT_LOAD_PAT=0 case.
> 
> Reported-by: Xiangfei Ma <xiangfeix.ma@intel.com>
> Cc: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  x86/vmx.c       | 4 +++-
>  x86/vmx_tests.c | 5 ++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 9f08c096..c803eaa6 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -1126,6 +1126,8 @@ static void init_vmcs_host(void)
>  	vmcs_write(HOST_CR4, read_cr4());
>  	vmcs_write(HOST_SYSENTER_EIP, (u64)(&entry_sysenter));
>  	vmcs_write(HOST_SYSENTER_CS,  KERNEL_CS);
> +	if (ctrl_exit_rev.clr & EXI_LOAD_PAT)
> +		vmcs_write(HOST_PAT, rdmsr(MSR_IA32_CR_PAT));
>  
>  	/* 26.2.3 */
>  	vmcs_write(HOST_SEL_CS, KERNEL_CS);
> @@ -1247,7 +1249,7 @@ int init_vmcs(struct vmcs **vmcs)
>  	/* All settings to pin/exit/enter/cpu
>  	   control fields should be placed here */
>  	ctrl_pin |= PIN_EXTINT | PIN_NMI | PIN_VIRT_NMI;
> -	ctrl_exit = EXI_LOAD_EFER | EXI_HOST_64;
> +	ctrl_exit = EXI_LOAD_EFER | EXI_HOST_64 | EXI_LOAD_PAT;
>  	ctrl_enter = (ENT_LOAD_EFER | ENT_GUEST_64);
>  	/* DIsable IO instruction VMEXIT now */
>  	ctrl_cpu[0] &= (~(CPU_IO | CPU_IO_BITMAP));
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 22e8812a..8a17dd90 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -7233,6 +7233,7 @@ static void test_guest_efer(void)
>  static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
>  		     u64 ctrl_bit)
>  {
> +	u64 pat_msr_saved = rdmsr(MSR_IA32_CR_PAT);
>  	u32 ctrl_saved = vmcs_read(ctrl_field);
>  	u64 pat_saved = vmcs_read(field);
>  	u64 i, val;
> @@ -7252,7 +7253,7 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
>  				report_prefix_pop();
>  
>  			} else {	// GUEST_PAT
> -				test_guest_state("ENT_LOAD_PAT enabled", false,
> +				test_guest_state("ENT_LOAD_PAT disabled", false,
>  						 val, "GUEST_PAT");
>  			}
>  		}
> @@ -7274,6 +7275,8 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
>  					error = 0;
>  
>  				test_vmx_vmlaunch(error);
> +				wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
> +
>  				report_prefix_pop();
>  
>  			} else {	// GUEST_PAT

Is it possible that ENT_LOAD_PAT of GUEST_PAT is tested when there's no support of
EXI_LOAD_PAT of HOST_PAT?
Then
	wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
is also required in this case?

> -- 
> 2.45.1.467.gbab1589fc0-goog
> 

Re: [PATCH 1/2] nVMX: Ensure host's PAT is loaded at the end of all VMX tests

[Sean Christopherson]

On Tue, Jun 11, 2024, Yan Zhao wrote:
> On Wed, Jun 05, 2024 at 03:45:26PM -0700, Sean Christopherson wrote:
> > @@ -7274,6 +7275,8 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
> >  					error = 0;
> >  
> >  				test_vmx_vmlaunch(error);
> > +				wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
> > +
> >  				report_prefix_pop();
> >  
> >  			} else {	// GUEST_PAT
> 
> Is it possible that ENT_LOAD_PAT of GUEST_PAT is tested when there's no support of
> EXI_LOAD_PAT of HOST_PAT?
> Then
> 	wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
> is also required in this case?

Heh, in theory, yeah, a nested setup could create such a monstrosity.  It's easy
enough to handle, so I guess why not?  Though I'm tempted to assert instead,
because practically speaking this code will never be hit, and thus never validated.

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 2063ee90..ffe7064c 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7288,6 +7288,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
                                error = (i == 0x2 || i == 0x3 || i >= 0x8);
                                test_guest_state("ENT_LOAD_PAT enabled", !!error,
                                                 val, "GUEST_PAT");
+
+                               if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT))
+                                       wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
                        }
 
                }

Re: [PATCH 1/2] nVMX: Ensure host's PAT is loaded at the end of all VMX tests

[Yan Zhao]

On Mon, Jun 10, 2024 at 06:47:55PM -0700, Sean Christopherson wrote:
> On Tue, Jun 11, 2024, Yan Zhao wrote:
> > On Wed, Jun 05, 2024 at 03:45:26PM -0700, Sean Christopherson wrote:
> > > @@ -7274,6 +7275,8 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
> > >  					error = 0;
> > >  
> > >  				test_vmx_vmlaunch(error);
> > > +				wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
> > > +
> > >  				report_prefix_pop();
> > >  
> > >  			} else {	// GUEST_PAT
> > 
> > Is it possible that ENT_LOAD_PAT of GUEST_PAT is tested when there's no support of
> > EXI_LOAD_PAT of HOST_PAT?
> > Then
> > 	wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
> > is also required in this case?
> 
> Heh, in theory, yeah, a nested setup could create such a monstrosity.  It's easy
> enough to handle, so I guess why not?  Though I'm tempted to assert instead,
> because practically speaking this code will never be hit, and thus never validated.
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 2063ee90..ffe7064c 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -7288,6 +7288,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
>                                 error = (i == 0x2 || i == 0x3 || i >= 0x8);
>                                 test_guest_state("ENT_LOAD_PAT enabled", !!error,
>                                                  val, "GUEST_PAT");
> +
> +                               if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT))
> +                                       wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
>                         }
>  
>                 }
Tested passed with EXI_LOAD_PAT intentionally cleared in ctrl_exit_rev.clr.

Reviewed-by: Yan Zhao <yan.y.zhao@intel.com>

[PATCH 2/2] nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR

[Sean Christopherson]

Check that the PAT MSR is actually loaded with vmcs.HOST_PAT on VM-Exit
in the testcase that shoves all legal PAT values into HOST_PAT.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/vmx_tests.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 8a17dd90..ef7e0ea9 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7275,6 +7275,11 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
 					error = 0;
 
 				test_vmx_vmlaunch(error);
+
+				if (!error)
+					report(rdmsr(MSR_IA32_CR_PAT) == val,
+					       "Expected PAT = 0x%lx, got 0x%lx",
+						val, rdmsr(MSR_IA32_CR_PAT));
 				wrmsr(MSR_IA32_CR_PAT, pat_msr_saved);
 
 				report_prefix_pop();
-- 
2.45.1.467.gbab1589fc0-goog

Re: [PATCH 0/2] nVMX: Ensure host's PAT stays sane

[Sean Christopherson]

On Wed, Jun 05, 2024, Sean Christopherson wrote:
> Fix a bug(s) where test_load_host_pat clobbers PAT and causes all
> subsequent tests to effectively run with all memory mapped as UC.  Xiangfei
> first noticed that this caused rdtsc_vmexit_diff_test to fail, but it can
> also lead to timeouts if more tests are added, i.e. if more stuff runs after
> test_load_host_pat.
> 
> Sean Christopherson (2):
>   nVMX: Ensure host's PAT is loaded at the end of all VMX tests
>   nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR
> 
>  x86/vmx.c       |  4 +++-
>  x86/vmx_tests.c | 10 +++++++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)

Doh, this is for kvm-unit-tests.  Been a while since I sent KUT patches...

Re: [PATCH 0/2] nVMX: Ensure host's PAT stays sane

[Sean Christopherson]

On Wed, 05 Jun 2024 15:45:25 -0700, Sean Christopherson wrote:
> Fix a bug(s) where test_load_host_pat clobbers PAT and causes all
> subsequent tests to effectively run with all memory mapped as UC.  Xiangfei
> first noticed that this caused rdtsc_vmexit_diff_test to fail, but it can
> also lead to timeouts if more tests are added, i.e. if more stuff runs after
> test_load_host_pat.
> 
> Sean Christopherson (2):
>   nVMX: Ensure host's PAT is loaded at the end of all VMX tests
>   nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR
> 
> [...]

Applied to kvm-x86 next, thanks!

[1/2] nVMX: Ensure host's PAT is loaded at the end of all VMX tests
      https://github.com/kvm-x86/kvm-unit-tests/commit/8cbb8d3cbca9
[2/2] nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR
      https://github.com/kvm-x86/kvm-unit-tests/commit/d31433807f2b

--
https://github.com/kvm-x86/kvm-unit-tests/tree/next

Re: [PATCH 0/2] nVMX: Ensure host's PAT stays sane

[Yan Zhao]

On Tue, Jun 11, 2024 at 06:18:40PM -0700, Sean Christopherson wrote:
> On Wed, 05 Jun 2024 15:45:25 -0700, Sean Christopherson wrote:
> > Fix a bug(s) where test_load_host_pat clobbers PAT and causes all
> > subsequent tests to effectively run with all memory mapped as UC.  Xiangfei
> > first noticed that this caused rdtsc_vmexit_diff_test to fail, but it can
> > also lead to timeouts if more tests are added, i.e. if more stuff runs after
> > test_load_host_pat.
> > 
> > Sean Christopherson (2):
> >   nVMX: Ensure host's PAT is loaded at the end of all VMX tests
> >   nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR
> > 
> > [...]
> 
> Applied to kvm-x86 next, thanks!
> 
> [1/2] nVMX: Ensure host's PAT is loaded at the end of all VMX tests
>       https://github.com/kvm-x86/kvm-unit-tests/commit/8cbb8d3cbca9
This is also good, though I thought you would fix the theoretical one as
https://lore.kernel.org/all/ZmesywdpFG3qDIkZ@google.com/ :)

> [2/2] nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR
>       https://github.com/kvm-x86/kvm-unit-tests/commit/d31433807f2b
> 
> --
> https://github.com/kvm-x86/kvm-unit-tests/tree/next

Re: [PATCH 0/2] nVMX: Ensure host's PAT stays sane

[Sean Christopherson]

On Wed, Jun 12, 2024, Yan Zhao wrote:
> On Tue, Jun 11, 2024 at 06:18:40PM -0700, Sean Christopherson wrote:
> > On Wed, 05 Jun 2024 15:45:25 -0700, Sean Christopherson wrote:
> > > Fix a bug(s) where test_load_host_pat clobbers PAT and causes all
> > > subsequent tests to effectively run with all memory mapped as UC.  Xiangfei
> > > first noticed that this caused rdtsc_vmexit_diff_test to fail, but it can
> > > also lead to timeouts if more tests are added, i.e. if more stuff runs after
> > > test_load_host_pat.
> > > 
> > > Sean Christopherson (2):
> > >   nVMX: Ensure host's PAT is loaded at the end of all VMX tests
> > >   nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR
> > > 
> > > [...]
> > 
> > Applied to kvm-x86 next, thanks!
> > 
> > [1/2] nVMX: Ensure host's PAT is loaded at the end of all VMX tests
> >       https://github.com/kvm-x86/kvm-unit-tests/commit/8cbb8d3cbca9
> This is also good, though I thought you would fix the theoretical one as
> https://lore.kernel.org/all/ZmesywdpFG3qDIkZ@google.com/ :)

Doh, I did squash that, I just forgot to update the "thanks" email with the
correct hashes.

[1/2] nVMX: Ensure host's PAT is loaded at the end of all VMX tests
      https://github.com/kvm-x86/kvm-unit-tests/commit/184ee0d5
[2/2] nVMX: Verify KVM actually loads the value in HOST_PAT into the PAT MSR
      https://github.com/kvm-x86/kvm-unit-tests/commit/ee1d79c3