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

[Nicolin Chen]

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.

> @@ -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.

Thanks
Nicolin