Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM

[Michael S. Tsirkin]

On Fri, Jan 20, 2017 at 11:28:53AM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月20日 06:27, Michael S. Tsirkin wrote:
> > On Thu, Jan 19, 2017 at 11:12:21AM +0800, Jason Wang wrote:
> > > 
> > > 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).
> > > > > 
> > > > > 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 work
> > > for legacy device. For kernel vhost, I'd prefer to keep the current
> > > fallback.
> > I'm not so sure. This has security implications, e.g. user might
> > expect not to be affected by a bug in qemu if specifying vhost=on.
> 
> Ok.
> 
> > 
> > > 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
> > I think we should at least deny connection.
> 
> Ok and we need do something after denying it, how about vdev->broken or
> others?
> 
> Thanks

For vhost-user qemu does not make progress until there's
a valid connection.