Re: [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation

[Robin Murphy]

On 04/04/2023 5:28 pm, Jerry Snitselaar wrote:
> On Tue, Apr 04, 2023 at 12:27:42AM -0700, Jerry Snitselaar wrote:
> > With the addition of the V2 page table support, the domain page size
> > bitmap needs to be set prior to iommu core setting up direct mappings
> > for reserved regions. When reserved regions are mapped, if this is not
> > done, it will be looking at the V1 page size bitmap when determining
> > the page size to use in iommu_pgsize(). When it gets into the actual
> > amd mapping code, a check of see if the page size is supported can
> > fail, because at that point it is checking it against the V2 page size
> > bitmap which only supports 4K, 2M, and 1G.
> >
> > Add a check to __iommu_domain_alloc() to not override the
> > bitmap if it was already set by the iommu ops domain_alloc() code path.
> >
> > Cc: Vasant Hegde <vasant.hegde@xxxxxxx>
> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: Joerg Roedel <joro@xxxxxxxxxx>
> > Fixes: 4db6c41f0946 ("iommu/amd: Add support for using AMD IOMMU v2 page table for DMA-API")
> > Signed-off-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>
>
>
>
> I'm still not sure this is the best solution. Feels odd with adding a
> check to core code to handle something one of the drivers has
> done. Another thought was something like arm does, with amd_iommu_ops
> dropping the const and setting the default page size bitmap in
> iommu_init_pci(), but I think that would still require adding
> something in the protection domain/init code to deal with it forcing
> v1, so it would still require a check in the core code.
>
> Would adding an op make more sense, with a generic op doing what the
> core code currently does for setting the default? Or am I overthinking
> this?

I think this is fine as-is. TBH it's probably high time 
ops->pgsize_bitmap finished going away entirely - as the Arm SMMU bodges 
prove, it's no longer a good fit for the multi-instance model which 
we've evolved, and for the sake of only this one assignment now, it 
really doesn't seem worthwhile any more. Since I'm still reworking 
domain_alloc, and already have plans to clean up the horrible 
default_domain_ops thing (honestly, what possessed me to think that was 
a good idea at the time!?), I can throw pgsize_bitmap in there as well.

Cheers,
Robin.

> snits
>
> > ---
> >   drivers/iommu/amd/iommu.c | 6 ++----
> >   drivers/iommu/iommu.c     | 9 +++++++--
> >   2 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > index 5a505ba5467e..167da5b1a5e3 100644
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -1666,10 +1666,6 @@ static void do_attach(struct iommu_dev_data *dev_data,
> >   	domain->dev_iommu[iommu->index] += 1;
> >   	domain->dev_cnt                 += 1;
> >   
> > -	/* Override supported page sizes */
> > -	if (domain->flags & PD_GIOV_MASK)
> > -		domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
> > -
> >   	/* Update device table */
> >   	set_dte_entry(iommu, dev_data->devid, domain,
> >   		      ats, dev_data->iommu_v2);
> > @@ -2048,6 +2044,8 @@ static int protection_domain_init_v2(struct protection_domain *domain)
> >   
> >   	domain->flags |= PD_GIOV_MASK;
> >   
> > +	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
> > +
> >   	if (domain_enable_v2(domain, 1)) {
> >   		domain_id_free(domain->id);
> >   		return -ENOMEM;
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 10db680acaed..256a38371120 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1964,8 +1964,13 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> >   		return NULL;
> >   
> >   	domain->type = type;
> > -	/* Assume all sizes by default; the driver may override this later */
> > -	domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
> > +	/*
> > +	 * If not already set, assume all sizes by default; the driver
> > +	 * may override this later
> > +	 */
> > +	if (!domain->pgsize_bitmap)
> > +		domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
> > +
> >   	if (!domain->ops)
> >   		domain->ops = bus->iommu_ops->default_domain_ops;
> >   
> > --
> > 2.38.1