On 3 May 2017, at 11:32, Robin Murphy [off-list ref] wrote:

quoted

On 28/04/17 14:22, Ard Biesheuvel wrote:

quoted

On 28 April 2017 at 14:17, Will Deacon [off-list ref] wrote:

quoted

On Fri, Apr 28, 2017 at 02:14:49PM +0100, Ard Biesheuvel wrote:

quoted

On 28 April 2017 at 14:11, Will Deacon [off-list ref] wrote:
Hi Ard,

[+ devicetree@]

quoted

On Fri, Apr 14, 2017 at 01:43:15PM +0100, Ard Biesheuvel wrote:
DT nodes may have a status property, and if they do, such nodes should
only be considered present if the status property is set to 'okay'.

Currently, we call the init function of IOMMUs described by the device
tree without taking this into account, which may result in the output
below on systems where some SMMUs may be legally disabled.

Failed to initialise IOMMU /smb/smmu at e0200000
Failed to initialise IOMMU /smb/smmu at e0c00000
arm-smmu e0a00000.smmu: probing hardware configuration...
arm-smmu e0a00000.smmu: SMMUv1 with:
arm-smmu e0a00000.smmu:  stage 2 translation
arm-smmu e0a00000.smmu:  coherent table walk
arm-smmu e0a00000.smmu:  stream matching with 32 register groups, mask 0x7fff
arm-smmu e0a00000.smmu:  8 context banks (8 stage-2 only)
arm-smmu e0a00000.smmu:  Supported page sizes: 0x60211000
arm-smmu e0a00000.smmu:  Stage-2: 40-bit IPA -> 40-bit PA
Failed to initialise IOMMU /smb/smmu at e0600000
Failed to initialise IOMMU /smb/smmu at e0800000

Since this is not an error condition, only call the init function if
the device is enabled, which also inhibits the spurious error messages.

Signed-off-by: Ard Biesheuvel <redacted>
---
drivers/iommu/of_iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 2683e9fc0dcf..2dd1206e6c0d 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c

@@ -183,7 +183,7 @@ static int __init of_iommu_init(void)
     for_each_matching_node_and_match(np, matches, &match) {
             const of_iommu_init_fn init_fn = match->data;

-             if (init_fn(np))
+             if (of_device_is_available(np) && init_fn(np))
                     pr_err("Failed to initialise IOMMU %s\n",
                             of_node_full_name(np));
     }

Is there a definition of what status = "disabled" is supposed to mean for an
IOMMU? For example, that could mean that the firmware has pre-programmed the
SMMU with particular translations or memory attributes (a bit like the
CCA=1, CPM=1, DACS=0 case in ACPI IORT), or even disabled DMA traffic
altogether.

So I think we'd need an update to the generic IOMMU binding text to say
exactly what the semantics are supposed to be here.

I agree that it might make sense to describe the behavior of the IOMMU
when it is left in the state we found it in. But that is not the same
as status=disabled.

The DTS subtree contains loads and loads of boilerplate
configurations, where only some pieces are enabled in the final image
by setting status=okay. So a node that has status 'disabled' should be
treated as 'not present', not as 'present but can be ignored under
assumptions such and such'

In other words, I think we are talking about two different issues here.

I'm not so sure... if we have a master device that has an iommus= property
pointing to an IOMMU with status="disabled", I really don't know whether we
should:

 1. Assume the master can do DMA with a 1:1 mapping of memory and no
    changes to memory attributes

 2. Assume the master can do DMA with a 1:1 mapping of memory, but
    potentially with changes to the attributes

 3. Assume the master can do DMA, but with some pre-existing translation
    (what?)

 4. Assume the master can't do DMA

and I also don't know whether the "dma-coherent" property remains valid.

Ah yes. Good point.

So indeed, there should be some IOMMU specific status property that
can convey all of the above, or 1. and 4. at the minimum

FWIW, the underlying issue being addressed here should be going away now
anyway, since the now-queued probe deferral series obviates the init_fn
early-device-creation bodge. I've been deliberately ignoring it for some
time for precisely that reason ;)

Ok. I have also updated the Seattle firmware to remove the smmu nodes and the associated iommus/iommu-map properties entirely when disabling SMMU support in the firmware, which should address Will's concern regarding unspecified behavior of a disabled SMMU.

IOW, this patch can be disregarded. Thanks.