Re: [PATCH v5 10/14] iommu/amd: Introduce logic to enable/disable IOPF

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Feb 09, 2024 at 12:07:16AM +0530, Vasant Hegde wrote:
> On 2/8/2024 11:01 PM, Jason Gunthorpe wrote:
> > On Wed, Feb 07, 2024 at 02:28:08PM +0530, Vasant Hegde wrote:
> > 
> >>> You already know what is going to happen at the very start of attach,
> >>> you don't need to "enable it after" just do it right the first time
> >>> through.
> >>
> >> First time we will not know whether device will actually use fault handler or
> >> not. All we will know is whether IOMMU and device is capable of PRI or not.
> > 
> > I don't understand this, you should know all of this before you get to
> > setting the DTE. What is missing?
> 
> In attach path we will know the device capabilities (like PRI) but we will not
> know whether device is going to use it or not. Its like device has capability,
> let us enable it without assuming device may use it.

I don't understand "device may use it"

At domain pasid attach time you have the domain being attached and
information the device capabilities.

The decision is simple, if the domain is using PRI and the device
supports PRI you enable PRI.

This means you enable PRI when you attach the first PRI domain to the
GCR3, which will require rewriting the DTE during set_dev_pasid.

This is part of the same logical flow where you'd rewrite the DTE
during set_dev_pasid to do things like IDENTITY and BLOCKING RID
support.

> >> If I have to enable PRI in attach path then I don't need to track number of
> >> domain stuff. I can simply do something like
> >> 	if (pdom_is_sva_capable(pdom))
> >> 		// enable PRI in IOMMU
> >> 		// enable device PRI
> >>
> >> and in detach path,
> >> 	if (PRI is enabled)
> >> 		// disable IOMMU/device PRI stuff
> > 
> > Each PASID can have PRI on or not, so you need to keep track of how
> > many PASIDs are using PRI at any moment and keep things in sync that
> > way.
> > 
> > If there are no PRI handlers installed then the PCI config space
> > should disable PRI and all the PRI bits flushed and disabled.
> 
> If we don't have handler then PRI is disabled in attach device path only.
>
> So in detach path, if number of PASIDs are zero then we are good to disable PRI
> (at least for the current usage model).

So this is why you are having the problem above, the PRI logic should
not be in the attach_dev path because that does not take in a PRI
capable domaine (currently only SVA)

It should be triggered in set_dev_pasid and remove_dev_pasid.

I'm trying to get you to organize things so they are ready for the
next steps and things are not in the wrong spot.

Jason