Message-ID: <9650ad31-d59e-9325-1e05-73261960ccdc@microchip.com>
Date:   Fri, 26 Aug 2022 17:28:01 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <heiko@...ech.de>, <atishp@...shpatra.org>, <anup@...infault.org>,
        <will@...nel.org>, <mark.rutland@....com>
CC:     <palmer@...belt.com>, <aou@...s.berkeley.edu>,
        <paul.walmsley@...ive.com>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-riscv@...ts.infradead.org>,
        <cmuellner@...ux.com>, <philipp.tomsich@...ll.eu>
Subject: Re: [PATCH] drivers/perf: riscv_pmu_sbi: add perf_user_access sysctl

Hey Heiko,
Couple comments, mostly nitpicky sorts of things..

On 26/08/2022 16:15, Heiko Stuebner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Add a sysctl similar to the one on arm64 to enable/disable
> access to counter CSRs from u-mode on RISC-V.
> 
> The default is of course set to disabled keeping the current
> state of access - to only the TIME CSR.
> 
> Signed-off-by: Heiko Stuebner <heiko@...ech.de>
> ---
>  Documentation/admin-guide/sysctl/kernel.rst |  6 +--
>  drivers/perf/riscv_pmu_sbi.c                | 43 ++++++++++++++++++++-
>  2 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index ee6572b1edad..efd4bc385e7a 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -894,15 +894,15 @@ enabled, otherwise writing to this file will return ``-EBUSY``.
>  The default value is 8.
> 
> 
> -perf_user_access (arm64 only)
> -=================================
> +perf_user_access (arm64 and riscv only)
> +=======================================
> 
>  Controls user space access for reading perf event counters. When set to 1,
>  user space can read performance monitor counter registers directly.
> 
>  The default value is 0 (access disabled).
> 
> -See Documentation/arm64/perf.rst for more information.
> +See Documentation/arm64/perf.rst for more information on arm64

Code aside, this reads somewhat confusingly. How come riscv doesn't have
further information to offer?

Even if we don't have anything from a riscv point of view, it would read
better if this statement mentioned what it is about arm64 that requires
more information. Something like "for more information on arm64 only
fetures x, y & z".

> 
> 
>  pid_max
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 6f6681bbfd36..7aab8d673357 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -41,6 +41,8 @@ static const struct attribute_group *riscv_pmu_attr_groups[] = {
>         NULL,
>  };
> 
> +static int sysctl_perf_user_access __read_mostly;
> +
>  /*
>   * RISC-V doesn't have hetergenous harts yet. This need to be part of

Not in your changes, but s/hetergenous/heterogeneous

>   * per_cpu in case of harts with different pmu counters
> @@ -640,13 +642,22 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>         return IRQ_HANDLED;
>  }
> 
> +/*
> + * Depending on the perf_user_access setting, enable the access
> + * from usermode either for all counters or for TIME csr only.
> + */
> +static void riscv_pmu_update_user_access(void *info)
> +{
> +       csr_write(CSR_SCOUNTEREN, sysctl_perf_user_access ? GENMASK(31, 0) :
> +                                                           0x2);

This file does not comply to 80 character limits to begin with, so I
think this would be better on an 82 character line than broken up.

> +}
> +
>  static int pmu_sbi_starting_cpu(unsigned int cpu, struct hlist_node *node)
>  {
>         struct riscv_pmu *pmu = hlist_entry_safe(node, struct riscv_pmu, node);
>         struct cpu_hw_events *cpu_hw_evt = this_cpu_ptr(pmu->hw_events);
> 
> -       /* Enable the access for TIME csr only from the user mode now */
> -       csr_write(CSR_SCOUNTEREN, 0x2);
> +       riscv_pmu_update_user_access(NULL);
> 
>         /* Stop all the counters so that they can be enabled from perf */
>         pmu_sbi_stop_all(pmu);
> @@ -785,6 +796,32 @@ static void riscv_pmu_destroy(struct riscv_pmu *pmu)
>         cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
>  }
> 
> +static int riscv_pmu_proc_user_access_handler(struct ctl_table *table,
> +                       int write, void *buffer, size_t *lenp, loff_t *ppos)

I assume checkpatch will whinge about the alignment here.

> +{
> +       int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> +
> +       if (ret || !write)
> +               return ret;
> +
> +       on_each_cpu(riscv_pmu_update_user_access, NULL, 1);
> +
> +       return 0;
> +}
> +
> +static struct ctl_table sbi_pmu_sysctl_table[] = {
> +       {
> +               .procname       = "perf_user_access",
> +               .data           = &sysctl_perf_user_access,
> +               .maxlen         = sizeof(unsigned int),
> +               .mode           = 0644,

Mixed use of whitespace here, some are spaces and some tabs..

Thanks,
Conor.

> +               .proc_handler   = riscv_pmu_proc_user_access_handler,
> +               .extra1         = SYSCTL_ZERO,
> +               .extra2         = SYSCTL_ONE,
> +       },
> +       { }
> +};
> +
>  static int pmu_sbi_device_probe(struct platform_device *pdev)
>  {
>         struct riscv_pmu *pmu = NULL;
> @@ -834,6 +871,8 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>         if (ret)
>                 goto out_unregister;
> 
> +       register_sysctl("kernel", sbi_pmu_sysctl_table);
> +
>         return 0;
> 
>  out_unregister:
> --
> 2.35.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists