[PATCH] arm64: perf: Add more support on caps under sysfs

[Shaokun Zhang]

Hi Robin,

On 2021/5/25 1:18, Robin Murphy wrote:
> On 2021-05-21 08:25, Shaokun Zhang wrote:
>> Armv8.7 has introduced BUS_SLOTS and BUS_WIDTH in PMMIR_EL1 register,
>> add two entries in caps for bus_slots and bus_width under sysfs. It
>> will return the true slots and width if the information is available,
>> otherwise it will return 0.
>>
>> Cc: Will Deacon <will at kernel.org>
>> Cc: Mark Rutland <mark.rutland at arm.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun at hisilicon.com>
>> ---
>>   arch/arm64/include/asm/perf_event.h |  5 +++
>>   arch/arm64/kernel/perf_event.c      | 64 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 69 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/perf_event.h b/arch/arm64/include/asm/perf_event.h
>> index 60731f602d3e..4ef6f19331f9 100644
>> --- a/arch/arm64/include/asm/perf_event.h
>> +++ b/arch/arm64/include/asm/perf_event.h
>> @@ -239,6 +239,11 @@
>>   /* PMMIR_EL1.SLOTS mask */
>>   #define ARMV8_PMU_SLOTS_MASK    0xff
>>   +#define ARMV8_PMU_BUS_SLOTS_SHIFT 8
>> +#define ARMV8_PMU_BUS_SLOTS_MASK 0xff
>> +#define ARMV8_PMU_BUS_WIDTH_SHIFT 16
>> +#define ARMV8_PMU_BUS_WIDTH_MASK 0xf
>> +
>>   #ifdef CONFIG_PERF_EVENTS
>>   struct pt_regs;
>>   extern unsigned long perf_instruction_pointer(struct pt_regs *regs);
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index f594957e29bd..f0847e4f48a9 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -317,8 +317,72 @@ static ssize_t slots_show(struct device *dev, struct device_attribute *attr,
>>     static DEVICE_ATTR_RO(slots);
>>   +static ssize_t bus_slots_show(struct device *dev, struct device_attribute *attr,
>> +                  char *page)
>> +{
>> +    struct pmu *pmu = dev_get_drvdata(dev);
>> +    struct arm_pmu *cpu_pmu = container_of(pmu, struct arm_pmu, pmu);
>> +    u32 bus_slots = (cpu_pmu->reg_pmmir >> ARMV8_PMU_BUS_SLOTS_SHIFT)
>> +            & ARMV8_PMU_BUS_SLOTS_MASK;
>> +
>> +    return snprintf(page, PAGE_SIZE, "0x%08x\n", bus_slots);
>> +}
>> +
>> +static DEVICE_ATTR_RO(bus_slots);
>> +
>> +static ssize_t bus_width_show(struct device *dev, struct device_attribute *attr,
>> +                  char *page)
>> +{
>> +    struct pmu *pmu = dev_get_drvdata(dev);
>> +    struct arm_pmu *cpu_pmu = container_of(pmu, struct arm_pmu, pmu);
>> +    u32 bus_width = (cpu_pmu->reg_pmmir >> ARMV8_PMU_BUS_WIDTH_SHIFT)
>> +            & ARMV8_PMU_BUS_WIDTH_MASK;
>> +    u32 val;
>> +
>> +    switch (bus_width) {
>> +    case 3:
>> +        val = 4;
>> +        break;
>> +    case 4:
>> +        val = 8;
>> +        break;
>> +    case 5:
>> +        val = 16;
>> +        break;
>> +    case 6:
>> +        val = 32;
>> +        break;
>> +    case 7:
>> +        val = 64;
>> +        break;
>> +    case 8:
>> +        val = 128;
>> +        break;
>> +    case 9:
>> +        val = 256;
>> +        break;
>> +    case 10:
>> +        val = 512;
>> +        break;
>> +    case 11:
>> +        val = 1024;
>> +        break;
>> +    case 12:
>> +        val = 2048;
>> +        break;
> 
> Nit: the encoding is explicitly defined as "log2(number of bytes) plus one", so this might be neater
> as simply:
> 
>     val = 1 << (bus_width - 1);
> 

Good catch, I will follow it in next version.

> I'm not really sure whether interpreting reserved values as 0 is any better or worse than
> interpreting them as implied :/

To be honest, I'm also not sure about it and I checked it in the document that
For BUS_WIDTH and BUS_SLOTS, if it is not from Armv8.7, it shall be Reserved, RAZ
and I returned 0 including other values from Armv8.7.

> 
>> +    default:
>> +        val = 0;
>> +    }
> 
> If the information is not available, wouldn't it make sense to just hide the attribute(s) entirely?
> Userspace should already have to cope with older kernels, so from that point of view it seems easier
> to only have two states of "not present" and "present and valid", without having to also consider a
> third "present but invalid" state.

When talked about SLOTS field [1], Will suggested to get rid of visible hook and
return zero if the CPUs without it.

[1]https://www.spinics.net/lists/arm-kernel/msg824289.html

Thanks,
Shaokun

> 
> Robin.
> 
>> +
>> +    return snprintf(page, PAGE_SIZE, "0x%08x\n", val);
>> +}
>> +
>> +static DEVICE_ATTR_RO(bus_width);
>> +
>>   static struct attribute *armv8_pmuv3_caps_attrs[] = {
>>       &dev_attr_slots.attr,
>> +    &dev_attr_bus_slots.attr,
>> +    &dev_attr_bus_width.attr,
>>       NULL,
>>   };
>>  
> .