| Message ID | 12-v4-c93b774edcc4+42d2b-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update SMMUv3 to the modern iommu API (part 1/3) | expand |
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, January 25, 2024 11:57 PM > To: iommu@lists.linux.dev; Joerg Roedel <joro@8bytes.org>; linux-arm- > kernel@lists.infradead.org; Robin Murphy <robin.murphy@arm.com>; Will > Deacon <will@kernel.org> > Cc: Moritz Fischer <mdf@kernel.org>; Moritz Fischer <moritzf@google.com>; > Michael Shavit <mshavit@google.com>; Nicolin Chen <nicolinc@nvidia.com>; > patches@lists.linux.dev; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com> > Subject: [PATCH v4 12/16] iommu/arm-smmu-v3: Add a global static > IDENTITY domain > > Move to the new static global for identity domains. Move all the logic out > of arm_smmu_attach_dev into an identity only function. > > Reviewed-by: Michael Shavit <mshavit@google.com> > Reviewed-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Moritz Fischer <moritzf@google.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 82 +++++++++++++++-- > ---- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 - > 2 files changed, 58 insertions(+), 25 deletions(-) > > 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 f08cfa9b90b3eb..d35bf9655c9b1b 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2226,8 +2226,7 @@ static struct iommu_domain > *arm_smmu_domain_alloc(unsigned type) > return arm_smmu_sva_domain_alloc(); > > if (type != IOMMU_DOMAIN_UNMANAGED && > - type != IOMMU_DOMAIN_DMA && > - type != IOMMU_DOMAIN_IDENTITY) > + type != IOMMU_DOMAIN_DMA) > return NULL; > > /* > @@ -2335,11 +2334,6 @@ static int arm_smmu_domain_finalise(struct > iommu_domain *domain) > struct arm_smmu_domain *smmu_domain = > to_smmu_domain(domain); > struct arm_smmu_device *smmu = smmu_domain->smmu; > > - if (domain->type == IOMMU_DOMAIN_IDENTITY) { > - smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS; > - return 0; > - } > - > /* Restrict the stage to what we can actually support */ > if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1)) > smmu_domain->stage = ARM_SMMU_DOMAIN_S2; > @@ -2537,7 +2531,7 @@ static void arm_smmu_detach_dev(struct > arm_smmu_master *master) > struct arm_smmu_domain *smmu_domain; > unsigned long flags; > > - if (!domain) > + if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING)) > return; > > smmu_domain = to_smmu_domain(domain); > @@ -2600,15 +2594,7 @@ static int arm_smmu_attach_dev(struct > iommu_domain *domain, struct device *dev) > > arm_smmu_detach_dev(master); > > - /* > - * The SMMU does not support enabling ATS with bypass. When the > STE is > - * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and > - * Translated transactions are denied as though ATS is disabled for > the > - * stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and > - * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry). > - */ > - if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS) > - master->ats_enabled = arm_smmu_ats_supported(master); > + master->ats_enabled = arm_smmu_ats_supported(master); > > spin_lock_irqsave(&smmu_domain->devices_lock, flags); > list_add(&master->domain_head, &smmu_domain->devices); > @@ -2645,13 +2631,6 @@ static int arm_smmu_attach_dev(struct > iommu_domain *domain, struct device *dev) > 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; > } > > arm_smmu_enable_ats(master, smmu_domain); > @@ -2667,6 +2646,60 @@ static int arm_smmu_attach_dev(struct > iommu_domain *domain, struct device *dev) > return ret; > } > > +static int arm_smmu_attach_dev_ste(struct device *dev, > + struct arm_smmu_ste *ste) > +{ > + struct arm_smmu_master *master = dev_iommu_priv_get(dev); > + > + if (arm_smmu_master_sva_enabled(master)) > + return -EBUSY; > + > + /* > + * Do not allow any ASID to be changed while are working on the STE, > + * otherwise we could miss invalidations. > + */ > + mutex_lock(&arm_smmu_asid_lock); > + > + /* > + * The SMMU does not support enabling ATS with bypass/abort. > When the > + * STE is in bypass (STE.Config[2:0] == 0b100), ATS Translation > Requests > + * and Translated transactions are denied as though ATS is disabled > for > + * the stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and > + * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry). > + */ > + arm_smmu_detach_dev(master); > + > + arm_smmu_install_ste_for_dev(master, ste); > + mutex_unlock(&arm_smmu_asid_lock); > + > + /* > + * This has to be done after removing the master from the > + * arm_smmu_domain->devices to avoid races updating the same > context > + * descriptor from arm_smmu_share_asid(). > + */ > + if (master->cd_table.cdtab) > + arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, > NULL); > + return 0; > +} > + > +static int arm_smmu_attach_dev_identity(struct iommu_domain *domain, > + struct device *dev) > +{ > + struct arm_smmu_ste ste; > + > + arm_smmu_make_bypass_ste(&ste); > + return arm_smmu_attach_dev_ste(dev, &ste); > +} > + > +static const struct iommu_domain_ops arm_smmu_identity_ops = { > + .attach_dev = arm_smmu_attach_dev_identity, > +}; > + > +static struct iommu_domain arm_smmu_identity_domain = { > + .type = IOMMU_DOMAIN_IDENTITY, > + .ops = &arm_smmu_identity_ops, > +}; > + > static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned > long iova, > phys_addr_t paddr, size_t pgsize, size_t pgcount, > int prot, gfp_t gfp, size_t *mapped) > @@ -3056,6 +3089,7 @@ static void arm_smmu_remove_dev_pasid(struct > device *dev, ioasid_t pasid) > } > > static struct iommu_ops arm_smmu_ops = { > + .identity_domain = &arm_smmu_identity_domain, This seems to create a problem when we have set the identity domain and try to enable sva for the device. Since there is no smmu_domain for this case and there is no specific domain type checking in iommu_sva_bind_device() path, it eventually crashes(hangs in my test) in, iommu_sva_bind_device() ... arm_smmu_sva_set_dev_pasid() __arm_smmu_sva_bind() arm_smmu_mmu_notifier_get(smmu_domain, ..) --> never exit the mmu notifier list loop. I think we should check for the domain type in iommu_sva_bind_device() or later before trying to use smmu_domain. At present(ie, without this series) it returns error while we are trying to write the CD. But that looks too late as well. Thanks, Shameer
On Mon, Jan 29, 2024 at 06:11:48PM +0000, Shameerali Kolothum Thodi wrote: > > @@ -3056,6 +3089,7 @@ static void arm_smmu_remove_dev_pasid(struct > > device *dev, ioasid_t pasid) > > } > > > > static struct iommu_ops arm_smmu_ops = { > > + .identity_domain = &arm_smmu_identity_domain, > > This seems to create a problem when we have set the identity domain and > try to enable sva for the device. Since there is no smmu_domain for this case > and there is no specific domain type checking in iommu_sva_bind_device() path, > it eventually crashes(hangs in my test) in, Yeah, that is a longstanding issue in the SVA implementation, it only works if the RID is set to a S1 paging domain. I cleaned it up here so that the SVA series was cleaer: https://lore.kernel.org/linux-iommu/1-v4-e7091cdd9e8d+43b1-smmuv3_newapi_p2_jgg@nvidia.com/ > iommu_sva_bind_device() > ... > arm_smmu_sva_set_dev_pasid() > __arm_smmu_sva_bind() > arm_smmu_mmu_notifier_get(smmu_domain, ..) --> never exit the mmu notifier list loop. > > I think we should check for the domain type in iommu_sva_bind_device() or later > before trying to use smmu_domain. At present(ie, without this series) it returns error > while we are trying to write the CD. But that looks too late as well. Oh wow, is that how it worked? OK, I figured it was just broken but if there was some error code that happened indirectly then lets've move the above patch ahead of this one. Thanks, Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Monday, January 29, 2024 6:38 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: iommu@lists.linux.dev; Joerg Roedel <joro@8bytes.org>; linux-arm- > kernel@lists.infradead.org; Robin Murphy <robin.murphy@arm.com>; Will > Deacon <will@kernel.org>; Moritz Fischer <mdf@kernel.org>; Moritz Fischer > <moritzf@google.com>; Michael Shavit <mshavit@google.com>; Nicolin Chen > <nicolinc@nvidia.com>; patches@lists.linux.dev > Subject: Re: [PATCH v4 12/16] iommu/arm-smmu-v3: Add a global static > IDENTITY domain > > On Mon, Jan 29, 2024 at 06:11:48PM +0000, Shameerali Kolothum Thodi > wrote: > > > > @@ -3056,6 +3089,7 @@ static void > arm_smmu_remove_dev_pasid(struct > > > device *dev, ioasid_t pasid) > > > } > > > > > > static struct iommu_ops arm_smmu_ops = { > > > + .identity_domain = &arm_smmu_identity_domain, > > > > This seems to create a problem when we have set the identity domain and > > try to enable sva for the device. Since there is no smmu_domain for this > case > > and there is no specific domain type checking in iommu_sva_bind_device() > path, > > it eventually crashes(hangs in my test) in, > > Yeah, that is a longstanding issue in the SVA implementation, it only > works if the RID is set to a S1 paging domain. > > I cleaned it up here so that the SVA series was cleaer: > > https://lore.kernel.org/linux-iommu/1-v4-e7091cdd9e8d+43b1- > smmuv3_newapi_p2_jgg@nvidia.com/ Yes, this will do. But I think it is not complete. I will comment on that one. > > > iommu_sva_bind_device() > > ... > > arm_smmu_sva_set_dev_pasid() > > __arm_smmu_sva_bind() > > arm_smmu_mmu_notifier_get(smmu_domain, ..) --> never exit the > mmu notifier list loop. > > > > I think we should check for the domain type in iommu_sva_bind_device() > or later > > before trying to use smmu_domain. At present(ie, without this series) it > returns error > > while we are trying to write the CD. But that looks too late as well. > > Oh wow, is that how it worked? OK, I figured it was just broken but if > there was some error code that happened indirectly then lets've move > the above patch ahead of this one. Yes, indirectly indeed :) https://elixir.bootlin.com/linux/v6.8-rc2/source/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c#L1068 Thanks, Shameer
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 f08cfa9b90b3eb..d35bf9655c9b1b 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2226,8 +2226,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) return arm_smmu_sva_domain_alloc(); if (type != IOMMU_DOMAIN_UNMANAGED && - type != IOMMU_DOMAIN_DMA && - type != IOMMU_DOMAIN_IDENTITY) + type != IOMMU_DOMAIN_DMA) return NULL; /* @@ -2335,11 +2334,6 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain) struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_device *smmu = smmu_domain->smmu; - if (domain->type == IOMMU_DOMAIN_IDENTITY) { - smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS; - return 0; - } - /* Restrict the stage to what we can actually support */ if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1)) smmu_domain->stage = ARM_SMMU_DOMAIN_S2; @@ -2537,7 +2531,7 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master) struct arm_smmu_domain *smmu_domain; unsigned long flags; - if (!domain) + if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING)) return; smmu_domain = to_smmu_domain(domain); @@ -2600,15 +2594,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) arm_smmu_detach_dev(master); - /* - * The SMMU does not support enabling ATS with bypass. When the STE is - * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and - * Translated transactions are denied as though ATS is disabled for the - * stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and - * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry). - */ - if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS) - master->ats_enabled = arm_smmu_ats_supported(master); + master->ats_enabled = arm_smmu_ats_supported(master); spin_lock_irqsave(&smmu_domain->devices_lock, flags); list_add(&master->domain_head, &smmu_domain->devices); @@ -2645,13 +2631,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) 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; } arm_smmu_enable_ats(master, smmu_domain); @@ -2667,6 +2646,60 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) return ret; } +static int arm_smmu_attach_dev_ste(struct device *dev, + struct arm_smmu_ste *ste) +{ + struct arm_smmu_master *master = dev_iommu_priv_get(dev); + + if (arm_smmu_master_sva_enabled(master)) + return -EBUSY; + + /* + * Do not allow any ASID to be changed while are working on the STE, + * otherwise we could miss invalidations. + */ + mutex_lock(&arm_smmu_asid_lock); + + /* + * The SMMU does not support enabling ATS with bypass/abort. When the + * STE is in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests + * and Translated transactions are denied as though ATS is disabled for + * the stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and + * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry). + */ + arm_smmu_detach_dev(master); + + arm_smmu_install_ste_for_dev(master, ste); + mutex_unlock(&arm_smmu_asid_lock); + + /* + * This has to be done after removing the master from the + * arm_smmu_domain->devices to avoid races updating the same context + * descriptor from arm_smmu_share_asid(). + */ + if (master->cd_table.cdtab) + arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, NULL); + return 0; +} + +static int arm_smmu_attach_dev_identity(struct iommu_domain *domain, + struct device *dev) +{ + struct arm_smmu_ste ste; + + arm_smmu_make_bypass_ste(&ste); + return arm_smmu_attach_dev_ste(dev, &ste); +} + +static const struct iommu_domain_ops arm_smmu_identity_ops = { + .attach_dev = arm_smmu_attach_dev_identity, +}; + +static struct iommu_domain arm_smmu_identity_domain = { + .type = IOMMU_DOMAIN_IDENTITY, + .ops = &arm_smmu_identity_ops, +}; + static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova, phys_addr_t paddr, size_t pgsize, size_t pgcount, int prot, gfp_t gfp, size_t *mapped) @@ -3056,6 +3089,7 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid) } static struct iommu_ops arm_smmu_ops = { + .identity_domain = &arm_smmu_identity_domain, .capable = arm_smmu_capable, .domain_alloc = arm_smmu_domain_alloc, .probe_device = arm_smmu_probe_device, 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 6b63ea7dae72da..23baf117e7e4b5 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -712,7 +712,6 @@ struct arm_smmu_master { enum arm_smmu_domain_stage { ARM_SMMU_DOMAIN_S1 = 0, ARM_SMMU_DOMAIN_S2, - ARM_SMMU_DOMAIN_BYPASS, }; struct arm_smmu_domain {