Re: [PATCH v5 04/14] iommu/amd: Refactor protection domain allocation code

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Robin,


On 8/22/2023 1:36 PM, Robin Murphy wrote:
> On 2023-08-21 11:42, Vasant Hegde wrote:
>> To replace if-else with switch-case statement due to increasing number of
>> domain types.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
>> ---
>>   drivers/iommu/amd/iommu.c | 45 +++++++++++++++++++--------------------
>>   1 file changed, 22 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index c2cb541b0553..d5569eec0fe9 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2078,24 +2078,8 @@ static struct protection_domain
>> *protection_domain_alloc(unsigned int type)
>>       struct io_pgtable_ops *pgtbl_ops;
>>       struct protection_domain *domain;
>>       int pgtable;
>> -    int mode = DEFAULT_PGTABLE_LEVEL;
>>       int ret;
>>   -    /*
>> -     * Force IOMMU v1 page table when iommu=pt and
>> -     * when allocating domain for pass-through devices.
>> -     */
>> -    if (type == IOMMU_DOMAIN_IDENTITY) {
>> -        pgtable = AMD_IOMMU_V1;
>> -        mode = PAGE_MODE_NONE;
>> -    } else if (type == IOMMU_DOMAIN_UNMANAGED) {
>> -        pgtable = AMD_IOMMU_V1;
>> -    } else if (type == IOMMU_DOMAIN_DMA || type == IOMMU_DOMAIN_DMA_FQ) {
>> -        pgtable = amd_iommu_pgtable;
>> -    } else {
>> -        return NULL;
>> -    }
>> -
>>       domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>>       if (!domain)
>>           return NULL;
>> @@ -2106,27 +2090,42 @@ static struct protection_domain
>> *protection_domain_alloc(unsigned int type)
>>         spin_lock_init(&domain->lock);
>>       INIT_LIST_HEAD(&domain->dev_list);
>> +    domain->nid = NUMA_NO_NODE;
>> +
>> +    switch (type) {
>> +    /* No need to allocate io pgtable ops in passthrough mode */
>> +    case IOMMU_DOMAIN_IDENTITY:
>> +        return domain;
>> +    case IOMMU_DOMAIN_DMA:
>> +    case IOMMU_DOMAIN_DMA_FQ:
> 
> Nit: drivers won't see DMA_FQ any more, so you don't need it here (looks like
> the addition of the original line above crossed over with me removing it from
> other drivers).

Thanks Robin. I will have follow up patch to drop DMA_FQ.

-Vasant