On Thu, Aug 17, 2023 at 6:37 AM Nicolin Chen [off-list ref] wrote:

On Wed, Aug 16, 2023 at 09:18:45PM +0800, Michael Shavit wrote:

quoted

+/*
+ * 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..

quoted

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

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel