| 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
| ||
|
Message-ID: <b106d24b-880e-07e6-e5e6-541b5fdccd64@quicinc.com>
Date: Sat, 19 Aug 2023 20:37:49 +0800
From: Tao Zhang <quic_taozha@...cinc.com>
To: Suzuki K Poulose <suzuki.poulose@....com>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Konrad Dybcio <konradybcio@...il.com>,
Mike Leach <mike.leach@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
CC: Jinlong Mao <quic_jinlmao@...cinc.com>,
Leo Yan <leo.yan@...aro.org>,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
<coresight@...ts.linaro.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
Tingwei Zhang <quic_tingweiz@...cinc.com>,
Yuanfang Zhang <quic_yuanfang@...cinc.com>,
Trilok Soni <quic_tsoni@...cinc.com>,
Hao Zhang <quic_hazha@...cinc.com>,
<linux-arm-msm@...r.kernel.org>, <andersson@...nel.org>
Subject: Re: [PATCH v7 13/13] coresight-tpdm: Add nodes for dsb msr support
On 8/19/2023 12:25 AM, Suzuki K Poulose wrote:
> On 18/08/2023 16:44, Tao Zhang wrote:
>>
>> On 8/7/2023 7:35 PM, Suzuki K Poulose wrote:
>>> On 25/07/2023 08:15, Tao Zhang wrote:
>>>> Add the nodes for DSB subunit MSR(mux select register) support.
>>>> The TPDM MSR (mux select register) interface is an optional
>>>> interface and associated bank of registers per TPDM subunit.
>>>> The intent of mux select registers is to control muxing structures
>>>> driving the TPDM’s’ various subunit interfaces.
>>>>
>>>> Signed-off-by: Tao Zhang <quic_taozha@...cinc.com>
>>>> ---
>>>> .../ABI/testing/sysfs-bus-coresight-devices-tpdm | 19 ++++-
>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 98
>>>> ++++++++++++++++++++++
>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 7 ++
>>>> 3 files changed, 123 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git
>>>> a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> index 74a0126..ee41a14 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpdm
>>>> @@ -185,4 +185,21 @@ Description:
>>>> Accepts only one of the 2 values - 0 or 1.
>>>> 0 : Set the DSB pattern type to value.
>>>> - 1 : Set the DSB pattern type to toggle.
>>>> \ No newline at end of file
>>>> + 1 : Set the DSB pattern type to toggle.
>>>> +
>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_msr_idx
>>>> +Date: March 2023
>>>> +KernelVersion 6.5
>>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Tao
>>>> Zhang (QUIC) <quic_taozha@...cinc.com>
>>>> +Description:
>>>> + Read/Write the index number of MSR(mux select register) on
>>>> DSB
>>>> + TPDM. This index number should not be greater than the number
>>>> + of MSR supported by this DSB TPDM.
>>>
>>> How does a user find it ? Why don't we expose it ? Scratch all of
>>> that, you could create a register file per exising msr.
>>>
>>> dsb_msrs/msr0 ... msrN
>>>
>>> You may :
>>>
>>> 1. either dynamically add files as you find the number
>>>
>>> OR
>>>
>>> 2. If there is a hard limit, create that many files and control
>>> visibility based on the dynamic number.
>>>
>> Hi Suzuki,
>>
>>
>> With regards to your suggestions, would you mind if you provide some
>> sample code for my reference?
>>
>> 1. How to dynamically add files to the sysfs as I find the number?
>>
>> 2. The hard limit of MSR should be 32. How can I control visibility
>> based on the dynamic number?
>
> You could add attributes via dev_ext_attribute, storing the "idx" as
> the data field for 0-31. And use the is_visible() hook for the
> attribute_group, just like you do for DSB related checks.
>
>
> Some skeleton code, untested :
>
> static ssize_t dsb_msr_read(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct dev_ext_attribute *eattr = container_of(attr,
> struct dev_ext_attribute, attr);
> int idx = (int)(unsigned long)eattr->var;
>
> /* Read the MSR at IDX */
> }
>
> /* Similarly for dsb_msr_write() */
>
> #define DSB_MSR_ATTR(idx) \
> &((struct dev_ext_attribute[]) { \
> { \
> __ATTR("msr"#idx, <RW>, dsb_msr_read, dsb_msr_write), \
> (void *)idx } \
> })[0].attr.attr
>
> #define DSB_MSR_ATTRS \
> DSB_MSR_ATTR(0), \
> DSB_MSR_ATTR(1), \
> ...
> DSB_MSR_ATTR(31)
>
> static struct attribute * dsb_msr_attrs[] = {
> DSB_MSR_ATTRS,
> NULL,
> };
>
> static umode_t
> dsb_msr_is_visible(struct kobject *kobj,
> struct attribute *attr, int unuse)
> {
> struct device *dev = kobject_to_dev(kobj);
> /* From dev, get drvdata */
> /* Then decide if it should be made visible, if yes R/W
> permissions */
> }
>
> static const struct attribute_group dsb_msr_group = {
> .is_visible = dsb_msr_is_visible,
> .attrs = dsb_msr_attrs,
> .name = "dsb_msr",
> };
>
> See : drivers/.../coresight-etm4x-sysfs.c:
>
> coresight_etmv4_mgmt_attrs & coresight_etmv4_mgmt_group
> for an example.
>
> Suzuki
>
I only can control if we need to expose the MSR sysfs files, but cannot
control how
much MSR sysfs files need to be exposed dynamically, right?
Best,
Tao
>
>
>>
>
>
>
>
>>>
>>>
>>>> +
>>>> +What: /sys/bus/coresight/devices/<tpdm-name>/dsb_msr
>>>> +Date: March 2023
>>>> +KernelVersion 6.5
>>>> +Contact: Jinlong Mao (QUIC) <quic_jinlmao@...cinc.com>, Tao
>>>> Zhang (QUIC) <quic_taozha@...cinc.com>
>>>> +Description:
>>>> + (Write) Set the MSR(mux select register) of DSB tpdm. Read
>>>> + the MSR(mux select register) of DSB tpdm.
>>>
>>> (RW)
>> Sure, I will update this in the next patch series.
>>>
>>>> \ No newline at end of file
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> index f9e5a1d..be7776b 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>>>> @@ -90,6 +90,18 @@ static void set_dsb_tier(struct tpdm_drvdata
>>>> *drvdata, u32 *val)
>>>> }
>>>> +static void set_dsb_msr(struct tpdm_drvdata *drvdata)
>>>> +{
>>>> + int i;
>>>> +
>>>
>>>
>>>> + if (drvdata->dsb->msr_num == 0)
>>>> + return;
>>>> +
>>>> + for (i = 0; i < drvdata->dsb->msr_num; i++)
>>>> + writel_relaxed(drvdata->dsb->msr[i],
>>>> + drvdata->base + TPDM_DSB_MSR(i));
>>>> +}
>>>> +
>>>> static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata)
>>>> {
>>>> u32 val, i;
>>>> @@ -116,6 +128,8 @@ static void tpdm_enable_dsb(struct tpdm_drvdata
>>>> *drvdata)
>>>> set_dsb_tier(drvdata, &val);
>>>> writel_relaxed(val, drvdata->base + TPDM_DSB_TIER);
>>>> + set_dsb_msr(drvdata);
>>>> +
>>>> val = readl_relaxed(drvdata->base + TPDM_DSB_CR);
>>>> /* Set the test accurate mode */
>>>> set_dsb_test_mode(drvdata, &val);
>>>> @@ -234,6 +248,14 @@ static int tpdm_datasets_setup(struct
>>>> tpdm_drvdata *drvdata)
>>>> if (!drvdata->dsb)
>>>> return -ENOMEM;
>>>> }
>>>> + if (!of_property_read_u32(drvdata->dev->of_node,
>>>> + "qcom,dsb_msr_num", &drvdata->dsb->msr_num)) {
>>>> + drvdata->dsb->msr = devm_kzalloc(drvdata->dev, > +
>>>> (drvdata->dsb->msr_num * sizeof(*drvdata->dsb->msr)),
>>>
>>> devm_kcalloc() ?
>> Sure, I will update this in the next patch series.
>>>
>>>> + GFP_KERNEL);
>>>> + if (!drvdata->dsb->msr)
>>>> + return -ENOMEM;
>>>> + }
>>>> }
>>>> return 0;
>>>> @@ -830,6 +852,80 @@ static ssize_t dsb_trig_ts_store(struct device
>>>> *dev,
>>>> }
>>>> static DEVICE_ATTR_RW(dsb_trig_ts);
>>>> +static ssize_t dsb_msr_idx_show(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +
>>>> + return sysfs_emit(buf, "%u\n",
>>>> + (unsigned int)drvdata->dsb->msr_idx);
>>>> +
>>>> +}
>>>> +
>>>> +static ssize_t dsb_msr_idx_store(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + const char *buf,
>>>> + size_t size)
>>>> +{
>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> + unsigned long index;
>>>> +
>>>> + if (kstrtoul(buf, 0, &index))
>>>> + return -EINVAL;
>>>> + if (index >= drvdata->dsb->msr_num)
>>>> + return -EPERM;
>>>> +
>>>> + spin_lock(&drvdata->spinlock);
>>>> + drvdata->dsb->msr_idx = index;
>>>> + spin_unlock(&drvdata->spinlock);
>>>> + return size;
>>>> +}
>>>> +static DEVICE_ATTR_RW(dsb_msr_idx);
>>>> +
>>>> +static ssize_t dsb_msr_show(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> + unsigned int i;
>>>> + unsigned long bytes;
>>>> + ssize_t size = 0;
>>>> +
>>>> + if (drvdata->dsb->msr_num == 0)
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock(&drvdata->spinlock);
>>>> + for (i = 0; i < drvdata->dsb->msr_num; i++) {
>>>> + bytes = sysfs_emit_at(buf, size,
>>>> + "0x%x\n", drvdata->dsb->msr[i]);
>>>
>>> Single value per single file.
>>
>> Sure, I will update this in the next patch series.
>>
>>
>> Best,
>>
>> Tao
>>
>>>
>>> Suzuki
>>>
>>>
>>>> + if (bytes <= 0)
>>>> + break;
>>>> + size += bytes;
>>>> + }
>>>> + spin_unlock(&drvdata->spinlock);
>>>> +
>>>> + return size;
>>>> +}
>>>> +
>>>> +static ssize_t dsb_msr_store(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + const char *buf,
>>>> + size_t size)
>>>> +{
>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> + unsigned long val;
>>>> +
>>>> + if (kstrtoul(buf, 0, &val))
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock(&drvdata->spinlock);
>>>> + drvdata->dsb->msr[drvdata->dsb->msr_idx] = val;
>>>> + spin_unlock(&drvdata->spinlock);
>>>> + return size;
>>>> +}
>>>> +static DEVICE_ATTR_RW(dsb_msr);
>>>> +
>>>> static struct attribute *tpdm_dsb_attrs[] = {
>>>> &dev_attr_dsb_mode.attr,
>>>> &dev_attr_dsb_edge_ctrl_idx.attr,
>>>> @@ -845,6 +941,8 @@ static struct attribute *tpdm_dsb_attrs[] = {
>>>> &dev_attr_dsb_trig_patt_mask.attr,
>>>> &dev_attr_dsb_trig_ts.attr,
>>>> &dev_attr_dsb_trig_type.attr,
>>>> + &dev_attr_dsb_msr_idx.attr,
>>>> + &dev_attr_dsb_msr.attr,
>>>> NULL,
>>>> };
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> index 7c52cf4..7b70db3 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>>>> @@ -18,6 +18,7 @@
>>>> #define TPDM_DSB_XPMR(n) (0x7E8 + (n * 4))
>>>> #define TPDM_DSB_EDCR(n) (0x808 + (n * 4))
>>>> #define TPDM_DSB_EDCMR(n) (0x848 + (n * 4))
>>>> +#define TPDM_DSB_MSR(n) (0x980 + (n * 4))
>>>> /* Enable bit for DSB subunit */
>>>> #define TPDM_DSB_CR_ENA BIT(0)
>>>> @@ -100,6 +101,9 @@
>>>> * @patt_mask: Save value for pattern mask
>>>> * @trig_patt: Save value for trigger pattern
>>>> * @trig_patt_mask: Save value for trigger pattern mask
>>>> + * @msr_num Number of MSR supported by DSB TPDM
>>>> + * @msr_idx Index number of the MSR
>>>> + * @msr Save value for MSR
>>>> * @patt_ts: Enable/Disable pattern timestamp
>>>> * @patt_type: Set pattern type
>>>> * @trig_ts: Enable/Disable trigger timestamp.
>>>> @@ -116,6 +120,9 @@ struct dsb_dataset {
>>>> u32 patt_mask[TPDM_DSB_MAX_PATT];
>>>> u32 trig_patt[TPDM_DSB_MAX_PATT];
>>>> u32 trig_patt_mask[TPDM_DSB_MAX_PATT];
>>>> + u32 msr_num;
>>>> + u32 msr_idx;
>>>> + u32 *msr;
>>>> bool patt_ts;
>>>> bool patt_type;
>>>> bool trig_ts;
>>>
>
Powered by blists - more mailing lists