Received-SPF: pass (google.com: domain of linux-kernel+bounces-185330-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 21 May 2024 20:36:36 +0200
From: Danilo Krummrich <dakr@redhat.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>,
	Philipp Stanner <pstanner@redhat.com>, wedsonaf@gmail.com
Cc: gregkh@linuxfoundation.org, rafael@kernel.org, bhelgaas@google.com,
	ojeda@kernel.org, alex.gaynor@gmail.com, boqun.feng@gmail.com,
	gary@garyguo.net, bjorn3_gh@protonmail.com, benno.lossin@proton.me,
	a.hindborg@samsung.com, aliceryhl@google.com, airlied@gmail.com,
	fujita.tomonori@gmail.com, lina@asahilina.net, ajanulgu@redhat.com,
	lyude@redhat.com, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [RFC PATCH 10/11] rust: add basic abstractions for iomem
 operations
Message-ID: <ZkzptG6fJx-MJ_6s@pollux>
References: <20240520172554.182094-1-dakr@redhat.com>
 <20240520172554.182094-11-dakr@redhat.com>
 <CANiq72kHrgOVrdw7rB9KpHvOMy244TgmEzAcL=v5O9rchs8T1g@mail.gmail.com>
 <cf89c02d45545b67272aba933fbc8a8a0df83358.camel@redhat.com>
 <CANiq72k7H3Y0ksdquVsrAbRtj_5CqMCYfo79UrhSVcK5VwfG5Q@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANiq72k7H3Y0ksdquVsrAbRtj_5CqMCYfo79UrhSVcK5VwfG5Q@mail.gmail.com>

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