KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features
@ 2020-07-15  8:31 Pierre Morel
  2020-07-15  8:31 ` [PATCH v7 1/2] " Pierre Morel
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Pierre Morel @ 2020-07-15  8:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: pasic, borntraeger, frankja, mst, jasowang, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor

Hi all,

The goal of the series is to give a chance to the architecture
to validate VIRTIO device features.

in this respin:

1) I kept removed the ack from Jason as I reworked the patch
   @Jason, the nature and goal of the patch did not really changed
           please can I get back your acked-by with these changes?

2) Rewording for warning messages

Regards,
Pierre

Pierre Morel (2):
  virtio: let arch validate VIRTIO features
  s390: virtio: PV needs VIRTIO I/O device protection

 arch/s390/mm/init.c           | 28 ++++++++++++++++++++++++++++
 drivers/virtio/virtio.c       | 19 +++++++++++++++++++
 include/linux/virtio_config.h |  1 +
 3 files changed, 48 insertions(+)

-- 
2.25.1

Changelog

to v7:

- typo in warning message
  (Connie)
to v6:

- rewording warning messages
  (Connie, Halil)

to v5:

- return directly from S390 arch_validate_virtio_features()
  when the guest is not protected.
  (Connie)

- Somme rewording
  (Connie, Michael)

- moved back code from arch/s390/ ...kernel/uv.c to ...mm/init.c
  (Christian)

to v4:

- separate virtio and arch code
  (Pierre)

- moved code from arch/s390/mm/init.c to arch/s390/kernel/uv.c
  (as interpreted from Heiko's comment)

- 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] 13+ messages in thread

* [PATCH v7 1/2] virtio: let arch validate VIRTIO features
  2020-07-15  8:31 [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel
@ 2020-07-15  8:31 ` Pierre Morel
  2020-07-15  8:31 ` [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2020-07-15  8:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: pasic, borntraeger, frankja, mst, jasowang, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor

An architecture may need to validate the VIRTIO devices features
based on architecture specifics.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Acked-by: Halil Pasic <pasic@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..c4e14d46a5b6 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_validate_virtio_features - provide arch specific hook when finalizing
+ *				   features for VIRTIO device dev
+ * @dev: the VIRTIO device being added
+ *
+ * Permits the platform to handle architecture-specific requirements when
+ * device 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	[flat|nested] 13+ messages in thread

* [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-15  8:31 [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel
  2020-07-15  8:31 ` [PATCH v7 1/2] " Pierre Morel
@ 2020-07-15  8:31 ` Pierre Morel
  2020-07-15  9:50   ` Michael S. Tsirkin
  2020-07-15  8:36 ` [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features Jason Wang
  2020-07-15  9:20 ` Cornelia Huck
  3 siblings, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2020-07-15  8:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: pasic, borntraeger, frankja, mst, jasowang, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor

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 <pmorel@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Halil Pasic <pasic@linux.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/mm/init.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 6dc7c3b60ef6..d39af6554d4f 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -45,6 +45,7 @@
 #include <asm/kasan.h>
 #include <asm/dma-mapping.h>
 #include <asm/uv.h>
+#include <linux/virtio_config.h>
 
 pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
 
@@ -161,6 +162,33 @@ 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,
+			 "legacy virtio not supported with protected virtualization\n");
+		return -ENODEV;
+	}
+
+	if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
+		dev_warn(&dev->dev,
+			 "support for limited memory access required for protected virtualization\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 /* protected virtualization */
 static void pv_init(void)
 {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features
  2020-07-15  8:31 [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel
  2020-07-15  8:31 ` [PATCH v7 1/2] " Pierre Morel
  2020-07-15  8:31 ` [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
@ 2020-07-15  8:36 ` Jason Wang
  2020-07-15  9:20 ` Cornelia Huck
  3 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2020-07-15  8:36 UTC (permalink / raw)
  To: Pierre Morel, linux-kernel
  Cc: pasic, borntraeger, frankja, mst, cohuck, kvm, linux-s390,
	virtualization, thomas.lendacky, david, linuxram, hca, gor


On 2020/7/15 下午4:31, Pierre Morel wrote:
> Hi all,
>
> The goal of the series is to give a chance to the architecture
> to validate VIRTIO device features.
>
> in this respin:
>
> 1) I kept removed the ack from Jason as I reworked the patch
>     @Jason, the nature and goal of the patch did not really changed
>             please can I get back your acked-by with these changes?


Yes.

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

Thanks


>
> 2) Rewording for warning messages
>
> Regards,
> Pierre
>
> Pierre Morel (2):
>    virtio: let arch validate VIRTIO features
>    s390: virtio: PV needs VIRTIO I/O device protection
>
>   arch/s390/mm/init.c           | 28 ++++++++++++++++++++++++++++
>   drivers/virtio/virtio.c       | 19 +++++++++++++++++++
>   include/linux/virtio_config.h |  1 +
>   3 files changed, 48 insertions(+)
>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features
  2020-07-15  8:31 [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel
                   ` (2 preceding siblings ...)
  2020-07-15  8:36 ` [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features Jason Wang
@ 2020-07-15  9:20 ` Cornelia Huck
  3 siblings, 0 replies; 13+ messages in thread
From: Cornelia Huck @ 2020-07-15  9:20 UTC (permalink / raw)
  To: Pierre Morel
  Cc: linux-kernel, pasic, borntraeger, frankja, mst, jasowang, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor

On Wed, 15 Jul 2020 10:31:07 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Hi all,
> 
> The goal of the series is to give a chance to the architecture
> to validate VIRTIO device features.
> 
> in this respin:
> 
> 1) I kept removed the ack from Jason as I reworked the patch
>    @Jason, the nature and goal of the patch did not really changed
>            please can I get back your acked-by with these changes?
> 
> 2) Rewording for warning messages
> 
> Regards,
> Pierre
> 
> Pierre Morel (2):
>   virtio: let arch validate VIRTIO features
>   s390: virtio: PV needs VIRTIO I/O device protection
> 
>  arch/s390/mm/init.c           | 28 ++++++++++++++++++++++++++++
>  drivers/virtio/virtio.c       | 19 +++++++++++++++++++
>  include/linux/virtio_config.h |  1 +
>  3 files changed, 48 insertions(+)
> 

LGTM.

I assume that this will go either via the virtio tree or via the s390
arch tree, i.e. not through virtio-ccw, right?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-15  8:31 ` [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
@ 2020-07-15  9:50   ` Michael S. Tsirkin
  2020-07-15 10:16     ` Jason Wang
  0 siblings, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2020-07-15  9:50 UTC (permalink / raw)
  To: Pierre Morel
  Cc: linux-kernel, pasic, borntraeger, frankja, jasowang, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor

On Wed, Jul 15, 2020 at 10:31:09AM +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 <pmorel@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Acked-by: Halil Pasic <pasic@linux.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/mm/init.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 6dc7c3b60ef6..d39af6554d4f 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -45,6 +45,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio_config.h>
>  
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>  
> @@ -161,6 +162,33 @@ 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,
> +			 "legacy virtio not supported with protected virtualization\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> +		dev_warn(&dev->dev,
> +			 "support for limited memory access required for protected virtualization\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {

What bothers me here is that arch code depends on virtio now.
It works even with a modular virtio when functions are inline,
but it seems fragile: e.g. it breaks virtio as an out of tree module,
since layout of struct virtio_device can change.

I'm not sure what to do with this yet, will try to think about it
over the weekend. Thanks!


> -- 
> 2.25.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-15  9:50   ` Michael S. Tsirkin
@ 2020-07-15 10:16     ` Jason Wang
  2020-07-15 11:51       ` Michael S. Tsirkin
  0 siblings, 1 reply; 13+ messages in thread
From: Jason Wang @ 2020-07-15 10:16 UTC (permalink / raw)
  To: Michael S. Tsirkin, Pierre Morel
  Cc: linux-kernel, pasic, borntraeger, frankja, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor


On 2020/7/15 下午5:50, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2020 at 10:31:09AM +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 <pmorel@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Acked-by: Halil Pasic <pasic@linux.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   arch/s390/mm/init.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index 6dc7c3b60ef6..d39af6554d4f 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -45,6 +45,7 @@
>>   #include <asm/kasan.h>
>>   #include <asm/dma-mapping.h>
>>   #include <asm/uv.h>
>> +#include <linux/virtio_config.h>
>>   
>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>   
>> @@ -161,6 +162,33 @@ 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,
>> +			 "legacy virtio not supported with protected virtualization\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>> +		dev_warn(&dev->dev,
>> +			 "support for limited memory access required for protected virtualization\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   /* protected virtualization */
>>   static void pv_init(void)
>>   {
> What bothers me here is that arch code depends on virtio now.
> It works even with a modular virtio when functions are inline,
> but it seems fragile: e.g. it breaks virtio as an out of tree module,
> since layout of struct virtio_device can change.


The code was only called from virtio.c so it should be fine.

And my understanding is that we don't need to care about the kABI issue 
during upstream development?

Thanks


>
> I'm not sure what to do with this yet, will try to think about it
> over the weekend. Thanks!
>
>
>> -- 
>> 2.25.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-15 10:16     ` Jason Wang
@ 2020-07-15 11:51       ` Michael S. Tsirkin
  2020-07-16 11:19         ` Christian Borntraeger
                           ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2020-07-15 11:51 UTC (permalink / raw)
  To: Jason Wang
  Cc: Pierre Morel, linux-kernel, pasic, borntraeger, frankja, cohuck,
	kvm, linux-s390, virtualization, thomas.lendacky, david,
	linuxram, hca, gor

On Wed, Jul 15, 2020 at 06:16:59PM +0800, Jason Wang wrote:
> 
> On 2020/7/15 下午5:50, Michael S. Tsirkin wrote:
> > On Wed, Jul 15, 2020 at 10:31:09AM +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 <pmorel@linux.ibm.com>
> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > > Acked-by: Halil Pasic <pasic@linux.ibm.com>
> > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > > ---
> > >   arch/s390/mm/init.c | 28 ++++++++++++++++++++++++++++
> > >   1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> > > index 6dc7c3b60ef6..d39af6554d4f 100644
> > > --- a/arch/s390/mm/init.c
> > > +++ b/arch/s390/mm/init.c
> > > @@ -45,6 +45,7 @@
> > >   #include <asm/kasan.h>
> > >   #include <asm/dma-mapping.h>
> > >   #include <asm/uv.h>
> > > +#include <linux/virtio_config.h>
> > >   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
> > > @@ -161,6 +162,33 @@ 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,
> > > +			 "legacy virtio not supported with protected virtualization\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > > +		dev_warn(&dev->dev,
> > > +			 "support for limited memory access required for protected virtualization\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   /* protected virtualization */
> > >   static void pv_init(void)
> > >   {
> > What bothers me here is that arch code depends on virtio now.
> > It works even with a modular virtio when functions are inline,
> > but it seems fragile: e.g. it breaks virtio as an out of tree module,
> > since layout of struct virtio_device can change.
> 
> 
> The code was only called from virtio.c so it should be fine.
> 
> And my understanding is that we don't need to care about the kABI issue
> during upstream development?
> 
> Thanks

No, but so far it has been convenient at least for me, for development,
to just be able to unload all of virtio and load a different version.


> 
> > 
> > I'm not sure what to do with this yet, will try to think about it
> > over the weekend. Thanks!
> > 
> > 
> > > -- 
> > > 2.25.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-15 11:51       ` Michael S. Tsirkin
@ 2020-07-16 11:19         ` Christian Borntraeger
  2020-07-16 21:46           ` Michael S. Tsirkin
  2020-07-22 11:48         ` Pierre Morel
  2020-07-30 11:31         ` Pierre Morel
  2 siblings, 1 reply; 13+ messages in thread
From: Christian Borntraeger @ 2020-07-16 11:19 UTC (permalink / raw)
  To: Michael S. Tsirkin, Jason Wang
  Cc: Pierre Morel, linux-kernel, pasic, frankja, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor



On 15.07.20 13:51, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2020 at 06:16:59PM +0800, Jason Wang wrote:
>>
>> On 2020/7/15 下午5:50, Michael S. Tsirkin wrote:
>>> On Wed, Jul 15, 2020 at 10:31:09AM +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 <pmorel@linux.ibm.com>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>> Acked-by: Halil Pasic <pasic@linux.ibm.com>
>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>   arch/s390/mm/init.c | 28 ++++++++++++++++++++++++++++
>>>>   1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>>>> index 6dc7c3b60ef6..d39af6554d4f 100644
>>>> --- a/arch/s390/mm/init.c
>>>> +++ b/arch/s390/mm/init.c
>>>> @@ -45,6 +45,7 @@
>>>>   #include <asm/kasan.h>
>>>>   #include <asm/dma-mapping.h>
>>>>   #include <asm/uv.h>
>>>> +#include <linux/virtio_config.h>
>>>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>>> @@ -161,6 +162,33 @@ 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,
>>>> +			 "legacy virtio not supported with protected virtualization\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>>> +		dev_warn(&dev->dev,
>>>> +			 "support for limited memory access required for protected virtualization\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>   /* protected virtualization */
>>>>   static void pv_init(void)
>>>>   {
>>> What bothers me here is that arch code depends on virtio now.
>>> It works even with a modular virtio when functions are inline,
>>> but it seems fragile: e.g. it breaks virtio as an out of tree module,
>>> since layout of struct virtio_device can change.
>>

If you prefer that, we can simply create an arch/s390/kernel/virtio.c ?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-16 11:19         ` Christian Borntraeger
@ 2020-07-16 21:46           ` Michael S. Tsirkin
  0 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2020-07-16 21:46 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Jason Wang, Pierre Morel, linux-kernel, pasic, frankja, cohuck,
	kvm, linux-s390, virtualization, thomas.lendacky, david,
	linuxram, hca, gor

On Thu, Jul 16, 2020 at 01:19:55PM +0200, Christian Borntraeger wrote:
> 
> 
> On 15.07.20 13:51, Michael S. Tsirkin wrote:
> > On Wed, Jul 15, 2020 at 06:16:59PM +0800, Jason Wang wrote:
> >>
> >> On 2020/7/15 下午5:50, Michael S. Tsirkin wrote:
> >>> On Wed, Jul 15, 2020 at 10:31:09AM +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 <pmorel@linux.ibm.com>
> >>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >>>> Acked-by: Halil Pasic <pasic@linux.ibm.com>
> >>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>> ---
> >>>>   arch/s390/mm/init.c | 28 ++++++++++++++++++++++++++++
> >>>>   1 file changed, 28 insertions(+)
> >>>>
> >>>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> >>>> index 6dc7c3b60ef6..d39af6554d4f 100644
> >>>> --- a/arch/s390/mm/init.c
> >>>> +++ b/arch/s390/mm/init.c
> >>>> @@ -45,6 +45,7 @@
> >>>>   #include <asm/kasan.h>
> >>>>   #include <asm/dma-mapping.h>
> >>>>   #include <asm/uv.h>
> >>>> +#include <linux/virtio_config.h>
> >>>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
> >>>> @@ -161,6 +162,33 @@ 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,
> >>>> +			 "legacy virtio not supported with protected virtualization\n");
> >>>> +		return -ENODEV;
> >>>> +	}
> >>>> +
> >>>> +	if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> >>>> +		dev_warn(&dev->dev,
> >>>> +			 "support for limited memory access required for protected virtualization\n");
> >>>> +		return -ENODEV;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>   /* protected virtualization */
> >>>>   static void pv_init(void)
> >>>>   {
> >>> What bothers me here is that arch code depends on virtio now.
> >>> It works even with a modular virtio when functions are inline,
> >>> but it seems fragile: e.g. it breaks virtio as an out of tree module,
> >>> since layout of struct virtio_device can change.
> >>
> 
> If you prefer that, we can simply create an arch/s390/kernel/virtio.c ?

How would that address the issues above?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-15 11:51       ` Michael S. Tsirkin
  2020-07-16 11:19         ` Christian Borntraeger
@ 2020-07-22 11:48         ` Pierre Morel
  2020-07-30 11:31         ` Pierre Morel
  2 siblings, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2020-07-22 11:48 UTC (permalink / raw)
  To: Michael S. Tsirkin, Jason Wang
  Cc: linux-kernel, pasic, borntraeger, frankja, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor



On 2020-07-15 13:51, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2020 at 06:16:59PM +0800, Jason Wang wrote:
>>
>> On 2020/7/15 下午5:50, Michael S. Tsirkin wrote:
>>> On Wed, Jul 15, 2020 at 10:31:09AM +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 <pmorel@linux.ibm.com>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>> Acked-by: Halil Pasic <pasic@linux.ibm.com>
>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>    arch/s390/mm/init.c | 28 ++++++++++++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>>>> index 6dc7c3b60ef6..d39af6554d4f 100644
>>>> --- a/arch/s390/mm/init.c
>>>> +++ b/arch/s390/mm/init.c
>>>> @@ -45,6 +45,7 @@
>>>>    #include <asm/kasan.h>
>>>>    #include <asm/dma-mapping.h>
>>>>    #include <asm/uv.h>
>>>> +#include <linux/virtio_config.h>
>>>>    pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>>> @@ -161,6 +162,33 @@ 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,
>>>> +			 "legacy virtio not supported with protected virtualization\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>>> +		dev_warn(&dev->dev,
>>>> +			 "support for limited memory access required for protected virtualization\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    /* protected virtualization */
>>>>    static void pv_init(void)
>>>>    {
>>> What bothers me here is that arch code depends on virtio now.
>>> It works even with a modular virtio when functions are inline,
>>> but it seems fragile: e.g. it breaks virtio as an out of tree module,
>>> since layout of struct virtio_device can change.
>>
>>
>> The code was only called from virtio.c so it should be fine.
>>
>> And my understanding is that we don't need to care about the kABI issue
>> during upstream development?
>>
>> Thanks
> 
> No, but so far it has been convenient at least for me, for development,
> to just be able to unload all of virtio and load a different version.
> 
> 
>>
>>>
>>> I'm not sure what to do with this yet, will try to think about it
>>> over the weekend. Thanks!
>>>
>>>
>>>> -- 
>>>> 2.25.1
> 

Hi Michael,

I am not sure to understand the problem so I may propose a wrong 
solution but, let's try:

Would a callback registration instead of a weak function solve the problem?
The registrating function in core could test a parameter to check if the 
callback is in sync with the VIRTIO core.

What do you think?

Regards,
Pierre



-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-15 11:51       ` Michael S. Tsirkin
  2020-07-16 11:19         ` Christian Borntraeger
  2020-07-22 11:48         ` Pierre Morel
@ 2020-07-30 11:31         ` Pierre Morel
  2020-08-06 14:19           ` Pierre Morel
  2 siblings, 1 reply; 13+ messages in thread
From: Pierre Morel @ 2020-07-30 11:31 UTC (permalink / raw)
  To: Michael S. Tsirkin, Jason Wang
  Cc: linux-kernel, pasic, borntraeger, frankja, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor


gentle ping.


On 2020-07-15 13:51, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2020 at 06:16:59PM +0800, Jason Wang wrote:
>>
>> On 2020/7/15 下午5:50, Michael S. Tsirkin wrote:
>>> On Wed, Jul 15, 2020 at 10:31:09AM +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 <pmorel@linux.ibm.com>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>> Acked-by: Halil Pasic <pasic@linux.ibm.com>
>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>    arch/s390/mm/init.c | 28 ++++++++++++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>>>> index 6dc7c3b60ef6..d39af6554d4f 100644
>>>> --- a/arch/s390/mm/init.c
>>>> +++ b/arch/s390/mm/init.c
>>>> @@ -45,6 +45,7 @@
>>>>    #include <asm/kasan.h>
>>>>    #include <asm/dma-mapping.h>
>>>>    #include <asm/uv.h>
>>>> +#include <linux/virtio_config.h>
>>>>    pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>>> @@ -161,6 +162,33 @@ 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,
>>>> +			 "legacy virtio not supported with protected virtualization\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>>> +		dev_warn(&dev->dev,
>>>> +			 "support for limited memory access required for protected virtualization\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    /* protected virtualization */
>>>>    static void pv_init(void)
>>>>    {
>>> What bothers me here is that arch code depends on virtio now.
>>> It works even with a modular virtio when functions are inline,
>>> but it seems fragile: e.g. it breaks virtio as an out of tree module,
>>> since layout of struct virtio_device can change.
>>
>>
>> The code was only called from virtio.c so it should be fine.
>>
>> And my understanding is that we don't need to care about the kABI issue
>> during upstream development?
>>
>> Thanks
> 
> No, but so far it has been convenient at least for me, for development,
> to just be able to unload all of virtio and load a different version.
> 
> 
>>
>>>
>>> I'm not sure what to do with this yet, will try to think about it
>>> over the weekend. Thanks!
>>>
>>>
>>>> -- 
>>>> 2.25.1
> 

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection
  2020-07-30 11:31         ` Pierre Morel
@ 2020-08-06 14:19           ` Pierre Morel
  0 siblings, 0 replies; 13+ messages in thread
From: Pierre Morel @ 2020-08-06 14:19 UTC (permalink / raw)
  To: Michael S. Tsirkin, Jason Wang
  Cc: linux-kernel, pasic, borntraeger, frankja, cohuck, kvm,
	linux-s390, virtualization, thomas.lendacky, david, linuxram,
	hca, gor



On 2020-07-30 13:31, Pierre Morel wrote:
...snip...
>>>> What bothers me here is that arch code depends on virtio now.
>>>> It works even with a modular virtio when functions are inline,
>>>> but it seems fragile: e.g. it breaks virtio as an out of tree module,
>>>> since layout of struct virtio_device can change.
>>>
>>>
>>> The code was only called from virtio.c so it should be fine.
>>>
>>> And my understanding is that we don't need to care about the kABI issue
>>> during upstream development?
>>>
>>> Thanks
>>
>> No, but so far it has been convenient at least for me, for development,
>> to just be able to unload all of virtio and load a different version.
>>
>>
>>>
>>>>
>>>> I'm not sure what to do with this yet, will try to think about it
>>>> over the weekend. Thanks!

After reflection, I am not sure that this problem must be treated on the 
architecture level or inside the VIRTIO transport.
Consequently, I will propose another patch series based on CCW transport.
This also should be more convenient for core development.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15  8:31 [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel
2020-07-15  8:31 ` [PATCH v7 1/2] " Pierre Morel
2020-07-15  8:31 ` [PATCH v7 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel
2020-07-15  9:50   ` Michael S. Tsirkin
2020-07-15 10:16     ` Jason Wang
2020-07-15 11:51       ` Michael S. Tsirkin
2020-07-16 11:19         ` Christian Borntraeger
2020-07-16 21:46           ` Michael S. Tsirkin
2020-07-22 11:48         ` Pierre Morel
2020-07-30 11:31         ` Pierre Morel
2020-08-06 14:19           ` Pierre Morel
2020-07-15  8:36 ` [PATCH v7 0/2] s390: virtio: let arch validate VIRTIO features Jason Wang
2020-07-15  9:20 ` Cornelia Huck

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git