| Message ID | 20231009013912.4048593-1-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rust abstractions for network PHY drivers | expand |
On Mon, Oct 09, 2023 at 10:39:09AM +0900, FUJITA Tomonori wrote: > This patchset adds Rust abstractions for phylib. It doesn't fully > cover the C APIs yet but I think that it's already useful. I implement > two PHY drivers (Asix AX88772A PHYs and Realtek Generic FE-GE). Seems > they work well with real hardware. > > The first patch introduces Rust bindings for phylib. > > The second patch updates the ETHERNET PHY LIBRARY entry in MAINTAINERS > file; adds the binding file and me as a maintainer of the Rust > bindings (as Andrew Lunn suggested). > > The last patch introduces the Rust version of Asix PHY drivers, > drivers/net/phy/ax88796b.c. The features are equivalent to the C > version. You can choose C (by default) or Rust version on kernel > configuration. I at last got around to installing a rust tool chain and tried to build the code. I get what looks like linker errors. linux$ make LLVM=1 rustavailable Rust is available! dpkg says: +++-==============-============-============-================================= ii llvm 1:16.0-57 amd64 Low-Level Virtual Machine (LLVM) I build with make LLVM=1 and get lots of warnings like: vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placeANtNtNtB4_5ascii10ascii_char9AsciiCharja_EB4_+0x0: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placeFUPuENtNtNtB4_4task4wake8RawWakerEB4_+0x0: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placeINtNtB4_6option6OptionINtNtNtNtB4_4iter8adapters7flatten7FlattenINtBJ_8IntoIterNtNtB4_4char11EscapeDebugEEEEB4_+0x0: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placeNtNtB4_3fmt5ErrorEB4_+0x0: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RINvNtCs4KbNGwnAC5t_4core3ptr13drop_in_placesEB4_+0x0: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f8classify+0x5a: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f16partial_classify+0x1f: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f13classify_bits+0x28: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f7next_up+0x32: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f9next_down+0x34: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RNvMNtCs4KbNGwnAC5t_4core3f32f8midpoint+0xc3: 'naked' return found in RETHUNK build vmlinux.o: warning: objtool: _RNvNvMNtCs4KbNGwnAC5t_4core3f32f7to_bits13ct_f32_to_u32+0x4a: 'naked' return found in RETHUNK build Any ideas? Andrew
On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > Any ideas? That is `RETHUNK` and `X86_KERNEL_IBT`. Since this will keep confusing people, I will make it a `depends on !` as discussed in the past. I hope it is OK for e.g. Andrea. Cheers, Miguel
On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote: > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > Any ideas? > > That is `RETHUNK` and `X86_KERNEL_IBT`. > > Since this will keep confusing people, I will make it a `depends on !` > as discussed in the past. I hope it is OK for e.g. Andrea. That's not ok as you want that option enabled on systems that have those broken processors which need this option for proper security. You would be forcing people to disable this to enable Rust support? confused, greg k-h
On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote: > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > Any ideas? > > That is `RETHUNK` and `X86_KERNEL_IBT`. > > Since this will keep confusing people, I will make it a `depends on !` > as discussed in the past. I hope it is OK for e.g. Andrea. I really do suggest you work on your kconfig. The expectation is any configuration that kconfig is happy with will build. People like Arnd Bergmann do lots of randconfig builds. We don't want his work upset by Rust code. And as a Rust beginning, i find this pretty unfriendly, in that i followed https://docs.kernel.org/rust/quick-start.html but did not get a working build. Andrew
On Mon, Oct 9, 2023 at 3:25 PM Andrew Lunn <andrew@lunn.ch> wrote: > > I really do suggest you work on your kconfig. The expectation is any > configuration that kconfig is happy with will build. People like Arnd > Bergmann do lots of randconfig builds. We don't want his work upset by > Rust code. > > And as a Rust beginning, i find this pretty unfriendly, in that i > followed https://docs.kernel.org/rust/quick-start.html but did not get > a working build. It is a "working" build: some people are actually using it as-is on purpose (with the warnings, I mean). Yes, it is bad, and we did not like it when they told us their build had those warnings and still used it, but it means it will make it harder for them when we restrict it. But your message is the perfect excuse for me to send the patch to restrict it, so thanks :) Cheers, Miguel
On Mon, Oct 9, 2023 at 3:46 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > That's not ok as you want that option enabled on systems that have those > broken processors which need this option for proper security. You would > be forcing people to disable this to enable Rust support? Yes, that is what would happen. But if we want to avoid the warnings and be proper (even if there are no real users of Rust yet), until the Rust compiler supports it and we wire it up, the only way is that, no? I think PeterZ preferred that we restricted it, and at this point I think it is a good idea to do so now in case somebody thinks this works. Cheers, Miguel
On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote: > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > Any ideas? > > That is `RETHUNK` and `X86_KERNEL_IBT`. > > Since this will keep confusing people, I will make it a `depends on !` > as discussed in the past. I hope it is OK for e.g. Andrea. Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel. If that constraint is introduced we either need to revert that patch in the Ubuntu kernel or disable Rust support. It would be nice to have a least something like CONFIG_RUST_IS_BROKEN_BUT_IM_HAPPY, off by default, and have `RUST_IS_BROKEN_BUT_IM_HAPPY || depends on !`. -Andrea > > Cheers, > Miguel
On Mon, Oct 9, 2023 at 4:21 PM Andrea Righi <andrea.righi@canonical.com> wrote: > > Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel. > If that constraint is introduced we either need to revert that patch > in the Ubuntu kernel or disable Rust support. Yeah, I was expecting that you would disable the Rust support, of course (not that you disable the security option! :). Cheers, Miguel
On Mon, Oct 09, 2023 at 04:13:02PM +0200, Miguel Ojeda wrote: > On Mon, Oct 9, 2023 at 3:46 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > That's not ok as you want that option enabled on systems that have those > > broken processors which need this option for proper security. You would > > be forcing people to disable this to enable Rust support? > > Yes, that is what would happen. But if we want to avoid the warnings > and be proper (even if there are no real users of Rust yet), until the > Rust compiler supports it and we wire it up, the only way is that, no? Then the main CONFIG_HAVE_RUST should have that dependency, don't force it on each individual driver. But note, that is probably not a good marketing statement as you are forced to make your system more insecure in order to use the "secure" language :( thanks, greg k-h
On Mon, Oct 09, 2023 at 04:21:09PM +0200, Andrea Righi wrote: > On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote: > > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > Any ideas? > > > > That is `RETHUNK` and `X86_KERNEL_IBT`. > > > > Since this will keep confusing people, I will make it a `depends on !` > > as discussed in the past. I hope it is OK for e.g. Andrea. > > Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel. > If that constraint is introduced we either need to revert that patch > in the Ubuntu kernel or disable Rust support. > > It would be nice to have a least something like > CONFIG_RUST_IS_BROKEN_BUT_IM_HAPPY, off by default, and have > `RUST_IS_BROKEN_BUT_IM_HAPPY || depends on !`. Should this actually be CONFIG_RUST_IS_BROKEN_ON_X86_BUT_IM_HAPPY ? At lest for networking, the code is architecture independent. For a driver to be useful, it needs to compile for most architectures. So i hope Rust will quickly make it to other architectures. And for PHY drivers, ARM and MIPS are probably more important than x86. Andrew
On Mon, Oct 09, 2023 at 04:21:09PM +0200, Andrea Righi wrote: > On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote: > > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > Any ideas? > > > > That is `RETHUNK` and `X86_KERNEL_IBT`. > > > > Since this will keep confusing people, I will make it a `depends on !` > > as discussed in the past. I hope it is OK for e.g. Andrea. > > Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel. > If that constraint is introduced we either need to revert that patch > in the Ubuntu kernel or disable Rust support. Why is rust enabled in the Ubuntu kernel as there is no in-kernel support for any real functionality? Or do you have out-of-tree rust drivers added to your kernel already? thanks, greg k-h
On Mon, Oct 09, 2023 at 04:56:36PM +0200, Andrew Lunn wrote: > On Mon, Oct 09, 2023 at 04:21:09PM +0200, Andrea Righi wrote: > > On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote: > > > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > Any ideas? > > > > > > That is `RETHUNK` and `X86_KERNEL_IBT`. > > > > > > Since this will keep confusing people, I will make it a `depends on !` > > > as discussed in the past. I hope it is OK for e.g. Andrea. > > > > Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel. > > If that constraint is introduced we either need to revert that patch > > in the Ubuntu kernel or disable Rust support. > > > > It would be nice to have a least something like > > CONFIG_RUST_IS_BROKEN_BUT_IM_HAPPY, off by default, and have > > `RUST_IS_BROKEN_BUT_IM_HAPPY || depends on !`. > > Should this actually be CONFIG_RUST_IS_BROKEN_ON_X86_BUT_IM_HAPPY ? Just do the proper dependency on RETHUNK and you should be fine, it will be able to be enabled on arches that do not require RETHUNK for proper security. > At lest for networking, the code is architecture independent. For a > driver to be useful, it needs to compile for most architectures. So i > hope Rust will quickly make it to other architectures. And for PHY > drivers, ARM and MIPS are probably more important than x86. Is MIPS a proper target for rust yet? thanks, greg k-h
On Mon, Oct 9, 2023 at 4:52 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > Then the main CONFIG_HAVE_RUST should have that dependency, don't force > it on each individual driver. Yes, that is what I meant (well, `CONFIG_RUST` is where we have the other restrictions). > But note, that is probably not a good marketing statement as you are > forced to make your system more insecure in order to use the "secure" > language :( Indeed, but until we catch up on that, it is what it is; i.e. it is not something that we want to keep there, it has to go away to make it viable. The other option we discussed back then was to print a big banner or something at runtime, but that is also not great (and people would still see warnings at build time -- for good reason). Cheers, Miguel
On Mon, Oct 09, 2023 at 04:56:47PM +0200, Greg KH wrote: > On Mon, Oct 09, 2023 at 04:21:09PM +0200, Andrea Righi wrote: > > On Mon, Oct 09, 2023 at 02:53:00PM +0200, Miguel Ojeda wrote: > > > On Mon, Oct 9, 2023 at 2:48 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > Any ideas? > > > > > > That is `RETHUNK` and `X86_KERNEL_IBT`. > > > > > > Since this will keep confusing people, I will make it a `depends on !` > > > as discussed in the past. I hope it is OK for e.g. Andrea. > > > > Disabling RETHUNK or IBT is not acceptable for a general-purpose kernel. > > If that constraint is introduced we either need to revert that patch > > in the Ubuntu kernel or disable Rust support. > > Why is rust enabled in the Ubuntu kernel as there is no in-kernel > support for any real functionality? Or do you have out-of-tree rust > drivers added to your kernel already? Rust in the Ubuntu kernel is just a "technology preview", enabled in the development release only. The idea is to provide all the toolchain, dependencies, headers, etc. in the generic distro kernel, so those who are willing to do experiments with Rust can do that without installing a custom kernel. And as soon as new Rust abstractions will be merged upstream we already have the infrastructure that would allow anybody to use them with all the components provided by the distro. So, we really don't have any out-of-tree module/driver that requires Rust at the moment. -Andrea > > thanks, > > greg k-h
On Mon, Oct 9, 2023 at 5:04 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > Is MIPS a proper target for rust yet? The compiler has support for it (https://github.com/Rust-for-Linux/linux/issues/107), but I didn't do mips pre-merge. The ones I tried (and that we had in the CI back then pre-merge) were: arm, arm64, ppc64le, riscv64 and x86_64. Cheers, Miguel
On Mon, Oct 09, 2023 at 05:06:45PM +0200, Miguel Ojeda wrote: > On Mon, Oct 9, 2023 at 4:52 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > Then the main CONFIG_HAVE_RUST should have that dependency, don't force > > it on each individual driver. > > Yes, that is what I meant (well, `CONFIG_RUST` is where we have the > other restrictions). Oops, yes, add it there please. > > But note, that is probably not a good marketing statement as you are > > forced to make your system more insecure in order to use the "secure" > > language :( > > Indeed, but until we catch up on that, it is what it is; i.e. it is > not something that we want to keep there, it has to go away to make it > viable. Is anyone working on the needed compiler changes for this to work properly on x86? > The other option we discussed back then was to print a big banner or > something at runtime, but that is also not great (and people would > still see warnings at build time -- for good reason). No, please don't do that, you would be making systems insecure and the mix of a kernel image with, and without, RET statements in it is going to be a huge mess. Just disable CONFIG_RUST for now until proper retbleed support is added to the compiler. thanks, greg k-h
On Mon, Oct 9, 2023 at 5:10 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > The compiler has support for it > (https://github.com/Rust-for-Linux/linux/issues/107), but I didn't do > mips pre-merge. > > The ones I tried (and that we had in the CI back then pre-merge) were: > arm, arm64, ppc64le, riscv64 and x86_64. By "in the CI" here means: booted in QEMU with a given kernel config. Also, I should have said that Michael Ellerman was the one that added the ppc64le one. Cheers, Miguel
On Mon, Oct 9, 2023 at 5:14 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > Is anyone working on the needed compiler changes for this to work > properly on x86? I don't know, I will ask. Cheers, Miguel