[v5,RESEND,10/17] s390: mm: Convert to GENERIC_IOREMAP

  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>
Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Tested-by: 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(-)
  

On Mon, May 15, 2023 at 05:08:41PM +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>
> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Tested-by: 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

Reviewed-by: Mike Rapoport (IBM) <rppt@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(-)
> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index db20c1589a98..f33923fa8c99 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -142,6 +142,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 afc3f33788da..d34d5813d006 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);
>  
> -- 
> 2.34.1
> 
>
  

On Mon, May 15, 2023 at 05:08:41PM +0800, Baoquan He wrote:
> +#define ioremap_wc(addr, size)  \
> +	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))

I'd move this out of line and just apply mio_wb_bit_mask directly
instead of the unbox/box/unbox/box dance.

> +#define ioremap_wt(addr, size)  \
> +	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))

and just define this to ioremap_wc.  Note that defining _wt to _wc is
very odd and seems wrong, but comes from the existing code.  Maybe the
s390 maintainers can chime on on the background and we can add a comment
while we're at it.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
  

On Tue, 2023-05-16 at 23:36 -0700, Christoph Hellwig wrote:
> On Mon, May 15, 2023 at 05:08:41PM +0800, Baoquan He wrote:
> > +#define ioremap_wc(addr, size)  \
> > +	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))
> 
> I'd move this out of line and just apply mio_wb_bit_mask directly
> instead of the unbox/box/unbox/box dance.
> 
> > +#define ioremap_wt(addr, size)  \
> > +	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))
> 
> and just define this to ioremap_wc.  Note that defining _wt to _wc is
> very odd and seems wrong, but comes from the existing code.  Maybe the
> s390 maintainers can chime on on the background and we can add a comment
> while we're at it.

I'm a bit confused where you see ioremap_wt() defined to ioremap_wc()
in the existing code? Our current definitions are:


void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
{
	return __ioremap(addr, size,
pgprot_writecombine(PAGE_KERNEL));
}

void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
{
	return __ioremap(addr, size,
pgprot_writethrough(PAGE_KERNEL));
}

Now if we don't have support for the enhanced PCI load/store
instructions (memory I/O aka MIO) then yes this gets ignored and both
.._wc() and .._wt() act the same but if we do have them
pgprot_writecombine() / pgprot_writethrough() set respectively clear 
the mio_wb bit in the PTE. It's a bit odd here because the exact
position of the bit is read from a firmware interface and could in
theory change but other than that it looks fine to me and yes I agree
that it would be odd and broken to define _wt to _wc.

Thanks,
Niklas
  

On Wed, May 17, 2023 at 09:58:26AM +0200, Niklas Schnelle wrote:
> > and just define this to ioremap_wc.  Note that defining _wt to _wc is
> > very odd and seems wrong, but comes from the existing code.  Maybe the
> > s390 maintainers can chime on on the background and we can add a comment
> > while we're at it.
> 
> I'm a bit confused where you see ioremap_wt() defined to ioremap_wc()
> in the existing code? Our current definitions are:

No, it's me wo is confused.  They clearly are different.  But the same
comment about just moving ioremap_wc applies to ioremap_wt based
on how pgprot_writethrough is implemented then.