Re: [PATCH v2 06/10] iommu/amd: Refactor helper function for setting / clearing GCR3

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Sep 27, 2023 at 11:51:20AM +0530, Vasant Hegde wrote:
> >>  	if (pte == NULL)
> >>  		return -ENOMEM;
> >>  
> >> -	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
> >> +	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
> >> +	__amd_iommu_flush_tlb(dev_data->domain, pasid);
> > 
> > This flushing doesn't seem to make sense to me, it shouldn't take in a
> > domain parameter.
> >
> > I'm reading the spec here so I may get this wrong but.. It looks like
> > the IOTLB cache is tagged by (DTE.domain_id, PASID)?
> > 
> 
> Yeah. It works, but name is confusing.

How does it work? You haven't explained anything about why does it
work.

Nothing guarantees that domain->id is per-device. It is emphatically
not, it is per-domain. It happens to act like it is per-device when
using the DMA API, as DMA API does not share domains across
groups. However iommufd does share domains and so this is not a
correct assumption.

You said below that you think the DTE.domain_id is per-device. I agree
that would fix this bug, if true. If it is to be per-device it should
come from the struct iommu_dev_data, not from the domain->id. This
should happen any time a GCR3 table is in-use. v1 mode is fine to use
a per-domain 'domain_id'.

> amd_iommu_device_flush_pasid_all(dev_data, pasid)
> {
>         // Flush domain
>        // Flush single device ATS
> }

That would make more logical sense, but it does not resolve the issue
of correctly choosing DTE.domain_id so the cache tags don't alias.

Jason