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

[Reinette Chatre <reinette.chatre@intel.com>]

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