On Wed Aug 12, 2026 at 9:52 PM CEST, Maurice Hieronymus wrote:
> +const QEMU_VENDOR_ID: u16 = 0x1234;
> +const QEMU_EDU_DEVICE_ID: u32 = 0x11e8;
> +const QEMU_EDU_DEVICE_MAGIC: u8 = 0xed;
This should be local to magic().
> +const QEMU_DMA_BASE: u64 = 0x40000;
This could be of type regs::DMA_DST and local to test_dma().
> +
> +const IRQ_MAGIC_VALUE: u32 = 42;
> +
> +/// Bit set in `IRQ_STATUS` when a DMA transfer has completed.
> +const DMA_IRQ: u32 = 0x100;
Why is this not part of the IRQ_STATUS declaration in register!()?
> +
> +mod regs {
> + use super::*;
> +
> + register! {
> + pub(super) IDENTIFICATION(u32) @ 0x0 {
> + 31:24 major;
> + 23:16 minor;
> + 7:0 magic;
> + }
> +
> + pub(super) LIVENESS_CHECK(u32) @ 0x04 {}
> +
> + pub(super) FACTORIAL(u32) @ 0x08 {}
> +
> + pub(super) STATUS(u32) @ 0x20 {
> + 0:0 computing;
> + 7:7 raise_interrupt;
> + }
> +
> + pub(super) IRQ_STATUS(u32) @ 0x24 {}
> + pub(super) IRQ_RAISE(u32) @ 0x60 {}
> + pub(super) IRQ_ACK(u32) @ 0x64 {}
> +
> + pub(super) DMA_SRC(u64) @ 0x80 {}
> + pub(super) DMA_DST(u64) @ 0x88 {}
> + pub(super) DMA_COUNT(u64) @ 0x90 {}
> + pub(super) DMA_COMMAND(u64) @ 0x98 {
> + 0:0 start_transfer;
> + 1:1 direction;
> + 2:2 raise_irq;
> + }
> + }
> +
> + pub(super) const END: usize = 0xA0;
> +}
> +
> +type Bar0<'a> = pci::Bar<'a, { regs::END }>;
> +
> +struct EduDriver;
> +
> +#[pin_data(PinnedDrop)]
> +struct EduDriverData<'bound> {
> + pdev: &'bound pci::Device,
> + #[pin]
> + irq_handler: irq::Registration<'bound, IrqHandler<'bound>>,
> + // Declared last so the device stays enabled until the IRQ handler is
> freed.
> + _enable: pci::DeviceEnableGuard<'bound>,
> +}
> +
> +#[pin_data]
> +struct IrqHandler<'a> {
> + pdev: &'a pci::Device,
> + bar: Bar0<'a>,
> + #[pin]
> + irq_test_completion: Completion,
> + #[pin]
> + irq_dma_completion: Completion,
> + dma: Coherent<u64>,
> +}
> +
> +impl EduDriver {
> + fn init(pdev: &pci::Device<Bound>, bar: &Bar0<'_>, handler:
> &IrqHandler<'_>) -> Result {
I think this should rather be named selftest() and it should be a method on
EduDriverData instead rather than a function on EduDriver. The same goes for all
the other functions below.
> + Self::config_space(pdev);
> + Self::magic(pdev, bar)?;
> + Self::liveness_check(pdev, bar)?;
> + Self::factorial(pdev, bar)?;
> + Self::test_irq(pdev, handler)?;
> + Self::test_dma(pdev, handler)?;
> + Ok(())
> + }
<snip>
> + fn magic(pdev: &pci::Device<Bound>, bar: &Bar0<'_>) -> Result {
> + let identification = bar.read(regs::IDENTIFICATION);
> +
> + let magic: u8 = identification.magic().into();
> +
> + if magic != QEMU_EDU_DEVICE_MAGIC {
Can't we construct a regs::IDENTIFICATION value and compare this instead of the
raw value?
> + dev_err!(
> + pdev,
> + "magic mismatch: expected {:#x} got {:#x}\n",
> + QEMU_EDU_DEVICE_MAGIC,
> + magic
> + );
> + return Err(ENODEV);
> + }
> +
> + dev_info!(
> + pdev,
> + "major: {:#x} minor: {:#x}\n",
> + identification.major(),
> + identification.minor()
> + );
> + Ok(())
> + }
> +
> + fn liveness_check(pdev: &pci::Device<Bound>, bar: &Bar0<'_>) -> Result {
> + let test_value = 0xabcd;
> +
> + bar.write(regs::LIVENESS_CHECK, test_value.into());
> +
> + let inverse_value = bar.read(regs::LIVENESS_CHECK).into_raw();
> +
> + if inverse_value != !test_value {
I think this would read better as
let pattern = regs::LIVENESS_CHECK::from_raw(0xdead_beef);
bar.write_reg(pattern);
if bar.read(regs::LIVENESS_CHECK) != !pattern {
return Err(ENODEV);
}
And in the regs module we could implement
impl core::ops::Not for LIVENESS_CHECK {
type Output = Self;
fn not(self) -> Self {
Self::from_raw(!self.into_raw())
}
}
> + fn test_dma(pdev: &pci::Device<Bound>, handler: &IrqHandler<'_>) ->
> Result {
> + dev_dbg!(pdev, "testing dma\n");
> +
> + let dma = &handler.dma;
> +
> + const DMA_VALUE: u64 = 42;
> +
> + kernel::dma_write!(dma, , DMA_VALUE);
> +
> + handler.bar.write(regs::DMA_SRC, dma.dma_handle().into());
> + handler.bar.write(regs::DMA_DST, QEMU_DMA_BASE.into());
> + handler
> + .bar
> + .write(regs::DMA_COUNT, (dma.size() as u64).into());
> + handler.bar.write(
> + regs::DMA_COMMAND,
> + regs::DMA_COMMAND::zeroed()
> + .with_start_transfer(true)
> + .with_direction(false)
> + .with_raise_irq(true),
> + );
> +
> + handler.irq_dma_completion.wait_for_completion();
> +
> + // Destroy previous value to test roundtrip
> + kernel::dma_write!(dma, , 0);
> +
> + handler.bar.write(regs::DMA_SRC, QEMU_DMA_BASE.into());
> + handler.bar.write(regs::DMA_DST, dma.dma_handle().into());
> + handler
> + .bar
> + .write(regs::DMA_COUNT, (dma.size() as u64).into());
Please avoid as casts and use FromSafeCast instead.
> +impl pci::Driver for EduDriver {
> + type IdInfo = ();
> + type Data<'bound> = EduDriverData<'bound>;
> +
> + const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
> +
> + fn probe<'bound>(
> + pdev: &'bound pci::Device<kernel::device::Core<'_>>,
> + _id_info: &'bound Self::IdInfo,
> + ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
> + pin_init::pin_init_scope(move || {
Let's make this a single try_pin_init!() block. You can subsequently chain it
with pin_chain() to call selftest().
This is what I came up with for a talk; it doesn't align perfectly as it has a
couple of things refactored and uses self-referencial pin-init, but you get the
idea.
fn probe<'bound>(
pdev: &'bound pci::Device<kernel::device::Core<'_>>,
_id_info: Option<&'bound Self::IdInfo>,
) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
try_pin_init!(EduDriverData {
_: {
dev_dbg!(
pdev,
"Probe Rust EDU driver sample (PCI ID: {}, 0x{:x}).\n",
pdev.vendor_id(),
pdev.device_id(),
);
},
_enable: pdev.enable_device().inspect(|_| pdev.set_master())?,
bar: pdev.iomap_region_sized(0, c"rust_driver_edu")?,
irq_vec: pdev.alloc_irq_vectors(1, 1,
IrqTypes::default().with(pci::IrqType::Msi))?,
irq_handler <- irq::Registration::new(
irq_vec.index(0)?.into(),
Flags::TRIGGER_NONE,
c"rust_edu_irq",
try_pin_init!(IrqHandler {
bar,
irq_test_completion <- Completion::new(),
irq_dma_completion <- Completion::new(),
pdev,
}),
),
dma: {
let mask = DmaMask::new::<28>();
// SAFETY: There are no concurrent calls to DMA allocation
and mapping primitives.
unsafe { pdev.dma_set_mask_and_coherent(mask)? };
Coherent::zeroed(pdev.as_ref(), GFP_KERNEL)
}?,
pdev,
})
.pin_chain(|this| this.selftest())
}
> + let bar = pdev.iomap_region_sized::<{ regs::END }>(0,
> c"rust_driver_edu")?;
With the above, we also get rid of the turbofish.
