* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 12:52 [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines Maxime Coquelin
@ 2016-12-14 13:12 ` Cornelia Huck
2016-12-14 13:16 ` Michael S. Tsirkin
2016-12-14 13:13 ` Michael S. Tsirkin
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Cornelia Huck @ 2016-12-14 13:12 UTC (permalink / raw)
To: Maxime Coquelin
Cc: mdroth, stefanha, qemu-devel, mst, marcel,
Dr . David Alan Gilbert
On Wed, 14 Dec 2016 13:52:37 +0100
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.
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 <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: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@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"?
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index b2a996f..1dca223 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 modern_broken;
> OnOffAuto disable_legacy;
> uint32_t class_code;
> uint32_t nvectors;
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 0f06e11..fe11723 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-modern-broken",\
> + .value = "on",\
This unfortunately has the same problem wrt -global propagation as the
other virtio-pci compat props (unexpeced overrides). There's basically
zero chance, however, that somebody will want to set this property
manually (unline disable-legacy/disable-modern), and there's a patch
targeting 2.8.1, so I think we can live with it. (Alternatively, you
would need to set this property on all possible virtio-pci devices.)
> },
>
> #define HW_COMPAT_2_6 \
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 13:12 ` Cornelia Huck
@ 2016-12-14 13:16 ` Michael S. Tsirkin
2016-12-14 13:21 ` Cornelia Huck
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2016-12-14 13:16 UTC (permalink / raw)
To: Cornelia Huck
Cc: Maxime Coquelin, mdroth, stefanha, qemu-devel, marcel,
Dr . David Alan Gilbert
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.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.
>
> 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 <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: Marcel Apfelbaum <marcel@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@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
?
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index b2a996f..1dca223 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 modern_broken;
> > OnOffAuto disable_legacy;
> > uint32_t class_code;
> > uint32_t nvectors;
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 0f06e11..fe11723 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-modern-broken",\
> > + .value = "on",\
>
> This unfortunately has the same problem wrt -global propagation as the
> other virtio-pci compat props (unexpeced overrides). There's basically
> zero chance, however, that somebody will want to set this property
> manually (unline disable-legacy/disable-modern), and there's a patch
> targeting 2.8.1, so I think we can live with it. (Alternatively, you
> would need to set this property on all possible virtio-pci devices.)
YEs.
> > },
> >
> > #define HW_COMPAT_2_6 \
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 13:16 ` Michael S. Tsirkin
@ 2016-12-14 13:21 ` Cornelia Huck
2016-12-14 13:29 ` Maxime Coquelin
0 siblings, 1 reply; 15+ messages in thread
From: Cornelia Huck @ 2016-12-14 13:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Maxime Coquelin, mdroth, stefanha, qemu-devel, marcel,
Dr . David Alan Gilbert
On Wed, 14 Dec 2016 15:16:13 +0200
"Michael S. Tsirkin" <mst@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.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.
> >
> > 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 <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: Marcel Apfelbaum <marcel@redhat.com>
> > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Signed-off-by: Maxime Coquelin <maxime.coquelin@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 :)
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 13:21 ` Cornelia Huck
@ 2016-12-14 13:29 ` Maxime Coquelin
2016-12-14 13:45 ` Stefan Hajnoczi
0 siblings, 1 reply; 15+ messages in thread
From: Maxime Coquelin @ 2016-12-14 13:29 UTC (permalink / raw)
To: Cornelia Huck, Michael S. Tsirkin
Cc: mdroth, stefanha, qemu-devel, marcel, Dr . David Alan Gilbert
On 12/14/2016 02:21 PM, Cornelia Huck wrote:
> On Wed, 14 Dec 2016 15:16:13 +0200
> "Michael S. Tsirkin" <mst@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.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.
>>>
>>> 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 <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: Marcel Apfelbaum <marcel@redhat.com>
>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@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.
- Maxime
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 13:29 ` Maxime Coquelin
@ 2016-12-14 13:45 ` Stefan Hajnoczi
0 siblings, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2016-12-14 13:45 UTC (permalink / raw)
To: Maxime Coquelin
Cc: Cornelia Huck, Michael S. Tsirkin, Marcel Apfelbaum,
Dr . David Alan Gilbert, Michael Roth, Stefan Hajnoczi,
qemu-devel
On Wed, Dec 14, 2016 at 1:29 PM, Maxime Coquelin
<maxime.coquelin@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" <mst@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.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.
>>>>
>>>>
>>>> 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 <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: Marcel Apfelbaum <marcel@redhat.com>
>>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@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 <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 12:52 [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines Maxime Coquelin
2016-12-14 13:12 ` Cornelia Huck
@ 2016-12-14 13:13 ` Michael S. Tsirkin
2016-12-14 13:31 ` Marcel Apfelbaum
2016-12-14 15:08 ` Michael Roth
3 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2016-12-14 13:13 UTC (permalink / raw)
To: Maxime Coquelin
Cc: mdroth, stefanha, qemu-devel, cornelia.huck, marcel,
Dr . David Alan Gilbert
On Wed, Dec 14, 2016 at 01:52:37PM +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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
I wouldn't call it modern broken though. Modern works fine.
What this does is allow modern capability in legacy mode.
x-virtio-legacy-with-modern-cap
?
> ---
>
> 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 &&
> + !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),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index b2a996f..1dca223 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 modern_broken;
> OnOffAuto disable_legacy;
> uint32_t class_code;
> uint32_t nvectors;
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 0f06e11..fe11723 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-modern-broken",\
> + .value = "on",\
> },
>
> #define HW_COMPAT_2_6 \
> --
> 2.9.3
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 12:52 [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines Maxime Coquelin
2016-12-14 13:12 ` Cornelia Huck
2016-12-14 13:13 ` Michael S. Tsirkin
@ 2016-12-14 13:31 ` Marcel Apfelbaum
2016-12-14 15:08 ` Michael Roth
3 siblings, 0 replies; 15+ messages in thread
From: Marcel Apfelbaum @ 2016-12-14 13:31 UTC (permalink / raw)
To: Maxime Coquelin, mdroth, stefanha, qemu-devel, mst, cornelia.huck
Cc: Dr . David Alan Gilbert
On 12/14/2016 02:52 PM, 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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@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 &&
> + !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),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index b2a996f..1dca223 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 modern_broken;
> OnOffAuto disable_legacy;
> uint32_t class_code;
> uint32_t nvectors;
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 0f06e11..fe11723 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-modern-broken",\
> + .value = "on",\
> },
>
> #define HW_COMPAT_2_6 \
>
Hi Maxime,
The patch looks good to me, I am neutral regarding the property name.
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 12:52 [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines Maxime Coquelin
` (2 preceding siblings ...)
2016-12-14 13:31 ` Marcel Apfelbaum
@ 2016-12-14 15:08 ` Michael Roth
2016-12-14 15:15 ` Michael S. Tsirkin
3 siblings, 1 reply; 15+ messages in thread
From: Michael Roth @ 2016-12-14 15:08 UTC (permalink / raw)
To: Maxime Coquelin, stefanha, qemu-devel, mst, cornelia.huck, marcel
Cc: Dr . David Alan Gilbert
Quoting Maxime Coquelin (2016-12-14 06:52:37)
> 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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
I can confirm this fixes the original issue I reported. I also did a
number of sanity runs with 2.7/2.8/pc-i440fx-2.{6,7,8} using various
combinations of disable-modern=true/false on hosts with/without virtio-1,
and some tests with pseries machines as well, and everything seems to
work.
Thanks for the quick fix!
> ---
>
> 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 &&
> + !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),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index b2a996f..1dca223 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 modern_broken;
> OnOffAuto disable_legacy;
> uint32_t class_code;
> uint32_t nvectors;
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 0f06e11..fe11723 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-modern-broken",\
> + .value = "on",\
> },
>
> #define HW_COMPAT_2_6 \
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 15:08 ` Michael Roth
@ 2016-12-14 15:15 ` Michael S. Tsirkin
2016-12-14 15:20 ` Maxime Coquelin
2016-12-14 16:02 ` Michael Roth
0 siblings, 2 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2016-12-14 15:15 UTC (permalink / raw)
To: Michael Roth
Cc: Maxime Coquelin, stefanha, qemu-devel, cornelia.huck, marcel,
Dr . David Alan Gilbert
On Wed, Dec 14, 2016 at 09:08:52AM -0600, Michael Roth wrote:
> Quoting Maxime Coquelin (2016-12-14 06:52:37)
> > 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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> I can confirm this fixes the original issue I reported. I also did a
> number of sanity runs with 2.7/2.8/pc-i440fx-2.{6,7,8} using various
> combinations of disable-modern=true/false on hosts with/without virtio-1,
> and some tests with pseries machines as well, and everything seems to
> work.
>
> Thanks for the quick fix!
FYI what I think does not work is a recent kernel on 2.7
machine type and host without virtio 1.
But this is not new.
> > ---
> >
> > 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 &&
> > + !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),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index b2a996f..1dca223 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 modern_broken;
> > OnOffAuto disable_legacy;
> > uint32_t class_code;
> > uint32_t nvectors;
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 0f06e11..fe11723 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-modern-broken",\
> > + .value = "on",\
> > },
> >
> > #define HW_COMPAT_2_6 \
> > --
> > 2.9.3
> >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 15:15 ` Michael S. Tsirkin
@ 2016-12-14 15:20 ` Maxime Coquelin
2016-12-14 16:02 ` Michael Roth
1 sibling, 0 replies; 15+ messages in thread
From: Maxime Coquelin @ 2016-12-14 15:20 UTC (permalink / raw)
To: Michael S. Tsirkin, Michael Roth
Cc: stefanha, qemu-devel, cornelia.huck, marcel,
Dr . David Alan Gilbert
On 12/14/2016 04:15 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 14, 2016 at 09:08:52AM -0600, Michael Roth wrote:
>> Quoting Maxime Coquelin (2016-12-14 06:52:37)
>>> 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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>
>> I can confirm this fixes the original issue I reported. I also did a
>> number of sanity runs with 2.7/2.8/pc-i440fx-2.{6,7,8} using various
>> combinations of disable-modern=true/false on hosts with/without virtio-1,
>> and some tests with pseries machines as well, and everything seems to
>> work.
>>
>> Thanks for the quick fix!
>
> FYI what I think does not work is a recent kernel on 2.7
> machine type and host without virtio 1.
> But this is not new.
I confirm, and as we discussed off-list, I will propose a kernel patch
to improve the situation, even if it will not fix current guests using
recent kernels.
>
>>> ---
>>>
>>> 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 &&
>>> + !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),
>>> DEFINE_PROP_END_OF_LIST(),
>>> };
>>>
>>> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>>> index b2a996f..1dca223 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 modern_broken;
>>> OnOffAuto disable_legacy;
>>> uint32_t class_code;
>>> uint32_t nvectors;
>>> diff --git a/include/hw/compat.h b/include/hw/compat.h
>>> index 0f06e11..fe11723 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-modern-broken",\
>>> + .value = "on",\
>>> },
>>>
>>> #define HW_COMPAT_2_6 \
>>> --
>>> 2.9.3
>>>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 15:15 ` Michael S. Tsirkin
2016-12-14 15:20 ` Maxime Coquelin
@ 2016-12-14 16:02 ` Michael Roth
2016-12-14 16:53 ` Michael Roth
1 sibling, 1 reply; 15+ messages in thread
From: Michael Roth @ 2016-12-14 16:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Maxime Coquelin, stefanha, qemu-devel, cornelia.huck, marcel,
Dr . David Alan Gilbert
Quoting Michael S. Tsirkin (2016-12-14 09:15:38)
> On Wed, Dec 14, 2016 at 09:08:52AM -0600, Michael Roth wrote:
> > Quoting Maxime Coquelin (2016-12-14 06:52:37)
> > > 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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
> > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >
> > I can confirm this fixes the original issue I reported. I also did a
> > number of sanity runs with 2.7/2.8/pc-i440fx-2.{6,7,8} using various
> > combinations of disable-modern=true/false on hosts with/without virtio-1,
> > and some tests with pseries machines as well, and everything seems to
> > work.
> >
> > Thanks for the quick fix!
>
> FYI what I think does not work is a recent kernel on 2.7
> machine type and host without virtio 1.
> But this is not new.
To clarify I was only testing migration compatibility, I assume virtio
on 2.7 machines is still broken for the configuration you mentioned.
The migration tests I ran on the virtio-1 host do cover networking over
a virtio-net device before/after migration with reboots before/after
migration as well though, and the guest in those cases had a 4.8 kernel,
so I think the sanity checks I mentioned also apply for confirming
virtio-net probe is succeeding in the guest.
The non-virtio-1 runs are being done on my local machine and the tests
in that case are a bit more basic and don't involve actively testing
networking. I'll try some manual tests to check this. I guess the main
things to confirm on that front are that after the patch virtio probing:
pc-i440fx-2.6, defaults -> succeeds
pc-i440fx-2.6, disable-modern=true -> succeeds
pc-i440fx-2.6, disable-modern=false -> fails
pc-i440fx-2.7, defaults -> fails
pc-i440fx-2.7, disable-modern=true -> succeeds
pc-i440fx-2.7, disable-modern=false -> fails
pc-i440fx-2.8, defaults -> succeeds
pc-i440fx-2.8, disable-modern=true -> succeeds
pc-i440fx-2.8, disable-modern=false -> succeeds
>
> > > ---
> > >
> > > 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 &&
> > > + !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),
> > > DEFINE_PROP_END_OF_LIST(),
> > > };
> > >
> > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > > index b2a996f..1dca223 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 modern_broken;
> > > OnOffAuto disable_legacy;
> > > uint32_t class_code;
> > > uint32_t nvectors;
> > > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > > index 0f06e11..fe11723 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-modern-broken",\
> > > + .value = "on",\
> > > },
> > >
> > > #define HW_COMPAT_2_6 \
> > > --
> > > 2.9.3
> > >
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 16:02 ` Michael Roth
@ 2016-12-14 16:53 ` Michael Roth
2016-12-14 16:56 ` Michael Roth
0 siblings, 1 reply; 15+ messages in thread
From: Michael Roth @ 2016-12-14 16:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Dr . David Alan Gilbert, marcel, Maxime Coquelin,
stefanha, cornelia.huck
Quoting Michael Roth (2016-12-14 10:02:15)
> Quoting Michael S. Tsirkin (2016-12-14 09:15:38)
> > On Wed, Dec 14, 2016 at 09:08:52AM -0600, Michael Roth wrote:
> > > Quoting Maxime Coquelin (2016-12-14 06:52:37)
> > > > 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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
> > > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > >
> > > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > >
> > > I can confirm this fixes the original issue I reported. I also did a
> > > number of sanity runs with 2.7/2.8/pc-i440fx-2.{6,7,8} using various
> > > combinations of disable-modern=true/false on hosts with/without virtio-1,
> > > and some tests with pseries machines as well, and everything seems to
> > > work.
> > >
> > > Thanks for the quick fix!
> >
> > FYI what I think does not work is a recent kernel on 2.7
> > machine type and host without virtio 1.
> > But this is not new.
>
> To clarify I was only testing migration compatibility, I assume virtio
> on 2.7 machines is still broken for the configuration you mentioned.
>
> The migration tests I ran on the virtio-1 host do cover networking over
> a virtio-net device before/after migration with reboots before/after
> migration as well though, and the guest in those cases had a 4.8 kernel,
> so I think the sanity checks I mentioned also apply for confirming
> virtio-net probe is succeeding in the guest.
>
> The non-virtio-1 runs are being done on my local machine and the tests
> in that case are a bit more basic and don't involve actively testing
> networking. I'll try some manual tests to check this. I guess the main
> things to confirm on that front are that after the patch virtio probing:
>
> pc-i440fx-2.6, defaults -> succeeds
> pc-i440fx-2.6, disable-modern=true -> succeeds
> pc-i440fx-2.6, disable-modern=false -> fails
>
> pc-i440fx-2.7, defaults -> fails
> pc-i440fx-2.7, disable-modern=true -> succeeds
> pc-i440fx-2.7, disable-modern=false -> fails
>
> pc-i440fx-2.8, defaults -> succeeds
> pc-i440fx-2.8, disable-modern=true -> succeeds
> pc-i440fx-2.8, disable-modern=false -> succeeds
I wasn't able to test disable-modern with pc-i440fx-2.6 due to the issue
being fixes by proposed patch "machine: Convert abstract typename on
compat_props to subclass names", but I think the rest of the cases align
with expectations:
2.6, defaults: succeeds
2.7, defaults: fails
2.7, disable-modern=true: succeeds
2.7, disable-modern=false: fails
2.8, defaults: succeeds
2.7, disable-modern=true: succeeds
2.7, disable-modern=false: succeeds
>
> >
> > > > ---
> > > >
> > > > 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 &&
> > > > + !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),
> > > > DEFINE_PROP_END_OF_LIST(),
> > > > };
> > > >
> > > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > > > index b2a996f..1dca223 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 modern_broken;
> > > > OnOffAuto disable_legacy;
> > > > uint32_t class_code;
> > > > uint32_t nvectors;
> > > > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > > > index 0f06e11..fe11723 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-modern-broken",\
> > > > + .value = "on",\
> > > > },
> > > >
> > > > #define HW_COMPAT_2_6 \
> > > > --
> > > > 2.9.3
> > > >
> >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 16:53 ` Michael Roth
@ 2016-12-14 16:56 ` Michael Roth
2016-12-14 16:57 ` Maxime Coquelin
0 siblings, 1 reply; 15+ messages in thread
From: Michael Roth @ 2016-12-14 16:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Dr . David Alan Gilbert, marcel, Maxime Coquelin,
stefanha, cornelia.huck
Quoting Michael Roth (2016-12-14 10:53:41)
> Quoting Michael Roth (2016-12-14 10:02:15)
> > Quoting Michael S. Tsirkin (2016-12-14 09:15:38)
> > > On Wed, Dec 14, 2016 at 09:08:52AM -0600, Michael Roth wrote:
> > > > Quoting Maxime Coquelin (2016-12-14 06:52:37)
> > > > > 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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
> > > > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > >
> > > > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > >
> > > > I can confirm this fixes the original issue I reported. I also did a
> > > > number of sanity runs with 2.7/2.8/pc-i440fx-2.{6,7,8} using various
> > > > combinations of disable-modern=true/false on hosts with/without virtio-1,
> > > > and some tests with pseries machines as well, and everything seems to
> > > > work.
> > > >
> > > > Thanks for the quick fix!
> > >
> > > FYI what I think does not work is a recent kernel on 2.7
> > > machine type and host without virtio 1.
> > > But this is not new.
> >
> > To clarify I was only testing migration compatibility, I assume virtio
> > on 2.7 machines is still broken for the configuration you mentioned.
> >
> > The migration tests I ran on the virtio-1 host do cover networking over
> > a virtio-net device before/after migration with reboots before/after
> > migration as well though, and the guest in those cases had a 4.8 kernel,
> > so I think the sanity checks I mentioned also apply for confirming
> > virtio-net probe is succeeding in the guest.
> >
> > The non-virtio-1 runs are being done on my local machine and the tests
> > in that case are a bit more basic and don't involve actively testing
> > networking. I'll try some manual tests to check this. I guess the main
> > things to confirm on that front are that after the patch virtio probing:
> >
> > pc-i440fx-2.6, defaults -> succeeds
> > pc-i440fx-2.6, disable-modern=true -> succeeds
> > pc-i440fx-2.6, disable-modern=false -> fails
> >
> > pc-i440fx-2.7, defaults -> fails
> > pc-i440fx-2.7, disable-modern=true -> succeeds
> > pc-i440fx-2.7, disable-modern=false -> fails
> >
> > pc-i440fx-2.8, defaults -> succeeds
> > pc-i440fx-2.8, disable-modern=true -> succeeds
> > pc-i440fx-2.8, disable-modern=false -> succeeds
>
> I wasn't able to test disable-modern with pc-i440fx-2.6 due to the issue
> being fixes by proposed patch "machine: Convert abstract typename on
> compat_props to subclass names", but I think the rest of the cases align
> with expectations:
>
> 2.6, defaults: succeeds
>
> 2.7, defaults: fails
> 2.7, disable-modern=true: succeeds
> 2.7, disable-modern=false: fails
>
> 2.8, defaults: succeeds
> 2.7, disable-modern=true: succeeds
> 2.7, disable-modern=false: succeeds
Typo on the latter 2, these were for pc-i440fx-2.8 as well.
>
> >
> > >
> > > > > ---
> > > > >
> > > > > 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 &&
> > > > > + !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),
> > > > > DEFINE_PROP_END_OF_LIST(),
> > > > > };
> > > > >
> > > > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > > > > index b2a996f..1dca223 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 modern_broken;
> > > > > OnOffAuto disable_legacy;
> > > > > uint32_t class_code;
> > > > > uint32_t nvectors;
> > > > > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > > > > index 0f06e11..fe11723 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-modern-broken",\
> > > > > + .value = "on",\
> > > > > },
> > > > >
> > > > > #define HW_COMPAT_2_6 \
> > > > > --
> > > > > 2.9.3
> > > > >
> > >
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-pci: Fix cross-version migration with older machines
2016-12-14 16:56 ` Michael Roth
@ 2016-12-14 16:57 ` Maxime Coquelin
0 siblings, 0 replies; 15+ messages in thread
From: Maxime Coquelin @ 2016-12-14 16:57 UTC (permalink / raw)
To: Michael Roth, Michael S. Tsirkin
Cc: qemu-devel, Dr . David Alan Gilbert, marcel, stefanha,
cornelia.huck
On 12/14/2016 05:56 PM, Michael Roth wrote:
> Quoting Michael Roth (2016-12-14 10:53:41)
>> > Quoting Michael Roth (2016-12-14 10:02:15)
>>> > > Quoting Michael S. Tsirkin (2016-12-14 09:15:38)
>>>> > > > On Wed, Dec 14, 2016 at 09:08:52AM -0600, Michael Roth wrote:
>>>>> > > > > Quoting Maxime Coquelin (2016-12-14 06:52:37)
>>>>>> > > > > > 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-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 <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: Marcel Apfelbaum <marcel@redhat.com>
>>>>>> > > > > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>> > > > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> > > > >
>>>>> > > > > Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>>>> > > > >
>>>>> > > > > I can confirm this fixes the original issue I reported. I also did a
>>>>> > > > > number of sanity runs with 2.7/2.8/pc-i440fx-2.{6,7,8} using various
>>>>> > > > > combinations of disable-modern=true/false on hosts with/without virtio-1,
>>>>> > > > > and some tests with pseries machines as well, and everything seems to
>>>>> > > > > work.
>>>>> > > > >
>>>>> > > > > Thanks for the quick fix!
>>>> > > >
>>>> > > > FYI what I think does not work is a recent kernel on 2.7
>>>> > > > machine type and host without virtio 1.
>>>> > > > But this is not new.
>>> > >
>>> > > To clarify I was only testing migration compatibility, I assume virtio
>>> > > on 2.7 machines is still broken for the configuration you mentioned.
>>> > >
>>> > > The migration tests I ran on the virtio-1 host do cover networking over
>>> > > a virtio-net device before/after migration with reboots before/after
>>> > > migration as well though, and the guest in those cases had a 4.8 kernel,
>>> > > so I think the sanity checks I mentioned also apply for confirming
>>> > > virtio-net probe is succeeding in the guest.
>>> > >
>>> > > The non-virtio-1 runs are being done on my local machine and the tests
>>> > > in that case are a bit more basic and don't involve actively testing
>>> > > networking. I'll try some manual tests to check this. I guess the main
>>> > > things to confirm on that front are that after the patch virtio probing:
>>> > >
>>> > > pc-i440fx-2.6, defaults -> succeeds
>>> > > pc-i440fx-2.6, disable-modern=true -> succeeds
>>> > > pc-i440fx-2.6, disable-modern=false -> fails
>>> > >
>>> > > pc-i440fx-2.7, defaults -> fails
>>> > > pc-i440fx-2.7, disable-modern=true -> succeeds
>>> > > pc-i440fx-2.7, disable-modern=false -> fails
>>> > >
>>> > > pc-i440fx-2.8, defaults -> succeeds
>>> > > pc-i440fx-2.8, disable-modern=true -> succeeds
>>> > > pc-i440fx-2.8, disable-modern=false -> succeeds
>> >
>> > I wasn't able to test disable-modern with pc-i440fx-2.6 due to the issue
>> > being fixes by proposed patch "machine: Convert abstract typename on
>> > compat_props to subclass names", but I think the rest of the cases align
>> > with expectations:
>> >
>> > 2.6, defaults: succeeds
>> >
>> > 2.7, defaults: fails
>> > 2.7, disable-modern=true: succeeds
>> > 2.7, disable-modern=false: fails
>> >
>> > 2.8, defaults: succeeds
>> > 2.7, disable-modern=true: succeeds
>> > 2.7, disable-modern=false: succeeds
> Typo on the latter 2, these were for pc-i440fx-2.8 as well.
Thanks Michael, I confirm this is in line with expectations.
- Maxime
^ permalink raw reply [flat|nested] 15+ messages in thread