Re: iommu/io-pgtable: depend on !GENERIC_ATOMIC64 when using COMPILE_TEST with LPAE

[Will Deacon]

[+PeterZ because he enjoys things like this]

On Mon, Sep 25, 2017 at 05:37:46PM +0200, Geert Uytterhoeven wrote:
> On Mon, Sep 25, 2017 at 5:21 PM, Will Deacon <will.deacon@xxxxxxx> wrote:
> > On Mon, Sep 25, 2017 at 09:16:22AM +0200, Geert Uytterhoeven wrote:
> >> On Wed, Jul 12, 2017 at 7:16 PM, Linux Kernel Mailing List
> >> <linux-kernel@xxxxxxxxxxxxxxx> wrote:
> >> > Web:        https://git.kernel.org/torvalds/c/c1004803b40596c1aabbbc78a6b1b33e4dfd96c6
> >> > Commit:     c1004803b40596c1aabbbc78a6b1b33e4dfd96c6
> >> > Parent:     58188afeb727e0f73706f1460707bd3ba6ccc221
> >> > Refname:    refs/heads/master
> >> > Author:     Will Deacon <will.deacon@xxxxxxx>
> >> > AuthorDate: Fri Jun 23 11:45:57 2017 +0100
> >> > Committer:  Will Deacon <will.deacon@xxxxxxx>
> >> > CommitDate: Fri Jun 23 17:58:02 2017 +0100
> >> >
> >> >     iommu/io-pgtable: depend on !GENERIC_ATOMIC64 when using COMPILE_TEST with LPAE
> >> >
> >> >     The LPAE/ARMv8 page table format relies on the ability to read and write
> >> >     64-bit page table entries in an atomic fashion. With the move to a lockless
> >> >     implementation, we also need support for cmpxchg64 to resolve races when
> >> >     installing table entries concurrently.
> >> >
> >> >     Unfortunately, not all architectures support cmpxchg64, so the code can
> >> >     fail to compiler when building for these architectures using COMPILE_TEST.
> >> >     Rather than disable COMPILE_TEST altogether, instead check that
> >> >     GENERIC_ATOMIC64 is not selected, which is a reasonable indication that
> >> >     the architecture has support for 64-bit cmpxchg.
> >> >
> >> >     Reported-by: kbuild test robot <fengguang.wu@xxxxxxxxx>
> >> >     Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
> >> > ---
> >> >  drivers/iommu/Kconfig | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> >> > index 6ee3a25ae731..c88cfa7522b2 100644
> >> > --- a/drivers/iommu/Kconfig
> >> > +++ b/drivers/iommu/Kconfig
> >> > @@ -23,7 +23,7 @@ config IOMMU_IO_PGTABLE
> >> >  config IOMMU_IO_PGTABLE_LPAE
> >> >         bool "ARMv7/v8 Long Descriptor Format"
> >> >         select IOMMU_IO_PGTABLE
> >> > -       depends on HAS_DMA && (ARM || ARM64 || COMPILE_TEST)
> >> > +       depends on HAS_DMA && (ARM || ARM64 || (COMPILE_TEST && !GENERIC_ATOMIC64))
> >> >         help
> >> >           Enable support for the ARM long descriptor pagetable format.
> >> >           This allocator supports 4K/2M/1G, 16K/32M and 64K/512M page
> >>
> >> I can't find where this patch was submitted and discussed, so I'm replying
> >> to this email. On which architectures did it fail to compile?
> >
> > It was in response to a report from the kbuild test robot on m32r, but
> > looking back I now see that it didn't go to the lists for some reason. I've
> > included the report at the end of this email so you can have a look.
> 
> Thanks!
> 
> >> cmpxchg64() is defined by include/asm-generic/cmpxchg.h, so I fail to
> >> see what's the relation with GENERIC_ATOMIC64, which is related to
> >> lib/atomic64.c instead.
> >
> > Yeah, it's a bit of a hack, but we're basically relying on architectures
> > that don't select GENERIC_ATOMIC64 providing their own cmpxchg64
> > implementation, which seems to be the case.
> >
> >> E.g. on m68k, which uses GENERIC_ATOMIC64, it compile-tested fine before.
> >
> > FWIW, the lock-based atomics wouldn't be sufficient at runtime, but I
> > appreciate that we're only talking about COMPILE_TEST here.
> 
> Correct.
> 
> >> Perhaps there's another (SMP vs UP?) dependency, as
> >> include/asm-generic/cmpxchg.h cannot be used on SMP?
> >> Should it be COMPILE_TEST && (!GENERIC_ATOMIC64 || !SMP)?
> >
> > I don't see how that helps. Are you seeing build failures on a non-SMP
> > arch?
> 
> I'm not seeing build failures.
> I see reduced build coverage on m68k due to this change.
> 
> So m32r fails to provide a definition for cmpxchg64(), which has nothing to
> do with GENERIC_ATOMIC64.
> Probably m32r just needs the same treatment as in commit feb20ec2bb859d1d
> ("m68k: Add missing cmpxchg64() if CONFIG_RMW_INSNS=y")

I have no idea, tbh. m32r claims to be SMP in some cases but it's orphaned
so I don't know who we could ask about this. If we ended up with a
lock-based 64-bit cmpxchg but a native 8/16/32-bit cmpxchg then we're
asking for trouble because they don't interwork.

> Worse, people are now adding more depends on !GENERIC_ATOMIC64 for
> various IOMMU drivers, to fix up "selects IOMMU_IO_PGTABLE_LPAE which
> has unmet direct dependencies" warnings :-(

Well I'd certainly be willing to revisit the dependency if all architectures
provided cmpxchg64, but that's not something that I'm comfortable
implementing without the ability to test it since it could actually cause
runtime breakage just for the sake of getting a driver building that will
never be used on those architectures.

I didn't want to drop the || COMPILE_TEST completely because we get useful
coverage from it on x86.

Will