[PATCH v4 6/8] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Robin Murphy]

On 29/07/16 19:55, Robin Murphy wrote:
> On 29/07/16 15:46, Jean-Philippe Brucker wrote:
>> Hi Robin,
>>
>> Very sorry about the delay, I forgot about this minor comment, below
>>
>> On Fri, Jul 01, 2016 at 05:50:15PM +0100, Robin Murphy wrote:
[...]
>>> +		smmu = arm_smmu_get_by_node(fwspec->iommu_np);
>>> +		if (!smmu)
>>> +			return -ENODEV;
>>> +		master = kzalloc(sizeof(*master), GFP_KERNEL);
>>> +		if (!master)
>>> +			return -ENOMEM;
>>> +
>>> +		master->smmu = smmu;
>>> +		fwspec->iommu_priv = master;
>>
>> It's probably best to initialise master->ste.bypass = true here, to
>> reflect the initial state of STEs. Otherwise attach_dev always calls
>> detach_dev first for nothing.
> 
> I'm actually now thinking that that check in attach_dev() should be for
> ste->valid, rather than ste->bypass. That matches the similar check in
> remove_device(), and looking at old local branches I apparently did have
> it that way at some point, and I now can't quite remember why it ended
> up differently. I'll have a proper dig into it next week.

[for a value of "next week" relative to "last week", at least...]

Having now remembered, the reason it is this way is a subtle concession
to the nasty PCI RID alias case. When you come to probe the second
device behind a non-transparent bridge, the first device is already
attached to the default domain so the underlying STE is no longer a
bypass entry, but we've got no way of knowing that - we can't tell we're
going to be in an alias group until we call iommu_group_get_for_dev(),
and by the time that returns it's already too late, since the attach to
the default domain (and thus the attempt to write a now-valid STE with
the same data again) will have happened.

The least complicated ways around that that I can think of are 1)
inspect the stream table itself on attach, 2) maintain a semi-redundant
list within the group of exactly which ID is attached to which domain at
any point in time, 3) treat the initial per-device state as undefined
(!valid, !bypass) so that the initial domain attach is always a safe
break-before-make on the stream table, or 4) disallow aliasing IDs
entirely and just let the BUG() in write_strtab_ent() fire if a
non-transparent bridge ever comes along.

Discounting #4 as unreasonable, #3 is by far the least complicated, so
I've kept this as it is for now. #2 might seem reasonable at a glance,
but it wouldn't be useful for anything _except_ this specific situation,
which in practice we'd expect to encounter rarely if ever.

Robin.

>> Apart from that, this version works fine with my twisted setup. Note
>> that we might have to add a master->fwspec reference in the future, to
>> access those SIDs from various places. If I understood correctly, it
>> should be fine as those objects have the same lifetime after the
>> add_device call.
> 
> That sounds reasonable, although I can't think offhand where we might
> have a master_data without having got it via the containing device
> (unless we also add some other means of keeping track of them). Since
> this series doesn't need any kind of reverse-lookup capabilities I'm
> just keeping things as simple as possible for the time being.
> 
> Robin.
> 
>>
>> Thanks,
>> Jean-Philippe
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>