Re: [PATCH v5 1/2] arm64: perf: Add support caps in sysfs

[Shaokun Zhang <zhangshaokun@hisilicon.com>]

Hi Will,

在 2020/7/21 19:56, Will Deacon 写道:
> Hi Shaokun,
> 
> On Tue, Jul 21, 2020 at 06:49:32PM +0800, Shaokun Zhang wrote:
>> ARMv8.4-PMU introduces the PMMIR_EL1 registers and some new PMU events,
>> like STALL_SLOT etc, are related to it. Let's add a caps directory to
>> /sys/bus/event_source/devices/armv8_pmuv3_0/ and support slots from
>> PMMIR_EL1 registers in this entry. The user programs can get the slots
>> from sysfs directly.
>>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> ---
>> ChangeLog in v5:
>>     * Add check STALL_SLOT in PMCEID1_EL0
> 
> Thanks. I was just about to apply this, but then I realised that it's
> completely broken for big.LITTLE :( One CPU might have PMMIR_EL1, but

My bad, I miss it completely.

> another might not and so code such as:
> 
>> +static umode_t
>> +armv8pmu_caps_attr_is_visible(struct kobject *kobj, struct attribute *attr,
>> +			      int unused)
>> +{
>> +	int pmuver = armv8pmu_get_pmu_version();
>> +	u32 pmceid1 = read_sysreg(pmceid1_el0);
>> +
>> +	/* Check the PMU version is >= v8.4 and STALL_SLOT is implemented */
>> +	if (pmuver >= ID_AA64DFR0_PMUVER_8_4 && (pmceid1 & BIT(31)))
>> +		return attr->mode;
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t slots_show(struct device *dev, struct device_attribute *attr,
>> +			  char *buf)
>> +{
>> +	int slots = read_sysreg_s(SYS_PMMIR_EL1) & 0xFF;
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%d\n", slots);
>> +}
> 
> Is dangerous if you can migrate between the two functions.
> 
> So I think what we need to do is use the cpu_pmu structure to stash the
> PMMIR_EL1 register during probe, setting it to zero for CPUs without it,
> and then you can just report that value on slots_show(), getting rid of

Got it.

> armv8pmu_caps_attr_is_visible() entirely.
> 

Do we need to check whether PMMIR_EL1 register is !0 and return attr->mode?
If not, it shall remove it completely.

> Does that make sense? Sorry I didn't spot this earlier.
> 

Thanks Will's detailed explanation, I will do it soon.

Shaokun,

> Will
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel