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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Sep 07, 2023 at 12:45:01PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 8/31/2023 5:16 AM, Jason Gunthorpe wrote:
> > 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.
> 
> 
> If you intend to remove enable_feature(SVA) API completely, then you help us to
> figure out how to handle below issues?
> 
> 1) During attach_device() we don't know whether device is really going to use
> SVA or not. So we want to start with minimal GCR3 table (that can handle just
> PASID 0) and during enable_feature(SVA) we want to enable GCR3 table that can
> handle max_pasids supported by a device. That way we can avoid performance
> penalty if device is not using SVA (extra GCR3 table walk).

Resize the GCR3 table when the first pasid using attach happens.

We can discuss if/when it should be shrunk back to minimal.

> 2) If we boot with passthrough mode, then during attach_device() we are not
> allocating GCR3 table. It will be allocated only when its needed. That way we
> are not wasting memory.

> Do you suggest that the driver setup GCR3 table at the first PASID binding to
> the device?

Yes. PASID must work correctly stand alone.

Jason