Cornelia Huck
2020-Aug-18 17:19 UTC
[PATCH v8 1/2] virtio: let arch validate VIRTIO features
On Tue, 18 Aug 2020 16:58:30 +0200 Pierre Morel <pmorel at linux.ibm.com> wrote:> An architecture may need to validate the VIRTIO devices features > based on architecture specifics. > > Provide a new Kconfig entry, CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS, > the architecture can select when it provides a callback named > arch_has_restricted_memory_access to validate the virtio device > features. > > Signed-off-by: Pierre Morel <pmorel at linux.ibm.com> > --- > drivers/virtio/Kconfig | 6 ++++++ > drivers/virtio/virtio.c | 4 ++++ > include/linux/virtio_config.h | 9 +++++++++ > 3 files changed, 19 insertions(+) > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig > index 5809e5f5b157..eef09e3c92f9 100644 > --- a/drivers/virtio/Kconfig > +++ b/drivers/virtio/Kconfig > @@ -6,6 +6,12 @@ config VIRTIO > bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG > or CONFIG_S390_GUEST. > > +config ARCH_HAS_RESTRICTED_MEMORY_ACCESS > + bool > + help > + This option is selected by any architecture enforcing > + VIRTIO_F_IOMMU_PLATFORMThis option is only for a very specific case of "restricted memory access", namely the kind that requires IOMMU_PLATFORM for virtio devices. ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS? Or is this intended to cover cases outside of virtio as well?> + > menuconfig VIRTIO_MENU > bool "Virtio drivers" > default y > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index a977e32a88f2..1471db7d6510 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -176,6 +176,10 @@ int virtio_finalize_features(struct virtio_device *dev) > if (ret) > return ret; > > + ret = arch_has_restricted_memory_access(dev); > + if (ret) > + return ret;Hm, I'd rather have expected something like if (arch_has_restricted_memory_access(dev)) { // enforce VERSION_1 and IOMMU_PLATFORM } Otherwise, you're duplicating the checks in the individual architecture callbacks again. [Not sure whether the device argument would be needed here; are there architectures where we'd only require IOMMU_PLATFORM for a subset of virtio devices?]> + > 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..f6b82541c497 100644 > --- a/include/linux/virtio_config.h > +++ b/include/linux/virtio_config.h > @@ -459,4 +459,13 @@ static inline void virtio_cwrite64(struct virtio_device *vdev, > _r; \ > }) > > +#ifdef CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS > +int arch_has_restricted_memory_access(struct virtio_device *dev); > +#else > +static inline int arch_has_restricted_memory_access(struct virtio_device *dev) > +{ > + return 0; > +} > +#endif /* CONFIG_ARCH_HAS_RESTRICTED_MEMORY_ACCESS */ > + > #endif /* _LINUX_VIRTIO_CONFIG_H */
Pierre Morel
2020-Aug-19 08:50 UTC
[PATCH v8 1/2] virtio: let arch validate VIRTIO features
On 2020-08-18 19:19, Cornelia Huck wrote:> On Tue, 18 Aug 2020 16:58:30 +0200 > Pierre Morel <pmorel at linux.ibm.com> wrote: >...>> +config ARCH_HAS_RESTRICTED_MEMORY_ACCESS >> + bool >> + help >> + This option is selected by any architecture enforcing >> + VIRTIO_F_IOMMU_PLATFORM > > This option is only for a very specific case of "restricted memory > access", namely the kind that requires IOMMU_PLATFORM for virtio > devices. ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS? Or is this intended > to cover cases outside of virtio as well?AFAIK we did not identify other restrictions so adding VIRTIO in the name should be the best thing to do. If new restrictions appear they also may be orthogonal. I will change to ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS if no one complains.> >> + >> menuconfig VIRTIO_MENU >> bool "Virtio drivers" >> default y >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >> index a977e32a88f2..1471db7d6510 100644 >> --- a/drivers/virtio/virtio.c >> +++ b/drivers/virtio/virtio.c >> @@ -176,6 +176,10 @@ int virtio_finalize_features(struct virtio_device *dev) >> if (ret) >> return ret; >> >> + ret = arch_has_restricted_memory_access(dev); >> + if (ret) >> + return ret; > > Hm, I'd rather have expected something like > > if (arch_has_restricted_memory_access(dev)) {may be also change the callback name to arch_has_restricted_virtio_memory_access() ?> // enforce VERSION_1 and IOMMU_PLATFORM > } > > Otherwise, you're duplicating the checks in the individual architecture > callbacks again.Yes, I agree and go back this way.> > [Not sure whether the device argument would be needed here; are there > architectures where we'd only require IOMMU_PLATFORM for a subset of > virtio devices?]I don't think so and since we do the checks locally, we do not need the device argument anymore. Thanks, Pierre -- Pierre Morel IBM Lab Boeblingen
Cornelia Huck
2020-Aug-19 09:34 UTC
[PATCH v8 1/2] virtio: let arch validate VIRTIO features
On Wed, 19 Aug 2020 10:50:18 +0200 Pierre Morel <pmorel at linux.ibm.com> wrote:> On 2020-08-18 19:19, Cornelia Huck wrote: > > On Tue, 18 Aug 2020 16:58:30 +0200 > > Pierre Morel <pmorel at linux.ibm.com> wrote: > > > ... > >> +config ARCH_HAS_RESTRICTED_MEMORY_ACCESS > >> + bool > >> + help > >> + This option is selected by any architecture enforcing > >> + VIRTIO_F_IOMMU_PLATFORM > > > > This option is only for a very specific case of "restricted memory > > access", namely the kind that requires IOMMU_PLATFORM for virtio > > devices. ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS? Or is this intended > > to cover cases outside of virtio as well? > > AFAIK we did not identify other restrictions so adding VIRTIO in the > name should be the best thing to do. > > If new restrictions appear they also may be orthogonal. > > I will change to ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS if no one > complains. > > > > >> + > >> menuconfig VIRTIO_MENU > >> bool "Virtio drivers" > >> default y > >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > >> index a977e32a88f2..1471db7d6510 100644 > >> --- a/drivers/virtio/virtio.c > >> +++ b/drivers/virtio/virtio.c > >> @@ -176,6 +176,10 @@ int virtio_finalize_features(struct virtio_device *dev) > >> if (ret) > >> return ret; > >> > >> + ret = arch_has_restricted_memory_access(dev); > >> + if (ret) > >> + return ret; > > > > Hm, I'd rather have expected something like > > > > if (arch_has_restricted_memory_access(dev)) { > > may be also change the callback name to > arch_has_restricted_virtio_memory_access() ?Yes, why not.> > > // enforce VERSION_1 and IOMMU_PLATFORM > > } > > > > Otherwise, you're duplicating the checks in the individual architecture > > callbacks again. > > Yes, I agree and go back this way. > > > > > [Not sure whether the device argument would be needed here; are there > > architectures where we'd only require IOMMU_PLATFORM for a subset of > > virtio devices?] > > I don't think so and since we do the checks locally, we do not need the > device argument anymore.Yes, that would also remove some layering entanglement.
Apparently Analagous Threads
- [PATCH v8 1/2] virtio: let arch validate VIRTIO features
- [PATCH v8 1/2] virtio: let arch validate VIRTIO features
- [PATCH v8 1/2] virtio: let arch validate VIRTIO features
- [PATCH v8 0/2] s390: virtio: let arch validate VIRTIO features
- [PATCH v9 1/2] virtio: let arch advertise guest's memory access restrictions