Re: [PATCH] prctl.2: Add PR_RISCV_SET_ICACHE_FLUSH_CTX

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5698 bytes --]

Hi Charlie,

On Wed, Jan 24, 2024 at 06:59:04PM -0800, Charlie Jenkins wrote:
> I have proposed and documented the PR_RISCV_SET_ICACHE_FLUSH_CTX flag
> for prctl(2) to LKML. It has been reviewed and is expected to land
> during the Linux version 6.9 merge window. This adds the relevant
> documentation from that patch.
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>

This patch triggers a few warnings:

	$ make lint check -k
	TROFF	.tmp/man/man2/prctl.2.cat.set
	an.tmac:man2/prctl.2:1155: style: .IR expects at least 2 arguments, got 1
	an.tmac:man2/prctl.2:1157: style: .IR expects at least 2 arguments, got 1
	an.tmac:man2/prctl.2:1179: style: .IR expects at least 2 arguments, got 1
	an.tmac:man2/prctl.2:1196: style: .IR expects at least 2 arguments, got 1
	make: *** [share/mk/build/catman.mk:54: .tmp/man/man2/prctl.2.cat.set] Error 1
	make: *** Deleting file '.tmp/man/man2/prctl.2.cat.set'
	make: Target 'check' not remade because of errors.

See <CONTRIBUTING.d/lint> for that.

> ---
> I have proposed and documented the PR_RISCV_SET_ICACHE_FLUSH_CTX
> flag for prctl(2) to LKML. It has been reviewed and is expected to land
> during the Linux version 6.9 merge window.
> ---
>  man2/prctl.2 | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/man2/prctl.2 b/man2/prctl.2
> index f1604a7cb..2889a7195 100644
> --- a/man2/prctl.2
> +++ b/man2/prctl.2
> @@ -1147,6 +1147,64 @@ For further information, see the kernel source file
>  (or
>  .I Documentation/security/Yama.txt
>  before Linux 4.13).
> +.\" prctl PR_RISCV_SET_ICACHE_FLUSH_CTX
> +.TP
> +.BR PR_RISCV_SET_ICACHE_FLUSH_CTX " (since Linux 6.9, RISC-V only)"
> +Enable/disable icache flushing instructions in userspace. The ctx and the

'ctx' isn't a common word.  The first time you mention it you should
probably say what it means.  For example:  "The context (ctx) and
the ..." and later just use ctx.

> +scope can be provided using
> +.IR arg2

This should be I, not IR.

> +and
> +.IR arg3
> +respectively. When scope is set to

Please use semantic newlines.  See man-pages(7):

   Use semantic newlines
     In  the  source of a manual page, new sentences should be started
     on new lines, long sentences should be split into lines at clause
     breaks (commas, semicolons, colons, and so on), and long  clauses
     should be split at phrase boundaries.  This convention, sometimes
     known  as  "semantic newlines", makes it easier to see the effect
     of patches, which often operate at the level of  individual  sen‐
     tences, clauses, or phrases.

> +.B PR_RISCV_SCOPE_PER_PROCESS ,

This should be BR, not B.  See groff_man(7).

> +all threads in the process are permitted to emit icache flushing
> +instructions. Whenever any thread in the process is migrated, the
> +corresponding hart's icache will be guaranteed to be consistent with
> +instruction storage. Note this does not enforce any guarantees outside of

'Note' is superfluous.  You can just remove that word.  "This does not
enforce ...".

> +migration. If a thread modifies an instruction that another thread may
> +attempt to execute, the other thread must still emit an icache flushing
> +instruction before attempting to execute the potentially modified
> +instruction. This must be performed by the userspace program.

We say "user space"; two words (see man-pages(7)).  In this case, since
it's an adjective, it would be "user-space", with a hyphen.

> +.IP
> +In per-thread context (eg. scope is set to
> +.B PR_RISCV_SCOPE_PER_THREAD )

The comma should go with the parenthesis.  Otherwise, you'll see a
spurious space in between.

> +, only the thread calling this function is permitted to emit icache
> +flushing instructions. When the thread is migrated, the corresponding
> +hart's icache will be guaranteed to be consistent with instruction storage.
> +.IP
> +On kernels configured without SMP, this function is a nop as migrations
> +across harts will not occur.
> +.IP
> +The following values for
> +.IR arg2
> +can be specified:
> +.RS
> +.TP
> +.BR PR_RISCV_CTX_SW_FENCEI_ON " (since Linux 6.9)"
> +Allow fence.i in userspace.

"user space".

> +.TP
> +.BR PR_RISCV_CTX_SW_FENCEI_OFF " (since Linux 6.9)"
> +Disallow fence.i in userspace. All threads in a process will be affected

.

> +when scope is set to
> +.B PR_RISCV_SCOPE_PER_PROCESS .
> +Therefore, caution must be taken -- only use this flag when you can

Use \[em] for an em dash.  Although in this case I think ':' or ';'
would be more appropriate.

Also, I think 'only' would go better right before "when".

> +guarantee that no thread in the process will emit fence.i from this point
> +onward.
> +.RE
> +.IP
> +The following values for
> +.IR arg3
> +can be specified:
> +.RS
> +.TP
> +.BR PR_RISCV_SCOPE_PER_PROCESS " (since Linux 6.9)"
> +Ensure the icache of any thread in this process is coherent with
> +instruction storage upon migration.
> +.TP
> +.BR PR_RISCV_SCOPE_PER_THREAD " (since Linux 6.9)"
> +Ensure the icache of the current thread is coherent with instruction
> +storage upon migration.
> +.RE
>  .\" prctl PR_SET_SECCOMP
>  .TP
>  .BR PR_SET_SECCOMP " (since Linux 2.6.23)"
> 
> ---
> base-commit: a81e893f2b9316869e6098c3a079c30a48158092
> change-id: 20240124-fencei_prctl-c24da2643379
> 
> Best regards,
> -- 
> Charlie Jenkins <charlie@rivosinc.com>

Thanks for the patch!

Have a lovely day,
Alex

-- 
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]