[PATCH v8 0/2] s390: virtio: let arch validate VIRTIO features

[PATCH v8 0/2] s390: virtio: let arch validate VIRTIO features

[Pierre Morel]

Hi all,

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

in this respin:

I use the original idea from Connie for an optional
arch_has_restricted_memory_access.

I renamed the callback accordingly, added the definition of
ARCH_HAS_RESTRICTED_MEMORY_ACCESS inside the VIRTIO Kconfig
and the selection in the PROTECTED_VIRTUALIZATION_GUEST
config entry.


Regards,
Pierre

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

 arch/s390/Kconfig             |  1 +
 arch/s390/mm/init.c           | 30 ++++++++++++++++++++++++++++++
 drivers/virtio/Kconfig        |  6 ++++++
 drivers/virtio/virtio.c       |  4 ++++
 include/linux/virtio_config.h |  9 +++++++++
 5 files changed, 50 insertions(+)

-- 
2.25.1

Changelog

to v8:

- refactoring by using an optional callback
  (Connie)

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)

[PATCH v8 1/2] virtio: let arch validate VIRTIO features

[Pierre Morel]

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

Provide a new Kconfig entry, CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS,
the architecture can select when it provides a callback named
arch_has_restricted_memory_access to validate the virtio device
features.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/virtio/Kconfig        | 6 ++++++
 drivers/virtio/virtio.c       | 4 ++++
 include/linux/virtio_config.h | 9 +++++++++
 3 files changed, 19 insertions(+)

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 5809e5f5b157..eef09e3c92f9 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -6,6 +6,12 @@ config VIRTIO
 	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG
 	  or CONFIG_S390_GUEST.
 
+config ARCH_HAS_RESTRICTED_MEMORY_ACCESS
+	bool
+	help
+	  This option is selected by any architecture enforcing
+	  VIRTIO_F_IOMMU_PLATFORM
+
 menuconfig VIRTIO_MENU
 	bool "Virtio drivers"
 	default y
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32a88f2..1471db7d6510 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -176,6 +176,10 @@ int virtio_finalize_features(struct virtio_device *dev)
 	if (ret)
 		return ret;
 
+	ret = arch_has_restricted_memory_access(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..f6b82541c497 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -459,4 +459,13 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
 		_r;							\
 	})
 
+#ifdef CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS
+int arch_has_restricted_memory_access(struct virtio_device *dev);
+#else
+static inline int arch_has_restricted_memory_access(struct virtio_device *dev)
+{
+	return 0;
+}
+#endif /* CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS */
+
 #endif /* _LINUX_VIRTIO_CONFIG_H */
-- 
2.25.1

Re: [PATCH v8 1/2] virtio: let arch validate VIRTIO features

[Cornelia Huck]

On Tue, 18 Aug 2020 16:58:30 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> An architecture may need to validate the VIRTIO devices features
> based on architecture specifics.
> 
> Provide a new Kconfig entry, CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS,
> the architecture can select when it provides a callback named
> arch_has_restricted_memory_access to validate the virtio device
> features.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/virtio/Kconfig        | 6 ++++++
>  drivers/virtio/virtio.c       | 4 ++++
>  include/linux/virtio_config.h | 9 +++++++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 5809e5f5b157..eef09e3c92f9 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -6,6 +6,12 @@ config VIRTIO
>  	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG
>  	  or CONFIG_S390_GUEST.
>  
> +config ARCH_HAS_RESTRICTED_MEMORY_ACCESS
> +	bool
> +	help
> +	  This option is selected by any architecture enforcing
> +	  VIRTIO_F_IOMMU_PLATFORM

This option is only for a very specific case of "restricted memory
access", namely the kind that requires IOMMU_PLATFORM for virtio
devices. ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS? Or is this intended
to cover cases outside of virtio as well?

> +
>  menuconfig VIRTIO_MENU
>  	bool "Virtio drivers"
>  	default y
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..1471db7d6510 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -176,6 +176,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (ret)
>  		return ret;
>  
> +	ret = arch_has_restricted_memory_access(dev);
> +	if (ret)
> +		return ret;

Hm, I'd rather have expected something like

if (arch_has_restricted_memory_access(dev)) {
	// enforce VERSION_1 and IOMMU_PLATFORM
}

Otherwise, you're duplicating the checks in the individual architecture
callbacks again.

[Not sure whether the device argument would be needed here; are there
architectures where we'd only require IOMMU_PLATFORM for a subset of
virtio devices?]

> +
>  	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..f6b82541c497 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -459,4 +459,13 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
>  		_r;							\
>  	})
>  
> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS
> +int arch_has_restricted_memory_access(struct virtio_device *dev);
> +#else
> +static inline int arch_has_restricted_memory_access(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS */
> +
>  #endif /* _LINUX_VIRTIO_CONFIG_H */

Re: [PATCH v8 1/2] virtio: let arch validate VIRTIO features

[Pierre Morel]

On 2020-08-18 19:19, Cornelia Huck wrote:
> On Tue, 18 Aug 2020 16:58:30 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
...
>> +config ARCH_HAS_RESTRICTED_MEMORY_ACCESS
>> +	bool
>> +	help
>> +	  This option is selected by any architecture enforcing
>> +	  VIRTIO_F_IOMMU_PLATFORM
> 
> This option is only for a very specific case of "restricted memory
> access", namely the kind that requires IOMMU_PLATFORM for virtio
> devices. ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS? Or is this intended
> to cover cases outside of virtio as well?

AFAIK we did not identify other restrictions so adding VIRTIO in the 
name should be the best thing to do.

If new restrictions appear they also may be orthogonal.

I will change to ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS if no one 
complains.

> 
>> +
>>   menuconfig VIRTIO_MENU
>>   	bool "Virtio drivers"
>>   	default y
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index a977e32a88f2..1471db7d6510 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -176,6 +176,10 @@ int virtio_finalize_features(struct virtio_device *dev)
>>   	if (ret)
>>   		return ret;
>>   
>> +	ret = arch_has_restricted_memory_access(dev);
>> +	if (ret)
>> +		return ret;
> 
> Hm, I'd rather have expected something like
> 
> if (arch_has_restricted_memory_access(dev)) {

may be also change the callback name to
arch_has_restricted_virtio_memory_access() ?

> 	// enforce VERSION_1 and IOMMU_PLATFORM
> }
> 
> Otherwise, you're duplicating the checks in the individual architecture
> callbacks again.

Yes, I agree and go back this way.

> 
> [Not sure whether the device argument would be needed here; are there
> architectures where we'd only require IOMMU_PLATFORM for a subset of
> virtio devices?]

I don't think so and since we do the checks locally, we do not need the 
device argument anymore.


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v8 1/2] virtio: let arch validate VIRTIO features

[Cornelia Huck]

On Wed, 19 Aug 2020 10:50:18 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 2020-08-18 19:19, Cornelia Huck wrote:
> > On Tue, 18 Aug 2020 16:58:30 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> ...
> >> +config ARCH_HAS_RESTRICTED_MEMORY_ACCESS
> >> +	bool
> >> +	help
> >> +	  This option is selected by any architecture enforcing
> >> +	  VIRTIO_F_IOMMU_PLATFORM  
> > 
> > This option is only for a very specific case of "restricted memory
> > access", namely the kind that requires IOMMU_PLATFORM for virtio
> > devices. ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS? Or is this intended
> > to cover cases outside of virtio as well?  
> 
> AFAIK we did not identify other restrictions so adding VIRTIO in the 
> name should be the best thing to do.
> 
> If new restrictions appear they also may be orthogonal.
> 
> I will change to ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS if no one 
> complains.
> 
> >   
> >> +
> >>   menuconfig VIRTIO_MENU
> >>   	bool "Virtio drivers"
> >>   	default y
> >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >> index a977e32a88f2..1471db7d6510 100644
> >> --- a/drivers/virtio/virtio.c
> >> +++ b/drivers/virtio/virtio.c
> >> @@ -176,6 +176,10 @@ int virtio_finalize_features(struct virtio_device *dev)
> >>   	if (ret)
> >>   		return ret;
> >>   
> >> +	ret = arch_has_restricted_memory_access(dev);
> >> +	if (ret)
> >> +		return ret;  
> > 
> > Hm, I'd rather have expected something like
> > 
> > if (arch_has_restricted_memory_access(dev)) {  
> 
> may be also change the callback name to
> arch_has_restricted_virtio_memory_access() ?

Yes, why not.

> 
> > 	// enforce VERSION_1 and IOMMU_PLATFORM
> > }
> > 
> > Otherwise, you're duplicating the checks in the individual architecture
> > callbacks again.  
> 
> Yes, I agree and go back this way.
> 
> > 
> > [Not sure whether the device argument would be needed here; are there
> > architectures where we'd only require IOMMU_PLATFORM for a subset of
> > virtio devices?]  
> 
> I don't think so and since we do the checks locally, we do not need the 
> device argument anymore.

Yes, that would also remove some layering entanglement.

[PATCH v8 2/2] s390: virtio: PV needs VIRTIO I/O device protection

[Pierre Morel]

If protected virtualization is active on s390, the virtio queues are
not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
negotiated.
Define CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS and export
arch_has_restricted_memory_access 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>
---
 arch/s390/Kconfig   |  1 +
 arch/s390/mm/init.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 9cfd8de907cb..d4a3ef4fa27b 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -820,6 +820,7 @@ menu "Virtualization"
 config PROTECTED_VIRTUALIZATION_GUEST
 	def_bool n
 	prompt "Protected virtualization guest support"
+	select ARCH_HAS_RESTRICTED_MEMORY_ACCESS
 	help
 	  Select this option, if you want to be able to run this
 	  kernel as a protected virtualization KVM guest.
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 6dc7c3b60ef6..aec04d7dd089 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,35 @@ bool force_dma_unencrypted(struct device *dev)
 	return is_prot_virt_guest();
 }
 
+#ifdef CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS
+/*
+ * arch_has_restricted_memory_access
+ * @dev: the VIRTIO device being added
+ *
+ * Return an error if required features are missing on a guest running
+ * with protected virtualization.
+ */
+int arch_has_restricted_memory_access(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;
+}
+EXPORT_SYMBOL(arch_has_restricted_memory_access);
+#endif
+
 /* protected virtualization */
 static void pv_init(void)
 {
-- 
2.25.1

Re: [PATCH v8 2/2] s390: virtio: PV needs VIRTIO I/O device protection

[Cornelia Huck]

On Tue, 18 Aug 2020 16:58:31 +0200
Pierre Morel <pmorel@linux.ibm.com> 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.
> Define CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS and export
> arch_has_restricted_memory_access 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>
> ---
>  arch/s390/Kconfig   |  1 +
>  arch/s390/mm/init.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 9cfd8de907cb..d4a3ef4fa27b 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -820,6 +820,7 @@ menu "Virtualization"
>  config PROTECTED_VIRTUALIZATION_GUEST
>  	def_bool n
>  	prompt "Protected virtualization guest support"
> +	select ARCH_HAS_RESTRICTED_MEMORY_ACCESS
>  	help
>  	  Select this option, if you want to be able to run this
>  	  kernel as a protected virtualization KVM guest.
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 6dc7c3b60ef6..aec04d7dd089 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,35 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
>  
> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS
> +/*
> + * arch_has_restricted_memory_access
> + * @dev: the VIRTIO device being added
> + *
> + * Return an error if required features are missing on a guest running
> + * with protected virtualization.
> + */
> +int arch_has_restricted_memory_access(struct virtio_device *dev)
> +{
> +	if (!is_prot_virt_guest())
> +		return 0;

If you just did a

return is_prot_virt_guest();

and did the virtio feature stuff in the virtio core, this function
would be short and sweet :)

> +
> +	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;
> +}
> +EXPORT_SYMBOL(arch_has_restricted_memory_access);
> +#endif
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {

Re: [PATCH v8 2/2] s390: virtio: PV needs VIRTIO I/O device protection

[Pierre Morel]

On 2020-08-18 19:22, Cornelia Huck wrote:

>> + */
>> +int arch_has_restricted_memory_access(struct virtio_device *dev)
>> +{
>> +	if (!is_prot_virt_guest())
>> +		return 0;
> 
> If you just did a
> 
> return is_prot_virt_guest();
> 
> and did the virtio feature stuff in the virtio core, this function
> would be short and sweet :)


yes, the smaller the better, thanks

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen