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

[Jason Wang <jasowang@redhat.com>]

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 <jasowang@redhat.com>
>>>> ---
>>>>    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. 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