Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Nov 07, 2023 at 11:25:49AM +0530, Vasant Hegde wrote:

> >  RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 
> 
> I haven't thought this scenario. Why do we even need this case?

It is triggerable by iommufd, it has to be supported.

You understand I'm going to insist that *all* of this is fixed before
you can progress with any new iommufd features, right? We must have
drivers implement the APIs correctly when they are exposed to
userspace. I sent patches fixing SMMUv3, we will have to make the same
fixing for AMD and Intel too.

> > Did I get it right? If so GIOV should ultimately be deduced based on
> > what domain the RID has?
> > 
> > Look at how the SMMUv3 stuff ended up. Their STE is the same purpose
> > as the AMD DTE. There are alot of combinations here, it was hard to
> > make a code flow that was clean. It turned out pretty good when the
> > DTE was generated in the ops->attach based on a calculation of exactly
> > what the current configuration is, because we already know what we are
> > in alot of detail at that point.
> > 
> > eg we know if we are attaching an identity domain and PASIDs are in
> > use that a single specific DTE should be created. So just call a
> > function directly to get the required DTE.
> > 
> > IOW - I'm not sure it really makes logical sense to store giov in
> > gcr3_info.
> 
> We had a choice of having a giov flag inside gcr3_info as it tells how to
> configure GCR3 related bits in DTE -OR- having a extra logic to calculate it
> every time. I can be calculated.

The codepath that determines the DTE will know already what the value
should be. Again look at how SMMUv3 turned out, the GIOV is very
similar to S1DSS.

> >>  	dev_data->domain = domain;
> >> @@ -2080,11 +2103,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
> >>  	if (domain_id_is_per_dev(domain))
> >>  		dev_data->domid = domain_id_alloc();
> > 
> > At some point this is the wrong place to put this, the domain ID is
> > logically associated with the gcr3 table, it should never be used if
> > there is no gcr table allocated, and it should be freed once the gcr3
> > table is freed.
> 
> Domain ID is decided based on page table type (and may be based on PASID later).
>  That's why I have a function to decide whether to allocate ID or not and it
> should be done in this path only. So that we can configure DTE.

The page table type determines if the GCR3 table is loaded, the domain
ID is needed if the GCR3 table is loaded.

Logically the domain ID is part of the GCR3 table.

Jason