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 <[email protected]>
---
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 <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,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
On Thu, 9 Jul 2020 10:39:19 +0200
Pierre Morel <[email protected]> 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 <[email protected]>
> ---
> 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 <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,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 <[email protected]>
On Thu, 9 Jul 2020 10:57:33 +0200
Cornelia Huck <[email protected]> wrote:
> On Thu, 9 Jul 2020 10:39:19 +0200
> Pierre Morel <[email protected]> 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 <[email protected]>
> > ---
> > 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 <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,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 <[email protected]>
>
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /* protected virtualization */
> > static void pv_init(void)
> > {
>
> Either way,
>
> Reviewed-by: Cornelia Huck <[email protected]>
>
On 2020-07-09 10:57, Cornelia Huck wrote:
> On Thu, 9 Jul 2020 10:39:19 +0200
> Pierre Morel <[email protected]> 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 <[email protected]>
>> ---
>> 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 <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,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 <[email protected]>
>
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
On 2020-07-09 11:55, Halil Pasic wrote:
> On Thu, 9 Jul 2020 10:57:33 +0200
> Cornelia Huck <[email protected]> wrote:
>
>> On Thu, 9 Jul 2020 10:39:19 +0200
>> Pierre Morel <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <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,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 <[email protected]>
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
On Thu, 9 Jul 2020 12:51:58 +0200
Pierre Morel <[email protected]> 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
On 2020-07-09 16:47, Halil Pasic wrote:
> On Thu, 9 Jul 2020 12:51:58 +0200
> Pierre Morel <[email protected]> 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
On Thu, 9 Jul 2020 16:51:04 +0200
Pierre Morel <[email protected]> wrote:
>
>
> On 2020-07-09 16:47, Halil Pasic wrote:
> > On Thu, 9 Jul 2020 12:51:58 +0200
> > Pierre Morel <[email protected]> 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:
<[email protected]>). 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