linux-kernel - Re: [PATCH v3 1/4] RISC-V: Create unique identification for SoC 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220608114712.608239a5@redslave.neermore.group>
Date:   Wed, 8 Jun 2022 11:47:12 +0300
From:   Nikita Shubin <nikita.shubin@...uefel.me>
To:     Atish Patra <atishp@...shpatra.org>
Cc:     Genevieve Chan <genevieve.chan@...rfivetech.com>,
        João Mário Domingos 
        <joao.mario@...nico.ulisboa.pt>,
        Nikita Shubin <n.shubin@...ro.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Anup Patel <anup@...infault.org>,
        Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 1/4] RISC-V: Create unique identification for SoC PMU

Hello Atish!

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

> On Tue, Jun 7, 2022 at 6:17 AM Nikita Shubin
> <nikita.shubin@...uefel.me> wrote:
> >
> > From: João Mário Domingos <joao.mario@...nico.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@...nico.ulisboa.pt>
> > Tested-by: Nikita Shubin <n.shubin@...ro.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