[RFC,v1,1/2] iommu/arm-smmu: support s2 domain attribute

[RFC,v1,1/2] iommu/arm-smmu: support s2 domain attribute

[Rick Song]

Under some condition, user want to use the
only stage 2 translation ability, so smmu
driver need support setting only s2 domain
attribute.

Signed-off-by: Rick Song <songwenjun@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 12 ++++++++++++
 include/linux/iommu.h       |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 15c01c3..d3b19d2 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1854,6 +1854,18 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	mutex_lock(&smmu_domain->init_mutex);
 
 	switch (attr) {
+	case DOMAIN_ATTR_S2:
+		if (smmu_domain->smmu) {
+			ret = -EPERM;
+			goto out_unlock;
+		}
+
+		if (*(int *)data)
+			smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
+		else
+			smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+
+		break;
 	case DOMAIN_ATTR_NESTING:
 		if (smmu_domain->smmu) {
 			ret = -EPERM;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 436dc21..0b5a387 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -114,6 +114,7 @@ enum iommu_attr {
 	DOMAIN_ATTR_FSL_PAMU_ENABLE,
 	DOMAIN_ATTR_FSL_PAMUV1,
 	DOMAIN_ATTR_NESTING,	/* two stages of translation */
+	DOMAIN_ATTR_S2,		/* only stage 2 translation */
 	DOMAIN_ATTR_MAX,
 };
 
-- 
1.9.1

[RFC,v1,1/2] iommu/arm-smmu: support s2 domain attribute

[Rick Song]

Under some condition, user want to use the
only stage 2 translation ability, so smmu
driver need support setting only s2 domain
attribute.

Signed-off-by: Rick Song <songwenjun@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 12 ++++++++++++
 include/linux/iommu.h       |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 15c01c3..d3b19d2 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1854,6 +1854,18 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	mutex_lock(&smmu_domain->init_mutex);
 
 	switch (attr) {
+	case DOMAIN_ATTR_S2:
+		if (smmu_domain->smmu) {
+			ret = -EPERM;
+			goto out_unlock;
+		}
+
+		if (*(int *)data)
+			smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
+		else
+			smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+
+		break;
 	case DOMAIN_ATTR_NESTING:
 		if (smmu_domain->smmu) {
 			ret = -EPERM;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 436dc21..0b5a387 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -114,6 +114,7 @@ enum iommu_attr {
 	DOMAIN_ATTR_FSL_PAMU_ENABLE,
 	DOMAIN_ATTR_FSL_PAMUV1,
 	DOMAIN_ATTR_NESTING,	/* two stages of translation */
+	DOMAIN_ATTR_S2,		/* only stage 2 translation */
 	DOMAIN_ATTR_MAX,
 };
 
-- 
1.9.1

[RFC,v1,1/2] iommu/arm-smmu: support s2 domain attribute

[Rick Song]

Under some condition, user want to use the
only stage 2 translation ability, so smmu
driver need support setting only s2 domain
attribute.

Signed-off-by: Rick Song <songwenjun@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 12 ++++++++++++
 include/linux/iommu.h       |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 15c01c3..d3b19d2 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1854,6 +1854,18 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	mutex_lock(&smmu_domain->init_mutex);
 
 	switch (attr) {
+	case DOMAIN_ATTR_S2:
+		if (smmu_domain->smmu) {
+			ret = -EPERM;
+			goto out_unlock;
+		}
+
+		if (*(int *)data)
+			smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
+		else
+			smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+
+		break;
 	case DOMAIN_ATTR_NESTING:
 		if (smmu_domain->smmu) {
 			ret = -EPERM;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 436dc21..0b5a387 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -114,6 +114,7 @@ enum iommu_attr {
 	DOMAIN_ATTR_FSL_PAMU_ENABLE,
 	DOMAIN_ATTR_FSL_PAMUV1,
 	DOMAIN_ATTR_NESTING,	/* two stages of translation */
+	DOMAIN_ATTR_S2,		/* only stage 2 translation */
 	DOMAIN_ATTR_MAX,
 };
 
-- 
1.9.1

[RFC,v1,2/2] vfio/iommu-type1: set only stage 2 translation

[Rick Song]

Normally, VFIO should use only stage 2 translation of
iommu, to create the address mapping. If nesting translation
is disabled from VFIO core, enable iommu domain only stage 2
attribute, otherwise, enable iommu domain nesting attribute.

Signed-off-by: Rick Song <songwenjun@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 2ba1942..c0265fe 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -741,7 +741,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	struct vfio_group *group, *g;
 	struct vfio_domain *domain, *d;
 	struct bus_type *bus = NULL;
-	int ret;
+	int attr, ret;
 
 	mutex_lock(&iommu->lock);
 
@@ -775,13 +775,22 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_free;
 	}
 
+	/*
+	 * Set iommu nesting domain attribute if nesting translation
+	 * is enabled from iommu vfio, otherwise set iommu only stage
+	 * 2 domain attribute.
+	 */
+	attr = 1;
 	if (iommu->nesting) {
-		int attr = 1;
-
 		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
 					    &attr);
 		if (ret)
 			goto out_domain;
+	} else {
+		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_S2,
+					    &attr);
+		if (ret)
+			goto out_domain;
 	}
 
 	ret = iommu_attach_group(domain->domain, iommu_group);
-- 
1.9.1

[RFC,v1,2/2] vfio/iommu-type1: set only stage 2 translation

[Rick Song]

Normally, VFIO should use only stage 2 translation of
iommu, to create the address mapping. If nesting translation
is disabled from VFIO core, enable iommu domain only stage 2
attribute, otherwise, enable iommu domain nesting attribute.

Signed-off-by: Rick Song <songwenjun@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 2ba1942..c0265fe 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -741,7 +741,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	struct vfio_group *group, *g;
 	struct vfio_domain *domain, *d;
 	struct bus_type *bus = NULL;
-	int ret;
+	int attr, ret;
 
 	mutex_lock(&iommu->lock);
 
@@ -775,13 +775,22 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_free;
 	}
 
+	/*
+	 * Set iommu nesting domain attribute if nesting translation
+	 * is enabled from iommu vfio, otherwise set iommu only stage
+	 * 2 domain attribute.
+	 */
+	attr = 1;
 	if (iommu->nesting) {
-		int attr = 1;
-
 		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
 					    &attr);
 		if (ret)
 			goto out_domain;
+	} else {
+		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_S2,
+					    &attr);
+		if (ret)
+			goto out_domain;
 	}
 
 	ret = iommu_attach_group(domain->domain, iommu_group);
-- 
1.9.1

[RFC,v1,2/2] vfio/iommu-type1: set only stage 2 translation

[Rick Song]

Normally, VFIO should use only stage 2 translation of
iommu, to create the address mapping. If nesting translation
is disabled from VFIO core, enable iommu domain only stage 2
attribute, otherwise, enable iommu domain nesting attribute.

Signed-off-by: Rick Song <songwenjun@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 2ba1942..c0265fe 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -741,7 +741,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	struct vfio_group *group, *g;
 	struct vfio_domain *domain, *d;
 	struct bus_type *bus = NULL;
-	int ret;
+	int attr, ret;
 
 	mutex_lock(&iommu->lock);
 
@@ -775,13 +775,22 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_free;
 	}
 
+	/*
+	 * Set iommu nesting domain attribute if nesting translation
+	 * is enabled from iommu vfio, otherwise set iommu only stage
+	 * 2 domain attribute.
+	 */
+	attr = 1;
 	if (iommu->nesting) {
-		int attr = 1;
-
 		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
 					    &attr);
 		if (ret)
 			goto out_domain;
+	} else {
+		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_S2,
+					    &attr);
+		if (ret)
+			goto out_domain;
 	}
 
 	ret = iommu_attach_group(domain->domain, iommu_group);
-- 
1.9.1

Re: [RFC,v1,2/2] vfio/iommu-type1: set only stage 2 translation

[Alex Williamson]

On Fri, 21 Oct 2016 12:39:24 +0800
Rick Song <songwenjun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:

> Normally, VFIO should use only stage 2 translation of
> iommu, to create the address mapping. If nesting translation
> is disabled from VFIO core, enable iommu domain only stage 2
> attribute, otherwise, enable iommu domain nesting attribute.
> 
> Signed-off-by: Rick Song <songwenjun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 2ba1942..c0265fe 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -741,7 +741,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	struct vfio_group *group, *g;
>  	struct vfio_domain *domain, *d;
>  	struct bus_type *bus = NULL;
> -	int ret;
> +	int attr, ret;
>  
>  	mutex_lock(&iommu->lock);
>  
> @@ -775,13 +775,22 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  		goto out_free;
>  	}
>  
> +	/*
> +	 * Set iommu nesting domain attribute if nesting translation
> +	 * is enabled from iommu vfio, otherwise set iommu only stage
> +	 * 2 domain attribute.
> +	 */
> +	attr = 1;
>  	if (iommu->nesting) {
> -		int attr = 1;
> -
>  		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
>  					    &attr);
>  		if (ret)
>  			goto out_domain;
> +	} else {
> +		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_S2,
> +					    &attr);
> +		if (ret)
> +			goto out_domain;
>  	}

This attribute is not relevant to the majority of current users, why
would we assume that we need to call it for all non-nesting cases?  Why
do we need to set the attribute at all, what benefit does it provide?
If this is the normal case for an IOMMU API domain, why is there an
option for it at all?  Maybe this should be the default and S1
(whatever that means) should be the alternate option.  Thanks,

Alex

>  
>  	ret = iommu_attach_group(domain->domain, iommu_group);

[RFC,v1,2/2] vfio/iommu-type1: set only stage 2 translation

[Alex Williamson]

On Fri, 21 Oct 2016 12:39:24 +0800
Rick Song <songwenjun@huawei.com> wrote:

> Normally, VFIO should use only stage 2 translation of
> iommu, to create the address mapping. If nesting translation
> is disabled from VFIO core, enable iommu domain only stage 2
> attribute, otherwise, enable iommu domain nesting attribute.
> 
> Signed-off-by: Rick Song <songwenjun@huawei.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 2ba1942..c0265fe 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -741,7 +741,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	struct vfio_group *group, *g;
>  	struct vfio_domain *domain, *d;
>  	struct bus_type *bus = NULL;
> -	int ret;
> +	int attr, ret;
>  
>  	mutex_lock(&iommu->lock);
>  
> @@ -775,13 +775,22 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  		goto out_free;
>  	}
>  
> +	/*
> +	 * Set iommu nesting domain attribute if nesting translation
> +	 * is enabled from iommu vfio, otherwise set iommu only stage
> +	 * 2 domain attribute.
> +	 */
> +	attr = 1;
>  	if (iommu->nesting) {
> -		int attr = 1;
> -
>  		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
>  					    &attr);
>  		if (ret)
>  			goto out_domain;
> +	} else {
> +		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_S2,
> +					    &attr);
> +		if (ret)
> +			goto out_domain;
>  	}

This attribute is not relevant to the majority of current users, why
would we assume that we need to call it for all non-nesting cases?  Why
do we need to set the attribute at all, what benefit does it provide?
If this is the normal case for an IOMMU API domain, why is there an
option for it at all?  Maybe this should be the default and S1
(whatever that means) should be the alternate option.  Thanks,

Alex

>  
>  	ret = iommu_attach_group(domain->domain, iommu_group);

Re: [RFC,v1,2/2] vfio/iommu-type1: set only stage 2 translation

[Robin Murphy]

On 21/10/16 15:27, Alex Williamson wrote:
> On Fri, 21 Oct 2016 12:39:24 +0800
> Rick Song <songwenjun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
> 
>> Normally, VFIO should use only stage 2 translation of
>> iommu, to create the address mapping. If nesting translation
>> is disabled from VFIO core, enable iommu domain only stage 2
>> attribute, otherwise, enable iommu domain nesting attribute.
>>
>> Signed-off-by: Rick Song <songwenjun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 2ba1942..c0265fe 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -741,7 +741,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>  	struct vfio_group *group, *g;
>>  	struct vfio_domain *domain, *d;
>>  	struct bus_type *bus = NULL;
>> -	int ret;
>> +	int attr, ret;
>>  
>>  	mutex_lock(&iommu->lock);
>>  
>> @@ -775,13 +775,22 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>  		goto out_free;
>>  	}
>>  
>> +	/*
>> +	 * Set iommu nesting domain attribute if nesting translation
>> +	 * is enabled from iommu vfio, otherwise set iommu only stage
>> +	 * 2 domain attribute.
>> +	 */
>> +	attr = 1;
>>  	if (iommu->nesting) {
>> -		int attr = 1;
>> -
>>  		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
>>  					    &attr);
>>  		if (ret)
>>  			goto out_domain;
>> +	} else {
>> +		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_S2,
>> +					    &attr);
>> +		if (ret)
>> +			goto out_domain;
>>  	}
> 
> This attribute is not relevant to the majority of current users, why
> would we assume that we need to call it for all non-nesting cases?  Why
> do we need to set the attribute at all, what benefit does it provide?
> If this is the normal case for an IOMMU API domain, why is there an
> option for it at all?  Maybe this should be the default and S1
> (whatever that means) should be the alternate option.  Thanks,

Indeed, it should be fairly straightforward to make
arm_smmu_domain_finalise() prefer stage 1/stage 2 based on domain->type
in the case that both stages are implemented. That would be preferable
to changing core VFIO code for something that really is SMMU-specific.

To echo Alex, though, what's the motivation for this? Could it be
addressed by simply implementing a force_stage parameter like the SMMUv2
driver has?

Robin.

> 
> Alex
> 
>>  
>>  	ret = iommu_attach_group(domain->domain, iommu_group);
> 

[RFC,v1,2/2] vfio/iommu-type1: set only stage 2 translation

[Robin Murphy]

On 21/10/16 15:27, Alex Williamson wrote:
> On Fri, 21 Oct 2016 12:39:24 +0800
> Rick Song <songwenjun@huawei.com> wrote:
> 
>> Normally, VFIO should use only stage 2 translation of
>> iommu, to create the address mapping. If nesting translation
>> is disabled from VFIO core, enable iommu domain only stage 2
>> attribute, otherwise, enable iommu domain nesting attribute.
>>
>> Signed-off-by: Rick Song <songwenjun@huawei.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 2ba1942..c0265fe 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -741,7 +741,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>  	struct vfio_group *group, *g;
>>  	struct vfio_domain *domain, *d;
>>  	struct bus_type *bus = NULL;
>> -	int ret;
>> +	int attr, ret;
>>  
>>  	mutex_lock(&iommu->lock);
>>  
>> @@ -775,13 +775,22 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>  		goto out_free;
>>  	}
>>  
>> +	/*
>> +	 * Set iommu nesting domain attribute if nesting translation
>> +	 * is enabled from iommu vfio, otherwise set iommu only stage
>> +	 * 2 domain attribute.
>> +	 */
>> +	attr = 1;
>>  	if (iommu->nesting) {
>> -		int attr = 1;
>> -
>>  		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
>>  					    &attr);
>>  		if (ret)
>>  			goto out_domain;
>> +	} else {
>> +		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_S2,
>> +					    &attr);
>> +		if (ret)
>> +			goto out_domain;
>>  	}
> 
> This attribute is not relevant to the majority of current users, why
> would we assume that we need to call it for all non-nesting cases?  Why
> do we need to set the attribute at all, what benefit does it provide?
> If this is the normal case for an IOMMU API domain, why is there an
> option for it at all?  Maybe this should be the default and S1
> (whatever that means) should be the alternate option.  Thanks,

Indeed, it should be fairly straightforward to make
arm_smmu_domain_finalise() prefer stage 1/stage 2 based on domain->type
in the case that both stages are implemented. That would be preferable
to changing core VFIO code for something that really is SMMU-specific.

To echo Alex, though, what's the motivation for this? Could it be
addressed by simply implementing a force_stage parameter like the SMMUv2
driver has?

Robin.

> 
> Alex
> 
>>  
>>  	ret = iommu_attach_group(domain->domain, iommu_group);
>