Received-SPF: pass (google.com: domain of linux-kernel+bounces-184404-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: Danilo Krummrich <dakr@redhat.com>
Precedence: bulk
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC PATCH 10/11] rust: add basic abstractions for iomem operations
Date: Tue, 21 May 2024 04:59:08 +0200
Message-Id: <CE78A4CB-F499-4BD8-8E29-28F561B320C1@redhat.com>
References: <CANiq72kHrgOVrdw7rB9KpHvOMy244TgmEzAcL=v5O9rchs8T1g@mail.gmail.com>
Cc: gregkh@linuxfoundation.org, rafael@kernel.org, bhelgaas@google.com,
 ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@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,
 pstanner@redhat.com, ajanulgu@redhat.com, lyude@redhat.com,
 rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-pci@vger.kernel.org
In-Reply-To: <CANiq72kHrgOVrdw7rB9KpHvOMy244TgmEzAcL=v5O9rchs8T1g@mail.gmail.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

(quick reply from my phone)

> On 21. May 2024, at 00:32, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> w=
rote:
>=20
> =EF=BB=BFOn Mon, May 20, 2024 at 7:27=E2=80=AFPM Danilo Krummrich <dakr@re=
dhat.com> wrote:
>>=20
>> through its Drop() implementation.
>=20
> 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.
>=20
>> +/// IO-mapped memory, starting at the base pointer @ioptr and spanning @=
malxen bytes.
>=20
> 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.
>=20
>> +pub struct IoMem {
>> +    pub ioptr: usize,
>=20
> This field is public, which raises some questions...
>=20
>> +    pub fn readb(&self, offset: usize) -> Result<u8> {
>> +        let ioptr: usize =3D self.get_io_addr(offset, 1)?;
>> +
>> +        Ok(unsafe { bindings::readb(ioptr as _) })
>> +    }
>=20
> 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`.
>=20

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. Hen=
ce I think we should just make IoMem a trait instead and just let the resour=
ce 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.
>=20
> Also, there are missing `// SAFETY:` comments here. Documentation and
> examples would also be nice to have.
>=20
> Thanks!
>=20
> Cheers,
> Miguel
>=20