* [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
@ 2017-01-17 4:01 Jason Wang
2017-01-17 14:44 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2017-01-17 4:01 UTC (permalink / raw)
To: mst, qemu-devel; +Cc: Jason Wang
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;
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
2017-01-17 4:01 [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM Jason Wang
@ 2017-01-17 14:44 ` Michael S. Tsirkin
2017-01-18 2:42 ` Jason Wang
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-01-17 14:44 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel
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.
> --
> 2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
2017-01-17 14:44 ` Michael S. Tsirkin
@ 2017-01-18 2:42 ` Jason Wang
2017-01-18 17:50 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2017-01-18 2:42 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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(). 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
2017-01-18 2:42 ` Jason Wang
@ 2017-01-18 17:50 ` Michael S. Tsirkin
2017-01-19 3:12 ` Jason Wang
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-01-18 17:50 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel
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.
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
2017-01-18 17:50 ` Michael S. Tsirkin
@ 2017-01-19 3:12 ` Jason Wang
2017-01-19 22:27 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2017-01-19 3:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
2017-01-19 3:12 ` Jason Wang
@ 2017-01-19 22:27 ` Michael S. Tsirkin
2017-01-20 3:28 ` Jason Wang
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-01-19 22:27 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel
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 <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.
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.
> 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.
--
MST
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
2017-01-19 22:27 ` Michael S. Tsirkin
@ 2017-01-20 3:28 ` Jason Wang
2017-01-20 16:31 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2017-01-20 3:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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 <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.
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM
2017-01-20 3:28 ` Jason Wang
@ 2017-01-20 16:31 ` Michael S. Tsirkin
0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-01-20 16:31 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel
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 <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.
> > 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-20 16:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-17 4:01 [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM Jason Wang
2017-01-17 14:44 ` Michael S. Tsirkin
2017-01-18 2:42 ` Jason Wang
2017-01-18 17:50 ` Michael S. Tsirkin
2017-01-19 3:12 ` Jason Wang
2017-01-19 22:27 ` Michael S. Tsirkin
2017-01-20 3:28 ` Jason Wang
2017-01-20 16:31 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).