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

On 8/6/21 4:08 PM, Nicholas Piggin wrote:
> Excerpts from Madhavan Srinivasan's message of August 6, 2021 5:33 pm:
>> On 7/26/21 9:19 AM, Nicholas Piggin 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.
>>
>> This is bit confusing, IIUC, we are manually disabling the perf
>> registration
>> with this option and not pmu.
>> If this option is used, we will unfreeze the
>> MMCR0_FC (only in the HV_mode) and not register perf subsystem.
> With the previous patch, this option un-freezes the PMU
> (and disables perf).
>
>> Since this option is valid only for HV_mode, canwe call it
>> kvm_disable_perf or kvm_dis_perf.
> It's only disabled for guests because it would require a bit
> of logic to set pmcregs_in_use when we register our lppaca. We could
> add that if needed, but the intention is for use on BML, not exactly
> KVM specific.
>
> I can add HV restriction to the help text. And we could rename the
> option. free_run_pmu= or something?


yeah having it a different name will be better. I am not sure
whether we should say "[PPC] Manually enable the PMU",
because IIUC, if we dont provide this option PMU and perf is
anyway enabled, but rest looks good to me.

Maddy


>
> Thanks,
> Nick
>
>>
>>> +			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);
>>> +}
>>> +
>>>    static int __init init_ppc64_pmu(void)
>>>    {
>>> +	if (cpu_has_feature(CPU_FTR_HVMODE) && pmu_override) {
>>> +		printk(KERN_WARNING "perf: disabling perf due to pmu= command line option.\n");
>>> +		on_each_cpu(do_pmu_override, NULL, 1);
>>> +		return 0;
>>> +	}
>>> +
>>>    	/* run through all the pmu drivers one at a time */
>>>    	if (!init_power5_pmu())
>>>    		return 0;
>>> @@ -2451,4 +2467,23 @@ static int __init init_ppc64_pmu(void)
>>>    		return init_generic_compat_pmu();
>>>    }
>>>    early_initcall(init_ppc64_pmu);
>>> +
>>> +static int __init pmu_setup(char *str)
>>> +{
>>> +	unsigned long val;
>>> +
>>> +	if (!early_cpu_has_feature(CPU_FTR_HVMODE))
>>> +		return 0;
>>> +
>>> +	pmu_override = true;
>>> +
>>> +	if (kstrtoul(str, 0, &val))
>>> +		val = 0;
>>> +
>>> +	pmu_override_val = val;
>>> +
>>> +	return 1;
>>> +}
>>> +__setup("pmu=", pmu_setup);
>>> +
>>>    #endif