Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 8 Jun 2022 11:47:12 +0300
From:   Nikita Shubin <nikita.shubin@maquefel.me>
To:     Atish Patra <atishp@atishpatra.org>
Cc:     Genevieve Chan <genevieve.chan@starfivetech.com>,
        =?UTF-8?B?Sm/Do28g?= =?UTF-8?B?TcOhcmlv?= Domingos 
        <joao.mario@tecnico.ulisboa.pt>,
        Nikita Shubin <n.shubin@yadro.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Anup Patel <anup@brainfault.org>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        <linux-riscv@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 1/4] RISC-V: Create unique identification for SoC PMU
Message-ID: <20220608114712.608239a5@redslave.neermore.group>
In-Reply-To: <CAOnJCULuQw2E3cD5KQKVs8EbcbPPeJQDqvikagxnkDSH+UWqjQ@mail.gmail.com>
References: <20220607131648.29439-1-nikita.shubin@maquefel.me>
        <20220607131648.29439-2-nikita.shubin@maquefel.me>
        <CAOnJCULuQw2E3cD5KQKVs8EbcbPPeJQDqvikagxnkDSH+UWqjQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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=C3=A3o M=C3=A1rio 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];
> > =20
>=20
> 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.
>=20
> 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.

>=20
> > 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=C3=A3o M=C3=A1rio Domingos <joao.mario@tecnico.ulisbo=
a.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 =3D 0;
> > +       pmuid.vendor =3D (uint32_t) sbi_get_mvendorid();
> > +       pmuid.arch =3D (sbi_get_marchid() >> (63 - 15) & (1 << 15)) |
> > (sbi_get_marchid() & 0x7FFF);
> > +       pmuid.imp =3D (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 =3D sprintf(buf, "0x%llx\n", pmu_sbi_get_pmu_id());
> > +       if (len <=3D 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[] =3D {
> > +       &dev_attr_id.attr,
> > +       NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(pmu_sbi);
> > +
> >  static int pmu_sbi_device_probe(struct platform_device *pdev)
> >  {
> >         struct riscv_pmu *pmu =3D NULL;
> > @@ -729,6 +769,13 @@ static int pmu_sbi_device_probe(struct
> > platform_device *pdev) pmu->ctr_clear_idx =3D pmu_sbi_ctr_clear_idx;
> >         pmu->ctr_read =3D pmu_sbi_ctr_read;
> >
> > +       ret =3D sysfs_create_group(&pdev->dev.kobj, &pmu_sbi_group);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "sysfs creation failed\n");
> > +               return ret;
> > +       }
> > +       pdev->dev.groups =3D pmu_sbi_groups;
> > +
> >         ret =3D
> > cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
> > if (ret) return ret;
> > --
> > 2.35.1
> > =20
>=20
>=20
> --
> Regards,
> Atish