> On 06-Aug-2021, at 4:12 PM, Nicholas Piggin <npiggin@xxxxxxxxx> wrote:
>
> Excerpts from Athira Rajeev's message of August 6, 2021 7:28 pm:
>>
>>
>>> On 26-Jul-2021, at 9:19 AM, Nicholas Piggin <npiggin@xxxxxxxxx> wrote:
>>>
>>> It can be useful in simulators (with very constrained environments)
>>> to allow some PMCs to run from boot so they can be sampled directly
>>> by a test harness, rather than having to run perf.
>>>
>>> A previous change freezes counters at boot by default, so provide
>>> a boot time option to un-freeze (plus a bit more flexibility).
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx>
>>> ---
>>> .../admin-guide/kernel-parameters.txt | 7 ++++
>>> arch/powerpc/perf/core-book3s.c | 35 +++++++++++++++++++
>>> 2 files changed, 42 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>> index bdb22006f713..96b7d0ebaa40 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -4089,6 +4089,13 @@
>>> Override pmtimer IOPort with a hex value.
>>> e.g. pmtmr=0x508
>>>
>>> + pmu= [PPC] Manually enable the PMU.
>>> + Enable the PMU by setting MMCR0 to 0 (clear FC bit).
>>> + This option is implemented for Book3S processors.
>>> + If a number is given, then MMCR1 is set to that number,
>>> + otherwise (e.g., 'pmu=on'), it is left 0. The perf
>>> + subsystem is disabled if this option is used.
>>> +
>>> pm_debug_messages [SUSPEND,KNL]
>>> Enable suspend/resume debug messages during boot up.
>>>
>>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>>> index 65795cadb475..e7cef4fe17d7 100644
>>> --- a/arch/powerpc/perf/core-book3s.c
>>> +++ b/arch/powerpc/perf/core-book3s.c
>>> @@ -2428,8 +2428,24 @@ int register_power_pmu(struct power_pmu *pmu)
>>> }
>>>
>>> #ifdef CONFIG_PPC64
>>> +static bool pmu_override = false;
>>> +static unsigned long pmu_override_val;
>>> +static void do_pmu_override(void *data)
>>> +{
>>> + ppc_set_pmu_inuse(1);
>>> + if (pmu_override_val)
>>> + mtspr(SPRN_MMCR1, pmu_override_val);
>>> + mtspr(SPRN_MMCR0, mfspr(SPRN_MMCR0) & ~MMCR0_FC);
>>
>> Hi Nick
>>
>> Here, we are not doing any validity check for the value used to set MMCR1.
>> For advanced users, the option to pass value for MMCR1 is fine. But other cases, it could result in
>> invalid event getting used. Do we need to restrict this boot time option for only PMC5/6 ?
>
> Depends what would be useful. We don't have to prevent the admin shooting
> themselves in the foot with options like this, but if we can make it
> safer without making it less useful then that's always a good option.
Hi Nick
I checked back on my comment and it will be difficult to add/maintain validity check for MMCR1 considering different platforms that we have.
We can go ahead with present approach you have in this patch. Changes looks good to me.
Reviewed-by: Athira Rajeev <atrajeev@xxxxxxxxxxxxxxxxxx>
>
> Thanks,
> Nick
