* rdcycle from userland with RISCV_PMU_SBI=y
@ 2022-09-01 21:15 Aurelien Jarno
[not found] ` <CAOnJCUJ-E-VyQOyh+psbJP4mDO9F+ShvnxDYjoCUUkH4rddJtw@mail.gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Aurelien Jarno @ 2022-09-01 21:15 UTC (permalink / raw)
To: linux-riscv; +Cc: Atish Patra, Mathieu Malaterre
Hi all,
Mathieu Malaterre reported an issue in Debian with the highway package,
which uses the rdcycle pseudo instruction, and started to fail recently.
We tracked down an issue to the "perf platform driver based on SBI PMU
extension" recently added in Linux 5.18 [1]. On a board which runs an
SBI with the PMU extension, this causes the rdcycle pseudo instruction
to generate a SIGILL. This is because the driver explicitly set
CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not
rdcycle.
I wonder if this change is intentional, and if there is a reason to
forbid using the rdcycle pseudo instruction from userland. If it is the
case, this should probably be changed so that the behaviour does not
differ from board to board depending on the available PMU extension.
Aurelien
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9991434596f5373dfd75857b445eb92a9253c56
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/riscv_pmu_sbi.c?id=2880e1a175b9f31798f9d9482ee49187f61b5539#n649
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 8+ messages in thread[parent not found: <CAOnJCUJ-E-VyQOyh+psbJP4mDO9F+ShvnxDYjoCUUkH4rddJtw@mail.gmail.com>]
[parent not found: <CAOnJCUKDQfCD7mh-RWrq0E8a3Fmp4Xz348THrS+SmJ71GX8=PA@mail.gmail.com>]
* Re: rdcycle from userland with RISCV_PMU_SBI=y [not found] ` <CAOnJCUKDQfCD7mh-RWrq0E8a3Fmp4Xz348THrS+SmJ71GX8=PA@mail.gmail.com> @ 2022-09-02 7:52 ` Mathieu Malaterre 2022-09-02 8:19 ` Anup Patel 0 siblings, 1 reply; 8+ messages in thread From: Mathieu Malaterre @ 2022-09-02 7:52 UTC (permalink / raw) To: Atish Patra Cc: linux-riscv, Aurelien Jarno, Anup Patel, Paul Walmsley, Jan Wassenberg Atish, On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote: > > > > On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote: >> >> >> >> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@aurel32.net> wrote: >>> >>> Hi all, >>> >>> Mathieu Malaterre reported an issue in Debian with the highway package, >>> which uses the rdcycle pseudo instruction, and started to fail recently. >>> >>> We tracked down an issue to the "perf platform driver based on SBI PMU >>> extension" recently added in Linux 5.18 [1]. On a board which runs an >>> SBI with the PMU extension, this causes the rdcycle pseudo instruction >>> to generate a SIGILL. This is because the driver explicitly set >>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not >>> rdcycle. >>> >>> I wonder if this change is intentional, and if there is a reason to >>> forbid using the rdcycle pseudo instruction from userland. If it is the >>> case, this should probably be changed so that the behaviour does not >>> differ from board to board depending on the available PMU extension. >>> >> >> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle & >> instructions of the entire kernel always which can lead to some sort of side channel attacks. >> >> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle >> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren. >> > > Merging the thread from sw-dev: > > " > > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley@sifive.com> wrote: > > >> > > >> If I recall correctly, other architectures don't allow direct access to their cycle counters from userspace for security reasons. Any reason why RISC-V shouldn't follow the same approach? > > > > > > > > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ? > > > > As already pointed out, we can't compromise security by having all > > apps unrestricted access to the cycle counter. We already have the > > Linux perf subsystem for managing counters so apps should always use > > Linux perf syscalls. > > > > IMO, the package "highway" directly accessing the cycle counter should > > be fixed instead of fixing the Linux SBI PMU driver. > > > > Further investigating the highway project > > (https://github.com/google/highway), it seems this project is using > > "rdcycle" to track timer ticks which is totally wrong. Instead the use > > of "rdcycle" should be replaced with "rdtime" in this project. > > (Refer, line 156 of > > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc) > > Pay attention that quite a few projects are using rdcycle in user-space already: > > * https://codesearch.debian.net/search?q=%22rdcycle+%250%22 > > Anyway if you believe rdtime is the right fix, we can fix one project > at a time..." > > Thanks for sharing the list. I took a quick look. The first few ones at least (firefox, llvm, chrome) > seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86. > The equivalent instruction in RISC-V should be rdtime not rdcycle. [CC-ing highway main dev] Here is his comment on the rdtime patch proposal: > For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rdcycle from userland with RISCV_PMU_SBI=y 2022-09-02 7:52 ` Mathieu Malaterre @ 2022-09-02 8:19 ` Anup Patel [not found] ` <CAGRC-mj-v6RoGjCNr_TiabgqYX7H9mTXYE9tx6BhCKT5nHD-Aw@mail.gmail.com> 0 siblings, 1 reply; 8+ messages in thread From: Anup Patel @ 2022-09-02 8:19 UTC (permalink / raw) To: Mathieu Malaterre Cc: Atish Patra, linux-riscv, Aurelien Jarno, Anup Patel, Paul Walmsley, Jan Wassenberg On Fri, Sep 2, 2022 at 1:23 PM Mathieu Malaterre <malat@debian.org> wrote: > > Atish, > > On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote: > > > > > > > > On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote: > >> > >> > >> > >> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@aurel32.net> wrote: > >>> > >>> Hi all, > >>> > >>> Mathieu Malaterre reported an issue in Debian with the highway package, > >>> which uses the rdcycle pseudo instruction, and started to fail recently. > >>> > >>> We tracked down an issue to the "perf platform driver based on SBI PMU > >>> extension" recently added in Linux 5.18 [1]. On a board which runs an > >>> SBI with the PMU extension, this causes the rdcycle pseudo instruction > >>> to generate a SIGILL. This is because the driver explicitly set > >>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not > >>> rdcycle. > >>> > >>> I wonder if this change is intentional, and if there is a reason to > >>> forbid using the rdcycle pseudo instruction from userland. If it is the > >>> case, this should probably be changed so that the behaviour does not > >>> differ from board to board depending on the available PMU extension. > >>> > >> > >> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle & > >> instructions of the entire kernel always which can lead to some sort of side channel attacks. > >> > >> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle > >> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren. > >> > > > > Merging the thread from sw-dev: > > > > " > > > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley@sifive.com> wrote: > > > >> > > > >> If I recall correctly, other architectures don't allow direct access to their cycle counters from userspace for security reasons. Any reason why RISC-V shouldn't follow the same approach? > > > > > > > > > > > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ? > > > > > > As already pointed out, we can't compromise security by having all > > > apps unrestricted access to the cycle counter. We already have the > > > Linux perf subsystem for managing counters so apps should always use > > > Linux perf syscalls. > > > > > > IMO, the package "highway" directly accessing the cycle counter should > > > be fixed instead of fixing the Linux SBI PMU driver. > > > > > > Further investigating the highway project > > > (https://github.com/google/highway), it seems this project is using > > > "rdcycle" to track timer ticks which is totally wrong. Instead the use > > > of "rdcycle" should be replaced with "rdtime" in this project. > > > (Refer, line 156 of > > > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc) > > > > Pay attention that quite a few projects are using rdcycle in user-space already: > > > > * https://codesearch.debian.net/search?q=%22rdcycle+%250%22 > > > > Anyway if you believe rdtime is the right fix, we can fix one project > > at a time..." > > > > Thanks for sharing the list. I took a quick look. The first few ones at least (firefox, llvm, chrome) > > seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86. > > The equivalent instruction in RISC-V should be rdtime not rdcycle. > > [CC-ing highway main dev] > > Here is his comment on the rdtime patch proposal: > > > For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful. The RISC-V spec does not make any guarantee about granularity of the cycle counter as well. In fact, on systems with cpufreq enabled the rate of cycle counter increment will vary at runtime. On other hand, the time CSR (i.e. rdtime) will always increment at a fixed platform specific rate which can be read from "/proc/device-tree/cpus/timebase-frequency". Regards, Anup > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CAGRC-mj-v6RoGjCNr_TiabgqYX7H9mTXYE9tx6BhCKT5nHD-Aw@mail.gmail.com>]
* Re: rdcycle from userland with RISCV_PMU_SBI=y [not found] ` <CAGRC-mj-v6RoGjCNr_TiabgqYX7H9mTXYE9tx6BhCKT5nHD-Aw@mail.gmail.com> @ 2022-09-02 10:31 ` Jan Wassenberg 2022-09-02 12:04 ` Anup Patel 1 sibling, 0 replies; 8+ messages in thread From: Jan Wassenberg @ 2022-09-02 10:31 UTC (permalink / raw) To: Anup Patel Cc: Mathieu Malaterre, Atish Patra, linux-riscv, Aurelien Jarno, Anup Patel, Paul Walmsley, Jan Newger (Forwarding in plain text mode as apparently required by the list, apologies for the duplication) On Fri, Sep 2, 2022 at 12:22 PM Jan Wassenberg <janwas@google.com> wrote: > > OK, but even if frequency changes, a cycle counter seems more useful for benchmarking than a low-resolution wall time. > > FYI it's not just Highway, the Google benchmark library also uses rdcycle. > > Some personal opinions on security (disclaimer: I have some experience but am not an expert): > Disabling cycle counters does not seem to provide any extra security on a multi-core system unless > it also detects and mitigates software timers. > See section 6 in http://cal.cs.columbia.edu/papers/isca12_timewarp.pdf. In short, other core(s) can increment a shared counter, > which is close enough to a cycle counter to serve as a side channel. Detecting such coordination via performance counters > seems feasible, though it risks false alarms for something like spinlocks. > > Reducing timer resolution could still be defeated by interpolating. If we're determined to do something, > perhaps the cycle counter could have some jitter (> LLC latency), ideally only enabled if > a possible attack is detected by trapping rdcycle and checking the elapsed time since the last rdcycle. > > On Fri, Sep 2, 2022 at 10:19 AM Anup Patel <anup@brainfault.org> wrote: >> >> On Fri, Sep 2, 2022 at 1:23 PM Mathieu Malaterre <malat@debian.org> wrote: >> > >> > Atish, >> > >> > On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote: >> > > >> > > >> > > >> > > On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote: >> > >> >> > >> >> > >> >> > >> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@aurel32.net> wrote: >> > >>> >> > >>> Hi all, >> > >>> >> > >>> Mathieu Malaterre reported an issue in Debian with the highway package, >> > >>> which uses the rdcycle pseudo instruction, and started to fail recently. >> > >>> >> > >>> We tracked down an issue to the "perf platform driver based on SBI PMU >> > >>> extension" recently added in Linux 5.18 [1]. On a board which runs an >> > >>> SBI with the PMU extension, this causes the rdcycle pseudo instruction >> > >>> to generate a SIGILL. This is because the driver explicitly set >> > >>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not >> > >>> rdcycle. >> > >>> >> > >>> I wonder if this change is intentional, and if there is a reason to >> > >>> forbid using the rdcycle pseudo instruction from userland. If it is the >> > >>> case, this should probably be changed so that the behaviour does not >> > >>> differ from board to board depending on the available PMU extension. >> > >>> >> > >> >> > >> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle & >> > >> instructions of the entire kernel always which can lead to some sort of side channel attacks. >> > >> >> > >> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle >> > >> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren. >> > >> >> > > >> > > Merging the thread from sw-dev: >> > > >> > > " >> > > > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley@sifive.com> wrote: >> > > > >> >> > > > >> If I recall correctly, other architectures don't allow direct access to their cycle counters from userspace for security reasons. Any reason why RISC-V shouldn't follow the same approach? >> > > > > >> > > > > >> > > > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ? >> > > > >> > > > As already pointed out, we can't compromise security by having all >> > > > apps unrestricted access to the cycle counter. We already have the >> > > > Linux perf subsystem for managing counters so apps should always use >> > > > Linux perf syscalls. >> > > > >> > > > IMO, the package "highway" directly accessing the cycle counter should >> > > > be fixed instead of fixing the Linux SBI PMU driver. >> > > > >> > > > Further investigating the highway project >> > > > (https://github.com/google/highway), it seems this project is using >> > > > "rdcycle" to track timer ticks which is totally wrong. Instead the use >> > > > of "rdcycle" should be replaced with "rdtime" in this project. >> > > > (Refer, line 156 of >> > > > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc) >> > > >> > > Pay attention that quite a few projects are using rdcycle in user-space already: >> > > >> > > * https://codesearch.debian.net/search?q=%22rdcycle+%250%22 >> > > >> > > Anyway if you believe rdtime is the right fix, we can fix one project >> > > at a time..." >> > > >> > > Thanks for sharing the list. I took a quick look. The first few ones at least (firefox, llvm, chrome) >> > > seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86. >> > > The equivalent instruction in RISC-V should be rdtime not rdcycle. >> > >> > [CC-ing highway main dev] >> > >> > Here is his comment on the rdtime patch proposal: >> > >> > > For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful. >> >> The RISC-V spec does not make any guarantee about granularity of the >> cycle counter as well. In fact, on systems with cpufreq enabled the >> rate of cycle counter increment will vary at runtime. >> >> On other hand, the time CSR (i.e. rdtime) will always increment at a >> fixed platform specific rate which can be read from >> "/proc/device-tree/cpus/timebase-frequency". >> >> Regards, >> Anup >> >> >> > >> > _______________________________________________ >> > linux-riscv mailing list >> > linux-riscv@lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/linux-riscv _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rdcycle from userland with RISCV_PMU_SBI=y [not found] ` <CAGRC-mj-v6RoGjCNr_TiabgqYX7H9mTXYE9tx6BhCKT5nHD-Aw@mail.gmail.com> 2022-09-02 10:31 ` Jan Wassenberg @ 2022-09-02 12:04 ` Anup Patel 2022-09-02 16:46 ` Aurelien Jarno 2022-09-05 5:40 ` Jan Wassenberg 1 sibling, 2 replies; 8+ messages in thread From: Anup Patel @ 2022-09-02 12:04 UTC (permalink / raw) To: Jan Wassenberg Cc: Anup Patel, Mathieu Malaterre, Atish Patra, linux-riscv, Aurelien Jarno, Paul Walmsley, Jan Newger On Fri, Sep 2, 2022 at 3:52 PM Jan Wassenberg <janwas@google.com> wrote: > > OK, but even if frequency changes, a cycle counter seems more useful for benchmarking than a low-resolution wall time. > > FYI it's not just Highway, the Google benchmark library also uses rdcycle. Since the RISC-V ecosystem is still evolving, it is best time to fix things now rather than later. Currently, only "time" CSR is available to user-space and all other performance counters should be enabled (or accessed) using Linux perf syscalls. > > Some personal opinions on security (disclaimer: I have some experience but am not an expert): > Disabling cycle counters does not seem to provide any extra security on a multi-core system unless > it also detects and mitigates software timers. > See section 6 in http://cal.cs.columbia.edu/papers/isca12_timewarp.pdf. In short, other core(s) can increment a shared counter, > which is close enough to a cycle counter to serve as a side channel. Detecting such coordination via performance counters > seems feasible, though it risks false alarms for something like spinlocks. At the moment, there is no HW mechanism defined in RISC-V to obfuscate "time" or "cycle" CSRs for user-space. On the other hand, using "cycle" for measuring passing time is semantically incorrect. In the future, someone can always propose a RISC-V ISA extension to obfuscate "time" CSR values visible to user-space but the "cycle" counter is purely a performance counter and accurately reflects cycles taken by the CPU. > > Reducing timer resolution could still be defeated by interpolating. If we're determined to do something, > perhaps the cycle counter could have some jitter (> LLC latency), ideally only enabled if > a possible attack is detected by trapping rdcycle and checking the elapsed time since the last rdcycle. The "cycle" counter is extensively used for performance analysis so it is very unlikely there might any ISA extension created to add jitters in performance counters. The "time" CSR (i.e. rdtime) on other hand is only used for measuring passing time so an ISA extension is possible to add some jitter. For example, in H-extension we already have "htimedelta" CSR to add some delta to the "time" CSR values seen by each Guest/VM under a hypervisor. Regards, Anup > > On Fri, Sep 2, 2022 at 10:19 AM Anup Patel <anup@brainfault.org> wrote: >> >> On Fri, Sep 2, 2022 at 1:23 PM Mathieu Malaterre <malat@debian.org> wrote: >> > >> > Atish, >> > >> > On Fri, Sep 2, 2022 at 9:12 AM Atish Patra <atishp@atishpatra.org> wrote: >> > > >> > > >> > > >> > > On Thu, Sep 1, 2022 at 3:34 PM Atish Patra <atishp@atishpatra.org> wrote: >> > >> >> > >> >> > >> >> > >> On Thu, Sep 1, 2022 at 2:15 PM Aurelien Jarno <aurelien@aurel32.net> wrote: >> > >>> >> > >>> Hi all, >> > >>> >> > >>> Mathieu Malaterre reported an issue in Debian with the highway package, >> > >>> which uses the rdcycle pseudo instruction, and started to fail recently. >> > >>> >> > >>> We tracked down an issue to the "perf platform driver based on SBI PMU >> > >>> extension" recently added in Linux 5.18 [1]. On a board which runs an >> > >>> SBI with the PMU extension, this causes the rdcycle pseudo instruction >> > >>> to generate a SIGILL. This is because the driver explicitly set >> > >>> CSR_SCOUNTEREN to 0x02, giving access from userland to rdtime but not >> > >>> rdcycle. >> > >>> >> > >>> I wonder if this change is intentional, and if there is a reason to >> > >>> forbid using the rdcycle pseudo instruction from userland. If it is the >> > >>> case, this should probably be changed so that the behaviour does not >> > >>> differ from board to board depending on the available PMU extension. >> > >>> >> > >> >> > >> Yes. The change was intentional due to security reasons. One rogue process can have access to cycle & >> > >> instructions of the entire kernel always which can lead to some sort of side channel attacks. >> > >> >> > >> However, I agree that we can't break userspace. I was not aware of the fact that there are already users of rdcycle >> > >> in the userspace. I will send a patch to restore the original behavior by enabling CY, IR bits in scounteren. >> > >> >> > > >> > > Merging the thread from sw-dev: >> > > >> > > " >> > > > > On Thu, Sep 1, 2022 at 7:36 PM Paul Walmsley <paul.walmsley@sifive.com> wrote: >> > > > >> >> > > > >> If I recall correctly, other architectures don't allow direct access to their cycle counters from userspace for security reasons. Any reason why RISC-V shouldn't follow the same approach? >> > > > > >> > > > > >> > > > > That was the intention behind disabling the access in the PMU driver. At that time, I didn’t find any users. Obviously I was wrong 😂. But is that a sufficient reason to existing break user space ? >> > > > >> > > > As already pointed out, we can't compromise security by having all >> > > > apps unrestricted access to the cycle counter. We already have the >> > > > Linux perf subsystem for managing counters so apps should always use >> > > > Linux perf syscalls. >> > > > >> > > > IMO, the package "highway" directly accessing the cycle counter should >> > > > be fixed instead of fixing the Linux SBI PMU driver. >> > > > >> > > > Further investigating the highway project >> > > > (https://github.com/google/highway), it seems this project is using >> > > > "rdcycle" to track timer ticks which is totally wrong. Instead the use >> > > > of "rdcycle" should be replaced with "rdtime" in this project. >> > > > (Refer, line 156 of >> > > > https://github.com/google/highway/blob/master/hwy/nanobenchmark.cc) >> > > >> > > Pay attention that quite a few projects are using rdcycle in user-space already: >> > > >> > > * https://codesearch.debian.net/search?q=%22rdcycle+%250%22 >> > > >> > > Anyway if you believe rdtime is the right fix, we can fix one project >> > > at a time..." >> > > >> > > Thanks for sharing the list. I took a quick look. The first few ones at least (firefox, llvm, chrome) >> > > seems to use rdcycle for timestamp. All of the packages use "rdtsc" for x86. >> > > The equivalent instruction in RISC-V should be rdtime not rdcycle. >> > >> > [CC-ing highway main dev] >> > >> > Here is his comment on the rdtime patch proposal: >> > >> > > For benchmarking we really would prefer a cycle count, rather than "real time clock ticks", for which the spec makes no guarantees. If/when those turn out to be millisecond-resolution, they are not very useful. >> >> The RISC-V spec does not make any guarantee about granularity of the >> cycle counter as well. In fact, on systems with cpufreq enabled the >> rate of cycle counter increment will vary at runtime. >> >> On other hand, the time CSR (i.e. rdtime) will always increment at a >> fixed platform specific rate which can be read from >> "/proc/device-tree/cpus/timebase-frequency". >> >> Regards, >> Anup >> >> >> > >> > _______________________________________________ >> > linux-riscv mailing list >> > linux-riscv@lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/linux-riscv _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rdcycle from userland with RISCV_PMU_SBI=y 2022-09-02 12:04 ` Anup Patel @ 2022-09-02 16:46 ` Aurelien Jarno 2022-09-05 5:40 ` Jan Wassenberg 1 sibling, 0 replies; 8+ messages in thread From: Aurelien Jarno @ 2022-09-02 16:46 UTC (permalink / raw) To: Anup Patel Cc: Jan Wassenberg, Anup Patel, Mathieu Malaterre, Atish Patra, linux-riscv, Paul Walmsley, Jan Newger On 2022-09-02 17:34, Anup Patel wrote: > On Fri, Sep 2, 2022 at 3:52 PM Jan Wassenberg <janwas@google.com> wrote: > > > > OK, but even if frequency changes, a cycle counter seems more useful for benchmarking than a low-resolution wall time. > > > > FYI it's not just Highway, the Google benchmark library also uses rdcycle. > > Since the RISC-V ecosystem is still evolving, it is best time to fix > things now rather > than later. Fair enough. > Currently, only "time" CSR is available to user-space and all other performance > counters should be enabled (or accessed) using Linux perf syscalls. This is more complex in practice. For a recent kernel, the availability of the "cycle" CSR depends on the PMU driver being used or compiled in the kernel. For instance on both QEMU and a Polarfire Icicle board, the legacy PMU is getting used, so rdcycle is available: [ 34.420435] Legacy PMU implementation is available In the meantime, I upgraded QEMU and with it OpenSBI, so the SBI PMU is used instead, and I should also update it on the Polarfire board, but it is not so easy. Not that for QEMU, the value of scounteren is ignored, so that rdcycle is still available. We should make that consistent so that all users see the same behavior. Regards Aurelien -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rdcycle from userland with RISCV_PMU_SBI=y 2022-09-02 12:04 ` Anup Patel 2022-09-02 16:46 ` Aurelien Jarno @ 2022-09-05 5:40 ` Jan Wassenberg 2022-09-28 13:20 ` Palmer Dabbelt 1 sibling, 1 reply; 8+ messages in thread From: Jan Wassenberg @ 2022-09-05 5:40 UTC (permalink / raw) To: Anup Patel Cc: Anup Patel, Mathieu Malaterre, Atish Patra, linux-riscv, Aurelien Jarno, Paul Walmsley, Jan Newger > Currently, only "time" CSR is available to user-space and all other performance > counters should be enabled (or accessed) using Linux perf syscalls. Unfortunately that would seem to prevent precisely measuring very small code regions (order of 10 cycles). What is the corresponding advantage/benefit? As mentioned, there is no security gain from merely disabling rdcycle. > In the future, someone can always propose a RISC-V ISA extension to obfuscate > "time" CSR values visible to user-space but the "cycle" counter is > purely a performance counter and accurately reflects cycles taken by the CPU. Yes, this is why we use it. I'm curious why we think rdcycle is being used to measure wall time? About consistency: I understand there will be a time when rdcycle may or may not be available on Linux depending on PMU. But there is also bare metal, and other OSes. Is there a way for userland to detect whether rdcycle will fault? _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rdcycle from userland with RISCV_PMU_SBI=y 2022-09-05 5:40 ` Jan Wassenberg @ 2022-09-28 13:20 ` Palmer Dabbelt 0 siblings, 0 replies; 8+ messages in thread From: Palmer Dabbelt @ 2022-09-28 13:20 UTC (permalink / raw) To: janwas Cc: apatel, malat, atishp, linux-riscv, aurelien, Paul Walmsley, jannewger [Sorry if this a mess, it's not showing up in my inbox.] >> Currently, only "time" CSR is available to user-space and all other performance >> counters should be enabled (or accessed) using Linux perf syscalls. > Unfortunately that would seem to prevent precisely measuring very small code > regions (order of 10 cycles). What is the corresponding advantage/benefit? > As mentioned, there is no security gain from merely disabling rdcycle. > >> In the future, someone can always propose a RISC-V ISA extension to obfuscate >> "time" CSR values visible to user-space but the "cycle" counter is >> purely a performance counter and accurately reflects cycles taken by the CPU. > Yes, this is why we use it. I'm curious why we think rdcycle is being > used to measure wall time? > > About consistency: I understand there will be a time when rdcycle may > or may not be available on Linux depending on PMU. > But there is also bare metal, and other OSes. Is there a way for > userland to detect whether rdcycle will fault? We can't just trap rdcycle as SIGILL into userspace, it's part of the uABI. I get the instruction was removed from the ISA, but that doesn't matter: we don't break userspace, regardless of how hard the ISA makes that. I understand there's some worries about the resolution of these counters on some systems, but the solution there is to maintain the uABI by trapping/emulating the removed instructions (and then providing some way for userspace to ask to turn that off, like we'll have to with a bunch of stuff). That's a way's off, though, so for now we need to just stop breaking userspace. There's also some proposals for sorting out the accuracy issues with ISA specs, but that's even farther off. I just sent <https://lore.kernel.org/r/20220928131807.30386-1-palmer@rivosinc.com>, which hopefully fixes the issue. I ended up with COVID so I'm kind of a mess, I can try to find some time to test it but it'd be much appreciated if someone who has a setup already can do so. Sorry for the breakage. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-28 13:20 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-01 21:15 rdcycle from userland with RISCV_PMU_SBI=y Aurelien Jarno
[not found] ` <CAOnJCUJ-E-VyQOyh+psbJP4mDO9F+ShvnxDYjoCUUkH4rddJtw@mail.gmail.com>
[not found] ` <CAOnJCUKDQfCD7mh-RWrq0E8a3Fmp4Xz348THrS+SmJ71GX8=PA@mail.gmail.com>
2022-09-02 7:52 ` Mathieu Malaterre
2022-09-02 8:19 ` Anup Patel
[not found] ` <CAGRC-mj-v6RoGjCNr_TiabgqYX7H9mTXYE9tx6BhCKT5nHD-Aw@mail.gmail.com>
2022-09-02 10:31 ` Jan Wassenberg
2022-09-02 12:04 ` Anup Patel
2022-09-02 16:46 ` Aurelien Jarno
2022-09-05 5:40 ` Jan Wassenberg
2022-09-28 13:20 ` Palmer Dabbelt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.