[PATCH v2 0/1] s390: virtio: let's arch choose to accept devices without IOMMU feature

[PATCH v2 0/1] s390: virtio: let's arch choose to accept devices without IOMMU feature

[Pierre Morel]

An architecture protecting the guest memory against unauthorized host
access may want to enforce VIRTIO I/O device protection through the
use of VIRTIO_F_IOMMU_PLATFORM.

Let's give a chance to the architecture to accept or not devices
without VIRTIO_F_IOMMU_PLATFORM.

Pierre Morel (1):
  s390: virtio: let arch accept devices without IOMMU feature

 arch/s390/mm/init.c     | 6 ++++++
 drivers/virtio/virtio.c | 9 +++++++++
 include/linux/virtio.h  | 2 ++
 3 files changed, 17 insertions(+)

-- 
2.25.1

Changelog to v2:

- put the test in virtio_finalize_features()
  (Connie)

- put the test inside VIRTIO core
  (Jason)

- pass a virtio device as parameter
  (Halil)

[PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Pierre Morel]

An architecture protecting the guest memory against unauthorized host
access may want to enforce VIRTIO I/O device protection through the
use of VIRTIO_F_IOMMU_PLATFORM.

Let's give a chance to the architecture to accept or not devices
without VIRTIO_F_IOMMU_PLATFORM.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 arch/s390/mm/init.c     | 6 ++++++
 drivers/virtio/virtio.c | 9 +++++++++
 include/linux/virtio.h  | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 87b2d024e75a..3f04ad09650f 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -46,6 +46,7 @@
 #include <asm/kasan.h>
 #include <asm/dma-mapping.h>
 #include <asm/uv.h>
+#include <linux/virtio.h>
 
 pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
 
@@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
 	return is_prot_virt_guest();
 }
 
+int arch_needs_iommu_platform(struct virtio_device *dev) 
+{
+	return is_prot_virt_guest();
+}
+
 /* protected virtualization */
 static void pv_init(void)
 {
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32a88f2..30091089bee8 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
 }
 EXPORT_SYMBOL_GPL(virtio_add_status);
 
+int __weak arch_needs_iommu_platform(struct virtio_device *dev)
+{
+	return 0;
+}
+
 int virtio_finalize_features(struct virtio_device *dev)
 {
 	int ret = dev->config->finalize_features(dev);
@@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
 	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
 		return 0;
 
+	if (arch_needs_iommu_platform(dev) &&
+		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
+		return -EIO;
+
 	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
 	status = dev->config->get_status(dev);
 	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index a493eac08393..2c46b310c38c 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
 #define module_virtio_driver(__virtio_driver) \
 	module_driver(__virtio_driver, register_virtio_driver, \
 			unregister_virtio_driver)
+
+int arch_needs_iommu_platform(struct virtio_device *dev);
 #endif /* _LINUX_VIRTIO_H */
-- 
2.25.1

[PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Pierre Morel]

An architecture protecting the guest memory against unauthorized host
access may want to enforce VIRTIO I/O device protection through the
use of VIRTIO_F_IOMMU_PLATFORM.

Let's give a chance to the architecture to accept or not devices
without VIRTIO_F_IOMMU_PLATFORM.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 arch/s390/mm/init.c     | 6 ++++++
 drivers/virtio/virtio.c | 9 +++++++++
 include/linux/virtio.h  | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 87b2d024e75a..3f04ad09650f 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -46,6 +46,7 @@
 #include <asm/kasan.h>
 #include <asm/dma-mapping.h>
 #include <asm/uv.h>
+#include <linux/virtio.h>
 
 pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
 
@@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
 	return is_prot_virt_guest();
 }
 
+int arch_needs_iommu_platform(struct virtio_device *dev) 
+{
+	return is_prot_virt_guest();
+}
+
 /* protected virtualization */
 static void pv_init(void)
 {
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32a88f2..30091089bee8 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
 }
 EXPORT_SYMBOL_GPL(virtio_add_status);
 
+int __weak arch_needs_iommu_platform(struct virtio_device *dev)
+{
+	return 0;
+}
+
 int virtio_finalize_features(struct virtio_device *dev)
 {
 	int ret = dev->config->finalize_features(dev);
@@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
 	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
 		return 0;
 
+	if (arch_needs_iommu_platform(dev) &&
+		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
+		return -EIO;
+
 	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
 	status = dev->config->get_status(dev);
 	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index a493eac08393..2c46b310c38c 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
 #define module_virtio_driver(__virtio_driver) \
 	module_driver(__virtio_driver, register_virtio_driver, \
 			unregister_virtio_driver)
+
+int arch_needs_iommu_platform(struct virtio_device *dev);
 #endif /* _LINUX_VIRTIO_H */
-- 
2.25.1

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Jason Wang]

On 2020/6/15 下午8:39, Pierre Morel wrote:
> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
>
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   arch/s390/mm/init.c     | 6 ++++++
>   drivers/virtio/virtio.c | 9 +++++++++
>   include/linux/virtio.h  | 2 ++
>   3 files changed, 17 insertions(+)
>
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 87b2d024e75a..3f04ad09650f 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -46,6 +46,7 @@
>   #include <asm/kasan.h>
>   #include <asm/dma-mapping.h>
>   #include <asm/uv.h>
> +#include <linux/virtio.h>
>   
>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>   
> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>   	return is_prot_virt_guest();
>   }
>   
> +int arch_needs_iommu_platform(struct virtio_device *dev)
> +{
> +	return is_prot_virt_guest();
> +}
> +
>   /* protected virtualization */
>   static void pv_init(void)
>   {
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..30091089bee8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>   }
>   EXPORT_SYMBOL_GPL(virtio_add_status);
>   
> +int __weak arch_needs_iommu_platform(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +
>   int virtio_finalize_features(struct virtio_device *dev)
>   {
>   	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>   		return 0;
>   
> +	if (arch_needs_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
> +		return -EIO;
> +
>   	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>   	status = dev->config->get_status(dev);
>   	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a493eac08393..2c46b310c38c 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>   #define module_virtio_driver(__virtio_driver) \
>   	module_driver(__virtio_driver, register_virtio_driver, \
>   			unregister_virtio_driver)
> +
> +int arch_needs_iommu_platform(struct virtio_device *dev);
>   #endif /* _LINUX_VIRTIO_H */

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Christian Borntraeger]

On 15.06.20 14:39, Pierre Morel wrote:
> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> 
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>


Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Shouldnt we maybe add a pr_warn if that happens to help the admins to understand what is going on?


> ---
>  arch/s390/mm/init.c     | 6 ++++++
>  drivers/virtio/virtio.c | 9 +++++++++
>  include/linux/virtio.h  | 2 ++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 87b2d024e75a..3f04ad09650f 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -46,6 +46,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio.h>
>  
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>  
> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
>  
> +int arch_needs_iommu_platform(struct virtio_device *dev) 
> +{
> +	return is_prot_virt_guest();
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..30091089bee8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>  }
>  EXPORT_SYMBOL_GPL(virtio_add_status);
>  
> +int __weak arch_needs_iommu_platform(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +
>  int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>  		return 0;
>  
> +	if (arch_needs_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
> +		return -EIO;
> +
>  	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>  	status = dev->config->get_status(dev);
>  	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a493eac08393..2c46b310c38c 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>  #define module_virtio_driver(__virtio_driver) \
>  	module_driver(__virtio_driver, register_virtio_driver, \
>  			unregister_virtio_driver)
> +
> +int arch_needs_iommu_platform(struct virtio_device *dev);
>  #endif /* _LINUX_VIRTIO_H */
> 

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Christian Borntraeger]

On 15.06.20 14:39, Pierre Morel wrote:
> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> 
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>


Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Shouldnt we maybe add a pr_warn if that happens to help the admins to understand what is going on?


> ---
>  arch/s390/mm/init.c     | 6 ++++++
>  drivers/virtio/virtio.c | 9 +++++++++
>  include/linux/virtio.h  | 2 ++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 87b2d024e75a..3f04ad09650f 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -46,6 +46,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio.h>
>  
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>  
> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
>  
> +int arch_needs_iommu_platform(struct virtio_device *dev) 
> +{
> +	return is_prot_virt_guest();
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..30091089bee8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>  }
>  EXPORT_SYMBOL_GPL(virtio_add_status);
>  
> +int __weak arch_needs_iommu_platform(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +
>  int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>  		return 0;
>  
> +	if (arch_needs_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
> +		return -EIO;
> +
>  	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>  	status = dev->config->get_status(dev);
>  	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a493eac08393..2c46b310c38c 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>  #define module_virtio_driver(__virtio_driver) \
>  	module_driver(__virtio_driver, register_virtio_driver, \
>  			unregister_virtio_driver)
> +
> +int arch_needs_iommu_platform(struct virtio_device *dev);
>  #endif /* _LINUX_VIRTIO_H */
> 

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Pierre Morel]

On 2020-06-16 08:22, Jason Wang wrote:
> 
> On 2020/6/15 下午8:39, Pierre Morel wrote:
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>>
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> 
> Acked-by: Jason Wang <jasowang@redhat.com>

Thanks,

Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Pierre Morel]

On 2020-06-16 08:55, Christian Borntraeger wrote:
> 
> 
> On 15.06.20 14:39, Pierre Morel wrote:
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>>
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> 
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks,

> 
> Shouldnt we maybe add a pr_warn if that happens to help the admins to understand what is going on?
> 
> 

Yes, Connie asked for it too, good that you remind it to me, I add it.

Thanks,
Pierre

>> ---
>>   arch/s390/mm/init.c     | 6 ++++++
>>   drivers/virtio/virtio.c | 9 +++++++++
>>   include/linux/virtio.h  | 2 ++
>>   3 files changed, 17 insertions(+)
>>
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index 87b2d024e75a..3f04ad09650f 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -46,6 +46,7 @@
>>   #include <asm/kasan.h>
>>   #include <asm/dma-mapping.h>
>>   #include <asm/uv.h>
>> +#include <linux/virtio.h>
>>   
>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>   
>> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>>   	return is_prot_virt_guest();
>>   }
>>   
>> +int arch_needs_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return is_prot_virt_guest();
>> +}
>> +
>>   /* protected virtualization */
>>   static void pv_init(void)
>>   {
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index a977e32a88f2..30091089bee8 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>>   }
>>   EXPORT_SYMBOL_GPL(virtio_add_status);
>>   
>> +int __weak arch_needs_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return 0;
>> +}
>> +
>>   int virtio_finalize_features(struct virtio_device *dev)
>>   {
>>   	int ret = dev->config->finalize_features(dev);
>> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>   		return 0;
>>   
>> +	if (arch_needs_iommu_platform(dev) &&
>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
>> +		return -EIO;
>> +
>>   	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>>   	status = dev->config->get_status(dev);
>>   	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
>> index a493eac08393..2c46b310c38c 100644
>> --- a/include/linux/virtio.h
>> +++ b/include/linux/virtio.h
>> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>>   #define module_virtio_driver(__virtio_driver) \
>>   	module_driver(__virtio_driver, register_virtio_driver, \
>>   			unregister_virtio_driver)
>> +
>> +int arch_needs_iommu_platform(struct virtio_device *dev);
>>   #endif /* _LINUX_VIRTIO_H */
>>

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Halil Pasic]

On Mon, 15 Jun 2020 14:39:24 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

I find the subject (commit short) sub optimal. The 'arch' is already
accepting devices 'without IOMMU feature'. What you are introducing is
the ability to reject.

> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> 
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
> 

I don't particularly like the commit message. In general, I believe
using access_platform instead of iommu_platform would really benefit us.

> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/mm/init.c     | 6 ++++++
>  drivers/virtio/virtio.c | 9 +++++++++
>  include/linux/virtio.h  | 2 ++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 87b2d024e75a..3f04ad09650f 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -46,6 +46,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio.h>

arch/s390/mm/init.c including virtio.h looks a bit strange to me, but
if Heiko and Vasily don't mind, neither do I.

>  
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>  
> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
>  
> +int arch_needs_iommu_platform(struct virtio_device *dev) 

Maybe prefixing the name with virtio_ would help provide the
proper context.

> +{
> +	return is_prot_virt_guest();
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..30091089bee8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>  }
>  EXPORT_SYMBOL_GPL(virtio_add_status);
>  
> +int __weak arch_needs_iommu_platform(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +

Adding some people that could be interested in overriding this as well
to the cc list.

>  int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>  		return 0;
>  
> +	if (arch_needs_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
> +		return -EIO;
> +

Why EIO?

Overall, I think it is a good idea to have something that is going to
protect us from this scenario.

Regards,
Halil

>  	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>  	status = dev->config->get_status(dev);
>  	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a493eac08393..2c46b310c38c 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>  #define module_virtio_driver(__virtio_driver) \
>  	module_driver(__virtio_driver, register_virtio_driver, \
>  			unregister_virtio_driver)
> +
> +int arch_needs_iommu_platform(struct virtio_device *dev);
>  #endif /* _LINUX_VIRTIO_H */

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Halil Pasic]

On Mon, 15 Jun 2020 14:39:24 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

I find the subject (commit short) sub optimal. The 'arch' is already
accepting devices 'without IOMMU feature'. What you are introducing is
the ability to reject.

> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> 
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
> 

I don't particularly like the commit message. In general, I believe
using access_platform instead of iommu_platform would really benefit us.

> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/mm/init.c     | 6 ++++++
>  drivers/virtio/virtio.c | 9 +++++++++
>  include/linux/virtio.h  | 2 ++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 87b2d024e75a..3f04ad09650f 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -46,6 +46,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio.h>

arch/s390/mm/init.c including virtio.h looks a bit strange to me, but
if Heiko and Vasily don't mind, neither do I.

>  
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>  
> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
>  
> +int arch_needs_iommu_platform(struct virtio_device *dev) 

Maybe prefixing the name with virtio_ would help provide the
proper context.

> +{
> +	return is_prot_virt_guest();
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..30091089bee8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>  }
>  EXPORT_SYMBOL_GPL(virtio_add_status);
>  
> +int __weak arch_needs_iommu_platform(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +

Adding some people that could be interested in overriding this as well
to the cc list.

>  int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>  		return 0;
>  
> +	if (arch_needs_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
> +		return -EIO;
> +

Why EIO?

Overall, I think it is a good idea to have something that is going to
protect us from this scenario.

Regards,
Halil

>  	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>  	status = dev->config->get_status(dev);
>  	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a493eac08393..2c46b310c38c 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>  #define module_virtio_driver(__virtio_driver) \
>  	module_driver(__virtio_driver, register_virtio_driver, \
>  			unregister_virtio_driver)
> +
> +int arch_needs_iommu_platform(struct virtio_device *dev);
>  #endif /* _LINUX_VIRTIO_H */

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Pierre Morel]

On 2020-06-16 11:52, Halil Pasic wrote:
> On Mon, 15 Jun 2020 14:39:24 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> I find the subject (commit short) sub optimal. The 'arch' is already
> accepting devices 'without IOMMU feature'. What you are introducing is
> the ability to reject.
> 
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>>
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
>>
> 
> I don't particularly like the commit message. In general, I believe
> using access_platform instead of iommu_platform would really benefit us.

IOMMU_PLATFORM is used overall in Linux, and I did not find any 
occurrence for ACCESS_PLATFORM.


> 
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   arch/s390/mm/init.c     | 6 ++++++
>>   drivers/virtio/virtio.c | 9 +++++++++
>>   include/linux/virtio.h  | 2 ++
>>   3 files changed, 17 insertions(+)
>>
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index 87b2d024e75a..3f04ad09650f 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -46,6 +46,7 @@
>>   #include <asm/kasan.h>
>>   #include <asm/dma-mapping.h>
>>   #include <asm/uv.h>
>> +#include <linux/virtio.h>
> 
> arch/s390/mm/init.c including virtio.h looks a bit strange to me, but
> if Heiko and Vasily don't mind, neither do I.

Do we have a better place to install the hook?
I though that since it is related to memory management and that, since 
force_dma_unencrypted already is there, it would be a good place.

However, kvm-s390 is another candidate.

> 
>>   
>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>   
>> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>>   	return is_prot_virt_guest();
>>   }
>>   
>> +int arch_needs_iommu_platform(struct virtio_device *dev)
> 
> Maybe prefixing the name with virtio_ would help provide the
> proper context.

The virtio_dev makes it obvious and from the virtio side it should be 
obvious that the arch is responsible for this.

However if nobody has something against I change it.

> 
>> +{
>> +	return is_prot_virt_guest();
>> +}
>> +
>>   /* protected virtualization */
>>   static void pv_init(void)
>>   {
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index a977e32a88f2..30091089bee8 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>>   }
>>   EXPORT_SYMBOL_GPL(virtio_add_status);
>>   
>> +int __weak arch_needs_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return 0;
>> +}
>> +
> 
> Adding some people that could be interested in overriding this as well
> to the cc list.

Thanks,

> 
>>   int virtio_finalize_features(struct virtio_device *dev)
>>   {
>>   	int ret = dev->config->finalize_features(dev);
>> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>   		return 0;
>>   
>> +	if (arch_needs_iommu_platform(dev) &&
>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
>> +		return -EIO;
>> +
> 
> Why EIO?

Because I/O can not occur correctly?
I am open to suggestions.

> 
> Overall, I think it is a good idea to have something that is going to
> protect us from this scenario.
> 

It would clearly be a good thing that trusted hypervizors like QEMU 
forbid this scenario however should we let the door open?

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Pierre Morel]

On 2020-06-16 11:52, Halil Pasic wrote:
> On Mon, 15 Jun 2020 14:39:24 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> I find the subject (commit short) sub optimal. The 'arch' is already
> accepting devices 'without IOMMU feature'. What you are introducing is
> the ability to reject.
> 
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>>
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
>>
> 
> I don't particularly like the commit message. In general, I believe
> using access_platform instead of iommu_platform would really benefit us.

IOMMU_PLATFORM is used overall in Linux, and I did not find any 
occurrence for ACCESS_PLATFORM.


> 
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   arch/s390/mm/init.c     | 6 ++++++
>>   drivers/virtio/virtio.c | 9 +++++++++
>>   include/linux/virtio.h  | 2 ++
>>   3 files changed, 17 insertions(+)
>>
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index 87b2d024e75a..3f04ad09650f 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -46,6 +46,7 @@
>>   #include <asm/kasan.h>
>>   #include <asm/dma-mapping.h>
>>   #include <asm/uv.h>
>> +#include <linux/virtio.h>
> 
> arch/s390/mm/init.c including virtio.h looks a bit strange to me, but
> if Heiko and Vasily don't mind, neither do I.

Do we have a better place to install the hook?
I though that since it is related to memory management and that, since 
force_dma_unencrypted already is there, it would be a good place.

However, kvm-s390 is another candidate.

> 
>>   
>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>   
>> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>>   	return is_prot_virt_guest();
>>   }
>>   
>> +int arch_needs_iommu_platform(struct virtio_device *dev)
> 
> Maybe prefixing the name with virtio_ would help provide the
> proper context.

The virtio_dev makes it obvious and from the virtio side it should be 
obvious that the arch is responsible for this.

However if nobody has something against I change it.

> 
>> +{
>> +	return is_prot_virt_guest();
>> +}
>> +
>>   /* protected virtualization */
>>   static void pv_init(void)
>>   {
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index a977e32a88f2..30091089bee8 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>>   }
>>   EXPORT_SYMBOL_GPL(virtio_add_status);
>>   
>> +int __weak arch_needs_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return 0;
>> +}
>> +
> 
> Adding some people that could be interested in overriding this as well
> to the cc list.

Thanks,

> 
>>   int virtio_finalize_features(struct virtio_device *dev)
>>   {
>>   	int ret = dev->config->finalize_features(dev);
>> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>   		return 0;
>>   
>> +	if (arch_needs_iommu_platform(dev) &&
>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
>> +		return -EIO;
>> +
> 
> Why EIO?

Because I/O can not occur correctly?
I am open to suggestions.

> 
> Overall, I think it is a good idea to have something that is going to
> protect us from this scenario.
> 

It would clearly be a good thing that trusted hypervizors like QEMU 
forbid this scenario however should we let the door open?

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Halil Pasic]

On Tue, 16 Jun 2020 12:52:50 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> >>   int virtio_finalize_features(struct virtio_device *dev)
> >>   {
> >>   	int ret = dev->config->finalize_features(dev);
> >> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
> >>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> >>   		return 0;
> >>   
> >> +	if (arch_needs_iommu_platform(dev) &&
> >> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
> >> +		return -EIO;
> >> +  
> > 
> > Why EIO?  
> 
> Because I/O can not occur correctly?
> I am open to suggestions.

We use -ENODEV if feature when the device rejects the features we
tried to negotiate (see virtio_finalize_features()) and -EINVAL when
the F_VERSION_1 and the virtio-ccw revision ain't coherent (in
virtio_ccw_finalize_features()). Any of those seems more fitting
that EIO to me. BTW does the error code itself matter in any way,
or is it just OK vs some error?

Regards,
Halil

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Cornelia Huck]

On Tue, 16 Jun 2020 13:57:26 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 16 Jun 2020 12:52:50 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > >>   int virtio_finalize_features(struct virtio_device *dev)
> > >>   {
> > >>   	int ret = dev->config->finalize_features(dev);
> > >> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
> > >>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> > >>   		return 0;
> > >>   
> > >> +	if (arch_needs_iommu_platform(dev) &&
> > >> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
> > >> +		return -EIO;
> > >> +    
> > > 
> > > Why EIO?    
> > 
> > Because I/O can not occur correctly?
> > I am open to suggestions.  
> 
> We use -ENODEV if feature when the device rejects the features we
> tried to negotiate (see virtio_finalize_features()) and -EINVAL when
> the F_VERSION_1 and the virtio-ccw revision ain't coherent (in
> virtio_ccw_finalize_features()). Any of those seems more fitting
> that EIO to me. BTW does the error code itself matter in any way,
> or is it just OK vs some error?

If I haven't lost my way, we end up in the driver core probe failure
handling; we probably should do -ENODEV if we just want probing to fail
and -EINVAL or -EIO if we want the code to moan.

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Cornelia Huck]

On Tue, 16 Jun 2020 12:52:50 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-06-16 11:52, Halil Pasic wrote:
> > On Mon, 15 Jun 2020 14:39:24 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

> >> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
> >>   	return is_prot_virt_guest();
> >>   }
> >>   
> >> +int arch_needs_iommu_platform(struct virtio_device *dev)  
> > 
> > Maybe prefixing the name with virtio_ would help provide the
> > proper context.  
> 
> The virtio_dev makes it obvious and from the virtio side it should be 
> obvious that the arch is responsible for this.
> 
> However if nobody has something against I change it.

arch_needs_virtio_iommu_platform()?

> 
> >   
> >> +{
> >> +	return is_prot_virt_guest();
> >> +}
> >> +
> >>   /* protected virtualization */
> >>   static void pv_init(void)
> >>   {

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Cornelia Huck]

On Tue, 16 Jun 2020 09:35:19 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-06-16 08:55, Christian Borntraeger wrote:
> > 
> > 
> > On 15.06.20 14:39, Pierre Morel wrote:  
> >> An architecture protecting the guest memory against unauthorized host
> >> access may want to enforce VIRTIO I/O device protection through the
> >> use of VIRTIO_F_IOMMU_PLATFORM.
> >>
> >> Let's give a chance to the architecture to accept or not devices
> >> without VIRTIO_F_IOMMU_PLATFORM.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>  
> > 
> > 
> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>  
> 
> Thanks,
> 
> > 
> > Shouldnt we maybe add a pr_warn if that happens to help the admins to understand what is going on?
> > 
> >   
> 
> Yes, Connie asked for it too, good that you remind it to me, I add it.

Yes, please :)

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Pierre Morel]

On 2020-06-16 14:20, Cornelia Huck wrote:
> On Tue, 16 Jun 2020 12:52:50 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2020-06-16 11:52, Halil Pasic wrote:
>>> On Mon, 15 Jun 2020 14:39:24 +0200
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>> @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev)
>>>>    	return is_prot_virt_guest();
>>>>    }
>>>>    
>>>> +int arch_needs_iommu_platform(struct virtio_device *dev)
>>>
>>> Maybe prefixing the name with virtio_ would help provide the
>>> proper context.
>>
>> The virtio_dev makes it obvious and from the virtio side it should be
>> obvious that the arch is responsible for this.
>>
>> However if nobody has something against I change it.
> 
> arch_needs_virtio_iommu_platform()?

fine with me

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Pierre Morel]

On 2020-06-16 14:17, Cornelia Huck wrote:
> On Tue, 16 Jun 2020 13:57:26 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On Tue, 16 Jun 2020 12:52:50 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>>>>    int virtio_finalize_features(struct virtio_device *dev)
>>>>>    {
>>>>>    	int ret = dev->config->finalize_features(dev);
>>>>> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>>>>>    	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>>>>    		return 0;
>>>>>    
>>>>> +	if (arch_needs_iommu_platform(dev) &&
>>>>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
>>>>> +		return -EIO;
>>>>> +
>>>>
>>>> Why EIO?
>>>
>>> Because I/O can not occur correctly?
>>> I am open to suggestions.
>>
>> We use -ENODEV if feature when the device rejects the features we
>> tried to negotiate (see virtio_finalize_features()) and -EINVAL when
>> the F_VERSION_1 and the virtio-ccw revision ain't coherent (in
>> virtio_ccw_finalize_features()). Any of those seems more fitting
>> that EIO to me. BTW does the error code itself matter in any way,
>> or is it just OK vs some error?
> 
> If I haven't lost my way, we end up in the driver core probe failure
> handling; we probably should do -ENODEV if we just want probing to fail
> and -EINVAL or -EIO if we want the code to moan.
> 

what about returning -ENODEV and add a dedicated warning here?

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature

[Cornelia Huck]

On Tue, 16 Jun 2020 15:41:20 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-06-16 14:17, Cornelia Huck wrote:
> > On Tue, 16 Jun 2020 13:57:26 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> On Tue, 16 Jun 2020 12:52:50 +0200
> >> Pierre Morel <pmorel@linux.ibm.com> wrote:
> >>  
> >>>>>    int virtio_finalize_features(struct virtio_device *dev)
> >>>>>    {
> >>>>>    	int ret = dev->config->finalize_features(dev);
> >>>>> @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev)
> >>>>>    	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> >>>>>    		return 0;
> >>>>>    
> >>>>> +	if (arch_needs_iommu_platform(dev) &&
> >>>>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM))
> >>>>> +		return -EIO;
> >>>>> +  
> >>>>
> >>>> Why EIO?  
> >>>
> >>> Because I/O can not occur correctly?
> >>> I am open to suggestions.  
> >>
> >> We use -ENODEV if feature when the device rejects the features we
> >> tried to negotiate (see virtio_finalize_features()) and -EINVAL when
> >> the F_VERSION_1 and the virtio-ccw revision ain't coherent (in
> >> virtio_ccw_finalize_features()). Any of those seems more fitting
> >> that EIO to me. BTW does the error code itself matter in any way,
> >> or is it just OK vs some error?  
> > 
> > If I haven't lost my way, we end up in the driver core probe failure
> > handling; we probably should do -ENODEV if we just want probing to fail
> > and -EINVAL or -EIO if we want the code to moan.
> >   
> 
> what about returning -ENODEV and add a dedicated warning here?
> 

Sounds good at least to me.