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

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

[Roman Kagan]

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@amazon.de;
until this (or some alternative) fix is merged the test will hang on
this testcase.

Signed-off-by: Roman Kagan <rkagan@amazon.de>
---
 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));
+}
+
+static void check_running_counter_wrmsr_in_pmi(void)
+{
+	pmu_counter_t evt = {
+		.ctr = MSR_GP_COUNTERx(0),
+		.config = EVNTSEL_OS | EVNTSEL_USR | EVNTSEL_INT |
+			  gp_events[1].unit_sel,
+	};
+
+	report_prefix_push("running counter wrmsr in PMI");
+
+	start_event(&evt);
+	apic_write(APIC_LVTPC, PMI_VECTOR + 1);
+
+	irq_received = 0;
+	irq_enable();
+	/*
+	 * the value written will be treated as -1 both for full-width and
+	 * non-full-width counters; for the latter upper 32 bits are ignored
+	 */
+	wrmsr(MSR_GP_COUNTERx(0),
+	      -1ull & ((1ull << pmu.gp_counter_width) - 1));
+
+	loop();
+	stop_event(&evt);
+	irq_disable();
+	report(evt.count >= gp_events[1].min, "cntr");
+	report(irq_received, "irq");
+
+	report_prefix_pop();
+}
+
 static void check_emulated_instr(void)
 {
 	uint64_t status, instr_start, brnch_start;
@@ -559,6 +598,7 @@ static void check_counters(void)
 	check_counter_overflow();
 	check_gp_counter_cmask();
 	check_running_counter_wrmsr();
+	check_running_counter_wrmsr_in_pmi();
 }
 
 static void do_unsupported_width_counter_write(void *index)
@@ -671,6 +711,7 @@ 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();
@@ -697,8 +738,6 @@ 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()) {
-- 
2.34.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

[Sean Christopherson]

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@amazon.de;
> until this (or some alternative) fix is merged the test will hang on
> this testcase.
> 
> Signed-off-by: Roman Kagan <rkagan@amazon.de>
> ---
>  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()) {