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@xxxxxxxxxxxxxxxxxxx >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
