Re: [kvm-unit-tests PATCH v1 1/2] arm/pmu: skip the PMU introspection test if missing

[Eric Auger <eric.auger@redhat.com>]

Hi,

On 7/9/24 17:05, Alexandru Elisei wrote:
> Hi,
>
> On Tue, Jul 09, 2024 at 03:05:07PM +0100, Alex Bennée wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Tue, 9 Jul 2024 at 09:58, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>> Hi,
>>>>
>>>> On Tue, Jul 02, 2024 at 05:35:14PM +0100, Alex Bennée wrote:
>>>>> The test for number of events is not a substitute for properly
>>>>> checking the feature register. Fix the define and skip if PMUv3 is not
>>>>> available on the system. This includes emulator such as QEMU which
>>>>> don't implement PMU counters as a matter of policy.
>>>>>
>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>> Cc: Anders Roxell <anders.roxell@linaro.org>
>>>>> ---
>>>>>  arm/pmu.c | 7 ++++++-
>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arm/pmu.c b/arm/pmu.c
>>>>> index 9ff7a301..66163a40 100644
>>>>> --- a/arm/pmu.c
>>>>> +++ b/arm/pmu.c
>>>>> @@ -200,7 +200,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits) {}
>>>>>  #define ID_AA64DFR0_PERFMON_MASK  0xf
>>>>>
>>>>>  #define ID_DFR0_PMU_NOTIMPL  0b0000
>>>>> -#define ID_DFR0_PMU_V3               0b0001
>>>>> +#define ID_DFR0_PMU_V3               0b0011
>>>>>  #define ID_DFR0_PMU_V3_8_1   0b0100
>>>>>  #define ID_DFR0_PMU_V3_8_4   0b0101
>>>>>  #define ID_DFR0_PMU_V3_8_5   0b0110
>>>>> @@ -286,6 +286,11 @@ static void test_event_introspection(void)
>>>>>               return;
>>>>>       }
>>>>>
>>>>> +     if (pmu.version < ID_DFR0_PMU_V3) {
>>>>> +             report_skip("PMUv3 extensions not supported, skip ...");
>>>>> +             return;
>>>>> +     }
>>>>> +
>>>> I don't get this patch - test_event_introspection() is only run on 64bit. On
>>>> arm64, if there is a PMU present, that PMU is a PMUv3.  A prerequisite to
>>>> running any PMU tests is for pmu_probe() to succeed, and pmu_probe() fails if
>>>> there is no PMU implemented (PMUVer is either 0, or 0b1111). As a result, if
>>>> test_event_introspection() is executed, then a PMUv3 is present.
>>>>
>>>> When does QEMU advertise FEAT_PMUv3*, but no event counters (other than the cycle
>>>> counter)?
>> The other option I have is this:
>>
>> --8<---------------cut here---------------start------------->8---
>> arm/pmu: event-introspection needs icount for TCG
>>
>> The TCG accelerator will report a PMU (unless explicitly disabled with
>> -cpu foo,pmu=off) however not all events are available unless you run
>> under icount. Fix this by splitting the test into a kvm and tcg
>> version.
> As far as I can tell, if test_event_introspection() fails under TCG without
> icount then there are two possible explanations for that:
>
> 1. Not all the events whose presence is checked by test_event_introspection()
> are actually required by the architecture.
>
> 2. TCG without icount is not implementing all the events required by the
> architecture.
>
> If 1, then test_event_introspection() should be fixed. I had a look and the
> function looked correct to me (except that the event name is not INST_PREC,
> it's INST_SPEC in the Arm DDI0487J.A and K.a, but that's not relevant for
> correctness).
>
> From what I can tell from what Peter and you have said, explanation 2 is the
> correct one, because TCG cannot implement all the required events when icount is
> not specified. As far as test_event_introspection() is concerned, I consider
> this to be the expected behaviour: it fails because the required events are not
> implemented. I don't think the function should be changed to work around how
> QEMU was invoked. Do you agree?
>
> If you know that the test will fail without special command line parameters when
> accel is TCG, then I think what you are suggesting looks correct to me: the
> original test is skipped if KVM is not present, and when run under TCG, the
> correct parameters are passed to QEMU.
This looks sensible to me too

Eric
>
> Thanks,
> Alex
>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> 1 file changed, 8 insertions(+)
>> arm/unittests.cfg | 8 ++++++++
>>
>> modified   arm/unittests.cfg
>> @@ -52,8 +52,16 @@ extra_params = -append 'cycle-counter 0'
>>  file = pmu.flat
>>  groups = pmu
>>  arch = arm64
>> +accel = kvm
>>  extra_params = -append 'pmu-event-introspection'
>>  
>> +[pmu-event-introspection-icount]
>> +file = pmu.flat
>> +groups = pmu
>> +arch = arm64
>> +accel = tcg
>> +extra_params = -icount shift=1 -append 'pmu-event-introspection'
>> +
>>  [pmu-event-counter-config]
>>  file = pmu.flat
>>  groups = pmu
>> --8<---------------cut here---------------end--------------->8---
>>
>> which just punts icount on TCG to its own test (note there are commented
>> out versions further down the unitests.cfg file)
>>
>> -- 
>> Alex Bennée
>> Virtualisation Tech Lead @ Linaro