Re: [PATCH v3 16/19] iommu/amd: Add domain_alloc_user based domain allocation

Suthikulpanit, Suravee
2023-10-17T02:01:00Z

Hi Joao,

On 9/23/2023 8:25 AM, Joao Martins wrote:
> Add the domain_alloc_user op implementation. To that end, refactor
> amd_iommu_domain_alloc() to receive a dev pointer and flags, while
> renaming it to .. such that it becomes a common function shared with
> domain_alloc_user() implementation. The sole difference with
> domain_alloc_user() is that we initialize also other fields that
> iommu_domain_alloc() does. It lets it return the iommu domain
> correctly initialized in one function.
>
> This is in preparation to add dirty enforcement on AMD implementation
> of domain_alloc_user.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> drivers/iommu/amd/iommu.c | 46 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 95bd7c25ba6f..af36c627022f 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -37,6 +37,7 @@
> #include <asm/iommu.h>
> #include <asm/gart.h>
> #include <asm/dma.h>
> +#include <uapi/linux/iommufd.h>
>
> #include "amd_iommu.h"
> #include "../dma-iommu.h"
> @@ -2155,7 +2156,10 @@ static inline u64 dma_max_address(void)
> return ((1ULL << PM_LEVEL_SHIFT(amd_iommu_gpt_level)) - 1);
> }
>
> -static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
> +static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
> + struct amd_iommu *iommu,
> + struct device *dev,
> + u32 flags)

Instead of passing in the struct amd_iommu here, what if we just derive
it in the do_iommu_domain_alloc() as needed? This way, we don't need to
... (see below)

> {
> struct protection_domain *domain;
>
> @@ -2164,19 +2168,54 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
> * default to use IOMMU_DOMAIN_DMA[_FQ].
> */
> if (amd_iommu_snp_en && (type == IOMMU_DOMAIN_IDENTITY))
> - return NULL;
> + return ERR_PTR(-EINVAL);
>
> domain = protection_domain_alloc(type);
> if (!domain)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> domain->domain.geometry.aperture_start = 0;
> domain->domain.geometry.aperture_end = dma_max_address();
> domain->domain.geometry.force_aperture = true;
>
> + if (dev) {
> + domain->domain.type = type;
> + domain->domain.pgsize_bitmap =
> + iommu->iommu.ops->pgsize_bitmap;
> + domain->domain.ops =
> + iommu->iommu.ops->default_domain_ops;
> + }
> +
> return &domain->domain;
> }
>
> +static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
> +{
> + struct iommu_domain *domain;
> +
> + domain = do_iommu_domain_alloc(type, NULL, NULL, 0);

... pass iommu = NULL here unnecessarily.

> + if (IS_ERR(domain))
> + return NULL;
> +
> + return domain;
> +}
> +
> +static struct iommu_domain *amd_iommu_domain_alloc_user(struct device *dev,
> + u32 flags)
> +{
> + unsigned int type = IOMMU_DOMAIN_UNMANAGED;
> + struct amd_iommu *iommu;
> +
> + iommu = rlookup_amd_iommu(dev);
> + if (!iommu)
> + return ERR_PTR(-ENODEV);

We should not need to derive this here.

Other than this part.

Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks,
Suravee