Re: [PATCH v5 14/17] iommu/amd: Refactor GCR3 table helper functions

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Jan 23, 2024 at 02:24:51PM +0530, Vasant Hegde wrote:

> domain_id_is_per_dev() decides how to allocate domain ID. Right now for V2 and
> pass through mode we allocate per-device-domain-ID as they can switch to SVA.

That makes no sense. You don't need a domain id for passthrough mode
unless you are also installing a gcr3 table. You don't need to install
a gcr3 table unless there is a PASID being attached too.

Pre-setting the domain ID to avoid setting it when the GCR3 is later
loaded is spaghetti logic.

> > All this logic should be shared between the pasid and rid attach
> > paths.>
> > The passthrough thing is only an issue of DTE construction.
> > 
> > If you build a DTE with a GCR3 table and RID=IDENTITY then you set
> > some bits, and that is it. Detect that case directly when you build
> > the DTE. It should have no effect on what domain ID is used to tag
> > translations retrived from a GCR3 table.
> 
> We build DTE as soon as we attach device to domain.
> 
> Now moving domain ID allocation to setup_gcr3_table complicates things.
>   - In attach_device() path we want to allocate domain ID but not GCR3 table
>     We can allocate GCR3 table, but if we don't use it its waste of memory.

It is a waste to allocate the domain id for identity too.

>   - In SVA enablement path we want to allocate GCR3 table
>     But by then domain ID should have been allocated. We don't want to allocate
> another domain_ID and change ID in SVA enablement path as our domain ID is not
> specific to GCR3.

Again, this seems to be a complication that is being created by the
DTE construction. You shouldn't need the caller to carefully sequence
what it is doing. The DTE programming should just install the correct
DTE for the *current state*.

A RID only identity/passthrough DTE does not have a GCR3 table and
does not need a unique domain ID.

> >> We need to handle passthrough as well. It doesn't make sense to allocate and
> >> keep GCR3 table when we are not going to use it.
> > 
> > Then don't, and my diff didn't - but check for the passthrough case
> > directly against the attached domain as identity.
> 
> We don't want to add condition check that depends on code path:
>   like attach_device : allocate domain ID but not GCR3
>   SVA path : Allocate GCR3 but not domain ID.

I'm not saying you should do that, I'm actively saying you should not
do that! All paths should be symmetric.

This matters, if you hope to implement the full driver functionality
the DTE construction must be clean, thare are too many cases
otherwise..

Jason