| Message ID | 1594111477-15401-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 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; > +}
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
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; > +} >
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; > > +}
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
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
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
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; +}
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(+)