[RFC,11/11] rust: PCI: add BAR request and ioremap

Commit Message

Danilo Krummrich May 20, 2024, 5:25 p.m. UTC

From: Philipp Stanner <[e-mail]>

This commit implements a basic mechanism for requesting and IO-mapping
PCI BARs.

To perform IO on PCI devices it is necessary to have memory mapped PCI
BARs. Before mapping those, a region request should be performed so that
collisions with other drivers can be avoided.

Since all the logic necessary to obtain the aforementioned resources are
already implemented in C, Rust abstractions should use these interfaces.
Hereby, the Rust implementation has to ensure that all resources are
released again latest when the driver's remove() callback gets invoked,
or earlier if the driver drop()s the PCI resource.

This can be achieved through the Devres container, which uses devres
callbacks combined with Revocable to block access to the resource - in
this case, the PCI BAR and its IoMem.

A pci::Bar's Drop() trait shall deregister the memory region request and
iounmap() the mapping. In case remove() is invoked before such a Bar is
drop()ed, the Devres container shall ensure that access to the Bar is
revoke()d (through Revocable) so that no UAFs can occur.

Implement 'Bar', a container for requested and ioremapped PCI BARs.

Implement the Drop() trait such that the memory request and IO-mapping
get freed if Bar goes out of scope.

Implement Deref() so that the container is transparent.

Implement iomap_region() to create a Bar and have the result returned
through a Devres container, ensuring that the resources are latest freed
in the driver's remove() callback and access to the Bar is revoke()d for
outstanding users.

Signed-off-by: Philipp Stanner <[e-mail]>
Co-developed-by: Danilo Krummrich <[e-mail]>
Signed-off-by: Danilo Krummrich <[e-mail]>
---
 rust/kernel/lib.rs |   1 +
 rust/kernel/pci.rs | 123 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 123 insertions(+), 1 deletion(-)

Comments

Miguel Ojeda May 20, 2024, 11:27 p.m. UTC | #1

Hi Philipp,

A few quick nits I noticed for this one...

On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <[e-mail]> wrote:
>
> +/// A PCI BAR to perform IO-Operations on.

Some more documentation, examples, and/or references would be nice.

> +pub struct Bar {
> +    pdev: Device,
> +    iomem: IoMem,
> +    num: u8,
> +}
> +
> +impl Bar {
> +    fn new(pdev: Device, num: u8, name: &CStr) -> Result<Self> {
> +        let barnr = num as i32;

Would it make sense to use newtypes for `num`/`barnr`, perhaps?

> +        let barlen = pdev.resource_len(num)?;
> +        if barlen == 0 {
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY:
> +        // `pdev` is always valid.

Please explain why it is always valid -- the point of a `SAFETY`
comment is not to say something is OK, but why it is so.

> +        // `barnr` is checked for validity at the top of the function.

Where was it checked? I may be missing something, but I only see a
widening cast.

> +        // SAFETY:
> +        // `pdev` is always valid.
> +        // `barnr` is checked for validity at the top of the function.
> +        // `name` is always valid.

Please use the convention we have elsewhere for this kind of lists,
i.e. use a list with the `-` bullet list marker.

> +        let ioptr: usize = unsafe { bindings::pci_iomap(pdev.as_raw(), barnr, 0) } as usize;

Is the type needed, since there is an explicit cast?

> +        if ioptr == 0 {
> +            // SAFETY:
> +            // `pdev` is always valid.
> +            // `barnr` is checked for validity at the top of the function.
> +            unsafe { bindings::pci_release_region(pdev.as_raw(), barnr) };
> +            return Err(ENOMEM);
> +        }

Should the region be managed in a RAII type instead?

> +    fn index_is_valid(i: u8) -> bool {
> +        // A pci_dev on the C side owns an array of resources with at most
> +        // PCI_NUM_RESOURCES entries.

Missing Markdown. There are other instances as well.

> +        if i as i32 >= bindings::PCI_NUM_RESOURCES as i32 {
> +            return false;
> +        }
> +
> +        true

The body of the function could just be `... < ...`, i.e. no `if` needed.

> +    // SAFETY: The caller should ensure that `ioptr` is valid.
> +    unsafe fn do_release(pdev: &Device, ioptr: usize, num: u8) {

This should not be a comment but documentation, and it should be a `#
Safety` section, not a `// SAFETY:` comment.

> +    /// Returns the size of the given PCI bar resource.
> +    pub fn resource_len(&self, bar: u8) -> Result<bindings::resource_size_t> {

Sometimes `bindings::` in signatures of public methods may be
justified -- is it the case here? Or should a newtype or similar be
provided instead? I only see this function being called inside the
module, anyway.

> +    /// Mapps an entire PCI-BAR after performing a region-request on it.

Typo.

Cheers,
Miguel

Danilo Krummrich May 21, 2024, 11:22 a.m. UTC | #2

On 5/21/24 01:27, Miguel Ojeda wrote:
> On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <[e-mail]> wrote:
>> +        let barlen = pdev.resource_len(num)?;
>> +        if barlen == 0 {
>> +            return Err(ENOMEM);
>> +        }
>> +
>> +        // SAFETY:
>> +        // `pdev` is always valid.
> 
> Please explain why it is always valid -- the point of a `SAFETY`
> comment is not to say something is OK, but why it is so.
> 
>> +        // `barnr` is checked for validity at the top of the function.

I added pci::Device::resource_len(), since I needed to get the VRAM bar size in Nova.

pci::Device::resource_len() also needs to check for a valid  bar index and is used
above, hence the check is implicit. I just forgot to change this comment accordingly.

>> +    /// Returns the size of the given PCI bar resource.
>> +    pub fn resource_len(&self, bar: u8) -> Result<bindings::resource_size_t> {
> 
> Sometimes `bindings::` in signatures of public methods may be
> justified -- is it the case here? Or should a newtype or similar be
> provided instead? I only see this function being called inside the
> module, anyway.

I agree, I just added this function real quick to let Nova report the VRAM bar size
and forgot to clean this up.

> 
>> +    /// Mapps an entire PCI-BAR after performing a region-request on it.
> 
> Typo.
> 
> Cheers,
> Miguel
>

Patch

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 606391cbff83..15730deca822 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -54,6 +54,7 @@ 
 
 #[doc(hidden)]
 pub use bindings;
+mod iomem;
 pub use macros;
 #[cfg(all(CONFIG_PCI, CONFIG_PCI_MSI))]
 pub mod pci;
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 323aea565d84..403a1f53eb25 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -5,12 +5,17 @@ 
 //! C header: [`include/linux/pci.h`](../../../../include/linux/pci.h)
 
 use crate::{
-    bindings, container_of, device, driver,
+    alloc::flags::*,
+    bindings, container_of, device,
+    devres::Devres,
+    driver,
     error::{to_result, Result},
+    iomem::IoMem,
     str::CStr,
     types::{ARef, ForeignOwnable},
     ThisModule,
 };
+use core::ops::Deref;
 use kernel::prelude::*; // for pinned_drop
 
 /// An adapter for the registration of PCI drivers.
@@ -287,6 +292,104 @@  pub trait Driver {
 #[derive(Clone)]
 pub struct Device(ARef<device::Device>);
 
+/// A PCI BAR to perform IO-Operations on.
+pub struct Bar {
+    pdev: Device,
+    iomem: IoMem,
+    num: u8,
+}
+
+impl Bar {
+    fn new(pdev: Device, num: u8, name: &CStr) -> Result<Self> {
+        let barnr = num as i32;
+
+        let barlen = pdev.resource_len(num)?;
+        if barlen == 0 {
+            return Err(ENOMEM);
+        }
+
+        // SAFETY:
+        // `pdev` is always valid.
+        // `barnr` is checked for validity at the top of the function.
+        // `name` is always valid.
+        let ret = unsafe { bindings::pci_request_region(pdev.as_raw(), barnr, name.as_char_ptr()) };
+        if ret != 0 {
+            return Err(EBUSY);
+        }
+
+        // SAFETY:
+        // `pdev` is always valid.
+        // `barnr` is checked for validity at the top of the function.
+        // `name` is always valid.
+        let ioptr: usize = unsafe { bindings::pci_iomap(pdev.as_raw(), barnr, 0) } as usize;
+        if ioptr == 0 {
+            // SAFETY:
+            // `pdev` is always valid.
+            // `barnr` is checked for validity at the top of the function.
+            unsafe { bindings::pci_release_region(pdev.as_raw(), barnr) };
+            return Err(ENOMEM);
+        }
+
+        let iomem = match IoMem::new(ioptr, barlen as usize) {
+            Ok(iomem) => iomem,
+            Err(err) => {
+                // SAFETY:
+                // `pdev` is always valid.
+                // `ioptr` was created above, and `num` was checked at the top of the function.
+                unsafe { Self::do_release(&pdev, ioptr, num) };
+                return Err(err);
+            }
+        };
+
+        Ok(Bar { pdev, iomem, num })
+    }
+
+    fn index_is_valid(i: u8) -> bool {
+        // A pci_dev on the C side owns an array of resources with at most
+        // PCI_NUM_RESOURCES entries.
+        if i as i32 >= bindings::PCI_NUM_RESOURCES as i32 {
+            return false;
+        }
+
+        true
+    }
+
+    // SAFETY: The caller should ensure that `ioptr` is valid.
+    unsafe fn do_release(pdev: &Device, ioptr: usize, num: u8) {
+        // SAFETY:
+        // `pdev` is Rust data and guaranteed to be valid.
+        // A valid `ioptr` should be provided by the caller, but an invalid one
+        // does not cause faults on the C side.
+        // `num` is checked for validity above.
+        unsafe {
+            bindings::pci_iounmap(pdev.as_raw(), ioptr as _);
+            bindings::pci_release_region(pdev.as_raw(), num as i32);
+        }
+    }
+
+    fn release(&self) {
+        // SAFETY:
+        // Safe because `self` always contains a refcounted device that belongs
+        // to a pci::Device.
+        // `ioptr` and `num` are always valid because the Bar was created successfully.
+        unsafe { Self::do_release(&self.pdev, self.iomem.ioptr, self.num) };
+    }
+}
+
+impl Drop for Bar {
+    fn drop(&mut self) {
+        self.release();
+    }
+}
+
+impl Deref for Bar {
+    type Target = IoMem;
+
+    fn deref(&self) -> &Self::Target {
+        &self.iomem
+    }
+}
+
 impl Device {
     /// Create a PCI Device instance from an existing `device::Device`.
     ///
@@ -319,6 +422,24 @@  pub fn set_master(&self) {
         // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
         unsafe { bindings::pci_set_master(self.as_raw()) };
     }
+
+    /// Returns the size of the given PCI bar resource.
+    pub fn resource_len(&self, bar: u8) -> Result<bindings::resource_size_t> {
+        if !Bar::index_is_valid(bar) {
+            return Err(EINVAL);
+        }
+
+        // SAFETY: Safe as by the type invariant.
+        Ok(unsafe { bindings::pci_resource_len(self.as_raw(), bar.into()) })
+    }
+
+    /// Mapps an entire PCI-BAR after performing a region-request on it.
+    pub fn iomap_region(&mut self, barnr: u8, name: &CStr) -> Result<Devres<Bar>> {
+        let bar = Bar::new(self.clone(), barnr, name)?;
+        let devres = Devres::new(self.0.clone(), bar, GFP_KERNEL)?;
+
+        Ok(devres)
+    }
 }
 
 impl AsRef<device::Device> for Device {