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

[Jason Gunthorpe <jgg@ziepe.ca>]

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?

> > The situation where attach fails and leaves the HW in an unknown state
> > is really hard to deal with - and without the reliable global blocked
> > domain the core code can't 100% rescue it either.
> 
> If attach fails we throw error message and skip updating DTE. I believe core
> layer understands that driver failed to attach device and puts device/group to
> its original domain. So things should work fine.

It is not just the DTE, the whole thing including changing PCIE config
space and so forth has to be kept correct.

> > This also means, broadly, you can't allow the DTE to evolve during the
> > operation of attach/detach as the in-between states may become
> > userspace visible and may be harmful in some way.
> 
> We make DTE changes in set_dte() function only (except dirty bit change that
> will be consolidated).

Sure, but set_dte doesn't take care to sequence the update.

> >>> security issue is solved. Especially if the more stuff is drifting
> >>> further from being correct. If you can keep the updates in set_dte
> >>> then maybe with some reluctance. But not like this with random touches
> >>> to the DTE all over the place.
> >>
> >> Currently all DTE update is happening inside set_dte only (dirty bit enable is
> >> an exception that may need to moved inside set_dte). This patch just invokes
> >> that set_dte and invalidates cache.
> > 
> > So then why all this strangeness?? Just set dev_data->ppr earlier in
> > attach and order the handler setup properly.
> > 
> > It should be really simple:
> > 
> >  // All protected by the core's group mutex
> > 
> >  if (domain->needs_pri)  {
> >      dev_data->ppr = true;
> >      if (!dev_data->num_pri_domains)
> 
> What is PRI domain?

Right now it is only a SVA domain, it is a domain that wishes to use
PRI. Quite soon we are going to expand this to PAGING domains as well.

> 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.

At some point you need to determine if any PASIDs have domains that
need PRI. A counter is a simple solution.

Jason