[1/5] iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains

Message ID 1484849955-1871-2-git-send-email-will.deacon@arm.com
State Accepted
Commit 0834cc28fa56c65887c614b6c045be2ba06fdcb0
Headers show
Series
  • Implement SMMU passthrough using the default domain
Related show

Commit Message

Will Deacon Jan. 19, 2017, 6:19 p.m.
The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute,
which allows callers of the IOMMU API to request that the page table
for a domain is installed at stage-2, if supported by the hardware.

Since setting this attribute only makes sense for UNMANAGED domains,
this patch returns -ENODEV if the domain_{get,set}_attr operations are
called on other domain types.

Signed-off-by: Will Deacon <will.deacon@arm.com>

---
 drivers/iommu/arm-smmu-v3.c | 6 ++++++
 drivers/iommu/arm-smmu.c    | 6 ++++++
 2 files changed, 12 insertions(+)

-- 
2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Robin Murphy Jan. 19, 2017, 6:41 p.m. | #1
On 19/01/17 18:19, Will Deacon wrote:
> The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute,

> which allows callers of the IOMMU API to request that the page table

> for a domain is installed at stage-2, if supported by the hardware.

> 

> Since setting this attribute only makes sense for UNMANAGED domains,

> this patch returns -ENODEV if the domain_{get,set}_attr operations are

> called on other domain types.


For the sake of discussion, would it make sense to enforce this in
domain_set_attr() itself? The intersection of drivers providing these
callbacks and drivers supporting anything other than unmanaged domains
is currently these two below, so it clearly wouldn't break anything to
put this check in core code today. Looking forward, is there likely to
be any plausible situation where users of a managed domain would be
legitimate in mucking about with its attrs, on any platform?

Robin.

> Signed-off-by: Will Deacon <will.deacon@arm.com>

> ---

>  drivers/iommu/arm-smmu-v3.c | 6 ++++++

>  drivers/iommu/arm-smmu.c    | 6 ++++++

>  2 files changed, 12 insertions(+)

> 

> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c

> index 4d6ec444a9d6..c254325b0c7a 100644

> --- a/drivers/iommu/arm-smmu-v3.c

> +++ b/drivers/iommu/arm-smmu-v3.c

> @@ -1839,6 +1839,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,

>  {

>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);

>  

> +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)

> +		return -ENODEV;

> +

>  	switch (attr) {

>  	case DOMAIN_ATTR_NESTING:

>  		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);

> @@ -1854,6 +1857,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,

>  	int ret = 0;

>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);

>  

> +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)

> +		return -ENODEV;

> +

>  	mutex_lock(&smmu_domain->init_mutex);

>  

>  	switch (attr) {

> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c

> index a60cded8a6ed..a328ffb75509 100644

> --- a/drivers/iommu/arm-smmu.c

> +++ b/drivers/iommu/arm-smmu.c

> @@ -1497,6 +1497,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,

>  {

>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);

>  

> +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)

> +		return -ENODEV;

> +

>  	switch (attr) {

>  	case DOMAIN_ATTR_NESTING:

>  		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);

> @@ -1512,6 +1515,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,

>  	int ret = 0;

>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);

>  

> +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)

> +		return -ENODEV;

> +

>  	mutex_lock(&smmu_domain->init_mutex);

>  

>  	switch (attr) {

> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Joerg Roedel Jan. 26, 2017, 5:03 p.m. | #2
On Thu, Jan 19, 2017 at 06:19:11PM +0000, Will Deacon wrote:
> The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute,

> which allows callers of the IOMMU API to request that the page table

> for a domain is installed at stage-2, if supported by the hardware.

> 

> Since setting this attribute only makes sense for UNMANAGED domains,

> this patch returns -ENODEV if the domain_{get,set}_attr operations are

> called on other domain types.


Isn't -EINVAL more suitable here? In the end the domain passed in is
invalid because it does not support attributes, no?



	Joerg


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Joerg Roedel Jan. 26, 2017, 5:06 p.m. | #3
On Thu, Jan 19, 2017 at 06:41:34PM +0000, Robin Murphy wrote:
> For the sake of discussion, would it make sense to enforce this in

> domain_set_attr() itself? The intersection of drivers providing these

> callbacks and drivers supporting anything other than unmanaged domains

> is currently these two below, so it clearly wouldn't break anything to

> put this check in core code today. Looking forward, is there likely to

> be any plausible situation where users of a managed domain would be

> legitimate in mucking about with its attrs, on any platform?


No, this belongs in driver code. I am pretty certain there will be a use
for attributes in unmanaged domains at some point. Crazy things happen
all the time :)


	Joerg


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon Jan. 26, 2017, 5:57 p.m. | #4
On Thu, Jan 26, 2017 at 06:03:30PM +0100, Joerg Roedel wrote:
> On Thu, Jan 19, 2017 at 06:19:11PM +0000, Will Deacon wrote:

> > The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute,

> > which allows callers of the IOMMU API to request that the page table

> > for a domain is installed at stage-2, if supported by the hardware.

> > 

> > Since setting this attribute only makes sense for UNMANAGED domains,

> > this patch returns -ENODEV if the domain_{get,set}_attr operations are

> > called on other domain types.

> 

> Isn't -EINVAL more suitable here? In the end the domain passed in is

> invalid because it does not support attributes, no?


Sure, I can change that.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Patch hide | download patch | download mbox

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4d6ec444a9d6..c254325b0c7a 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1839,6 +1839,9 @@  static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -ENODEV;
+
 	switch (attr) {
 	case DOMAIN_ATTR_NESTING:
 		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
@@ -1854,6 +1857,9 @@  static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	int ret = 0;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -ENODEV;
+
 	mutex_lock(&smmu_domain->init_mutex);
 
 	switch (attr) {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a60cded8a6ed..a328ffb75509 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1497,6 +1497,9 @@  static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -ENODEV;
+
 	switch (attr) {
 	case DOMAIN_ATTR_NESTING:
 		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
@@ -1512,6 +1515,9 @@  static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	int ret = 0;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -ENODEV;
+
 	mutex_lock(&smmu_domain->init_mutex);
 
 	switch (attr) {