From: Pierre Morel <pmorel@linux.ibm.com> To: Cornelia Huck <cohuck@redhat.com> Cc: linux-kernel@vger.kernel.org, pasic@linux.ibm.com, borntraeger@de.ibm.com, frankja@linux.ibm.com, mst@redhat.com, jasowang@redhat.com, kvm@vger.kernel.org, linux-s390@vger.kernel.org, virtualization@lists.linux-foundation.org, thomas.lendacky@amd.com, david@gibson.dropbear.id.au, linuxram@us.ibm.com, hca@linux.ibm.com, gor@linux.ibm.com Subject: Re: [PATCH v8 1/2] virtio: let arch validate VIRTIO features Date: Wed, 19 Aug 2020 10:50:18 +0200 [thread overview] Message-ID: <64acd55a-8a22-4b84-0f9e-e13196c1520d@linux.ibm.com> (raw) In-Reply-To: <20200818191910.1fc300f2.cohuck@redhat.com> On 2020-08-18 19:19, Cornelia Huck wrote: > On Tue, 18 Aug 2020 16:58:30 +0200 > Pierre Morel <pmorel@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
WARNING: multiple messages have this Message-ID (diff)
From: Pierre Morel <pmorel@linux.ibm.com> To: Cornelia Huck <cohuck@redhat.com> Cc: gor@linux.ibm.com, linux-s390@vger.kernel.org, frankja@linux.ibm.com, kvm@vger.kernel.org, mst@redhat.com, linuxram@us.ibm.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, pasic@linux.ibm.com, borntraeger@de.ibm.com, thomas.lendacky@amd.com, hca@linux.ibm.com, david@gibson.dropbear.id.au Subject: Re: [PATCH v8 1/2] virtio: let arch validate VIRTIO features Date: Wed, 19 Aug 2020 10:50:18 +0200 [thread overview] Message-ID: <64acd55a-8a22-4b84-0f9e-e13196c1520d@linux.ibm.com> (raw) In-Reply-To: <20200818191910.1fc300f2.cohuck@redhat.com> On 2020-08-18 19:19, Cornelia Huck wrote: > On Tue, 18 Aug 2020 16:58:30 +0200 > Pierre Morel <pmorel@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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2020-08-19 8:50 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-18 14:58 [PATCH v8 0/2] s390: virtio: let arch validate VIRTIO features Pierre Morel 2020-08-18 14:58 ` Pierre Morel 2020-08-18 14:58 ` [PATCH v8 1/2] " Pierre Morel 2020-08-18 14:58 ` Pierre Morel 2020-08-18 17:19 ` Cornelia Huck 2020-08-18 17:19 ` Cornelia Huck 2020-08-19 8:50 ` Pierre Morel [this message] 2020-08-19 8:50 ` Pierre Morel 2020-08-19 9:34 ` Cornelia Huck 2020-08-19 9:34 ` Cornelia Huck 2020-08-18 14:58 ` [PATCH v8 2/2] s390: virtio: PV needs VIRTIO I/O device protection Pierre Morel 2020-08-18 14:58 ` Pierre Morel 2020-08-18 17:22 ` Cornelia Huck 2020-08-18 17:22 ` Cornelia Huck 2020-08-19 8:51 ` Pierre Morel 2020-08-19 8:51 ` 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=64acd55a-8a22-4b84-0f9e-e13196c1520d@linux.ibm.com \ --to=pmorel@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=hca@linux.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=pasic@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.