diff mbox series

[v3,1/4] RISC-V: Create unique identification for SoC PMU

Message ID 20220607131648.29439-2-nikita.shubin@maquefel.me (mailing list archive)
State New, archived
Headers show
Series [v3,1/4] RISC-V: Create unique identification for SoC PMU | expand

Commit Message

Nikita Shubin June 7, 2022, 1:16 p.m. UTC
From: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>

The SBI PMU platform driver did not provide any identification for
perf events matching. This patch introduces a new sysfs file inside the
platform device (soc:pmu/id) for pmu identification.

The identification is a 64-bit value generated as:
[63-32]: mvendorid;
[31]: marchid[MSB];
[30-16]: marchid[15-0];
[15-0]: mimpid[15MSBs];

The CSRs are detailed in the RISC-V privileged spec [1].
The marchid is split in MSB + 15LSBs, due to the MSB being used for
open-source architecture identification.

[1] https://github.com/riscv/riscv-isa-manual

Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
Tested-by: Nikita Shubin <n.shubin@yadro.com>
---
 arch/riscv/kernel/sbi.c      |  3 +++
 drivers/perf/riscv_pmu_sbi.c | 47 ++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

Comments

Atish Patra June 7, 2022, 4:37 p.m. UTC | #1
On Tue, Jun 7, 2022 at 6:17 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
>
> The SBI PMU platform driver did not provide any identification for
> perf events matching. This patch introduces a new sysfs file inside the
> platform device (soc:pmu/id) for pmu identification.
>
> The identification is a 64-bit value generated as:
> [63-32]: mvendorid;
> [31]: marchid[MSB];
> [30-16]: marchid[15-0];
> [15-0]: mimpid[15MSBs];
>

This is not entirely correct as marchid or mimpid can be MXLEN. The
encoding scheme is left upto the
vendor. We can not assume anything about it.

The purpose of the PMU ID is to distinguish between different
vendors/generations. The perf tool expects
a json string.
I think you can just keep all these 3 registers into the JSON string
as it is to avoid any pitfalls with vendor weirdness.

> The CSRs are detailed in the RISC-V privileged spec [1].
> The marchid is split in MSB + 15LSBs, due to the MSB being used for
> open-source architecture identification.
>
> [1] https://github.com/riscv/riscv-isa-manual
>
> Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> Tested-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>  arch/riscv/kernel/sbi.c      |  3 +++
>  drivers/perf/riscv_pmu_sbi.c | 47 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 775d3322b422..50dd9b6ecc9e 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -627,16 +627,19 @@ long sbi_get_mvendorid(void)
>  {
>         return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
>  }
> +EXPORT_SYMBOL(sbi_get_mvendorid);
>
>  long sbi_get_marchid(void)
>  {
>         return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
>  }
> +EXPORT_SYMBOL(sbi_get_marchid);
>
>  long sbi_get_mimpid(void)
>  {
>         return __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
>  }
> +EXPORT_SYMBOL(sbi_get_mimpid);
>
>  static void sbi_send_cpumask_ipi(const struct cpumask *target)
>  {
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index a1317a483512..15ab3dc68e7a 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -693,6 +693,46 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
>         return 0;
>  }
>
> +static uint64_t pmu_sbi_get_pmu_id(void)
> +{
> +       union sbi_pmu_id {
> +               uint64_t value;
> +               struct {
> +                       uint16_t imp:16;
> +                       uint16_t arch:16;
> +                       uint32_t vendor:32;
> +               };
> +       } pmuid;
> +
> +       pmuid.value = 0;
> +       pmuid.vendor = (uint32_t) sbi_get_mvendorid();
> +       pmuid.arch = (sbi_get_marchid() >> (63 - 15) & (1 << 15)) | (sbi_get_marchid() & 0x7FFF);
> +       pmuid.imp = (sbi_get_mimpid() >> 16);
> +
> +       return pmuid.value;
> +}
> +
> +static ssize_t pmu_sbi_id_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       int len;
> +
> +       len = sprintf(buf, "0x%llx\n", pmu_sbi_get_pmu_id());
> +       if (len <= 0)
> +               dev_err(dev, "mydrv: Invalid sprintf len: %dn", len);
> +
> +       return len;
> +}
> +
> +static DEVICE_ATTR(id, S_IRUGO | S_IWUSR, pmu_sbi_id_show, 0);
> +
> +static struct attribute *pmu_sbi_attrs[] = {
> +       &dev_attr_id.attr,
> +       NULL
> +};
> +
> +ATTRIBUTE_GROUPS(pmu_sbi);
> +
>  static int pmu_sbi_device_probe(struct platform_device *pdev)
>  {
>         struct riscv_pmu *pmu = NULL;
> @@ -729,6 +769,13 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>         pmu->ctr_clear_idx = pmu_sbi_ctr_clear_idx;
>         pmu->ctr_read = pmu_sbi_ctr_read;
>
> +       ret = sysfs_create_group(&pdev->dev.kobj, &pmu_sbi_group);
> +       if (ret) {
> +               dev_err(&pdev->dev, "sysfs creation failed\n");
> +               return ret;
> +       }
> +       pdev->dev.groups = pmu_sbi_groups;
> +
>         ret = cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
>         if (ret)
>                 return ret;
> --
> 2.35.1
>


--
Regards,
Atish
Nikita Shubin June 8, 2022, 8:47 a.m. UTC | #2
Hello Atish!

On Tue, 7 Jun 2022 09:37:19 -0700
Atish Patra <atishp@atishpatra.org> wrote:

> On Tue, Jun 7, 2022 at 6:17 AM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> >
> > From: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> >
> > The SBI PMU platform driver did not provide any identification for
> > perf events matching. This patch introduces a new sysfs file inside
> > the platform device (soc:pmu/id) for pmu identification.
> >
> > The identification is a 64-bit value generated as:
> > [63-32]: mvendorid;
> > [31]: marchid[MSB];
> > [30-16]: marchid[15-0];
> > [15-0]: mimpid[15MSBs];
> >  
> 
> This is not entirely correct as marchid or mimpid can be MXLEN. The
> encoding scheme is left upto the
> vendor. We can not assume anything about it.
> 
> The purpose of the PMU ID is to distinguish between different
> vendors/generations. The perf tool expects
> a json string.
> I think you can just keep all these 3 registers into the JSON string
> as it is to avoid any pitfalls with vendor weirdness.

This make sense to me. I'll rework this patch according your
suggestions.

Yours,
Nikita Shubin.

> 
> > The CSRs are detailed in the RISC-V privileged spec [1].
> > The marchid is split in MSB + 15LSBs, due to the MSB being used for
> > open-source architecture identification.
> >
> > [1] https://github.com/riscv/riscv-isa-manual
> >
> > Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> > Tested-by: Nikita Shubin <n.shubin@yadro.com>
> > ---
> >  arch/riscv/kernel/sbi.c      |  3 +++
> >  drivers/perf/riscv_pmu_sbi.c | 47
> > ++++++++++++++++++++++++++++++++++++ 2 files changed, 50
> > insertions(+)
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 775d3322b422..50dd9b6ecc9e 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -627,16 +627,19 @@ long sbi_get_mvendorid(void)
> >  {
> >         return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> >  }
> > +EXPORT_SYMBOL(sbi_get_mvendorid);
> >
> >  long sbi_get_marchid(void)
> >  {
> >         return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
> >  }
> > +EXPORT_SYMBOL(sbi_get_marchid);
> >
> >  long sbi_get_mimpid(void)
> >  {
> >         return __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
> >  }
> > +EXPORT_SYMBOL(sbi_get_mimpid);
> >
> >  static void sbi_send_cpumask_ipi(const struct cpumask *target)
> >  {
> > diff --git a/drivers/perf/riscv_pmu_sbi.c
> > b/drivers/perf/riscv_pmu_sbi.c index a1317a483512..15ab3dc68e7a
> > 100644 --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -693,6 +693,46 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu
> > *pmu, struct platform_device *pde return 0;
> >  }
> >
> > +static uint64_t pmu_sbi_get_pmu_id(void)
> > +{
> > +       union sbi_pmu_id {
> > +               uint64_t value;
> > +               struct {
> > +                       uint16_t imp:16;
> > +                       uint16_t arch:16;
> > +                       uint32_t vendor:32;
> > +               };
> > +       } pmuid;
> > +
> > +       pmuid.value = 0;
> > +       pmuid.vendor = (uint32_t) sbi_get_mvendorid();
> > +       pmuid.arch = (sbi_get_marchid() >> (63 - 15) & (1 << 15)) |
> > (sbi_get_marchid() & 0x7FFF);
> > +       pmuid.imp = (sbi_get_mimpid() >> 16);
> > +
> > +       return pmuid.value;
> > +}
> > +
> > +static ssize_t pmu_sbi_id_show(struct device *dev,
> > +               struct device_attribute *attr, char *buf)
> > +{
> > +       int len;
> > +
> > +       len = sprintf(buf, "0x%llx\n", pmu_sbi_get_pmu_id());
> > +       if (len <= 0)
> > +               dev_err(dev, "mydrv: Invalid sprintf len: %dn",
> > len); +
> > +       return len;
> > +}
> > +
> > +static DEVICE_ATTR(id, S_IRUGO | S_IWUSR, pmu_sbi_id_show, 0);
> > +
> > +static struct attribute *pmu_sbi_attrs[] = {
> > +       &dev_attr_id.attr,
> > +       NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(pmu_sbi);
> > +
> >  static int pmu_sbi_device_probe(struct platform_device *pdev)
> >  {
> >         struct riscv_pmu *pmu = NULL;
> > @@ -729,6 +769,13 @@ static int pmu_sbi_device_probe(struct
> > platform_device *pdev) pmu->ctr_clear_idx = pmu_sbi_ctr_clear_idx;
> >         pmu->ctr_read = pmu_sbi_ctr_read;
> >
> > +       ret = sysfs_create_group(&pdev->dev.kobj, &pmu_sbi_group);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "sysfs creation failed\n");
> > +               return ret;
> > +       }
> > +       pdev->dev.groups = pmu_sbi_groups;
> > +
> >         ret =
> > cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
> > if (ret) return ret;
> > --
> > 2.35.1
> >  
> 
> 
> --
> Regards,
> Atish
Sunil V L June 14, 2022, 10:16 a.m. UTC | #3
Hi Nikita,

On Tue, Jun 07, 2022 at 04:16:44PM +0300, Nikita Shubin wrote:
> From: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> 
> The SBI PMU platform driver did not provide any identification for
> perf events matching. This patch introduces a new sysfs file inside the
> platform device (soc:pmu/id) for pmu identification.
> 
> The identification is a 64-bit value generated as:
> [63-32]: mvendorid;
> [31]: marchid[MSB];
> [30-16]: marchid[15-0];
> [15-0]: mimpid[15MSBs];
> 
> The CSRs are detailed in the RISC-V privileged spec [1].
> The marchid is split in MSB + 15LSBs, due to the MSB being used for
> open-source architecture identification.
> 
> [1] https://github.com/riscv/riscv-isa-manual
> 
> Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> Tested-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>  arch/riscv/kernel/sbi.c      |  3 +++
>  drivers/perf/riscv_pmu_sbi.c | 47 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 775d3322b422..50dd9b6ecc9e 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -627,16 +627,19 @@ long sbi_get_mvendorid(void)
>  {
>  	return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
>  }
> +EXPORT_SYMBOL(sbi_get_mvendorid);
>  
>  long sbi_get_marchid(void)
>  {
>  	return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
>  }
> +EXPORT_SYMBOL(sbi_get_marchid);
>  
>  long sbi_get_mimpid(void)
>  {
>  	return __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
>  }
> +EXPORT_SYMBOL(sbi_get_mimpid);
>  
>  static void sbi_send_cpumask_ipi(const struct cpumask *target)
>  {
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index a1317a483512..15ab3dc68e7a 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -693,6 +693,46 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
>  	return 0;
>  }
>  
> +static uint64_t pmu_sbi_get_pmu_id(void)
> +{
> +	union sbi_pmu_id {
> +		uint64_t value;
> +		struct {
> +			uint16_t imp:16;
> +			uint16_t arch:16;
> +			uint32_t vendor:32;
> +		};
> +	} pmuid;
> +
> +	pmuid.value = 0;
> +	pmuid.vendor = (uint32_t) sbi_get_mvendorid();
> +	pmuid.arch = (sbi_get_marchid() >> (63 - 15) & (1 << 15)) | (sbi_get_marchid() & 0x7FFF);

This statement generates below warning in rv32 build.

drivers/perf/riscv_pmu_sbi.c: In function 'pmu_sbi_get_pmu_id':
 drivers/perf/riscv_pmu_sbi.c:715:41: warning: right shift count >=
 width of type [-Wshift-count-overflow]
  715 |        pmuid.arch = (sbi_get_marchid() >> (63 - 15) & (1 << 15))
  | (sbi_get_marchid() & 0x7FFF);
     

Please take care of this when you rework this patch.

Thanks
Sunil

> +	pmuid.imp = (sbi_get_mimpid() >> 16);
> +
> +	return pmuid.value;
> +}
> +
> +static ssize_t pmu_sbi_id_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	int len;
> +
> +	len = sprintf(buf, "0x%llx\n", pmu_sbi_get_pmu_id());
> +	if (len <= 0)
> +		dev_err(dev, "mydrv: Invalid sprintf len: %dn", len);
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR(id, S_IRUGO | S_IWUSR, pmu_sbi_id_show, 0);
> +
> +static struct attribute *pmu_sbi_attrs[] = {
> +	&dev_attr_id.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(pmu_sbi);
> +
>  static int pmu_sbi_device_probe(struct platform_device *pdev)
>  {
>  	struct riscv_pmu *pmu = NULL;
> @@ -729,6 +769,13 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	pmu->ctr_clear_idx = pmu_sbi_ctr_clear_idx;
>  	pmu->ctr_read = pmu_sbi_ctr_read;
>  
> +	ret = sysfs_create_group(&pdev->dev.kobj, &pmu_sbi_group);
> +	if (ret) {
> +		dev_err(&pdev->dev, "sysfs creation failed\n");
> +		return ret;
> +	}
> +	pdev->dev.groups = pmu_sbi_groups;
> +
>  	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
>  	if (ret)
>  		return ret;
> -- 
> 2.35.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
diff mbox series

Patch

diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 775d3322b422..50dd9b6ecc9e 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -627,16 +627,19 @@  long sbi_get_mvendorid(void)
 {
 	return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
 }
+EXPORT_SYMBOL(sbi_get_mvendorid);
 
 long sbi_get_marchid(void)
 {
 	return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
 }
+EXPORT_SYMBOL(sbi_get_marchid);
 
 long sbi_get_mimpid(void)
 {
 	return __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
 }
+EXPORT_SYMBOL(sbi_get_mimpid);
 
 static void sbi_send_cpumask_ipi(const struct cpumask *target)
 {
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index a1317a483512..15ab3dc68e7a 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -693,6 +693,46 @@  static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
 	return 0;
 }
 
+static uint64_t pmu_sbi_get_pmu_id(void)
+{
+	union sbi_pmu_id {
+		uint64_t value;
+		struct {
+			uint16_t imp:16;
+			uint16_t arch:16;
+			uint32_t vendor:32;
+		};
+	} pmuid;
+
+	pmuid.value = 0;
+	pmuid.vendor = (uint32_t) sbi_get_mvendorid();
+	pmuid.arch = (sbi_get_marchid() >> (63 - 15) & (1 << 15)) | (sbi_get_marchid() & 0x7FFF);
+	pmuid.imp = (sbi_get_mimpid() >> 16);
+
+	return pmuid.value;
+}
+
+static ssize_t pmu_sbi_id_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	int len;
+
+	len = sprintf(buf, "0x%llx\n", pmu_sbi_get_pmu_id());
+	if (len <= 0)
+		dev_err(dev, "mydrv: Invalid sprintf len: %dn", len);
+
+	return len;
+}
+
+static DEVICE_ATTR(id, S_IRUGO | S_IWUSR, pmu_sbi_id_show, 0);
+
+static struct attribute *pmu_sbi_attrs[] = {
+	&dev_attr_id.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(pmu_sbi);
+
 static int pmu_sbi_device_probe(struct platform_device *pdev)
 {
 	struct riscv_pmu *pmu = NULL;
@@ -729,6 +769,13 @@  static int pmu_sbi_device_probe(struct platform_device *pdev)
 	pmu->ctr_clear_idx = pmu_sbi_ctr_clear_idx;
 	pmu->ctr_read = pmu_sbi_ctr_read;
 
+	ret = sysfs_create_group(&pdev->dev.kobj, &pmu_sbi_group);
+	if (ret) {
+		dev_err(&pdev->dev, "sysfs creation failed\n");
+		return ret;
+	}
+	pdev->dev.groups = pmu_sbi_groups;
+
 	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
 	if (ret)
 		return ret;