Re: [kvm-unit-tests PATCH v2 4/6] arm: pmu: Fix chain counter enable/disable sequences

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On Wed, May 31, 2023 at 10:14:36PM +0200, Eric Auger wrote:
> In some ARM ARM ddi0487 revisions it is said that
> disabling/enabling a pair of counters that are paired
> by a CHAIN event should follow a given sequence:
> 
> Enable the high counter first, isb, enable low counter
> Disable the low counter first, isb, disable the high counter
> 
> This was the case in Fc. However this is not written anymore
> in subsequent revions such as Ia.
> 
> Anyway, just in case, and because it also makes the code a
> little bit simpler, introduce 2 helpers to enable/disable chain
> counters that execute those sequences and replace the existing
> PMCNTENCLR/ENSET calls (at least this cannot do any harm).
> 
> Also fix 2 write_sysreg_s(0x0, PMCNTENSET_EL0) in subtest 5 & 6
> and replace them by PMCNTENCLR writes since writing 0 in
> PMCNTENSET_EL0 has no effect.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - fix the enable_chain_counter()/disable_chain_counter()
>   sequence, ie. swap n + 1 / n as reported by Alexandru.
> - fix an other comment using the 'low' terminology
> ---
>  arm/pmu.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 74dd4c10..74c9f6f9 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -731,6 +731,22 @@ static void test_chained_sw_incr(bool unused)
>  		    read_regn_el0(pmevcntr, 0), \
>  		    read_sysreg(pmovsclr_el0))
>  
> +static void enable_chain_counter(int even)
> +{
> +	write_sysreg_s(BIT(even + 1), PMCNTENSET_EL0); /* Enable the high counter first */
> +	isb();
> +	write_sysreg_s(BIT(even), PMCNTENSET_EL0); /* Enable the low counter */
> +	isb();
> +}
> +
> +static void disable_chain_counter(int even)
> +{
> +	write_sysreg_s(BIT(even), PMCNTENCLR_EL0); /* Disable the low counter first*/
> +	isb();
> +	write_sysreg_s(BIT(even + 1), PMCNTENCLR_EL0); /* Disable the high counter */
> +	isb();
> +}
> +
>  static void test_chain_promotion(bool unused)

Here's what test_chain_promotion() does for the first subtest:

static void test_chain_promotion(bool unused)
{
        uint32_t events[] = {MEM_ACCESS, CHAIN};
        void *addr = malloc(PAGE_SIZE);

        if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
                return;

        /* Only enable CHAIN counter */
        report_prefix_push("subtest1");
        pmu_reset();
        write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
        write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
        write_sysreg_s(0x2, PMCNTENSET_EL0);
        isb();

        mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);

And here's what test_chained_counters() does:

static void test_chained_counters(bool unused)
{
        uint32_t events[] = {CPU_CYCLES, CHAIN};
        uint64_t all_set = pmevcntr_mask();

        if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
                return;

        pmu_reset();

        write_regn_el0(pmevtyper, 0, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
        write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
        /* enable counters #0 and #1 */
        write_sysreg_s(0x3, PMCNTENSET_EL0);
        write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);

        precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);

Why the extra ISB in test_chain_promotion()? Or, if you want to look at it
the other way around, is the ISB missing from test_chained_counters()?

>  {
>  	uint32_t events[] = {MEM_ACCESS, CHAIN};
> @@ -769,16 +785,17 @@ static void test_chain_promotion(bool unused)
>  	/* 1st COUNT with CHAIN enabled, next COUNT with CHAIN disabled */
>  	report_prefix_push("subtest3");
>  	pmu_reset();
> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
> -	isb();
> +	enable_chain_counter(0);
>  	PRINT_REGS("init");

Here's how subtest3 ends up looking:

        report_prefix_push("subtest3");
        pmu_reset();
        write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
        enable_chain_counter(0);
        PRINT_REGS("init");

        mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);

And here's something similar from test_chained_counters():

        pmu_reset();
        write_sysreg_s(0x3, PMCNTENSET_EL0);

        write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
        write_regn_el0(pmevcntr, 1, 0x1);
        precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);


Why does test_chain_promotion() use enable_chain_counter() and
test_chained_counters() doesn't?

Could probably find more examples of this in test_chain_promotion().

As an aside, it's extremely difficult to figure out how the counters are
programmed for a subtest. In the example above, you need to go back 2
subtests, to the start of test_chain_promotion(), to figure that out. And
that only gets worse the subtest number increases. test_chain_promotion()
would really benefit from being split into separate functions, each with
each own clear initial state. But that's for another patch, not for this
series.

Thanks,
Alex

>  
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
>  	/* disable the CHAIN event */
> -	write_sysreg_s(0x2, PMCNTENCLR_EL0);
> +	disable_chain_counter(0);
> +	write_sysreg_s(0x1, PMCNTENSET_EL0); /* Enable the low counter */
> +	isb();
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 2nd loop");
>  	report(read_sysreg(pmovsclr_el0) == 0x1,
> @@ -799,9 +816,11 @@ static void test_chain_promotion(bool unused)
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
> -	/* enable the CHAIN event */
> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	/* Disable the low counter first and enable the chain counter */
> +	write_sysreg_s(0x1, PMCNTENCLR_EL0);
>  	isb();
> +	enable_chain_counter(0);
> +
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  
>  	PRINT_REGS("After 2nd loop");
> @@ -825,10 +844,10 @@ static void test_chain_promotion(bool unused)
>  	PRINT_REGS("After 1st loop");
>  
>  	/* 0 becomes CHAINED */
> -	write_sysreg_s(0x0, PMCNTENSET_EL0);
> +	write_sysreg_s(0x3, PMCNTENCLR_EL0);
>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	write_regn_el0(pmevcntr, 1, 0x0);
> +	enable_chain_counter(0);
>  
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 2nd loop");
> @@ -844,13 +863,13 @@ static void test_chain_promotion(bool unused)
>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	enable_chain_counter(0);
>  	PRINT_REGS("init");
>  
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
> -	write_sysreg_s(0x0, PMCNTENSET_EL0);
> +	disable_chain_counter(0);
>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
> -- 
> 2.38.1
>