Message ID | 7-v5-9a37e0c884ce+31e3-smmuv3_newapi_p2_jgg@nvidia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Update SMMUv3 to the modern iommu API (part 2/3) | expand |
On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > Introduce arm_smmu_make_s1_cd() to build the CD from the paging S1 domain, > and reorganize all the places programming S1 domain CD table entries to > call it. > > Split arm_smmu_update_s1_domain_cd_entry() from > arm_smmu_update_ctx_desc_devices() so that the S1 path has its own call > chain separate from the unrelated SVA path. > > arm_smmu_update_s1_domain_cd_entry() only works on S1 domains > attached to RIDs and refreshes all their CDs. > > Remove the forced clear of the CD during S1 domain attach, > arm_smmu_write_cd_entry() will do this automatically if necessary. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 25 +++++++- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 60 +++++++++++++------ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 8 +++ > 3 files changed, 75 insertions(+), 18 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > index bb9bb6fd7914ce..6acc65f6d00a71 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > @@ -54,6 +54,29 @@ static void arm_smmu_update_ctx_desc_devices(struct arm_smmu_domain *smmu_domain > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > } > > +static void > +arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain) > +{ > + struct arm_smmu_master *master; > + struct arm_smmu_cd target_cd; > + unsigned long flags; > + > + spin_lock_irqsave(&smmu_domain->devices_lock, flags); > + list_for_each_entry(master, &smmu_domain->devices, domain_head) { > + struct arm_smmu_cd *cdptr; > + > + /* S1 domains only support RID attachment right now */ > + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > + if (WARN_ON(!cdptr)) > + continue; > + > + arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); > + arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, > + &target_cd); > + } > + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > +} > + > /* > * Check if the CPU ASID is available on the SMMU side. If a private context > * descriptor is using it, try to replace it. > @@ -97,7 +120,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid) > * be some overlap between use of both ASIDs, until we invalidate the > * TLB. > */ > - arm_smmu_update_ctx_desc_devices(smmu_domain, IOMMU_NO_PASID, cd); > + arm_smmu_update_s1_domain_cd_entry(smmu_domain); Not to be too nitpicky, but is calling this arm_smmu_update_s1_domain_cd_entry the right choice here? Yes the RID domain has type "ARM_SMMU_DOMAIN_S1", but CDs are also stage 1 translations. > > /* Invalidate TLB entries previously associated with that context */ > arm_smmu_tlb_inv_asid(smmu, asid); > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 3fb4a1523d1d3f..e25dbb982feeee 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1222,8 +1222,8 @@ static void arm_smmu_write_cd_l1_desc(__le64 *dst, > WRITE_ONCE(*dst, cpu_to_le64(val)); > } > > -static struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, > - u32 ssid) > +struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, > + u32 ssid) > { > __le64 *l1ptr; > unsigned int idx; > @@ -1288,9 +1288,9 @@ static const struct arm_smmu_entry_writer_ops arm_smmu_cd_writer_ops = { > .num_entry_qwords = sizeof(struct arm_smmu_cd) / sizeof(u64), > }; > > -static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > - struct arm_smmu_cd *cdptr, > - const struct arm_smmu_cd *target) > +void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > + struct arm_smmu_cd *cdptr, > + const struct arm_smmu_cd *target) > { > struct arm_smmu_cd_writer cd_writer = { > .writer = { > @@ -1303,6 +1303,32 @@ static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data); > } > > +void arm_smmu_make_s1_cd(struct arm_smmu_cd *target, > + struct arm_smmu_master *master, > + struct arm_smmu_domain *smmu_domain) > +{ > + struct arm_smmu_ctx_desc *cd = &smmu_domain->cd; > + > + memset(target, 0, sizeof(*target)); > + > + target->data[0] = cpu_to_le64( > + cd->tcr | > +#ifdef __BIG_ENDIAN > + CTXDESC_CD_0_ENDI | > +#endif > + CTXDESC_CD_0_V | > + CTXDESC_CD_0_AA64 | > + (master->stall_enabled ? CTXDESC_CD_0_S : 0) | > + CTXDESC_CD_0_R | > + CTXDESC_CD_0_A | > + CTXDESC_CD_0_ASET | > + FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid) > + ); > + > + target->data[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK); > + target->data[3] = cpu_to_le64(cd->mair); > +} > + > void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid) > { > struct arm_smmu_cd target = {}; > @@ -2689,29 +2715,29 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > switch (smmu_domain->stage) { > - case ARM_SMMU_DOMAIN_S1: > + case ARM_SMMU_DOMAIN_S1: { > + struct arm_smmu_cd target_cd; > + struct arm_smmu_cd *cdptr; > + > if (!master->cd_table.cdtab) { > ret = arm_smmu_alloc_cd_tables(master); > if (ret) > 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) > - goto out_list_del; > } > > - ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd); > - if (ret) > + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > + if (!cdptr) { > + ret = -ENOMEM; > goto out_list_del; > + } > > + arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); > + arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, > + &target_cd); > 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); > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > index 87a7b57f566fbc..d32da11058aab6 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -750,6 +750,14 @@ extern struct mutex arm_smmu_asid_lock; > extern struct arm_smmu_ctx_desc quiet_cd; > > void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid); > +struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, > + u32 ssid); > +void arm_smmu_make_s1_cd(struct arm_smmu_cd *target, > + struct arm_smmu_master *master, > + struct arm_smmu_domain *smmu_domain); > +void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > + struct arm_smmu_cd *cdptr, > + const struct arm_smmu_cd *target); > > int arm_smmu_write_ctx_desc(struct arm_smmu_master *smmu_master, int ssid, > struct arm_smmu_ctx_desc *cd); > -- > 2.43.2 > Reviewed-by: Michael Shavit <mshavit@google.com>
On Wed, Mar 13, 2024 at 08:13:12PM +0800, Michael Shavit wrote: > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > index bb9bb6fd7914ce..6acc65f6d00a71 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > @@ -54,6 +54,29 @@ static void arm_smmu_update_ctx_desc_devices(struct arm_smmu_domain *smmu_domain > > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > } > > > > +static void > > +arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain) > > +{ > > + struct arm_smmu_master *master; > > + struct arm_smmu_cd target_cd; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&smmu_domain->devices_lock, flags); > > + list_for_each_entry(master, &smmu_domain->devices, domain_head) { > > + struct arm_smmu_cd *cdptr; > > + > > + /* S1 domains only support RID attachment right now */ > > + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > > + if (WARN_ON(!cdptr)) > > + continue; > > + > > + arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); > > + arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, > > + &target_cd); > > + } > > + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > +} > > + > > /* > > * Check if the CPU ASID is available on the SMMU side. If a private context > > * descriptor is using it, try to replace it. > > @@ -97,7 +120,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid) > > * be some overlap between use of both ASIDs, until we invalidate the > > * TLB. > > */ > > - arm_smmu_update_ctx_desc_devices(smmu_domain, IOMMU_NO_PASID, cd); > > + arm_smmu_update_s1_domain_cd_entry(smmu_domain); > > Not to be too nitpicky, but is calling this > arm_smmu_update_s1_domain_cd_entry the right choice here? Yes the RID > domain has type "ARM_SMMU_DOMAIN_S1", but CDs are also stage 1 > translations. A later patch will inline this function into arm_smmu_realloc_s1_domain_asid(), so I wouldn't fuss too much over the name.. But the name is trying to convey the two important details: 1) The domain is an ARM_SMMU_DOMAIN_S1 2) We are changing the CD Table Entry *not* the STE One of the things that was really confusing about this code was exactly what the smm_domain *was*, it is actually always a ARM_SMMU_DOMAIN_S1. Thanks, Jason
On Mon, Mar 04, 2024 at 07:43:55PM -0400, Jason Gunthorpe wrote: > Introduce arm_smmu_make_s1_cd() to build the CD from the paging S1 domain, > and reorganize all the places programming S1 domain CD table entries to > call it. > > Split arm_smmu_update_s1_domain_cd_entry() from > arm_smmu_update_ctx_desc_devices() so that the S1 path has its own call > chain separate from the unrelated SVA path. > > arm_smmu_update_s1_domain_cd_entry() only works on S1 domains > attached to RIDs and refreshes all their CDs. > > Remove the forced clear of the CD during S1 domain attach, > arm_smmu_write_cd_entry() will do this automatically if necessary. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 25 +++++++- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 60 +++++++++++++------ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 8 +++ > 3 files changed, 75 insertions(+), 18 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > index bb9bb6fd7914ce..6acc65f6d00a71 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > @@ -54,6 +54,29 @@ static void arm_smmu_update_ctx_desc_devices(struct arm_smmu_domain *smmu_domain > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > } > > +static void > +arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain) > +{ > + struct arm_smmu_master *master; > + struct arm_smmu_cd target_cd; > + unsigned long flags; > + > + spin_lock_irqsave(&smmu_domain->devices_lock, flags); > + list_for_each_entry(master, &smmu_domain->devices, domain_head) { > + struct arm_smmu_cd *cdptr; > + > + /* S1 domains only support RID attachment right now */ > + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > + if (WARN_ON(!cdptr)) This should never hit, no? Otherwise that means this path can allocate memory with a spinlock. > + continue; > + > + arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); > + arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, > + &target_cd); > + } > + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > +} > + > /* > * Check if the CPU ASID is available on the SMMU side. If a private context > * descriptor is using it, try to replace it. > @@ -97,7 +120,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid) > * be some overlap between use of both ASIDs, until we invalidate the > * TLB. > */ > - arm_smmu_update_ctx_desc_devices(smmu_domain, IOMMU_NO_PASID, cd); > + arm_smmu_update_s1_domain_cd_entry(smmu_domain); > > /* Invalidate TLB entries previously associated with that context */ > arm_smmu_tlb_inv_asid(smmu, asid); > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 3fb4a1523d1d3f..e25dbb982feeee 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1222,8 +1222,8 @@ static void arm_smmu_write_cd_l1_desc(__le64 *dst, > WRITE_ONCE(*dst, cpu_to_le64(val)); > } > > -static struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, > - u32 ssid) > +struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, > + u32 ssid) > { > __le64 *l1ptr; > unsigned int idx; > @@ -1288,9 +1288,9 @@ static const struct arm_smmu_entry_writer_ops arm_smmu_cd_writer_ops = { > .num_entry_qwords = sizeof(struct arm_smmu_cd) / sizeof(u64), > }; > > -static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > - struct arm_smmu_cd *cdptr, > - const struct arm_smmu_cd *target) > +void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > + struct arm_smmu_cd *cdptr, > + const struct arm_smmu_cd *target) > { > struct arm_smmu_cd_writer cd_writer = { > .writer = { > @@ -1303,6 +1303,32 @@ static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data); > } > > +void arm_smmu_make_s1_cd(struct arm_smmu_cd *target, > + struct arm_smmu_master *master, > + struct arm_smmu_domain *smmu_domain) > +{ > + struct arm_smmu_ctx_desc *cd = &smmu_domain->cd; > + > + memset(target, 0, sizeof(*target)); > + > + target->data[0] = cpu_to_le64( > + cd->tcr | > +#ifdef __BIG_ENDIAN > + CTXDESC_CD_0_ENDI | > +#endif > + CTXDESC_CD_0_V | > + CTXDESC_CD_0_AA64 | > + (master->stall_enabled ? CTXDESC_CD_0_S : 0) | > + CTXDESC_CD_0_R | > + CTXDESC_CD_0_A | > + CTXDESC_CD_0_ASET | > + FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid) > + ); > + > + target->data[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK); > + target->data[3] = cpu_to_le64(cd->mair); > +} > + That seems to duplicate some logic from arm_smmu_write_ctx_desc(), can that be consolidated? > void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid) > { > struct arm_smmu_cd target = {}; > @@ -2689,29 +2715,29 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > switch (smmu_domain->stage) { > - case ARM_SMMU_DOMAIN_S1: > + case ARM_SMMU_DOMAIN_S1: { > + struct arm_smmu_cd target_cd; > + struct arm_smmu_cd *cdptr; > + > if (!master->cd_table.cdtab) { > ret = arm_smmu_alloc_cd_tables(master); > if (ret) > 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) > - goto out_list_del; > } > > - ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd); > - if (ret) > + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > + if (!cdptr) { > + ret = -ENOMEM; > goto out_list_del; > + } > > + arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); > + arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, > + &target_cd); > 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); > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > index 87a7b57f566fbc..d32da11058aab6 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -750,6 +750,14 @@ extern struct mutex arm_smmu_asid_lock; > extern struct arm_smmu_ctx_desc quiet_cd; > > void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid); > +struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, > + u32 ssid); > +void arm_smmu_make_s1_cd(struct arm_smmu_cd *target, > + struct arm_smmu_master *master, > + struct arm_smmu_domain *smmu_domain); > +void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > + struct arm_smmu_cd *cdptr, > + const struct arm_smmu_cd *target); > > int arm_smmu_write_ctx_desc(struct arm_smmu_master *smmu_master, int ssid, > struct arm_smmu_ctx_desc *cd); > -- > 2.43.2 > Thanks, Mostafa
On Sat, Mar 23, 2024 at 01:11:49PM +0000, Mostafa Saleh wrote: > On Mon, Mar 04, 2024 at 07:43:55PM -0400, Jason Gunthorpe wrote: > > Introduce arm_smmu_make_s1_cd() to build the CD from the paging S1 domain, > > and reorganize all the places programming S1 domain CD table entries to > > call it. > > > > Split arm_smmu_update_s1_domain_cd_entry() from > > arm_smmu_update_ctx_desc_devices() so that the S1 path has its own call > > chain separate from the unrelated SVA path. > > > > arm_smmu_update_s1_domain_cd_entry() only works on S1 domains > > attached to RIDs and refreshes all their CDs. > > > > Remove the forced clear of the CD during S1 domain attach, > > arm_smmu_write_cd_entry() will do this automatically if necessary. > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 25 +++++++- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 60 +++++++++++++------ > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 8 +++ > > 3 files changed, 75 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > index bb9bb6fd7914ce..6acc65f6d00a71 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c > > @@ -54,6 +54,29 @@ static void arm_smmu_update_ctx_desc_devices(struct arm_smmu_domain *smmu_domain > > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > } > > > > +static void > > +arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain) > > +{ > > + struct arm_smmu_master *master; > > + struct arm_smmu_cd target_cd; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&smmu_domain->devices_lock, flags); > > + list_for_each_entry(master, &smmu_domain->devices, domain_head) { > > + struct arm_smmu_cd *cdptr; > > + > > + /* S1 domains only support RID attachment right now */ > > + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > > + if (WARN_ON(!cdptr)) > > This should never hit, no? Otherwise that means this path can allocate memory > with a spinlock. Right, WARN_ON's should never be hit. > > +void arm_smmu_make_s1_cd(struct arm_smmu_cd *target, > > + struct arm_smmu_master *master, > > + struct arm_smmu_domain *smmu_domain) > > +{ > > + struct arm_smmu_ctx_desc *cd = &smmu_domain->cd; > > + > > + memset(target, 0, sizeof(*target)); > > + > > + target->data[0] = cpu_to_le64( > > + cd->tcr | > > +#ifdef __BIG_ENDIAN > > + CTXDESC_CD_0_ENDI | > > +#endif > > + CTXDESC_CD_0_V | > > + CTXDESC_CD_0_AA64 | > > + (master->stall_enabled ? CTXDESC_CD_0_S : 0) | > > + CTXDESC_CD_0_R | > > + CTXDESC_CD_0_A | > > + CTXDESC_CD_0_ASET | > > + FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid) > > + ); > > + > > + target->data[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK); > > + target->data[3] = cpu_to_le64(cd->mair); > > +} > > + > > That seems to duplicate some logic from arm_smmu_write_ctx_desc(), > can that be consolidated? Yes, it is consolidated by deleting arm_smmu_write_ctx_desc() in a few more patches. Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c index bb9bb6fd7914ce..6acc65f6d00a71 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c @@ -54,6 +54,29 @@ static void arm_smmu_update_ctx_desc_devices(struct arm_smmu_domain *smmu_domain spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); } +static void +arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain) +{ + struct arm_smmu_master *master; + struct arm_smmu_cd target_cd; + unsigned long flags; + + spin_lock_irqsave(&smmu_domain->devices_lock, flags); + list_for_each_entry(master, &smmu_domain->devices, domain_head) { + struct arm_smmu_cd *cdptr; + + /* S1 domains only support RID attachment right now */ + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); + if (WARN_ON(!cdptr)) + continue; + + arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); + arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, + &target_cd); + } + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); +} + /* * Check if the CPU ASID is available on the SMMU side. If a private context * descriptor is using it, try to replace it. @@ -97,7 +120,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid) * be some overlap between use of both ASIDs, until we invalidate the * TLB. */ - arm_smmu_update_ctx_desc_devices(smmu_domain, IOMMU_NO_PASID, cd); + arm_smmu_update_s1_domain_cd_entry(smmu_domain); /* Invalidate TLB entries previously associated with that context */ arm_smmu_tlb_inv_asid(smmu, asid); diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 3fb4a1523d1d3f..e25dbb982feeee 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1222,8 +1222,8 @@ static void arm_smmu_write_cd_l1_desc(__le64 *dst, WRITE_ONCE(*dst, cpu_to_le64(val)); } -static struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, - u32 ssid) +struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, + u32 ssid) { __le64 *l1ptr; unsigned int idx; @@ -1288,9 +1288,9 @@ static const struct arm_smmu_entry_writer_ops arm_smmu_cd_writer_ops = { .num_entry_qwords = sizeof(struct arm_smmu_cd) / sizeof(u64), }; -static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, - struct arm_smmu_cd *cdptr, - const struct arm_smmu_cd *target) +void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, + struct arm_smmu_cd *cdptr, + const struct arm_smmu_cd *target) { struct arm_smmu_cd_writer cd_writer = { .writer = { @@ -1303,6 +1303,32 @@ static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data); } +void arm_smmu_make_s1_cd(struct arm_smmu_cd *target, + struct arm_smmu_master *master, + struct arm_smmu_domain *smmu_domain) +{ + struct arm_smmu_ctx_desc *cd = &smmu_domain->cd; + + memset(target, 0, sizeof(*target)); + + target->data[0] = cpu_to_le64( + cd->tcr | +#ifdef __BIG_ENDIAN + CTXDESC_CD_0_ENDI | +#endif + CTXDESC_CD_0_V | + CTXDESC_CD_0_AA64 | + (master->stall_enabled ? CTXDESC_CD_0_S : 0) | + CTXDESC_CD_0_R | + CTXDESC_CD_0_A | + CTXDESC_CD_0_ASET | + FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid) + ); + + target->data[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK); + target->data[3] = cpu_to_le64(cd->mair); +} + void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid) { struct arm_smmu_cd target = {}; @@ -2689,29 +2715,29 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); switch (smmu_domain->stage) { - case ARM_SMMU_DOMAIN_S1: + case ARM_SMMU_DOMAIN_S1: { + struct arm_smmu_cd target_cd; + struct arm_smmu_cd *cdptr; + if (!master->cd_table.cdtab) { ret = arm_smmu_alloc_cd_tables(master); if (ret) 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) - goto out_list_del; } - ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd); - if (ret) + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); + if (!cdptr) { + ret = -ENOMEM; goto out_list_del; + } + arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); + arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, + &target_cd); 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); diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 87a7b57f566fbc..d32da11058aab6 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -750,6 +750,14 @@ extern struct mutex arm_smmu_asid_lock; extern struct arm_smmu_ctx_desc quiet_cd; void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid); +struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, + u32 ssid); +void arm_smmu_make_s1_cd(struct arm_smmu_cd *target, + struct arm_smmu_master *master, + struct arm_smmu_domain *smmu_domain); +void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, + struct arm_smmu_cd *cdptr, + const struct arm_smmu_cd *target); int arm_smmu_write_ctx_desc(struct arm_smmu_master *smmu_master, int ssid, struct arm_smmu_ctx_desc *cd);