Hi Sean,
On 6/5/24 7:22 AM, Sean Christopherson wrote:
On Tue, Jun 04, 2024, Reinette Chatre wrote:
+/*
+ * Pick one convenient value, 1.5GHz. No special meaning and different from
+ * the default value, 1GHz.
I have no idea where the 1GHz comes from. KVM doesn't force a default TSC, KVM
uses the underlying CPU's frequency. Peeking further ahead, I don't understand
why this test sets KVM_SET_TSC_KHZ. That brings in a whole different set of
behavior, and that behavior is already verified by tsc_scaling_sync.c.
I suspect/assume this test forces a frequency so that it can hardcode the math,
but (a) that's odd and (b) x86 selftests really should provide a udelay() so that
goofy stuff like this doesn't end up in random tests.
I believe the "default 1GHz" actually refers to the default APIC bus frequency and
the goal was indeed to (a) make the TSC frequency different from APIC bus frequency,
and (b) make math easier.
Yes, there is no need to use KVM_SET_TSC_KHZ. An implementation of udelay() would
require calibration and to make this simple for KVM I think we can just use
KVM_GET_TSC_KHZ. For now I continue to open code this (see later) since I did not
notice similar patterns in existing tests that may need a utility. I'd be happy
to add a utility if the needed usage pattern is clear since the closest candidate
I could find was xapic_ipi_test.c that does not have a nop loop.
Please add a utility. ARM and RISC-V already implement udelay(), and this isn't
the first test that's wanted udelay() functionality. At the very least, it'd be
nice to have for debug, e.g. to be able to insert artificial delay if a test is
failing due to a suspected race suspected. I.e. this is likely an "if you build
it, they will come" situations.
Will do.
Unless I'm misremembering, the timer still counts when the LVT entry is masked
so just mask the IRQ in the LVT. Or rather, keep the entry masked in the LVT.
hmmm ... I do not think this is specific to LVT entry but instead an attempt
to ignore all maskable external interrupt that may interfere with the test.
I doubt it. And if that really is the motiviation, that's a fools errand. This
is _guest_ code. Disabling IRQs in the guest doesn't prevent host IRQs from being
delivered, it only blocks virtual IRQs. And KVM selftests guests should never
receive virtual IRQs unless the test itself explicitly sends them.
Thank you for clarifying.
LVT entry is prevented from triggering because if the large configuration value.
Yes and no. A large configuration value _should_ avoid a timer IRQ, but the
entire point of this test is to verify KVM correctly emulates the timer. If this
test does its job and finds a KVM bug that causes the timer to prematurely expire,
then unmasking the LVT entry will generate an unexpected IRQ.
Of course, the test doesn't configure a legal vector so the IRQ will never be
delivered. We could fix that problem, but then a test failure would manifest as
a hard-to-triage unexpected event, compared to an explicit TEST_ASSERT() on the
timer value.
That said, I'm not totally pposed to letting the guest die if KVM unexpectedly
injects a timer IRQ, e.g. if all is well, it's a cheap way to provide a bit of
extra coverage. On the other hand, masking the interrupt is even simpler, and
the odds of false pass are low.
Understood. Will mask the interrupt in LVT entry as you suggested initially. While
checking which bits to set I realized that the existing test was enabling oneshot
mode in an entry where those bits are actually reserved. This is now fixed also.
I plan to send the changes as next version of this work, with merged patches
dropped from series.
Reinette
