Re: [PATCH v4 00/10] riscv: Allow userspace to directly access perf counters

[Rémi Denis-Courmont]

Hi,

Le 3 juillet 2023 15:46:37 GMT+03:00, Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> a écrit :
>riscv used to allow direct access to cycle/time/instret counters,
>bypassing the perf framework, this patchset intends to allow the user to
>mmap any counter when accessed through perf. But we can't break the
>existing behaviour so we introduce a sysctl perf_user_access like arm64
>does, which defaults to the legacy mode described above.

AFAIK, if the default settings breaks user space, the patchset is considered to break user space. That being the case, either this case is deemed special enough that breaking user space is OK, or it is not.

If it is not OK, then the only way out that I can think of, consists of trapping and emulating the counters, returning the same sanitised values that Linux perf would return. Then you can add a kernel config option to disable that trap-and-emulation code in the future.

Either way, I don't suppose that there should be an option to be insecurely backward compatible.

>
>This version needs openSBI v1.3 *and* a kernel fix that went upstream lately
>(https://lore.kernel.org/lkml/20230616114831.3186980-1-maz@xxxxxxxxxx/T/).
>
>**Important**: In this version, the default mode is now user access, not
>the legacy so some applications will break.
>
>base-commit-tag: v6.4-rc6
>
>Changes in v4:
>- Fixed some nits in riscv_pmu_sbi.c thanks to Andrew
>- Fixed the documentation thanks to Andrew
>- Added RB from Andrew \o/
>
>Changes in v3:
>- patch 1 now contains the ref to the faulty commit (no Fixes tag as it is only a comment), as Andrew suggested
>- Removed RISCV_PMU_LEGACY_TIME from patch 3, as Andrew suggested
>- Rename RISCV_PMU_PDEV_NAME to "riscv-pmu-sbi", patch4 is just cosmetic now, as Andrew suggested
>- Removed a few useless (and wrong) comments, as Andrew suggested
>- Simplify arch_perf_update_userpage code, as Andrew suggested
>- Documentation now mentions that time CSR is *always* accessible, whatever the mode, as suggested by Andrew
>- Removed CYCLEH reference and add TODO for rv32 support, as suggested by Atish
>- Do not rename the pmu instance as Atish suggested
>- Set pmc_width only if rdpmc is enabled and CONFIG_RISCV_PMU is set and the event is a hw event
>- Move arch_perf_update_userpage https://lore.kernel.org/lkml/20230616114831.3186980-1-maz@xxxxxxxxxx/T/
>- **Switch to user mode access by default**
>
>Changes in v2:
>- Split into smaller patches, way better!
>- Add RB from Conor
>- Simplify the way we checked riscv architecture
>- Fix race mmap and other thread running on other cpus
>- Use hwc when available
>- Set all userspace access flags in event_init, too cumbersome to handle sysctl changes
>- Fix arch_perf_update_userpage for pmu other than riscv-pmu by renaming pmu driver
>- Fixed kernel test robot build error
>- Fixed documentation (Andrew and Bagas)
>- perf testsuite passes mmap tests in all 3 modes
>
>Alexandre Ghiti (10):
>  perf: Fix wrong comment about default event_idx
>  include: riscv: Fix wrong include guard in riscv_pmu.h
>  riscv: Make legacy counter enum match the HW numbering
>  drivers: perf: Rename riscv pmu sbi driver
>  riscv: Prepare for user-space perf event mmap support
>  drivers: perf: Implement perf event mmap support in the legacy backend
>  drivers: perf: Implement perf event mmap support in the SBI backend
>  Documentation: admin-guide: Add riscv sysctl_perf_user_access
>  tools: lib: perf: Implement riscv mmap support
>  perf: tests: Adapt mmap-basic.c for riscv
>
> Documentation/admin-guide/sysctl/kernel.rst |  27 ++-
> drivers/perf/riscv_pmu.c                    | 113 +++++++++++
> drivers/perf/riscv_pmu_legacy.c             |  28 ++-
> drivers/perf/riscv_pmu_sbi.c                | 196 +++++++++++++++++++-
> include/linux/perf/riscv_pmu.h              |  12 +-
> include/linux/perf_event.h                  |   3 +-
> tools/lib/perf/mmap.c                       |  65 +++++++
> tools/perf/tests/mmap-basic.c               |   4 +-
> 8 files changed, 428 insertions(+), 20 deletions(-)
>