Re: [PATCH v3 5/5] rust: samples: add EDU PCI driver sample

[Danilo Krummrich]

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.