Re: [PATCH v4 09/16] s390: mm: Convert to GENERIC_IOREMAP

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Tue, 2023-02-21 at 19:48 +0800, Baoquan He wrote:
> On 02/16/23 at 05:21pm, Niklas Schnelle wrote:
> > On Thu, 2023-02-16 at 20:34 +0800, Baoquan He wrote:
> > > By taking GENERIC_IOREMAP method, the generic generic_ioremap_prot(),
> > > generic_iounmap(), and their generic wrapper ioremap_prot(), ioremap()
> > > and iounmap() are all visible and available to arch. Arch needs to
> > > provide wrapper functions to override the generic versions if there's
> > > arch specific handling in its ioremap_prot(), ioremap() or iounmap().
> > > This change will simplify implementation by removing duplicated codes
> > > with generic_ioremap_prot() and generic_iounmap(), and has the equivalent
> > > functioality as before.
> > > 
> > > Here, add wrapper functions ioremap_prot() and iounmap() for s390's
> > > special operation when ioremap() and iounmap().
> > > 
> > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > Cc: Niklas Schnelle <schnelle@linux.ibm.com>
> > > Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> > > Cc: Heiko Carstens <hca@linux.ibm.com>
> > > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > > Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> > > Cc: Sven Schnelle <svens@linux.ibm.com>
> > > Cc: linux-s390@vger.kernel.org
> > > ---
> > >  arch/s390/Kconfig          |  1 +
> > >  arch/s390/include/asm/io.h | 21 ++++++++------
> > >  arch/s390/pci/pci.c        | 57 +++++++-------------------------------
> > >  3 files changed, 23 insertions(+), 56 deletions(-)
> > 
> > Thanks for the patch. This is a very clear improvement for us! I tested
> > this series with s390 systems with and without the PCI memory-I/O (MIO)
> > support and everything works as expected.
> > 
> > One thing I did stumble upon but which is independent from this patch
> > is that I think memremap(…, MEMREMAP_WB) on system RAM outside the
> > direct map is broken for us. At least on systems without PCI memory-I/O
> > support. I think with this series it would be much easier to fix
> > though. Basically I think we would have to define arch_memremap_wb() to
> > generic_ioremap_prot(…, PAGE_KERNEL) and then have iounmap() check
> > is_ioremap_addr() to see if it is an actual mapping or an address
> > cookie. But again this is independent of this patch which doesn't
> > change the behavior in this area.
> 
> OK, I can check this after this patchset done.

Oh this wasn't meant as adding a task for you just a FYI. I'm already
experimenting with this and plan to possibly send a patch that fixes
the case of remapping system RAM once your series has landed. The thing
is that memremap() is quite tricky for us whichever way you look at
it. 

We don't have real MMIO on s390x and thus using memremap() for PCI MMIO
spaces doesn't really work as the void* returned can still only be
accessed using ioread()/iowrite() because only our special PCI access
instructions work on the pseudo-MMIO addresses (with PCI MIO)
respectively the address cookies. Obviously void* and memremap() really
isn't the right tool if you need to use ioread()/iowrite() to access it
and can't treat it as memory though.

Thanks,
Niklas