| Message ID | 20230307102441.94417-1-conor.dooley@microchip.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RISC-V: enable rust | expand |
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
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.
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.
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
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
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
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.
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.
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.
> 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.
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
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!
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?
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
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 :)
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
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.
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
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.
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
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.
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
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.
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
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
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
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".
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
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?
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?
> 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
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.