| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Oct 2022 09:37:49 +0200
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Baoquan He <bhe@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
akpm@...ux-foundation.org, hch@...radead.org,
agordeev@...ux.ibm.com, wangkefeng.wang@...wei.com,
christophe.leroy@...roup.eu, David.Laight@...lab.com,
shorne@...il.com, Gerald Schaefer <gerald.schaefer@...ux.ibm.com>,
Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Sven Schnelle <svens@...ux.ibm.com>, linux-s390@...r.kernel.org
Subject: Re: [PATCH v3 09/11] s390: mm: Convert to GENERIC_IOREMAP
On Wed, 2022-10-12 at 13:52 +0800, Baoquan He wrote:
> On 10/11/22 at 05:16pm, Niklas Schnelle wrote:
> > On Sun, 2022-10-09 at 18:31 +0800, Baoquan He wrote:
> > > By taking GENERIC_IOREMAP method, the generic ioremap_prot() and
> > > iounmap() are visible and available to arch. Arch only needs to
> > > provide implementation of arch_ioremap() or arch_iounmap() if there's
> > > arch specific handling needed in its ioremap() or iounmap(). This
> > > change will simplify implementation by removing duplicated codes with
> > > generic ioremap() and iounmap(), and has the equivalent functioality
> > > as before.
> > >
> > > For s390, add hooks arch_ioremap() and arch_iounmap() for s390's special
> > > operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> > > converted to use ioremap_prot() from GENERIC_IOREMAP.
> > >
> > > Signed-off-by: Baoquan He <bhe@...hat.com>
> > > Cc: Niklas Schnelle <schnelle@...ux.ibm.com>
> > > Cc: Gerald Schaefer <gerald.schaefer@...ux.ibm.com>
> > > Cc: Heiko Carstens <hca@...ux.ibm.com>
> > > Cc: Vasily Gorbik <gor@...ux.ibm.com>
> > > Cc: Alexander Gordeev <agordeev@...ux.ibm.com>
> > > Cc: Christian Borntraeger <borntraeger@...ux.ibm.com>
> > > Cc: Sven Schnelle <svens@...ux.ibm.com>
> > > Cc: linux-s390@...r.kernel.org
> > > ---
> > > v2->v3:
> > > - Add code comment inside arch_ioremap() to help uderstand the
> > > obsucre code. Christoph suggested this, Niklas provided the
> > > paragraph of text.
> > >
> > > arch/s390/Kconfig | 1 +
> > > arch/s390/include/asm/io.h | 25 +++++++++------
> > > arch/s390/pci/pci.c | 65 ++++++++------------------------------
> > > 3 files changed, 30 insertions(+), 61 deletions(-)
> > >
> > > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > > index 318fce77601d..c59e1b25f59d 100644
> > > --- a/arch/s390/Kconfig
> > > +++ b/arch/s390/Kconfig
> > > @@ -135,6 +135,7 @@ config S390
> > > select GENERIC_SMP_IDLE_THREAD
> > > select GENERIC_TIME_VSYSCALL
> > > select GENERIC_VDSO_TIME_NS
> > > + select GENERIC_IOREMAP
> >
> > I think you should add the "if PCI" from the diff in your last mail to
> > this patch.
>
> That's reasonable, will do.
>
> The code change in driver should be posted separately to get reviewing
> from the relevant drvier maintainers. I may wrap it into this series in
> next post so that people know its background.
I agree about doing the driver change separately. Since the problem
already exists one could send it separately. If you want I can take of
that too.
>
> > > select HAVE_ALIGNED_STRUCT_PAGE if SLUB
> > > select HAVE_ARCH_AUDITSYSCALL
> > > select HAVE_ARCH_JUMP_LABEL
> ......
> > > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > > index 73cdc5539384..3c00dc7d79bc 100644
> > > --- a/arch/s390/pci/pci.c
> > > +++ b/arch/s390/pci/pci.c
> > > @@ -244,64 +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 *
> > > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> > > {
> > > - 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;
> > > -
> > > - 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);
> > > + return (void __iomem *) *paddr;
> >
> > nit: no space after the cast
>
> Sorry, remember you pointed this out in v2, while I didn't get what
> it is. Could you be more specific or give the right line of code?
>
> Are you suggesting below line?
>
> - return (void __iomem *) ((unsigned long) area->addr + offset);
> + return (void __iomem *)*paddr;
Yes, though I did just check and somehow checkpatch doesn't complain,
maybe because of the dereference. I do think I remember it complaining
but I guess if it doesn't you might as well keep it this way.
>
> > > + return NULL;
> > > }
> > >
---8<---
Powered by blists - more mailing lists