Re: [kvm-unit-tests] x86/pmu: add testcase for WRMSR to counter in PMI handler

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Thu, May 04, 2023, Roman Kagan wrote:
> Add a testcase where the PMI handler writes a negative value to the perf
> counter whose overflow would trigger that PMI.
> 
> It's meant specifically to cover the KVM bug where every negative value
> written to the counter caused an immediate overflow; in that case the
> vCPU would never leave PMI loop.
> 
> The bug is addressed in
> https://lore.kernel.org/kvm/20230504120042.785651-1-rkagan@xxxxxxxxx;
> until this (or some alternative) fix is merged the test will hang on
> this testcase.
> 
> Signed-off-by: Roman Kagan <rkagan@xxxxxxxxx>
> ---
>  x86/pmu.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/x86/pmu.c b/x86/pmu.c
> index 72c2c9cfd8b0..cdf9093722fb 100644
> --- a/x86/pmu.c
> +++ b/x86/pmu.c
> @@ -74,6 +74,7 @@ static void cnt_overflow(isr_regs_t *regs)
>  static bool check_irq(void)
>  {
>  	int i;
> +	apic_write(APIC_LVTPC, PMI_VECTOR);
>  	irq_received = 0;
>  	irq_enable();
>  	for (i = 0; i < 100000 && !irq_received; i++)
> @@ -156,7 +157,6 @@ static void __start_event(pmu_counter_t *evt, uint64_t count)
>  	    wrmsr(MSR_CORE_PERF_FIXED_CTR_CTRL, ctrl);
>      }
>      global_enable(evt);
> -    apic_write(APIC_LVTPC, PMI_VECTOR);
>  }
>  
>  static void start_event(pmu_counter_t *evt)
> @@ -474,6 +474,45 @@ static void check_running_counter_wrmsr(void)
>  	report_prefix_pop();
>  }
>  
> +static void cnt_overflow_with_wrmsr(isr_regs_t *regs)
> +{
> +	cnt_overflow(regs);
> +	/* write negative value that won't cause immediate overflow */
> +	wrmsr(MSR_GP_COUNTERx(0),
> +	      ((-1ull) << 31) & ((1ull << pmu.gp_counter_width) - 1));
> +}

This seems way more complicated than it needs to be.  Linux does the write in its
PMI, but that isn't relevant to hitting the bug, it only makes the bug visible,
i.e. hangs the guest.

Wouldn't it suffice to write a negative value that isn't supposed to overflow,
and then assert that overflow doesn't happen?

If the the PMI shenanigans are needed for some reason, I would vote to just switch
out the handler, not change the vector, which I find weird and unintuitive, e.g.

diff --git a/x86/pmu.c b/x86/pmu.c
index f67da863..6cdd644c 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -159,6 +159,7 @@ static void __start_event(pmu_counter_t *evt, uint64_t count)
            wrmsr(MSR_CORE_PERF_FIXED_CTR_CTRL, ctrl);
     }
     global_enable(evt);
+    apic_write(APIC_LVTPC, PMI_VECTOR);
 }
 
 static void start_event(pmu_counter_t *evt)
@@ -493,9 +494,9 @@ static void check_running_counter_wrmsr_in_pmi(void)
        };
 
        report_prefix_push("running counter wrmsr in PMI");
+       handle_irq(PMI_VECTOR, cnt_overflow_with_wrmsr);
 
        start_event(&evt);
-       apic_write(APIC_LVTPC, PMI_VECTOR + 1);
 
        irq_received = 0;
        irq_enable();
@@ -509,6 +510,9 @@ static void check_running_counter_wrmsr_in_pmi(void)
        loop();
        stop_event(&evt);
        irq_disable();
+
+       handle_irq(PMI_VECTOR, cnt_overflow);
+
        report(evt.count >= gp_events[1].min, "cntr");
        report(irq_received, "irq");
 
@@ -755,7 +759,6 @@ int main(int ac, char **av)
 {
        setup_vm();
        handle_irq(PMI_VECTOR, cnt_overflow);
-       handle_irq(PMI_VECTOR + 1, cnt_overflow_with_wrmsr);
        buf = malloc(N*64);
 
        check_invalid_rdpmc_gp();
@@ -782,6 +785,8 @@ int main(int ac, char **av)
        printf("Fixed counters:      %d\n", pmu.nr_fixed_counters);
        printf("Fixed counter width: %d\n", pmu.fixed_counter_width);
 
+       apic_write(APIC_LVTPC, PMI_VECTOR);
+
        check_counters();
 
        if (pmu_has_full_writes()) {