* [PATCH v4 0/2] s390: virtio: let arch validate VIRTIO features
@ 2020-07-07 8:44 Pierre Morel
2020-07-07 8:44 ` [PATCH v4 1/2] " Pierre Morel
2020-07-07 8:44 ` [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
0 siblings, 2 replies; 14+ messages in thread
From: Pierre Morel @ 2020-07-07 8:44 UTC (permalink / raw)
To: linux-kernel
Cc: pasic, borntraeger, frankja, mst, jasowang, cohuck, kvm,
linux-s390, virtualization, thomas.lendacky, david, linuxram,
heiko.carstens, gor
Hi all,
I changed the patch subject to reflect the content, becoming more
general.
1) I removed the ack from Christian and Jason even far as
I understand they gave it for the functionality more than for the
implementation.
@Jason, @Christian, please can I get back your acked-by with these changes?
2) previous patch had another name:
[PATCH v3 0/1] s390: virtio: let arch choose to accept devices without IOMMU feature
id: Message-Id: <1592390637-17441-2-git-send-email-pmorel@linux.ibm.com>
3) The new version generalize the validation of the features by the
architecture, making it not IOMMU_PLATFORM specific anymore inside
virtio.c
The architecture specific code for s390 is now testing the virtio
features.
4) Since I reworked the patch I also moved the arch specific code
from arch/s390/mm/init.c to arch/s390/kernel/to uv.c
5) Finaly, I splitted the patch into generic virtio and arch
specific code.
Regards,
Pierre
Pierre Morel (2):
virtio: let arch validate VIRTIO features
s390: virtio: PV needs VIRTIO I/O device protection
arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
drivers/virtio/virtio.c | 19 +++++++++++++++++++
include/linux/virtio_config.h | 1 +
3 files changed, 45 insertions(+)
--
2.25.1
Changelog
to v4:
- separate virtio and arch code
(Pierre)
- moved code from arch/s390/mm/init.c to arch/s390/kernel/uv.c
(Heiko)
- moved validation inside the arch code
(Connie)
- moved the call to arch validation before VIRTIO_F_1 test
(Michael)
to v3:
- add warning
(Connie, Christian)
- add comment
(Connie)
- change hook name
(Halil, Connie)
to v2:
- put the test in virtio_finalize_features()
(Connie)
- put the test inside VIRTIO core
(Jason)
- pass a virtio device as parameter
(Halil)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 1/2] virtio: let arch validate VIRTIO features
2020-07-07 8:44 [PATCH v4 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel
@ 2020-07-07 8:44 ` Pierre Morel
2020-07-07 9:26 ` Cornelia Huck
2020-07-07 8:44 ` [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
1 sibling, 1 reply; 14+ messages in thread
From: Pierre Morel @ 2020-07-07 8:44 UTC (permalink / raw)
To: linux-kernel
Cc: pasic, borntraeger, frankja, mst, jasowang, cohuck, kvm,
linux-s390, virtualization, thomas.lendacky, david, linuxram,
heiko.carstens, gor
An architecture may need to validate the VIRTIO devices features
based on architecture specificities.
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
drivers/virtio/virtio.c | 19 +++++++++++++++++++
include/linux/virtio_config.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32a88f2..3179a8aa76f5 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
}
EXPORT_SYMBOL_GPL(virtio_add_status);
+/*
+ * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
+ * features for VIRTIO device dev
+ * @dev: the VIRTIO device being added
+ *
+ * Permits the platform to provide architecture specific functionality when
+ * devices features are finalized. This is the default implementation.
+ * Architecture implementations can override this.
+ */
+
+int __weak arch_validate_virtio_features(struct virtio_device *dev)
+{
+ return 0;
+}
+
int virtio_finalize_features(struct virtio_device *dev)
{
int ret = dev->config->finalize_features(dev);
@@ -176,6 +191,10 @@ int virtio_finalize_features(struct virtio_device *dev)
if (ret)
return ret;
+ ret = arch_validate_virtio_features(dev);
+ if (ret)
+ return ret;
+
if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
return 0;
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index bb4cc4910750..3f4117adf311 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -459,4 +459,5 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
_r; \
})
+int arch_validate_virtio_features(struct virtio_device *dev);
#endif /* _LINUX_VIRTIO_CONFIG_H */
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
2020-07-07 8:44 [PATCH v4 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel
2020-07-07 8:44 ` [PATCH v4 1/2] " Pierre Morel
@ 2020-07-07 8:44 ` Pierre Morel
2020-07-07 9:46 ` Cornelia Huck
2020-07-07 11:12 ` Christian Borntraeger
1 sibling, 2 replies; 14+ messages in thread
From: Pierre Morel @ 2020-07-07 8:44 UTC (permalink / raw)
To: linux-kernel
Cc: pasic, borntraeger, frankja, mst, jasowang, cohuck, kvm,
linux-s390, virtualization, thomas.lendacky, david, linuxram,
heiko.carstens, gor
S390, protecting the guest memory against unauthorized host access
needs to enforce VIRTIO I/O device protection through the use of
VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index c296e5c8dbf9..106330f6eda1 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -14,6 +14,7 @@
#include <linux/memblock.h>
#include <linux/pagemap.h>
#include <linux/swap.h>
+#include <linux/virtio_config.h>
#include <asm/facility.h>
#include <asm/sections.h>
#include <asm/uv.h>
@@ -413,3 +414,27 @@ static int __init uv_info_init(void)
}
device_initcall(uv_info_init);
#endif
+
+/*
+ * arch_validate_virtio_iommu_platform
+ * @dev: the VIRTIO device being added
+ *
+ * Return value: returns -ENODEV if any features of the
+ * device breaks the protected virtualization
+ * 0 otherwise.
+ */
+int arch_validate_virtio_features(struct virtio_device *dev)
+{
+ if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+ dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
+ return is_prot_virt_guest() ? -ENODEV : 0;
+ }
+
+ if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
+ dev_warn(&dev->dev,
+ "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
+ return is_prot_virt_guest() ? -ENODEV : 0;
+ }
+
+ return 0;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] virtio: let arch validate VIRTIO features
2020-07-07 8:44 ` [PATCH v4 1/2] " Pierre Morel
@ 2020-07-07 9:26 ` Cornelia Huck
2020-07-07 10:39 ` Pierre Morel
2020-07-07 11:09 ` Christian Borntraeger
0 siblings, 2 replies; 14+ messages in thread
From: Cornelia Huck @ 2020-07-07 9:26 UTC (permalink / raw)
To: Pierre Morel
Cc: linux-kernel, pasic, borntraeger, frankja, mst, jasowang, kvm,
linux-s390, virtualization, thomas.lendacky, david, linuxram,
heiko.carstens, gor
On Tue, 7 Jul 2020 10:44:36 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:
> An architecture may need to validate the VIRTIO devices features
> based on architecture specificities.
s/specifities/specifics/
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
> drivers/virtio/virtio.c | 19 +++++++++++++++++++
> include/linux/virtio_config.h | 1 +
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..3179a8aa76f5 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
> }
> EXPORT_SYMBOL_GPL(virtio_add_status);
>
> +/*
> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
s/arch_needs_virtio_iommu_platform/arch_validate_virtio_features/
:)
> + * features for VIRTIO device dev
> + * @dev: the VIRTIO device being added
> + *
> + * Permits the platform to provide architecture specific functionality when
s/provide architecture specific functionality/handle architecture-specific requirements/
?
> + * devices features are finalized. This is the default implementation.
s/devices/device/
> + * Architecture implementations can override this.
> + */
> +
> +int __weak arch_validate_virtio_features(struct virtio_device *dev)
> +{
> + return 0;
> +}
> +
> int virtio_finalize_features(struct virtio_device *dev)
> {
> int ret = dev->config->finalize_features(dev);
> @@ -176,6 +191,10 @@ int virtio_finalize_features(struct virtio_device *dev)
> if (ret)
> return ret;
>
> + ret = arch_validate_virtio_features(dev);
> + if (ret)
> + return ret;
> +
> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> return 0;
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index bb4cc4910750..3f4117adf311 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -459,4 +459,5 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
> _r; \
> })
>
> +int arch_validate_virtio_features(struct virtio_device *dev);
> #endif /* _LINUX_VIRTIO_CONFIG_H */
With the wording fixed,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
2020-07-07 8:44 ` [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
@ 2020-07-07 9:46 ` Cornelia Huck
2020-07-07 10:38 ` Pierre Morel
2020-07-07 11:14 ` Michael S. Tsirkin
2020-07-07 11:12 ` Christian Borntraeger
1 sibling, 2 replies; 14+ messages in thread
From: Cornelia Huck @ 2020-07-07 9:46 UTC (permalink / raw)
To: Pierre Morel
Cc: linux-kernel, pasic, borntraeger, frankja, mst, jasowang, kvm,
linux-s390, virtualization, thomas.lendacky, david, linuxram,
heiko.carstens, gor
On Tue, 7 Jul 2020 10:44:37 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:
> S390, protecting the guest memory against unauthorized host access
> needs to enforce VIRTIO I/O device protection through the use of
> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
Hm... what about:
"If protected virtualization is active on s390, the virtio queues are
not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
negotiated. Use the new arch_validate_virtio_features() interface to
enforce this."
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index c296e5c8dbf9..106330f6eda1 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -14,6 +14,7 @@
> #include <linux/memblock.h>
> #include <linux/pagemap.h>
> #include <linux/swap.h>
> +#include <linux/virtio_config.h>
> #include <asm/facility.h>
> #include <asm/sections.h>
> #include <asm/uv.h>
> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
> }
> device_initcall(uv_info_init);
> #endif
> +
> +/*
> + * arch_validate_virtio_iommu_platform
s/arch_validate_virtio_iommu_platform/arch_validate_virtio_features/
> + * @dev: the VIRTIO device being added
> + *
> + * Return value: returns -ENODEV if any features of the
> + * device breaks the protected virtualization
> + * 0 otherwise.
I don't think you need to specify the contract here: that belongs to
the definition in the virtio core. What about simply adding a sentence
"Return an error if required features are missing on a guest running
with protected virtualization." ?
> + */
> +int arch_validate_virtio_features(struct virtio_device *dev)
> +{
Maybe jump out immediately if the guest is not protected?
> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
> + return is_prot_virt_guest() ? -ENODEV : 0;
> + }
> +
> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> + dev_warn(&dev->dev,
> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> + return is_prot_virt_guest() ? -ENODEV : 0;
> + }
if (!is_prot_virt_guest())
return 0;
if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
dev_warn(&dev->dev,
"legacy virtio is incompatible with protected guests");
return -ENODEV;
}
if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
dev_warn(&dev->dev,
"device does not work with limited memory access in protected guests");
return -ENODEV;
}
> +
> + return 0;
> +}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
2020-07-07 9:46 ` Cornelia Huck
@ 2020-07-07 10:38 ` Pierre Morel
2020-07-07 11:19 ` Halil Pasic
2020-07-07 11:14 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Pierre Morel @ 2020-07-07 10:38 UTC (permalink / raw)
To: Cornelia Huck
Cc: linux-kernel, pasic, borntraeger, frankja, mst, jasowang, kvm,
linux-s390, virtualization, thomas.lendacky, david, linuxram,
heiko.carstens, gor
On 2020-07-07 11:46, Cornelia Huck wrote:
> On Tue, 7 Jul 2020 10:44:37 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> S390, protecting the guest memory against unauthorized host access
>> needs to enforce VIRTIO I/O device protection through the use of
>> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>
> Hm... what about:
>
> "If protected virtualization is active on s390, the virtio queues are
> not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
> negotiated. Use the new arch_validate_virtio_features() interface to
> enforce this."
Yes, thanks.
>
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
>> index c296e5c8dbf9..106330f6eda1 100644
>> --- a/arch/s390/kernel/uv.c
>> +++ b/arch/s390/kernel/uv.c
>> @@ -14,6 +14,7 @@
>> #include <linux/memblock.h>
>> #include <linux/pagemap.h>
>> #include <linux/swap.h>
>> +#include <linux/virtio_config.h>
>> #include <asm/facility.h>
>> #include <asm/sections.h>
>> #include <asm/uv.h>
>> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
>> }
>> device_initcall(uv_info_init);
>> #endif
>> +
>> +/*
>> + * arch_validate_virtio_iommu_platform
>
> s/arch_validate_virtio_iommu_platform/arch_validate_virtio_features/
>
>> + * @dev: the VIRTIO device being added
>> + *
>> + * Return value: returns -ENODEV if any features of the
>> + * device breaks the protected virtualization
>> + * 0 otherwise.
>
> I don't think you need to specify the contract here: that belongs to
> the definition in the virtio core. What about simply adding a sentence
> "Return an error if required features are missing on a guest running
> with protected virtualization." ?
OK, right.
>
>> + */
>> +int arch_validate_virtio_features(struct virtio_device *dev)
>> +{
>
> Maybe jump out immediately if the guest is not protected?
>
>> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
>> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
>> + return is_prot_virt_guest() ? -ENODEV : 0;
>> + }
>> +
>> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>> + dev_warn(&dev->dev,
>> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>> + return is_prot_virt_guest() ? -ENODEV : 0;
>> + }
>
> if (!is_prot_virt_guest())
> return 0;
>
> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> dev_warn(&dev->dev,
> "legacy virtio is incompatible with protected guests");
> return -ENODEV;
> }
>
> if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> dev_warn(&dev->dev,
> "device does not work with limited memory access in protected guests");
> return -ENODEV;
> }
Yes, easier to read.
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] virtio: let arch validate VIRTIO features
2020-07-07 9:26 ` Cornelia Huck
@ 2020-07-07 10:39 ` Pierre Morel
2020-07-07 11:09 ` Christian Borntraeger
1 sibling, 0 replies; 14+ messages in thread
From: Pierre Morel @ 2020-07-07 10:39 UTC (permalink / raw)
To: Cornelia Huck
Cc: linux-kernel, pasic, borntraeger, frankja, mst, jasowang, kvm,
linux-s390, virtualization, thomas.lendacky, david, linuxram,
heiko.carstens, gor
On 2020-07-07 11:26, Cornelia Huck wrote:
> On Tue, 7 Jul 2020 10:44:36 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> An architecture may need to validate the VIRTIO devices features
>> based on architecture specificities.
>
> s/specifities/specifics/
OK
>
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> drivers/virtio/virtio.c | 19 +++++++++++++++++++
>> include/linux/virtio_config.h | 1 +
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index a977e32a88f2..3179a8aa76f5 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>> }
>> EXPORT_SYMBOL_GPL(virtio_add_status);
>>
>> +/*
>> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
>
> s/arch_needs_virtio_iommu_platform/arch_validate_virtio_features/
>
> :)
grrr... yes.
>
>> + * features for VIRTIO device dev
>> + * @dev: the VIRTIO device being added
>> + *
>> + * Permits the platform to provide architecture specific functionality when
>
> s/provide architecture specific functionality/handle architecture-specific requirements/
>
> ?
better, thanks.
>
>> + * devices features are finalized. This is the default implementation.
>
> s/devices/device/
yes.
>
>> + * Architecture implementations can override this.
>> + */
>> +
>> +int __weak arch_validate_virtio_features(struct virtio_device *dev)
>> +{
>> + return 0;
>> +}
>> +
>> int virtio_finalize_features(struct virtio_device *dev)
>> {
>> int ret = dev->config->finalize_features(dev);
>> @@ -176,6 +191,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>> if (ret)
>> return ret;
>>
>> + ret = arch_validate_virtio_features(dev);
>> + if (ret)
>> + return ret;
>> +
>> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>> return 0;
>>
>> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
>> index bb4cc4910750..3f4117adf311 100644
>> --- a/include/linux/virtio_config.h
>> +++ b/include/linux/virtio_config.h
>> @@ -459,4 +459,5 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
>> _r; \
>> })
>>
>> +int arch_validate_virtio_features(struct virtio_device *dev);
>> #endif /* _LINUX_VIRTIO_CONFIG_H */
>
> With the wording fixed,
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
Thanks for the review.
regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] virtio: let arch validate VIRTIO features
2020-07-07 9:26 ` Cornelia Huck
2020-07-07 10:39 ` Pierre Morel
@ 2020-07-07 11:09 ` Christian Borntraeger
2020-07-07 11:17 ` Pierre Morel
1 sibling, 1 reply; 14+ messages in thread
From: Christian Borntraeger @ 2020-07-07 11:09 UTC (permalink / raw)
To: Cornelia Huck, Pierre Morel
Cc: linux-kernel, pasic, frankja, mst, jasowang, kvm, linux-s390,
virtualization, thomas.lendacky, david, linuxram, heiko.carstens,
gor
On 07.07.20 11:26, Cornelia Huck wrote:
> On Tue, 7 Jul 2020 10:44:36 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> An architecture may need to validate the VIRTIO devices features
>> based on architecture specificities.
>
> s/specifities/specifics/
>
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> drivers/virtio/virtio.c | 19 +++++++++++++++++++
>> include/linux/virtio_config.h | 1 +
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index a977e32a88f2..3179a8aa76f5 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>> }
>> EXPORT_SYMBOL_GPL(virtio_add_status);
>>
>> +/*
>> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
>
> s/arch_needs_virtio_iommu_platform/arch_validate_virtio_features/
With the things from Conny fixed,
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
2020-07-07 8:44 ` [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
2020-07-07 9:46 ` Cornelia Huck
@ 2020-07-07 11:12 ` Christian Borntraeger
2020-07-07 11:16 ` Pierre Morel
1 sibling, 1 reply; 14+ messages in thread
From: Christian Borntraeger @ 2020-07-07 11:12 UTC (permalink / raw)
To: Pierre Morel, linux-kernel
Cc: pasic, frankja, mst, jasowang, cohuck, kvm, linux-s390,
virtualization, thomas.lendacky, david, linuxram, heiko.carstens,
gor
On 07.07.20 10:44, Pierre Morel wrote:
> S390, protecting the guest memory against unauthorized host access
> needs to enforce VIRTIO I/O device protection through the use of
> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index c296e5c8dbf9..106330f6eda1 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -14,6 +14,7 @@
> #include <linux/memblock.h>
> #include <linux/pagemap.h>
> #include <linux/swap.h>
> +#include <linux/virtio_config.h>
> #include <asm/facility.h>
> #include <asm/sections.h>
> #include <asm/uv.h>
> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
> }
> device_initcall(uv_info_init);
> #endif
> +
> +/*
> + * arch_validate_virtio_iommu_platform
> + * @dev: the VIRTIO device being added
> + *
> + * Return value: returns -ENODEV if any features of the
> + * device breaks the protected virtualization
> + * 0 otherwise.
> + */
> +int arch_validate_virtio_features(struct virtio_device *dev)
> +{
> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
I think you only want to warn if is_prot_virt_guest is true? We certainly
want to be able to run as a guest of older hypervisors with virtio 0.95, no?
> + return is_prot_virt_guest() ? -ENODEV : 0;
> + }
> +
> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> + dev_warn(&dev->dev,
> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
same here.
> + return is_prot_virt_guest() ? -ENODEV : 0;
> + }
> +
> + return 0;
> +}
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
2020-07-07 9:46 ` Cornelia Huck
2020-07-07 10:38 ` Pierre Morel
@ 2020-07-07 11:14 ` Michael S. Tsirkin
2020-07-07 11:19 ` Pierre Morel
1 sibling, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2020-07-07 11:14 UTC (permalink / raw)
To: Cornelia Huck
Cc: Pierre Morel, linux-kernel, pasic, borntraeger, frankja,
jasowang, kvm, linux-s390, virtualization, thomas.lendacky,
david, linuxram, heiko.carstens, gor
On Tue, Jul 07, 2020 at 11:46:33AM +0200, Cornelia Huck wrote:
> On Tue, 7 Jul 2020 10:44:37 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
> > S390, protecting the guest memory against unauthorized host access
> > needs to enforce VIRTIO I/O device protection through the use of
> > VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>
> Hm... what about:
>
> "If protected virtualization is active on s390, the virtio queues are
> not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
> negotiated. Use the new arch_validate_virtio_features() interface to
> enforce this."
s/enforce this/fail probe if that's not the case, preventing a host error on access attempt/
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> > arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> > index c296e5c8dbf9..106330f6eda1 100644
> > --- a/arch/s390/kernel/uv.c
> > +++ b/arch/s390/kernel/uv.c
> > @@ -14,6 +14,7 @@
> > #include <linux/memblock.h>
> > #include <linux/pagemap.h>
> > #include <linux/swap.h>
> > +#include <linux/virtio_config.h>
> > #include <asm/facility.h>
> > #include <asm/sections.h>
> > #include <asm/uv.h>
> > @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
> > }
> > device_initcall(uv_info_init);
> > #endif
> > +
> > +/*
> > + * arch_validate_virtio_iommu_platform
>
> s/arch_validate_virtio_iommu_platform/arch_validate_virtio_features/
>
> > + * @dev: the VIRTIO device being added
> > + *
> > + * Return value: returns -ENODEV if any features of the
> > + * device breaks the protected virtualization
> > + * 0 otherwise.
>
> I don't think you need to specify the contract here: that belongs to
> the definition in the virtio core. What about simply adding a sentence
> "Return an error if required features are missing on a guest running
> with protected virtualization." ?
>
> > + */
> > +int arch_validate_virtio_features(struct virtio_device *dev)
> > +{
>
> Maybe jump out immediately if the guest is not protected?
>
> > + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> > + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
> > + return is_prot_virt_guest() ? -ENODEV : 0;
> > + }
> > +
> > + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > + dev_warn(&dev->dev,
> > + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> > + return is_prot_virt_guest() ? -ENODEV : 0;
> > + }
>
> if (!is_prot_virt_guest())
> return 0;
>
> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> dev_warn(&dev->dev,
> "legacy virtio is incompatible with protected guests");
> return -ENODEV;
> }
>
> if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> dev_warn(&dev->dev,
> "device does not work with limited memory access in protected guests");
> return -ENODEV;
> }
>
> > +
> > + return 0;
> > +}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
2020-07-07 11:12 ` Christian Borntraeger
@ 2020-07-07 11:16 ` Pierre Morel
0 siblings, 0 replies; 14+ messages in thread
From: Pierre Morel @ 2020-07-07 11:16 UTC (permalink / raw)
To: Christian Borntraeger, linux-kernel
Cc: pasic, frankja, mst, jasowang, cohuck, kvm, linux-s390,
virtualization, thomas.lendacky, david, linuxram, heiko.carstens,
gor
On 2020-07-07 13:12, Christian Borntraeger wrote:
>
>
> On 07.07.20 10:44, Pierre Morel wrote:
>> S390, protecting the guest memory against unauthorized host access
>> needs to enforce VIRTIO I/O device protection through the use of
>> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
>> index c296e5c8dbf9..106330f6eda1 100644
>> --- a/arch/s390/kernel/uv.c
>> +++ b/arch/s390/kernel/uv.c
>> @@ -14,6 +14,7 @@
>> #include <linux/memblock.h>
>> #include <linux/pagemap.h>
>> #include <linux/swap.h>
>> +#include <linux/virtio_config.h>
>> #include <asm/facility.h>
>> #include <asm/sections.h>
>> #include <asm/uv.h>
>> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
>> }
>> device_initcall(uv_info_init);
>> #endif
>> +
>> +/*
>> + * arch_validate_virtio_iommu_platform
>> + * @dev: the VIRTIO device being added
>> + *
>> + * Return value: returns -ENODEV if any features of the
>> + * device breaks the protected virtualization
>> + * 0 otherwise.
>> + */
>> +int arch_validate_virtio_features(struct virtio_device *dev)
>> +{
>> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
>> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
>
> I think you only want to warn if is_prot_virt_guest is true? We certainly
> want to be able to run as a guest of older hypervisors with virtio 0.95, no?
clear, yes.
I will first check for PV as Connie sugested.
>
>
>> + return is_prot_virt_guest() ? -ENODEV : 0;
>> + }
>> +
>> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>> + dev_warn(&dev->dev,
>> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>
> same here.
Yes,
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] virtio: let arch validate VIRTIO features
2020-07-07 11:09 ` Christian Borntraeger
@ 2020-07-07 11:17 ` Pierre Morel
0 siblings, 0 replies; 14+ messages in thread
From: Pierre Morel @ 2020-07-07 11:17 UTC (permalink / raw)
To: Christian Borntraeger, Cornelia Huck
Cc: linux-kernel, pasic, frankja, mst, jasowang, kvm, linux-s390,
virtualization, thomas.lendacky, david, linuxram, heiko.carstens,
gor
On 2020-07-07 13:09, Christian Borntraeger wrote:
>
>
> On 07.07.20 11:26, Cornelia Huck wrote:
>> On Tue, 7 Jul 2020 10:44:36 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> An architecture may need to validate the VIRTIO devices features
>>> based on architecture specificities.
>>
>> s/specifities/specifics/
yes
>>
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>> drivers/virtio/virtio.c | 19 +++++++++++++++++++
>>> include/linux/virtio_config.h | 1 +
>>> 2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>> index a977e32a88f2..3179a8aa76f5 100644
>>> --- a/drivers/virtio/virtio.c
>>> +++ b/drivers/virtio/virtio.c
>>> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>>> }
>>> EXPORT_SYMBOL_GPL(virtio_add_status);
>>>
>>> +/*
>>> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
>>
>> s/arch_needs_virtio_iommu_platform/arch_validate_virtio_features/
>
> With the things from Conny fixed,
>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
2020-07-07 10:38 ` Pierre Morel
@ 2020-07-07 11:19 ` Halil Pasic
0 siblings, 0 replies; 14+ messages in thread
From: Halil Pasic @ 2020-07-07 11:19 UTC (permalink / raw)
To: Pierre Morel
Cc: Cornelia Huck, linux-kernel, borntraeger, frankja, mst, jasowang,
kvm, linux-s390, virtualization, thomas.lendacky, david,
linuxram, heiko.carstens, gor
On Tue, 7 Jul 2020 12:38:17 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>
> On 2020-07-07 11:46, Cornelia Huck wrote:
> > On Tue, 7 Jul 2020 10:44:37 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >
> >> S390, protecting the guest memory against unauthorized host access
> >> needs to enforce VIRTIO I/O device protection through the use of
> >> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
> >
> > Hm... what about:
> >
> > "If protected virtualization is active on s390, the virtio queues are
> > not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
> > negotiated. Use the new arch_validate_virtio_features() interface to
> > enforce this."
>
> Yes, thanks.
>
>
> >
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> ---
> >> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
Is this the right place to put this stuff? This file seems to be about
implementing the interface for interacting with the ultravisor. I would
rather expect something like arch/s390/kernel/virtio.c
Should we ever get arch hooks for balloon those could go in
arch/s390/kernel/virtio.c as well.
> >> 1 file changed, 25 insertions(+)
> >>
> >> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> >> index c296e5c8dbf9..106330f6eda1 100644
> >> --- a/arch/s390/kernel/uv.c
> >> +++ b/arch/s390/kernel/uv.c
> >> @@ -14,6 +14,7 @@
> >> #include <linux/memblock.h>
> >> #include <linux/pagemap.h>
> >> #include <linux/swap.h>
> >> +#include <linux/virtio_config.h>
> >> #include <asm/facility.h>
> >> #include <asm/sections.h>
> >> #include <asm/uv.h>
> >> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
> >> }
> >> device_initcall(uv_info_init);
> >> #endif
> >> +
> >> +/*
> >> + * arch_validate_virtio_iommu_platform
> >
> > s/arch_validate_virtio_iommu_platform/arch_validate_virtio_features/
> >
> >> + * @dev: the VIRTIO device being added
> >> + *
> >> + * Return value: returns -ENODEV if any features of the
> >> + * device breaks the protected virtualization
> >> + * 0 otherwise.
> >
> > I don't think you need to specify the contract here: that belongs to
> > the definition in the virtio core. What about simply adding a sentence
> > "Return an error if required features are missing on a guest running
> > with protected virtualization." ?
>
> OK, right.
>
> >
> >> + */
> >> +int arch_validate_virtio_features(struct virtio_device *dev)
> >> +{
> >
> > Maybe jump out immediately if the guest is not protected?
> >
> >> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> >> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
> >> + return is_prot_virt_guest() ? -ENODEV : 0;
> >> + }
> >> +
> >> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> >> + dev_warn(&dev->dev,
> >> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> >> + return is_prot_virt_guest() ? -ENODEV : 0;
> >> + }
> >
> > if (!is_prot_virt_guest())
> > return 0;
> >
> > if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> > dev_warn(&dev->dev,
> > "legacy virtio is incompatible with protected guests");
> > return -ENODEV;
> > }
> >
> > if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > dev_warn(&dev->dev,
> > "device does not work with limited memory access in protected guests");
> > return -ENODEV;
> > }
>
> Yes, easier to read.
>
Not only easier to read but does not produce warnings
if !is_prot_virt_guest(). I strongly prefer the variant proposed by
Connie.
Otherwise LGTM.
Regards,
Halil
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
2020-07-07 11:14 ` Michael S. Tsirkin
@ 2020-07-07 11:19 ` Pierre Morel
0 siblings, 0 replies; 14+ messages in thread
From: Pierre Morel @ 2020-07-07 11:19 UTC (permalink / raw)
To: Michael S. Tsirkin, Cornelia Huck
Cc: linux-kernel, pasic, borntraeger, frankja, jasowang, kvm,
linux-s390, virtualization, thomas.lendacky, david, linuxram,
heiko.carstens, gor
On 2020-07-07 13:14, Michael S. Tsirkin wrote:
> On Tue, Jul 07, 2020 at 11:46:33AM +0200, Cornelia Huck wrote:
>> On Tue, 7 Jul 2020 10:44:37 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> S390, protecting the guest memory against unauthorized host access
>>> needs to enforce VIRTIO I/O device protection through the use of
>>> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>>
>> Hm... what about:
>>
>> "If protected virtualization is active on s390, the virtio queues are
>> not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
>> negotiated. Use the new arch_validate_virtio_features() interface to
>> enforce this."
>
> s/enforce this/fail probe if that's not the case, preventing a host error on access attempt/
>
yes, more complete, thanks.
regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-07-07 11:19 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 8:44 [PATCH v4 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel
2020-07-07 8:44 ` [PATCH v4 1/2] " Pierre Morel
2020-07-07 9:26 ` Cornelia Huck
2020-07-07 10:39 ` Pierre Morel
2020-07-07 11:09 ` Christian Borntraeger
2020-07-07 11:17 ` Pierre Morel
2020-07-07 8:44 ` [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
2020-07-07 9:46 ` Cornelia Huck
2020-07-07 10:38 ` Pierre Morel
2020-07-07 11:19 ` Halil Pasic
2020-07-07 11:14 ` Michael S. Tsirkin
2020-07-07 11:19 ` Pierre Morel
2020-07-07 11:12 ` Christian Borntraeger
2020-07-07 11:16 ` Pierre Morel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).