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

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

[Pierre Morel]

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)

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

[Pierre Morel]

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

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

[Cornelia Huck]

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>

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

[Pierre Morel]

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

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

[Christian Borntraeger]

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>

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

[Pierre Morel]

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

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

[Pierre Morel]

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

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

[Cornelia Huck]

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;
> +}

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

[Pierre Morel]

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

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

[Halil Pasic]

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

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

[Michael S. Tsirkin]

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;
> > +}

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

[Pierre Morel]

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

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

[Christian Borntraeger]

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;
> +}
> 

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

[Pierre Morel]

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