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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxx>
>
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);
}
