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

Joao Martins
2023-10-17T09:07:48Z

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);
}