Re: [PATCH 11/21] iommu/amd: Clean up and enhance protection domain flags

[Jason Gunthorpe <jgg@ziepe.ca>]

On Sun, Jul 16, 2023 at 08:22:37PM -0500, Suthikulpanit, Suravee wrote:
> Jason,
> 
> On 7/14/2023 12:25 PM, Jason Gunthorpe wrote:
> > On Wed, Jul 12, 2023 at 02:15:06PM +0000, Vasant Hegde wrote:
> > > From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > > 
> > > With IOMMU v1 and v2 page tables per domain, and the v2 table can
> > > be used in various modes, keeping track of how a particular domain
> > > is configured becoming cumbersome.
> > > 
> > > Enhance protection_domain.flags to keep track of how a domain is
> > > configured. Also Remove unused flags, and rename them accordingly.
> > > 
> > > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > > Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> > > Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> > > ---
> > >   drivers/iommu/amd/amd_iommu_types.h | 14 +++++---------
> > >   drivers/iommu/amd/io_pgtable_v2.c   |  2 +-
> > >   drivers/iommu/amd/iommu.c           | 21 ++++++++++++---------
> > >   3 files changed, 18 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> > > index 346c32343417..907af6e41ae9 100644
> > > --- a/drivers/iommu/amd/amd_iommu_types.h
> > > +++ b/drivers/iommu/amd/amd_iommu_types.h
> > > @@ -443,13 +443,11 @@
> > >   #define MAX_DOMAIN_ID 65536
> > >   /* Protection domain flags */
> > > -#define PD_DMA_OPS_MASK		BIT(0) /* domain used for dma_ops */
> > > -#define PD_DEFAULT_MASK		BIT(1) /* domain is a default dma_ops
> > > -					      domain for an IOMMU */
> > > -#define PD_PASSTHROUGH_MASK	BIT(2) /* domain has no page
> > > -					      translation */
> > > -#define PD_IOMMUV2_MASK		BIT(3) /* domain has gcr3 table */
> > > -#define PD_GIOV_MASK		BIT(4) /* domain enable GIOV support */
> > 
> > > +#define PD_FLAG_V1DMA		BIT(1) /* DMA-API mode w/ v1 page table */
> > > +#define PD_FLAG_V2DMA		BIT(2) /* DMA-API mode w/ v2 page table */
> > 
> > "DMA-API" should not be part of the driver vocabulary. Does this have
> > anything to do with DMA-API?
> > 
> > Only "paging" domains should have IO page tables and an IO page table
> > type.
> > 
> > Maybe this is just misnamed?
> 
> These flags are for struct protection_domain in AMD IOMMU driver, and not
> for the struct iommu_domain.

I understand that

> PD_FLAG_V[1|2]DMA flags are used to signify that the domain is using v1 or
> v2 page table to implement IOMMU_DOMAIN_DMA (which has __IOMMU_DOMAIN_PAGING
> | __IOMMU_DOMAIN_DMA_API).

Again, no, drivers should have no sensitivity to
__IOMMU_DOMAIN_DMA_API, we are trying to get rid of this. Please don't
add more to the driver.

> > Also, shouldn't it be an enum not flags?
> 
> We use flags instead of enum because we exepect to have combination of the
> flags. For example
> * IOMMU_DOMAIN_DMA + amd_iommu=pgtbl_v1 = (PD_FLAG_V1DMA)
> * IOMMU_DOMAIN_DMA + amd_iommu=pgtbl_v2 = (PD_FLAG_V2DMA | PD_FLAG_GIOV)
> * IOMMU_DOMAIN_SVA = (PD_FLAG_SVA | PD_FLAG_GCR3)
> * Domain for v2API = (PD_FLAG_V2API | PD_FLAG_GCR3)

You shouldn't have these distinctions. There should only be one enum
that specifies that format the IOPTEs are, and the rest is implied.

GCR3 is the "PASID table" right? it should not be part of the
iommu_domain struct either, SMMUv3 is busy undoing this mistake in
their design right now.

> It helps keeping track of how the IOMMU driver needs to setup DTEs for
> device in a particular domain.

Yes, I understand what it is for. It is important that the right data
be stored in the right places or you will have to redo it all. If it
does not directly related to describing the format of the IOPTEs then
it has no buisness being in the iommu_domain struct or it's driver
varients like protection_domain. It belongs someplace else.

> > > +#define PD_FLAG_GIOV		BIT(4) /* domain enable GIOV support */
> > 
> > This really shouldn't be a iommu_domain flag, this should be computed
> > based on what domains are attached where
> > 
> > Eg this seems like it should be triggered by attaching identity to the
> > RID with PASID enabled or something like that.
> > 
> > Why do you need to set it for the legacy GPU path?
> 
> The PD_FLAG_GIOV is necessary for when we setup the v2 page table for
> devices, which cannot support PASID. With the DTE[GIOV] bit set, IOMMU
> hardware would treat the request from these devices as if it use PASID 0.

I understand that, but you are supposed to deduce this from having a
RID bound page table installed into a PASID 0 slot but other logic, it
is very much not part of the domain itself.

(as we all the other variations of this flag when you set an IDENTITY
domain on the RID while having PASID operating)

Jason