[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated |
Date: |
Wed, 14 Dec 2016 08:44:51 +0100 |
On Tue, 13 Dec 2016 15:27:45 -0600
Michael Roth <address@hidden> wrote:
> Quoting Maxime Coquelin (2016-09-13 08:30:30)
> > Currently, devices are plugged before features are negotiated.
> > If the backend doesn't support VIRTIO_F_VERSION_1, the transport
> > needs to rewind some settings.
> >
> > This is the case for CCW, for which a post_plugged callback had
> > been introduced, where max_rev field is just updated if
> > VIRTIO_F_VERSION_1 is not supported by the backend.
> > For PCI, implementing post_plugged would be much more
> > complicated, so it needs to know whether the backend supports
> > VIRTIO_F_VERSION_1 at plug time.
> >
> > Currently, nothing is done for PCI. Modern capabilities get
> > exposed to the guest even if VIRTIO_F_VERSION_1 is not supported
> > by the backend, which confuses the guest.
> >
> > This patch replaces existing post_plugged solution with an
> > approach that fits with both transports.
> > Features negotiation is performed before ->device_plugged() call.
> > A pre_plugged callback is introduced so that the transports can
> > set their supported features.
> >
> > Cc: Michael S. Tsirkin <address@hidden>
> > Cc: address@hidden
> > Tested-by: Cornelia Huck <address@hidden> [ccw]
> > Reviewed-by: Cornelia Huck <address@hidden>
> > Reviewed-by: Marcel Apfelbaum <address@hidden>
> > Signed-off-by: Maxime Coquelin <address@hidden>
>
>
> It looks like this patch breaks migration under certain circumstances.
> One such scenario is migrating 2.7 -> 2.8.0-rc3 as detailed below on a
> host that doesn't have support for virtio-1 in vhost (which was
> introduced via 41e3e42 in kernel 3.18. In my case, I'm using Ubuntu
> 14.04, kernel 3.13):
>
> source (2.7.0):
>
> sudo build/v2.7.0/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -L
> build/v2.7.0-bios -M pc-i440fx-2.7 -m 512M -drive
> file=/home/mdroth/vm/win7_home_sp1_virtio_testing.qcow2,if=virtio -vga
> cirrus -smp 4 -device virtio-net-pci,netdev=net0 -netdev
> tap,id=net0,vhost=on,script=/etc/qemu-ifup -monitor
> unix:/tmp/vm-hmp.sock,server,nowait -qmp
> unix:/tmp/vm-qmp.sock,server,nowait -vnc :200
>
> target (2.8.0-rc3):
>
> sudo build/v2.8.0-rc3/x86_64-softmmu/qemu-system-x86_64 -enable-kvm
> -L build/v2.8.0-rc3-bios -M pc-i440fx-2.7 -m 512M -drive
> file=/home/mdroth/vm/win7_home_sp1_virtio_testing.qcow2,if=virtio -vga
> cirrus -smp 4 -device virtio-net-pci,netdev=net0 -netdev
> tap,id=net0,vhost=on,script=/etc/qemu-ifup -monitor
> unix:/tmp/vm-hmp-incoming.sock,server,nowait -qmp
> unix:/tmp/vm-qmp-incoming.sock,server,nowait -vnc :201
> -incoming unix:/tmp/migrate.sock
>
> error on target after migration:
>
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x34
> read: 98 device: 40 cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load virtio-net:virtio
> qemu-system-x86_64: error while loading state for instance 0x0 of
> device '0000:00:03.0/virtio-net'
> qemu-system-x86_64: load of migration failed: Invalid argument
>
Ugh. Let me double-check what happens for ccw. I could have sworn I
tested this...
>
> Based on discussion on IRC (excerpts below), I think the new handling needs
> to be controllable via a machine option that can be disabled by default
> for older machine types. This is being considered a release blocker for
> 2.8 since there are still pre-3.18 LTS kernels in the wild.
>
>
> root-cause:
>
> 14:35 < stefanha> v3.13 will not work
> 14:35 < stefanha> VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY)
> |
> 14:35 < stefanha> (1ULL <<
> VIRTIO_RING_F_INDIRECT_DESC) |
> 14:35 < stefanha> (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> 14:35 < stefanha> (1ULL << VHOST_F_LOG_ALL),
> 14:35 < stefanha> The kernel side only knows about these guys
> 14:35 < davidgiluk> our downstream 3.10.0-524 seems to work - but that's
> probably got all the vhost stuff backported
> 14:35 < stefanha> plus these guys:
> 14:35 < stefanha> F VHOST_NET_FEATURES = VHOST_FEATURES |
> 14:35 < stefanha> (1ULL <<
> VHOST_NET_F_VIRTIO_NET_HDR) |
> 14:35 < stefanha> (1ULL << VIRTIO_NET_F_MRG_RXBUF),
> 14:36 < stefanha> mdroth: So QEMU is going to check a list of feature bits
> including VERSION_1
> 14:36 < stefanha> and it will see that vhost doesn't support them.
> 14:36 < stefanha> So we're kind of doing the right thing now.
> 14:36 < stefanha> Before userspace was overriding the fact that vhost cannot
> handle VERSION_1.
> 14:36 < stefanha> ...except we broke migration
> 14:36 < davidgiluk> stefanha: But shouldn't it refuse to startup ?
> 14:36 < stefanha> Everything is perfect*
> 14:36 < stefanha> * except we broke migration
> 14:36 < stefanha> :)
>
> suggestions on how to fix this:
>
> 14:40 < stefanha> My understanding is this bug is vhost-specific. If you
> have an old kernel that doesn't support VERSION_1 vhost then migration will
> break.
> 14:41 < stefanha> The actual bug is in QEMU though, not vhost
> 14:42 < stefanha> vhost is reporting the truth. It's QEMU that has changed
> behavior.
> 14:44 < stefanha> mdroth: I think a lame fix would be to make
> virtio_pci_device_plugged() dependent on a flag that says: use old broken
> behavior instead of reporting the truth to the guest.
> 14:44 < stefanha> The flag could be enabled for all old machine types
> 14:44 < stefanha> and new machine types will report the truth.
> 14:44 < stefanha> That way migration continues to work.
I'll check whether we would need something for ccw as well.
> 14:44 < stefanha> Not sure if anyone can think of a nicer solution.
> 14:45 < stefanha> But we're going to have to keep lying to the guest if we
> want to preserve migration compatibility
> 14:45 < stefanha> The key change in behavior with the patch you identified is:
> 14:46 < stefanha> if (!virtio_has_feature(vdev->host_features,
> VIRTIO_F_VERSION_1)) {
> 14:46 < stefanha> virtio_pci_disable_modern(proxy);
> 14:46 < stefanha> Previously it didn't care about vdev->host_features. It
> simply allowed VERSION_1 when proxy's disable_modern boolean was false.
> 14:47 < mdroth> stefanha: ok, that explains why it seems to work with
> disable-modern=true
> 14:48 < stefanha> mdroth: Your Ubuntu kernel old but 14.04 LTS is definitely
> still around so I don't think we can ship QEMU 2.8 like this.
> 14:49 < stefanha> mdroth: Let's summarize it on the mailing list and see what
> Michael Tsirkin and Maxime Coquelin think.
> 14:49 < mdroth> stefanha: i suppose a potential workaround would be to tell
> users to set disable-modern= to match their vhost capabilities, but it's hard
> for them to apply that retroactively if they're looking to migrate
Another thought: Maybe this bug only surfaced right now because older
qemus defaulted virtio-pci to legacy?
(I think modern virtio-pci with old vhost resulted in a config that was
rejected at least by Linux guests. Because pci defaulted to legacy, we
only had the post-plugged workaround for ccw before.)
> 14:49 < stefanha> We can delay the release in the meantime.
> 14:50 < stefanha> mdroth: I don't think a workaround is going to be smooth in
> this case
> 14:50 < stefanha> People will only notice once migration fails,
> 14:50 < stefanha> and that's when you lose your VM state!
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Michael Roth, 2016/12/13
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated,
Cornelia Huck <=
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Maxime Coquelin, 2016/12/14
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Stefan Hajnoczi, 2016/12/14
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Cornelia Huck, 2016/12/14
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Maxime Coquelin, 2016/12/14
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Dr. David Alan Gilbert, 2016/12/14
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Maxime Coquelin, 2016/12/14
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Cornelia Huck, 2016/12/14
- Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Marcel Apfelbaum, 2016/12/14
Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated, Maxime Coquelin, 2016/12/14