Message-ID: <fba4628f-9786-4e76-84cb-178508d90fd8@intel.com>
Date: Tue, 11 Jun 2024 14:34:58 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: <isaku.yamahata@...el.com>, <pbonzini@...hat.com>,
	<erdemaktas@...gle.com>, <vkuznets@...hat.com>, <vannapurve@...gle.com>,
	<jmattson@...gle.com>, <mlevitsk@...hat.com>, <xiaoyao.li@...el.com>,
	<chao.gao@...el.com>, <rick.p.edgecombe@...el.com>, <yuan.yao@...el.com>,
	<kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V8 2/2] KVM: selftests: Add test for configure of x86 APIC
 bus frequency

Hi Sean,

On 6/10/24 5:51 PM, Sean Christopherson wrote:
> On Mon, Jun 10, 2024, Reinette Chatre wrote:
>> diff --git a/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c
>> new file mode 100644
>> index 000000000000..602cec91d8ee
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/x86_64/apic_bus_clock_test.c
>> @@ -0,0 +1,219 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024 Intel Corporation
>> + *
>> + * Verify KVM correctly emulates the APIC bus frequency when the VMM configures
>> + * the frequency via KVM_CAP_X86_APIC_BUS_CYCLES_NS.  Start the APIC timer by
>> + * programming TMICT (timer initial count) to the largest value possible (so
>> + * that the timer will not expire during the test).  Then, after an arbitrary
>> + * amount of time has elapsed, verify TMCCT (timer current count) is within 1%
>> + * of the expected value based on the time elapsed, the APIC bus frequency, and
>> + * the programmed TDCR (timer divide configuration register).
>> + */
>> +
>> +#include "apic.h"
>> +#include "test_util.h"
>> +
>> +/*
>> + * Pick 25MHz for APIC bus frequency. Different enough from the default 1GHz.
>> + * User can override via command line.
>> + */
>> +unsigned long apic_hz = 25 * 1000 * 1000;
> 
> static, and maybe a uint64_t to match the other stuff?

Sure. Also moved all other globals and functions back to static.

> 
>> +/*
>> + * Possible TDCR values with matching divide count. Used to modify APIC
>> + * timer frequency.
>> + */
>> +struct {
>> +	uint32_t tdcr;
>> +	uint32_t divide_count;
>> +} tdcrs[] = {
>> +	{0x0, 2},
>> +	{0x1, 4},
>> +	{0x2, 8},
>> +	{0x3, 16},
>> +	{0x8, 32},
>> +	{0x9, 64},
>> +	{0xa, 128},
>> +	{0xb, 1},
>> +};
>> +
>> +void guest_verify(uint64_t tsc_cycles, uint32_t apic_cycles, uint32_t divide_count)
> 
> uin64_t for apic_cycles?  And maybe something like guest_check_apic_count(), to
> make it more obvious what is being verified?  Actually, it should be quite easy
> to have the two flavors share the bulk of the code.

I now plan to drop this function and instead just open code the
checks in what has now become a shared function between xAPIC and x2APIC.

> 
>> +{
>> +	unsigned long tsc_hz = tsc_khz * 1000;
>> +	uint64_t freq;
>> +
>> +	GUEST_ASSERT(tsc_cycles > 0);
> 
> Is this necessary?  Won't the "freq < ..." check fail?  I love me some paranoia,
> but this seems unnecessary.

Sure. After needing to field reports from static checkers not able to determine
that a denominator can never be zero I do tend to add these checks just to
pre-emptively placate them. I did run the code through a static checker after making
all planned changes and it had no complaints so it is now gone.

> 
>> +	freq = apic_cycles * divide_count * tsc_hz / tsc_cycles;
>> +	/* Check if measured frequency is within 1% of configured frequency. */
>> +	GUEST_ASSERT(freq < apic_hz * 101 / 100);
>> +	GUEST_ASSERT(freq > apic_hz * 99 / 100);
>> +}
>> +
>> +void x2apic_guest_code(void)
>> +{
>> +	uint32_t tmict, tmcct;
>> +	uint64_t tsc0, tsc1;
>> +	int i;
>> +
>> +	x2apic_enable();
>> +
>> +	/*
>> +	 * Setup one-shot timer.  The vector does not matter because the
>> +	 * interrupt should not fire.
>> +	 */
>> +	x2apic_write_reg(APIC_LVTT, APIC_LVT_TIMER_ONESHOT | APIC_LVT_MASKED);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tdcrs); i++) {
>> +		x2apic_write_reg(APIC_TDCR, tdcrs[i].tdcr);
>> +
>> +		/* Set the largest value to not trigger the interrupt. */
> 
> Nit, the goal isn't to avoid triggering the interrupt, e.g. the above masking
> takes care of that.  The goal is to prevent the timer from expiring, because if
> the timer expires it stops counting and will trigger a false failure because the
> TSC doesn't stop counting.
> 
> Honestly, I would just delete the comment.  Same with the "busy wait for 100 msec"
> and "Read APIC timer and TSC" comments.  They state the obvious.  Loading the max
> TMICT is mildly interesting, but that's covered by the file-level comment.
> 
>> +		tmict = ~0;
> 
> This really belongs outside of the loop, e.g.
> 
> 	const uint32_t tmict = ~0u;
> 
>> +		x2apic_write_reg(APIC_TMICT, tmict);
>> +
>> +		/* Busy wait for 100 msec. */
> 
> Hmm, should this be configurable?

Will do.

> 
>> +		tsc0 = rdtsc();
>> +		udelay(100000);
>> +		/* Read APIC timer and TSC. */
>> +		tmcct = x2apic_read_reg(APIC_TMCCT);
>> +		tsc1 = rdtsc();
>> +
>> +		/* Stop timer. */
> 
> This comment is a bit more interesting, as readers might not know writing '0'
> stops the timer.  But that's even more interesting is the ordering, e.g. it's
> not at all unreasonable to think that the timer should be stopped _before_ reading
> the current count.  E.g. something like:
> 
> 		/*
> 		 * Stop the timer _after_ reading the current, final count, as
> 		 * writing the initial counter also modifies the current count.
> 		 */
> 
>> +		x2apic_write_reg(APIC_TMICT, 0);
>> +
>> +		guest_verify(tsc1 - tsc0, tmict - tmcct, tdcrs[i].divide_count);
>> +	}
>> +
>> +	GUEST_DONE();
>> +}
>> +
>> +void xapic_guest_code(void)
>> +{
>> +	uint32_t tmict, tmcct;
>> +	uint64_t tsc0, tsc1;
>> +	int i;
>> +
>> +	xapic_enable();
>> +
>> +	/*
>> +	 * Setup one-shot timer.  The vector does not matter because the
>> +	 * interrupt should not fire.
>> +	 */
>> +	xapic_write_reg(APIC_LVTT, APIC_LVT_TIMER_ONESHOT | APIC_LVT_MASKED);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tdcrs); i++) {
>> +		xapic_write_reg(APIC_TDCR, tdcrs[i].tdcr);
>> +
>> +		/* Set the largest value to not trigger the interrupt. */
>> +		tmict = ~0;
>> +		xapic_write_reg(APIC_TMICT, tmict);
>> +
>> +		/* Busy wait for 100 msec. */
>> +		tsc0 = rdtsc();
>> +		udelay(100000);
>> +		/* Read APIC timer and TSC. */
>> +		tmcct = xapic_read_reg(APIC_TMCCT);
>> +		tsc1 = rdtsc();
>> +
>> +		/* Stop timer. */
>> +		xapic_write_reg(APIC_TMICT, 0);
>> +
>> +		guest_verify(tsc1 - tsc0, tmict - tmcct, tdcrs[i].divide_count);
> 
> That's some nice copy+paste :-)
> 
> This test isn't writing ICR, so the whole 32-bit vs. 64-bit weirdness with xAPIC
> vs X2APIC is irrevelant.  Two tiny helpers, a global flag, and you can avoid a
> pile of copy+paste, and the need to find a better name than guest_verify().

Will do. Thank you very much for your detailed and valuable feedback.

Reinette

Powered by blists - more mailing lists