On Fri, Jun 22, 2018 at 4:48 PM Krish Sadhukhan
<krish.sadhukhan@xxxxxxxxxx> wrote:
>
>
>
> On 06/20/2018 05:30 PM, Marc Orr wrote:
> > This patch adds a test for the prereq checks done as a part of a nested
> > VM launch related to event injection.
> >
> > Signed-off-by: Marc Orr <marcorr@xxxxxxxxxx>
> > ---
> > Changelog since v1:
> > - Add positive test case for interrupt type != INTR_TYPE_RESERVED
> > - Renamed nr to vector
> > - Add positive test case for interrupt type == INTR_TYPE_NMI_INTR
> > - Explicitly encode 0 vecotr (i.e., DE_VECTOR)
> > - Updated deliver errorcode checks to test postive cases
> > - Moved test for INTR_TYPE_OTHER_EVENT after test for INTR_TYPE_RESERVED
> >
> > lib/x86/processor.h | 8 ++
> > x86/types.h | 1 +
> > x86/vmx.h | 27 ++++++
> > x86/vmx_tests.c | 231 ++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 267 insertions(+)
> >
> > diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> > index 247386207bb0..886170cfa163 100644
> > --- a/lib/x86/processor.h
> > +++ b/lib/x86/processor.h
> > @@ -15,6 +15,14 @@
> > # define S "4"
> > #endif
> >
> > +#define DF_VECTOR 8
> > +#define TS_VECTOR 10
> > +#define NP_VECTOR 11
> > +#define SS_VECTOR 12
> > +#define GP_VECTOR 13
> > +#define PF_VECTOR 14
> > +#define AC_VECTOR 17
> > +
> > #define X86_CR0_PE 0x00000001
> > #define X86_CR0_MP 0x00000002
> > #define X86_CR0_TS 0x00000008
> > diff --git a/x86/types.h b/x86/types.h
> > index fd22743990f1..047556e854d6 100644
> > --- a/x86/types.h
> > +++ b/x86/types.h
> > @@ -3,6 +3,7 @@
> >
> > #define DE_VECTOR 0
> > #define DB_VECTOR 1
> > +#define NMI_VECTOR 2
> > #define BP_VECTOR 3
> > #define OF_VECTOR 4
> > #define BR_VECTOR 5
> > diff --git a/x86/vmx.h b/x86/vmx.h
> > index bdcaac0edc01..116bbd515beb 100644
> > --- a/x86/vmx.h
> > +++ b/x86/vmx.h
> > @@ -420,6 +420,15 @@ enum Intr_type {
> >
> > #define INTR_INFO_INTR_TYPE_SHIFT 8
> >
> > +#define INTR_TYPE_EXT_INTR (0 << 8) /* external interrupt */
> > +#define INTR_TYPE_RESERVED (1 << 8) /* reserved */
> > +#define INTR_TYPE_NMI_INTR (2 << 8) /* NMI */
> > +#define INTR_TYPE_HARD_EXCEPTION (3 << 8) /* processor exception */
> > +#define INTR_TYPE_SOFT_INTR (4 << 8) /* software interrupt */
> > +#define INTR_TYPE_PRIV_SW_EXCEPTION (5 << 8) /* priv. software exception */
> > +#define INTR_TYPE_SOFT_EXCEPTION (6 << 8) /* software exception */
> > +#define INTR_TYPE_OTHER_EVENT (7 << 8) /* other event */
> > +
> > /*
> > * VM-instruction error numbers
> > */
> > @@ -712,6 +721,24 @@ static inline bool invvpid(unsigned long type, u64 vpid, u64 gla)
> > return ret;
> > }
> >
> > +static inline int enable_unrestricted_guest(void)
> > +{
> > + if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY))
> > + return -1;
> > +
> > + if (!(ctrl_cpu_rev[1].clr & CPU_URG))
> > + return -1;
> > +
> > + vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY);
> > + vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG);
> > + return 0;
> > +}
> > +
> > +static inline void disable_unrestricted_guest(void)
> > +{
> > + vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) & ~CPU_URG);
> > +}
> > +
> > const char *exit_reason_description(u64 reason);
> > void print_vmexit_info();
> > void print_vmentry_failure_info(struct vmentry_failure *failure);
> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> > index 0c1a6952f9ea..98f95df1316b 100644
> > --- a/x86/vmx_tests.c
> > +++ b/x86/vmx_tests.c
> > @@ -3561,6 +3561,236 @@ static void try_tpr_threshold_and_vtpr(unsigned threshold, unsigned vtpr)
> > report_prefix_pop();
> > }
> >
> > +static void test_invalid_event_injection(void)
> > +{
> > + u32 ent_intr_info_save = vmcs_read(ENT_INTR_INFO);
> > + u32 ent_intr_error_save = vmcs_read(ENT_INTR_ERROR);
> > + u32 ent_inst_len_save = vmcs_read(ENT_INST_LEN);
> > + u32 primary_save = vmcs_read(CPU_EXEC_CTRL0);
> > + u32 secondary_save = vmcs_read(CPU_EXEC_CTRL1);
> > + u64 guest_cr0_save = vmcs_read(GUEST_CR0);
> > + u32 ent_intr_info_base = INTR_INFO_VALID_MASK;
> > + u32 ent_intr_info, ent_intr_err, ent_intr_len;
> > + u32 cnt;
> > +
> > + /* Setup */
> > + report_prefix_push("invalid event injection");
> > + vmcs_write(ENT_INTR_ERROR, 0x00000000);
> > + vmcs_write(ENT_INST_LEN, 0x00000001);
> > +
> > + /* The field’s interruption type is not set to a reserved value. */
> > + ent_intr_info = ent_intr_info_base | INTR_TYPE_RESERVED | DE_VECTOR;
> This test is all about checking that the interruption type is not a
> reserved value. Is DE_VECTOR relevant here ?
No, it's not related. The vector is arbitrarily encoded to be 0 (i.e.,
DE_VECTOR). I've made that encoding explicit in this version of the
patch. If you have a preference, let me know which you prefer---v1,
which implicitly encodes DE_VECTOR, or v2 which explicitly encodes
DE_VECTOR. I have no preference.
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "RESERVED interruption type invalid",
> > + ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > +
> Don't we want to just remove INTR_TYPE_RESERVED and run the test again
> for positive test case ?
It's arbitrary. Removing INTR_TYPE_RESERVED encodes leaves the
interruption type as 0, which is INTR_TYPE_EXT_INTR. But I'll go ahead
and change it.
> > + ent_intr_info = ent_intr_info_base | INTR_TYPE_HARD_EXCEPTION |
> > + DE_VECTOR;
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "RESERVED interruption type invalid",
> > + ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(true, false);
> > + report_prefix_pop();
> > +
> > + /* If the interruption type is other event, the vector is 0. */
> > + ent_intr_info = ent_intr_info_base | INTR_TYPE_OTHER_EVENT | DB_VECTOR;
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "(OTHER EVENT && vector != 0) invalid",
> > + ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> It's more readable if you place this test right after the one for
> INTR_TYPE_RESERVED, because the SDM describes them together.
The previous test case is supposed to be for the positive test case,
where the interruption type is not INTR_TYPE_RESERVED. I think we
should leave the negative and positive test case adjacent to each
other. But let me know if you strongly disagree, and I'll move this
one above.
> > +
> > + /* If the interruption type is NMI, the vector is 2 (negative case). */
> > + ent_intr_info = ent_intr_info_base | INTR_TYPE_NMI_INTR | DE_VECTOR;
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "(NMI && vector != 2) invalid", ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > +
> > + /* If the interruption type is NMI, the vector is 2 (positive case). */
> > + ent_intr_info = ent_intr_info_base | INTR_TYPE_NMI_INTR | NMI_VECTOR;
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "(NMI && vector == 2) valid", ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(true, false);
> > + report_prefix_pop();
> > +
> > + /*
> > + * If the interruption type
> > + * is HW exception, the vector is at most 31.
> > + */
> > + ent_intr_info = ent_intr_info_base | INTR_TYPE_HARD_EXCEPTION | 0x20;
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "(HW exception && vector > 31) invalid",
> > + ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > +
> > + /*
> > + * deliver-error-code is 1 iff either
> > + * (a) the "unrestricted guest" VM-execution control is 0
> > + * (b) CR0.PE is set.
> > + */
> > + ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK |
> > + INTR_TYPE_HARD_EXCEPTION | GP_VECTOR;
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "error code <-> (!URG || prot_mode)",
> > + ent_intr_info);
> > + enable_unrestricted_guest();
> > + vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> May be you can add another test with the same conditions except
> unrestricted guest being disabled ?
Will do.
> > +
> > + ent_intr_info = ent_intr_info_base | INTR_TYPE_HARD_EXCEPTION |
> > + GP_VECTOR;
> You are missing INTR_INFO_DELIVER_CODE_MASK. Or are you purposely
> turning that bit off ? If you are purposely turning the bit off, the
> test should pass. The SDM says that if INTR_INFO_DELIVER_CODE_MASK is
> set, certain conditions need to be fulfilled. But if it's not set,
> there's nothing to test for failure.
"But if it's not set, there's nothing to test for failure." Your
interpretation is not correct (I had the same mis-interpretation when
I first reviewed the text in the SDM). When the SDM says "If and only
if" that specifies both the negative and positive case. In other
words: error code <-> (!URG || prot_mode). Thus, if !(!URG ||
prot_mode), then the deliver error code bit MUST be clear.
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "error code <-> (!URG || prot_mode)",
> > + ent_intr_info);
> > + vmcs_write(GUEST_CR0, guest_cr0_save | X86_CR0_PE);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > +
> > + /* deliver-error-code is 1 iff the interruption type is HW exception */
> > + report_prefix_push("error code <-> HW exception");
> > + for (cnt = 0; cnt < 8; cnt++) {
> > + u32 exception_type_mask = cnt << 8;
> > + u32 deliver_error_code_mask =
> > + exception_type_mask != INTR_TYPE_HARD_EXCEPTION ?
> > + INTR_INFO_DELIVER_CODE_MASK : 0;
> > +
> > + ent_intr_info = ent_intr_info_base | deliver_error_code_mask |
> > + exception_type_mask | GP_VECTOR;
> > + report_prefix_pushf("VM-entry intr info=0x%x", ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > + }
> > + report_prefix_pop();
> > +
> > + /*
> > + * deliver-error-code is 1 iff the the vector
> > + * indicates an exception that would normally deliver an error code
> > + */
> > + report_prefix_push("error code <-> vector delivers error code");
> > + for (cnt = 0; cnt < 32; cnt++) {
> > + bool has_error_code = false;
> > + u32 deliver_error_code_mask;
> > +
> > + switch (cnt) {
> > + case DF_VECTOR:
> > + case TS_VECTOR:
> > + case NP_VECTOR:
> > + case SS_VECTOR:
> > + case GP_VECTOR:
> > + case PF_VECTOR:
> > + case AC_VECTOR:
> > + has_error_code = true;
> > + }
> > +
> > + /* Negative case */
> > + deliver_error_code_mask = has_error_code ?
> > + 0 :
> > + INTR_INFO_DELIVER_CODE_MASK;
> > + ent_intr_info = ent_intr_info_base | deliver_error_code_mask |
> > + INTR_TYPE_HARD_EXCEPTION | cnt;
> > + report_prefix_pushf("VM-entry intr info=0x%x", ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > +
> > + /* Positive case */
> > + deliver_error_code_mask = has_error_code ?
> > + INTR_INFO_DELIVER_CODE_MASK :
> > + 0;
> > + ent_intr_info = ent_intr_info_base | deliver_error_code_mask |
> > + INTR_TYPE_HARD_EXCEPTION | cnt;
> > + report_prefix_pushf("VM-entry intr info=0x%x", ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(true, false);
> > + report_prefix_pop();
> > + }
> > + report_prefix_pop();
> Do we need the extra call to report_prefix_pop() at the end of each loop ?
Yes, it's paired with a report_prefix_push to distinguish the exact
value of the VM-entry interruption information field. This makes the
log much easier to parse. For example:
PASS: invalid event injection: error code <-> vector delivers error
code: VM-entry intr info=0x80000b00: vmlaunch fails
PASS: invalid event injection: error code <-> vector delivers error
code: VM-entry intr info=0x80000b00: VMX inst error is 7 (actual 7)
PASS: invalid event injection: error code <-> vector delivers error
code: VM-entry intr info=0x80000300: vmlaunch succeeds
PASS: invalid event injection: error code <-> vector delivers error
code: VM-entry intr info=0x80000b01: vmlaunch fails
PASS: invalid event injection: error code <-> vector delivers error
code: VM-entry intr info=0x80000b01: VMX inst error is 7 (actual 7)
PASS: invalid event injection: error code <-> vector delivers error
code: VM-entry intr info=0x80000301: vmlaunch succeeds
> > +
> > + /* Reserved bits in the field (30:12) are 0. */
> > + report_prefix_push("reserved bits clear");
> > + for (cnt = 12; cnt <= 30; cnt++) {
> > + ent_intr_info = ent_intr_info_base |
> > + INTR_INFO_DELIVER_CODE_MASK |
> > + INTR_TYPE_HARD_EXCEPTION | GP_VECTOR |
> > + (1U << cnt);
> Do we need to set INTR_INFO_DELIVER_CODE_MASK, INTR_TYPE_HARD_EXCEPTION
> and GP_VECTOR here ? All we are testing here are bits [30:12].
We need an VM-entry information interruption field that delivers an
error code. I chose this one because I knew that it meets the
conditions to deliver an error code.
> > + report_prefix_pushf("VM-entry intr info=0x%x", ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > + }
> > + report_prefix_pop();
> > +
> > + /*
> > + * If deliver-error-code is 1
> > + * bits 31:15 of the VM-entry exception error-code field are 0.
> > + */
> > + ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK |
> > + INTR_TYPE_HARD_EXCEPTION | GP_VECTOR;
> Again, do we need INTR_TYPE_HARD_EXCEPTION and GP_VECTOR here ?
Same response as above.
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "VM-entry exception error code[31:15] clear",
> > + ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > + for (cnt = 15; cnt <= 31; cnt++) {
> > + ent_intr_err = 1U << cnt;
> > + report_prefix_pushf("VM-entry intr error=0x%x", ent_intr_err);
> > + vmcs_write(ENT_INTR_ERROR, ent_intr_err);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > + }
> > + vmcs_write(ENT_INTR_ERROR, 0x00000000);
> > + report_prefix_pop();
> > +
> > + /*
> > + * If the interruption type is software interrupt, software exception,
> > + * or privileged software exception, the VM-entry instruction-length
> > + * field is in the range 0–15.
> > + */
> > +
> > + ent_intr_info = ent_intr_info_base | INTR_TYPE_SOFT_EXCEPTION;
> > + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> > + "VM-entry instruction-length check", ent_intr_info);
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> > +
> > + /* Instruction length set to -1 (0xFFFFFFFF) should fail */
> > + ent_intr_len = -1;
> > + report_prefix_pushf("VM-entry intr length = 0x%x", ent_intr_len);
> > + vmcs_write(ENT_INST_LEN, ent_intr_len);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > +
> > + /* Instruction length set to 16 should fail */
> > + ent_intr_len = 0x00000010;
> > + report_prefix_pushf("VM-entry intr length = 0x%x", ent_intr_len);
> > + vmcs_write(ENT_INST_LEN, 0x00000010);
> > + test_vmx_controls(false, false);
> > + report_prefix_pop();
> > +
> > + report_prefix_pop();
> You should also test INTR_TYPE_PRIV_SW_EXCEPTION and INTR_TYPE_SOFT_INTR
> in addition to INTR_TYPE_SOFT_EXCEPTION.
Good call, I'll add the tests.
> > +
> > + /* Cleanup */
> > + vmcs_write(ENT_INTR_INFO, ent_intr_info_save);
> > + vmcs_write(ENT_INTR_ERROR, ent_intr_error_save);
> > + vmcs_write(ENT_INST_LEN, ent_inst_len_save);
> > + vmcs_write(CPU_EXEC_CTRL0, primary_save);
> > + vmcs_write(CPU_EXEC_CTRL1, secondary_save);
> > + vmcs_write(GUEST_CR0, guest_cr0_save);
> > + report_prefix_pop();
> > +}
> > +
> > /*
> > * Test interesting vTPR values for a given TPR threshold.
> > */
> > @@ -3807,6 +4037,7 @@ static void vmx_controls_test(void)
> > test_apic_virt_addr();
> > test_tpr_threshold();
> > test_nmi_ctrls();
> > + test_invalid_event_injection();
> > }
> >
> > static bool valid_vmcs_for_vmentry(void)
>
