| Message ID | 20161214163035.3297-1-maxime.coquelin@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 14, 2016 at 4:30 PM, Maxime Coquelin <maxime.coquelin@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. > > 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-ignore-backend-features" > 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 <mdroth@linux.vnet.ibm.com> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > > V3 fixes commit message with the new property name. > V2 changes the naming as proposed by Michael T.and Cornelia, and > fixes commit message. > > hw/virtio/virtio-pci.c | 5 ++++- > hw/virtio/virtio-pci.h | 1 + > include/hw/compat.h | 4 ++++ > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 521ba0b..21c2b9d 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->ignore_backend_features && > + !virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { > virtio_pci_disable_modern(proxy); > > if (!legacy) { > @@ -1852,6 +1853,8 @@ 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-ignore-backend-features", VirtIOPCIProxy, > + ignore_backend_features, false), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index b2a996f..5e07886 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -153,6 +153,7 @@ struct VirtIOPCIProxy { > int config_cap; > uint32_t flags; > bool disable_modern; > + bool ignore_backend_features; > OnOffAuto disable_legacy; > uint32_t class_code; > uint32_t nvectors; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 0f06e11..8dfc7a3 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -18,6 +18,10 @@ > .driver = "intel-iommu",\ > .property = "x-buggy-eim",\ > .value = "true",\ > + },{\ > + .driver = "virtio-pci",\ > + .property = "x-ignore-backend-features",\ > + .value = "on",\ > }, Waiting for Michael Tsirkin's R-b before applying. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Wed, Dec 14, 2016 at 05:30:35PM +0100, Maxime Coquelin 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. > > 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-ignore-backend-features" > 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 <mdroth@linux.vnet.ibm.com> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > > V3 fixes commit message with the new property name. > V2 changes the naming as proposed by Michael T.and Cornelia, and > fixes commit message. > > hw/virtio/virtio-pci.c | 5 ++++- > hw/virtio/virtio-pci.h | 1 + > include/hw/compat.h | 4 ++++ > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 521ba0b..21c2b9d 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->ignore_backend_features && > + !virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { > virtio_pci_disable_modern(proxy); > > if (!legacy) { > @@ -1852,6 +1853,8 @@ 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-ignore-backend-features", VirtIOPCIProxy, > + ignore_backend_features, false), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index b2a996f..5e07886 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -153,6 +153,7 @@ struct VirtIOPCIProxy { > int config_cap; > uint32_t flags; > bool disable_modern; > + bool ignore_backend_features; > OnOffAuto disable_legacy; > uint32_t class_code; > uint32_t nvectors; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 0f06e11..8dfc7a3 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -18,6 +18,10 @@ > .driver = "intel-iommu",\ > .property = "x-buggy-eim",\ > .value = "true",\ > + },{\ > + .driver = "virtio-pci",\ > + .property = "x-ignore-backend-features",\ > + .value = "on",\ > }, > > #define HW_COMPAT_2_6 \ > -- > 2.9.3
On Wed, Dec 14, 2016 at 05:30:35PM +0100, Maxime Coquelin 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. > > 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-ignore-backend-features" > 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 <mdroth@linux.vnet.ibm.com> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > > V3 fixes commit message with the new property name. > V2 changes the naming as proposed by Michael T.and Cornelia, and > fixes commit message. > > hw/virtio/virtio-pci.c | 5 ++++- > hw/virtio/virtio-pci.h | 1 + > include/hw/compat.h | 4 ++++ > 3 files changed, 9 insertions(+), 1 deletion(-) Thanks, applied to my staging tree: https://github.com/stefanha/qemu/commits/staging Stefan
On Thu, 15 Dec 2016 07:38:09 +0000 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Dec 14, 2016 at 05:30:35PM +0100, Maxime Coquelin 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. > > > > 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-ignore-backend-features" > > 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 <mdroth@linux.vnet.ibm.com> > > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > --- > > > > V3 fixes commit message with the new property name. > > V2 changes the naming as proposed by Michael T.and Cornelia, and > > fixes commit message. > > > > hw/virtio/virtio-pci.c | 5 ++++- > > hw/virtio/virtio-pci.h | 1 + > > include/hw/compat.h | 4 ++++ > > 3 files changed, 9 insertions(+), 1 deletion(-) > > Thanks, applied to my staging tree: > https://github.com/stefanha/qemu/commits/staging > > Stefan Hm, missing my R-b I gave for v2... feel free to add Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> if you still want to change it.
On Thu, Dec 15, 2016 at 09:26:56AM +0100, Cornelia Huck wrote: > On Thu, 15 Dec 2016 07:38:09 +0000 > Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > On Wed, Dec 14, 2016 at 05:30:35PM +0100, Maxime Coquelin 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. > > > > > > 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-ignore-backend-features" > > > 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 <mdroth@linux.vnet.ibm.com> > > > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > > --- > > > > > > V3 fixes commit message with the new property name. > > > V2 changes the naming as proposed by Michael T.and Cornelia, and > > > fixes commit message. > > > > > > hw/virtio/virtio-pci.c | 5 ++++- > > > hw/virtio/virtio-pci.h | 1 + > > > include/hw/compat.h | 4 ++++ > > > 3 files changed, 9 insertions(+), 1 deletion(-) > > > > Thanks, applied to my staging tree: > > https://github.com/stefanha/qemu/commits/staging > > > > Stefan > > Hm, missing my R-b I gave for v2... feel free to add > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > if you still want to change it. Sorry, the R-b didn't make it into v2.8.0-rc4. The patch management scripts I use apply all R-b for a single email thread. They do not look at previous revisions (it would be hard to tell which R-b still stand and which do not). The usual solution is for the patch author to include previous R-b in new revisions as long as code changes aren't substantial. Stefan
On 12/15/2016 11:31 AM, Stefan Hajnoczi wrote: > On Thu, Dec 15, 2016 at 09:26:56AM +0100, Cornelia Huck wrote: >> On Thu, 15 Dec 2016 07:38:09 +0000 >> Stefan Hajnoczi <stefanha@gmail.com> wrote: >> >>> On Wed, Dec 14, 2016 at 05:30:35PM +0100, Maxime Coquelin 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. >>>> >>>> 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-ignore-backend-features" >>>> 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 <mdroth@linux.vnet.ibm.com> >>>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> >>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> >>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> >>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>>> Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>> --- >>>> >>>> V3 fixes commit message with the new property name. >>>> V2 changes the naming as proposed by Michael T.and Cornelia, and >>>> fixes commit message. >>>> >>>> hw/virtio/virtio-pci.c | 5 ++++- >>>> hw/virtio/virtio-pci.h | 1 + >>>> include/hw/compat.h | 4 ++++ >>>> 3 files changed, 9 insertions(+), 1 deletion(-) >>> >>> Thanks, applied to my staging tree: >>> https://github.com/stefanha/qemu/commits/staging >>> >>> Stefan >> >> Hm, missing my R-b I gave for v2... feel free to add >> >> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> >> >> if you still want to change it. > > Sorry, the R-b didn't make it into v2.8.0-rc4. > > The patch management scripts I use apply all R-b for a single email > thread. They do not look at previous revisions (it would be hard to > tell which R-b still stand and which do not). The usual solution is for > the patch author to include previous R-b in new revisions as long as > code changes aren't substantial. Right that's my fault, sorry Cornelia. Maxime > > Stefan >
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 521ba0b..21c2b9d 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->ignore_backend_features && + !virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { virtio_pci_disable_modern(proxy); if (!legacy) { @@ -1852,6 +1853,8 @@ 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-ignore-backend-features", VirtIOPCIProxy, + ignore_backend_features, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index b2a996f..5e07886 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -153,6 +153,7 @@ struct VirtIOPCIProxy { int config_cap; uint32_t flags; bool disable_modern; + bool ignore_backend_features; OnOffAuto disable_legacy; uint32_t class_code; uint32_t nvectors; diff --git a/include/hw/compat.h b/include/hw/compat.h index 0f06e11..8dfc7a3 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -18,6 +18,10 @@ .driver = "intel-iommu",\ .property = "x-buggy-eim",\ .value = "true",\ + },{\ + .driver = "virtio-pci",\ + .property = "x-ignore-backend-features",\ + .value = "on",\ }, #define HW_COMPAT_2_6 \
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. 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-ignore-backend-features" 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 <mdroth@linux.vnet.ibm.com> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- V3 fixes commit message with the new property name. V2 changes the naming as proposed by Michael T.and Cornelia, and fixes commit message. hw/virtio/virtio-pci.c | 5 ++++- hw/virtio/virtio-pci.h | 1 + include/hw/compat.h | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-)