Re: [PATCH v5 16/27] iommu/arm-smmu-v3: Keep track of valid CD entries in the cd_table

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Mar 19, 2024 at 09:55:17PM +0800, Michael Shavit wrote:
> On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > We no longer need a master->sva_enable to control what attaches are
> > allowed.
> >
> > Instead keep track inside the cd_table how many valid CD entries exist,
> > and if the RID has a valid entry.
> >
> > Replace all the attach focused master->sva_enabled tests with a check if
> > the CD has valid entries (or not). If there are any valid entries then the
> > CD table must be currently programmed to the STE.
> >
> > 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   |  5 +---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 26 ++++++++++---------
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 10 +++++++
> >  3 files changed, 25 insertions(+), 16 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 ab9de8e36c45f5..82b9c4d4061c3d 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
> > @@ -433,9 +433,6 @@ static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
> >         if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> >                 return -ENODEV;
> >
> I assume this doesn't matter because of subsequent patches, but the
> check above could also be removed since used_sid precisely means that
> the attached domain is an ARM_SMMU_DOMAIN_S1 domain.

Right, but lets move the delete here for clarity.

The same comment applies to some later patches too that do:

	if (!arm_smmu_is_s1_domain(iommu_get_domain_for_dev(master->dev)) ||
	    !master->cd_table.used_sid)
		return -ENODEV;

> > 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 98dc5885c48655..7e1f6af4ce4e79 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -602,11 +602,21 @@ struct arm_smmu_ctx_desc_cfg {
> >         dma_addr_t                      cdtab_dma;
> >         struct arm_smmu_l1_ctx_desc     *l1_desc;
> >         unsigned int                    num_l1_ents;
> > +       unsigned int                    used_ssids;
> > +       bool                            used_sid;
> 
> This probably deserves a comment. There's plenty of places where the
> "rid" domain is handled as the CD with ssid 0; but we don't count it
> as a used_ssid here.

As a page table? I didn't think so, the only way to get a CD page table
installed is through arm_smmu_write_cd_entry() which will capture
this..

Non paging domains don't get captured here, they are translating the
RID but they are not using the CD table.

> I also don't find the meaning of used_sid obvious, especially if I
> didn't have the context from the commit description.

Hum, okay, so looking over all of this again I think we can
simplify. At the end there was only one place using used_sid and it
can instead be calling arm_smmu_ssids_in_use() directly:

@@ -2987,11 +2986,13 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
         * When the last user of the CD table goes away downgrade the STE back
         * to a non-cd_table one.
         */
-       if (last_ssid && !master->cd_table.used_sid) {
+       if (!arm_smmu_ssids_in_use(&master->cd_table)) {
                struct iommu_domain *sid_domain =
                        iommu_get_domain_for_dev(master->dev);
 
-               sid_domain->ops->attach_dev(sid_domain, master->dev);
+               if (domain->type == IOMMU_DOMAIN_IDENTITY ||
+                   domain->type == IOMMU_DOMAIN_BLOCKED)
+                       sid_domain->ops->attach_dev(sid_domain, dev);
        }

Then we can get rid of used_sid and just have used_ssids count the
!0 ssids directly.

I reorganized a bunch of things in the in between patches so we go
more directly to this final outcome.

Thanks,
Jason