Re: [Qemu-devel] [Qemu-stable] [PATCH v2] virtio-bus: Plug devices after features are negotiated

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Maxime Coquelin (maxime.coquelin@redhat.com) wrote:
> 
> 
> On 12/14/2016 09:59 AM, Cornelia Huck wrote:
> > On Wed, 14 Dec 2016 08:41:55 +0000
> > Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > 
> > > On Wed, Dec 14, 2016 at 8:28 AM, Maxime Coquelin
> > > <maxime.coquelin@redhat.com> wrote:
> > > > 
> > > > 
> > > > On 12/14/2016 08:44 AM, Cornelia Huck wrote:
> > > > > > 
> > > > > > > 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.)
> > > > 
> > > > 
> > > > Yes, for PCI with old vhost, modern enabled and recent kernel on guest,
> > > > we get this failure at virtio-pci probe time:
> > > > 
> > > > virtio_net virtio0: virtio: device uses modern interface but does not have
> > > > VIRTIO_F_VERSION_1.
> > > 
> > > Is this error a regression in QEMU 2.8?
> > 
> > I think it pokes up because modern virtio-pci is now by default on. It
> > was broken before if the user wanted a modern virtio-pci device
> > explicitly.
> > 
> > (ccw defaulted to virtio 1.0 much earlier, so we had the post-plugged
> > solution that this patch replaced and which is basically the same for
> > ccw.)
> > 
> > > 
> > > It's better to ship with an existing issue still open than with a new
> > > regression.  We must not break existing users' setups.
> > > 
> > > A solution for the next QEMU version is to use a flag in the machine
> > > type version telling virtio whether or not allow devices (e.g.
> > > vhost-net) to influence the host feature bits.  Old machine types will
> > > say no, new machine types will say yes.
> > > 
> > > In the meantime I would revert your patch for QEMU 2.8.
> > > 
> > > Maxime, Cornelia, Michael: Do you agree?
> > > 
> > > Stefan
> > 
> > Reverting the patch should be fine for ccw. What about the virtio-pci
> > with old vhost mess, though? Defaulting to modern would mean that users
> > get unusable devices in that setup.
> 
> Just did some tests, and can confirm that reverting the patch would
> re-introduce initial bug, which is breaking virtio-pci when host does
> not support VERSION_1.

Can you modify it so it only fixes the problem on new machine types?
Either that or *fail* if you attempt to bring up a virtio interface
using version 1 with a kernel that doesn't support it (with a note
saying that you could use an option to disable it).

Dave

> Note that this problem is present in v2.7.0 since:
> 
> commit 9a4c0e220d8a4f82b5665d0ee95ef94d8e1509d5
> Author: Marcel Apfelbaum <marcel@redhat.com>
> Date:   Wed Jul 20 18:28:21 2016 +0300
> 
>     hw/virtio-pci: fix virtio behaviour
> 
>     Enable transitional virtio devices by default.
>     Enable virtio-1.0 for devices plugged into
>     PCIe ports (Root ports or Downstream ports).
> 
>     Using the virtio-1 mode will remove the limitation
>     of the number of devices that can be attached to a machine
>     by removing the need for the IO BAR.
> 
>     Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> 
> Maybe better to implement the workaround you proposed Stefan?
> 
> Thanks,
> Maxime
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK