On Wed, Oct 23, 2024 at 10:57:37AM -0500, Rob Herring wrote:
> On Tue, Oct 22, 2024 at 11:31:50PM +0200, Danilo Krummrich wrote:
> > This commit adds a sample Rust PCI driver for QEMU's "pci-testdev"
> > device. To enable this device QEMU has to be called with
> > `-device pci-testdev`.
>
> Note that the DT unittests also use this device. So this means we have 2
> drivers that bind to the device. Probably it's okay, but does make
> them somewhat mutually-exclusive.
>
> > The same driver shows how to use the PCI device / driver abstractions,
> > as well as how to request and map PCI BARs, including a short sequence of
> > MMIO operations.
> >
> > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
> > samples/rust/Kconfig | 11 ++++
> > samples/rust/Makefile | 1 +
> > samples/rust/rust_driver_pci.rs | 109 ++++++++++++++++++++++++++++++++
> > 4 files changed, 122 insertions(+)
> > create mode 100644 samples/rust/rust_driver_pci.rs
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2d00d3845b4a..d9c512a3e72b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17940,6 +17940,7 @@ F: include/linux/of_pci.h
> > F: include/linux/pci*
> > F: include/uapi/linux/pci*
> > F: rust/kernel/pci.rs
> > +F: samples/rust/rust_driver_pci.rs
> >
> > PCIE DRIVER FOR AMAZON ANNAPURNA LABS
> > M: Jonathan Chocron <jonnyc@xxxxxxxxxx>
> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> > index b0f74a81c8f9..6d468193cdd8 100644
> > --- a/samples/rust/Kconfig
> > +++ b/samples/rust/Kconfig
> > @@ -30,6 +30,17 @@ config SAMPLE_RUST_PRINT
> >
> > If unsure, say N.
> >
> > +config SAMPLE_RUST_DRIVER_PCI
> > + tristate "PCI Driver"
> > + depends on PCI
> > + help
> > + This option builds the Rust PCI driver sample.
> > +
> > + To compile this as a module, choose M here:
> > + the module will be called driver_pci.
> > +
> > + If unsure, say N.
> > +
> > config SAMPLE_RUST_HOSTPROGS
> > bool "Host programs"
> > help
> > diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> > index 03086dabbea4..b66767f4a62a 100644
> > --- a/samples/rust/Makefile
> > +++ b/samples/rust/Makefile
> > @@ -2,5 +2,6 @@
> >
> > obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o
> > obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o
> > +obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o
> >
> > subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS) += hostprogs
> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> > new file mode 100644
> > index 000000000000..d24dc1fde9e8
> > --- /dev/null
> > +++ b/samples/rust/rust_driver_pci.rs
> > @@ -0,0 +1,109 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Rust PCI driver sample (based on QEMU's `pci-testdev`).
> > +//!
> > +//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
> > +
> > +use kernel::{bindings, c_str, devres::Devres, pci, prelude::*};
> > +
> > +struct Regs;
> > +
> > +impl Regs {
> > + const TEST: usize = 0x0;
> > + const OFFSET: usize = 0x4;
> > + const DATA: usize = 0x8;
> > + const COUNT: usize = 0xC;
> > + const END: usize = 0x10;
> > +}
> > +
> > +type Bar0 = pci::Bar<{ Regs::END }>;
> > +
> > +#[derive(Debug)]
> > +struct TestIndex(u8);
> > +
> > +impl TestIndex {
> > + const NO_EVENTFD: Self = Self(0);
> > +}
> > +
> > +struct SampleDriver {
> > + pdev: pci::Device,
> > + bar: Devres<Bar0>,
> > +}
> > +
> > +kernel::pci_device_table!(
> > + PCI_TABLE,
> > + MODULE_PCI_TABLE,
> > + <SampleDriver as pci::Driver>::IdInfo,
> > + [(
> > + pci::DeviceId::new(bindings::PCI_VENDOR_ID_REDHAT, 0x5),
> > + TestIndex::NO_EVENTFD
> > + )]
> > +);
> > +
> > +impl SampleDriver {
> > + fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
> > + // Select the test.
> > + bar.writeb(index.0, Regs::TEST);
> > +
> > + let offset = u32::from_le(bar.readl(Regs::OFFSET)) as usize;
>
> The C version of readl takes care of from_le for you. Why not here?
It's just an abstraction around the C readl(), so it does -- good catch.
>
> Also, can't we do better with rust and make this a generic:
>
> let offset = bar.read::<u32>(Regs::OFFSET)) as usize;
I think we probably could, but we'd still need to handle the special cases for 1
to 8 bytes type size (always using memcopy_{to,from}io() would lead to
unnecessary overhead). Hence, there's probably not much benefit in that.
Also, what would be the logic for a generic `{read, write}::<T>` in terms of
memory barriers? I think memcopy_{to,from}io() is always "relaxed", isn't it?
I think it's probably best to keep the two separate, the b,w,l,q variants and
a generic one that maps to memcopy_{to,from}io().
>
>
> > + let data = bar.readb(Regs::DATA);
> > +
> > + // Write `data` to `offset` to increase `count` by one.
> > + //
> > + // Note that we need `try_writeb`, since `offset` can't be checked at compile-time.
> > + bar.try_writeb(data, offset)?;
> > +
> > + Ok(u32::from_le(bar.readl(Regs::COUNT)))
> > + }
> > +}
>
