[PATCH v1 0/2] RISC-V: enable rust

[PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

After the authorship debacle on the RFC, I've tried to be even more
careful this time around. Gary opted for a Co-developed-by in the replies
of the RFC stuff, so I have given them one.
I have added SoB's too, but if that is not okay Gary, then please scream
loudly.

As this is lifted from the state of the Rust-for-Linux tree, the commit
messages from there cannot be preserved, so these patches have commit
messages that I wrote.

I've tested this on Icicle, and the modules seem to work as expected.
Unfortunately there appear to be implicit 32-bit divisions (or similar)
in core Rust code, so, as in the downstream Rust-for-Linux tree, Rust is
only enabled for 64-bit.

Thanks,
Conor.

Changes in v1:
- rebase on v6.3-rc1
- resort the `um` entry in the arch-support table while adding RISC-V
  to it
- drop 32-bit bits
- have another crack at assigning authorship

Changes in RFC-RESEND:
- fix the asymmetrical additions in the Makefile bits
- add cc-cover to my git send-email command...

CC: Miguel Ojeda <ojeda@kernel.org>
CC: Alex Gaynor <alex.gaynor@gmail.com>
CC: Wedson Almeida Filho <wedsonaf@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Gary Guo <gary@garyguo.net>
CC: Björn Roy Baron <bjorn3_gh@protonmail.com>
CC: Jonathan Corbet <corbet@lwn.net>
CC: Paul Walmsley <paul.walmsley@sifive.com>
CC: Palmer Dabbelt <palmer@dabbelt.com>
CC: Nathan Chancellor <nathan@kernel.org>
CC: Nick Desaulniers <ndesaulniers@google.com>
CC: Tom Rix <trix@redhat.com>
CC: rust-for-linux@vger.kernel.org
CC: linux-doc@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-riscv@lists.infradead.org
CC: llvm@lists.linux.dev

Miguel Ojeda (2):
  scripts: generate_rust_target: enable building on RISC-V
  RISC-V: enable building 64-bit kernels with rust support

 Documentation/rust/arch-support.rst |  3 ++-
 arch/riscv/Kconfig                  |  1 +
 arch/riscv/Makefile                 |  2 ++
 scripts/generate_rust_target.rs     | 16 ++++++++++++++++
 4 files changed, 21 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH v1 1/2] scripts: generate_rust_target: enable building on RISC-V

[Conor Dooley]

From: Miguel Ojeda <ojeda@kernel.org>

Add the required bits from rust-for-linux to enable generating a RISC-V
target for rust. The script, written by Miguel, was originally a
config file contributed by Gary.

Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Despite removing 32-bit support, I kept the structure of the if
statement, despite early return being stylistically preferred, for
alignment with the Rust-for-Linux tree. I'm happy to respin to sort that
out of desired.
---
 scripts/generate_rust_target.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/scripts/generate_rust_target.rs b/scripts/generate_rust_target.rs
index 3c6cbe2b278d3..85d690f764389 100644
--- a/scripts/generate_rust_target.rs
+++ b/scripts/generate_rust_target.rs
@@ -161,6 +161,22 @@ fn main() {
         ts.push("features", features);
         ts.push("llvm-target", "x86_64-linux-gnu");
         ts.push("target-pointer-width", "64");
+    } else if cfg.has("RISCV") {
+        if cfg.has("64BIT") {
+            ts.push("arch", "riscv64");
+            ts.push("data-layout", "e-m:e-p:64:64-i64:64-i128:128-n64-S128");
+            ts.push("llvm-target", "riscv64-linux-gnu");
+            ts.push("target-pointer-width", "64");
+        } else {
+            panic!("32-bit RISC-V is an unsupported architecture")
+        }
+        ts.push("code-model", "medium");
+        ts.push("disable-redzone", true);
+        let mut features = "+m,+a".to_string();
+        if cfg.has("RISCV_ISA_C") {
+            features += ",+c";
+        }
+        ts.push("features", features);
     } else {
         panic!("Unsupported architecture");
     }
-- 
2.39.2

Re: [PATCH v1 1/2] scripts: generate_rust_target: enable building on RISC-V

[Miguel Ojeda]

On Tue, Mar 7, 2023 at 11:25 AM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> Despite removing 32-bit support, I kept the structure of the if
> statement, despite early return being stylistically preferred, for
> alignment with the Rust-for-Linux tree. I'm happy to respin to sort that
> out of desired.

This is a case of 2 "equal" sides to the branch (though at the moment
an error), so it sounds good, and it will mean a smaller diff later.

> +            panic!("32-bit RISC-V is an unsupported architecture")

Nit: if there is a v2, please add a semicolon to be consistent with
the others in the file (not sure which style we will go for, it looks
like `rustfmt` accepts both ways).

Cheers,
Miguel

[PATCH v1 2/2] RISC-V: enable building 64-bit kernels with rust support

[Conor Dooley]

From: Miguel Ojeda <ojeda@kernel.org>

The rust modules work on 64-bit RISC-V, with no twiddling required.
Select HAVE_RUST and provide the required flags to kbuild so that the
modules can be used. The Makefile and Kconfig changes are lifted from
work done by Miguel in the Rust-for-Linux tree, hence his authorship.
Following the rabbit hole, the Makefile changes originated in a script,
created based on config files originally added by Gary, hence his
co-authorship.

32-bit is broken in core rust code, so support is limited to 64-bit:
ld.lld: error: undefined symbol: __udivdi3

As 64-bit RISC-V is now supported, add it to the arch support table,
taking the opportunity to sort the table in alphabetical order.

Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
While adding RISC-V to the table, I took the chance to re-sort it
alphabetically. I didn't think that warranted co-authorship, but given
we had an authorship conversation already I am happy to provide one for
that...
---
 Documentation/rust/arch-support.rst | 3 ++-
 arch/riscv/Kconfig                  | 1 +
 arch/riscv/Makefile                 | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/rust/arch-support.rst b/Documentation/rust/arch-support.rst
index ed7f4f5b3cf15..77765ffd5af41 100644
--- a/Documentation/rust/arch-support.rst
+++ b/Documentation/rust/arch-support.rst
@@ -15,7 +15,8 @@ support corresponds to ``S`` values in the ``MAINTAINERS`` file.
 ============  ================  ==============================================
 Architecture  Level of support  Constraints
 ============  ================  ==============================================
-``x86``       Maintained        ``x86_64`` only.
+``riscv``     Maintained        ``riscv64`` only.
 ``um``        Maintained        ``x86_64`` only.
+``x86``       Maintained        ``x86_64`` only.
 ============  ================  ==============================================
 
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index c5e42cc376048..c3179b139361f 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -114,6 +114,7 @@ config RISCV
 	select HAVE_POSIX_CPU_TIMERS_TASK_WORK
 	select HAVE_REGS_AND_STACK_ACCESS_API
 	select HAVE_RSEQ
+	select HAVE_RUST if 64BIT
 	select HAVE_STACKPROTECTOR
 	select HAVE_SYSCALL_TRACEPOINTS
 	select IRQ_DOMAIN
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 6203c33789228..950612bf193cf 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -31,6 +31,8 @@ ifeq ($(CONFIG_ARCH_RV64I),y)
 	KBUILD_AFLAGS += -mabi=lp64
 
 	KBUILD_LDFLAGS += -melf64lriscv
+
+	KBUILD_RUSTFLAGS += -Ctarget-cpu=generic-rv64
 else
 	BITS := 32
 	UTS_MACHINE := riscv32
-- 
2.39.2

Re: [PATCH v1 2/2] RISC-V: enable building 64-bit kernels with rust support

[Miguel Ojeda]

On Tue, Mar 7, 2023 at 11:25 AM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> While adding RISC-V to the table, I took the chance to re-sort it
> alphabetically.

For reference, there is a patch for this coming at:

    https://lore.kernel.org/rust-for-linux/I0YeaNjTtc4Nh47ZLJfAs6rgfAc_QZxhynNfz-GQKssVZ1S2UI_cTScCkp9-oX-hPYVcP3EfF7N0HMB9iAlm1FcvOJagnQoLeHtiW3bGCgM=@bamelis.dev/

Cheers,
Miguel

Re: [PATCH v1 2/2] RISC-V: enable building 64-bit kernels with rust support

[Conor Dooley]

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

On Tue, Mar 07, 2023 at 11:56:30AM +0100, Miguel Ojeda wrote:
> On Tue, Mar 7, 2023 at 11:25 AM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > While adding RISC-V to the table, I took the chance to re-sort it
> > alphabetically.
> 
> For reference, there is a patch for this coming at:
> 
>     https://lore.kernel.org/rust-for-linux/I0YeaNjTtc4Nh47ZLJfAs6rgfAc_QZxhynNfz-GQKssVZ1S2UI_cTScCkp9-oX-hPYVcP3EfF7N0HMB9iAlm1FcvOJagnQoLeHtiW3bGCgM=@bamelis.dev/

Cool. Git should resolve that, probably without even generating a
conflict in -next, right?


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

Re: [PATCH v1 2/2] RISC-V: enable building 64-bit kernels with rust support

[Miguel Ojeda]

On Tue, Mar 7, 2023 at 12:01 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> Cool. Git should resolve that, probably without even generating a
> conflict in -next, right?

I think it will conflict.

Since the cleanup is elsewhere and it may add work for the -next
maintainer, it is probably best to take it out in v2.

Cheers,
Miguel

Re: [PATCH v1 2/2] RISC-V: enable building 64-bit kernels with rust support

[Conor Dooley]

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

On Tue, Mar 07, 2023 at 12:56:30PM +0100, Miguel Ojeda wrote:
> On Tue, Mar 7, 2023 at 12:01 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > Cool. Git should resolve that, probably without even generating a
> > conflict in -next, right?
> 
> I think it will conflict.
> 
> Since the cleanup is elsewhere and it may add work for the -next
> maintainer, it is probably best to take it out in v2.

If you think it will conflict as-is, it's probably gonna conflict either
way.
You've pointed out several small bits, I'll send a v2 in a few days.

Cheers,
Conor.

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Tue, Mar 7, 2023 at 11:25 AM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> I have added SoB's too, but if that is not okay Gary, then please scream
> loudly.

Note that `Co-developed-by`s always go with a `Signed-off-by`s, i.e.
it is not possible to add just a `Co-developed-by`.

By the way, like for the Arm patch set, if you end up doing a v2,
could you please add the `BINDGEN_TARGET_*` in `rust/Makefile` (GCC
builds are really experimental, but since they are there anyway, it is
best to be consistent and add it).

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Tue, Mar 07, 2023 at 12:07:29PM +0100, Miguel Ojeda wrote:
> On Tue, Mar 7, 2023 at 11:25 AM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > I have added SoB's too, but if that is not okay Gary, then please scream
> > loudly.
> 
> Note that `Co-developed-by`s always go with a `Signed-off-by`s, i.e.
> it is not possible to add just a `Co-developed-by`.

Aye, but that does not mean that I am entitled to add someone else's!

> By the way, like for the Arm patch set, if you end up doing a v2,
> could you please add the `BINDGEN_TARGET_*` in `rust/Makefile` (GCC
> builds are really experimental, but since they are there anyway, it is
> best to be consistent and add it).

Sure.

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Thu, Mar 30, 2023 at 09:23:45AM +0100, Conor Dooley wrote:
> On Tue, Mar 07, 2023 at 12:07:29PM +0100, Miguel Ojeda wrote:

> > By the way, like for the Arm patch set, if you end up doing a v2,
> > could you please add the `BINDGEN_TARGET_*` in `rust/Makefile` (GCC
> > builds are really experimental, but since they are there anyway, it is
> > best to be consistent and add it).

Hmm, so I came across this commit while looking at that:
https://github.com/Rust-for-Linux/linux/commit/cfc17fed52b9585e2f19e2381bfb7094561b8027a
(rust: bindgen: ignore RISC-V extensions for GCC builds)

I don't want to add even more workarounds for this sort of thing,
especially as in this case it is outside of arch/riscv.
The extension stuff when mixing compilers is such a massive pain & given
this one requires Rust it's even less likely to be tested when someone
comes along and adds some additional extension support that appears in
-march :(

I'd rather do this in the RISC-V Makefile so that it does not get
forgotten.

If my understanding of bindgen is correct, we don't actually need to be
honest to it about what extensions the rest of the kernel is compiled
with, only make sure that it is not called with arguments it does not
understand?

| bindgen_c_flags_patsubst1 = $(patsubst -march=rv%_zicbom,-march=rv%,$(bindgen_c_flags_patsubst2))

This one is no longer needed as of 9a5c09dd9701 ("Merge patch series
"Remove toolchain dependencies for Zicbom"").

| bindgen_c_flags_patsubst  = $(patsubst -march=rv%_zicsr_zifencei,-march=rv%,$(bindgen_c_flags_patsubst1))

Oh and clang-17 is going to support both of these, and Nathan and I
already spent a bunch of time fixing the fallout from that!
It still functions correctly without having them passed, but I have
heard requiring these may become the default at some point too.
What's done here may end up needing to be dynamic, but that bridge can be
crossed if/when we come to it.

What version of GCC do I need to replicate this? I can build tip-of-tree
gcc if needs be.

Cheers,
Conor.

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Thu, Mar 30, 2023 at 11:12 AM Conor Dooley
<conor.dooley@microchip.com> wrote:
>
> I'd rather do this in the RISC-V Makefile so that it does not get
> forgotten.

Sounds good to me! We want to have the least amount of things possible
in the common pieces (e.g. for the target spec file we moved some
flags); so the more we move out to `arch/`, the better.

> If my understanding of bindgen is correct, we don't actually need to be
> honest to it about what extensions the rest of the kernel is compiled
> with, only make sure that it is not called with arguments it does not
> understand?

As long as bindgen generates things with the right ABI etc., yeah.
But, in principle, enabling one extension one side but not the other
could be wrong if it ends up in something that Rust uses, e.g. if the
C side does:

    #ifdef __ARM_ARCH_7R__
        int x;
    #else
        char x;
    #endif

and Rust attempts to use it, then particular `-march` builds could be broken.

> Oh and clang-17 is going to support both of these, and Nathan and I
> already spent a bunch of time fixing the fallout from that!
> It still functions correctly without having them passed, but I have
> heard requiring these may become the default at some point too.
> What's done here may end up needing to be dynamic, but that bridge can be
> crossed if/when we come to it.
>
> What version of GCC do I need to replicate this? I can build tip-of-tree
> gcc if needs be.

Sorry, what do you want to replicate? If you mean what we had in the
old GitHub CI, I see:

    CONFIG_CC_VERSION_TEXT="riscv64-linux-gnu-gcc (Ubuntu
11.3.0-1ubuntu1~22.04) 11.3.0"

which successfully boots in QEMU for the kernel config we tested.

But if you are asking what should be supported, I guess it depends on
the RISC-V maintainers. Ideally, everything that the kernel supports
(GCC >= 5.1), but since the GCC+Rust builds are so experimental, I
think as long as something is tested from time to time, it would be
great (to at least know not everything is completely broken).

But if you think that would be too much effort to maintain, or even
GCC builds in general, then please feel free to ignore it for the time
being, i.e. it is better to have LLVM builds rather than nothing! :)

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Mon, Apr 03, 2023 at 06:35:45PM +0200, Miguel Ojeda wrote:
> On Thu, Mar 30, 2023 at 11:12 AM Conor Dooley
> <conor.dooley@microchip.com> wrote:
> >
> > I'd rather do this in the RISC-V Makefile so that it does not get
> > forgotten.
> 
> Sounds good to me! We want to have the least amount of things possible
> in the common pieces (e.g. for the target spec file we moved some
> flags); so the more we move out to `arch/`, the better.
> 
> > If my understanding of bindgen is correct, we don't actually need to be
> > honest to it about what extensions the rest of the kernel is compiled
> > with, only make sure that it is not called with arguments it does not
> > understand?
> 
> As long as bindgen generates things with the right ABI etc., yeah.
> But, in principle, enabling one extension one side but not the other
> could be wrong if it ends up in something that Rust uses, e.g. if the
> C side does:
> 
>     #ifdef __ARM_ARCH_7R__
>         int x;
>     #else
>         char x;
>     #endif
> 
> and Rust attempts to use it, then particular `-march` builds could be broken.

To be on the safe side then, we should really disable the extensions
across the whole kernel. I don't *think* we have any madness at the
moment like in the above, but it is better to be on the safe side.
As I note below, it's just one extension for now anyway.

> > What version of GCC do I need to replicate this? I can build tip-of-tree
> > gcc if needs be.
> 
> Sorry, what do you want to replicate? If you mean what we had in the
> old GitHub CI, I see:
> 
>     CONFIG_CC_VERSION_TEXT="riscv64-linux-gnu-gcc (Ubuntu
> 11.3.0-1ubuntu1~22.04) 11.3.0"
> 
> which successfully boots in QEMU for the kernel config we tested.

No, I misunderstood your question. I thought you meant something else
entirely.

> But if you are asking what should be supported, I guess it depends on
> the RISC-V maintainers. Ideally, everything that the kernel supports
> (GCC >= 5.1),

Heh, as if that number is true across the board!

> but since the GCC+Rust builds are so experimental, I
> think as long as something is tested from time to time, it would be
> great (to at least know not everything is completely broken).
> 
> But if you think that would be too much effort to maintain, or even
> GCC builds in general, then please feel free to ignore it for the time
> being, i.e. it is better to have LLVM builds rather than nothing! :)

Yeah, it may be worth getting just the LLVM bits in. I abhor the -march
handling and it may end up looking like shite with the zicsr &
zifencei handling.
Worst comes to worst, can permit gcc builds by just removing all the
extensions that get passed in -march for RUST && CC_IS_GCC type
scenarios. The only one of those at the moment is zihintpause & I don't
suppose too many tears will be shed over that.
For now it's safe to assume that LLVM doesn't require zicsr or zifencei
[1], we don't need to do a version dance right away.

¯\_(ツ)_/¯,
Conor.

1 - https://reviews.llvm.org/D147183#4233360

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Mon, Apr 03, 2023 at 06:14:57PM +0100, Conor Dooley wrote:
> On Mon, Apr 03, 2023 at 06:35:45PM +0200, Miguel Ojeda wrote:

> > As long as bindgen generates things with the right ABI etc., yeah.
> > But, in principle, enabling one extension one side but not the other
> > could be wrong if it ends up in something that Rust uses, e.g. if the
> > C side does:
> > 
> >     #ifdef __ARM_ARCH_7R__
> >         int x;
> >     #else
> >         char x;
> >     #endif
> > 
> > and Rust attempts to use it, then particular `-march` builds could be broken.
> 
> To be on the safe side then, we should really disable the extensions
> across the whole kernel. I don't *think* we have any madness at the
> moment like in the above, but it is better to be on the safe side.

So I am still of this opinion. I don't want to silently have a mismatch
between one side of the kernel and the other. Recipe for disaster.
If it's off for the Rust side of things, it should be off for C too.

> > but since the GCC+Rust builds are so experimental, I
> > think as long as something is tested from time to time, it would be
> > great (to at least know not everything is completely broken).
> > 
> > But if you think that would be too much effort to maintain, or even
> > GCC builds in general, then please feel free to ignore it for the time
> > being, i.e. it is better to have LLVM builds rather than nothing! :)
> 
> Yeah, it may be worth getting just the LLVM bits in. I abhor the -march
> handling and it may end up looking like shite with the zicsr &
> zifencei handling.
> Worst comes to worst, can permit gcc builds by just removing all the
> extensions that get passed in -march for RUST && CC_IS_GCC type
> scenarios. The only one of those at the moment is zihintpause & I don't
> suppose too many tears will be shed over that.

Been thinking about this some more, and I don't really like where this
is going. I think I am gonna explicitly disable gcc support if
anything.
I wrote out a list of issues I have with all of this, but I then had
second thoughts about some of them, so I've deleted that section of this
mail.
I need to think long and hard about the mixing and matching of support
between several versions of the tools (bindgen/llvm, rustc, gcc) for
different extensions & potentially different versions of the ISA spec.

I'll revisit this when my thoughts have settled down.

> For now it's safe to assume that LLVM doesn't require zicsr or zifencei
> [1], we don't need to do a version dance right away.

I also needed to remove `-mno-riscv-attribute` from bindgen's cflags
for things to work. That's probably not something yous have to deal with
as you're on an old kernel for the rust branch. Or maybe it got
backported to v6.2.n, idk.

Oh and bindgen doesn't actually seem to succeed with the hacks anyway:
thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at__/__/include/linux/compiler_types_h_146_2)" is not a valid Ident'
I had a quick check on lore but didn't see a fix for that one.

And there's also the code model that doesn't yet seem to be handled.
The script looks to always use medany. Writing that here lest I forget
about it.

Either way, I marked the series as "Changes Requested" on patchwork :)

Cheers,
Conor.

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Thu, Mar 30, 2023 at 10:24 AM Conor Dooley
<conor.dooley@microchip.com> wrote:
>
> Aye, but that does not mean that I am entitled to add someone else's!

Yeah, definitely! I meant that, from what you said, it sounded like
adding the `Co-developed-by` was OK without the other (i.e. due to the
"too" in your sentence).

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

Hey Kwang,

On 08/06/2023 05:46, 손광훈/Tizen Platform Lab(SR)/삼성전자 wrote:
> Hi,
> Recently I'm trying to put a rust patch on the risc-v board.
> I saw a patch [1] and looked through it roughly.
> Only if llvm(not gcc) is allowed, it looks good with no major problems.
> 
> > I'll revisit this when my thoughts have settled down.
> 
> If you let me know the problematic part, may I try the patch?
> 
> [1] https://lore.kernel.org/linux-riscv/20230405-itinerary-handgrip-
> a5ffba368148@spud/

Yeah, you can definitely try this or the downstream rust-for-linux
project - both should work well on RISC-V.
The problematic part is figuring out what ISA extensions are supported
by the rust compiler being used (and by bindgen), and deciding what to
put in -march as a result.

I think it is unlikely to matter for you, unless you're aggressively
mixing versions for different parts of your toolchain.

I do intend revisting this, probably after the min. version for rust
gets bumped, I've just been really busy with other work the last weeks.

Cheers,
Conor.

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

Whoops, actually to Kwang this time...

On Thu, Jun 08, 2023 at 08:01:16AM +0100, Conor Dooley wrote:
> Hey Kwang,
> 
> On 08/06/2023 05:46, 손광훈/Tizen Platform Lab(SR)/삼성전자 wrote:
> > Hi,
> > Recently I'm trying to put a rust patch on the risc-v board.
> > I saw a patch [1] and looked through it roughly.
> > Only if llvm(not gcc) is allowed, it looks good with no major problems.
> > 
> > > I'll revisit this when my thoughts have settled down.
> > 
> > If you let me know the problematic part, may I try the patch?
> > 
> > [1] https://lore.kernel.org/linux-riscv/20230405-itinerary-handgrip-
> > a5ffba368148@spud/
> 
> Yeah, you can definitely try this or the downstream rust-for-linux
> project - both should work well on RISC-V.
> The problematic part is figuring out what ISA extensions are supported
> by the rust compiler being used (and by bindgen), and deciding what to
> put in -march as a result.
> 
> I think it is unlikely to matter for you, unless you're aggressively
> mixing versions for different parts of your toolchain.
> 
> I do intend revisting this, probably after the min. version for rust
> gets bumped, I've just been really busy with other work the last weeks.
> 
> Cheers,
> Conor.



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

RE: [PATCH v1 0/2] RISC-V: enable rust

[Kwanghoon Son]

> Hey Kwang,
> 
> > Hi,
> > Recently I'm trying to put a rust patch on the risc-v board.
> > I saw a patch [1] and looked through it roughly.
> > Only if llvm(not gcc) is allowed, it looks good with no major problems.
> >
> > > I'll revisit this when my thoughts have settled down.
> >
> > If you let me know the problematic part, may I try the patch?
> >
> > [1] https://lore.kernel.org/linux-riscv/20230405-itinerary-handgrip-
> > a5ffba368148@spud/
> 
> Yeah, you can definitely try this or the downstream rust-for-linux
> project - both should work well on RISC-V.
> The problematic part is figuring out what ISA extensions are supported
> by the rust compiler being used (and by bindgen), and deciding what to
> put in -march as a result.
> 
> I think it is unlikely to matter for you, unless you're aggressively
> mixing versions for different parts of your toolchain.
> 
> I do intend revisting this, probably after the min. version for rust
> gets bumped, I've just been really busy with other work the last weeks.

No rush! I was just curious.
Thank you for the explanation!

> 
> Cheers,
> Conor.

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Thu, Jun 8, 2023 at 9:01 AM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> I do intend revisting this, probably after the min. version for rust
> gets bumped, I've just been really busy with other work the last weeks.

Thanks Conor! That would be great. We are increasing the minimum
version after the merge window to Rust 1.70.0 (assuming no unexpected
issues).

This is the branch I have in case you want to use it, I will submit it
soon: https://github.com/ojeda/linux/tree/rust-1.70

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Thu, Jun 08, 2023 at 01:52:47PM +0200, Miguel Ojeda wrote:
> On Thu, Jun 8, 2023 at 9:01 AM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > I do intend revisting this, probably after the min. version for rust
> > gets bumped, I've just been really busy with other work the last weeks.
> 
> Thanks Conor! That would be great. We are increasing the minimum
> version after the merge window to Rust 1.70.0 (assuming no unexpected
> issues).

Right, so probably I won't resubmit anything until after v6.6 then,
as it won't be in the RISC-V tree until then, by the sounds of your
timeline.
Gives me plenty of time to try and unravel the mess of libclang versions
and what extensions are supported by each tool. Not like I am devoid of
other things that need to be done!


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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

Palmer, Miguel, Nathan, etc

On Thu, Jun 08, 2023 at 01:28:06PM +0100, Conor Dooley wrote:
> On Thu, Jun 08, 2023 at 01:52:47PM +0200, Miguel Ojeda wrote:
> > On Thu, Jun 8, 2023 at 9:01 AM Conor Dooley <conor.dooley@microchip.com> wrote:
> > >
> > > I do intend revisting this, probably after the min. version for rust
> > > gets bumped, I've just been really busy with other work the last weeks.
> > 
> > Thanks Conor! That would be great. We are increasing the minimum
> > version after the merge window to Rust 1.70.0 (assuming no unexpected
> > issues).
> 
> Right, so probably I won't resubmit anything until after v6.6 then,
> as it won't be in the RISC-V tree until then, by the sounds of your
> timeline.
> Gives me plenty of time to try and unravel the mess of libclang versions
> and what extensions are supported by each tool. Not like I am devoid of
> other things that need to be done!

6.6 came and went, and I have been busy dealing with the other
responsibilities I mentioned and have not had a chance to look here.
I rebased this today and things still work as they did when I submitted
this version, but things have gotten muddier on the LLVM side of things,
as more recent versions have added yet more extension support.

My inclination at this point is to engage in a bit of LARPing as an
ostrich, and sorta ignore these concerns initially. Specifically, I'd
like to drop the idea of having the gcc support, and restrict to LLVM=1.
When it comes to asymmetrical extension support between the C and Rust
toolchains, I'm think we deal with that as we do for the C toolchains,
sort issues out as-and-when they arrive rather than punt this again.

Thoughts?

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Wed, Jan 17, 2024 at 12:31 PM Conor Dooley <conor@kernel.org> wrote:
>
> 6.6 came and went, and I have been busy dealing with the other
> responsibilities I mentioned and have not had a chance to look here.
> I rebased this today and things still work as they did when I submitted
> this version, but things have gotten muddier on the LLVM side of things,
> as more recent versions have added yet more extension support.

Sounds fun :)

> My inclination at this point is to engage in a bit of LARPing as an
> ostrich, and sorta ignore these concerns initially. Specifically, I'd
> like to drop the idea of having the gcc support, and restrict to LLVM=1.

Yeah, if `LLVM=1` works, then I would suggest going ahead with that.

(Now that `rustc_codegen_gcc` is here, we will move to that and forget
about mixed compiler builds, but we still have to handle `bindgen`
flags until we have an alternative for that)

> When it comes to asymmetrical extension support between the C and Rust
> toolchains, I'm think we deal with that as we do for the C toolchains,
> sort issues out as-and-when they arrive rather than punt this again.

Sounds good, thanks a lot!

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Wed, Jan 17, 2024 at 07:23:17PM +0100, Miguel Ojeda wrote:
> On Wed, Jan 17, 2024 at 12:31 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > 6.6 came and went, and I have been busy dealing with the other
> > responsibilities I mentioned and have not had a chance to look here.
> > I rebased this today and things still work as they did when I submitted
> > this version, but things have gotten muddier on the LLVM side of things,
> > as more recent versions have added yet more extension support.
> 
> Sounds fun :)
> 
> > My inclination at this point is to engage in a bit of LARPing as an
> > ostrich, and sorta ignore these concerns initially. Specifically, I'd
> > like to drop the idea of having the gcc support, and restrict to LLVM=1.
> 
> Yeah, if `LLVM=1` works, then I would suggest going ahead with that.
> 
> (Now that `rustc_codegen_gcc` is here, we will move to that and forget
> about mixed compiler builds, but we still have to handle `bindgen`
> flags until we have an alternative for that)

The bit that worries me most is bindgen, and in particular detecting the
version of libclang used. I mentioned to Nathan or Nick about needing a
buildtime test for the version of LIBCLANG being used.
I'm less worried about this for LLVM=1 builds, since while I think it is
possible to provide a LIBCLANG path to the build system, I suspect that
for LLVM=1 builds it's almost always going to match the LLVM toolchain
in use.

> > When it comes to asymmetrical extension support between the C and Rust
> > toolchains, I'm think we deal with that as we do for the C toolchains,
> > sort issues out as-and-when they arrive rather than punt this again.
> 
> Sounds good, thanks a lot!

I'll do another rebase and resend after the merge window closes I
suppose :)

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Thu, Jan 18, 2024 at 4:49 PM Conor Dooley <conor@kernel.org> wrote:
>
> The bit that worries me most is bindgen, and in particular detecting the
> version of libclang used. I mentioned to Nathan or Nick about needing a
> buildtime test for the version of LIBCLANG being used.
> I'm less worried about this for LLVM=1 builds, since while I think it is
> possible to provide a LIBCLANG path to the build system, I suspect that
> for LLVM=1 builds it's almost always going to match the LLVM toolchain
> in use.

`scripts/rust_is_available.sh` tests whether `libclang` is at least
the minimum LLVM supported version; and under `LLVM=1` builds, it also
tests whether the `bindgen` found one matches the C compiler. Do you
mean something like that?

For `bindgen` under GCC builds, we will eventually want a "proper" way
to use GCC instead (or possibly other approaches like querying the
information): https://github.com/rust-lang/rust-bindgen/issues/1949.
Recently, there has been a thread in our Zulip and a couple people are
experimenting: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Bindgen.20--.20GCC.20backend.20port

> I'll do another rebase and resend after the merge window closes I
> suppose :)

Thanks!

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Thu, Jan 18, 2024 at 05:09:40PM +0100, Miguel Ojeda wrote:
> On Thu, Jan 18, 2024 at 4:49 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > The bit that worries me most is bindgen, and in particular detecting the
> > version of libclang used. I mentioned to Nathan or Nick about needing a
> > buildtime test for the version of LIBCLANG being used.
> > I'm less worried about this for LLVM=1 builds, since while I think it is
> > possible to provide a LIBCLANG path to the build system, I suspect that
> > for LLVM=1 builds it's almost always going to match the LLVM toolchain
> > in use.

I chatted with the clang built linux folks about this yesterday, Nathan
agreed that dealing with incompatibility issues iff they crop up is a
reasonable way to go.

> `scripts/rust_is_available.sh` tests whether `libclang` is at least
> the minimum LLVM supported version; and under `LLVM=1` builds, it also
> tests whether the `bindgen` found one matches the C compiler. Do you
> mean something like that?

If by "the bindgen found one matches the C compiler" you mean that the
version of libclang used by bindgen matches the C compiler, then that
sounds great.

> For `bindgen` under GCC builds, we will eventually want a "proper" way
> to use GCC instead (or possibly other approaches like querying the
> information): https://github.com/rust-lang/rust-bindgen/issues/1949.

> Recently, there has been a thread in our Zulip and a couple people are
> experimenting: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Bindgen.20--.20GCC.20backend.20port

That link for me goes to a message on 22/01, so later than the email you
sent.

> > I'll do another rebase and resend after the merge window closes I
> > suppose :)


That said, I gave things another spin today, in a different environment,
as a final check before sending and found an issue causing kernel
panics. RISC-V (and x86/arm64) supports kcfi (CFI_CLANG) but enabling
sanitisers seems to be a nightly only option for rustc. The kernel I
built today had CFI_CLANG enabled and that caused panics when the rust
samples were loaded.

The CFI_CLANG Kconfig entry has a cc-option test for whether the option
is supported, but from a quick check I don't see a comparable test to
use for rust. Even if a test was added, the current flag is an unstable
one, so I am not sure if testing for it is the right call in the first
place, given the stabilised flag would be entirely different?

The tracking issue seems to be complete:
https://github.com/rust-lang/rust/issues/89653
but the tracking issue for sanitisiers themselves is only 3/5:
https://github.com/rust-lang/rust/issues/39699

The simple thing would be to make them mutually exclusive options in
Kconfig.

What do you think?

Cheers,
Conor.

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Thu, Jan 25, 2024 at 1:31 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> I chatted with the clang built linux folks about this yesterday, Nathan
> agreed that dealing with incompatibility issues iff they crop up is a
> reasonable way to go.
>
> If by "the bindgen found one matches the C compiler" you mean that the
> version of libclang used by bindgen matches the C compiler, then that
> sounds great.

Yeah, exactly. So, unless I am misunderstanding, the incompatibilities
could only happen if someone ignores the warning. We could make it an
error too.

> > For `bindgen` under GCC builds, we will eventually want a "proper" way
> > to use GCC instead (or possibly other approaches like querying the
> > information): https://github.com/rust-lang/rust-bindgen/issues/1949.
>
> > Recently, there has been a thread in our Zulip and a couple people are
> > experimenting: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Bindgen.20--.20GCC.20backend.20port
>
> That link for me goes to a message on 22/01, so later than the email you
> sent.

Zulip seems to scroll to the latest message in the topic -- you should
be able to scroll a bit up, but if that doesn't work, this link should
go to the first message:
https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Bindgen.20--.20GCC.20backend.20port/near/412609074

> That said, I gave things another spin today, in a different environment,
> as a final check before sending and found an issue causing kernel
> panics. RISC-V (and x86/arm64) supports kcfi (CFI_CLANG) but enabling
> sanitisers seems to be a nightly only option for rustc. The kernel I
> built today had CFI_CLANG enabled and that caused panics when the rust
> samples were loaded.
>
> The CFI_CLANG Kconfig entry has a cc-option test for whether the option
> is supported, but from a quick check I don't see a comparable test to
> use for rust. Even if a test was added, the current flag is an unstable
> one, so I am not sure if testing for it is the right call in the first
> place, given the stabilised flag would be entirely different?

Yeah, KCFI and other mitigations is WIP -- Cc'ing Ramon and Matthew
who may be able to tell us the latest status.

Testing for unstable flags is fine, i.e. we only support a single
compiler, so we can change the name when we do the upgrade.

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Thu, Jan 25, 2024 at 01:50:05PM +0100, Miguel Ojeda wrote:
> On Thu, Jan 25, 2024 at 1:31 PM Conor Dooley <conor.dooley@microchip.com> wrote:

> > > Recently, there has been a thread in our Zulip and a couple people are
> > > experimenting: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Bindgen.20--.20GCC.20backend.20port
> >
> > That link for me goes to a message on 22/01, so later than the email you
> > sent.
> 
> Zulip seems to scroll to the latest message in the topic -- you should
> be able to scroll a bit up, but if that doesn't work, this link should
> go to the first message:
> https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Bindgen.20--.20GCC.20backend.20port/near/412609074

Ah, thanks for the direct link :)

> 
> > That said, I gave things another spin today, in a different environment,
> > as a final check before sending and found an issue causing kernel
> > panics. RISC-V (and x86/arm64) supports kcfi (CFI_CLANG) but enabling

I mention x86 and arm64 here, because my grepping didn't see the flag
being set for x86 (in tree) or arm64 (in that series) if CFI_CLANG was
set or any mutual exclusion. Has noone tried CFI_CLANG + RUST there or
just not run into any issues?

> > sanitisers seems to be a nightly only option for rustc. The kernel I
> > built today had CFI_CLANG enabled and that caused panics when the rust
> > samples were loaded.
> >
> > The CFI_CLANG Kconfig entry has a cc-option test for whether the option
> > is supported, but from a quick check I don't see a comparable test to
> > use for rust. Even if a test was added, the current flag is an unstable
> > one, so I am not sure if testing for it is the right call in the first
> > place, given the stabilised flag would be entirely different?
> 
> Yeah, KCFI and other mitigations is WIP -- Cc'ing Ramon and Matthew
> who may be able to tell us the latest status.

Also CC Sami I guess, since he is the one who added the CFI_CLANG bits
to the kernel, and can probably comment on the suitability of adding a
check etc.

> Testing for unstable flags is fine, i.e. we only support a single
> compiler, so we can change the name when we do the upgrade.

Actually, thinking about it for a moment - if only a single compiler
version is supported (the minimum, right?) then you could just add the
-Zsanitizer=kcfi flag whenever CFI_CLANG and RUST are both set.

I'm not sure if that is a better option though. It's a choice between
CFI_CLANG being disabled if the check is not updated when the toolchain
is bumped versus being enabled for C and not for RUST. I think I prefer
the former though, tracking down the cause of the latter I would rather
not wish on a user.

I vote for having the check, even if it can only ever be true at the
moment.

Cheers,
Conor.

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Thu, Jan 25, 2024 at 2:46 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> Ah, thanks for the direct link :)

My pleasure!

> Actually, thinking about it for a moment - if only a single compiler
> version is supported (the minimum, right?) then you could just add the

Yeah, the minimum listed in `scripts/min-tool-version.sh` and in
`Documentation/process/changes.rst`. It also happens to be the maximum
too, until we can relax that.

> -Zsanitizer=kcfi flag whenever CFI_CLANG and RUST are both set.

Since the flag goes to the Rust compiler, `RUST` would be always
enabled, so the flag would only need to be added when `CFI_CLANG=y`,
no? Or what do you mean?

> I'm not sure if that is a better option though. It's a choice between
> CFI_CLANG being disabled if the check is not updated when the toolchain
> is bumped versus being enabled for C and not for RUST. I think I prefer
> the former though, tracking down the cause of the latter I would rather
> not wish on a user.
>
> I vote for having the check, even if it can only ever be true at the
> moment.

Since we only support a single version, we don't need `rc-option`
tests until we start supporting several versions (which is why other
tests like that do not exist so far).

In my previous message I thought you meant using the flag to test for
arch/target support or similar. That would be fine, but we can also do
the usual `ARCH_SUPPORTS_CFI_RUST` here, I would assume.

Now, during the version bump to a stable flag, if we happen to forget
to update the flag name, it would be a build error, so it should be
easily spotted and fixed. And if we did use an `rc-option` for the
arch/target support idea, it would be the first case you mention, so
it is all good.

What we may want to add, though, to avoid the confusion you mention
meanwhile, is just a `depends on !CFI_CLANG` for `RUST`, like for the
other requirements we have there (which are things that should
eventually go away). Then they can remove that when the `-Z` flag is
deemed ready to be used. But perhaps let's see what Ramon et al. say.

In other words, the flag was added back in 1.68.0 to `rustc`, but it
was not ready, so we need to store the "ready" bit in our side
meanwhile, i.e. we can't know that just by testing the flag itself.

By the way, concerning the tracking issue, since you mentioned it: it
has a list of PRs, but not fixes, there is a "known issues" link
there. On top of that, we are "shifted in time" w.r.t. the latest
status in the compiler, since we use stable versions of the compiler.

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Fri, Jan 26, 2024 at 10:00:02PM +0100, Miguel Ojeda wrote:
> On Thu, Jan 25, 2024 at 2:46 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > Ah, thanks for the direct link :)
> 
> My pleasure!
> 
> > Actually, thinking about it for a moment - if only a single compiler
> > version is supported (the minimum, right?) then you could just add the
> 
> Yeah, the minimum listed in `scripts/min-tool-version.sh` and in
> `Documentation/process/changes.rst`. It also happens to be the maximum
> too, until we can relax that.
> 
> > -Zsanitizer=kcfi flag whenever CFI_CLANG and RUST are both set.
> 
> Since the flag goes to the Rust compiler, `RUST` would be always
> enabled, so the flag would only need to be added when `CFI_CLANG=y`,
> no?

Sure.

> Or what do you mean?

Oh I was probably just getting myself mixed up between what the Kconfig
and Makefile stuff would look like. dw :)

> > I'm not sure if that is a better option though. It's a choice between
> > CFI_CLANG being disabled if the check is not updated when the toolchain
> > is bumped versus being enabled for C and not for RUST. I think I prefer
> > the former though, tracking down the cause of the latter I would rather
> > not wish on a user.
> >
> > I vote for having the check, even if it can only ever be true at the
> > moment.
> 
> Since we only support a single version, we don't need `rc-option`
> tests until we start supporting several versions (which is why other
> tests like that do not exist so far).
> 
> In my previous message I thought you meant using the flag to test for
> arch/target support or similar. That would be fine, but we can also do
> the usual `ARCH_SUPPORTS_CFI_RUST` here, I would assume.

Is that even needed? We already have ARCH_SUPPORTS_CFI_CLANG and AFAIU
rust supports it if clang does, so a second option is superfluous?

> Now, during the version bump to a stable flag, if we happen to forget
> to update the flag name, it would be a build error, so it should be
> easily spotted and fixed.

I'm reading back what I wrote, and I must have been trying to get out
the door or something because none of it really makes that much sense.
Of course an unknown option should be detectable at build time and not
be a silent breakage. Maybe I should have written the patch for this
before sending the mail rather than writing the mail based on what was
in my head.

> What we may want to add, though, to avoid the confusion you mention
> meanwhile, is just a `depends on !CFI_CLANG` for `RUST`, like for the
> other requirements we have there (which are things that should
> eventually go away). Then they can remove that when the `-Z` flag is
> deemed ready to be used. But perhaps let's see what Ramon et al. say.

I don't really mind either way. Whatever of the two that you guys want
that prevents broken kernels works for me!

> By the way, concerning the tracking issue, since you mentioned it: it
> has a list of PRs, but not fixes, there is a "known issues" link
> there. On top of that, we are "shifted in time" w.r.t. the latest
> status in the compiler, since we use stable versions of the compiler.

Yah, I was linking it to point out that the stuff is unlikely to be
usable any time soon, since it is not complete in the most recent
toolchain.

Cheers,
Conor.

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Fri, Jan 26, 2024 at 11:01 PM Conor Dooley <conor@kernel.org> wrote:
>
> Is that even needed? We already have ARCH_SUPPORTS_CFI_CLANG and AFAIU
> rust supports it if clang does, so a second option is superfluous?

From a quick look, I don't see it enabled in any RISC-V built-in
target in `rustc` yet.

It may also still be the case that KCFI needs some tweaks for, say,
RISC-V, before the flag actually works, i.e. we couldn't just test the
flag in that case -- Ramon: how likely is it that RISC-V would work if
KCFI works for aarch64 and x86_64?

> I'm reading back what I wrote, and I must have been trying to get out
> the door or something because none of it really makes that much sense.
> Of course an unknown option should be detectable at build time and not
> be a silent breakage. Maybe I should have written the patch for this
> before sending the mail rather than writing the mail based on what was
> in my head.

No worries, it happens. I probably added to the confusion when I
replied with the "testing the unstable flag".

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Sat, Jan 27, 2024 at 02:46:38PM +0100, Miguel Ojeda wrote:
> On Fri, Jan 26, 2024 at 11:01 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > Is that even needed? We already have ARCH_SUPPORTS_CFI_CLANG and AFAIU
> > rust supports it if clang does, so a second option is superfluous?
> 
> From a quick look, I don't see it enabled in any RISC-V built-in
> target in `rustc` yet.
> 
> It may also still be the case that KCFI needs some tweaks for, say,
> RISC-V, before the flag actually works, i.e. we couldn't just test the
> flag in that case -- Ramon: how likely is it that RISC-V would work if
> KCFI works for aarch64 and x86_64?

Well, there's been no reply here. I'll do sa you suggested and add a 
depends on !CFI_CLANG to RUST.

Cheers,
Conor.


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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Trevor Gross]

On Fri, Feb 9, 2024 at 9:18 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Sat, Jan 27, 2024 at 02:46:38PM +0100, Miguel Ojeda wrote:
> > On Fri, Jan 26, 2024 at 11:01 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > Is that even needed? We already have ARCH_SUPPORTS_CFI_CLANG and AFAIU
> > > rust supports it if clang does, so a second option is superfluous?
> >
> > From a quick look, I don't see it enabled in any RISC-V built-in
> > target in `rustc` yet.
> >
> > It may also still be the case that KCFI needs some tweaks for, say,
> > RISC-V, before the flag actually works, i.e. we couldn't just test the
> > flag in that case -- Ramon: how likely is it that RISC-V would work if
> > KCFI works for aarch64 and x86_64?
>
> Well, there's been no reply here. I'll do sa you suggested and add a
> depends on !CFI_CLANG to RUST.
>
> Cheers,
> Conor.
>

I asked on Zulip and it sounds like Ramon may be out [1]. It
_probably_ works, but going with a dependency to not be blocked on
KCFI is probably reasonable for now.

- Trevor

[1]: https://rust-lang.zulipchat.com/#narrow/stream/343119-project-exploit-mitigations/topic/KCFI.20on.20RISC-V.20questions

Re: [PATCH v1 0/2] RISC-V: enable rust

[Ramon de C Valle]

Sorry for the late reply. Sami might be the best person to answer
this, but KCFI (not CFI) tests are lowered by passes that are
architecture specific (see https://reviews.llvm.org/D119296), so we'd
need to add support for RISC-V. There is no additional work required
in the Rust compiler besides enabling it for the new target.


On Sat, Feb 10, 2024 at 12:13 AM Trevor Gross <tmgross@umich.edu> wrote:
>
> On Fri, Feb 9, 2024 at 9:18 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Sat, Jan 27, 2024 at 02:46:38PM +0100, Miguel Ojeda wrote:
> > > On Fri, Jan 26, 2024 at 11:01 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > Is that even needed? We already have ARCH_SUPPORTS_CFI_CLANG and AFAIU
> > > > rust supports it if clang does, so a second option is superfluous?
> > >
> > > From a quick look, I don't see it enabled in any RISC-V built-in
> > > target in `rustc` yet.
> > >
> > > It may also still be the case that KCFI needs some tweaks for, say,
> > > RISC-V, before the flag actually works, i.e. we couldn't just test the
> > > flag in that case -- Ramon: how likely is it that RISC-V would work if
> > > KCFI works for aarch64 and x86_64?
> >
> > Well, there's been no reply here. I'll do sa you suggested and add a
> > depends on !CFI_CLANG to RUST.
> >
> > Cheers,
> > Conor.
> >
>
> I asked on Zulip and it sounds like Ramon may be out [1]. It
> _probably_ works, but going with a dependency to not be blocked on
> KCFI is probably reasonable for now.
>
> - Trevor
>
> [1]: https://rust-lang.zulipchat.com/#narrow/stream/343119-project-exploit-mitigations/topic/KCFI.20on.20RISC-V.20questions

Re: [PATCH v1 0/2] RISC-V: enable rust

[Sami Tolvanen]

On Mon, Feb 12, 2024 at 11:04 AM Ramon de C Valle <rcvalle@google.com> wrote:
>
> Sorry for the late reply. Sami might be the best person to answer
> this, but KCFI (not CFI) tests are lowered by passes that are
> architecture specific (see https://reviews.llvm.org/D119296), so we'd
> need to add support for RISC-V. There is no additional work required
> in the Rust compiler besides enabling it for the new target.

LLVM 17 added KCFI support for RISC-V. Sounds like it's just a
question of whether rustc uses a new enough version of LLVM then?

Sami

Re: [PATCH v1 0/2] RISC-V: enable rust

[Ramon de C Valle]

On Mon, Feb 12, 2024 at 12:36 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Mon, Feb 12, 2024 at 11:04 AM Ramon de C Valle <rcvalle@google.com> wrote:
> >
> > Sorry for the late reply. Sami might be the best person to answer
> > this, but KCFI (not CFI) tests are lowered by passes that are
> > architecture specific (see https://reviews.llvm.org/D119296), so we'd
> > need to add support for RISC-V. There is no additional work required
> > in the Rust compiler besides enabling it for the new target.
>
> LLVM 17 added KCFI support for RISC-V. Sounds like it's just a
> question of whether rustc uses a new enough version of LLVM then?

rustc is using LLVM 17 since
https://github.com/rust-lang/rust/pull/114048, so I guess I can enable
it for RISC-V targets in
https://github.com/rust-lang/rust/tree/master/compiler/rustc_target/src/spec/targets,
and the Linux kernel will get it on the next compiler update?

Re: [PATCH v1 0/2] RISC-V: enable rust

[Trevor Gross]

On Tue, Feb 13, 2024 at 3:09 PM Ramon de C Valle <rcvalle@google.com> wrote:
>
> On Mon, Feb 12, 2024 at 12:36 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Mon, Feb 12, 2024 at 11:04 AM Ramon de C Valle <rcvalle@google.com> wrote:
> > >
> > > Sorry for the late reply. Sami might be the best person to answer
> > > this, but KCFI (not CFI) tests are lowered by passes that are
> > > architecture specific (see https://reviews.llvm.org/D119296), so we'd
> > > need to add support for RISC-V. There is no additional work required
> > > in the Rust compiler besides enabling it for the new target.
> >
> > LLVM 17 added KCFI support for RISC-V. Sounds like it's just a
> > question of whether rustc uses a new enough version of LLVM then?
>
> rustc is using LLVM 17 since
> https://github.com/rust-lang/rust/pull/114048, so I guess I can enable
> it for RISC-V targets in
> https://github.com/rust-lang/rust/tree/master/compiler/rustc_target/src/spec/targets,
> and the Linux kernel will get it on the next compiler update?

18 as of a few hours ago :) https://github.com/rust-lang/rust/pull/120055

Just for an idea of timing, anything merged this month will be in 1.78
on 2024-05-02.

Re: [PATCH v1 0/2] RISC-V: enable rust

[Miguel Ojeda]

On Mon, Feb 12, 2024 at 8:02 PM Ramon de C Valle <rcvalle@google.com> wrote:
>
> Sorry for the late reply. Sami might be the best person to answer this, but KCFI (not CFI) tests are lowered by passes that are architecture specific (see https://reviews.llvm.org/D119296), so we'd need to add support for RISC-V. There is no additional work required in the Rust compiler besides enabling it for the new target.

Thanks a lot Ramon!

Then for RISC-V let's go for the `depends on` for the moment, and we
can remove when the support lands for RISC-V (ideally when someone has
managed to boot it at least under some configuration).

Cheers,
Miguel

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Mon, Feb 12, 2024 at 08:11:21PM +0100, Miguel Ojeda wrote:
> On Mon, Feb 12, 2024 at 8:02 PM Ramon de C Valle <rcvalle@google.com> wrote:
> >
> > Sorry for the late reply. Sami might be the best person to answer this, but KCFI (not CFI) tests are lowered by passes that are architecture specific (see https://reviews.llvm.org/D119296), so we'd need to add support for RISC-V. There is no additional work required in the Rust compiler besides enabling it for the new target.
> 
> Thanks a lot Ramon!
> 
> Then for RISC-V let's go for the `depends on` for the moment, and we
> can remove when the support lands for RISC-V (ideally when someone has
> managed to boot it at least under some configuration).

If all you want is a boot under some configuration, that's not
difficult. After all, I found the original issue by booting a kernel
with CFI_CLANG enabled on the C side...

> There is no additional work required in the Rust compiler besides enabling it for the new target.

This is not super clear though, it says "in the Rust compiler", not "in
the kernel's buildsystem".

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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Conor Dooley]

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

On Mon, Feb 12, 2024 at 08:17:31PM +0000, Conor Dooley wrote:
> On Mon, Feb 12, 2024 at 08:11:21PM +0100, Miguel Ojeda wrote:
> > On Mon, Feb 12, 2024 at 8:02 PM Ramon de C Valle <rcvalle@google.com> wrote:
> > >
> > > Sorry for the late reply. Sami might be the best person to answer this, but KCFI (not CFI) tests are lowered by passes that are architecture specific (see https://reviews.llvm.org/D119296), so we'd need to add support for RISC-V. There is no additional work required in the Rust compiler besides enabling it for the new target.
> > 
> > Thanks a lot Ramon!
> > 
> > Then for RISC-V let's go for the `depends on` for the moment, and we
> > can remove when the support lands for RISC-V (ideally when someone has
> > managed to boot it at least under some configuration).
> 
> If all you want is a boot under some configuration, that's not
> difficult. After all, I found the original issue by booting a kernel
> with CFI_CLANG enabled on the C side...

Also, regardless of depends on on RISC-V, things will still be broken
on arm64 and x86_64, since KCFI is not enabled in rustc there either?

> > There is no additional work required in the Rust compiler besides enabling it for the new target.
> 
> This is not super clear though, it says "in the Rust compiler", not "in
> the kernel's buildsystem".

I realise I was not clear either. What I meant was that this talks about
rustc and not kbuild, so what is meant by "the new target" is not clear.
Do arm64 and x86_64 have functional support, so adding RISC-V in rustc
is needed, or did you mean for the new target in the kernel?



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

Re: [PATCH v1 0/2] RISC-V: enable rust

[Ramon de C Valle]

> I realise I was not clear either. What I meant was that this talks about
> rustc and not kbuild, so what is meant by "the new target" is not clear.
> Do arm64 and x86_64 have functional support, so adding RISC-V in rustc
> is needed, or did you mean for the new target in the kernel?

Sorry, I was referring to the targets at:
https://github.com/rust-lang/rust/tree/master/compiler/rustc_target/src/spec/targets