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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Peter,

On 8/12/23 11:59, Peter Maydell wrote:
> 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.

I agree... but (as discussed yesterday on IRC), this is a bigger rework.

This icount cleanup blocks 60+ patches on top :/ Since I have a v3
addressing Richard's comments already done, I'll post it, mentioning the
PMU issue in the cover; then see if cleaning it isn't too invasive.
If I end in another rabbit hole, I'll suggest to accept this current
patch and clean the technical debt later, again.

Thanks,

Phil.