Cornelia Huck
2020-Jul-07 09:46 UTC
[PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On Tue, 7 Jul 2020 10:44:37 +0200 Pierre Morel <pmorel at 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 at 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_platforms/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; > +}
Pierre Morel
2020-Jul-07 10:38 UTC
[PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On 2020-07-07 11:46, Cornelia Huck wrote:> On Tue, 7 Jul 2020 10:44:37 +0200 > Pierre Morel <pmorel at 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 at 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
Michael S. Tsirkin
2020-Jul-07 11:14 UTC
[PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
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 at 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 at 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; > > +}
Halil Pasic
2020-Jul-07 11:19 UTC
[PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
On Tue, 7 Jul 2020 12:38:17 +0200 Pierre Morel <pmorel at 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 at 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 at 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
Pierre Morel
2020-Jul-07 11:19 UTC
[PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
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 at 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
Possibly Parallel Threads
- [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
- [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
- [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
- [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection
- [PATCH v4 2/2] s390: virtio: PV needs VIRTIO I/O device protection