Re: [PATCH v2 2/5] target/arm: Ensure icount is enabled when emulating INST_RETIRED

[Peter Maydell]

On Fri, 8 Dec 2023 at 10:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 7/12/23 23:12, Richard Henderson wrote:
> > On 12/7/23 07:45, Philippe Mathieu-Daudé wrote:
> >> pmu_init() register its event checking the pm_event::supported()
> >> handler. For INST_RETIRED, the event is only registered and the
> >> bit enabled in the PMU Common Event Identification register when
> >> icount is enabled as ICOUNT_PRECISE.
> >>
> >> Assert the pm_event::get_count() and pm_event::ns_per_count()
> >> handler will only be called under this icount mode.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   target/arm/helper.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/target/arm/helper.c b/target/arm/helper.c
> >> index adb0960bba..333fd5f4bf 100644
> >> --- a/target/arm/helper.c
> >> +++ b/target/arm/helper.c
> >> @@ -940,11 +940,13 @@ static bool instructions_supported(CPUARMState
> >> *env)
> >>   static uint64_t instructions_get_count(CPUARMState *env)
> >>   {
> >> +    assert(icount_enabled() == ICOUNT_PRECISE);
> >>       return (uint64_t)icount_get_raw();
> >>   }
> >>   static int64_t instructions_ns_per(uint64_t icount)
> >>   {
> >> +    assert(icount_enabled() == ICOUNT_PRECISE);
> >>       return icount_to_ns((int64_t)icount);
> >>   }
> >>   #endif
> >
> > I don't think an assert is required -- that's exactly what the
> > .supported field is for. If you think this needs additional
> > clarification, a comment is sufficient.
>
> Without this I'm getting this link failure with TCG disabled:
>
> ld: Undefined symbols:
>    _icount_to_ns, referenced from:
>        _instructions_ns_per in target_arm_helper.c.o
> clang: error: linker command failed with exit code 1 (use -v to see
> invocation)

I think we should fix this earlier by not trying to enable
these TCG-only PMU event types in a non-TCG config.

-- PMM