|
From: | Jason Wang |
Subject: | Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM |
Date: | Thu, 19 Jan 2017 11:12:21 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 |
On 2017年01月19日 01:50, Michael S. Tsirkin wrote:
On Wed, Jan 18, 2017 at 10:42:48AM +0800, Jason Wang wrote:On 2017年01月17日 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 <address@hidden> --- 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 = VIRTIO_BUS(qbus); VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); + bool has_iommu = 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 != NULL && - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { + if (klass->get_dma_as != NULL && has_iommu) { + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); vdev->dma_as = klass->get_dma_as(qbus->parent); } else { vdev->dma_as = &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 existed even before this patch). ThanksI 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 work for legacy device. For kernel vhost, I'd prefer to keep the current fallback. For vhost-user, it seems not easy and we could not just terminate qemu in this case consider this kind of failure can only be detected during start not init.
Thanks
[Prev in Thread] | Current Thread | [Next in Thread] |