Messages in this thread | | | Date | Tue, 4 Apr 2023 09:28:32 -0700 | From | Jerry Snitselaar <> | Subject | Re: [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation |
| |
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@amd.com> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Joerg Roedel <joro@8bytes.org> > Fixes: 4db6c41f0946 ("iommu/amd: Add support for using AMD IOMMU v2 page table for DMA-API") > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
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?
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 >
| |