On Wed, Dec 14, 2016 at 1:29 PM, Maxime Coquelin <maxime.coque...@redhat.com> wrote: > On 12/14/2016 02:21 PM, Cornelia Huck wrote: >> >> On Wed, 14 Dec 2016 15:16:13 +0200 >> "Michael S. Tsirkin" <m...@redhat.com> wrote: >> >>> On Wed, Dec 14, 2016 at 02:12:10PM +0100, Cornelia Huck wrote: >>>> >>>> On Wed, 14 Dec 2016 13:52:37 +0100 >>>> Maxime Coquelin <maxime.coque...@redhat.com> wrote: >>>> >>>>> This patch fixes a cross-version migration regression introduced >>>>> by commit d1b4259f ("virtio-bus: Plug devices after features are >>>>> negotiated"). >>>>> >>>>> The problem is encountered when host's vhost backend does not support >>>>> VIRTIO_F_VERSION_1, and migration is initiated from a v2.7 or prior >>>>> machine with virtio-pci modern capabilities enabled to a v2.8 machine. >>>> >>>> >>>> Wait, machine versions or qemu versions? >>>> >>>>> >>>>> In this case, modern capabilities get exposed to the guest by the >>>>> source, >>>>> whereas the target will detect version 1 is not supported so will only >>>>> expose legacy capabilities. >>>>> >>>>> The problem is fixed by introducing a new "x-modern-broken" property, >>>>> which is set in v2.7 and prior compatibility modes. Doing this, v2.7 >>>>> machine keeps its broken behaviour (enabling modern while version is >>>>> not supported), and newer machines will behave correctly. >>>>> >>>>> Reported-by: Michael Roth <mdr...@linux.vnet.ibm.com> >>>>> Suggested-by: Stefan Hajnoczi <stefa...@redhat.com> >>>>> Cc: Michael S. Tsirkin <m...@redhat.com> >>>>> Cc: Cornelia Huck <cornelia.h...@de.ibm.com> >>>>> Cc: Marcel Apfelbaum <mar...@redhat.com> >>>>> Cc: Dr. David Alan Gilbert <dgilb...@redhat.com> >>>>> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com> >>>>> --- >>>>> >>>>> I'm not sure about the property name, let me know if you have better >>>>> ideas. >>>>> I didn't tested migration yet, but I wanted to share the patch while I >>>>> test it. >>>>> I tested booting v2.8 and v2.7 machines with !VERSION_1 and get >>>>> expected result: >>>>> - v2.8: Virtio-pci probe succeed >>>>> - v2.7: Virtio-pci probe fails >>>>> >>>>> Thanks, >>>>> Maxime >>>>> >>>>> hw/virtio/virtio-pci.c | 4 +++- >>>>> hw/virtio/virtio-pci.h | 1 + >>>>> include/hw/compat.h | 4 ++++ >>>>> 3 files changed, 8 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >>>>> index 521ba0b..93f6b54 100644 >>>>> --- a/hw/virtio/virtio-pci.c >>>>> +++ b/hw/virtio/virtio-pci.c >>>>> @@ -1580,7 +1580,8 @@ static void virtio_pci_device_plugged(DeviceState >>>>> *d, Error **errp) >>>>> * Virtio capabilities present without >>>>> * VIRTIO_F_VERSION_1 confuses guests >>>>> */ >>>>> - if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) >>>>> { >>>>> + if (!proxy->modern_broken && >>>> >>>> >>>> What you are de facto doing here is ignoring the features supported by >>>> the backend. Call this ->ignore_backend_features or so? >>>> >>>>> + !virtio_has_feature(vdev->host_features, >>>>> VIRTIO_F_VERSION_1)) { >>>>> virtio_pci_disable_modern(proxy); >>>>> >>>>> if (!legacy) { >>>>> @@ -1852,6 +1853,7 @@ static Property virtio_pci_properties[] = { >>>>> VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false), >>>>> DEFINE_PROP_BIT("page-per-vq", VirtIOPCIProxy, flags, >>>>> VIRTIO_PCI_FLAG_PAGE_PER_VQ_BIT, false), >>>>> + DEFINE_PROP_BOOL("x-modern-broken", VirtIOPCIProxy, modern_broken, >>>>> false), >>>> >>>> >>>> What about "x-ignore-backend-features"? >>> >>> >>> It's just the capability that ignores that backend is legacy. >>> >>> x-cap-ignore-backend-legacy >>> >>> ? >> >> >> Sounds good. And has the benefit that nobody will be tempted to set >> that manually :) > > > Thanks Michael & Cornelia, you're definitely better than me at naming > things :) > > I'll send the v2 using this nameing as soon as I get migration case > tested.
Once the property is renamed... Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>