Re: [PATCH v2 6/7] iommu/arm-smmu: Finish off SMMUv3 default domain support

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

On 06/06/16 16:47, Jean-Philippe Brucker wrote:
> Hi Robin,
>
> I quite like this change, the result looks pretty clean. I rebased my
> current work and didn't notice any major issue. Some nits below.

Thanks for looking!

> On Fri, Jun 03, 2016 at 06:15:41PM +0100, Robin Murphy wrote:
>> The driver's current reliance on attaching/detaching an entire group
>> for the first device it sees is at odds with the IOMMU core's initial
>> construction of a group by adding each device and attaching it to the
>> default domain in turn. As it turns out, we can happily do away with the
>> whole palaver by simply letting each device be in charge of its own
>> stream ID/stream table entry, and reducing the problem of tracking
>> duplicate IDs and domains down to "Is the STE already associated with
>> the appropriate context?", which is easily done by just looking at the
>> stream table itself.
>>
>> With an of_xlate() callback in place, devices attached to default
>> domains will now get appropriate DMA ops installed, so make sure we
>> enable translation again to stop them getting horribly confused - this
>> reverts the SMMUv3 portion of cbf8277ef456 ("iommu/arm-smmu: Treat
>> IOMMU_DOMAIN_DMA as bypass for now")
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
> ...
>>
>> -static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
>> +static void arm_smmu_install_ste(struct arm_smmu_master_data *master,
>> +				struct arm_smmu_domain *smmu_domain)
>
> (Second line is misaligned here)

Fixed.

>>   {
>> -	int i;
>> -	struct arm_smmu_domain *smmu_domain = smmu_group->domain;
>> -	struct arm_smmu_strtab_ent *ste = &smmu_group->ste;
>> -	struct arm_smmu_device *smmu = smmu_group->smmu;
>> +	struct arm_smmu_device *smmu = master->smmu;
>> +	struct arm_smmu_strtab_ent *ste = &master->ste;
>> +	__le64 *step = arm_smmu_get_step_for_sid(smmu, master->sid);
>>
>>   	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>>   		ste->s1_cfg = &smmu_domain->s1_cfg;
>> @@ -1634,42 +1628,16 @@ static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
>>   		ste->s2_cfg = &smmu_domain->s2_cfg;
>>   	}
>>
>> -	for (i = 0; i < smmu_group->num_sids; ++i) {
>> -		u32 sid = smmu_group->sids[i];
>> -		__le64 *step = arm_smmu_get_step_for_sid(smmu, sid);
>> -
>> -		arm_smmu_write_strtab_ent(smmu, sid, step, ste);
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -static void arm_smmu_detach_dev(struct device *dev)
>> -{
>> -	struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
>> -
>> -	smmu_group->ste.bypass = true;
>> -	if (arm_smmu_install_ste_for_group(smmu_group) < 0)
>> -		dev_warn(dev, "failed to install bypass STE\n");
>> -
>> -	smmu_group->domain = NULL;
>> +	arm_smmu_write_strtab_ent(smmu, master->sid, step, ste);
>>   }
>>
>>   static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>   {
>>   	int ret = 0;
>> -	struct arm_smmu_device *smmu;
>>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> -	struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
>> +	struct arm_smmu_master_data *master = dev->archdata.iommu;
>
> (calling this member 'master' or 'smmu_master' consistently, instead of
> 'data', would make the driver easier to grep)

Good point; it's now called "master" everywhere.

>> +	struct arm_smmu_device *smmu = master->smmu;
>>
>> -	if (!smmu_group)
>> -		return -ENOENT;
>> -
>> -	/* Already attached to a different domain? */
>> -	if (smmu_group->domain && smmu_group->domain != smmu_domain)
>> -		arm_smmu_detach_dev(dev);
>> -
>> -	smmu = smmu_group->smmu;
>>   	mutex_lock(&smmu_domain->init_mutex);
>>
>>   	if (!smmu_domain->smmu) {
>> @@ -1688,21 +1656,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>   		goto out_unlock;
>>   	}
>>
>> -	/* Group already attached to this domain? */
>> -	if (smmu_group->domain)
>> -		goto out_unlock;
>> +	master->ste.bypass = false;
>
> Should also set master->ste.valid = true. It worked out of the box
> during my first test, because master is allocated with kmalloc and
> initialised with garbage. Could we also use kzalloc, in the of_xlate
> patch?

Yikes! I think this is the of_xlate directly cherry-picked from a more 
aggressive attempt to remove arm_smmu_strtab_ent along with 
arm_smmu_group, and those bits of initialisation never got added back; 
both fixed.

Thanks for catching all those - I've recreated the unversioned 
iommu/generic branch with a fixup patch on top for the time being.

Robin.

>
> Thanks,
> Jean-Philippe
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html