[PATCH v2] iommu/arm-smmu-v3: Allow default substream bypass with a pasid support

Thu Aug 17 09:24:51 PDT 2023

On 2023-08-17 16:20, Jason Gunthorpe wrote:
> On Wed, Aug 16, 2023 at 09:21:35PM -0700, Nicolin Chen wrote:
>> When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver sets the
>> arm_smmu_domain->stage to ARM_SMMU_DOMAIN_BYPASS and skips the allocation
>> of a CD table, and then sets STRTAB_STE_0_CFG_BYPASS to the CONFIG field
>> of the STE. This works well for devices that only have one substream, i.e.
>> pasid disabled.
>>
>> With a pasid-capable device, however, there could be a use case where it
>> allows an IDENTITY domain attachment without disabling its pasid feature.
>> This requires the driver to allocate a multi-entry CD table to attach the
>> IDENTITY domain to its default substream and to configure the S1DSS filed
>> of the STE to STRTAB_STE_1_S1DSS_BYPASS. So, there is a missing link here
>> between the STE setup and an IDENTITY domain attachment.
>>
>> Add a new stage ARM_SMMU_DOMAIN_BYPASS_S1DSS to tag this configuration by
>> overriding the ARM_SMMU_DOMAIN_BYPASS if the device has pasid capability.
>> This new tag will allow the driver allocating a CD table yet skipping an
>> CD insertion from the IDENTITY domain, and setting up the STE accordingly.
>>
>> In a use case of ARM_SMMU_DOMAIN_BYPASS_S1DSS, the SHCFG field of the STE
>> should be set to STRTAB_STE_1_SHCFG_INCOMING. In other cases of having a
>> CD table, the shareability comes from a CD, not the SHCFG field: according
>> to "13.5 Summary of attribute/permission configuration fields" in the spec
>> the SHCFG field value is irrelevant. So, always configure the SHCFG field
>> of the STE to STRTAB_STE_1_SHCFG_INCOMING when a CD table is present, for
>> simplification.
>>
>> Signed-off-by: Nicolin Chen <nicolinc at nvidia.com>
>> ---
>>
>> Changelog
>> v2:
>>   * Rebased on top of Michael's series reworking CD table ownership:
>>     https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@google.com/
>>   * Added a new ARM_SMMU_DOMAIN_BYPASS_S1DSS stage to tag the use case
>> v1: https://lore.kernel.org/all/20230627033326.5236-1-nicolinc@nvidia.com/
> 
> After rebasing there really shouldn't be a
> ARM_SMMU_DOMAIN_BYPASS_S1DSS. I want to get to a model where the
> identity domain is a global static, so it can't be changing depending
> on how it is attched.
> 
> I continue to think that the right way to think about this is to have
> the CD table code generate the STE it wants and when doing so it will
> inspect what SSID0 is. If it is the IDENTITY domain then it fills
> s1dss / etc

Indeed, that's what I was getting at with "generalisation of
ARM_SMMU_DOMAIN_BYPASS based on s1cdmax" - just one type all the way 
down to the bowels of arm_smmu_write_strtab_ent(), which then decides 
whether it means touching S1DSS or Config in the given STE.

>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index b27011b2bec9..860db4fbb995 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -1271,6 +1271,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>>   	 * 3. Update Config, sync
>>   	 */
>>   	u64 val = le64_to_cpu(dst[0]);
>> +	u8 s1dss = STRTAB_STE_1_S1DSS_SSID0;
>>   	bool ste_live = false;
>>   	struct arm_smmu_device *smmu = NULL;
>>   	struct arm_smmu_ctx_desc_cfg *cd_table = NULL;
>> @@ -1290,6 +1291,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>>   
>>   	if (smmu_domain) {
>>   		switch (smmu_domain->stage) {
>> +		case ARM_SMMU_DOMAIN_BYPASS_S1DSS:
>> +			s1dss = STRTAB_STE_1_S1DSS_BYPASS;
>> +			fallthrough;
>>   		case ARM_SMMU_DOMAIN_S1:
>>   			cd_table = &master->cd_table;
>>   			break;
> 
> Eg, I think the code looks much nicer if the logic here is more like:
> 
> if (master->cd_table.cdtab)
>     arm_smmu_cd_table_get_ste(master->cd_table, &ste)
> else if (master->domain)
>     arm_smmu_domain_get_ste(master->domain, &ste);
> else
>     ste = not attached
> 
> And you'd check in arm_smmu_cd_table_get_ste() to learn the CD
> parameters and also what SSID=0 is. If SSID=0 is IDENTITY then
> arm_smmu_cd_table_get_ste would return with S1DSS set.
> 
> arm_smmu_domain_get_ste() would multiplex based on the domain type.
> 
>> @@ -2435,6 +2440,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>   	} else if (smmu_domain->smmu != smmu)
>>   		ret = -EINVAL;
>>   
>> +	/*
>> +	 * When attaching an IDENTITY domain to a master with pasid capability,
>> +	 * the master can still enable SVA feature by allocating a multi-entry
>> +	 * CD table and attaching the IDENTITY domain to its default substream
>> +	 * that alone can be byassed using the S1DSS field of the STE.
>> +	 */
>> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS && master->ssid_bits &&
>> +	    smmu->features & ARM_SMMU_FEAT_TRANS_S1)
>> +		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS_S1DSS;
> 
> Then you don't technically need to do this.
> 
> Though if we can't atomically change the STE from IDENTITY to IDENTIY
> with a CD then you still have to do something here,

Strictly I think we are safe to do that - fill in all the S1* fields 
while Config[0] is still 0 and they're ignored, sync, then set 
Config[0]. Adding a CD table under a translation domain should be 
achievable as well, since S1CDMax, S1ContextPtr and S1Fmt can all be 
updated together atomically (although it's still the kind of switcheroo 
where I'd be scared of a massive boulder suddenly rolling out of the 
ceiling...)

> but really what we
> want is to force a CD table for all cases if PASID is enabled, and
> force DMA domains to be S2 domains as well.

Wut? No, DMA domains really want to be stage 1, for many reasons. 
Implementing them with stage 2 when stage 1 isn't supported was always a 
bit of a bodge, but thankfully I'm not aware of anyone ever building a 
stage-2-only SMMUv3 anyway.

The most glaringly obvious one, though, is that I think people like 
PASID support and SVA to actually work ;)

Thanks,
Robin.

>>   	mutex_unlock(&smmu_domain->init_mutex);
>>   	if (ret)
>>   		return ret;
>> @@ -2456,7 +2471,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>   	list_add(&master->domain_head, &smmu_domain->devices);
>>   	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>>   
>> -	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 ||
>> +	    smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS_S1DSS) {
>>   		if (!master->cd_table.cdtab) {
>>   			ret = arm_smmu_alloc_cd_tables(master);
>>   			if (ret) {
> 
> So more like:
> 
>   if (smmu_domain == IDENTIY && arm_smmu_support_ssid(dev))
>       arm_smmu_alloc_cd_tables()
> 
> Jason