* [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
@ 2020-06-15 12:39 ` Pierre Morel
0 siblings, 0 replies; 19+ messages in thread
From: Pierre Morel @ 2020-06-15 12:39 UTC (permalink / raw)
To: linux-kernel
Cc: linux-s390, frankja, kvm, mst, cohuck, virtualization, pasic,
borntraeger
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-15 12:39 ` Pierre Morel
(?)
@ 2020-06-16 6:22 ` Jason Wang
2020-06-16 7:33 ` Pierre Morel
-1 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2020-06-16 6:22 UTC (permalink / raw)
To: Pierre Morel, linux-kernel
Cc: pasic, borntraeger, frankja, mst, cohuck, kvm, linux-s390,
virtualization
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 */
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 6:22 ` Jason Wang
@ 2020-06-16 7:33 ` Pierre Morel
0 siblings, 0 replies; 19+ messages in thread
From: Pierre Morel @ 2020-06-16 7:33 UTC (permalink / raw)
To: Jason Wang, linux-kernel
Cc: pasic, borntraeger, frankja, mst, cohuck, kvm, linux-s390,
virtualization
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-15 12:39 ` Pierre Morel
@ 2020-06-16 6:55 ` Christian Borntraeger
-1 siblings, 0 replies; 19+ messages in thread
From: Christian Borntraeger @ 2020-06-16 6:55 UTC (permalink / raw)
To: Pierre Morel, linux-kernel
Cc: pasic, frankja, mst, jasowang, cohuck, kvm, linux-s390, virtualization
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 */
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
@ 2020-06-16 6:55 ` Christian Borntraeger
0 siblings, 0 replies; 19+ messages in thread
From: Christian Borntraeger @ 2020-06-16 6:55 UTC (permalink / raw)
To: Pierre Morel, linux-kernel
Cc: linux-s390, frankja, kvm, mst, cohuck, virtualization, pasic
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 */
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 6:55 ` Christian Borntraeger
(?)
@ 2020-06-16 7:35 ` Pierre Morel
2020-06-16 12:21 ` Cornelia Huck
-1 siblings, 1 reply; 19+ messages in thread
From: Pierre Morel @ 2020-06-16 7:35 UTC (permalink / raw)
To: Christian Borntraeger, linux-kernel
Cc: pasic, frankja, mst, jasowang, cohuck, kvm, linux-s390, virtualization
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 7:35 ` Pierre Morel
@ 2020-06-16 12:21 ` Cornelia Huck
0 siblings, 0 replies; 19+ messages in thread
From: Cornelia Huck @ 2020-06-16 12:21 UTC (permalink / raw)
To: Pierre Morel
Cc: Christian Borntraeger, linux-kernel, pasic, frankja, mst,
jasowang, kvm, linux-s390, virtualization
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 :)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-15 12:39 ` Pierre Morel
@ 2020-06-16 9:52 ` Halil Pasic
-1 siblings, 0 replies; 19+ messages in thread
From: Halil Pasic @ 2020-06-16 9:52 UTC (permalink / raw)
To: Pierre Morel
Cc: linux-kernel, borntraeger, frankja, mst, jasowang, cohuck, kvm,
linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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 */
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
@ 2020-06-16 9:52 ` Halil Pasic
0 siblings, 0 replies; 19+ messages in thread
From: Halil Pasic @ 2020-06-16 9:52 UTC (permalink / raw)
To: Pierre Morel
Cc: linux-kernel, borntraeger, frankja, mst, jasowang, cohuck, kvm,
linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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 */
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 9:52 ` Halil Pasic
@ 2020-06-16 10:52 ` Pierre Morel
-1 siblings, 0 replies; 19+ messages in thread
From: Pierre Morel @ 2020-06-16 10:52 UTC (permalink / raw)
To: Halil Pasic
Cc: linux-kernel, borntraeger, frankja, mst, jasowang, cohuck, kvm,
linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
@ 2020-06-16 10:52 ` Pierre Morel
0 siblings, 0 replies; 19+ messages in thread
From: Pierre Morel @ 2020-06-16 10:52 UTC (permalink / raw)
To: Halil Pasic
Cc: Vasily Gorbik, linux-s390, frankja, kvm, mst, Heiko Carstens,
cohuck, Ram Pai, linux-kernel, virtualization, borntraeger,
thomas.lendacky, David Gibson
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 10:52 ` Pierre Morel
(?)
@ 2020-06-16 11:57 ` Halil Pasic
2020-06-16 12:17 ` Cornelia Huck
-1 siblings, 1 reply; 19+ messages in thread
From: Halil Pasic @ 2020-06-16 11:57 UTC (permalink / raw)
To: Pierre Morel
Cc: linux-kernel, borntraeger, frankja, mst, jasowang, cohuck, kvm,
linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 11:57 ` Halil Pasic
@ 2020-06-16 12:17 ` Cornelia Huck
2020-06-16 13:41 ` Pierre Morel
0 siblings, 1 reply; 19+ messages in thread
From: Cornelia Huck @ 2020-06-16 12:17 UTC (permalink / raw)
To: Halil Pasic
Cc: Pierre Morel, linux-kernel, borntraeger, frankja, mst, jasowang,
kvm, linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 12:17 ` Cornelia Huck
@ 2020-06-16 13:41 ` Pierre Morel
2020-06-16 13:50 ` Cornelia Huck
0 siblings, 1 reply; 19+ messages in thread
From: Pierre Morel @ 2020-06-16 13:41 UTC (permalink / raw)
To: Cornelia Huck, Halil Pasic
Cc: linux-kernel, borntraeger, frankja, mst, jasowang, kvm,
linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 13:41 ` Pierre Morel
@ 2020-06-16 13:50 ` Cornelia Huck
0 siblings, 0 replies; 19+ messages in thread
From: Cornelia Huck @ 2020-06-16 13:50 UTC (permalink / raw)
To: Pierre Morel
Cc: Halil Pasic, linux-kernel, borntraeger, frankja, mst, jasowang,
kvm, linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 10:52 ` Pierre Morel
(?)
(?)
@ 2020-06-16 12:20 ` Cornelia Huck
2020-06-16 13:36 ` Pierre Morel
-1 siblings, 1 reply; 19+ messages in thread
From: Cornelia Huck @ 2020-06-16 12:20 UTC (permalink / raw)
To: Pierre Morel
Cc: Halil Pasic, linux-kernel, borntraeger, frankja, mst, jasowang,
kvm, linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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)
> >> {
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature
2020-06-16 12:20 ` Cornelia Huck
@ 2020-06-16 13:36 ` Pierre Morel
0 siblings, 0 replies; 19+ messages in thread
From: Pierre Morel @ 2020-06-16 13:36 UTC (permalink / raw)
To: Cornelia Huck
Cc: Halil Pasic, linux-kernel, borntraeger, frankja, mst, jasowang,
kvm, linux-s390, virtualization, thomas.lendacky, David Gibson,
Ram Pai, Heiko Carstens, Vasily Gorbik
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
^ permalink raw reply [flat|nested] 19+ messages in thread