> + /* The field’s interruption type is not set to a reserved value. */
> + ent_intr_info = ent_intr_info_base | INTR_TYPE_RESERVED;
> + 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();
>
> What about the positive test case i.e., when INTR_TYPE_RESERVED is not set ?
I'll add a positive test case for interrupt type != INTR_TYPE_RESERVED.
> + /* If the interruption type is NMI, the vector is 2. */
> + ent_intr_info = ent_intr_info_base | INTR_TYPE_NMI_INTR;
> + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> + "(NMI && nr != 2) invalid", ent_intr_info);
>
> 'nr' is not meaningful in the string. You should probably replace it with 'vector' or some other meaningful description.
I'll change nr to vector.
> + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> + test_vmx_controls(false, false);
> + report_prefix_pop();
>
> We should also add do the positive test case i.e., when the vector value is 2, the test should pass.
> For readability purpose, I think it is useful to add a vector value (say, zero) explicitly in the negative case (the one you have here).
I'll add the positive test case for NMI && vector == 2.
I'll also explicitly set the vector to 0 (i.e., DE_VECTOR).
> + /*
> + * 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 && nr > 31) invalid", ent_intr_info);
> + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> + test_vmx_controls(false, false);
> + report_prefix_pop();
>
> We should also add a positive test case i.e., when the vector is 31 or less, the test should pass.
Rather than add the positive test case you're suggesting here, I'll
update the loop below, that sets the "deliver error code" bit for each
interrupt vector to all also check the positive case. This will
achieve the positive test coverage you're suggesting.
> + /* If the interruption type is other event, the vector is 0. */
> + ent_intr_info = ent_intr_info_base | INTR_TYPE_OTHER_EVENT | 0x1;
> + report_prefix_pushf("%s, VM-entry intr info=0x%x",
> + "(OTHER EVENT && nr != 0) invalid", ent_intr_info);
>
> + vmcs_write(ENT_INTR_INFO, ent_intr_info);
> + test_vmx_controls(false, false);
> + report_prefix_pop();
>
> Nit: It is better to place this test right after the test for INTR_TYPE_RESERVED above because the SDM describes the two rules together.
I'll move it.
>
> According to the SDM, the INTR_TYPE_OTHER_EVENT bits will be zero on processors that don't support the "monitor trap flag" control. Don't we need to include "monitor trap flag" here ?
Yes, you're right about the SDM and the patch that this test
corresponds to. But the function nested_vmx_setup_ctls_msrs hard-codes
this control as disabled. Since it's not actually configurable (e.g.,
by a KVM ioctl), I decided not to add a test for this case.
