On 23/09/2019 12:10, Russell King - ARM Linux admin wrote:
On Mon, Sep 23, 2019 at 09:41:34AM +0100, Russell King - ARM Linux admin wrote:
On Mon, Sep 23, 2019 at 02:51:15AM +0000, Y.b. Lu wrote:
Hi Russell,
-----Original Message-----
From: Russell King <rmk@xxxxxxxxxxxxxxx> On Behalf Of Russell King
Sent: Sunday, September 22, 2019 6:27 PM
To: Robin Murphy <robin.murphy@xxxxxxx>; dann frazier
<dann.frazier@xxxxxxxxxxxxx>; Will Deacon <will.deacon@xxxxxxx>; Nicolin
Chen <nicoleotsuka@xxxxxxxxx>; Y.b. Lu <yangbo.lu@xxxxxxx>; Christoph
Hellwig <hch@xxxxxx>
Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>; Ulf Hansson
<ulf.hansson@xxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx
Subject: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA
coherence
We must not unconditionally set the DMA snoop bit; if the DMA API is
assuming that the device is not DMA coherent, and the device snoops the CPU
caches, the device can see stale cache lines brought in by speculative prefetch.
This leads to the device seeing stale data, potentially resulting in corrupted
data transfers. Commonly, this results in a descriptor fetch error such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
but can lead to other errors, and potentially direct the SDHCI controller to
read/write data to other memory locations (e.g. if a valid descriptor is visible
to the device in a stale cache line.)
Fix this by ensuring that the DMA snoop bit corresponds with the behaviour of
the DMA API. Since the driver currently only supports DT, use
of_dma_is_coherent(). Note that device_get_dma_attr() can not be used as
that risks re-introducing this bug if/when the driver is converted to ACPI.
[Y.b. Lu] May I have your suggestion how to check this condition with ACPI?
Although we didn’t support APCI on upstream for this driver now, as I know we had already had ACPI used internally and would be upstreamed.
Thanks.
The short answer is, I don't believe there is an equivalent for ACPI.
However, it's a question for ACPI people, not me, as I have little
knowedge about ACPI.
From what I've looked at so far, for ACPI, the decision used for
dev->dma_coherent() / dev_is_dma_coherent() (which controls the DMA
API behaviour) is not available - this is a decision by the SMMU code.
See:
drivers/acpi/arm64/iort.c:arm_smmu_dma_configure()
drivers/acpi/scan.c:acpi_dma_configure()
arch/arm64/mm/dma-mapping.c:arch_setup_dma_ops()
The decision is made whether the SMMU is in coherent walk mode.
The proposed interface to use is device_get_dma_attr(). For OF,
this works as expected - it mirrors of_dma_is_coherent(). However,
for ACPI it does not - under ACPI, it uses acpi_get_dma_attr().
Which is more or less the direct equivalent of of_dma_is_coherent(),
just with an extra state and rather different-looking inheritance logic.
See:
drivers/acpi/scan.c:acpi_get_dma_attr()
This can return one of three states. Let's concentrate on the
coherent/non-coherent states.
(note that the third state is moot anyway because it will actively
prevent the driver from doing DMA API operations - see dma_dummy_ops)
This depends on
adev->flags.coherent_dma. This gets set by acpi_init_coherency(),
which looks up the _CCA property in the device handle.
It looks to me like it is entirely possible for ACPI to say that, for
example, the SMMU is coherent, which will cause dev->dma_coherent to
be set, but the device (and it's parents) not to have _CCA indicating
that the device is coherent.
It is indeed entirely possible to have an inherently non-coherent device
translated by a coherent SMMU - see the USB controller in your Juno, for
example - however in terms of these firmware properties those
coherencies are orthogonal; the latter only describes the SMMU's
pagetable walk interface, not the path(s) through its translation
interface(s) which aren't even necessarily all equal.
It seems that the only way this could be guaranteed is if the ESDHC
was a child of the SMMU - but I don't know whether that is the case
or not. If it isn't, using device_get_dma_attr() will result in a
coherency disagreement between the DMA API and the device, which
will lead to ADMA errors and potential data corruption.
There may be some subtle reason this can't happen, but from merely
looking at the code, I'd say using device_get_dma_attr() here would
be dangerous.
This can't ever happen because SMMUs aren't represented in the ACPI
namespace at all. And even if they were, such a topology would still be
as bogus as it would be in DT - physically, it's perfectly possible for
a single "device" like a PCI host bridge to have different sets of
requester IDs translated by different SMMUs, so treating the SMMU as a
hierarchical "bus" just doesn't work.
The best I can come up with is something like:
enum dev_dma_attr attr, expected;
attr = device_get_dma_attr(dev);
expected = dev_is_dma_coherent(dev) ? DEV_DMA_COHERENT :
DEV_DMA_NON_COHERENT;
WARN_ON(attr != expected);
This would be a tautology given where dev->dma_coherent comes from in
the first place - see {platform,pci}_dma_configure().
Maybe even aborting the probe if they don't agree. However, it seems
dev_is_dma_coherent() is an IOMMU/DMA API implementation detail that
only IOMMU drivers and arch code are supposed to use.
What I'm hearing at the moment from Jon Nettleton is that the NXP ACPI
description does not include an IORT table and doesn't mention the
SMMU. If I'm reading the code correctly, that means the Linux DMA API
will assume all devices are DMA non-coherent - and if we use
device_get_dma_attr() for this, and the ACPI device description has
_CCA=1, we'll end up with potentially data corrupting mismatched DMA
coherency expectations.
In terms of DMA API coherency, unless you're working on the SMMU drivers
you can consider IORT entirely irrelevant and just treat _CCA for
namespace devices as the first and last word. Yes, technically IORT can
describe weird stuff with the CPM attribute, but I have every intention
to keep ignoring that in Linux.
Robin.
