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

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

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.

> +		pgtable = amd_iommu_pgtable;
> +		break;
> +	/*
> +	 * Force IOMMU v1 page table when allocating
> +	 * domain for pass-through devices.
> +	 */
> +	case IOMMU_DOMAIN_UNMANAGED:
> +		pgtable = AMD_IOMMU_V1;
> +		break;
> +	default:
> +		goto out_err;
> +	}
>   
>   	switch (pgtable) {
>   	case AMD_IOMMU_V1:
> -		ret = protection_domain_init_v1(domain, mode);
> +		ret = protection_domain_init_v1(domain, DEFAULT_PGTABLE_LEVEL);
>   		break;
>   	case AMD_IOMMU_V2:
>   		ret = protection_domain_init_v2(domain);
>   		break;
>   	default:
>   		ret = -EINVAL;
> +		break;
>   	}
>   
>   	if (ret)
>   		goto out_err;
>   
> -	/* No need to allocate io pgtable ops in passthrough mode */
> -	if (type == IOMMU_DOMAIN_IDENTITY)
> -		return domain;
> -
> -	domain->nid = NUMA_NO_NODE;
> -
>   	pgtbl_ops = alloc_io_pgtable_ops(pgtable, &domain->iop.pgtbl_cfg, domain);
>   	if (!pgtbl_ops) {
>   		domain_id_free(domain->id);