[PATCH v5 09/17] iommu/arm-smmu-v3: Put writing the context descriptor in the right order

[Jason Gunthorpe]

On Tue, Feb 13, 2024 at 03:42:53PM +0000, Mostafa Saleh wrote:
> > @@ -2659,6 +2651,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  				master->domain = NULL;
> >  				goto out_list_del;
> >  			}
> > +		} else {
> > +			/*
> > +			 * arm_smmu_write_ctx_desc() relies on the entry being
> > +			 * invalid to work, clear any existing entry.
> > +			 */
> > +			ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> > +						      NULL);
> > +			if (ret) {
> > +				master->domain = NULL;
> > +				goto out_list_del;
> > +			}
> 
> Instead of having duplicate
>            if (ret) {
>                master->domain = NULL;
>                goto out_list_del;
>            }
> 
> In the if and the else, we can just move it outside.

Stylistically I often try to avoid shifting the error path from its
statement, but it is OK either way..

However, part 2 removes the need for error handling here entirely, so
let's leave it.

> > @@ -2668,15 +2671,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  		}
> >  
> >  		arm_smmu_make_cdtable_ste(&target, master);
> > +		arm_smmu_install_ste_for_dev(master, &target);
> >  		break;
> >  	case ARM_SMMU_DOMAIN_S2:
> >  		arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
> > +		arm_smmu_install_ste_for_dev(master, &target);
> > +		if (master->cd_table.cdtab)
> > +			arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> > +						      NULL);
> >  		break;
> >  	case ARM_SMMU_DOMAIN_BYPASS:
> >  		arm_smmu_make_bypass_ste(&target);
> > +		arm_smmu_install_ste_for_dev(master, &target);
> > +		if (master->cd_table.cdtab)
> > +			arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> > +						      NULL);
> >  		break;
> >  	}
> This invalidates the CD while the STE is in bypass/S2 which is a new behavior
> to the driver, 

Yes

> I don’t see anything from the user manual about this, so I
> believe that is fine.

Nor do I. Nor can I see any reason why HW would care. We also
invalidate ASID's and VMID's after their tables have been removed from
the STE/CD too.

There are other options here if this is found out to be a trouble but
they are convoluted enough to not do them without a concrete reason.

Thanks,
Jason