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

[Joao Martins <joao.m.martins@oracle.com>]

On 17/10/2023 03:00, Suthikulpanit, Suravee wrote:
> 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)
> 
Hmm, you mean to derive amd_iommu from the dev pointer. Yeah, sounds good.

>>   {
>>       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.
> 
OK

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

OK, will do.

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

Here's the diff

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index af36c627022f..cfc7d2992aa6 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2157,11 +2157,17 @@ static inline u64 dma_max_address(void)
 }

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

        /*
         * Since DTE[Mode]=0 is prohibited on SNP-enabled system,
@@ -2178,7 +2184,7 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned
int type,
        domain->domain.geometry.aperture_end   = dma_max_address();
        domain->domain.geometry.force_aperture = true;

-       if (dev) {
+       if (iommu) {
                domain->domain.type = type;
                domain->domain.pgsize_bitmap =
                        iommu->iommu.ops->pgsize_bitmap;
@@ -2193,7 +2199,7 @@ static struct iommu_domain
*amd_iommu_domain_alloc(unsigned type)
 {
        struct iommu_domain *domain;

-       domain = do_iommu_domain_alloc(type, NULL, NULL, 0);
+       domain = do_iommu_domain_alloc(type, NULL, 0);
        if (IS_ERR(domain))
                return NULL;

@@ -2204,16 +2210,11 @@ 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);

        if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT)
                return ERR_PTR(-EOPNOTSUPP);

-       return do_iommu_domain_alloc(type, iommu, dev, flags);
+       return do_iommu_domain_alloc(type, dev, flags);
 }