Re: [PATCH v1 16/55] powerpc/64s: Implement PMU override command line option

> On 06-Aug-2021, at 4:12 PM, Nicholas Piggin <[email protected]> wrote:
> 
> Excerpts from Athira Rajeev's message of August 6, 2021 7:28 pm:
>> 
>> 
>>> On 26-Jul-2021, at 9:19 AM, Nicholas Piggin <[email protected]> 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 <[email protected]>
>>> ---
>>> .../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 <[email protected]>

> 
> Thanks,
> Nick