On Wed, Aug 10, 2022, Santosh Shukla wrote:
> Add a VNMI test case to test Virtual NMI in a nested environment,
> The test covers the Virtual NMI (VNMI) delivery.
>
> Signed-off-by: Santosh Shukla <santosh.shukla@xxxxxxx>
> ---
Quite a few comments, but I'll post a v3 with everything cleaned up. More below.
> diff --git a/x86/svm.h b/x86/svm.h
> index 766ff7e36449..91a0dee2c864 100644
> --- a/x86/svm.h
> +++ b/x86/svm.h
> @@ -131,6 +131,13 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
> #define V_INTR_MASKING_SHIFT 24
> #define V_INTR_MASKING_MASK (1 << V_INTR_MASKING_SHIFT)
>
> +#define V_NMI_PENDING_SHIFT 11
> +#define V_NMI_PENDING (1 << V_NMI_PENDING_SHIFT)
> +#define V_NMI_MASK_SHIFT 12
> +#define V_NMI_MASK (1 << V_NMI_MASK_SHIFT)
> +#define V_NMI_ENABLE_SHIFT 26
> +#define V_NMI_ENABLE (1 << V_NMI_ENABLE_SHIFT)
Same complaints as I had for the kernel side, e.g. VM_NMI_MASK vs. V_NMI_BLOCKING_MASK.
> +
> #define SVM_INTERRUPT_SHADOW_MASK 1
>
> #define SVM_IOIO_STR_SHIFT 2
> @@ -419,6 +426,7 @@ void default_prepare(struct svm_test *test);
> void default_prepare_gif_clear(struct svm_test *test);
> bool default_finished(struct svm_test *test);
> bool npt_supported(void);
> +bool vnmi_supported(void);
> int get_test_stage(struct svm_test *test);
> void set_test_stage(struct svm_test *test, int s);
> void inc_test_stage(struct svm_test *test);
> diff --git a/x86/svm_tests.c b/x86/svm_tests.c
> index e2ec9541fd29..f83a2b56ce52 100644
> --- a/x86/svm_tests.c
> +++ b/x86/svm_tests.c
> @@ -1445,6 +1445,93 @@ static bool nmi_hlt_check(struct svm_test *test)
> return get_test_stage(test) == 3;
> }
>
> +static volatile bool vnmi_fired;
There's no need to use a separate flag, just reuse nmi_fired. And then this
test can also reuse nmi_prepare().
> +static void vnmi_handler(isr_regs_t *regs)
> +{
> + vnmi_fired = true;
Use tabs, not spaces.
> +}
> +
> +static void vnmi_prepare(struct svm_test *test)
> +{
> + default_prepare(test);
> + vnmi_fired = false;
> + vmcb->control.int_ctl = V_NMI_ENABLE;
> + vmcb->control.int_vector = NMI_VECTOR;
> + handle_irq(NMI_VECTOR, vnmi_handler);
> + set_test_stage(test, 0);
> +}
> +
> +static void vnmi_test(struct svm_test *test)
> +{
> + if (vnmi_fired) {
> + report(!vnmi_fired, "vNMI dispatched even before injection");
> + set_test_stage(test, -1);
Ugh, so much copy+paste in the test. I'll add a patch to provide a macro to dedup
the report_fail() + set_test_stage() + vmmcall(), the last of which is unnecessarily
dependent on non-failing code in many cases.
> +static bool vnmi_finished(struct svm_test *test)
> +{
> + switch (get_test_stage(test)) {
> + case 0:
> + if (vmcb->control.exit_code != SVM_EXIT_ERR) {
Took me a bit of staring to understand why an error is expected. The setup path
really needs a comment explaining that it deliberately creates a bad configuration,
and it should explicitly clear INTERCEPT_NMI.
> + report_fail("VMEXIT not due to error. Exit reason 0x%x",
> + vmcb->control.exit_code);
> + return true;
> + }
> + report(!vnmi_fired, "vNMI enabled but NMI_INTERCEPT unset!");
> + vmcb->control.intercept |= (1ULL << INTERCEPT_NMI);
> + vmcb->save.rip += 3;
> + break;
> +
> + case 1:
> + if (vmcb->control.exit_code != SVM_EXIT_VMMCALL) {
> + report_fail("VMEXIT not due to error. Exit reason 0x%x",
VMMCALL expected, not ERR.
