Message-ID: <a3005d2871d6571a436dacca2d93eb10cca54bed.camel@linux.ibm.com>
Date:   Tue, 06 Apr 2021 13:14:11 +0200
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     Bixuan Cui <cuibixuan@...wei.com>, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org
Cc:     gor@...ux.ibm.com, borntraeger@...ibm.com, john.wanghui@...wei.com
Subject: Re: [PATCH] s390/pci: move
 ioremap/ioremap_prot/ioremap_wc/ioremap_wt/iounmap to arch/s390/mm/ioremap.c

On Thu, 2021-04-01 at 20:46 +0800, Bixuan Cui wrote:
> The ioremap/iounmap is implemented in arch/s390/pci/pci.c.
> While CONFIG_PCI is disabled,the compilation error is reported:
>     s390x-linux-gnu-ld: drivers/pcmcia/cistpl.o: in function `set_cis_map':
>     cistpl.c:(.text+0x32a): undefined reference to `ioremap'
>     s390x-linux-gnu-ld: cistpl.c:(.text+0x360): undefined reference to `iounmap'
>     s390x-linux-gnu-ld: cistpl.c:(.text+0x384): undefined reference to `iounmap'
>     s390x-linux-gnu-ld: cistpl.c:(.text+0x396): undefined reference to `ioremap'
>     s390x-linux-gnu-ld: drivers/pcmcia/cistpl.o: in function `release_cis_mem':
>     cistpl.c:(.text+0xcb8): undefined reference to `iounmap'

Hmm, so I guess we're the only architecture which only defines
ioremap() if CONFIG_PCI is set and on top of that ioremap() only
actually remaps if we have a runtime detected hardware feature (MIO
support) and otherwise definitely only works for PCI BARs while in
theory it could also remap other physical memory with, though without
other (pseudo-)MMIO memory that's a bit pointless.

There doesn't seem to be a HAVE_IOREMAP config flag, only
HAVE_IOREMAP_PROT precicely because everyone else with an MMU probably
also uses ioremap(). Anything else that driver's that don't require PCI
but do require ioremap() could depend on?

> 
> Add arch/s390/mm/ioremap.c file and move ioremap/ioremap_wc/ioremap_rt/iounmap
> to it to fix the error.

See below but this patch code doesn't just move code around.

> 
> Reported-by: kernel test robot <lkp@...el.com>
> Signed-off-by: Bixuan Cui <cuibixuan@...wei.com>
> ---
>  arch/s390/include/asm/io.h |  8 ++---
>  arch/s390/mm/Makefile      |  2 +-
>  arch/s390/mm/ioremap.c     | 64 +++++++++++++++++++++++++++++++++
>  arch/s390/pci/pci.c        | 73 ++++++--------------------------------
>  4 files changed, 80 insertions(+), 67 deletions(-)
>  create mode 100644 arch/s390/mm/ioremap.c
> 
> diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
> index e3882b012bfa..48a55644c34f 100644
> --- a/arch/s390/include/asm/io.h
> +++ b/arch/s390/include/asm/io.h
> @@ -22,6 +22,10 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
>  
>  #define IO_SPACE_LIMIT 0
>  
> +#define ioremap ioremap
> +#define ioremap_wt ioremap_wt
> +#define ioremap_wc ioremap_wc
> +
>  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);
> @@ -51,10 +55,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/mm/Makefile b/arch/s390/mm/Makefile
> index cd67e94c16aa..74c22dfb131b 100644
> --- a/arch/s390/mm/Makefile
> +++ b/arch/s390/mm/Makefile
> @@ -4,7 +4,7 @@
>  #
>  
>  obj-y		:= init.o fault.o extmem.o mmap.o vmem.o maccess.o
> -obj-y		+= page-states.o pageattr.o pgtable.o pgalloc.o
> +obj-y		+= page-states.o pageattr.o pgtable.o pgalloc.o ioremap.o
>  
>  obj-$(CONFIG_CMM)		+= cmm.o
>  obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
> diff --git a/arch/s390/mm/ioremap.c b/arch/s390/mm/ioremap.c
> new file mode 100644
> index 000000000000..132e6ddff36f
> --- /dev/null
> +++ b/arch/s390/mm/ioremap.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Huawei Ltd.
> + * Author: Bixuan Cui <cuibixuan@...wei.com>
> + */
> +#include <linux/vmalloc.h>
> +#include <linux/io.h>
> +#include <linux/mm.h>
> +#include <linux/jump_label.h>
> +
> +static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t 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;
> +
> +	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));
> +}
> +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)
> +{
> +	vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
> +}
> +EXPORT_SYMBOL(iounmap);
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index dd14641b2d20..be300850df9c 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -227,65 +227,6 @@ 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)
> -{
> -	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;
> -
> -	if (!static_branch_unlikely(&have_mio))
> -		return (void __iomem *) addr;

Please don't make your commit message sound as if the commit only moves
code to a new file when your move instead fundamentally alters that
code by e.g. dropping the above static_branch out of ioremap() which
in
fact would completely break our PCI support on machines without the MIO
hardware feature this checks.

This code might look stupid to you but is actually our long standing
original ioremap() implementation dating back to our initial PCI
support in commit cd24834130ac ("s390/pci: base support").

This is because we need to work around the fact that s390 does not
currently have true MMIO support, that MIO flag we're checking on here
gives us pseudo-MMIO memory that works conceptually like MMIO but only
allows special PCI load/stores (on translated addresses). Without that
we can only access the PCI bus and PCI devices through special PCI
access instructions which do not use MMIO addresses. Instead they rely
on what we call a function handle, a BAR number and an offset. Since
Linux only knows how to work with PCI based on MMIO we adapt to that
interface by issuing artificial BAR/__iomem addresses which allow our
readX/writeX primities to map them back to the special PCI access
instructions. Since these aren't actuallyt physical addresses and can
only be used with these primitives remapping makes little sense.
Nevertheless PCI drivers expect to be able to call ioremap() on the BAR
physical addresses and get a usable __iomem address back.

I guess one way to fix this would be to turn the above if into:

if (IS_ENABLED(CONFIG_PCI) && !static_branch_unlikely(&have_mio))

and move the have_mio variable out of the PCI only code or use a raw
"#ifdef CONFIG_PCI". Obviously we don't have any actual users of
ioremap() that don't depend on CONFIG_PCI but it would make it so that
ioremap() exists and should actually function without CONFIG_PCI.
The weird part though is that for anyone using it without CONFIG_PCI it
would stop working if that is set and the machine doesn't have MIO
support but would work if it does.


> -
> -	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));
> -}
> -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));
> -}
> -EXPORT_SYMBOL(iounmap);
> -
>  /* Create a virtual mapping cookie for a PCI BAR */
>  static void __iomem *pci_iomap_range_fh(struct pci_dev *pdev, int bar,
>  					unsigned long offset, unsigned long max)
> @@ -312,7 +253,10 @@ static void __iomem *pci_iomap_range_mio(struct pci_dev *pdev, int bar,
>  	struct zpci_dev *zdev = to_zpci(pdev);
>  	void __iomem *iova;
>  
> -	iova = ioremap((unsigned long) zdev->bars[bar].mio_wt, barsize);
> +	if (!static_branch_unlikely(&have_mio))
> +		iova = (void __iomem *) zdev->bars[bar].mio_wt;
> +	else
> +		iova = ioremap((unsigned long) zdev->bars[bar].mio_wt, barsize);
>  	return iova ? iova + offset : iova;
>  }
>  
> @@ -342,7 +286,11 @@ static void __iomem *pci_iomap_wc_range_mio(struct pci_dev *pdev, int bar,
>  	struct zpci_dev *zdev = to_zpci(pdev);
>  	void __iomem *iova;
>  
> -	iova = ioremap((unsigned long) zdev->bars[bar].mio_wb, barsize);
> +	if (!static_branch_unlikely(&have_mio))
> +		iova = (void __iomem *) zdev->bars[bar].mio_wb;
> +	else
> +		iova = ioremap((unsigned long) zdev->bars[bar].mio_wb, barsize);
> +
>  	return iova ? iova + offset : iova;
>  }
>  
> @@ -381,7 +329,8 @@ static void pci_iounmap_fh(struct pci_dev *pdev, void __iomem *addr)
>  
>  static void pci_iounmap_mio(struct pci_dev *pdev, void __iomem *addr)
>  {
> -	iounmap(addr);
> +	if (static_branch_likely(&have_mio))
> +		iounmap(addr);
>  }
>  
>  void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)

Powered by blists - more mailing lists