diff mbox series

[RFC,10/11] rust: add basic abstractions for iomem operations

Message ID 20240520172554.182094-11-dakr@redhat.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Device / Driver and PCI Rust abstractions | expand

Commit Message

Danilo Krummrich May 20, 2024, 5:25 p.m. UTC
From: Philipp Stanner <pstanner@redhat.com>

Access to the kernel's IO-functions (e.g., readb()) is needed by almost
all drivers. Currently, there are no abstractions for those functions.

Since iomem is so widely used, it's necessary to provide a generic
interface that all subsystems providing IO memory can use. The existing
C implementations of such subsystems typically provide their own
wrappers around functions like ioremap() which take care of respecting
resource boundaries etc. It is, therefore, desirable to use these
wrappers because using ioremap() and iounmap() directly would
effectively result in parts of those subsystems being reimplemented in
Rust.

As most if not all device drivers should be fully satisfied regarding
their iomem demands by the existing subsystem interfaces, Rust
abstractions as congruent as possible with the existing infrastructure
shall use the existing subsystem (e.g., PCI) interfaces for creating
IO mappings, while simultaneously wrapping those mappings in Rust
containers whose Drop() traits ensure that the resources are released
again.

The process for mapping iomem would consequently look as follows:

  1. The subsystem abstraction (e.g., PCI) requests and ioremaps the
     memory through the corresponding C functions.
  2. The subsystem uses resources obtained in step #1 to create a Rust
     IoMem data structure that implements the IO functionality such as
     readb() for the iomem.
  3. The subsystem code wrapps IoMem into additional containers that
     ensure, e.g., thread safety, prevent UAF etc. Additionally, the
     subsystem ensures that access to IoMem is revoked latest when the
     driver's remove() callback is invoked.

Hereby, the subsystem data structure obtains ownership over the iomem.
Release of the iomem and, possibly, other subsystem associated data is
then handled through the Drop() trait of the subsystem data structure.

IO memory can become invalid during runtime (for example because the
driver's remove() callback was invoked, revoking access to the driver's
resources). However, in parallel executing routines might still be
active. Consequently, the subsytem should also guard the iomem in some
way.

One way to do this is the Devres implementation, which provides a
container that is capable of revoking access to its payload when
the driver's remove() callback is invoked.

The figure illustrates what usage of IoMem through subsystems might look
like:
               Devres
  *------------------------------*
  |                              |
  |   subsystem data structure   |
  |   *----------------------*   |
  |   |        IoMem         |   |
  |   | *------------------* |   |
  |   | |  io_addr = 0x42, | |   |
  |   | |  io_len = 9001,  | |   |
  |   | |                  | |   |
  |   | |  readb(),        | |   |
  |   | |  writeb(),       | |   |
  |   | |  ...             | |   |
  |   | *------------------* |   |
  |   |   deref(),           |   |
  |   |   drop(),            |   |
  |   |   ...                |   |
  |   *----------------------*   |
  |    deref(),                  |
  |    drop(),                   |
  *------------------------------*

For additional convenience, subsystem abstractions can implement the
Deref() trait for their data structures so that access he iomem can be
fully transparent.

In summary, IoMem would just serve as a container providing the IO
functions, and the subsystem, which knows about the memory layout,
request mechanisms etc, shall create and guard IoMem and ensure that its
resources are released latest at remove() (through Devres) or earlier
through its Drop() implementation.

Add 'IoMem', a struct holding an IO-Pointer and a length parameter,
which both shall be initialized validly by the subsystem.

Add Rust abstractions for basic IO memory operations on IoMem.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/helpers.c       | 106 +++++++++++++++++++++++++++++++++
 rust/kernel/iomem.rs | 135 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 241 insertions(+)
 create mode 100644 rust/kernel/iomem.rs

Comments

Miguel Ojeda May 20, 2024, 10:32 p.m. UTC | #1
On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> through its Drop() implementation.

Nit: `Drop`, `Deref` and so on are traits -- what do the `()` mean
here? I guess you may be referring to their method, but those are
lowercase.

> +/// IO-mapped memory, starting at the base pointer @ioptr and spanning @malxen bytes.

Please use Markdown code spans instead (and intra-doc links where
possible) -- we don't use the `@` notation. There is a typo on the
variable name too.

> +pub struct IoMem {
> +    pub ioptr: usize,

This field is public, which raises some questions...

> +    pub fn readb(&self, offset: usize) -> Result<u8> {
> +        let ioptr: usize = self.get_io_addr(offset, 1)?;
> +
> +        Ok(unsafe { bindings::readb(ioptr as _) })
> +    }

These methods are unsound, since `ioptr` may end up being anything
here, given `self.ioptr` it is controlled by the caller. One could
also trigger an overflow in `get_io_addr`.

Wedson wrote a similar abstraction in the past
(`rust/kernel/io_mem.rs` in the old `rust` branch), with a
compile-time `SIZE` -- it is probably worth taking a look.

Also, there are missing `// SAFETY:` comments here. Documentation and
examples would also be nice to have.

Thanks!

Cheers,
Miguel
Dave Airlie May 21, 2024, 2:07 a.m. UTC | #2
>
> Wedson wrote a similar abstraction in the past
> (`rust/kernel/io_mem.rs` in the old `rust` branch), with a
> compile-time `SIZE` -- it is probably worth taking a look.
>

Just on this point, we can't know in advance what size the IO BARs are
at compile time.

The old method just isn't useful for real devices with runtime IO BAR sizes.

Dave.
Danilo Krummrich May 21, 2024, 2:59 a.m. UTC | #3
(quick reply from my phone)

> On 21. May 2024, at 00:32, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@redhat.com> wrote:
>> 
>> through its Drop() implementation.
> 
> Nit: `Drop`, `Deref` and so on are traits -- what do the `()` mean
> here? I guess you may be referring to their method, but those are
> lowercase.
> 
>> +/// IO-mapped memory, starting at the base pointer @ioptr and spanning @malxen bytes.
> 
> Please use Markdown code spans instead (and intra-doc links where
> possible) -- we don't use the `@` notation. There is a typo on the
> variable name too.
> 
>> +pub struct IoMem {
>> +    pub ioptr: usize,
> 
> This field is public, which raises some questions...
> 
>> +    pub fn readb(&self, offset: usize) -> Result<u8> {
>> +        let ioptr: usize = self.get_io_addr(offset, 1)?;
>> +
>> +        Ok(unsafe { bindings::readb(ioptr as _) })
>> +    }
> 
> These methods are unsound, since `ioptr` may end up being anything
> here, given `self.ioptr` it is controlled by the caller. One could
> also trigger an overflow in `get_io_addr`.
> 

I think the only thing we really want from IoMem is a generic implementation of the read*() and write*() functions.

Everything else should be up the the resource itself, see e.g. pci::Bar. Hence I think we should just make IoMem a trait instead and just let the resource implement a getter for the MMIO pointer, etc.

> Wedson wrote a similar abstraction in the past
> (`rust/kernel/io_mem.rs` in the old `rust` branch), with a
> compile-time `SIZE` -- it is probably worth taking a look.
> 
> Also, there are missing `// SAFETY:` comments here. Documentation and
> examples would also be nice to have.
> 
> Thanks!
> 
> Cheers,
> Miguel
>
Wedson Almeida Filho May 21, 2024, 3:01 a.m. UTC | #4
On Mon, 20 May 2024 at 23:07, Dave Airlie <airlied@gmail.com> wrote:
>
> >
> > Wedson wrote a similar abstraction in the past
> > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a
> > compile-time `SIZE` -- it is probably worth taking a look.
> >
>
> Just on this point, we can't know in advance what size the IO BARs are
> at compile time.
>
> The old method just isn't useful for real devices with runtime IO BAR sizes.

The compile-time `SIZE` in my implementation is a minimum size.

Attempts to read/write with constants within that size (offset + size)
were checked at compile time, that is, they would have zero additional
runtime cost when compared to C. Reads/writes beyond the minimum would
have to be checked at runtime.
Philipp Stanner May 21, 2024, 7:36 a.m. UTC | #5
On Tue, 2024-05-21 at 00:32 +0200, Miguel Ojeda wrote:
> On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@redhat.com>
> wrote:
> > 
> > through its Drop() implementation.
> 
> Nit: `Drop`, `Deref` and so on are traits -- what do the `()` mean
> here? I guess you may be referring to their method, but those are
> lowercase.

ACK

> 
> > +/// IO-mapped memory, starting at the base pointer @ioptr and
> > spanning @malxen bytes.
> 
> Please use Markdown code spans instead (and intra-doc links where
> possible) -- we don't use the `@` notation. There is a typo on the
> variable name too.
> 
> > +pub struct IoMem {
> > +    pub ioptr: usize,
> 
> This field is public, which raises some questions...

Justified questions – it is public because the Drop implementation for
pci::Bar requires the ioptr to pass it to pci_iounmap().

The alternative would be to give pci::Bar a copy of ioptr (it's just an
integer after all), but that would also not be exactly beautiful.

The subsystem (as PCI does here) shall not make an instance of IoMem
mutable, so the driver programmer couldn't modify ioptr.

I'm very open for ideas for alternatives, though. See also the other
mail where Danilo brainstorms about making IoMem a trait.

> 
> > +    pub fn readb(&self, offset: usize) -> Result<u8> {
> > +        let ioptr: usize = self.get_io_addr(offset, 1)?;
> > +
> > +        Ok(unsafe { bindings::readb(ioptr as _) })
> > +    }
> 
> These methods are unsound, since `ioptr` may end up being anything
> here, given `self.ioptr` it is controlled by the caller. 

Only if IoMem is mutable, correct?

The commit message states (btw this file would get more extensive
comments soonish) that with this design its the subsystem that is
responsible for creating IoMem validly, because the subsystem is the
one who knows about the memory regions and lengths and stuff.

The driver should only ever take an IoMem through a subsystem, so that
would be safe.

> One could
> also trigger an overflow in `get_io_addr`.

Yes, if the addition violates the capacity of a usize. But that would
then be a bug we really want to notice, wouldn't we?

Only alternative I can think of would be to do a wrapping_add(), but
that would be even worse UB.

Ideas?

> 
> Wedson wrote a similar abstraction in the past
> (`rust/kernel/io_mem.rs` in the old `rust` branch), with a
> compile-time `SIZE` -- it is probably worth taking a look.

Yes, we're aware of that one. We also did some experiments with it.
Will discuss it in the other thread where Dave and Wedson mention it.

> 
> Also, there are missing `// SAFETY:` comments here. Documentation and
> examples would also be nice to have.

Oh yes, ACK, will do


Thx for the review!


> 
> Thanks!
> 
> Cheers,
> Miguel
>
Philipp Stanner May 21, 2024, 8:03 a.m. UTC | #6
On Tue, 2024-05-21 at 00:01 -0300, Wedson Almeida Filho wrote:
> On Mon, 20 May 2024 at 23:07, Dave Airlie <airlied@gmail.com> wrote:
> > 
> > > 
> > > Wedson wrote a similar abstraction in the past
> > > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a
> > > compile-time `SIZE` -- it is probably worth taking a look.
> > > 
> > 
> > Just on this point, we can't know in advance what size the IO BARs
> > are
> > at compile time.
> > 
> > The old method just isn't useful for real devices with runtime IO
> > BAR sizes.
> 
> The compile-time `SIZE` in my implementation is a minimum size.
> 
> Attempts to read/write with constants within that size (offset +
> size)
> were checked at compile time, that is, they would have zero
> additional
> runtime cost when compared to C. Reads/writes beyond the minimum
> would
> have to be checked at runtime.
> 

We looked at this implementation

Its disadvantage is that it moves the responsibility for setting that
minimum size to the driver programmer. Andreas Hindborg is using that
currently for rnvme [1].

I believe that the driver programmer in Rust should not be responsible
for controlling such sensitive parameters (one could far more easily
provide invalid values), but the subsystem (e.g. PCI) should do it,
because it knows about the exact resource lengths.

The only way to set the actual, real value is through subsystem code.
But when we (i.e., currently, the driver programmer) have to use that
anyways, we can just use it from the very beginning and have the exact
valid parameters.

So driver programmers would always have correct IoMem and would have a
harder time breaking stuff, and wouldn't have to think about minimum
lengths and things like that.


P.

[1] https://github.com/metaspace/linux/blob/rnvme/drivers/block/rnvme.rs#L580
Miguel Ojeda May 21, 2024, 9:18 a.m. UTC | #7
On Tue, May 21, 2024 at 9:36 AM Philipp Stanner <pstanner@redhat.com> wrote:
>
> Justified questions – it is public because the Drop implementation for
> pci::Bar requires the ioptr to pass it to pci_iounmap().
>
> The alternative would be to give pci::Bar a copy of ioptr (it's just an
> integer after all), but that would also not be exactly beautiful.

If by copy you mean keeping an actual copy elsewhere, then you could
provide an access method instead.

If you meant the access method, it may not be "beautiful" (Why? What
do you mean?), but it is way more important to be sound.

> The commit message states (btw this file would get more extensive
> comments soonish) that with this design its the subsystem that is
> responsible for creating IoMem validly, because the subsystem is the
> one who knows about the memory regions and lengths and stuff.
>
> The driver should only ever take an IoMem through a subsystem, so that
> would be safe.

The Rust safety boundary is normally the module -- you want that
subsystems cannot make mistakes either when using the `iomem` module,
not just drivers when using the subsystem APIs.

So you can't rely on a user, even if it is a subsystem, to "validly
create" objects and also hope that they do not modify the fields later
etc. If you need to ask subsystems for that, then you need to require
`unsafe` somewhere, e.g. the constructor (and make the field private,
and add an invariant to the type, and add `INVARIANT:` comments).

Think about it this way: if we were to write all the code like that
(e.g. with all structs using public fields), then essentially we would
be back at C, since we would be trusting everybody not to touch what
they shouldn't, and not to put values that could later lead something
else into UB, and we would not even have the documentation/invariants
to verify those either, etc.

> Yes, if the addition violates the capacity of a usize. But that would
> then be a bug we really want to notice, wouldn't we?

Definitely, but what I wanted is that you consider whether it is
reasonable to have the panic possibility there, since it depends on a
value that others control. For instance, would it make sense to make
it a fallible operation instead? Should the panic be documented
otherwise? Could it be prevented somehow? etc.

Please check Wedson's `io_mem` in the old `rust` branch for some ideas too.

Cheers,
Miguel
Danilo Krummrich May 21, 2024, 6:36 p.m. UTC | #8
On Tue, May 21, 2024 at 11:18:04AM +0200, Miguel Ojeda wrote:
> On Tue, May 21, 2024 at 9:36 AM Philipp Stanner <pstanner@redhat.com> wrote:
> >
> > Justified questions – it is public because the Drop implementation for
> > pci::Bar requires the ioptr to pass it to pci_iounmap().
> >
> > The alternative would be to give pci::Bar a copy of ioptr (it's just an
> > integer after all), but that would also not be exactly beautiful.
> 
> If by copy you mean keeping an actual copy elsewhere, then you could
> provide an access method instead.

As mentioned earlier, given the context how we use IoMem, I think IoMem should
just be a trait. And given that, maybe we'd want to name this trait differently
then, something like `trait IoOps` maybe?

pub trait IoOps {
   // INVARIANT: The implementation must ensure that the returned value is
   // either an error code or a non-null and valid address suitable for  I/O
   // operations of the given offset and length.
   fn io_addr(&self, offset: usize, len: usize) -> Result<usize>;

   fn readb(&self, offset: usize) -> Result<u8> {
      let addr = self.io_addr(offset, 1)?;

      // SAFETY: `addr` is guaranteed to be valid as by the invariant required
      // by `io_addr`.
      Ok(unsafe { bindings::readb(addr as _) })
   }

   [...]
}

We can let the resource type (e.g. `pci::Bar`) track the base address and limit
instead and just let pci::Bar implement `IoMem::io_addr`.

As for the compile time size, this would be up the the actual resource then.
`pci::Bar` can't make use of this optimization, while others might be able to.

Does that sound reasonable?

- Danilo
Wedson Almeida Filho May 25, 2024, 7:24 p.m. UTC | #9
On Tue, 21 May 2024 at 05:03, Philipp Stanner <pstanner@redhat.com> wrote:
>
> On Tue, 2024-05-21 at 00:01 -0300, Wedson Almeida Filho wrote:
> > On Mon, 20 May 2024 at 23:07, Dave Airlie <airlied@gmail.com> wrote:
> > >
> > > >
> > > > Wedson wrote a similar abstraction in the past
> > > > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a
> > > > compile-time `SIZE` -- it is probably worth taking a look.
> > > >
> > >
> > > Just on this point, we can't know in advance what size the IO BARs
> > > are
> > > at compile time.
> > >
> > > The old method just isn't useful for real devices with runtime IO
> > > BAR sizes.
> >
> > The compile-time `SIZE` in my implementation is a minimum size.
> >
> > Attempts to read/write with constants within that size (offset +
> > size)
> > were checked at compile time, that is, they would have zero
> > additional
> > runtime cost when compared to C. Reads/writes beyond the minimum
> > would
> > have to be checked at runtime.
> >
>
> We looked at this implementation
>
> Its disadvantage is that it moves the responsibility for setting that
> minimum size to the driver programmer. Andreas Hindborg is using that
> currently for rnvme [1].
>
> I believe that the driver programmer in Rust should not be responsible
> for controlling such sensitive parameters (one could far more easily
> provide invalid values), but the subsystem (e.g. PCI) should do it,
> because it knows about the exact resource lengths.

There is no responsibility being moved. The bus is still that one that
knows about the resources attached to the device.

The driver, however, can say for example: I need at least 4 registers
of 32 bits starting at offset X, which results in a minimum size of X
+ 16. If at runtime a device compatible with this driver appears and
has an io mem of at least that size, then the driver can drive it
without any additional runtime checks. I did this in the gpio driver
here: https://lwn.net/Articles/863459/

Note that in addition to not having to check offset at runtime, the
reads/writes are also infallible because all failures are caught at
compile time.

Obviously not all drivers can benefit from this, but it is
considerable simplification for the ones that can.

> The only way to set the actual, real value is through subsystem code.
> But when we (i.e., currently, the driver programmer) have to use that
> anyways, we can just use it from the very beginning and have the exact
> valid parameters.

Yes, only the bus knows. But to reiterate: if the driver declares and
checks a minimum size at attach time, it obviates the needs to check
again throughout the lifetime of the driver, which is more performant
and eliminates error paths.
diff mbox series

Patch

diff --git a/rust/helpers.c b/rust/helpers.c
index c3d80301185c..dc2405772b1a 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -34,6 +34,7 @@ 
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/pci.h>
+#include <linux/io.h>
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -179,6 +180,111 @@  int rust_helper_devm_add_action(struct device *dev, void (*action)(void *), void
 	return devm_add_action(dev, action, data);
 }
 
+/* io.h */
+u8 rust_helper_readb(const volatile void __iomem *addr)
+{
+	return readb(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readb);
+
+u16 rust_helper_readw(const volatile void __iomem *addr)
+{
+	return readw(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readw);
+
+u32 rust_helper_readl(const volatile void __iomem *addr)
+{
+	return readl(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readl);
+
+#ifdef CONFIG_64BIT
+u64 rust_helper_readq(const volatile void __iomem *addr)
+{
+	return readq(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readq);
+#endif
+
+void rust_helper_writeb(u8 value, volatile void __iomem *addr)
+{
+	writeb(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writeb);
+
+void rust_helper_writew(u16 value, volatile void __iomem *addr)
+{
+	writew(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writew);
+
+void rust_helper_writel(u32 value, volatile void __iomem *addr)
+{
+	writel(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writel);
+
+#ifdef CONFIG_64BIT
+void rust_helper_writeq(u64 value, volatile void __iomem *addr)
+{
+	writeq(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writeq);
+#endif
+
+u8 rust_helper_readb_relaxed(const volatile void __iomem *addr)
+{
+	return readb_relaxed(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readb_relaxed);
+
+u16 rust_helper_readw_relaxed(const volatile void __iomem *addr)
+{
+	return readw_relaxed(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readw_relaxed);
+
+u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
+{
+	return readl_relaxed(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readl_relaxed);
+
+#ifdef CONFIG_64BIT
+u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
+{
+	return readq_relaxed(addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_readq_relaxed);
+#endif
+
+void rust_helper_writeb_relaxed(u8 value, volatile void __iomem *addr)
+{
+	writeb_relaxed(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writeb_relaxed);
+
+void rust_helper_writew_relaxed(u16 value, volatile void __iomem *addr)
+{
+	writew_relaxed(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writew_relaxed);
+
+void rust_helper_writel_relaxed(u32 value, volatile void __iomem *addr)
+{
+	writel_relaxed(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writel_relaxed);
+
+#ifdef CONFIG_64BIT
+void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
+{
+	writeq_relaxed(value, addr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_writeq_relaxed);
+#endif
+
 void rust_helper_pci_set_drvdata(struct pci_dev *pdev, void *data)
 {
 	pci_set_drvdata(pdev, data);
diff --git a/rust/kernel/iomem.rs b/rust/kernel/iomem.rs
new file mode 100644
index 000000000000..efb6cd0829b4
--- /dev/null
+++ b/rust/kernel/iomem.rs
@@ -0,0 +1,135 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+use crate::bindings;
+use crate::error::{code::EINVAL, Result};
+
+/// IO-mapped memory, starting at the base pointer @ioptr and spanning @malxen bytes.
+///
+/// The creator (usually a subsystem such as PCI) is responsible for creating the
+/// mapping, performing an additional region request etc.
+pub struct IoMem {
+    pub ioptr: usize,
+    maxlen: usize,
+}
+
+impl IoMem {
+    pub(crate) fn new(ioptr: usize, maxlen: usize) -> Result<Self> {
+        if ioptr == 0 || maxlen == 0 {
+            return Err(EINVAL);
+        }
+
+        Ok(Self { ioptr, maxlen })
+    }
+
+    fn get_io_addr(&self, offset: usize, len: usize) -> Result<usize> {
+        if offset + len > self.maxlen {
+            return Err(EINVAL);
+        }
+
+        Ok(self.ioptr + offset)
+    }
+
+    pub fn readb(&self, offset: usize) -> Result<u8> {
+        let ioptr: usize = self.get_io_addr(offset, 1)?;
+
+        Ok(unsafe { bindings::readb(ioptr as _) })
+    }
+
+    pub fn readw(&self, offset: usize) -> Result<u16> {
+        let ioptr: usize = self.get_io_addr(offset, 2)?;
+
+        Ok(unsafe { bindings::readw(ioptr as _) })
+    }
+
+    pub fn readl(&self, offset: usize) -> Result<u32> {
+        let ioptr: usize = self.get_io_addr(offset, 4)?;
+
+        Ok(unsafe { bindings::readl(ioptr as _) })
+    }
+
+    pub fn readq(&self, offset: usize) -> Result<u64> {
+        let ioptr: usize = self.get_io_addr(offset, 8)?;
+
+        Ok(unsafe { bindings::readq(ioptr as _) })
+    }
+
+    pub fn readb_relaxed(&self, offset: usize) -> Result<u8> {
+        let ioptr: usize = self.get_io_addr(offset, 1)?;
+
+        Ok(unsafe { bindings::readb_relaxed(ioptr as _) })
+    }
+
+    pub fn readw_relaxed(&self, offset: usize) -> Result<u16> {
+        let ioptr: usize = self.get_io_addr(offset, 2)?;
+
+        Ok(unsafe { bindings::readw_relaxed(ioptr as _) })
+    }
+
+    pub fn readl_relaxed(&self, offset: usize) -> Result<u32> {
+        let ioptr: usize = self.get_io_addr(offset, 4)?;
+
+        Ok(unsafe { bindings::readl_relaxed(ioptr as _) })
+    }
+
+    pub fn readq_relaxed(&self, offset: usize) -> Result<u64> {
+        let ioptr: usize = self.get_io_addr(offset, 8)?;
+
+        Ok(unsafe { bindings::readq_relaxed(ioptr as _) })
+    }
+
+    pub fn writeb(&self, byte: u8, offset: usize) -> Result {
+        let ioptr: usize = self.get_io_addr(offset, 1)?;
+
+        unsafe { bindings::writeb(byte, ioptr as _) }
+        Ok(())
+    }
+
+    pub fn writew(&self, word: u16, offset: usize) -> Result {
+        let ioptr: usize = self.get_io_addr(offset, 2)?;
+
+        unsafe { bindings::writew(word, ioptr as _) }
+        Ok(())
+    }
+
+    pub fn writel(&self, lword: u32, offset: usize) -> Result {
+        let ioptr: usize = self.get_io_addr(offset, 4)?;
+
+        unsafe { bindings::writel(lword, ioptr as _) }
+        Ok(())
+    }
+
+    pub fn writeq(&self, qword: u64, offset: usize) -> Result {
+        let ioptr: usize = self.get_io_addr(offset, 8)?;
+
+        unsafe { bindings::writeq(qword, ioptr as _) }
+        Ok(())
+    }
+
+    pub fn writeb_relaxed(&self, byte: u8, offset: usize) -> Result {
+        let ioptr: usize = self.get_io_addr(offset, 1)?;
+
+        unsafe { bindings::writeb_relaxed(byte, ioptr as _) }
+        Ok(())
+    }
+
+    pub fn writew_relaxed(&self, word: u16, offset: usize) -> Result {
+        let ioptr: usize = self.get_io_addr(offset, 2)?;
+
+        unsafe { bindings::writew_relaxed(word, ioptr as _) }
+        Ok(())
+    }
+
+    pub fn writel_relaxed(&self, lword: u32, offset: usize) -> Result {
+        let ioptr: usize = self.get_io_addr(offset, 4)?;
+
+        unsafe { bindings::writel_relaxed(lword, ioptr as _) }
+        Ok(())
+    }
+
+    pub fn writeq_relaxed(&self, qword: u64, offset: usize) -> Result {
+        let ioptr: usize = self.get_io_addr(offset, 8)?;
+
+        unsafe { bindings::writeq_relaxed(qword, ioptr as _) }
+        Ok(())
+    }
+}