Re: [PATCH kvm-unit-tests] svm: Verify a pending interrupt queued before entering the guest is not lost

[Paolo Bonzini <pbonzini@xxxxxxxxxx>]

On 05/11/19 16:12, Cathy Avery wrote:
> +static void pending_event_prepare(struct test *test)
> +{
> +    int ipi_vector = 0xf1;
> +
> +    default_prepare(test);
> +

I think this test should call VMRUN with EFLAGS.IF=1.  The GIF/IF
interaction is as follows:

- in the host, IF is simply ignored with GIF=0.  VMRUN atomically sets
GIF to 1. Therefore, interrupts are wholly masked between CLGI and VMRUN
and between VMRUN and STGI, but not during VMRUN.  Currently, interrupts
are masked during all of VMRUN actually, because default_prepare clears
the interrupt flag.

- in the guest, if VINTR_MASKING=0 (in kvm-unit-tests: bit 24 of
int_ctl), IF governs whether host interrupts are delivered even while
the guest is running.

- if VINTR_MASKING=1, however, the pre-VMRUN value of IF (that's stored
in HF_HIF_MASK, let's call it HIF from now on) governs whether host
interrupts are delivered.

Actually, I think HIF=1 is a good default for most tests, so I would
start with something like

diff --git a/x86/svm.c b/x86/svm.c
index 4ddfaa4..7db3798 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -254,6 +255,7 @@ static void test_run(struct test *test,
     u64 vmcb_phys = virt_to_phys(vmcb);
     u64 guest_stack[10000];

+    irq_disable();
     test->vmcb = vmcb;
     test->prepare(test);
     vmcb->save.rip = (ulong)test_thunk;
@@ -269,7 +271,9 @@ static void test_run(struct test *test,
             "mov regs, %%r15\n\t"       // rax
             "mov %%r15, 0x1f8(%0)\n\t"
             LOAD_GPR_C
+            "sti \n\t"		// only used if V_INTR_MASKING=1
             "vmrun \n\t"
+            "cli \n\t"
             SAVE_GPR_C
             "mov 0x170(%0), %%r15\n\t"  // rflags
             "mov %%r15, regs+0x80\n\t"
@@ -284,6 +288,7 @@ static void test_run(struct test *test, struct vmcb
*vmcb)
 	tsc_end = rdtsc();
         ++test->exits;
     } while (!test->finished(test));
+    irq_enable();

     report("%s", test->succeeded(test), test->name);
 }
@@ -301,7 +306,6 @@ static bool default_supported(void)
 static void default_prepare(struct test *test)
 {
     vmcb_ident(test->vmcb);
-    cli();
 }

 static bool default_finished(struct test *test)

and see if it breaks something.  Then, it's probably useful to modify
your prepare callback to set IF=1 in the regs.rflags, since otherwise
the interrupts shouldn't be processed anyway.

> +        test->vmcb->control.intercept &= ~(1ULL << INTERCEPT_INTR);
> +        test->vmcb->control.int_ctl &= ~V_INTR_MASKING_MASK;
> +

I think these adjustments are not needed.  However, you could add two
other tests:

- one which runs with V_INTR_MASKING=1 and HIF=0.  In that case, the
VMMCALL should be reached without a prior SVM_EXIT_INTR vmexit.

- one which runs with V_INTR_MASKING=0 and EFLAGS.IF=1.  In that case,
the VMMCALL should be reached without a prior SVM_EXIT_INTR vmexit, and
the interrupt should be delivered while in guest mode, before
pending_event_guest_run is set.

Thanks,

Paolo