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

Commit Message

Baoquan He Feb. 16, 2023, 12:34 p.m. UTC

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 <[e-mail]>
Cc: Niklas Schnelle <[e-mail]>
Cc: Gerald Schaefer <[e-mail]>
Cc: Heiko Carstens <[e-mail]>
Cc: Vasily Gorbik <[e-mail]>
Cc: Alexander Gordeev <[e-mail]>
Cc: Christian Borntraeger <[e-mail]>
Cc: Sven Schnelle <[e-mail]>
Cc: [e-mail]
---
 arch/s390/Kconfig          |  1 +
 arch/s390/include/asm/io.h | 21 ++++++++------
 arch/s390/pci/pci.c        | 57 +++++++-------------------------------
 3 files changed, 23 insertions(+), 56 deletions(-)

Comments

Niklas Schnelle Feb. 16, 2023, 4:21 p.m. UTC | #1

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 <[e-mail]>
> Cc: Niklas Schnelle <[e-mail]>
> Cc: Gerald Schaefer <[e-mail]>
> Cc: Heiko Carstens <[e-mail]>
> Cc: Vasily Gorbik <[e-mail]>
> Cc: Alexander Gordeev <[e-mail]>
> Cc: Christian Borntraeger <[e-mail]>
> Cc: Sven Schnelle <[e-mail]>
> Cc: [e-mail]
> ---
>  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.

So feel free to add my:

Reviewed-by: Niklas Schnelle <[e-mail]>
Tested-by: Niklas Schnelle <[e-mail]>

Baoquan He Feb. 21, 2023, 11:48 a.m. UTC | #2

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 <[e-mail]>
> > Cc: Niklas Schnelle <[e-mail]>
> > Cc: Gerald Schaefer <[e-mail]>
> > Cc: Heiko Carstens <[e-mail]>
> > Cc: Vasily Gorbik <[e-mail]>
> > Cc: Alexander Gordeev <[e-mail]>
> > Cc: Christian Borntraeger <[e-mail]>
> > Cc: Sven Schnelle <[e-mail]>
> > Cc: [e-mail]
> > ---
> >  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.

> 
> So feel free to add my:
> 
> Reviewed-by: Niklas Schnelle <[e-mail]>
> Tested-by: Niklas Schnelle <[e-mail]>

Thanks a lot, will add these when repost.

Niklas Schnelle Feb. 21, 2023, 12:26 p.m. UTC | #3

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 <[e-mail]>
> > > Cc: Niklas Schnelle <[e-mail]>
> > > Cc: Gerald Schaefer <[e-mail]>
> > > Cc: Heiko Carstens <[e-mail]>
> > > Cc: Vasily Gorbik <[e-mail]>
> > > Cc: Alexander Gordeev <[e-mail]>
> > > Cc: Christian Borntraeger <[e-mail]>
> > > Cc: Sven Schnelle <[e-mail]>
> > > Cc: [e-mail]
> > > ---
> > >  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

Baoquan He Feb. 21, 2023, 12:52 p.m. UTC | #4

On 02/21/23 at 01:26pm, Niklas Schnelle wrote:
> 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 <[e-mail]>
> > > > Cc: Niklas Schnelle <[e-mail]>
> > > > Cc: Gerald Schaefer <[e-mail]>
> > > > Cc: Heiko Carstens <[e-mail]>
> > > > Cc: Vasily Gorbik <[e-mail]>
> > > > Cc: Alexander Gordeev <[e-mail]>
> > > > Cc: Christian Borntraeger <[e-mail]>
> > > > Cc: Sven Schnelle <[e-mail]>
> > > > Cc: [e-mail]
> > > > ---
> > > >  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.

Ah, I misunderstood it. Thanks for sharing and expect to see your post.

Patch

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 7fd08755a1f9..be600b299328 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -141,6 +141,7 @@  config S390
 	select GENERIC_SMP_IDLE_THREAD
 	select GENERIC_TIME_VSYSCALL
 	select GENERIC_VDSO_TIME_NS
+	select GENERIC_IOREMAP if PCI
 	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_JUMP_LABEL
diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
index e3882b012bfa..4453ad7c11ac 100644
--- a/arch/s390/include/asm/io.h
+++ b/arch/s390/include/asm/io.h
@@ -22,11 +22,18 @@  void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
 
 #define IO_SPACE_LIMIT 0
 
-void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
-void __iomem *ioremap(phys_addr_t addr, size_t size);
-void __iomem *ioremap_wc(phys_addr_t addr, size_t size);
-void __iomem *ioremap_wt(phys_addr_t addr, size_t size);
-void iounmap(volatile void __iomem *addr);
+/*
+ * I/O memory mapping functions.
+ */
+#define ioremap_prot ioremap_prot
+#define iounmap iounmap
+
+#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
+
+#define ioremap_wc(addr, size)  \
+	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))
+#define ioremap_wt(addr, size)  \
+	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))
 
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
@@ -51,10 +58,6 @@  static inline void ioport_unmap(void __iomem *p)
 #define pci_iomap_wc pci_iomap_wc
 #define pci_iomap_wc_range pci_iomap_wc_range
 
-#define ioremap ioremap
-#define ioremap_wt ioremap_wt
-#define ioremap_wc ioremap_wc
-
 #define memcpy_fromio(dst, src, count)	zpci_memcpy_fromio(dst, src, count)
 #define memcpy_toio(dst, src, count)	zpci_memcpy_toio(dst, src, count)
 #define memset_io(dst, val, count)	zpci_memset_io(dst, val, count)
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index ef38b1514c77..9590bf2c0d88 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -244,62 +244,25 @@  void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
        zpci_memcpy_toio(to, from, count);
 }
 
-static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t prot)
+void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
+			   unsigned long prot)
 {
-	unsigned long offset, vaddr;
-	struct vm_struct *area;
-	phys_addr_t last_addr;
-
-	last_addr = addr + size - 1;
-	if (!size || last_addr < addr)
-		return NULL;
-
+	/*
+	 * When PCI MIO instructions are unavailable the "physical" address
+	 * encodes a hint for accessing the PCI memory space it represents.
+	 * Just pass it unchanged such that ioread/iowrite can decode it.
+	 */
 	if (!static_branch_unlikely(&have_mio))
-		return (void __iomem *) addr;
+		return (void __iomem *)phys_addr;
 
-	offset = addr & ~PAGE_MASK;
-	addr &= PAGE_MASK;
-	size = PAGE_ALIGN(size + offset);
-	area = get_vm_area(size, VM_IOREMAP);
-	if (!area)
-		return NULL;
-
-	vaddr = (unsigned long) area->addr;
-	if (ioremap_page_range(vaddr, vaddr + size, addr, prot)) {
-		free_vm_area(area);
-		return NULL;
-	}
-	return (void __iomem *) ((unsigned long) area->addr + offset);
-}
-
-void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
-{
-	return __ioremap(addr, size, __pgprot(prot));
+	return generic_ioremap_prot(phys_addr, size, __pgprot(prot));
 }
 EXPORT_SYMBOL(ioremap_prot);
 
-void __iomem *ioremap(phys_addr_t addr, size_t size)
-{
-	return __ioremap(addr, size, PAGE_KERNEL);
-}
-EXPORT_SYMBOL(ioremap);
-
-void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
-{
-	return __ioremap(addr, size, pgprot_writecombine(PAGE_KERNEL));
-}
-EXPORT_SYMBOL(ioremap_wc);
-
-void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
-{
-	return __ioremap(addr, size, pgprot_writethrough(PAGE_KERNEL));
-}
-EXPORT_SYMBOL(ioremap_wt);
-
 void iounmap(volatile void __iomem *addr)
 {
 	if (static_branch_likely(&have_mio))
-		vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
+		generic_iounmap(addr);
 }
 EXPORT_SYMBOL(iounmap);