[kvm-unit-tests PATCH v1 0/2] Some fixes for running under -cpu max on QEMU

[kvm-unit-tests PATCH v1 0/2] Some fixes for running under -cpu max on QEMU

[Alex Bennée]

Hi,

The following fixes try and make the experience of QEMU -cpu max a bit
smoother by actually checking the PMU versions supported. You can also
set -cpu max,pmu=off to fully hide PMU functionality from the
processor.

As max includes all the features we also need to take into account the
additional TGran values you can have with 52 bit addressing.

Please review,


Alex Bennée (2):
  arm/pmu: skip the PMU introspection test if missing
  arm/mmu: widen the page size check to account for LPA2

 lib/arm64/asm/processor.h | 29 ++++++++++++++---------------
 arm/pmu.c                 |  7 ++++++-
 2 files changed, 20 insertions(+), 16 deletions(-)

-- 
2.39.2

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

[Alex Bennée]

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;
+	}
+
 	/* PMUv3 requires an implementation includes some common events */
 	required_events = is_event_supported(SW_INCR, true) &&
 			  is_event_supported(CPU_CYCLES, true) &&
-- 
2.39.2

[kvm-unit-tests PATCH v1 2/2] arm/mmu: widen the page size check to account for LPA2

[Alex Bennée]

If FEAT_LPA2 is enabled there are different valid TGran values
possible to indicate the granule is supported for 52 bit addressing.
This will cause most tests to abort on QEMU's -cpu max with the error:

  lib/arm/mmu.c:216: assert failed: system_supports_granule(PAGE_SIZE): Unsupported translation granule 4096

Expand the test to tale this into account.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Anders Roxell <anders.roxell@linaro.org>
---
 lib/arm64/asm/processor.h | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 1c73ba32..4a213aec 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -110,31 +110,30 @@ static inline unsigned long get_id_aa64mmfr0_el1(void)
 #define ID_AA64MMFR0_TGRAN64_SHIFT	24
 #define ID_AA64MMFR0_TGRAN16_SHIFT	20
 
-#define ID_AA64MMFR0_TGRAN4_SUPPORTED	0x0
-#define ID_AA64MMFR0_TGRAN64_SUPPORTED	0x0
-#define ID_AA64MMFR0_TGRAN16_SUPPORTED	0x1
+#define ID_AA64MMFR0_TGRAN4_OK	        0x0
+#define ID_AA64MMFR0_TGRAN4_52_OK       0x1
+#define ID_AA64MMFR0_TGRAN64_OK	0x0
+#define ID_AA64MMFR0_TGRAN16_OK	        0x1
+#define ID_AA64MMFR0_TGRAN16_52_OK      0x2
 
 static inline bool system_supports_granule(size_t granule)
 {
-	u32 shift;
 	u32 val;
-	u64 mmfr0;
+	u64 mmfr0 = get_id_aa64mmfr0_el1();
 
 	if (granule == SZ_4K) {
-		shift = ID_AA64MMFR0_TGRAN4_SHIFT;
-		val = ID_AA64MMFR0_TGRAN4_SUPPORTED;
+		val = ((mmfr0 >> ID_AA64MMFR0_TGRAN4_SHIFT) & 0xf);
+		return (val == ID_AA64MMFR0_TGRAN4_OK) ||
+		       (val == ID_AA64MMFR0_TGRAN4_52_OK);
 	} else if (granule == SZ_16K) {
-		shift = ID_AA64MMFR0_TGRAN16_SHIFT;
-		val = ID_AA64MMFR0_TGRAN16_SUPPORTED;
+		val = ((mmfr0 >> ID_AA64MMFR0_TGRAN16_SHIFT) & 0xf);
+		return val == ID_AA64MMFR0_TGRAN16_OK;
 	} else {
 		assert(granule == SZ_64K);
-		shift = ID_AA64MMFR0_TGRAN64_SHIFT;
-		val = ID_AA64MMFR0_TGRAN64_SUPPORTED;
+		val = ((mmfr0 >> ID_AA64MMFR0_TGRAN64_SHIFT) & 0xf);
+		return (val == ID_AA64MMFR0_TGRAN64_OK) ||
+		       (val == ID_AA64MMFR0_TGRAN4_52_OK);
 	}
-
-	mmfr0 = get_id_aa64mmfr0_el1();
-
-	return ((mmfr0 >> shift) & 0xf) == val;
 }
 
 #endif /* !__ASSEMBLY__ */
-- 
2.39.2

Re: [kvm-unit-tests PATCH v1 2/2] arm/mmu: widen the page size check to account for LPA2

[Zenghui Yu]

Hi Alex,

[ Please don't send patches to the old kvmarm@lists.cs.columbia.edu as
it had been dropped since early 2023. [1] ]

On 2024/7/3 0:35, Alex Bennée wrote:
> If FEAT_LPA2 is enabled there are different valid TGran values
> possible to indicate the granule is supported for 52 bit addressing.
> This will cause most tests to abort on QEMU's -cpu max with the error:
> 
>   lib/arm/mmu.c:216: assert failed: system_supports_granule(PAGE_SIZE): Unsupported translation granule 4096
> 
> Expand the test to tale this into account.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Anders Roxell <anders.roxell@linaro.org>

There's a similar patch on the list [2], haven't been merged in master
though.

[1] https://git.kernel.org/torvalds/c/960c3028a1d5
[2] 
https://lore.kernel.org/all/20240402132739.201939-6-andrew.jones@linux.dev

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

[Zenghui Yu]

On 2024/7/3 0:35, 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

Why? This is a macro used for AArch64 and DDI0487J.a (D19.2.59, the
description of the PMUVer field) says that

"0b0001	Performance Monitors Extension, PMUv3 implemented."

while 0b0011 is a reserved value.

Thanks,
Zenghui

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

[Marc Zyngier]

On 2024-07-03 08:09, Zenghui Yu wrote:
> On 2024/7/3 0:35, 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
> 
> Why? This is a macro used for AArch64 and DDI0487J.a (D19.2.59, the
> description of the PMUVer field) says that
> 
> "0b0001	Performance Monitors Extension, PMUv3 implemented."
> 
> while 0b0011 is a reserved value.

I think this is a mix of 32bit and 64bit views (ID_DFR0_EL1.PerfMon
instead of ID_AA64DFR0_EL1.PMUVer), and the whole thing is a mess
(ID_AA64DFR0_PERFMON_MASK is clearly confused...).

I haven't looked at how this patch fits in the rest of the code though.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [kvm-unit-tests PATCH v1 2/2] arm/mmu: widen the page size check to account for LPA2

[Andrew Jones]

On Wed, Jul 03, 2024 at 11:52:05AM GMT, Zenghui Yu wrote:
> Hi Alex,
> 
> [ Please don't send patches to the old kvmarm@lists.cs.columbia.edu as
> it had been dropped since early 2023. [1] ]
> 
> On 2024/7/3 0:35, Alex Bennée wrote:
> > If FEAT_LPA2 is enabled there are different valid TGran values
> > possible to indicate the granule is supported for 52 bit addressing.
> > This will cause most tests to abort on QEMU's -cpu max with the error:
> > 
> >   lib/arm/mmu.c:216: assert failed: system_supports_granule(PAGE_SIZE): Unsupported translation granule 4096
> > 
> > Expand the test to tale this into account.
> > 
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > Cc: Anders Roxell <anders.roxell@linaro.org>
> 
> There's a similar patch on the list [2], haven't been merged in master
> though.

Drat, I queued that, and several other patches, and then, for whatever
reason, I delayed the merge (I was probably just waiting for the gitlab
pipeline to finish...) and then forgot to actually merge... I've merged
now.

Please don't hesitate to ping me on patches that linger too long. I
sometimes need that interrupt to trigger my context switch!

Thanks,
drew

> 
> [1] https://git.kernel.org/torvalds/c/960c3028a1d5
> [2]
> https://lore.kernel.org/all/20240402132739.201939-6-andrew.jones@linux.dev

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

[Alex Bennée]

Marc Zyngier <maz@kernel.org> writes:

> On 2024-07-03 08:09, Zenghui Yu wrote:
>> On 2024/7/3 0:35, 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
>> Why? This is a macro used for AArch64 and DDI0487J.a (D19.2.59, the
>> description of the PMUVer field) says that
>> "0b0001	Performance Monitors Extension, PMUv3 implemented."
>> while 0b0011 is a reserved value.
>
> I think this is a mix of 32bit and 64bit views (ID_DFR0_EL1.PerfMon
> instead of ID_AA64DFR0_EL1.PMUVer), and the whole thing is a mess
> (ID_AA64DFR0_PERFMON_MASK is clearly confused...).
>
> I haven't looked at how this patch fits in the rest of the code
> though.

Doh - yes different set of values for 32 bit.

>
>         M.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Alexandru Elisei]

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)?

If you want to be extra correct, you can add the above check to pmu_probe() for
32bit, since I doubt that the PMU tests were designed or tested on anything
other than a PMUv3 (and probably not much interest to maintain the tests for
PMUv1 or v2 either). If do do this, may I suggest:

#if defined(__arm__)
	if (pmu.version == ID_DFR0_PMU_V1 || pmu.version == ID_DFR0_PMU_V2)
		return false;
#endif

That way the check is self documenting.

Thanks,
Alex

>  	/* PMUv3 requires an implementation includes some common events */
>  	required_events = is_event_supported(SW_INCR, true) &&
>  			  is_event_supported(CPU_CYCLES, true) &&
> -- 
> 2.39.2
> 

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

[Peter Maydell]

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)?

When we're using TCG but not icount mode we present only the SW_INCR,
CPU_CYCLES, STALL_FRONTEND, STALL_BACKEND and STALL events.
If we aren't counting instructions we can't present an INST_RETIRED
event, because we don't have the information. (The STALL events
always return a zero count.)

thanks
-- PMM

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

[Alex Bennée]

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.

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

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

[Alexandru Elisei]

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.

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

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

[Eric Auger]

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