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

Jason Gunthorpe
2024-02-13T17:51:06Z

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