* [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation
@ 2023-04-04 7:27 Jerry Snitselaar
2023-04-04 16:28 ` Jerry Snitselaar
2023-04-13 9:55 ` Joerg Roedel
0 siblings, 2 replies; 5+ messages in thread
From: Jerry Snitselaar @ 2023-04-04 7:27 UTC (permalink / raw
To: linux-kernel, iommu
Cc: Vasant Hegde, Suravee Suthikulpanit, Robin Murphy, Will Deacon,
Joerg Roedel
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>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation
2023-04-04 7:27 [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation Jerry Snitselaar
@ 2023-04-04 16:28 ` Jerry Snitselaar
2023-04-06 6:12 ` Vasant Hegde
2023-04-06 13:08 ` Robin Murphy
2023-04-13 9:55 ` Joerg Roedel
1 sibling, 2 replies; 5+ messages in thread
From: Jerry Snitselaar @ 2023-04-04 16:28 UTC (permalink / raw
To: linux-kernel, iommu
Cc: Vasant Hegde, Suravee Suthikulpanit, Robin Murphy, Will Deacon,
Joerg Roedel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation
2023-04-04 16:28 ` Jerry Snitselaar
@ 2023-04-06 6:12 ` Vasant Hegde
2023-04-06 13:08 ` Robin Murphy
1 sibling, 0 replies; 5+ messages in thread
From: Vasant Hegde @ 2023-04-06 6:12 UTC (permalink / raw
To: Jerry Snitselaar, linux-kernel, iommu
Cc: Suravee Suthikulpanit, Robin Murphy, Will Deacon, Joerg Roedel
Jerry,
On 4/4/2023 9:58 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@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.
>
I am not familiar with how arm paging works. But in AMD case we decide the page
table during protection domain allocation. Also we can have different page size
for different domains. (like one domain in v1 and another in v2 page mode).
> 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?
IMO adding another ops is not required. I think current fix is good enough.
Thanks for debugging/fixing this issue.
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
-Vasant
>
> 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
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation
2023-04-04 16:28 ` Jerry Snitselaar
2023-04-06 6:12 ` Vasant Hegde
@ 2023-04-06 13:08 ` Robin Murphy
1 sibling, 0 replies; 5+ messages in thread
From: Robin Murphy @ 2023-04-06 13:08 UTC (permalink / raw
To: Jerry Snitselaar, linux-kernel, iommu
Cc: Vasant Hegde, Suravee Suthikulpanit, Will Deacon, Joerg Roedel
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@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?
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
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation
2023-04-04 7:27 [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation Jerry Snitselaar
2023-04-04 16:28 ` Jerry Snitselaar
@ 2023-04-13 9:55 ` Joerg Roedel
1 sibling, 0 replies; 5+ messages in thread
From: Joerg Roedel @ 2023-04-13 9:55 UTC (permalink / raw
To: Jerry Snitselaar
Cc: linux-kernel, iommu, Vasant Hegde, Suravee Suthikulpanit,
Robin Murphy, Will Deacon
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>
> ---
> drivers/iommu/amd/iommu.c | 6 ++----
> drivers/iommu/iommu.c | 9 +++++++--
> 2 files changed, 9 insertions(+), 6 deletions(-)
Applied, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-04-13 9:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-04 7:27 [PATCH] iommu: amd: Set page size bitmap during V2 domain allocation Jerry Snitselaar
2023-04-04 16:28 ` Jerry Snitselaar
2023-04-06 6:12 ` Vasant Hegde
2023-04-06 13:08 ` Robin Murphy
2023-04-13 9:55 ` Joerg Roedel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.