On Fri, Jul 30, 2021 at 06:58:30PM +0200, Arnd Bergmann wrote:

On Fri, Jul 30, 2021 at 6:43 PM Boqun Feng [off-list ref] wrote:

quoted

Hi,

The background is that I'm reviewing Wedson's PR on IoMem for
Rust-for-Linux project:

        https://github.com/Rust-for-Linux/linux/pull/462

readX() and writeX() are used to provide Rust code to read/write IO
memory. And I want to find whether we need to check the alignment of the
pointer. I wonder whether the addresses passed to readX() and writeX()
need to be aligned to the size of the accesses (e.g. the parameter of
readl() has to be a 4-byte aligned pointer).

The only related information I get so far is the following quote in
Documentation/driver-io/device-io.rst:

        On many platforms, I/O accesses must be aligned with respect to
        the access size; failure to do so will result in an exception or
        unpredictable results.

Does it mean all readX() and writeX() need to use aligned addresses?
Or the alignment requirement is arch-dependent, i.e. if the architecture
supports and has enabled misalignment load and store, no alignment
requirement on readX() and writeX(), otherwise still need to use aligned
addresses.

I know different archs have their own alignment requirement on memory
accesses, just want to make sure the requirement of the readX() and
writeX() APIs.

I am not aware of any driver that requires unaligned access on __iomem
pointers, and since it definitely doesn't work on most architectures, I think
having an unconditional alignment check makes sense.

What would the alignment check look like? Is there a way to annotate
a pointer that is 'void __iomem *' in C as having a minimum alignment
when it gets passed into a function that uses readl()/writel() on it?

If we want to check, I'd expect we do the checks inside
readX()/writeX(), for example, readl() could be implemented as:

	#define readl(c) 					\
	({							\
		u32 __v;					\
								\
		/* alignment checking */			\
		BUG_ON(c & (sizeof(__v) - 1));			\
		__v = readl_relaxed(c);				\
		__iormb(__v);					\
		__v;						\
	})

It's a runtime check, so if anyone hates it I can understand ;-)

Regards,
Boqun

       Arnd