[PATCH v5 2/2] s390: virtio: PV needs VIRTIO I/O device protection
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
fail probe if that's not the case, preventing a host error on access
attempt
Signed-off-by: Pierre Morel
---
arch/s390/mm/init.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 6dc7c3b60ef6..b8e6f90117da 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -45,6 +45,7 @@
#include
#include
#include
+#include
pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
@@ -161,6 +162,32 @@ bool force_dma_unencrypted(struct device *dev)
return is_prot_virt_guest();
}
+/*
+ * arch_validate_virtio_features
+ * @dev: the VIRTIO device being added
+ *
+ * Return an error if required features are missing on a guest running
+ * with protected virtualization.
+ */
+int arch_validate_virtio_features(struct virtio_device *dev)
+{
+ if (!is_prot_virt_guest())
+ return 0;
+
+ if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+ dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
+ return -ENODEV;
+ }
+
+ if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
+ dev_warn(&dev->dev,
+"device must provide VIRTIO_F_IOMMU_PLATFORM\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
/* protected virtualization */
static void pv_init(void)
{
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On Thu, 9 Jul 2020 10:39:19 +0200
Pierre Morel wrote:
> 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
> fail probe if that's not the case, preventing a host error on access
> attempt
>
> Signed-off-by: Pierre Morel
> ---
> arch/s390/mm/init.c | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 6dc7c3b60ef6..b8e6f90117da 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -45,6 +45,7 @@
> #include
> #include
> #include
> +#include
>
> pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>
> @@ -161,6 +162,32 @@ bool force_dma_unencrypted(struct device *dev)
> return is_prot_virt_guest();
> }
>
> +/*
> + * arch_validate_virtio_features
> + * @dev: the VIRTIO device being added
> + *
> + * Return an error if required features are missing on a guest running
> + * with protected virtualization.
> + */
> +int arch_validate_virtio_features(struct virtio_device *dev)
> +{
> + if (!is_prot_virt_guest())
> + return 0;
> +
> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
I'd probably use "legacy virtio not supported with protected
virtualization".
> + return -ENODEV;
> + }
> +
> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> + dev_warn(&dev->dev,
> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
"support for limited memory access required for protected
virtualization"
?
Mentioning the feature flag is shorter in both cases, though.
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> /* protected virtualization */
> static void pv_init(void)
> {
Either way,
Reviewed-by: Cornelia Huck
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On Thu, 9 Jul 2020 10:57:33 +0200
Cornelia Huck wrote:
> On Thu, 9 Jul 2020 10:39:19 +0200
> Pierre Morel wrote:
>
> > 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
> > fail probe if that's not the case, preventing a host error on access
> > attempt
Punctuation at the end?
Also 'that's not the case' refers to the negation
'VIRTIO_F_IOMMU_PLATFORM has been negotiated',
arch_validate_virtio_features() is however part of
virtio_finalize_features(), which is in turn part of the feature
negotiation. But that is details. I'm fine with keeping the message as
is.
> >
> > Signed-off-by: Pierre Morel
> > ---
> > arch/s390/mm/init.c | 27 +++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> > index 6dc7c3b60ef6..b8e6f90117da 100644
> > --- a/arch/s390/mm/init.c
> > +++ b/arch/s390/mm/init.c
> > @@ -45,6 +45,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
> >
> > @@ -161,6 +162,32 @@ bool force_dma_unencrypted(struct device *dev)
> > return is_prot_virt_guest();
> > }
> >
> > +/*
> > + * arch_validate_virtio_features
> > + * @dev: the VIRTIO device being added
> > + *
> > + * Return an error if required features are missing on a guest running
> > + * with protected virtualization.
> > + */
> > +int arch_validate_virtio_features(struct virtio_device *dev)
> > +{
> > + if (!is_prot_virt_guest())
> > + return 0;
> > +
> > + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> > + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
>
> I'd probably use "legacy virtio not supported with protected
> virtualization".
>
> > + return -ENODEV;
> > + }
> > +
> > + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > + dev_warn(&dev->dev,
> > +"device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>
> "support for limited memory access required for protected
> virtualization"
>
> ?
>
> Mentioning the feature flag is shorter in both cases, though.
I liked the messages in v4. Why did we change those? Did somebody
complain?
I prefer the old ones, but it any case:
Acked-by: Halil Pasic
>
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /* protected virtualization */
> > static void pv_init(void)
> > {
>
> Either way,
>
> Reviewed-by: Cornelia Huck
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On 2020-07-09 10:57, Cornelia Huck wrote:
On Thu, 9 Jul 2020 10:39:19 +0200
Pierre Morel wrote:
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
fail probe if that's not the case, preventing a host error on access
attempt
Signed-off-by: Pierre Morel
---
arch/s390/mm/init.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 6dc7c3b60ef6..b8e6f90117da 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -45,6 +45,7 @@
#include
#include
#include
+#include
pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
@@ -161,6 +162,32 @@ bool force_dma_unencrypted(struct device *dev)
return is_prot_virt_guest();
}
+/*
+ * arch_validate_virtio_features
+ * @dev: the VIRTIO device being added
+ *
+ * Return an error if required features are missing on a guest running
+ * with protected virtualization.
+ */
+int arch_validate_virtio_features(struct virtio_device *dev)
+{
+ if (!is_prot_virt_guest())
+ return 0;
+
+ if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+ dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
I'd probably use "legacy virtio not supported with protected
virtualization".
+ return -ENODEV;
+ }
+
+ if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
+ dev_warn(&dev->dev,
+"device must provide VIRTIO_F_IOMMU_PLATFORM\n");
"support for limited memory access required for protected
virtualization"
?
Mentioning the feature flag is shorter in both cases, though.
And I think easier to look for in case of debugging purpose.
I change it if there is more demands.
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
/* protected virtualization */
static void pv_init(void)
{
Either way,
Reviewed-by: Cornelia Huck
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On 2020-07-09 11:55, Halil Pasic wrote:
On Thu, 9 Jul 2020 10:57:33 +0200
Cornelia Huck wrote:
On Thu, 9 Jul 2020 10:39:19 +0200
Pierre Morel wrote:
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
fail probe if that's not the case, preventing a host error on access
attempt
Punctuation at the end?
Also 'that's not the case' refers to the negation
'VIRTIO_F_IOMMU_PLATFORM has been negotiated',
arch_validate_virtio_features() is however part of
virtio_finalize_features(), which is in turn part of the feature
negotiation. But that is details. I'm fine with keeping the message as
is.
Signed-off-by: Pierre Morel
---
arch/s390/mm/init.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 6dc7c3b60ef6..b8e6f90117da 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -45,6 +45,7 @@
#include
#include
#include
+#include
pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
@@ -161,6 +162,32 @@ bool force_dma_unencrypted(struct device *dev)
return is_prot_virt_guest();
}
+/*
+ * arch_validate_virtio_features
+ * @dev: the VIRTIO device being added
+ *
+ * Return an error if required features are missing on a guest running
+ * with protected virtualization.
+ */
+int arch_validate_virtio_features(struct virtio_device *dev)
+{
+ if (!is_prot_virt_guest())
+ return 0;
+
+ if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+ dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
I'd probably use "legacy virtio not supported with protected
virtualization".
+ return -ENODEV;
+ }
+
+ if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
+ dev_warn(&dev->dev,
+"device must provide VIRTIO_F_IOMMU_PLATFORM\n");
"support for limited memory access required for protected
virtualization"
?
Mentioning the feature flag is shorter in both cases, though.
I liked the messages in v4. Why did we change those? Did somebody
complain?
I prefer the old ones, but it any case:
Acked-by: Halil Pasic
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On Thu, 9 Jul 2020 12:51:58 +0200
Pierre Morel wrote:
> >> +int arch_validate_virtio_features(struct virtio_device *dev)
> >> +{
> >> + if (!is_prot_virt_guest())
> >> + return 0;
> >> +
> >> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> >> + dev_warn(&dev->dev, "device must provide
> >> VIRTIO_F_VERSION_1\n");
> >
> > I'd probably use "legacy virtio not supported with protected
> > virtualization".
> >
> >> + return -ENODEV;
> >> + }
> >> +
> >> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> >> + dev_warn(&dev->dev,
> >> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> >
> > "support for limited memory access required for protected
> > virtualization"
> >
> > ?
> >
> > Mentioning the feature flag is shorter in both cases, though.
>
> And I think easier to look for in case of debugging purpose.
> I change it if there is more demands.
Not all our end users are kernel and/or qemu developers. I find the
messages from v4 less technical, more informative, and way better.
Regards,
Halil
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On 2020-07-09 16:47, Halil Pasic wrote:
On Thu, 9 Jul 2020 12:51:58 +0200
Pierre Morel wrote:
+int arch_validate_virtio_features(struct virtio_device *dev)
+{
+ if (!is_prot_virt_guest())
+ return 0;
+
+ if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+ dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
I'd probably use "legacy virtio not supported with protected
virtualization".
+ return -ENODEV;
+ }
+
+ if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
+ dev_warn(&dev->dev,
+"device must provide VIRTIO_F_IOMMU_PLATFORM\n");
"support for limited memory access required for protected
virtualization"
?
Mentioning the feature flag is shorter in both cases, though.
And I think easier to look for in case of debugging purpose.
I change it if there is more demands.
Not all our end users are kernel and/or qemu developers. I find the
messages from v4 less technical, more informative, and way better.
Regards,
Halil
Can you please tell me the messages you are speaking of, because for me
the warning's messages are exactly the same in v4 and v5!?
I checked many times, but may be I still missed something.
Regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On Thu, 9 Jul 2020 16:51:04 +0200
Pierre Morel wrote:
>
>
> On 2020-07-09 16:47, Halil Pasic wrote:
> > On Thu, 9 Jul 2020 12:51:58 +0200
> > Pierre Morel wrote:
> >
> +int arch_validate_virtio_features(struct virtio_device *dev)
> +{
> +if (!is_prot_virt_guest())
> +return 0;
> +
> +if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> +dev_warn(&dev->dev, "device must provide
> VIRTIO_F_VERSION_1\n");
> >>>
> >>> I'd probably use "legacy virtio not supported with protected
> >>> virtualization".
> >>>
> +return -ENODEV;
> +}
> +
> +if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> +dev_warn(&dev->dev,
> + "device must provide
> VIRTIO_F_IOMMU_PLATFORM\n");
> >>>
> >>> "support for limited memory access required for protected
> >>> virtualization"
> >>>
> >>> ?
> >>>
> >>> Mentioning the feature flag is shorter in both cases, though.
> >>
> >> And I think easier to look for in case of debugging purpose.
> >> I change it if there is more demands.
> >
> > Not all our end users are kernel and/or qemu developers. I find the
> > messages from v4 less technical, more informative, and way better.
> >
> > Regards,
> > Halil
> >
>
> Can you please tell me the messages you are speaking of, because for me
> the warning's messages are exactly the same in v4 and v5!?
>
> I checked many times, but may be I still missed something.
>
Sorry, my bad. My brain is over-generating. The messages where discussed
at v3 and Connie made a very similar proposal to the one above which I
seconded (for reference look at Message-ID:
<833c71f2-0057-896a-5e21-2c6263834...@linux.ibm.com>). I was under the
impression that it got implemented in v4, but it was not. That's why I
ended up talking bs.
Regards,
Halil
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
