From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38295) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cU39S-0004lX-D5 for qemu-devel@nongnu.org; Wed, 18 Jan 2017 22:12:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cU39N-00049Z-F3 for qemu-devel@nongnu.org; Wed, 18 Jan 2017 22:12:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35504) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cU39N-00048p-63 for qemu-devel@nongnu.org; Wed, 18 Jan 2017 22:12:33 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 984187D2C0 for ; Thu, 19 Jan 2017 03:12:32 +0000 (UTC) References: <1484625660-3312-1-git-send-email-jasowang@redhat.com> <20170117164326-mutt-send-email-mst@kernel.org> <38dfab22-412f-94a5-4b01-351edf627027@redhat.com> <20170118193122-mutt-send-email-mst@kernel.org> From: Jason Wang Message-ID: Date: Thu, 19 Jan 2017 11:12:21 +0800 MIME-Version: 1.0 In-Reply-To: <20170118193122-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org On 2017=E5=B9=B401=E6=9C=8819=E6=97=A5 01:50, Michael S. Tsirkin wrote: > On Wed, Jan 18, 2017 at 10:42:48AM +0800, Jason Wang wrote: >> >> On 2017=E5=B9=B401=E6=9C=8817=E6=97=A5 22:44, Michael S. Tsirkin wrote= : >>> On Tue, Jan 17, 2017 at 12:01:00PM +0800, Jason Wang wrote: >>>> We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since >>>> VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by >>>> enforce it after vdc->get_features(). >>>> >>>> Signed-off-by: Jason Wang >>>> --- >>>> hw/virtio/virtio-bus.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c >>>> index d31cc00..a886011 100644 >>>> --- a/hw/virtio/virtio-bus.c >>>> +++ b/hw/virtio/virtio-bus.c >>>> @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev,= Error **errp) >>>> VirtioBusState *bus =3D VIRTIO_BUS(qbus); >>>> VirtioBusClass *klass =3D VIRTIO_BUS_GET_CLASS(bus); >>>> VirtioDeviceClass *vdc =3D VIRTIO_DEVICE_GET_CLASS(vdev); >>>> + bool has_iommu =3D virtio_host_has_feature(vdev, VIRTIO_F_IOMMU= _PLATFORM); >>>> DPRINTF("%s: plug device.\n", qbus->name); >>>> @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev,= Error **errp) >>>> klass->device_plugged(qbus->parent, errp); >>>> } >>>> - if (klass->get_dma_as !=3D NULL && >>>> - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { >>>> + if (klass->get_dma_as !=3D NULL && has_iommu) { >>>> + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLA= TFORM); >>>> vdev->dma_as =3D klass->get_dma_as(qbus->parent); >>>> } else { >>>> vdev->dma_as =3D &address_space_memory; >>> I suspect that's not enough, we must also fail or disable vhost >>> (depending on the options), otherwise things won't work. >> Looks like with the patch, VIRTIO_F_IOMMU_PLATFORM will be passed to >> vhost_set_features(). > Okay then. Could you please test on an old host kernel and confirm > what happens? Pls mention this in commit log. Test were done before sending this patch :) Vhost fallback to userspace since it lacks VIRTIO_F_IOMMU_PLATFORM. > >> So if vhost backend does not support it, it will fall >> back to userspace (This may not work for vhost-user, but it's a bug ex= isted >> even before this patch). >> >> Thanks > I guess this is true but this suddenly makes it important to > fix this. So I prefer a patchset where patch 2 addresses > the fallback bug. In which way? Do you think we should > just fail if vhost doesn't work? Does this apply to both > userspace and kernel? For 1.0 devices we could do this through NEEDS_RESET, but it doesn't=20 work for legacy device. For kernel vhost, I'd prefer to keep the current=20 fallback. For vhost-user, it seems not easy and we could not just=20 terminate qemu in this case consider this kind of failure can only be=20 detected during start not init. Thanks