| Message ID | 15-v2-de8b10590bf5+400-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 |
On Tue, Nov 14, 2023 at 1:53 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > Move to the new static global for identity domains. Move all the logic out > of arm_smmu_attach_dev into an identity only function. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Michael Shavit <mshavit@google.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 23dda64722ea17..d6f68a6187d290 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2174,8 +2174,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; > > /* > @@ -2283,11 +2282,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; > @@ -2484,7 +2478,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); > @@ -2547,15 +2541,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); > @@ -2592,13 +2578,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); > @@ -2614,6 +2593,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) > @@ -3006,6 +3039,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 21f2f73501019a..154808f96718df 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 { > -- > 2.42.0 > On Tue, Nov 14, 2023 at 1:53 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > Move to the new static global for identity domains. Move all the logic out > of arm_smmu_attach_dev into an identity only function. > > 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 23dda64722ea17..d6f68a6187d290 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2174,8 +2174,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; > > /* > @@ -2283,11 +2282,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; > @@ -2484,7 +2478,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); > @@ -2547,15 +2541,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); > @@ -2592,13 +2578,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); > @@ -2614,6 +2593,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) > @@ -3006,6 +3039,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 21f2f73501019a..154808f96718df 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 { > -- > 2.42.0 >
On Mon, Nov 13, 2023 at 01:53:22PM -0400, Jason Gunthorpe wrote: > @@ -2592,13 +2578,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); > @@ -2614,6 +2593,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); This arm_smmu_write_ctx_desc was previously within the asid lock protection, yet now it's moved out of that?
On Mon, Dec 04, 2023 at 08:28:23PM -0800, Nicolin Chen wrote: > > +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); > > This arm_smmu_write_ctx_desc was previously within the asid lock > protection, yet now it's moved out of that? Yes, arm_smmu_write_ctx_desc() updates a CD table entry and that does not need ASID lock protection. The ASID lock exists because of the BTM code rewriting STEs asyncronously. Jason
On Tue, Dec 05, 2023 at 10:37:42AM -0400, Jason Gunthorpe wrote: > On Mon, Dec 04, 2023 at 08:28:23PM -0800, Nicolin Chen wrote: > > > > +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); > > > > This arm_smmu_write_ctx_desc was previously within the asid lock > > protection, yet now it's moved out of that? > > Yes, arm_smmu_write_ctx_desc() updates a CD table entry and that does > not need ASID lock protection. The ASID lock exists because of the BTM > code rewriting STEs asyncronously. I see. Thanks for elaborating. For this patch: Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
On Tue, Dec 05, 2023 at 09:25:59AM -0800, Nicolin Chen wrote: > On Tue, Dec 05, 2023 at 10:37:42AM -0400, Jason Gunthorpe wrote: > > On Mon, Dec 04, 2023 at 08:28:23PM -0800, Nicolin Chen wrote: > > > > > > +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); > > > > > > This arm_smmu_write_ctx_desc was previously within the asid lock > > > protection, yet now it's moved out of that? > > > > Yes, arm_smmu_write_ctx_desc() updates a CD table entry and that does > > not need ASID lock protection. The ASID lock exists because of the BTM > > code rewriting STEs asyncronously. > > I see. Thanks for elaborating. For this patch Actually wait, that explanation is not right.. The BTM code is changing the ASID which is done with a CD update The ordering is OK here because the BTM code iterates over the &smmu_domain->devices list. The arm_smmu_detach_dev() has removed the master from the devices list under a lock so the BTM code won't see this. Thus there is no race between the arm_smmu_share_asid() flow and this code, indeed we've already removed the cdtable from the STE at this point. Jason
On Tue, Dec 05, 2023 at 01:42:19PM -0400, Jason Gunthorpe wrote: > On Tue, Dec 05, 2023 at 09:25:59AM -0800, Nicolin Chen wrote: > > On Tue, Dec 05, 2023 at 10:37:42AM -0400, Jason Gunthorpe wrote: > > > On Mon, Dec 04, 2023 at 08:28:23PM -0800, Nicolin Chen wrote: > > > > > > > > +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); > > > > > > > > This arm_smmu_write_ctx_desc was previously within the asid lock > > > > protection, yet now it's moved out of that? > > > > > > Yes, arm_smmu_write_ctx_desc() updates a CD table entry and that does > > > not need ASID lock protection. The ASID lock exists because of the BTM > > > code rewriting STEs asyncronously. > > > > I see. Thanks for elaborating. For this patch > > Actually wait, that explanation is not right.. > > The BTM code is changing the ASID which is done with a CD update > > The ordering is OK here because the BTM code iterates over the > &smmu_domain->devices list. > > The arm_smmu_detach_dev() has removed the master from the devices list > under a lock so the BTM code won't see this. > > Thus there is no race between the arm_smmu_share_asid() flow and this > code, indeed we've already removed the cdtable from the STE at this > point. I see. Maybe worth mentioning this in the comments above or commit message? Also, the arm_smmu_attach_dev_ste seems to be used by IDENTITY and BLOCK domains only. This turns its nature to be a cleanup function against a translate domain, while the naming sounds very generic. I cannot think of any better name than this one, yet maybe we can highlight this with a line of comments above the function? Nicolin
On Tue, Dec 05, 2023 at 10:21:18AM -0800, Nicolin Chen wrote: > On Tue, Dec 05, 2023 at 01:42:19PM -0400, Jason Gunthorpe wrote: > > On Tue, Dec 05, 2023 at 09:25:59AM -0800, Nicolin Chen wrote: > > > On Tue, Dec 05, 2023 at 10:37:42AM -0400, Jason Gunthorpe wrote: > > > > On Mon, Dec 04, 2023 at 08:28:23PM -0800, Nicolin Chen wrote: > > > > > > > > > > +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); > > > > > > > > > > This arm_smmu_write_ctx_desc was previously within the asid lock > > > > > protection, yet now it's moved out of that? > > > > > > > > Yes, arm_smmu_write_ctx_desc() updates a CD table entry and that does > > > > not need ASID lock protection. The ASID lock exists because of the BTM > > > > code rewriting STEs asyncronously. > > > > > > I see. Thanks for elaborating. For this patch > > > > Actually wait, that explanation is not right.. > > > > The BTM code is changing the ASID which is done with a CD update > > > > The ordering is OK here because the BTM code iterates over the > > &smmu_domain->devices list. > > > > The arm_smmu_detach_dev() has removed the master from the devices list > > under a lock so the BTM code won't see this. > > > > Thus there is no race between the arm_smmu_share_asid() flow and this > > code, indeed we've already removed the cdtable from the STE at this > > point. > > I see. Maybe worth mentioning this in the comments above or commit > message? It does have a comment: + /* + * 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(). + */ > Also, the arm_smmu_attach_dev_ste seems to be used by IDENTITY and > BLOCK domains only. This turns its nature to be a cleanup function > against a translate domain, while the naming sounds very generic. > I cannot think of any better name than this one, yet maybe we can > highlight this with a line of comments above the function? I would not call it a cleanup, it installs a domain-less thing via a raw STE Jason
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 23dda64722ea17..d6f68a6187d290 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2174,8 +2174,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; /* @@ -2283,11 +2282,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; @@ -2484,7 +2478,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); @@ -2547,15 +2541,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); @@ -2592,13 +2578,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); @@ -2614,6 +2593,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) @@ -3006,6 +3039,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 21f2f73501019a..154808f96718df 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 {
Move to the new static global for identity domains. Move all the logic out of arm_smmu_attach_dev into an identity only function. 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(-)