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
>
