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

[Atish Patra]

On Mon, Jul 17, 2023 at 6:38 AM Rémi Denis-Courmont <remi@xxxxxxxxxx> wrote:
>
> 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.
>

This case is a special case as the usage was incorrect in the first
place. Any application that genuinely requires rdcycle can always get
it now via the perf interface.
If the insecure and incorrect behavior is allowed, we suspect the user
space behavior will never be fixed as it is hard to put a future flag
date in these cases.
Given that the RISC-V eco-system is still young, it is better to
disallow this behavior to avoid further proliferation of the same
incorrect usage.

> 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.
>

What do you mean by "sanitised" value ? If you mean the correct value,
it doesn't know when to stop providing that value (without reinventing
the entire context switch which the perf framework already does for
us).
If we just provide a dummy value, that doesn't help the current users
as well. They will probably get unexpected results instead of sigill.

I have cc'd a few folks who were involved in the initial discussions
when this issue was reported to collect the feedback.

> 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(-)
> >
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish