| Message ID | 1594283959-13742-3-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: virtio: let arch validate VIRTIO features | expand |
On Thu, 9 Jul 2020 10:39:19 +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. 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> > --- > 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 <cohuck@redhat.com>
On Thu, 9 Jul 2020 10:57:33 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, 9 Jul 2020 10:39:19 +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. 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 <pmorel@linux.ibm.com> > > --- > > 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 <pasic@linux.ibm.com> > > > + return -ENODEV; > > + } > > + > > + return 0; > > +} > > + > > /* protected virtualization */ > > static void pv_init(void) > > { > > Either way, > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> >
On 2020-07-09 10:57, Cornelia Huck wrote: > On Thu, 9 Jul 2020 10:39:19 +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. 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> >> --- >> 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 <cohuck@redhat.com> > Thanks, Pierre
On 2020-07-09 11:55, Halil Pasic wrote: > On Thu, 9 Jul 2020 10:57:33 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Thu, 9 Jul 2020 10:39:19 +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. 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 <pmorel@linux.ibm.com> >>> --- >>> 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 <pasic@linux.ibm.com> Thanks, Pierre
On Thu, 9 Jul 2020 12:51:58 +0200 Pierre Morel <pmorel@linux.ibm.com> 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 <pmorel@linux.ibm.com> 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
On Thu, 9 Jul 2020 16:51:04 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > > > On 2020-07-09 16:47, Halil Pasic wrote: > > On Thu, 9 Jul 2020 12:51:58 +0200 > > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > >>>> +int arch_validate_virtio_features(struct virtio_device *dev) > >>>> +{ > >>>> + if (!is_prot_virt_guest()) > >>>> + return 0; > >>>> + > >>>> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) { > >>>> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n"); > >>> > >>> I'd probably use "legacy virtio not supported with protected > >>> virtualization". > >>> > >>>> + return -ENODEV; > >>>> + } > >>>> + > >>>> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) { > >>>> + dev_warn(&dev->dev, > >>>> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n"); > >>> > >>> "support for limited memory access required for protected > >>> virtualization" > >>> > >>> ? > >>> > >>> Mentioning the feature flag is shorter in both cases, though. > >> > >> And I think easier to look for in case of debugging purpose. > >> I change it if there is more demands. > > > > Not all our end users are kernel and/or qemu developers. I find the > > messages from v4 less technical, more informative, and way better. > > > > Regards, > > Halil > > > > Can you please tell me the messages you are speaking of, because for me > the warning's messages are exactly the same in v4 and v5!? > > I checked many times, but may be I still missed something. > Sorry, my bad. My brain is over-generating. The messages where discussed at v3 and Connie made a very similar proposal to the one above which I seconded (for reference look at Message-ID: <833c71f2-0057-896a-5e21-2c6263834402@linux.ibm.com>). I was under the impression that it got implemented in v4, but it was not. That's why I ended up talking bs. Regards, Halil
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) {
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> --- arch/s390/mm/init.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)