diff mbox series

Documentation: riscv: note that counter access is part of the uABI

Message ID 20221201135110.3855965-1-conor.dooley@microchip.com
State New
Headers
Series Documentation: riscv: note that counter access is part of the uABI |

Commit Message

Conor Dooley Dec. 1, 2022, 1:51 p.m. UTC 
  Commit 5a5294fbe020 ("RISC-V: Re-enable counter access from userspace")
fixed userspace access to CYCLE, TIME & INSTRET counters and left a nice
comment in-place about why they must not be restricted. Since we now
have a uABI doc in RISC-V land, add a section documenting it.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Based on an, as yet, unsent v2 of my other uABI changes. I don't expect
it to be applicable, just getting a patch into patchwork while I don't
forget about this.
---
 Documentation/riscv/uabi.rst | 7 +++++++
 1 file changed, 7 insertions(+)


base-commit: 13ee7ef407cfcf63f4f047460ac5bb6ba5a3447d
prerequisite-patch-id: d17a9ffb6fcf99eb683728da98cd50e18cd28fe8
prerequisite-patch-id: 0df4127e3f4a0c02a235fea00bcb69cd94fabb38
prerequisite-patch-id: 171724b870ba212b714ebbded480269accd83733

Comments

Palmer Dabbelt Dec. 1, 2022, 7:21 p.m. UTC | #1 
On Thu, 01 Dec 2022 05:51:10 PST (-0800), Conor Dooley wrote:
> Commit 5a5294fbe020 ("RISC-V: Re-enable counter access from userspace")
> fixed userspace access to CYCLE, TIME & INSTRET counters and left a nice
> comment in-place about why they must not be restricted. Since we now
> have a uABI doc in RISC-V land, add a section documenting it.
>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Based on an, as yet, unsent v2 of my other uABI changes. I don't expect
> it to be applicable, just getting a patch into patchwork while I don't
> forget about this.
> ---
>  Documentation/riscv/uabi.rst | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
> index 8d2651e42fda..638ddce56700 100644
> --- a/Documentation/riscv/uabi.rst
> +++ b/Documentation/riscv/uabi.rst
> @@ -3,6 +3,13 @@
>  RISC-V Linux User ABI
>  =====================
>
> +Counter access
> +--------------
> +
> +Access to the CYCLE, TIME and INSTRET counters, now controlled by the SBI PMU
> +extension, were part of the ISA when the uABI was frozen & so remain accessible
> +from userspace.
> +
>  ISA string ordering in /proc/cpuinfo
>  ------------------------------------
>
>
> base-commit: 13ee7ef407cfcf63f4f047460ac5bb6ba5a3447d
> prerequisite-patch-id: d17a9ffb6fcf99eb683728da98cd50e18cd28fe8
> prerequisite-patch-id: 0df4127e3f4a0c02a235fea00bcb69cd94fabb38
> prerequisite-patch-id: 171724b870ba212b714ebbded480269accd83733

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

I think I merged the last one of these, but if the doc folks pick it up 
that's fine with me.  Otherwise I'll take it when it comes back around, 
so folks have time to take a look.
Jonathan Corbet Dec. 3, 2022, 10:38 a.m. UTC | #2 
Palmer Dabbelt <palmer@dabbelt.com> writes:

> On Thu, 01 Dec 2022 05:51:10 PST (-0800), Conor Dooley wrote:
>> Commit 5a5294fbe020 ("RISC-V: Re-enable counter access from userspace")
>> fixed userspace access to CYCLE, TIME & INSTRET counters and left a nice
>> comment in-place about why they must not be restricted. Since we now
>> have a uABI doc in RISC-V land, add a section documenting it.
>>
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>> Based on an, as yet, unsent v2 of my other uABI changes. I don't expect
>> it to be applicable, just getting a patch into patchwork while I don't
>> forget about this.
>> ---
>>  Documentation/riscv/uabi.rst | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
>> index 8d2651e42fda..638ddce56700 100644
>> --- a/Documentation/riscv/uabi.rst
>> +++ b/Documentation/riscv/uabi.rst
>> @@ -3,6 +3,13 @@
>>  RISC-V Linux User ABI
>>  =====================
>>
>> +Counter access
>> +--------------
>> +
>> +Access to the CYCLE, TIME and INSTRET counters, now controlled by the SBI PMU
>> +extension, were part of the ISA when the uABI was frozen & so remain accessible
>> +from userspace.
>> +
>>  ISA string ordering in /proc/cpuinfo
>>  ------------------------------------
>>
>>
>> base-commit: 13ee7ef407cfcf63f4f047460ac5bb6ba5a3447d
>> prerequisite-patch-id: d17a9ffb6fcf99eb683728da98cd50e18cd28fe8
>> prerequisite-patch-id: 0df4127e3f4a0c02a235fea00bcb69cd94fabb38
>> prerequisite-patch-id: 171724b870ba212b714ebbded480269accd83733
>
> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> I think I merged the last one of these, but if the doc folks pick it up 
> that's fine with me.  Otherwise I'll take it when it comes back around, 
> so folks have time to take a look.

"Doc folks" applied it, thanks. :)

jon
Jonathan Corbet Dec. 3, 2022, 10:45 a.m. UTC | #3 
Jonathan Corbet <corbet@lwn.net> writes:

> Palmer Dabbelt <palmer@dabbelt.com> writes:
>> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
>> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> I think I merged the last one of these, but if the doc folks pick it up 
>> that's fine with me.  Otherwise I'll take it when it comes back around, 
>> so folks have time to take a look.
>
> "Doc folks" applied it, thanks. :)

Actually, I take that back.  I'd missed this part from the patch:

> Based on an, as yet, unsent v2 of my other uABI changes. I don't expect
> it to be applicable, just getting a patch into patchwork while I don't
> forget about this.

...but b4 happily picked up a couple of *other* patches from this thread
and applied them instead; I've now undone that.  Sorry for the noise.

jon
Conor Dooley Dec. 3, 2022, 10:56 a.m. UTC | #4 
On Sat, Dec 03, 2022 at 03:45:35AM -0700, Jonathan Corbet wrote:
> Jonathan Corbet <corbet@lwn.net> writes:
> 
> > Palmer Dabbelt <palmer@dabbelt.com> writes:
> >> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> >>
> >> I think I merged the last one of these, but if the doc folks pick it up 
> >> that's fine with me.  Otherwise I'll take it when it comes back around, 
> >> so folks have time to take a look.
> >
> > "Doc folks" applied it, thanks. :)
> 
> Actually, I take that back.  I'd missed this part from the patch:
> 
> > Based on an, as yet, unsent v2 of my other uABI changes. I don't expect
> > it to be applicable, just getting a patch into patchwork while I don't
> > forget about this.
> 
> ...but b4 happily picked up a couple of *other* patches from this thread
> and applied them instead; I've now undone that.  Sorry for the noise.

Huh, I accidentally put an "in-reply-to" header on this patch. I have
been updating some of my submission helper scripts & I must have left
the field populated from sending another set by accident:
https://lore.kernel.org/linux-riscv/20221129144742.2935581-1-conor.dooley@microchip.com/

Apologies!
Atish Patra Jan. 9, 2023, 9:36 p.m. UTC | #5 
On Sat, Dec 3, 2022 at 2:57 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Sat, Dec 03, 2022 at 03:45:35AM -0700, Jonathan Corbet wrote:
> > Jonathan Corbet <corbet@lwn.net> writes:
> >
> > > Palmer Dabbelt <palmer@dabbelt.com> writes:
> > >> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> > >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> > >>
> > >> I think I merged the last one of these, but if the doc folks pick it up
> > >> that's fine with me.  Otherwise I'll take it when it comes back around,
> > >> so folks have time to take a look.
> > >
> > > "Doc folks" applied it, thanks. :)
> >
> > Actually, I take that back.  I'd missed this part from the patch:
> >
> > > Based on an, as yet, unsent v2 of my other uABI changes. I don't expect
> > > it to be applicable, just getting a patch into patchwork while I don't
> > > forget about this.
> >
> > ...but b4 happily picked up a couple of *other* patches from this thread
> > and applied them instead; I've now undone that.  Sorry for the noise.
>
> Huh, I accidentally put an "in-reply-to" header on this patch. I have
> been updating some of my submission helper scripts & I must have left
> the field populated from sending another set by accident:
> https://lore.kernel.org/linux-riscv/20221129144742.2935581-1-conor.dooley@microchip.com/
>

I don't see the patch upstream. Is this patch merged already ?
If not, please hold on merging this for now. We had some discussions
around this in the perf community.
Here is the thread
https://lore.kernel.org/lkml/Y7gN32eHJNyWBvVD@FVFF77S0Q05N/T/

TLDR; Even though x86 allows unrestricted access through rdpmc (not
default), it still reads zero unless a privileged root user modifies
the MSR interface exposed by the kernel.

Quoting PeterZ

"RDPMC is only useful if you read counters you own on yourself -- IOW
selfmonitoring, using the interface outlined in uapi/linux/perf_events.h
near struct perf_event_mmap_page.

Any other usage -- you get to keep the pieces."

"Anyway, given RISC-V being a very young platform, I would try really
*really* *REALLY* hard to stomp on these applications and get them to
change in order to reclaim the PMU usage."

We need to decide what's the best approach for RISC-V. The current
text in uABI will let users assume that
cycle/instret can be read without any issues which is wrong.

There are few options what we can do for RISC-V:

1. We can trap n emulate and report 0 always as suggested by Mark in
that thread.
2. Continue to allow the user to read the counters directly but
expects the garbage value depending on the other activities
on the system. Hopefully, folks will fix their application by that time.

Once we have the procfs interface, we enforce the behavior by breaking
the application.

In either case, the uABI needs to be updated accordingly.

> Apologies!
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



--
Regards,
Atish
Conor Dooley Jan. 9, 2023, 9:46 p.m. UTC | #6 
On Mon, Jan 09, 2023 at 01:36:19PM -0800, Atish Patra wrote:
> On Sat, Dec 3, 2022 at 2:57 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Sat, Dec 03, 2022 at 03:45:35AM -0700, Jonathan Corbet wrote:
> > > Jonathan Corbet <corbet@lwn.net> writes:
> > >
> > > > Palmer Dabbelt <palmer@dabbelt.com> writes:
> > > >> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> > > >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> > > >>
> > > >> I think I merged the last one of these, but if the doc folks pick it up
> > > >> that's fine with me.  Otherwise I'll take it when it comes back around,
> > > >> so folks have time to take a look.
> > > >
> > > > "Doc folks" applied it, thanks. :)
> > >
> > > Actually, I take that back.  I'd missed this part from the patch:
> > >
> > > > Based on an, as yet, unsent v2 of my other uABI changes. I don't expect
> > > > it to be applicable, just getting a patch into patchwork while I don't
> > > > forget about this.
> > >
> > > ...but b4 happily picked up a couple of *other* patches from this thread
> > > and applied them instead; I've now undone that.  Sorry for the noise.
> >
> > Huh, I accidentally put an "in-reply-to" header on this patch. I have
> > been updating some of my submission helper scripts & I must have left
> > the field populated from sending another set by accident:
> > https://lore.kernel.org/linux-riscv/20221129144742.2935581-1-conor.dooley@microchip.com/
> >
> 
> I don't see the patch upstream. Is this patch merged already ?

No it's not. I was going to resend it actually, but I noticed the thread
yesterday or something. Not sure why you CC'ed half the world on it but
not linux-riscv? Or me if you didn't want me resending it!

> If not, please hold on merging this for now. We had some discussions
> around this in the perf community.
> Here is the thread
> https://lore.kernel.org/lkml/Y7gN32eHJNyWBvVD@FVFF77S0Q05N/T/
> 
> TLDR; Even though x86 allows unrestricted access through rdpmc (not
> default), it still reads zero unless a privileged root user modifies
> the MSR interface exposed by the kernel.
> 
> Quoting PeterZ
> 
> "RDPMC is only useful if you read counters you own on yourself -- IOW
> selfmonitoring, using the interface outlined in uapi/linux/perf_events.h
> near struct perf_event_mmap_page.
> 
> Any other usage -- you get to keep the pieces."
> 
> "Anyway, given RISC-V being a very young platform, I would try really
> *really* *REALLY* hard to stomp on these applications and get them to
> change in order to reclaim the PMU usage."
> 
> We need to decide what's the best approach for RISC-V. The current
> text in uABI will let users assume that
> cycle/instret can be read without any issues which is wrong.
> 
> There are few options what we can do for RISC-V:
> 
> 1. We can trap n emulate and report 0 always as suggested by Mark in
> that thread.
> 2. Continue to allow the user to read the counters directly but
> expects the garbage value depending on the other activities
> on the system. Hopefully, folks will fix their application by that time.
> 
> Once we have the procfs interface, we enforce the behavior by breaking
> the application.
> 
> In either case, the uABI needs to be updated accordingly.

I don't disagree with what I saw either PeterZ or Mark say. I'm happy
not to resend this, I've got no skin in the game other than not wanting
something like that "documented" only in the driver.

What do you intend doing to instigate the "stomping"?

Thanks,
Conor.
diff mbox series

Patch

diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
index 8d2651e42fda..638ddce56700 100644
--- a/Documentation/riscv/uabi.rst
+++ b/Documentation/riscv/uabi.rst
@@ -3,6 +3,13 @@ 
 RISC-V Linux User ABI
 =====================
 
+Counter access
+--------------
+
+Access to the CYCLE, TIME and INSTRET counters, now controlled by the SBI PMU
+extension, were part of the ISA when the uABI was frozen & so remain accessible
+from userspace.
+
 ISA string ordering in /proc/cpuinfo
 ------------------------------------