On 6/23/2022 3:12 PM, Robin Murphy wrote:
> On 2022-06-23 09:03, Joerg Roedel wrote:
>> On Fri, Jun 03, 2022 at 04:51:03PM +0530, Vasant Hegde wrote:
>>> Fix below sparse warning:
>>> CHECK drivers/iommu/amd/iommu.c
>>> drivers/iommu/amd/iommu.c:73:24: warning: symbol 'amd_iommu_ops' was not
>>> declared. Should it be static?
>>>
>>> Also we are going to introduce v2 page table which has different
>>> pgsize_bitmaps. Hence remove 'const' qualifier.
>>
>> I am not a fan of removing the consts. Please use separate ops
>> structures for v2 page-tables and make then const as well. This probably
>> also has some optimization potential in the future when we can make the
>> ops call-back functions page-table specific.
>
> TBH it's probably time to retire iommu_ops->pgsize_bitmap anyway. At the very
> least it would be logical to move it to iommu_domain_ops now, but maybe we
> could skip ahead and just rely on drivers initialising domain->pgsize_bitmap
> directly in their .domain_alloc?
>
Robin,
Something like below? If yes, I will cleanup and get proper fix.
-Vasant
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 840831d5d2ad..32dd84a7c1da 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1916,6 +1916,7 @@ static int protection_domain_init_v1(struct
protection_domain *domain, int mode)
return -ENOMEM;
}
+ domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES;
amd_iommu_domain_set_pgtable(domain, pt_root, mode);
return 0;
@@ -2282,7 +2283,6 @@ const struct iommu_ops amd_iommu_ops = {
.get_resv_regions = amd_iommu_get_resv_regions,
.put_resv_regions = generic_iommu_put_resv_regions,
.is_attach_deferred = amd_iommu_is_attach_deferred,
- .pgsize_bitmap = AMD_IOMMU_PGSIZES,
.def_domain_type = amd_iommu_def_domain_type,
.default_domain_ops = &(const struct iommu_domain_ops) {
.attach_dev = amd_iommu_attach_device,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..73cfba6a6728 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1915,8 +1915,6 @@ 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 (!domain->ops)
domain->ops = bus->iommu_ops->default_domain_ops;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5e1afe169549..0c028aa71b96 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -255,7 +255,6 @@ struct iommu_ops {
int (*def_domain_type)(struct device *dev);
const struct iommu_domain_ops *default_domain_ops;
- unsigned long pgsize_bitmap;
struct module *owner;
};
> (and FWIW I'm leaning towards the same for the domain->ops as well; the more
> I look at ops->default_domain_ops, the more it starts looking like a stupid
> mess... my stupid mess... sorry about that)
>
> Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
