Re: [kvm-unit-tests PATCHv2] x86: nSVM: Add support for VNMI test

[Sean Christopherson <seanjc@google.com>]

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@amd.com>
> ---

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.