linux-kernel - Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9a5db958-2fef-ff5e-c83f-394039e5bd0e@huawei.com>
Date:   Tue, 8 Mar 2022 17:32:09 +0800
From:   "liuqi (BA)" <liuqi115@...wei.com>
To:     John Garry <john.garry@...wei.com>, <will@...nel.org>,
        <mark.rutland@....com>, <acme@...nel.org>
CC:     <linux-perf-users@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>
Subject: Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU


Hi John,
On 2022/3/8 1:50, John Garry wrote:
> 
>>>> +
>>>> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>>>> +    val &= ~(CPA_PERF_CTRL_EN);
>>>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>>>> +}
>>>> +
>>>> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
>>>
>>> this seems unique for this new driver - why do we need to disable PM?
>>>
>> CPA PMU doesn't work under power mangement state, so we need to 
>> disable PM before using CPA PMU.
> 
> OK, so you need to disable something related to PM to use the HW. But 
> why do it when you enable the counter? I mean, can we just do it once 
> when we probe the HW? Why is it even enabled at all?
> 
>>

PM is enabled by default. yes, we could disable it in probe function and 
restore in remove function, I'll change this in next version.

Thanks,
Qi
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>>> +    val |= CPA_PM_CTRL;
>>>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>>>> +}
>>>> +
>>>> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>>> +    val &= ~CPA_PM_CTRL;
>>>
>>> nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so 
>>> please be consistent
>>>
>> ]
> 
> ...
> 
>>>> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
>>>> +                  struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>>>> +                     &cpa_pmu->sccl_id)) {
>>>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
>>>
>>> strange that the FW uses "scl-id" but driver uses sccl_id (I am 
>>> talking about sccl vs scl difference)
>>>
>> yes... these two names means the same thing, maybe could use a cleanup 
>> patch to keep them consistent.
>>
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    cpa_pmu->ccl_id = -1;
>>>> +
>>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
>>>> +                     &cpa_pmu->index_id)) {
>>>> +        dev_err(&pdev->dev, "Cannot read idx-id\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>>>> +    if (IS_ERR(cpa_pmu->base))
>>>> +        return PTR_ERR(cpa_pmu->base);
>>>> +
>>>> +    cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
> 
> ...
> 
>>>> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct hisi_pmu *cpa_pmu;
>>>> +    char *name;
>>>> +    int ret;
>>>> +
>>>> +    cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
>>>> +    if (!cpa_pmu)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>>> +                       &cpa_pmu->node);
>>>> +    if (ret) {
>>>> +        dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    name = devm_kasprintf(&pdev->dev, GFP_KERNEL, 
>>>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>>>
>>> sorry, but I still don't like this "- 1". From checking the chat on 
>>> v1, my impression was that you agreed with me on this one.
>>>
>>
>> sorry, maybe I didn't express it clearly.
>> CPA PMU is on IO die,
> 
> Then it is quite strange to provide the sccl id and not some sicl id 
> (since it is in the IO die)
> 
>   but BIOS give CPA PMU driver the sccl-id of
>> adjacent CPU die (as we need this sccl-id to pass 
>> hisi_pmu_cpu_is_associated_pmu() and find the associated cpu).
> 
> 
> Is the PMU only ever used by one SCCL? Or can more than one SCCL access? 
> If so, is there a special affinity to one particular SCCL?
> 
>> so, when naming CPA PMU, we use the sccl-id supported by BIOS to get 
>> the sicl-id of IO die which CPA PMU located in, that is: 
>> cpa_pmu->sccl_id - 1.
>>
>>>> +                  cpa_pmu->index_id);
> 
> Thanks,
> John
> .