Re: [PATCH 06/11] iommu/amd: Refactor helper function for attaching / detaching device

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Aug 14, 2023 at 10:33:08PM -0700, Suthikulpanit, Suravee wrote:
> Jason,
> 
> On 8/11/2023 4:50 PM, Jason Gunthorpe wrote:
> > On Fri, Aug 11, 2023 at 10:20:56PM +0530, Vasant Hegde wrote:
> > 
> > > Our hardware is capable to boot with either V1 or V2 page table. By default
> > > driver decides what is the best mode.
> > > But if user wants he has a flexibility to choose V1 vs V2 page table. In that
> > > mode we will adhere to user provided option and all devices will be put in that
> > > mode.
> > 
> > There is no valid reason for the user to override the kernel. If we do
> > this for AMD we have to do it for every single driver that supports
> > two formats. It makes no sense
> > 
> > Jason
> 
> The amd_iommu=pgtbl_v2 was first introduced to enable nested IOMMU
> translation w/ HW-vIOMMU support. This option is necessary for the guest
> kernel since we need guest kernel to use IOMMU v2 page table for DMA-API but
> the IOMMU driver is currently default to use pgtbl_v1 for DMA-API.
> Currently, there is no good way to auto detect this use case.

I will say it again - upstream kernel does not support nested IOMMU,
please stop adding junk to your driver to support out of tree patch
sets.

iommufd has an automatic solution to this problem.

> * There is an upcoming feature (e.g. TMPM :
> https://www.amd.com/system/files/TechDocs/58151_0.51-PUB.pdf), which
> requires devices to be setup w/ pgtbl_v1. However, the TMPM driver is loaded
> after devices are probed. Therefore, this would require Linux to boot with
> option amd_iommu=pgtbl_v1.

That is horrible, and it is even worse that you plan to support this
with a command line option. You need a proper fix.

> * IOMMU pgtbl_v1 and pgtbl_v2 are different from HW perspective, which could
> yield different performance depending on the workload and end-point devices.
> Therefore, the kernel option amd_iommu=pgtbl_v1|pgtbl_v2 adds flexibility
> and give users additional control if needed.

Maybe, but I'd want to see actual data on this. I'm skeptical.
 
> For SVA, we need to ensure that the iommu group is using pgtbl_v2 or in
> pass-through mode (no page table setup) prior to enabling SVA for the
> device. Currently, this would require the pgtbl_v2 option. However, we could
> consider default to using v2 table for an IOMMU group if it contains an
> PASID-capable device.

It cannot continue to require the command line option - we expect that
all the iommu drivers will support SVA without admin
intervention. This will likel have to happen via some kind of 'is
pasid possible' core call which will make the distinction based on the
composition of the group regarding pasid support of the devices in it.

Jason