Re: [kvm-unit-tests PATCH v3 3/4] arm: pmu: Add tests for 64-bit overflows

[Ricardo Koller <ricarkol@google.com>]

On Wed, Jan 25, 2023 at 08:55:50AM +0100, Eric Auger wrote:
> Hi,
> 
> On 1/25/23 05:11, Reiji Watanabe wrote:
> > On Tue, Jan 24, 2023 at 6:19 PM Ricardo Koller <ricarkol@google.com> wrote:
> >> On Wed, Jan 18, 2023 at 09:58:38PM -0800, Reiji Watanabe wrote:
> >>> Hi Ricardo,
> >>>
> >>> On Mon, Jan 9, 2023 at 1:18 PM Ricardo Koller <ricarkol@google.com> wrote:
> >>>> @@ -898,12 +913,12 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> >>>>
> >>>>         pmu_reset_stats();
> >>> This isn't directly related to the patch.
> >>> But, as bits of pmovsclr_el0 are already set (although interrupts
> >>> are disabled), I would think it's good to clear pmovsclr_el0 here.
> >>>
> >>> Thank you,
> >>> Reiji
> >>>
> >> There's no need in this case as there's this immediately before the
> >> pmu_reset_stats();
> >>
> >>         report(expect_interrupts(0), "no overflow interrupt after counting");
> >>
> >> so pmovsclr_el0 should be clear.
> > In my understanding, it means that no overflow *interrupt* was reported,
> > as the interrupt is not enabled yet (pmintenset_el1 is not set).
> > But, (as far as I checked the test case,) the both counters should be
> > overflowing here. So, pmovsclr_el0 must be 0x3.
> 
> I would tend to agree with Reiji.  The PMOVSSET_EL0<idx> will impact the
> next test according to aarch64/debug/pmu/AArch64.CheckForPMUOverflow and
> I think the overflow reg should be reset.

Ahh, right. The overflow interrupt could fire as soon as they are
enabled again. OK, will add a fix in the next version.

> 
> Eric
> >
> > Or am I misunderstanding something?
> >
> > Thank you,
> > Reiji
> >
> >
> >>>> -       write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >>>> -       write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> >>>> +       write_regn_el0(pmevcntr, 0, pre_overflow);
> >>>> +       write_regn_el0(pmevcntr, 1, pre_overflow);
> >>>>         write_sysreg(ALL_SET, pmintenset_el1);
> >>>>         isb();
> >>>>
>