From: Halil Pasic <pasic@linux.ibm.com> To: Pierre Morel <pmorel@linux.ibm.com> Cc: linux-kernel@vger.kernel.org, borntraeger@de.ibm.com, frankja@linux.ibm.com, mst@redhat.com, jasowang@redhat.com, cohuck@redhat.com, kvm@vger.kernel.org, linux-s390@vger.kernel.org, virtualization@lists.linux-foundation.org, <thomas.lendacky@amd.com>, David Gibson <david@gibson.dropbear.id.au>, Ram Pai <linuxram@us.ibm.com>, Heiko Carstens <heiko.carstens@de.ibm.com>, Vasily Gorbik <gor@linux.ibm.com> Subject: Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature Date: Tue, 16 Jun 2020 11:52:02 +0200 [thread overview] Message-ID: <20200616115202.0285aa08.pasic@linux.ibm.com> (raw) In-Reply-To: <1592224764-1258-2-git-send-email-pmorel@linux.ibm.com> On Mon, 15 Jun 2020 14:39:24 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: I find the subject (commit short) sub optimal. The 'arch' is already accepting devices 'without IOMMU feature'. What you are introducing is the ability to reject. > An architecture protecting the guest memory against unauthorized host > access may want to enforce VIRTIO I/O device protection through the > use of VIRTIO_F_IOMMU_PLATFORM. > > Let's give a chance to the architecture to accept or not devices > without VIRTIO_F_IOMMU_PLATFORM. > I don't particularly like the commit message. In general, I believe using access_platform instead of iommu_platform would really benefit us. > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > arch/s390/mm/init.c | 6 ++++++ > drivers/virtio/virtio.c | 9 +++++++++ > include/linux/virtio.h | 2 ++ > 3 files changed, 17 insertions(+) > > diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c > index 87b2d024e75a..3f04ad09650f 100644 > --- a/arch/s390/mm/init.c > +++ b/arch/s390/mm/init.c > @@ -46,6 +46,7 @@ > #include <asm/kasan.h> > #include <asm/dma-mapping.h> > #include <asm/uv.h> > +#include <linux/virtio.h> arch/s390/mm/init.c including virtio.h looks a bit strange to me, but if Heiko and Vasily don't mind, neither do I. > > pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir); > > @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev) > return is_prot_virt_guest(); > } > > +int arch_needs_iommu_platform(struct virtio_device *dev) Maybe prefixing the name with virtio_ would help provide the proper context. > +{ > + return is_prot_virt_guest(); > +} > + > /* protected virtualization */ > static void pv_init(void) > { > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index a977e32a88f2..30091089bee8 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status) > } > EXPORT_SYMBOL_GPL(virtio_add_status); > > +int __weak arch_needs_iommu_platform(struct virtio_device *dev) > +{ > + return 0; > +} > + Adding some people that could be interested in overriding this as well to the cc list. > int virtio_finalize_features(struct virtio_device *dev) > { > int ret = dev->config->finalize_features(dev); > @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev) > if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) > return 0; > > + if (arch_needs_iommu_platform(dev) && > + !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) > + return -EIO; > + Why EIO? Overall, I think it is a good idea to have something that is going to protect us from this scenario. Regards, Halil > virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); > status = dev->config->get_status(dev); > if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index a493eac08393..2c46b310c38c 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv); > #define module_virtio_driver(__virtio_driver) \ > module_driver(__virtio_driver, register_virtio_driver, \ > unregister_virtio_driver) > + > +int arch_needs_iommu_platform(struct virtio_device *dev); > #endif /* _LINUX_VIRTIO_H */
WARNING: multiple messages have this Message-ID (diff)
From: Halil Pasic <pasic@linux.ibm.com> To: Pierre Morel <pmorel@linux.ibm.com> Cc: linux-kernel@vger.kernel.org, borntraeger@de.ibm.com, frankja@linux.ibm.com, mst@redhat.com, jasowang@redhat.com, cohuck@redhat.com, kvm@vger.kernel.org, linux-s390@vger.kernel.org, virtualization@lists.linux-foundation.org, thomas.lendacky@amd.com, David Gibson <david@gibson.dropbear.id.au>, Ram Pai <linuxram@us.ibm.com>, Heiko Carstens <heiko.carstens@de.ibm.com>, Vasily Gorbik <gor@linux.ibm.com> Subject: Re: [PATCH v2 1/1] s390: virtio: let arch accept devices without IOMMU feature Date: Tue, 16 Jun 2020 11:52:02 +0200 [thread overview] Message-ID: <20200616115202.0285aa08.pasic@linux.ibm.com> (raw) In-Reply-To: <1592224764-1258-2-git-send-email-pmorel@linux.ibm.com> On Mon, 15 Jun 2020 14:39:24 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: I find the subject (commit short) sub optimal. The 'arch' is already accepting devices 'without IOMMU feature'. What you are introducing is the ability to reject. > An architecture protecting the guest memory against unauthorized host > access may want to enforce VIRTIO I/O device protection through the > use of VIRTIO_F_IOMMU_PLATFORM. > > Let's give a chance to the architecture to accept or not devices > without VIRTIO_F_IOMMU_PLATFORM. > I don't particularly like the commit message. In general, I believe using access_platform instead of iommu_platform would really benefit us. > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > arch/s390/mm/init.c | 6 ++++++ > drivers/virtio/virtio.c | 9 +++++++++ > include/linux/virtio.h | 2 ++ > 3 files changed, 17 insertions(+) > > diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c > index 87b2d024e75a..3f04ad09650f 100644 > --- a/arch/s390/mm/init.c > +++ b/arch/s390/mm/init.c > @@ -46,6 +46,7 @@ > #include <asm/kasan.h> > #include <asm/dma-mapping.h> > #include <asm/uv.h> > +#include <linux/virtio.h> arch/s390/mm/init.c including virtio.h looks a bit strange to me, but if Heiko and Vasily don't mind, neither do I. > > pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir); > > @@ -162,6 +163,11 @@ bool force_dma_unencrypted(struct device *dev) > return is_prot_virt_guest(); > } > > +int arch_needs_iommu_platform(struct virtio_device *dev) Maybe prefixing the name with virtio_ would help provide the proper context. > +{ > + return is_prot_virt_guest(); > +} > + > /* protected virtualization */ > static void pv_init(void) > { > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index a977e32a88f2..30091089bee8 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -167,6 +167,11 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status) > } > EXPORT_SYMBOL_GPL(virtio_add_status); > > +int __weak arch_needs_iommu_platform(struct virtio_device *dev) > +{ > + return 0; > +} > + Adding some people that could be interested in overriding this as well to the cc list. > int virtio_finalize_features(struct virtio_device *dev) > { > int ret = dev->config->finalize_features(dev); > @@ -179,6 +184,10 @@ int virtio_finalize_features(struct virtio_device *dev) > if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) > return 0; > > + if (arch_needs_iommu_platform(dev) && > + !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) > + return -EIO; > + Why EIO? Overall, I think it is a good idea to have something that is going to protect us from this scenario. Regards, Halil > virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); > status = dev->config->get_status(dev); > if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index a493eac08393..2c46b310c38c 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv); > #define module_virtio_driver(__virtio_driver) \ > module_driver(__virtio_driver, register_virtio_driver, \ > unregister_virtio_driver) > + > +int arch_needs_iommu_platform(struct virtio_device *dev); > #endif /* _LINUX_VIRTIO_H */
next prev parent reply other threads:[~2020-06-16 9:52 UTC|newest] Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-15 12:39 [PATCH v2 0/1] s390: virtio: let's arch choose to accept devices without IOMMU feature Pierre Morel 2020-06-15 12:39 ` [PATCH v2 1/1] s390: virtio: let arch " Pierre Morel 2020-06-15 12:39 ` Pierre Morel 2020-06-16 6:22 ` Jason Wang 2020-06-16 7:33 ` Pierre Morel 2020-06-16 6:55 ` Christian Borntraeger 2020-06-16 6:55 ` Christian Borntraeger 2020-06-16 7:35 ` Pierre Morel 2020-06-16 12:21 ` Cornelia Huck 2020-06-16 9:52 ` Halil Pasic [this message] 2020-06-16 9:52 ` Halil Pasic 2020-06-16 10:52 ` Pierre Morel 2020-06-16 10:52 ` Pierre Morel 2020-06-16 11:57 ` Halil Pasic 2020-06-16 12:17 ` Cornelia Huck 2020-06-16 13:41 ` Pierre Morel 2020-06-16 13:50 ` Cornelia Huck 2020-06-16 12:20 ` Cornelia Huck 2020-06-16 13:36 ` Pierre Morel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200616115202.0285aa08.pasic@linux.ibm.com \ --to=pasic@linux.ibm.com \ --cc=borntraeger@de.ibm.com \ --cc=cohuck@redhat.com \ --cc=david@gibson.dropbear.id.au \ --cc=frankja@linux.ibm.com \ --cc=gor@linux.ibm.com \ --cc=heiko.carstens@de.ibm.com \ --cc=jasowang@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=linuxram@us.ibm.com \ --cc=mst@redhat.com \ --cc=pmorel@linux.ibm.com \ --cc=thomas.lendacky@amd.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.