Re: [PATCH V9 2/2] KVM: selftests: Add test for configure of x86 APIC bus frequency

[Sean Christopherson <seanjc@google.com>]

On Wed, Jun 12, 2024, Reinette Chatre wrote:
> +/*
> + * Pick 25MHz for APIC bus frequency. Different enough from the default 1GHz.
> + * User can override via command line.
> + */
> +static uint64_t apic_hz = 25 * 1000 * 1000;
> +
> +/*
> + * Delay in msec that guest uses to determine APIC bus frequency.
> + * User can override via command line.
> + */
> +static unsigned long delay_ms = 100;

There's no need for these to be global, it's easy enough to pass them as params
to apic_guest_code().  Making is_x2apic is wortwhile as it cuts down on the noise,
but for these, I think it's better to keep them local.

> +
> +/*
> + * Possible TDCR values with matching divide count. Used to modify APIC
> + * timer frequency.
> + */
> +static struct {
> +	uint32_t tdcr;
> +	uint32_t divide_count;

These can/should all be const.

> +} tdcrs[] = {
> +	{0x0, 2},
> +	{0x1, 4},
> +	{0x2, 8},
> +	{0x3, 16},
> +	{0x8, 32},
> +	{0x9, 64},
> +	{0xa, 128},
> +	{0xb, 1},
> +};
> +
> +/* true if x2APIC test is running, false if xAPIC test is running. */
> +static bool is_x2apic;
> +
> +static void apic_enable(void)
> +{
> +	if (is_x2apic)
> +		x2apic_enable();
> +	else
> +		xapic_enable();
> +}
> +
> +static uint32_t apic_read_reg(unsigned int reg)
> +{
> +	return is_x2apic ? x2apic_read_reg(reg) : xapic_read_reg(reg);
> +}
> +
> +static void apic_write_reg(unsigned int reg, uint32_t val)
> +{
> +	if (is_x2apic)
> +		x2apic_write_reg(reg, val);
> +	else
> +		xapic_write_reg(reg, val);
> +}
> +
> +static void apic_guest_code(void)
> +{
> +	uint64_t tsc_hz = (uint64_t)tsc_khz * 1000;
> +	const uint32_t tmict = ~0u;
> +	uint64_t tsc0, tsc1, freq;
> +	uint32_t tmcct;
> +	int i;
> +
> +	apic_enable();
> +
> +	/*
> +	 * Setup one-shot timer.  The vector does not matter because the
> +	 * interrupt should not fire.
> +	 */
> +	apic_write_reg(APIC_LVTT, APIC_LVT_TIMER_ONESHOT | APIC_LVT_MASKED);
> +
> +	for (i = 0; i < ARRAY_SIZE(tdcrs); i++) {
> +
> +		apic_write_reg(APIC_TDCR, tdcrs[i].tdcr);
> +		apic_write_reg(APIC_TMICT, tmict);
> +
> +		tsc0 = rdtsc();
> +		udelay(delay_ms * 1000);
> +		tmcct = apic_read_reg(APIC_TMCCT);
> +		tsc1 = rdtsc();
> +
> +		/*
> +		 * Stop the timer _after_ reading the current, final count, as
> +		 * writing the initial counter also modifies the current count.
> +		 */
> +		apic_write_reg(APIC_TMICT, 0);
> +
> +		freq = (tmict - tmcct) * tdcrs[i].divide_count * tsc_hz / (tsc1 - tsc0);
> +		/* Check if measured frequency is within 1% of configured frequency. */

1% is likely too aggressive, i.e. we'll get false failures due to host activity.
On our systems, even a single pr_warn() in the timer path causes failure.  For
now, I think 5% is good enough, e.g. it'll catch cases where KVM is waaay off.
If we want to do better (or that's still too tight), then we can add a '-t <tolerance'
param or something.

> +		GUEST_ASSERT(freq < apic_hz * 101 / 100);
> +		GUEST_ASSERT(freq > apic_hz * 99 / 100);

Combine these into a single assert, and print the params, i.e. don't rely on the
line number to figure out what's up.

		__GUEST_ASSERT(freq < apic_hz * 105 / 100 && freq > apic_hz * 95 / 100,
			       "Frequency = %lu (wanted %lu - %lu), bus = %lu, div = %u, tsc = %lu",
			       freq, apic_hz * 95 / 100, apic_hz * 105 / 100,
			       apic_hz, tdcrs[i].divide_count, tsc_hz);

> +int main(int argc, char *argv[])
> +{
> +	int opt;
> +
> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_X86_APIC_BUS_CYCLES_NS));
> +
> +	while ((opt = getopt(argc, argv, "d:f:h")) != -1) {
> +		switch (opt) {
> +		case 'f':
> +			apic_hz = atol(optarg);
> +			break;
> +		case 'd':
> +			delay_ms = atol(optarg);
> +			break;
> +		case 'h':
> +			help(argv[0]);
> +			exit(0);
> +		default:
> +			help(argv[0]);
> +			exit(1);

Heh, selftests are anything but consistent, but this should be exit(KSFT_SKIP)
for both.

> +		}
> +	}
> +
> +	run_apic_bus_clock_test(false);
> +	run_apic_bus_clock_test(true);
> +}
> -- 
> 2.34.1
>