[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM |
Date: |
Wed, 18 Jan 2017 19:50:54 +0200 |
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.
> 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).
>
> 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?
--
MST