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

[Robin Murphy <robin.murphy@arm.com>]

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
>>
>