Re: [kvm-unit-tests PATCH v4 5/5] x86: add HPET counter read micro benchmark and enable/disable torture tests

[Peter Xu <peterx@redhat.com>]

On Fri, Jul 25, 2025 at 11:54:29AM +0200, Igor Mammedov wrote:
> test is to be used for benchmarking/validating HPET main counter reading
> 
> how to run:
>    QEMU=/foo/qemu-system-x86_64 x86/run x86/hpet_read_test.flat -smp X
> where X is desired number of logical CPUs to test with
> 
> it will 1st execute concurrent read benchmark
> and after that it will run torture test enabling/disabling HPET counter,
> while running readers in parallel. Goal is to verify counter that always
> goes up.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>    * use global for test cycles and siplify code a bit
>    * use cpu number instead of APCI ID as index into latency array
>    * report failure if a cpu measured 0 latency
>    * replace on_cpus() with on_cpu_async() to avoid BSP
>      interrupting itself
>    * drop volatile
> 
> v3:
>    * measure lat inside threads so that, threads startup time wouldn't
>      throw off results
>    * fix BSP iterrupting itself by running read test and stalling
>      other cpus as result. (fix it by exiting read test earlier if
>      it's running on BSP)
> v2:
>    * fix broken timer going backwards check
>    * report # of fails
>    * warn if number of vcpus is not sufficient for torture test and skip
>      it
>    * style fixups
> ---
>  x86/Makefile.common  |  2 +
>  x86/hpet_read_test.c | 93 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 95 insertions(+)
>  create mode 100644 x86/hpet_read_test.c
> 
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index 5663a65d..ef0e09a6 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -101,6 +101,8 @@ tests-common += $(TEST_DIR)/realmode.$(exe) \
>  realmode_bits := $(if $(call cc-option,-m16,""),16,32)
>  endif
>  
> +tests-common += $(TEST_DIR)/hpet_read_test.$(exe)
> +
>  test_cases: $(tests-common) $(tests)
>  
>  $(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/x86 -I lib
> diff --git a/x86/hpet_read_test.c b/x86/hpet_read_test.c
> new file mode 100644
> index 00000000..a44cdac2
> --- /dev/null
> +++ b/x86/hpet_read_test.c
> @@ -0,0 +1,93 @@
> +#include "libcflat.h"
> +#include "smp.h"
> +#include "apic.h"
> +#include "asm/barrier.h"
> +#include "x86/atomic.h"
> +#include "vmalloc.h"
> +#include "alloc.h"
> +
> +#define HPET_ADDR         0xFED00000L
> +#define HPET_COUNTER_ADDR ((uint8_t *)HPET_ADDR + 0xF0UL)
> +#define HPET_CONFIG_ADDR  ((uint8_t *)HPET_ADDR + 0x10UL)
> +#define HPET_ENABLE_BIT   0x01UL
> +#define HPET_CLK_PERIOD   10
> +
> +#define TEST_CYCLES 100000
> +
> +static atomic_t fail;
> +static uint64_t latency[MAX_TEST_CPUS];
> +
> +static void hpet_reader(void *data)
> +{
> +	long i;
> +	uint64_t old_counter = 0, new_counter;
> +	long id = (long)data;
> +
> +	latency[id] = *(uint64_t *)HPET_COUNTER_ADDR;
> +	for (i = 0; i < TEST_CYCLES; ++i) {
> +		new_counter = *(uint64_t *)HPET_COUNTER_ADDR;
> +		if (new_counter < old_counter)
> +			atomic_inc(&fail);
> +		old_counter = new_counter;
> +	}
> +	/* claculate job latency in ns */
> +	latency[id] = (*(uint64_t *)HPET_COUNTER_ADDR - latency[id])
> +		* HPET_CLK_PERIOD / TEST_CYCLES;
> +}
> +
> +static void hpet_writer(void *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < TEST_CYCLES; ++i)
> +		if (i % 2)
> +			*(uint64_t *)HPET_CONFIG_ADDR |= HPET_ENABLE_BIT;
> +		else
> +			*(uint64_t *)HPET_CONFIG_ADDR &= ~HPET_ENABLE_BIT;
> +}
> +
> +int main(void)
> +{
> +	unsigned long cpu, time_ns, lat = 0;
> +	uint64_t start, end;
> +	int ncpus = cpu_count();
> +
> +	do {
> +		printf("* starting concurrent read bench on %d cpus\n", ncpus);
> +		*(uint64_t *)HPET_CONFIG_ADDR |= HPET_ENABLE_BIT;
> +		start = *(uint64_t *)HPET_COUNTER_ADDR;
> +
> +		for (cpu = cpu_count() - 1; cpu > 0; --cpu)
> +			on_cpu_async(cpu, hpet_reader, (void *)cpu);
> +		while (cpus_active() > 1)
> +			pause();
> +
> +		end = (*(uint64_t *)HPET_COUNTER_ADDR);
> +		time_ns = (end - start) * HPET_CLK_PERIOD;
> +
> +		for (cpu = 1; cpu < ncpus; cpu++)
> +			if (latency[cpu])
> +				lat += latency[cpu];
> +			else
> +				report_fail("cpu %lu reported invalid latency (0)\n", cpu);
> +		lat = lat / ncpus;
> +
> +		report(time_ns && !atomic_read(&fail),
> +			"read test took %lu ms, avg read: %lu ns\n", time_ns/1000000,  lat);
> +	} while (0);
> +
> +	do {
> +		printf("* starting enable/disable with concurrent readers torture\n");
> +		if (ncpus > 2) {
> +			for (cpu = 2; cpu < ncpus; cpu++)
> +				on_cpu_async(cpu, hpet_reader, (void *)TEST_CYCLES);
> +
> +			on_cpu(1, hpet_writer, (void *)TEST_CYCLES);
> +			report(!atomic_read(&fail), "torture test, fails: %u\n",
> +				atomic_read(&fail));
> +		} else
> +			printf("SKIP: torture test: '-smp X' should be greater than 2\n");
> +	} while (0);

Nitpick: IMHO the "do... while" indentation isn't strictly needed.  Maybe
it implies the blobs can be put separately into two functions, e.g. as
test_hpet_concurrent_ro() / test_hpet_concurrent_rw().

Reviewed-by: Peter Xu <peterx@redhat.com>

> +
> +	return report_summary();
> +}
> -- 
> 2.47.1
> 

-- 
Peter Xu