Re: [PATCH] RISC-V: Fix PCI I/O port addressing for MMU-less configurations

["Arnd Bergmann" <arnd@arndb.de>]

On Fri, Apr 11, 2025, at 04:34, Liu Runrun wrote:
> This patch addresses the PCI I/O port address handling in RISC-V's
> port-mapped I/O emulation routines when the MMU is not enabled.
> The changes ensure that:

Do you have a system that requires this? I sent a patch the other
day to make PCI 'depends on MMU', based on how nothing today
uses it. Having a NOMMU system with PCI wounds rather silly,
so I hope we don't ever get that.

> 1. For non-MMU systems, the PCI I/O port addresses are properly
>    calculated in marcos inX and outX when PCI_IOBASE is not
>    defined, this avoids the null pointer calculating warning
>    from the compiler.

This is the wrong way around: the warning tells you that you
have failed to configure PCI_IOBASE for the particular hardware,
and that you actually get a NULL pointer dereference.

The solution is not to shut up the warning but making it
not a NULL pointer dereference!

Part of the issue is that historically the asm-generic/io.h
header includes an incorrect fallback of PCI_IOBASE when
the architecture does not provide the correct one. I think only
sparc still relies on that, so that fallback definition should
be moved into arch/sparc/include/asm/io_{32,64}.h instead.

> 2. In asm-generic/io.h, function ioport_map(), casting PCI_IOPORT
>    to type "long" firstly makes it could compute with variable addr
>    directly, which avoids the null pointer calculating warning when
>    PCI_IOPORT is a null pointer in some case.

I don't understand that sentence, please rephrase.

> The original implementation used `PCI_IOBASE + (addr)` for MMU-enabled
> systems, but failed to handle non-MMU cases correctly. This change adds
> conditional compilation guards (#ifdef CONFIG_MMU) to differentiate
> between MMU and non-MMU environments, providing consistent behavior
> for both scenarios.

This also looks wrong: what you are distinguishing here is systems
with (potentially) I/O port support and those that never have I/O
port support.

> diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
> index a0e51840b9db..d5181bb02c98 100644
> --- a/arch/riscv/include/asm/io.h
> +++ b/arch/riscv/include/asm/io.h
> @@ -101,9 +101,15 @@ __io_reads_ins(reads, u32, l, __io_br(), 
> __io_ar(addr))
>  __io_reads_ins(ins,  u8, b, __io_pbr(), __io_par(addr))
>  __io_reads_ins(ins, u16, w, __io_pbr(), __io_par(addr))
>  __io_reads_ins(ins, u32, l, __io_pbr(), __io_par(addr))
> +#ifdef CONFIG_MMU
>  #define insb(addr, buffer, count) __insb(PCI_IOBASE + (addr), buffer, 
> count)
>  #define insw(addr, buffer, count) __insw(PCI_IOBASE + (addr), buffer, 
> count)
>  #define insl(addr, buffer, count) __insl(PCI_IOBASE + (addr), buffer, 
> count)

I see that these are defined unconditionally here, which is probably
the real mistake. What I think we need instead is to enclose
them in "#ifdef CONFIG_HAS_IOPORT" with no "#else" block, and
ensure that HAS_IOPORT is only set when building for targets that
have a sensible definition of PCI_IOBASE and IO_SPACE_LIMIT.

The same approach is used in asm-generic/io.h, we just haven't
done it for all architectures yet, since it's not entirely
clear which ones can support ISA bridges and legacy PCI devices
with port I/O.

Ideally we'd go through the individual PCI host driver
implementations and only enable HAS_IOPORT for those that
are can handle the I/O space window correctly, but leave
it out if none of them are selected.

      Arnd