Re: [PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Aug 31, 2023 at 12:30:21AM +0530, Vasant Hegde wrote:
> >>>> +int amd_iommu_sva_disable(struct device *dev)
> >>>> +{
> >>>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> >>>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> >>>> +
> >>>> +	if (!iommu || !dev_data)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	return amd_iommu_sva_gcr3_uninit(dev_data);
> >>>> +}
> >>>
> >>> I think these features are a mistake, you need to implement them for
> >>> now but I wouldn't touch the gcr3 table, and disable should be a NOP.
> >>
> >> We need to configure Device Table Entry when we enable/disable SVA.
> > 
> > The DTE should only be changed by bind/unbind of RID/PASID - it has
> > nothing to do with these APIs.
> 
> Above functions are called during enable_feature(SVA) path. Not in PASID
> bind/unbind path. In PASID bind/unbind path we just update PASID table.

I know, which is why they can't change anything about the IOMMU
HW. Only attach/bind/unbind should change HW state.

I intend to remove these APIs once ARM is fixed, so please don't start
to rely on them. The driver is doing something wrong if it is changing
HW setups outside of attach/bind/unbind.

These APIs are from an era before we had proper PASID API support and
no longer make sense.

> > Again, start with enabling native PASID support for UNAMANGED v2
> > domains and your SVA will make alot more sense and be alot cleaner.
> 
> Currently, the IOMMU_DOMAIN_UNMANAGED is used by VFIO, which sets up the v1
> table for GPA->SPA translation.

> If we were to force the unmanaged domain to use the v2 table by default, we
> would not be able to support nested translation, where it requires v1 table to
> be managed by the host ( GPA->SPA) and v2 in the guest (GVA->GPA).

I've said this so many times now

Upstream kernel does not support nested translation with VFIO. The
force v1 hack in the AMD driver *SHOULD NOT* be part of mainline.

I don't care about any unmerged out of tree patch sets that rely on it -
especially ones that ignored that the semi-upstream approved hack to do
this is VFIO_TYPE1_NESTING_IOMMU (which is also about to be removed),
not what you are talking about here.

iommufd has the upstream solution for this problem and it relies on
the iommufd user specifically asking for a nesting parent page table
which is the only signal that the AMD driver should use to force a v1
format on a PASID capable device.

Otherwise *all* requests for any kind of paging domain should use the
v2 if the device has PASID support.

Indeed after my other series is merged the AMD driver should be
converted to the alloc_domain_paging() interface that directly
prevents this abuse of the API.

Jason