powerpc/dma: Fix invalid DMA mmap behavior

The refactor of powerpc DMA functions in commit 6666cc17d780
("powerpc/dma: remove dma_nommu_mmap_coherent") incorrectly
changes the way DMA mappings are handled on powerpc.
Since this change, all mapped pages are marked as cache-inhibited
through the default implementation of arch_dma_mmap_pgprot.
This differs from the previous behavior of only marking pages
in noncoherent mappings as cache-inhibited and has resulted in
sporadic system crashes in certain hardware configurations and
workloads (see Bugzilla).

This commit restores the previous correct behavior by providing
an implementation of arch_dma_mmap_pgprot that only marks
pages in noncoherent mappings as cache-inhibited. As this behavior
should be universal for all powerpc platforms a new file,
dma-generic.c, was created to store it.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204145
Fixes: 6666cc17d780 ("powerpc/dma: remove dma_nommu_mmap_coherent")
Signed-off-by: Shawn Anastasio <shawn@anastas.io>
---
 arch/powerpc/Kconfig             |  1 +
 arch/powerpc/kernel/Makefile     |  3 ++-
 arch/powerpc/kernel/dma-common.c | 17 +++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/kernel/dma-common.c

On 18/07/2019 09:54, Shawn Anastasio wrote:
> The refactor of powerpc DMA functions in commit 6666cc17d780
> ("powerpc/dma: remove dma_nommu_mmap_coherent") incorrectly
> changes the way DMA mappings are handled on powerpc.
> Since this change, all mapped pages are marked as cache-inhibited
> through the default implementation of arch_dma_mmap_pgprot.
> This differs from the previous behavior of only marking pages
> in noncoherent mappings as cache-inhibited and has resulted in
> sporadic system crashes in certain hardware configurations and
> workloads (see Bugzilla).
> 
> This commit restores the previous correct behavior by providing
> an implementation of arch_dma_mmap_pgprot that only marks
> pages in noncoherent mappings as cache-inhibited. As this behavior
> should be universal for all powerpc platforms a new file,
> dma-generic.c, was created to store it.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204145
> Fixes: 6666cc17d780 ("powerpc/dma: remove dma_nommu_mmap_coherent")
> Signed-off-by: Shawn Anastasio <shawn@anastas.io>


Is this the default one?

include/linux/dma-noncoherent.h
# define arch_dma_mmap_pgprot(dev, prot, attrs) pgprot_noncached(prot)

Out of curiosity - do not we want to fix this one for everyone?

Either way, the patch is correct. I'm glad to know it was not my " 
powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59" 
which broke it :)

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> ---
>   arch/powerpc/Kconfig             |  1 +
>   arch/powerpc/kernel/Makefile     |  3 ++-
>   arch/powerpc/kernel/dma-common.c | 17 +++++++++++++++++
>   3 files changed, 20 insertions(+), 1 deletion(-)
>   create mode 100644 arch/powerpc/kernel/dma-common.c
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index d8dcd8820369..77f6ebf97113 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -121,6 +121,7 @@ config PPC
>   	select ARCH_32BIT_OFF_T if PPC32
>   	select ARCH_HAS_DEBUG_VIRTUAL
>   	select ARCH_HAS_DEVMEM_IS_ALLOWED
> +	select ARCH_HAS_DMA_MMAP_PGPROT
>   	select ARCH_HAS_ELF_RANDOMIZE
>   	select ARCH_HAS_FORTIFY_SOURCE
>   	select ARCH_HAS_GCOV_PROFILE_ALL
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 56dfa7a2a6f2..ea0c69236789 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -49,7 +49,8 @@ obj-y				:= cputable.o ptrace.o syscalls.o \
>   				   signal.o sysfs.o cacheinfo.o time.o \
>   				   prom.o traps.o setup-common.o \
>   				   udbg.o misc.o io.o misc_$(BITS).o \
> -				   of_platform.o prom_parse.o
> +				   of_platform.o prom_parse.o \
> +				   dma-common.o
>   obj-$(CONFIG_PPC64)		+= setup_64.o sys_ppc32.o \
>   				   signal_64.o ptrace32.o \
>   				   paca.o nvram_64.o firmware.o
> diff --git a/arch/powerpc/kernel/dma-common.c b/arch/powerpc/kernel/dma-common.c
> new file mode 100644
> index 000000000000..5a15f99f4199
> --- /dev/null
> +++ b/arch/powerpc/kernel/dma-common.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Contains common dma routines for all powerpc platforms.
> + *
> + * Copyright (C) 2019 Shawn Anastasio (shawn@anastas.io)
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/dma-noncoherent.h>
> +
> +pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot,
> +		unsigned long attrs)
> +{
> +	if (!dev_is_dma_coherent(dev))
> +		return pgprot_noncached(prot);
> +	return prot;
> +}
>

On 7/17/19 9:59 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 18/07/2019 09:54, Shawn Anastasio wrote:
>> The refactor of powerpc DMA functions in commit 6666cc17d780
>> ("powerpc/dma: remove dma_nommu_mmap_coherent") incorrectly
>> changes the way DMA mappings are handled on powerpc.
>> Since this change, all mapped pages are marked as cache-inhibited
>> through the default implementation of arch_dma_mmap_pgprot.
>> This differs from the previous behavior of only marking pages
>> in noncoherent mappings as cache-inhibited and has resulted in
>> sporadic system crashes in certain hardware configurations and
>> workloads (see Bugzilla).
>>
>> This commit restores the previous correct behavior by providing
>> an implementation of arch_dma_mmap_pgprot that only marks
>> pages in noncoherent mappings as cache-inhibited. As this behavior
>> should be universal for all powerpc platforms a new file,
>> dma-generic.c, was created to store it.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204145
>> Fixes: 6666cc17d780 ("powerpc/dma: remove dma_nommu_mmap_coherent")
>> Signed-off-by: Shawn Anastasio <shawn@anastas.io>
> 
> 
> Is this the default one?
> 
> include/linux/dma-noncoherent.h
> # define arch_dma_mmap_pgprot(dev, prot, attrs) pgprot_noncached(prot)

Yep, that's the one.

> Out of curiosity - do not we want to fix this one for everyone?

Other than m68k, mips, and arm64, everybody else that doesn't have
ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
I assume this behavior is acceptable on those architectures.

> Either way, the patch is correct. I'm glad to know it was not my " 
> powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59" 
> which broke it :)

Yeah, turns out it was just bad luck that I happened to run into these
crashes right after deciding to try out your patch :)

> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Thanks!

> 
> 
> 
>> ---
>>   arch/powerpc/Kconfig             |  1 +
>>   arch/powerpc/kernel/Makefile     |  3 ++-
>>   arch/powerpc/kernel/dma-common.c | 17 +++++++++++++++++
>>   3 files changed, 20 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/powerpc/kernel/dma-common.c
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index d8dcd8820369..77f6ebf97113 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -121,6 +121,7 @@ config PPC
>>       select ARCH_32BIT_OFF_T if PPC32
>>       select ARCH_HAS_DEBUG_VIRTUAL
>>       select ARCH_HAS_DEVMEM_IS_ALLOWED
>> +    select ARCH_HAS_DMA_MMAP_PGPROT
>>       select ARCH_HAS_ELF_RANDOMIZE
>>       select ARCH_HAS_FORTIFY_SOURCE
>>       select ARCH_HAS_GCOV_PROFILE_ALL
>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
>> index 56dfa7a2a6f2..ea0c69236789 100644
>> --- a/arch/powerpc/kernel/Makefile
>> +++ b/arch/powerpc/kernel/Makefile
>> @@ -49,7 +49,8 @@ obj-y                := cputable.o ptrace.o 
>> syscalls.o \
>>                      signal.o sysfs.o cacheinfo.o time.o \
>>                      prom.o traps.o setup-common.o \
>>                      udbg.o misc.o io.o misc_$(BITS).o \
>> -                   of_platform.o prom_parse.o
>> +                   of_platform.o prom_parse.o \
>> +                   dma-common.o
>>   obj-$(CONFIG_PPC64)        += setup_64.o sys_ppc32.o \
>>                      signal_64.o ptrace32.o \
>>                      paca.o nvram_64.o firmware.o
>> diff --git a/arch/powerpc/kernel/dma-common.c 
>> b/arch/powerpc/kernel/dma-common.c
>> new file mode 100644
>> index 000000000000..5a15f99f4199
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/dma-common.c
>> @@ -0,0 +1,17 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Contains common dma routines for all powerpc platforms.
>> + *
>> + * Copyright (C) 2019 Shawn Anastasio (shawn@anastas.io)
>> + */
>> +
>> +#include <linux/mm.h>
>> +#include <linux/dma-noncoherent.h>
>> +
>> +pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot,
>> +        unsigned long attrs)
>> +{
>> +    if (!dev_is_dma_coherent(dev))
>> +        return pgprot_noncached(prot);
>> +    return prot;
>> +}
>>
>

On Thu, Jul 18, 2019 at 1:16 PM Shawn Anastasio <shawn@anastas.io> wrote:
>
> On 7/17/19 9:59 PM, Alexey Kardashevskiy wrote:
> >
> > On 18/07/2019 09:54, Shawn Anastasio wrote:
> >> The refactor of powerpc DMA functions in commit 6666cc17d780
> >> ("powerpc/dma: remove dma_nommu_mmap_coherent") incorrectly
> >> changes the way DMA mappings are handled on powerpc.
> >> Since this change, all mapped pages are marked as cache-inhibited
> >> through the default implementation of arch_dma_mmap_pgprot.
> >> This differs from the previous behavior of only marking pages
> >> in noncoherent mappings as cache-inhibited and has resulted in
> >> sporadic system crashes in certain hardware configurations and
> >> workloads (see Bugzilla).
> >>
> >> This commit restores the previous correct behavior by providing
> >> an implementation of arch_dma_mmap_pgprot that only marks
> >> pages in noncoherent mappings as cache-inhibited. As this behavior
> >> should be universal for all powerpc platforms a new file,
> >> dma-generic.c, was created to store it.
> >>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204145
> >> Fixes: 6666cc17d780 ("powerpc/dma: remove dma_nommu_mmap_coherent")
> >> Signed-off-by: Shawn Anastasio <shawn@anastas.io>
> >
> > Is this the default one?
> >
> > include/linux/dma-noncoherent.h
> > # define arch_dma_mmap_pgprot(dev, prot, attrs) pgprot_noncached(prot)
>
> Yep, that's the one.
>
> > Out of curiosity - do not we want to fix this one for everyone?
>
> Other than m68k, mips, and arm64, everybody else that doesn't have
> ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
> I assume this behavior is acceptable on those architectures.

It might be acceptable, but there's no reason to use pgport_noncached
if the platform supports cache-coherent DMA.

Christoph (+cc) made the change so maybe he saw something we're missing.

> >> ---
> >>   arch/powerpc/Kconfig             |  1 +
> >>   arch/powerpc/kernel/Makefile     |  3 ++-
> >>   arch/powerpc/kernel/dma-common.c | 17 +++++++++++++++++
> >>   3 files changed, 20 insertions(+), 1 deletion(-)
> >>   create mode 100644 arch/powerpc/kernel/dma-common.c
> >>
> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> >> index d8dcd8820369..77f6ebf97113 100644
> >> --- a/arch/powerpc/Kconfig
> >> +++ b/arch/powerpc/Kconfig
> >> @@ -121,6 +121,7 @@ config PPC
> >>       select ARCH_32BIT_OFF_T if PPC32
> >>       select ARCH_HAS_DEBUG_VIRTUAL
> >>       select ARCH_HAS_DEVMEM_IS_ALLOWED
> >> +    select ARCH_HAS_DMA_MMAP_PGPROT
> >>       select ARCH_HAS_ELF_RANDOMIZE
> >>       select ARCH_HAS_FORTIFY_SOURCE
> >>       select ARCH_HAS_GCOV_PROFILE_ALL
> >> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> >> index 56dfa7a2a6f2..ea0c69236789 100644
> >> --- a/arch/powerpc/kernel/Makefile
> >> +++ b/arch/powerpc/kernel/Makefile
> >> @@ -49,7 +49,8 @@ obj-y                := cputable.o ptrace.o
> >> syscalls.o \
> >>                      signal.o sysfs.o cacheinfo.o time.o \
> >>                      prom.o traps.o setup-common.o \
> >>                      udbg.o misc.o io.o misc_$(BITS).o \
> >> -                   of_platform.o prom_parse.o
> >> +                   of_platform.o prom_parse.o \
> >> +                   dma-common.o
> >>   obj-$(CONFIG_PPC64)        += setup_64.o sys_ppc32.o \
> >>                      signal_64.o ptrace32.o \
> >>                      paca.o nvram_64.o firmware.o
> >> diff --git a/arch/powerpc/kernel/dma-common.c
> >> b/arch/powerpc/kernel/dma-common.c
> >> new file mode 100644
> >> index 000000000000..5a15f99f4199
> >> --- /dev/null
> >> +++ b/arch/powerpc/kernel/dma-common.c
> >> @@ -0,0 +1,17 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * Contains common dma routines for all powerpc platforms.
> >> + *
> >> + * Copyright (C) 2019 Shawn Anastasio (shawn@anastas.io)
> >> + */
> >> +
> >> +#include <linux/mm.h>
> >> +#include <linux/dma-noncoherent.h>
> >> +
> >> +pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot,
> >> +        unsigned long attrs)
> >> +{
> >> +    if (!dev_is_dma_coherent(dev))
> >> +        return pgprot_noncached(prot);
> >> +    return prot;
> >> +}
> >>
> >

On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
> > Other than m68k, mips, and arm64, everybody else that doesn't have
> > ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
> > I assume this behavior is acceptable on those architectures.
> 
> It might be acceptable, but there's no reason to use pgport_noncached
> if the platform supports cache-coherent DMA.
> 
> Christoph (+cc) made the change so maybe he saw something we're missing.

I always found the forcing of noncached access even for coherent
devices a little odd, but this was inherited from the previous
implementation, which surprised me a bit as the different attributes
are usually problematic even on x86.  Let me dig into the history a
bit more, but I suspect the righ fix is to default to cached mappings
for coherent devices.

On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote:
> On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
> > > Other than m68k, mips, and arm64, everybody else that doesn't have
> > > ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
> > > I assume this behavior is acceptable on those architectures.
> > 
> > It might be acceptable, but there's no reason to use pgport_noncached
> > if the platform supports cache-coherent DMA.
> > 
> > Christoph (+cc) made the change so maybe he saw something we're missing.
> 
> I always found the forcing of noncached access even for coherent
> devices a little odd, but this was inherited from the previous
> implementation, which surprised me a bit as the different attributes
> are usually problematic even on x86.  Let me dig into the history a
> bit more, but I suspect the righ fix is to default to cached mappings
> for coherent devices.

Ok, some history:

The generic dma mmap implementation, which we are effectively still
using today was added by:

commit 64ccc9c033c6089b2d426dad3c56477ab066c999
Author: Marek Szyprowski <m.szyprowski@samsung.com>
Date:   Thu Jun 14 13:03:04 2012 +0200

    common: dma-mapping: add support for generic dma_mmap_* calls

and unconditionally uses pgprot_noncached in dma_common_mmap, which is
then used as the fallback by dma_mmap_attrs if no ->mmap method is
present.  At that point we already had the powerpc implementation
that only uses pgprot_noncached for non-coherent mappings, and
the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE
is set and otherwise pgprot_dmacoherent, which seems to be uncached.
Arm did support coherent platforms at that time, but they might have
been an afterthought and not handled properly.

So it migt have been that we were all wrong for that time and might
have to fix it up.

On 7/18/19 4:52 AM, Christoph Hellwig wrote:
> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote:
>> On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
>>>> Other than m68k, mips, and arm64, everybody else that doesn't have
>>>> ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
>>>> I assume this behavior is acceptable on those architectures.
>>>
>>> It might be acceptable, but there's no reason to use pgport_noncached
>>> if the platform supports cache-coherent DMA.
>>>
>>> Christoph (+cc) made the change so maybe he saw something we're missing.
>>
>> I always found the forcing of noncached access even for coherent
>> devices a little odd, but this was inherited from the previous
>> implementation, which surprised me a bit as the different attributes
>> are usually problematic even on x86.  Let me dig into the history a
>> bit more, but I suspect the righ fix is to default to cached mappings
>> for coherent devices.
> 
> Ok, some history:
> 
> The generic dma mmap implementation, which we are effectively still
> using today was added by:
> 
> commit 64ccc9c033c6089b2d426dad3c56477ab066c999
> Author: Marek Szyprowski <m.szyprowski@samsung.com>
> Date:   Thu Jun 14 13:03:04 2012 +0200
> 
>      common: dma-mapping: add support for generic dma_mmap_* calls
> 
> and unconditionally uses pgprot_noncached in dma_common_mmap, which is
> then used as the fallback by dma_mmap_attrs if no ->mmap method is
> present.  At that point we already had the powerpc implementation
> that only uses pgprot_noncached for non-coherent mappings, and
> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE
> is set and otherwise pgprot_dmacoherent, which seems to be uncached.
> Arm did support coherent platforms at that time, but they might have
> been an afterthought and not handled properly.
> 
> So it migt have been that we were all wrong for that time and might
> have to fix it up.

Personally, I'm not a huge fan of an implicit default for something
inherently architecture-dependent like this at all. What I'd like to
see is a mechanism that forces architecture code to explicitly
opt in to the default pgprot settings if they don't provide an
implementation of arch_dma_mmap_pgprot. This could perhaps be done
by reversing ARCH_HAS_DMA_MMAP_PGPROT to something like
ARCH_USE_DEFAULT_DMA_MMAP_PGPROT.

This way as more systems are moved to use the common mmap code instead
of their ops->mmap, the people doing the refactoring have to make an
explicit decision about the pgprot settings to use. Such a configuration
would have likely prevented this situation with powerpc from happening.

That being said, if the default behavior doesn't make sense in the
general case it should probably be fixed as well.

Curious to hear some thoughts on this.

On Thu, Jul 18, 2019 at 02:46:00PM -0500, Shawn Anastasio wrote:
> Personally, I'm not a huge fan of an implicit default for something
> inherently architecture-dependent like this at all.

What is inherently architecture specific here over the fact that
the pgprot_* expand to architecture specific bits?

> What I'd like to
> see is a mechanism that forces architecture code to explicitly
> opt in to the default pgprot settings if they don't provide an
> implementation of arch_dma_mmap_pgprot. This could perhaps be done
> by reversing ARCH_HAS_DMA_MMAP_PGPROT to something like
> ARCH_USE_DEFAULT_DMA_MMAP_PGPROT.

I'd rather not create boilerplate code where we don't have to it.  Note
that arch_dma_mmap_pgprot is a little misnamed now, as we also use it
for the in-kernel remapping in kernel/dma/remap.c, which I'm slowly
switching a lot of architectures to.  powerpc will follow soon once
I get the ppc44x that was given to me to actually boot with a recent
kernel (not that I've tried much so far).

>
> This way as more systems are moved to use the common mmap code instead
> of their ops->mmap, the people doing the refactoring have to make an
> explicit decision about the pgprot settings to use. Such a configuration
> would have likely prevented this situation with powerpc from happening.

Every arch except for arm32 now uses dma direct for the direct mapping,
and thus the common code without the indeed odd default.  I also have
a series to remove the default fallback, which is inherently a bad idea:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-no-defaults

> That being said, if the default behavior doesn't make sense in the
> general case it should probably be fixed as well.
>
> Curious to hear some thoughts on this.

I think your patch that started this thread is fine for 5.3 and -stable:

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

But going forward I'd rather have a sane default.

On 7/19/19 2:06 AM, Christoph Hellwig wrote:
 > What is inherently architecture specific here over the fact that
 > the pgprot_* expand to architecture specific bits?

What I meant is that different architectures seem to have different
criteria for setting the different pgprot_ bits. i.e. ppc checks
for cache coherency, arm64 checks for cache coherency and
writecombine, mips just checks for writecombine, etc.

That being said, I'm no expert here and there is probably some behavior
here that would make for a much more sane default.

 > I'd rather not create boilerplate code where we don't have to it. Note
 > that arch_dma_mmap_pgprot is a little misnamed now, as we also use it
 > for the in-kernel remapping in kernel/dma/remap.c, which I'm slowly
 > switching a lot of architectures to.  powerpc will follow soon once
 > I get the ppc44x that was given to me to actually boot with a recent
 > kernel (not that I've tried much so far).

Fair enough. I didn't realize that most of the other architectures
don't use the common code anyways as you mention below.

 > Every arch except for arm32 now uses dma direct for the direct mapping,
 > and thus the common code without the indeed odd default.  I also have
 > a series to remove the default fallback, which is inherently a bad idea:
 >
 > 
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-no-defaults

Awesome. This is great to hear.

 > I think your patch that started this thread is fine for 5.3 and -stable:
 >
 > Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

 > But going forward I'd rather have a sane default.

Agreed.

On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote:
> > On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
> > > > Other than m68k, mips, and arm64, everybody else that doesn't have
> > > > ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
> > > > I assume this behavior is acceptable on those architectures.
> > >
> > > It might be acceptable, but there's no reason to use pgport_noncached
> > > if the platform supports cache-coherent DMA.
> > >
> > > Christoph (+cc) made the change so maybe he saw something we're missing.
> >
> > I always found the forcing of noncached access even for coherent
> > devices a little odd, but this was inherited from the previous
> > implementation, which surprised me a bit as the different attributes
> > are usually problematic even on x86.  Let me dig into the history a
> > bit more, but I suspect the righ fix is to default to cached mappings
> > for coherent devices.
>
> Ok, some history:
>
> The generic dma mmap implementation, which we are effectively still
> using today was added by:
>
> commit 64ccc9c033c6089b2d426dad3c56477ab066c999
> Author: Marek Szyprowski <m.szyprowski@samsung.com>
> Date:   Thu Jun 14 13:03:04 2012 +0200
>
>     common: dma-mapping: add support for generic dma_mmap_* calls
>
> and unconditionally uses pgprot_noncached in dma_common_mmap, which is
> then used as the fallback by dma_mmap_attrs if no ->mmap method is
> present.  At that point we already had the powerpc implementation
> that only uses pgprot_noncached for non-coherent mappings, and
> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE
> is set and otherwise pgprot_dmacoherent, which seems to be uncached.
> Arm did support coherent platforms at that time, but they might have
> been an afterthought and not handled properly.

Cache-coherent devices are still very rare on 32-bit ARM.

Among the callers of dma_mmap_coherent(), almost all are in platform
specific device drivers that only ever run on noncoherent ARM SoCs,
which explains why nobody would have noticed problems.

There is also a difference in behavior between ARM and PowerPC
when dealing with mismatched cacheability attributes: If the same
page is mapped as both cached and uncached to, this may
cause silent undefined behavior on ARM, while PowerPC should
enter a checkstop as soon as it notices.

      Arnd

On Wed, 2019-07-17 at 23:54:37 UTC, Shawn Anastasio wrote:
> The refactor of powerpc DMA functions in commit 6666cc17d780
> ("powerpc/dma: remove dma_nommu_mmap_coherent") incorrectly
> changes the way DMA mappings are handled on powerpc.
> Since this change, all mapped pages are marked as cache-inhibited
> through the default implementation of arch_dma_mmap_pgprot.
> This differs from the previous behavior of only marking pages
> in noncoherent mappings as cache-inhibited and has resulted in
> sporadic system crashes in certain hardware configurations and
> workloads (see Bugzilla).
> 
> This commit restores the previous correct behavior by providing
> an implementation of arch_dma_mmap_pgprot that only marks
> pages in noncoherent mappings as cache-inhibited. As this behavior
> should be universal for all powerpc platforms a new file,
> dma-generic.c, was created to store it.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204145
> Fixes: 6666cc17d780 ("powerpc/dma: remove dma_nommu_mmap_coherent")
> Signed-off-by: Shawn Anastasio <shawn@anastas.io>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/b4fc36e60f25cf22bf8b7b015a701015740c3743

cheers

Arnd Bergmann <arnd@arndb.de> writes:
> On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig <hch@lst.de> wrote:
>> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote:
>> > On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
>> > > > Other than m68k, mips, and arm64, everybody else that doesn't have
>> > > > ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
>> > > > I assume this behavior is acceptable on those architectures.
>> > >
>> > > It might be acceptable, but there's no reason to use pgport_noncached
>> > > if the platform supports cache-coherent DMA.
>> > >
>> > > Christoph (+cc) made the change so maybe he saw something we're missing.
>> >
>> > I always found the forcing of noncached access even for coherent
>> > devices a little odd, but this was inherited from the previous
>> > implementation, which surprised me a bit as the different attributes
>> > are usually problematic even on x86.  Let me dig into the history a
>> > bit more, but I suspect the righ fix is to default to cached mappings
>> > for coherent devices.
>>
>> Ok, some history:
>>
>> The generic dma mmap implementation, which we are effectively still
>> using today was added by:
>>
>> commit 64ccc9c033c6089b2d426dad3c56477ab066c999
>> Author: Marek Szyprowski <m.szyprowski@samsung.com>
>> Date:   Thu Jun 14 13:03:04 2012 +0200
>>
>>     common: dma-mapping: add support for generic dma_mmap_* calls
>>
>> and unconditionally uses pgprot_noncached in dma_common_mmap, which is
>> then used as the fallback by dma_mmap_attrs if no ->mmap method is
>> present.  At that point we already had the powerpc implementation
>> that only uses pgprot_noncached for non-coherent mappings, and
>> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE
>> is set and otherwise pgprot_dmacoherent, which seems to be uncached.
>> Arm did support coherent platforms at that time, but they might have
>> been an afterthought and not handled properly.
>
> Cache-coherent devices are still very rare on 32-bit ARM.
>
> Among the callers of dma_mmap_coherent(), almost all are in platform
> specific device drivers that only ever run on noncoherent ARM SoCs,
> which explains why nobody would have noticed problems.
>
> There is also a difference in behavior between ARM and PowerPC
> when dealing with mismatched cacheability attributes: If the same
> page is mapped as both cached and uncached to, this may
> cause silent undefined behavior on ARM, while PowerPC should
> enter a checkstop as soon as it notices.

On newer Power CPUs it's actually more like the ARM behaviour.

I don't know for sure that it will *never* checkstop but there are at
least cases where it won't. There's some (not much) detail in the
Power8/9 user manuals.

cheers

On 7/22/19 7:16 AM, Michael Ellerman wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
>> On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig <hch@lst.de> wrote:
>>> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote:
>>>> On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
>>>>>> Other than m68k, mips, and arm64, everybody else that doesn't have
>>>>>> ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
>>>>>> I assume this behavior is acceptable on those architectures.
>>>>>
>>>>> It might be acceptable, but there's no reason to use pgport_noncached
>>>>> if the platform supports cache-coherent DMA.
>>>>>
>>>>> Christoph (+cc) made the change so maybe he saw something we're missing.
>>>>
>>>> I always found the forcing of noncached access even for coherent
>>>> devices a little odd, but this was inherited from the previous
>>>> implementation, which surprised me a bit as the different attributes
>>>> are usually problematic even on x86.  Let me dig into the history a
>>>> bit more, but I suspect the righ fix is to default to cached mappings
>>>> for coherent devices.
>>>
>>> Ok, some history:
>>>
>>> The generic dma mmap implementation, which we are effectively still
>>> using today was added by:
>>>
>>> commit 64ccc9c033c6089b2d426dad3c56477ab066c999
>>> Author: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Date:   Thu Jun 14 13:03:04 2012 +0200
>>>
>>>      common: dma-mapping: add support for generic dma_mmap_* calls
>>>
>>> and unconditionally uses pgprot_noncached in dma_common_mmap, which is
>>> then used as the fallback by dma_mmap_attrs if no ->mmap method is
>>> present.  At that point we already had the powerpc implementation
>>> that only uses pgprot_noncached for non-coherent mappings, and
>>> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE
>>> is set and otherwise pgprot_dmacoherent, which seems to be uncached.
>>> Arm did support coherent platforms at that time, but they might have
>>> been an afterthought and not handled properly.
>>
>> Cache-coherent devices are still very rare on 32-bit ARM.
>>
>> Among the callers of dma_mmap_coherent(), almost all are in platform
>> specific device drivers that only ever run on noncoherent ARM SoCs,
>> which explains why nobody would have noticed problems.
>>
>> There is also a difference in behavior between ARM and PowerPC
>> when dealing with mismatched cacheability attributes: If the same
>> page is mapped as both cached and uncached to, this may
>> cause silent undefined behavior on ARM, while PowerPC should
>> enter a checkstop as soon as it notices.
> 
> On newer Power CPUs it's actually more like the ARM behaviour.
> 
> I don't know for sure that it will *never* checkstop but there are at
> least cases where it won't. There's some (not much) detail in the
> Power8/9 user manuals.

The issue was discovered due to sporadic checkstops on P9, so it
seems like it will happen at least sometimes.

> cheers

Shawn Anastasio <shawn@anastas.io> writes:
> On 7/22/19 7:16 AM, Michael Ellerman wrote:
>> Arnd Bergmann <arnd@arndb.de> writes:
>>> On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig <hch@lst.de> wrote:
>>>> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote:
>>>>> On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
>>>>>>> Other than m68k, mips, and arm64, everybody else that doesn't have
>>>>>>> ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
>>>>>>> I assume this behavior is acceptable on those architectures.
>>>>>>
>>>>>> It might be acceptable, but there's no reason to use pgport_noncached
>>>>>> if the platform supports cache-coherent DMA.
>>>>>>
>>>>>> Christoph (+cc) made the change so maybe he saw something we're missing.
>>>>>
>>>>> I always found the forcing of noncached access even for coherent
>>>>> devices a little odd, but this was inherited from the previous
>>>>> implementation, which surprised me a bit as the different attributes
>>>>> are usually problematic even on x86.  Let me dig into the history a
>>>>> bit more, but I suspect the righ fix is to default to cached mappings
>>>>> for coherent devices.
>>>>
>>>> Ok, some history:
>>>>
>>>> The generic dma mmap implementation, which we are effectively still
>>>> using today was added by:
>>>>
>>>> commit 64ccc9c033c6089b2d426dad3c56477ab066c999
>>>> Author: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> Date:   Thu Jun 14 13:03:04 2012 +0200
>>>>
>>>>      common: dma-mapping: add support for generic dma_mmap_* calls
>>>>
>>>> and unconditionally uses pgprot_noncached in dma_common_mmap, which is
>>>> then used as the fallback by dma_mmap_attrs if no ->mmap method is
>>>> present.  At that point we already had the powerpc implementation
>>>> that only uses pgprot_noncached for non-coherent mappings, and
>>>> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE
>>>> is set and otherwise pgprot_dmacoherent, which seems to be uncached.
>>>> Arm did support coherent platforms at that time, but they might have
>>>> been an afterthought and not handled properly.
>>>
>>> Cache-coherent devices are still very rare on 32-bit ARM.
>>>
>>> Among the callers of dma_mmap_coherent(), almost all are in platform
>>> specific device drivers that only ever run on noncoherent ARM SoCs,
>>> which explains why nobody would have noticed problems.
>>>
>>> There is also a difference in behavior between ARM and PowerPC
>>> when dealing with mismatched cacheability attributes: If the same
>>> page is mapped as both cached and uncached to, this may
>>> cause silent undefined behavior on ARM, while PowerPC should
>>> enter a checkstop as soon as it notices.
>> 
>> On newer Power CPUs it's actually more like the ARM behaviour.
>> 
>> I don't know for sure that it will *never* checkstop but there are at
>> least cases where it won't. There's some (not much) detail in the
>> Power8/9 user manuals.
>
> The issue was discovered due to sporadic checkstops on P9, so it
> seems like it will happen at least sometimes.

Yeah true. I wasn't sure if that checkstop was actually caused by a
cached/uncached mismatch or something else, but looks like it was, from
the hostboot issue (https://github.com/open-power/hostboot/issues/180):

  12.47015|  Signature Description    : pu.ex:k0:n0:s0:p00:c0 (L2FIR[16]) Cache line inhibited hit cacheable space


So I'm not really sure how to square that with the documentation in the
user manual:

  If a caching-inhibited load instruction hits in the L1 data cache, the
  load data is serviced from the L1 data cache and no request is sent to
  the NCU.
  If a caching-inhibited store instruction hits in the L1 data cache,
  the store data is written to the L1 data cache and sent to the NCU.
  Note that the L1 data cache and L2 cache are no longer coherent.

I guess I'm either misinterpreting that section or there's some *other*
documentation somewhere I haven't found that says that it will also
checkstop.

cheers