LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Wedson Almeida Filho <wedsonaf@google.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	ojeda@kernel.org, Linus Torvalds <torvalds@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	rust-for-linux@vger.kernel.org,
	linux-kbuild <linux-kbuild@vger.kernel.org>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 00/13] [RFC] Rust support
Date: Mon, 26 Apr 2021 15:40:29 +0100
Message-ID: <YIbQ3dHOpyD/yymW@google.com> (raw)
In-Reply-To: <CACRpkdat-4BbKHMBerdxXBseMb9O3PiDRZmMLP_OWFE2ctSgEg@mail.gmail.com>

Linus, again thanks for taking the time to look into this. I think it's great
for us to get into this level of detail.

On Mon, Apr 26, 2021 at 02:18:33AM +0200, Linus Walleij wrote:
> For device drivers you will certainly have to wrap assembly as well.
> Or C calls that only contain assembly to be precise.

Sure, I don't think this would be a problem.

> A typical example is the way device drivers talk to actual hardware:
> readl()/writel(), readw()/writew(), readb()/writeb() for memory-mapped
> IO or inb()/outb() for port-mapped I/O.
> 
> So there is for example this (drivers/gpio/gpio-pl061.c):
> 
>         writeb(pl061->csave_regs.gpio_is, pl061->base + GPIOIS);
>         writeb(pl061->csave_regs.gpio_ibe, pl061->base + GPIOIBE);
>         writeb(pl061->csave_regs.gpio_iev, pl061->base + GPIOIEV);
>         writeb(pl061->csave_regs.gpio_ie, pl061->base + GPIOIE);
> 
> We write a number of u32 into u32 sized registers, this
> pl061->base is a void __iomem * so a pretty unsafe thing to
> begin with and then we add an offset to get to the register
> we want.
> 
> writel() on ARM for example turns into (arch/arm/include/asm/io.h):
> 
> static inline void __raw_writel(u32 val, volatile void __iomem *addr)
> {
>         asm volatile("str %1, %0"
>                      : : "Qo" (*(volatile u32 __force *)addr), "r" (val));
> }
> 
> This is usually sprinkled all over a device driver, called in loops etc.
> Some of these will contain things like buffer drains and memory
> barriers. Elaborately researched for years so they will need to
> be there.
> 
> I have no clue how this thing would be expressed in Rust.
> Even less how it would call the right code in the end.
> That makes me feel unsafe and puzzled so this is a specific
> area where "the Rust way" needs to be made very tangible
> and easy to understand.
> 
> How would I write these 4 registers in Rust? From the actual
> statements down to the CPU instructions, top to bottom,
> that is what a driver writer wants to know.

Here's an example of how this could be implemented. Again, we're happy to
iterate on this (just like any other piece of software, independently of
language), but I think this will give you an idea. We'd begin with an
abstraction for a mapped io region:

pub struct IoMemBlock<const SIZE: usize> {
    ptr: *mut u8
}

Note here that we encode the size of the block at compile time. We'll get our
safety guarantees from it.

For this abstraction, we provide the following implementation of the write
function:

impl<const SIZE: usize> IoMemBlock<SIZE> {
    pub fn write<T>(&self, value: T, offset: usize) {
        if let Some(end) = offset.checked_add(size_of::<T>()) {
            if end <= SIZE {
                // SAFETY: We just checked above that offset was within bounds.
                let ptr = unsafe { self.ptr.add(offset) } as *mut T;
                // SAFETY: We just checked that the offset+size was within bounds.
                unsafe { ptr.write_volatile(value) };
                return;
            }
        }
        // SAFETY: Unimplemented function to cause compilation error.
        unsafe { bad_write() };
    }
}

Now suppose we have some struct like:

pub struct MyDevice {
    base: IoMemBlock<100>,
    reg1: u32,
    reg2: u64,
}

Then a function similar to your example would be this:

pub fn do_something(pl061: &MyDevice) {
    pl061.base.write(pl061.reg1, GPIOIS);
    pl061.base.write(pl061.reg2, GPIOIBE);
    pl061.base.write(20u8, 99);
}

I have this example here: https://rust.godbolt.org/z/chE3vjacE

The x86 compiled output of the code above is as follows:

        mov     eax, dword ptr [rdi + 16]
        mov     rcx, qword ptr [rdi]
        mov     dword ptr [rcx + 16], eax
        mov     rax, qword ptr [rdi + 8]
        mov     qword ptr [rcx + 32], rax
        mov     byte ptr [rcx + 99], 20
        ret

Some observations:
1. do_something is completely safe: all accesses to memory are checked.
2. The only unsafe part that could involve the driver for this would be the
creation of IoMemBlock: my expectation is that this would be implemented by the
bus driver or some library that maps the appropriate region and caps the size.
That is, we can also build a safe abstraction for this.
3. All checks are optimised away because they uses compile-time constants. The
code presented above is as efficient as C.
4. All code is Rust code and therefore type-checked during compilation, there is
no need for macros.
5. Note that the code supports all sizes, and selects which one to use based on
the type of the first argument (the example above has 8, 32, 64 bit examples).
6. If the developer writing a driver accidentally uses an offset beyond the
limit, they will get a compilation error (bad_write is left unimplemented).
Perhaps we could find a better way to indicate this, but a compilation error is
definitely better than corrupting state (potentially silently) at runtime.
7. We could potentially design a way to limit which offsets are available for a
given IoMemBlock, I just haven't thought through it yet, but it would also
reduce the number of mistakes a developer could make.


> If the result of the exercise is that a typical device driver
> will contain more unsafe code than not, then device drivers
> are not a good starting point for Rust in the Linux kernel.
> In that case I would recommend that Rust start at a point
> where there is a lot of abstract code that is prone to the
> kind of problems that Rust is trying to solve. My intuition
> would be such things as network protocols. But I may be
> wrong.

Agreed. But based on the example above, I don't expect a lot (if any) of unsafe
code in drivers due accessing io memory.

> This is really neat. I think it is a good example where Rust
> really provides the right tool for the job.
> 
> And it is very far away from any device driver. Though some
> drivers need pages.

Sure, I didn't mean to imply that this is useful in drivers, I just meant it as
an example.

> This is true for any constrained language. I suppose we could write
> kernel modules in Haskell as well, or Prolog, given the right wrappers,
> and that would also attain the same thing: you get the desired
> restrictions in the target language by way of this adapter.

Agreed. Rust is different in that it doesn't need a garbage collector, so it can
achieve performance comparable to C, which is something that we can't claim
about Haskell and Prolog atm -- I actually like Haskell better than Rust, but
it's not practical at the moment for kernel development.

> The syntax and semantic meaning of things with lots of
> impl <T: ?Sized> Wrapper<T> for ... is just really intimidating
> but I suppose one can learn it. No big deal.

I agree it's intimidating, but so are macros like ____MAKE_OP in bitfield.h --
the former has the advantage of being type-checked. Writing macros like
____MAKE_OP is a hit-and-miss exercise in my experience. However, I feel that
both cases benefit from being specialised implementations that are somewhat
rare.

> What I need to know as device driver infrastructure maintainer is:
> 
> 1. If the language is expressive enough to do what device driver
>    authors need to do in an efficient and readable manner which
>    is as good or better than what we have today.

What do you think of the example I provided above? I think that generics give
Rust an edge over C in terms of expressiveness, though abusing it may
significantly reduce readability.

> 2. Worry about double implementations of core library functions.

This indeed may be a problem, but I'm happy to have Rust wrappers call
C/assembly functions. With LTO this should not affect performance.

> 3. Kickback in practical problem solving.
> 
> This will be illustrated below.
> 
> Here is a device driver example that I wrote and merged
> just the other week (drivers/iio/magnetometer/yamaha-yas530.c)
> it's a nasty example, so I provide it to make a point.
> 
> static void yas53x_extract_calibration(u8 *data, struct yas5xx_calibration *c)
> {
>         u64 val = get_unaligned_be64(data);
> 
>         /*
>          * Bitfield layout for the axis calibration data, for factor
>          * a2 = 2 etc, k = k, c = clock divider
>          *
>          * n   7 6 5 4 3 2 1 0
>          * 0 [ 2 2 2 2 2 2 3 3 ] bits 63 .. 56
>          * 1 [ 3 3 4 4 4 4 4 4 ] bits 55 .. 48
>          * 2 [ 5 5 5 5 5 5 6 6 ] bits 47 .. 40
>          * 3 [ 6 6 6 6 7 7 7 7 ] bits 39 .. 32
>          * 4 [ 7 7 7 8 8 8 8 8 ] bits 31 .. 24
>          * 5 [ 8 9 9 9 9 9 9 9 ] bits 23 .. 16
>          * 6 [ 9 k k k k k c c ] bits 15 .. 8
>          * 7 [ c x x x x x x x ] bits  7 .. 0
>          */
>         c->a2 = FIELD_GET(GENMASK_ULL(63, 58), val) - 32;
>         c->a3 = FIELD_GET(GENMASK_ULL(57, 54), val) - 8;
>         c->a4 = FIELD_GET(GENMASK_ULL(53, 48), val) - 32;
>         c->a5 = FIELD_GET(GENMASK_ULL(47, 42), val) + 38;
>         c->a6 = FIELD_GET(GENMASK_ULL(41, 36), val) - 32;
>         c->a7 = FIELD_GET(GENMASK_ULL(35, 29), val) - 64;
>         c->a8 = FIELD_GET(GENMASK_ULL(28, 23), val) - 32;
>         c->a9 = FIELD_GET(GENMASK_ULL(22, 15), val);
>         c->k = FIELD_GET(GENMASK_ULL(14, 10), val) + 10;
>         c->dck = FIELD_GET(GENMASK_ULL(9, 7), val);
> }
> 
> This extracts calibration for the sensor from an opaque
> chunk of bytes. The calibration is stuffed into sequences of
> bits to save space at different offsets and lengths. So we turn
> the whole shebang passed in the u8 *data into a 64bit
> integer and start picking out the pieces we want.
> 
> We know a priori that u8 *data will be more than or equal
> to 64 bits of data. (Which is another problem but do not
> focus on that, let us look at this function.)
> 
> I have no idea how to perform this in
> Rust despite reading quite a lot of examples. We have
> created a lot of these helpers like FIELD_GET() and
> that make this kind of operations simple.

Would you mind sharing more about which aspect of this you feel is challenging?

I see now that Miguel has already responded to this thread so I'll stop here.
Happy to follow up on anything.

Thanks,
-Wedson

  parent reply index

Thread overview: 187+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-14 18:45 ojeda
2021-04-14 18:45 ` [PATCH 01/13] kallsyms: Support "big" kernel symbols (2-byte lengths) ojeda
2021-04-14 19:44   ` Matthew Wilcox
2021-04-14 19:59     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 02/13] kallsyms: Increase maximum kernel symbol length to 512 ojeda
2021-04-14 23:48   ` Nick Desaulniers
2021-04-14 18:45 ` [PATCH 03/13] Makefile: Generate CLANG_FLAGS even in GCC builds ojeda
2021-04-14 18:59   ` Nathan Chancellor
2021-04-15 10:18     ` Miguel Ojeda
2021-04-14 23:46   ` Nick Desaulniers
2021-04-15  0:47     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 04/13] Kbuild: Rust support ojeda
2021-04-14 23:19   ` Nick Desaulniers
2021-04-15  0:43     ` Miguel Ojeda
2021-04-15 18:03       ` Nick Desaulniers
2021-04-16 12:23         ` Miguel Ojeda
2021-04-17 19:35       ` Masahiro Yamada
2021-04-16 13:38   ` Peter Zijlstra
2021-04-16 17:05     ` Linus Torvalds
2021-04-16 17:47       ` Miguel Ojeda
2021-04-16 18:09         ` Al Viro
2021-04-16 18:57           ` Miguel Ojeda
2021-04-16 20:22             ` Willy Tarreau
2021-04-16 20:34               ` Connor Kuehl
2021-04-16 20:58                 ` Willy Tarreau
2021-04-16 21:39                   ` Miguel Ojeda
2021-04-16 22:04                     ` Willy Tarreau
2021-04-16 22:45                       ` Al Viro
2021-04-16 23:46                       ` Miguel Ojeda
2021-04-17  4:24                         ` Willy Tarreau
2021-04-17 15:38                           ` Miguel Ojeda
2021-04-16 21:19               ` Miguel Ojeda
2021-04-16 17:34     ` Miguel Ojeda
2021-04-19 19:58       ` David Sterba
2021-04-19 20:17         ` Matthew Wilcox
2021-04-19 21:03           ` Miguel Ojeda
2021-04-19 20:54         ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 05/13] Rust: Compiler builtins crate ojeda
2021-04-14 19:19   ` Linus Torvalds
2021-04-14 19:34     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 06/13] Rust: Module crate ojeda
2021-04-14 18:45 ` [PATCH 07/13] Rust: Kernel crate ojeda
2021-04-14 19:31   ` Linus Torvalds
2021-04-14 19:50     ` Miguel Ojeda
2021-04-14 18:45 ` [PATCH 08/13] Rust: Export generated symbols ojeda
2021-04-14 18:46 ` [PATCH 09/13] Samples: Rust examples ojeda
2021-04-14 19:34   ` Linus Torvalds
2021-04-14 19:42     ` Miguel Ojeda
2021-04-14 19:49       ` Matthew Wilcox
2021-04-16 11:46       ` Andrej Shadura
2021-04-14 23:24     ` Nick Desaulniers
2021-04-15  7:10       ` Greg Kroah-Hartman
2021-04-15  7:39         ` Nick Desaulniers
2021-04-15 12:42         ` Miguel Ojeda
2021-04-16 13:07         ` Sven Van Asbroeck
2021-04-16 13:20           ` Greg Kroah-Hartman
2021-04-14 18:46 ` [PATCH 10/13] Documentation: Rust general information ojeda
2021-04-14 22:17   ` Nick Desaulniers
2021-04-14 23:34     ` Miguel Ojeda
2021-04-14 18:46 ` [PATCH 11/13] MAINTAINERS: Rust ojeda
2021-04-14 21:55   ` Nick Desaulniers
2021-04-14 22:02     ` Miguel Ojeda
2021-04-14 22:36   ` Nick Desaulniers
2021-04-14 18:46 ` [PATCH 12/13] Rust: add abstractions for Binder (WIP) ojeda
2021-04-14 18:46 ` [PATCH 13/13] Android: Binder IPC in Rust (WIP) ojeda
2021-04-14 19:44 ` [PATCH 00/13] [RFC] Rust support Linus Torvalds
2021-04-14 20:20   ` Miguel Ojeda
2021-04-15  1:38     ` Kees Cook
2021-04-15  8:26       ` David Laight
2021-04-15 18:08         ` Kees Cook
2021-04-15 12:39       ` Miguel Ojeda
2021-04-14 20:09 ` Matthew Wilcox
2021-04-14 20:21   ` Linus Torvalds
2021-04-14 20:35     ` Josh Triplett
2021-04-14 22:08     ` David Laight
2021-04-14 20:29   ` Miguel Ojeda
2021-04-18 15:31   ` Wedson Almeida Filho
2021-04-15  0:22 ` Nick Desaulniers
2021-04-15 10:05   ` Miguel Ojeda
2021-04-15 18:58 ` Peter Zijlstra
2021-04-16  2:22   ` Wedson Almeida Filho
2021-04-16  4:25     ` Al Viro
2021-04-16  5:02       ` Wedson Almeida Filho
2021-04-16  5:39         ` Paul Zimmerman
2021-04-16  7:46         ` Peter Zijlstra
2021-04-16  7:09     ` Peter Zijlstra
2021-04-17  5:23       ` comex
2021-04-17 12:46       ` David Laight
2021-04-17 14:51       ` Paolo Bonzini
2021-04-19  7:32         ` Peter Zijlstra
2021-04-19  7:53           ` Paolo Bonzini
2021-04-19  8:26             ` Peter Zijlstra
2021-04-19  8:35               ` Peter Zijlstra
2021-04-19  9:02               ` Paolo Bonzini
2021-04-19  9:36                 ` Peter Zijlstra
2021-04-19  9:40                   ` Paolo Bonzini
2021-04-19 11:01                     ` Will Deacon
2021-04-19 17:14                   ` Linus Torvalds
2021-04-19 18:38                     ` Paolo Bonzini
2021-04-19 18:50                       ` Linus Torvalds
2021-04-22 10:03     ` Linus Walleij
2021-04-22 14:09       ` David Laight
2021-04-22 15:24       ` Wedson Almeida Filho
2021-04-26  0:18         ` Linus Walleij
2021-04-26 14:26           ` Miguel Ojeda
2021-04-26 14:40           ` Wedson Almeida Filho [this message]
2021-04-26 16:03             ` Miguel Ojeda
2021-04-27 10:54             ` Linus Walleij
2021-04-27 11:13               ` Robin Randhawa
2021-04-29  1:52               ` Wedson Almeida Filho
2021-04-26 18:01           ` Miguel Ojeda
2021-04-22 21:28       ` Miguel Ojeda
2021-04-26  0:31         ` Linus Walleij
2021-04-26 18:18           ` Miguel Ojeda
2021-04-27 11:13             ` Linus Walleij
2021-04-28  2:51               ` Kyle Strand
2021-04-28  3:10               ` Miguel Ojeda
2021-05-04 21:21                 ` Linus Walleij
2021-05-04 23:30                   ` Miguel Ojeda
2021-05-05 11:34                     ` Linus Walleij
2021-05-05 14:17                       ` Miguel Ojeda
2021-05-05 15:13                         ` Enrico Weigelt, metux IT consult
2021-05-06 12:47                         ` Linus Walleij
2021-05-07 18:23                           ` Miguel Ojeda
2021-04-16  4:27   ` Boqun Feng
2021-04-16  6:04     ` Nick Desaulniers
2021-04-16 18:47       ` Paul E. McKenney
2021-04-19 20:35         ` Nick Desaulniers
2021-04-19 21:37           ` Paul E. McKenney
2021-04-19 22:03           ` Miguel Ojeda
2021-04-16 20:48     ` Josh Triplett
2021-04-16  8:16   ` Michal Kubecek
2021-04-16  9:29     ` Willy Tarreau
2021-04-16 11:24 ` Peter Zijlstra
2021-04-16 13:07   ` Wedson Almeida Filho
2021-04-16 14:19     ` Peter Zijlstra
2021-04-16 15:04       ` Miguel Ojeda
2021-04-16 15:43         ` Peter Zijlstra
2021-04-16 16:21           ` Miguel Ojeda
2021-04-16 15:33       ` Wedson Almeida Filho
2021-04-16 16:14         ` Willy Tarreau
2021-04-16 17:10           ` Miguel Ojeda
2021-04-16 17:18             ` Peter Zijlstra
2021-04-16 18:08               ` Matthew Wilcox
2021-04-17 11:17                 ` Peter Zijlstra
2021-04-17 11:46                   ` Willy Tarreau
2021-04-17 14:24                     ` Peter Zijlstra
2021-04-17 14:36                       ` Willy Tarreau
2021-04-17 13:46                   ` David Laight
2021-04-16 17:37             ` Willy Tarreau
2021-04-16 17:46               ` Connor Kuehl
2021-04-20  0:24               ` Nick Desaulniers
2021-04-20  3:47                 ` Willy Tarreau
2021-04-20  5:56                 ` Greg Kroah-Hartman
2021-04-20  6:16                   ` Willy Tarreau
2021-04-29 15:38                     ` peter enderborg
2021-04-17 13:53           ` Wedson Almeida Filho
2021-04-17 14:21             ` Willy Tarreau
2021-04-17 15:23               ` Miguel Ojeda
2021-04-18 15:51               ` Wedson Almeida Filho
2021-04-17 12:41       ` David Laight
2021-04-17 13:01         ` Wedson Almeida Filho
2021-04-16 15:03     ` Matthew Wilcox
2021-04-17 13:29       ` Wedson Almeida Filho
2021-04-16 15:58     ` Theodore Ts'o
2021-04-16 16:21       ` Wedson Almeida Filho
2021-04-17 15:11       ` Paolo Bonzini
2021-04-16 14:21   ` Miguel Ojeda
2021-04-17 20:42 ` Richard Weinberger
2021-04-28 18:34 ` Mariusz Ceier
2021-04-28 20:25   ` Nick Desaulniers
2021-04-28 21:21   ` David Laight
2021-04-29 11:14     ` Kajetan Puchalski
2021-04-29 11:25   ` Kajetan Puchalski
2021-04-29 14:06     ` Mariusz Ceier
2021-04-29 14:13       ` Sven Van Asbroeck
2021-04-29 14:26         ` Willy Tarreau
2021-04-29 15:06       ` Al Viro
2021-04-29 16:09         ` Mariusz Ceier
2021-04-30  6:39     ` Thomas Schoebel-Theuer
2021-04-30  8:30       ` David Laight
2021-05-05 13:58       ` Enrico Weigelt, metux IT consult
2021-05-05 14:41         ` Miguel Ojeda
2021-04-29  5:20 Mariusz Ceier
2021-04-29  5:21 Mariusz Ceier
2021-04-29  8:18 ` David Laight
2021-07-30 23:22 Dillan Jackson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YIbQ3dHOpyD/yymW@google.com \
    --to=wedsonaf@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ojeda@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git