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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Aug 11, 2023 at 03:37:25PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 8/8/2023 9:09 PM, Jason Gunthorpe wrote:
> > On Tue, Aug 08, 2023 at 10:02:27AM +0000, Vasant Hegde wrote:
> >> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >>
> >> To use the new helper function for setting up GCR3 table.
> >>
> >> If system is booted with V2 page table then setup default GCR3 with
> >> domain GCR3 pointer. So that all devices in the domain uses same page
> >> table for translation. Also return page table setup status from
> >> do_attach() function.
> >>
> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> >> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> >> ---
> >>  drivers/iommu/amd/iommu.c | 54 ++++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 50 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> >> index cc42732820dd..4fed3fbe069e 100644
> >> --- a/drivers/iommu/amd/iommu.c
> >> +++ b/drivers/iommu/amd/iommu.c
> >> @@ -1821,15 +1821,41 @@ static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
> >>  	amd_iommu_apply_erratum_63(iommu, devid);
> >>  }
> >>  
> >> -static void do_attach(struct iommu_dev_data *dev_data,
> >> -		      struct protection_domain *domain)
> >> +/*
> >> + * Note: This is currently used when booting w/ amd_iommu=pgtbl_v2
> >> + */
> > 
> > I would drop all this commentary about amd_iommu=pgtbl_v2. That
> > command line option should be removed.
> 
> AMD IOMMU can boot with PT, V1 page table and V2 page table. We need a way to
> tell what page table we want to use. Hence we have these command line options.
 
No, you don't, it is wrong.

PT or not is controlled only by the core code using existing common
mechanims. Not by iommu drivers.

For v1/v2 kernel needs to automatically select the best page table
option, not try to make an admin figure it out with a command line option.

I expect it to be removed in due course.

Jason