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
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 <[email protected]>
Cc: Yan Zhao <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
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
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 <[email protected]>
---
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
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...
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 <[email protected]>
> Cc: Yan Zhao <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> 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
>
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);
}
}
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 <[email protected]>
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
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/[email protected]/ :)
> [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
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/[email protected]/ :)
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