Re: [PATCH v6 05/10] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Michael Shavit]

On Thu, Aug 17, 2023 at 6:37 AM Nicolin Chen <nicolinc@xxxxxxxxxx> wrote:
>
> On Wed, Aug 16, 2023 at 09:18:45PM +0800, Michael Shavit wrote:
>
> > +/*
> > + * Write the CD to the CD tables for all masters that this domain is attached
> > + * to. Note that this is used to update the non-pasid CD entry when SVA takes
> > + * over an existing ASID, as well as to write new pasid CD entries when
> > + * attaching an SVA domain (although the domain passed as the parameter is the
> > + * RID domain that this domain is mapped to).
> > + */
> > +static int arm_smmu_write_ctx_desc_devices(struct arm_smmu_domain *smmu_domain,
>
> Iterating the entire device list of a domain looks like the
> arm_smmu_atc_inv_domain(). So it feels to me that it could be
> called arm_smmu_write_ctx_desc_domain()? Not a critical thing
> though..
>
> > +                                          int ssid,
> > +                                          struct arm_smmu_ctx_desc *cd)
> > +{
> > +       struct arm_smmu_master *master;
> > +       unsigned long flags;
> > +       int ret;
> > +
> > +       spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> > +       list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> > +               ret = arm_smmu_write_ctx_desc(master, ssid, cd);
> > +               if (ret) {
> > +                       list_for_each_entry_from_reverse(master, &smmu_domain->devices, domain_head)
> > +                               arm_smmu_write_ctx_desc(master, ssid, NULL);
>                                                                         ^
> Here it always reverts back to NULL, which isn't ideal since
> an CD entry could be something valid other than NULL prior to
> this function call. IIUIC the conversation in v5, we'd need
> another SVA series to clean up domain sharing, so this would
> be cleaned up too after that? If so, perhaps we could note it
> down in the comments above too.

Technically this is OK because it can only fail when an entirely new
CD entry is written. Honestly I'm tempted to revert the addition of
this helper function, especially as it ends up further complicated in
next patches since it has to deal with the fact that it's used to
update PASID-entries-on-RID-domains as well as the non pasid CD
entries for the asid-sharing case.

>
> > @@ -2458,8 +2446,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >                 ret = -EINVAL;
> >                 goto out_unlock;
> >         } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
> > -                  smmu_domain->cd_table.stall_enabled !=
> > -                          master->stall_enabled) {
> > +                  smmu_domain->cd_table.stall_enabled != master->stall_enabled) {
> >                 ret = -EINVAL;
> >                 goto out_unlock;
>
> This doesn't seem to be a related change? Probably should be in
> one of the previous patches, or just dropped.

Whoops, must have sneaked in during a rebase.