* [PATCH] kvm-unit-test: nVMX: Fix 95d6d2c32288 ("nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests")
@ 2019-08-12 21:11 Krish Sadhukhan
2019-09-17 14:52 ` Paolo Bonzini
0 siblings, 1 reply; 2+ messages in thread
From: Krish Sadhukhan @ 2019-08-12 21:11 UTC (permalink / raw)
To: kvm; +Cc: rkrcmar, pbonzini, jmattson
Commit 95d6d2c32288 added a test for the Segment Selector VMCS field. That test
sets the "host address-space size" VM-exit control to zero and as a result, on
VM-exit the guest exits as 32-bit. Since vmx tests are 64-bit, this results in
a hardware error.
This patch also cleans up a few other areas in commit 95d6d2c32288, including
replacing make_non_canonical() with NONCANONICAL.
Reported-by: Nadav Amit <nadav.amit@gmail.com>
Signed-off-by: Krish Sadhukhan <kris.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
lib/x86/processor.h | 5 ---
x86/vmx_tests.c | 76 ++++++++++++++++++++-------------------------
2 files changed, 33 insertions(+), 48 deletions(-)
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 8b8bb7a..4fef0bc 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -461,11 +461,6 @@ static inline void write_pkru(u32 pkru)
: : "a" (eax), "c" (ecx), "d" (edx));
}
-static inline u64 make_non_canonical(u64 addr)
-{
- return (addr | 1ull << 48);
-}
-
static inline bool is_canonical(u64 addr)
{
return (s64)(addr << 16) >> 16 == addr;
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 8ad2674..e115e48 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -6952,15 +6952,14 @@ static void test_load_host_pat(void)
}
/*
- * Test a value for the given VMCS field.
- *
- * "field" - VMCS field
- * "field_name" - string name of VMCS field
- * "bit_start" - starting bit
- * "bit_end" - ending bit
- * "val" - value that the bit range must or must not contain
- * "valid_val" - whether value given in 'val' must be valid or not
- * "error" - expected VMCS error when vmentry fails for an invalid value
+ * test_vmcs_field - test a value for the given VMCS field
+ * @field: VMCS field
+ * @field_name: string name of VMCS field
+ * @bit_start: starting bit
+ * @bit_end: ending bit
+ * @val: value that the bit range must or must not contain
+ * @valid_val: whether value given in 'val' must be valid or not
+ * @error: expected VMCS error when vmentry fails for an invalid value
*/
static void test_vmcs_field(u64 field, const char *field_name, u32 bit_start,
u32 bit_end, u64 val, bool valid_val, u32 error)
@@ -7004,16 +7003,14 @@ static void test_vmcs_field(u64 field, const char *field_name, u32 bit_start,
static void test_canonical(u64 field, const char * field_name)
{
u64 addr_saved = vmcs_read(field);
- u64 addr = addr_saved;
- report_prefix_pushf("%s %lx", field_name, addr);
- if (is_canonical(addr)) {
+ report_prefix_pushf("%s %lx", field_name, addr_saved);
+ if (is_canonical(addr_saved)) {
test_vmx_vmlaunch(0, false);
report_prefix_pop();
- addr = make_non_canonical(addr);
- vmcs_write(field, addr);
- report_prefix_pushf("%s %lx", field_name, addr);
+ vmcs_write(field, NONCANONICAL);
+ report_prefix_pushf("%s %llx", field_name, NONCANONICAL);
test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
false);
@@ -7025,6 +7022,14 @@ static void test_canonical(u64 field, const char * field_name)
report_prefix_pop();
}
+#define TEST_RPL_TI_FLAGS(reg, name) \
+ test_vmcs_field(reg, name, 0, 2, 0x0, true, \
+ VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+
+#define TEST_CS_TR_FLAGS(reg, name) \
+ test_vmcs_field(reg, name, 3, 15, 0x0000, false, \
+ VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+
/*
* 1. In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
* RPL (bits 1:0) and the TI flag (bit 2) must be 0.
@@ -7036,34 +7041,24 @@ static void test_canonical(u64 field, const char * field_name)
*/
static void test_host_segment_regs(void)
{
- u32 exit_ctrl_saved = vmcs_read(EXI_CONTROLS);
u16 selector_saved;
/*
* Test RPL and TI flags
*/
- test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 0, 2, 0x0, true,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
- test_vmcs_field(HOST_SEL_SS, "HOST_SEL_SS", 0, 2, 0x0, true,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
- test_vmcs_field(HOST_SEL_DS, "HOST_SEL_DS", 0, 2, 0x0, true,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
- test_vmcs_field(HOST_SEL_ES, "HOST_SEL_ES", 0, 2, 0x0, true,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
- test_vmcs_field(HOST_SEL_FS, "HOST_SEL_FS", 0, 2, 0x0, true,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
- test_vmcs_field(HOST_SEL_GS, "HOST_SEL_GS", 0, 2, 0x0, true,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
- test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 0, 2, 0x0, true,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+ TEST_RPL_TI_FLAGS(HOST_SEL_CS, "HOST_SEL_CS");
+ TEST_RPL_TI_FLAGS(HOST_SEL_SS, "HOST_SEL_SS");
+ TEST_RPL_TI_FLAGS(HOST_SEL_DS, "HOST_SEL_DS");
+ TEST_RPL_TI_FLAGS(HOST_SEL_ES, "HOST_SEL_ES");
+ TEST_RPL_TI_FLAGS(HOST_SEL_FS, "HOST_SEL_FS");
+ TEST_RPL_TI_FLAGS(HOST_SEL_GS, "HOST_SEL_GS");
+ TEST_RPL_TI_FLAGS(HOST_SEL_TR, "HOST_SEL_TR");
/*
* Test that CS and TR fields can not be 0x0000
*/
- test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 3, 15, 0x0000, false,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
- test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 3, 15, 0x0000, false,
- VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+ TEST_CS_TR_FLAGS(HOST_SEL_CS, "HOST_SEL_CS");
+ TEST_CS_TR_FLAGS(HOST_SEL_TR, "HOST_SEL_TR");
/*
* SS field can not be 0x0000 if "host address-space size" VM-exit
@@ -7071,20 +7066,15 @@ static void test_host_segment_regs(void)
*/
selector_saved = vmcs_read(HOST_SEL_SS);
vmcs_write(HOST_SEL_SS, 0);
- if (exit_ctrl_saved & EXI_HOST_64) {
- report_prefix_pushf("HOST_SEL_SS 0");
+ report_prefix_pushf("HOST_SEL_SS 0");
+ if (vmcs_read(EXI_CONTROLS) & EXI_HOST_64) {
test_vmx_vmlaunch(0, false);
- report_prefix_pop();
-
- vmcs_write(EXI_CONTROLS, exit_ctrl_saved & ~EXI_HOST_64);
+ } else {
+ test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false);
}
-
- report_prefix_pushf("HOST_SEL_SS 0");
- test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false);
report_prefix_pop();
vmcs_write(HOST_SEL_SS, selector_saved);
- vmcs_write(EXI_CONTROLS, exit_ctrl_saved);
#ifdef __x86_64__
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] kvm-unit-test: nVMX: Fix 95d6d2c32288 ("nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests")
2019-08-12 21:11 [PATCH] kvm-unit-test: nVMX: Fix 95d6d2c32288 ("nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests") Krish Sadhukhan
@ 2019-09-17 14:52 ` Paolo Bonzini
0 siblings, 0 replies; 2+ messages in thread
From: Paolo Bonzini @ 2019-09-17 14:52 UTC (permalink / raw)
To: Krish Sadhukhan, kvm; +Cc: rkrcmar, jmattson
On 12/08/19 23:11, Krish Sadhukhan wrote:
> Commit 95d6d2c32288 added a test for the Segment Selector VMCS field. That test
> sets the "host address-space size" VM-exit control to zero and as a result, on
> VM-exit the guest exits as 32-bit. Since vmx tests are 64-bit, this results in
> a hardware error.
> This patch also cleans up a few other areas in commit 95d6d2c32288, including
> replacing make_non_canonical() with NONCANONICAL.
>
> Reported-by: Nadav Amit <nadav.amit@gmail.com>
> Signed-off-by: Krish Sadhukhan <kris.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
> lib/x86/processor.h | 5 ---
> x86/vmx_tests.c | 76 ++++++++++++++++++++-------------------------
> 2 files changed, 33 insertions(+), 48 deletions(-)
>
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 8b8bb7a..4fef0bc 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -461,11 +461,6 @@ static inline void write_pkru(u32 pkru)
> : : "a" (eax), "c" (ecx), "d" (edx));
> }
>
> -static inline u64 make_non_canonical(u64 addr)
> -{
> - return (addr | 1ull << 48);
> -}
> -
> static inline bool is_canonical(u64 addr)
> {
> return (s64)(addr << 16) >> 16 == addr;
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 8ad2674..e115e48 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -6952,15 +6952,14 @@ static void test_load_host_pat(void)
> }
>
> /*
> - * Test a value for the given VMCS field.
> - *
> - * "field" - VMCS field
> - * "field_name" - string name of VMCS field
> - * "bit_start" - starting bit
> - * "bit_end" - ending bit
> - * "val" - value that the bit range must or must not contain
> - * "valid_val" - whether value given in 'val' must be valid or not
> - * "error" - expected VMCS error when vmentry fails for an invalid value
> + * test_vmcs_field - test a value for the given VMCS field
> + * @field: VMCS field
> + * @field_name: string name of VMCS field
> + * @bit_start: starting bit
> + * @bit_end: ending bit
> + * @val: value that the bit range must or must not contain
> + * @valid_val: whether value given in 'val' must be valid or not
> + * @error: expected VMCS error when vmentry fails for an invalid value
> */
> static void test_vmcs_field(u64 field, const char *field_name, u32 bit_start,
> u32 bit_end, u64 val, bool valid_val, u32 error)
> @@ -7004,16 +7003,14 @@ static void test_vmcs_field(u64 field, const char *field_name, u32 bit_start,
> static void test_canonical(u64 field, const char * field_name)
> {
> u64 addr_saved = vmcs_read(field);
> - u64 addr = addr_saved;
>
> - report_prefix_pushf("%s %lx", field_name, addr);
> - if (is_canonical(addr)) {
> + report_prefix_pushf("%s %lx", field_name, addr_saved);
> + if (is_canonical(addr_saved)) {
> test_vmx_vmlaunch(0, false);
> report_prefix_pop();
>
> - addr = make_non_canonical(addr);
> - vmcs_write(field, addr);
> - report_prefix_pushf("%s %lx", field_name, addr);
> + vmcs_write(field, NONCANONICAL);
> + report_prefix_pushf("%s %llx", field_name, NONCANONICAL);
> test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
> false);
>
> @@ -7025,6 +7022,14 @@ static void test_canonical(u64 field, const char * field_name)
> report_prefix_pop();
> }
>
> +#define TEST_RPL_TI_FLAGS(reg, name) \
> + test_vmcs_field(reg, name, 0, 2, 0x0, true, \
> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +
> +#define TEST_CS_TR_FLAGS(reg, name) \
> + test_vmcs_field(reg, name, 3, 15, 0x0000, false, \
> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +
> /*
> * 1. In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
> * RPL (bits 1:0) and the TI flag (bit 2) must be 0.
> @@ -7036,34 +7041,24 @@ static void test_canonical(u64 field, const char * field_name)
> */
> static void test_host_segment_regs(void)
> {
> - u32 exit_ctrl_saved = vmcs_read(EXI_CONTROLS);
> u16 selector_saved;
>
> /*
> * Test RPL and TI flags
> */
> - test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 0, 2, 0x0, true,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> - test_vmcs_field(HOST_SEL_SS, "HOST_SEL_SS", 0, 2, 0x0, true,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> - test_vmcs_field(HOST_SEL_DS, "HOST_SEL_DS", 0, 2, 0x0, true,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> - test_vmcs_field(HOST_SEL_ES, "HOST_SEL_ES", 0, 2, 0x0, true,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> - test_vmcs_field(HOST_SEL_FS, "HOST_SEL_FS", 0, 2, 0x0, true,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> - test_vmcs_field(HOST_SEL_GS, "HOST_SEL_GS", 0, 2, 0x0, true,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> - test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 0, 2, 0x0, true,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> + TEST_RPL_TI_FLAGS(HOST_SEL_CS, "HOST_SEL_CS");
> + TEST_RPL_TI_FLAGS(HOST_SEL_SS, "HOST_SEL_SS");
> + TEST_RPL_TI_FLAGS(HOST_SEL_DS, "HOST_SEL_DS");
> + TEST_RPL_TI_FLAGS(HOST_SEL_ES, "HOST_SEL_ES");
> + TEST_RPL_TI_FLAGS(HOST_SEL_FS, "HOST_SEL_FS");
> + TEST_RPL_TI_FLAGS(HOST_SEL_GS, "HOST_SEL_GS");
> + TEST_RPL_TI_FLAGS(HOST_SEL_TR, "HOST_SEL_TR");
>
> /*
> * Test that CS and TR fields can not be 0x0000
> */
> - test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 3, 15, 0x0000, false,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> - test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 3, 15, 0x0000, false,
> - VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> + TEST_CS_TR_FLAGS(HOST_SEL_CS, "HOST_SEL_CS");
> + TEST_CS_TR_FLAGS(HOST_SEL_TR, "HOST_SEL_TR");
>
> /*
> * SS field can not be 0x0000 if "host address-space size" VM-exit
> @@ -7071,20 +7066,15 @@ static void test_host_segment_regs(void)
> */
> selector_saved = vmcs_read(HOST_SEL_SS);
> vmcs_write(HOST_SEL_SS, 0);
> - if (exit_ctrl_saved & EXI_HOST_64) {
> - report_prefix_pushf("HOST_SEL_SS 0");
> + report_prefix_pushf("HOST_SEL_SS 0");
> + if (vmcs_read(EXI_CONTROLS) & EXI_HOST_64) {
> test_vmx_vmlaunch(0, false);
> - report_prefix_pop();
> -
> - vmcs_write(EXI_CONTROLS, exit_ctrl_saved & ~EXI_HOST_64);
> + } else {
> + test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false);
> }
> -
> - report_prefix_pushf("HOST_SEL_SS 0");
> - test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false);
> report_prefix_pop();
>
> vmcs_write(HOST_SEL_SS, selector_saved);
> - vmcs_write(EXI_CONTROLS, exit_ctrl_saved);
>
> #ifdef __x86_64__
> /*
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-09-17 14:52 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-12 21:11 [PATCH] kvm-unit-test: nVMX: Fix 95d6d2c32288 ("nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests") Krish Sadhukhan
2019-09-17 14:52 ` Paolo Bonzini
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).