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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Oct 10, 2023 at 11:32:53AM +0530, Vasant Hegde wrote:

> __amd_iommu_flush_tlb() flushes both domain ID and all devices within the domain
> for the given PASID. So it works fine. All these functions are reworked as part
> of invalidation series. Hence this function is not relevant anymore.

But the replacement has the same issue:

static int domain_flush_tlb_range(struct protection_domain *pdom,
                                  ioasid_t pasid, u64 address, size_t size)
{
        struct iommu_cmd cmd;
        int i, ret = 0;
        bool gn = is_pasid_valid(pasid);

        build_inv_iommu_pages(&cmd, address, size, pdom->id, pasid, gn);
                                              ^^^^^^^^^^^^^^^

(btw I have no issue/comments with the invalidate series, it just
doesn't go far enough to correctly support the iommu core's PASID ops)

> > 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.
> 
> That's exactly why I have been saying for SVA I don't need
> per-device-domain ID.  Current code works fine. Anyway, I will add
> this support in v3.

The threshold for upstream is more than "works fine" in some limited
testing.

It needs to correctly implement the driver op APIs we have defined in
the core code. In this regard it does not "work fine", you can see it
by code inspection that it is not producing a correct cache tag in the
IOTLB for domains attached via op->set_dev_pasid()

So, lets see v3.

Jason