[PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[Marcin Juszkiewicz]

Arm BSA (Base System Architecture) specification says:

B_PE_09: PEs must implement the FEAT_PMUv3p1 extension, and the base
system must expose a minimum of four programmable PMU counters to the
operating system.

B_PE_21: The base system must expose a minimum of two programmable PMU
counters to a hypervisor.

It is then repeated in SBSA (Server Base System Architecture)
specification in level 3 requirements:

Each PE must implement a minimum of six programmable PMU counters.

So let make QEMU provide those 6 PMU counters.

SBSA-ACS says now:

  12 : Check number of PMU counters      : Result:  PASS

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0e1a3b9421..02e25b5c22 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -38,7 +38,7 @@
 #endif
 
 #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
-#define PMCR_NUM_COUNTERS 4 /* QEMU IMPDEF choice */
+#define PMCR_NUM_COUNTERS 6 /* QEMU IMPDEF choice */
 
 #ifndef CONFIG_USER_ONLY
 
-- 
2.29.2

Re: [PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[Leif Lindholm]

On Wed, Mar 03, 2021 at 16:16:34 +0100, Marcin Juszkiewicz wrote:
> Arm BSA (Base System Architecture) specification says:
> 
> B_PE_09: PEs must implement the FEAT_PMUv3p1 extension, and the base
> system must expose a minimum of four programmable PMU counters to the
> operating system.
> 
> B_PE_21: The base system must expose a minimum of two programmable PMU
> counters to a hypervisor.
> 
> It is then repeated in SBSA (Server Base System Architecture)
> specification in level 3 requirements:
> 
> Each PE must implement a minimum of six programmable PMU counters.
> 
> So let make QEMU provide those 6 PMU counters.
> 
> SBSA-ACS says now:
> 
>   12 : Check number of PMU counters      : Result:  PASS
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

It would be good if we could get 6.0 closer to SBSA compliance.
Would it be worth the effort to make this controllable per cpu model?

/
    Leif

> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0e1a3b9421..02e25b5c22 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -38,7 +38,7 @@
>  #endif
>  
>  #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
> -#define PMCR_NUM_COUNTERS 4 /* QEMU IMPDEF choice */
> +#define PMCR_NUM_COUNTERS 6 /* QEMU IMPDEF choice */
>  
>  #ifndef CONFIG_USER_ONLY
>  
> -- 
> 2.29.2
> 

Re: [PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[Peter Maydell]

On Wed, 3 Mar 2021 at 17:48, Leif Lindholm <leif@nuviainc.com> wrote:
> It would be good if we could get 6.0 closer to SBSA compliance.

How far away are we at the moment ?

> Would it be worth the effort to make this controllable per cpu model?

I don't have a strong opinion on whether we should, but if we do then the
right way to implement that would be to have the PMCR reset value
as a reset_pmcr_el0 field in struct ARMCPU (like the existing reset_fpsid,
reset_sctlr, etc) that gets set per-CPU to whatever the CPU's value for
it is; and then instead of using a PMCR_NUM_COUNTERS value,
extract the PMCR.N field when needed. The hardest part would be
going through all the CPU TRMs to find out the correct reset value.

thanks
-- PMM

Re: [PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[Marcin Juszkiewicz]

W dniu 03.03.2021 o 19:06, Peter Maydell pisze:
> On Wed, 3 Mar 2021 at 17:48, Leif Lindholm <leif@nuviainc.com> wrote:
>> It would be good if we could get 6.0 closer to SBSA compliance.
> 
> How far away are we at the moment ?

Hard to tell me how many of those things are missing in QEMU and how 
many in EDK2 we use as firmware.

SBSA-ACS failures:

GIC ITS is missing (Shashi Mallela works on it):

  102 : If PCIe, then GIC implements ITS  : Result:  --FAIL-- 1
  104 : GIC Maintenance Interrupt         : Result:  --FAIL-- 1


System timers are not present in GTDT so few more tests are not run:

  206 : SYS Timer if PE Timer not ON      : Result:  --FAIL-- 1
        PE Timers are not always-on.
  207 : CNTCTLBase & CNTBaseN access      : Result:  -SKIPPED- 1
        No System timers are defined
  505 : Wake from System Timer Interrupt  : Result:  -SKIPPED- 1
        No system timers implemented


There is no SMMU present so SMMU and IO virtualization tests are not 
run. This one is probably related:

  605 : Memory Access to Un-Populated addr: Result:  --FAIL-- 1
        Memory access check fails at address = 0x104C0000

Re: [PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[Leif Lindholm]

On Wed, Mar 03, 2021 at 18:06:46 +0000, Peter Maydell wrote:
> On Wed, 3 Mar 2021 at 17:48, Leif Lindholm <leif@nuviainc.com> wrote:
> > It would be good if we could get 6.0 closer to SBSA compliance.
> 
> How far away are we at the moment ?
> 
> > Would it be worth the effort to make this controllable per cpu model?
> 
> I don't have a strong opinion on whether we should, but if we do then the
> right way to implement that would be to have the PMCR reset value
> as a reset_pmcr_el0 field in struct ARMCPU (like the existing reset_fpsid,
> reset_sctlr, etc) that gets set per-CPU to whatever the CPU's value for
> it is; and then instead of using a PMCR_NUM_COUNTERS value,
> extract the PMCR.N field when needed. The hardest part would be
> going through all the CPU TRMs to find out the correct reset value.

That makes sense.

I guess we could also phase the transition by using the default value
if zero?

Regards,

Leif

Re: [PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[Peter Maydell]

On Thu, 4 Mar 2021 at 13:53, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Wed, Mar 03, 2021 at 18:06:46 +0000, Peter Maydell wrote:
> > On Wed, 3 Mar 2021 at 17:48, Leif Lindholm <leif@nuviainc.com> wrote:
> > > It would be good if we could get 6.0 closer to SBSA compliance.
> >
> > How far away are we at the moment ?
> >
> > > Would it be worth the effort to make this controllable per cpu model?
> >
> > I don't have a strong opinion on whether we should, but if we do then the
> > right way to implement that would be to have the PMCR reset value
> > as a reset_pmcr_el0 field in struct ARMCPU (like the existing reset_fpsid,
> > reset_sctlr, etc) that gets set per-CPU to whatever the CPU's value for
> > it is; and then instead of using a PMCR_NUM_COUNTERS value,
> > extract the PMCR.N field when needed. The hardest part would be
> > going through all the CPU TRMs to find out the correct reset value.
>
> That makes sense.
>
> I guess we could also phase the transition by using the default value
> if zero?

I tend to prefer to avoid that kind of transitional thing, because
as a project we have a tendency to never complete transitions. The
PMU stuff only applies to the v7 and v8 cores, and we don't implement
that many of them, so it's better to just make the effort to find out
the correct PMCR reset value for them and be done with it.

-- PMM

Re: [PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[Leif Lindholm]

On Thu, Mar 04, 2021 at 15:14:36 +0000, Peter Maydell wrote:
> On Thu, 4 Mar 2021 at 13:53, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > On Wed, Mar 03, 2021 at 18:06:46 +0000, Peter Maydell wrote:
> > > On Wed, 3 Mar 2021 at 17:48, Leif Lindholm <leif@nuviainc.com> wrote:
> > > > It would be good if we could get 6.0 closer to SBSA compliance.
> > >
> > > How far away are we at the moment ?
> > >
> > > > Would it be worth the effort to make this controllable per cpu model?
> > >
> > > I don't have a strong opinion on whether we should, but if we do then the
> > > right way to implement that would be to have the PMCR reset value
> > > as a reset_pmcr_el0 field in struct ARMCPU (like the existing reset_fpsid,
> > > reset_sctlr, etc) that gets set per-CPU to whatever the CPU's value for
> > > it is; and then instead of using a PMCR_NUM_COUNTERS value,
> > > extract the PMCR.N field when needed. The hardest part would be
> > > going through all the CPU TRMs to find out the correct reset value.
> >
> > That makes sense.
> >
> > I guess we could also phase the transition by using the default value
> > if zero?
> 
> I tend to prefer to avoid that kind of transitional thing, because
> as a project we have a tendency to never complete transitions. The
> PMU stuff only applies to the v7 and v8 cores, and we don't implement
> that many of them, so it's better to just make the effort to find out
> the correct PMCR reset value for them and be done with it.

Understood.

I'll throw this on my never-shrinking pile of things I hope to get
around to at some point.

/
    Leif

Re: [PATCH] arm: bump amount of PMU counters to pass SBSA ACS

[Peter Maydell]

On Thu, 4 Mar 2021 at 15:25, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Thu, Mar 04, 2021 at 15:14:36 +0000, Peter Maydell wrote:
> > On Thu, 4 Mar 2021 at 13:53, Leif Lindholm <leif@nuviainc.com> wrote:
> > >
> > > On Wed, Mar 03, 2021 at 18:06:46 +0000, Peter Maydell wrote:
> > > > On Wed, 3 Mar 2021 at 17:48, Leif Lindholm <leif@nuviainc.com> wrote:
> > > > > It would be good if we could get 6.0 closer to SBSA compliance.
> > > >
> > > > How far away are we at the moment ?
> > > >
> > > > > Would it be worth the effort to make this controllable per cpu model?
> > > >
> > > > I don't have a strong opinion on whether we should, but if we do then the
> > > > right way to implement that would be to have the PMCR reset value
> > > > as a reset_pmcr_el0 field in struct ARMCPU (like the existing reset_fpsid,
> > > > reset_sctlr, etc) that gets set per-CPU to whatever the CPU's value for
> > > > it is; and then instead of using a PMCR_NUM_COUNTERS value,
> > > > extract the PMCR.N field when needed. The hardest part would be
> > > > going through all the CPU TRMs to find out the correct reset value.
> > >
> > > That makes sense.

> I'll throw this on my never-shrinking pile of things I hope to get
> around to at some point.

I just sent a patch that does it that way, though it is desperately
in need of more testing. I'm currently on the fence about whether to
put this patch or that one in for 6.0.

thanks
-- PMM