[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] vhost-scsi: Update 'ioeventfd_started' with hos
From: |
Felipe Franciosi |
Subject: |
Re: [Qemu-devel] [PATCH] vhost-scsi: Update 'ioeventfd_started' with host notifiers |
Date: |
Tue, 8 Nov 2016 17:23:15 +0000 |
> On 8 Nov 2016, at 18:18, Paolo Bonzini <address@hidden> wrote:
>
>
>
> On 07/11/2016 18:23, Felipe Franciosi wrote:
>> Following the recent refactor of virtio notfiers [1], more specifically
>> the patch that uses virtio_bus_set_host_notifier [2] by default, core
>> virtio code requires 'ioeventfd_started' to be set to true/false when
>> the host notifiers are configured. Since vhost-scsi uses the legacy
>> interface, this value is not updated.
>>
>> When booting a guest with a vhost-scsi backend controller, SeaBIOS will
>> initially configure the device which sets all notifiers. The guest will
>> continue to boot fine until the kernel virtio-scsi module reinitialises
>> the device causing a stop followed by another start. Since
>> ioeventfd_started was never set to true, the 'stop' operation triggered
>> by virtio_bus_set_host_notifier() will not result in a call to
>> virtio_pci_ioeventfd_assign(assign=false). This leaves the memory
>> regions with stale notifiers and results on the next start triggering
>> the following assertion:
>>
>> kvm_mem_ioeventfd_add: error adding ioeventfd: File exists
>> Aborted
>>
>> This patch updates ioeventfd_started whenever the notifiers are set or
>> cleared, fixing this issue.
>>
>> Signed-off-by: Felipe Franciosi <address@hidden>
>>
>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07748.html
>> [2] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07760.html
>> ---
>> hw/scsi/vhost-scsi.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index 5b26946..1c6e6d4 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -95,6 +95,7 @@ static int vhost_scsi_start(VHostSCSI *s)
>> if (ret < 0) {
>> return ret;
>> }
>> + VIRTIO_BUS(qbus)->ioeventfd_started = true;
>>
>> s->dev.acked_features = vdev->guest_features;
>> ret = vhost_dev_start(&s->dev, vdev);
>> @@ -152,6 +153,7 @@ static void vhost_scsi_stop(VHostSCSI *s)
>> vhost_scsi_clear_endpoint(s);
>> vhost_dev_stop(&s->dev, vdev);
>> vhost_dev_disable_notifiers(&s->dev, vdev);
>> + VIRTIO_BUS(qbus)->ioeventfd_started = false;
>> }
>>
>> static uint64_t vhost_scsi_get_features(VirtIODevice *vdev,
>>
>
> While a bit hacky, at least for 2.8 the idea should be fine. Only it
> has to be done in vhost_dev_enable_notifiers and
> vhost_dev_disable_notifiers, close to the calls to
> virtio_device_stop_ioeventfd (i.e., set bus->ioeventfd_started after the
> call) and virtio_device_start_ioeventfd (clear it before the call).
Ok. I'll send a v2 tomorrow which does this from vhost_dev_enable_notifiers().
I won't have time to test it now.
>
> I've now worked through a fix that uses start_ioeventfd/stop_ioeventfd,
> and it does comes out nice (29 lines added, 65 removed :)), but it isn't
> bisectable (so it has to be squashed into a single patch) and does not
> cover vhost-net yet. So let's go with your patch for now.
Perfect. Yeah as I told you on IRC I had a quick go at using the new
start/stop, but it didn't seem straightforward and I ran out of time to do it
properly.
>
> I'll send the vm_running fix separately, since that one is a real bugfix.
Ok. Thanks for taking on that one.
>
> Paolo
Cheers,
Felipe