[15/18] qemu: Support input model=virtio-{non-}transitional

Message ID 39eff8fd2aef05ce62ce1650b6717c765f6e4472.1547746868.git.crobinso@redhat.com
State New
Headers show
Series
  • qemu: virtio-{non-}transitional support
Related show

Commit Message

Cole Robinson Jan. 17, 2019, 5:52 p.m.
Add <input> model handling for virtio transitional devices. Ex:

  <input type='passthrough' bus='virtio' model='virtio-transitional'>
    ...
  </input>

* "virtio-transitional" maps to qemu "virtio-input-host-pci-transitional"
* "virtio-non-transitional" maps to qemu "virtio-input-host-non-transitional"

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/qemu/qemu_capabilities.c                         |  4 ++++
 src/qemu/qemu_capabilities.h                         |  2 ++
 src/qemu/qemu_command.c                              | 12 ++++++++++--
 src/qemu/qemu_domain_address.c                       |  9 +++++++++
 tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml     |  2 ++
 .../virtio-non-transitional.x86_64-3.1.0.args        |  4 ++--
 .../virtio-non-transitional.x86_64-latest.args       |  4 ++--
 .../virtio-transitional.x86_64-3.1.0.args            |  7 +++----
 .../virtio-transitional.x86_64-latest.args           |  7 +++----
 tests/qemuxml2xmloutdata/virtio-transitional.xml     |  9 ++-------
 10 files changed, 39 insertions(+), 21 deletions(-)

-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Andrea Bolognani Jan. 21, 2019, 4:20 p.m. | #1
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
[...]
> @@ -1142,6 +1144,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {

>      {"virtio-balloon-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BALLOON_NON_TRANSITIONAL},

>      {"vhost-vsock-pci-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL},

>      {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},

> +    {"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},

> +    {"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},

>  };


Same comment as always for capabilities.

[...]
> @@ -501,9 +501,14 @@ qemuBuildVirtioTransitional(virBufferPtr buf,

>              tmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL;

>              ntmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL;

>              break;


Empty line here.

[...]
> @@ -917,6 +917,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,

>      case VIR_DOMAIN_DEVICE_INPUT:

>          switch ((virDomainInputBus) dev->data.input->bus) {

>          case VIR_DOMAIN_INPUT_BUS_VIRTIO:

> +            switch ((virDomainInputModel) dev->data.input->model) {

> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:

> +                return pciFlags;

> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO:

> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:

> +            case VIR_DOMAIN_INPUT_MODEL_DEFAULT:

> +            case VIR_DOMAIN_INPUT_MODEL_LAST:

> +                break;

> +            }

>              return virtioFlags;


VIR_DOMAIN_INPUT_MODEL_DEFAULT and VIR_DOMAIN_INPUT_MODEL_LAST
should result in 0 rather than virtioFlags being returned.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 22, 2019, 5:32 p.m. | #2
On 01/21/2019 11:20 AM, Andrea Bolognani wrote:
> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

> [...]

>> @@ -1142,6 +1144,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {

>>      {"virtio-balloon-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BALLOON_NON_TRANSITIONAL},

>>      {"vhost-vsock-pci-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL},

>>      {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},

>> +    {"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},

>> +    {"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},

>>  };

> 

> Same comment as always for capabilities.

> 

> [...]

>> @@ -501,9 +501,14 @@ qemuBuildVirtioTransitional(virBufferPtr buf,

>>              tmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL;

>>              ntmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL;

>>              break;

> 

> Empty line here.

> 

> [...]

>> @@ -917,6 +917,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,

>>      case VIR_DOMAIN_DEVICE_INPUT:

>>          switch ((virDomainInputBus) dev->data.input->bus) {

>>          case VIR_DOMAIN_INPUT_BUS_VIRTIO:

>> +            switch ((virDomainInputModel) dev->data.input->model) {

>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:

>> +                return pciFlags;

>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO:

>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:

>> +            case VIR_DOMAIN_INPUT_MODEL_DEFAULT:

>> +            case VIR_DOMAIN_INPUT_MODEL_LAST:

>> +                break;

>> +            }

>>              return virtioFlags;

> 

> VIR_DOMAIN_INPUT_MODEL_DEFAULT and VIR_DOMAIN_INPUT_MODEL_LAST

> should result in 0 rather than virtioFlags being returned.

> 


Oops good catch

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 23, 2019, 6:30 p.m. | #3
On 01/22/2019 12:32 PM, Cole Robinson wrote:
> On 01/21/2019 11:20 AM, Andrea Bolognani wrote:

>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

>> [...]

>>> @@ -1142,6 +1144,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {

>>>      {"virtio-balloon-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BALLOON_NON_TRANSITIONAL},

>>>      {"vhost-vsock-pci-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL},

>>>      {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},

>>> +    {"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},

>>> +    {"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},

>>>  };

>>

>> Same comment as always for capabilities.

>>

>> [...]

>>> @@ -501,9 +501,14 @@ qemuBuildVirtioTransitional(virBufferPtr buf,

>>>              tmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL;

>>>              ntmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL;

>>>              break;

>>

>> Empty line here.

>>

>> [...]

>>> @@ -917,6 +917,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,

>>>      case VIR_DOMAIN_DEVICE_INPUT:

>>>          switch ((virDomainInputBus) dev->data.input->bus) {

>>>          case VIR_DOMAIN_INPUT_BUS_VIRTIO:

>>> +            switch ((virDomainInputModel) dev->data.input->model) {

>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:

>>> +                return pciFlags;

>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO:

>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:

>>> +            case VIR_DOMAIN_INPUT_MODEL_DEFAULT:

>>> +            case VIR_DOMAIN_INPUT_MODEL_LAST:

>>> +                break;

>>> +            }

>>>              return virtioFlags;

>>

>> VIR_DOMAIN_INPUT_MODEL_DEFAULT and VIR_DOMAIN_INPUT_MODEL_LAST

>> should result in 0 rather than virtioFlags being returned.

>>

> 

> Oops good catch

> 


Actually this missed a case: at least DEFAULT (meaning no model=
specified) should return virtioFlags, since this block is already
conditional on bus='virtio'. I'll have DEFAULT return virtioFlags and
LAST return 0;

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani Jan. 24, 2019, 2:48 p.m. | #4
On Wed, 2019-01-23 at 13:30 -0500, Cole Robinson wrote:
> On 01/22/2019 12:32 PM, Cole Robinson wrote:

> > On 01/21/2019 11:20 AM, Andrea Bolognani wrote:

> > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

> > > > @@ -917,6 +917,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,

> > > >      case VIR_DOMAIN_DEVICE_INPUT:

> > > >          switch ((virDomainInputBus) dev->data.input->bus) {

> > > >          case VIR_DOMAIN_INPUT_BUS_VIRTIO:

> > > > +            switch ((virDomainInputModel) dev->data.input->model) {

> > > > +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:

> > > > +                return pciFlags;

> > > > +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO:

> > > > +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:

> > > > +            case VIR_DOMAIN_INPUT_MODEL_DEFAULT:

> > > > +            case VIR_DOMAIN_INPUT_MODEL_LAST:

> > > > +                break;

> > > > +            }

> > > >              return virtioFlags;

> > > 

> > > VIR_DOMAIN_INPUT_MODEL_DEFAULT and VIR_DOMAIN_INPUT_MODEL_LAST

> > > should result in 0 rather than virtioFlags being returned.

> > 

> > Oops good catch

> 

> Actually this missed a case: at least DEFAULT (meaning no model=

> specified) should return virtioFlags, since this block is already

> conditional on bus='virtio'. I'll have DEFAULT return virtioFlags and

> LAST return 0;


Now that you mention it, for devices such as <input> and <disk>,
where we're introducing the model attribute just now, we should make
sure that we expand the default correctly, such that eg.

  <disk type='file' device='disk'>
    <target dev='vda' bus='virtio'/>
  </disk>

is formatted back as

  <disk type='file' device='disk' model='virtio'>
    <target dev='vda' bus='virtio'/>
  </disk>

We might have to be careful when formatting the XML for migration
and scrub model='virtio' in that case in order not to break
backwards migration, as we already do for plenty of other cases.

Once that's done, then you'll be guaranteed the model above is
never going to be DEFAULT.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 24, 2019, 5:05 p.m. | #5
On 01/24/2019 09:48 AM, Andrea Bolognani wrote:
> On Wed, 2019-01-23 at 13:30 -0500, Cole Robinson wrote:

>> On 01/22/2019 12:32 PM, Cole Robinson wrote:

>>> On 01/21/2019 11:20 AM, Andrea Bolognani wrote:

>>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

>>>>> @@ -917,6 +917,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,

>>>>>      case VIR_DOMAIN_DEVICE_INPUT:

>>>>>          switch ((virDomainInputBus) dev->data.input->bus) {

>>>>>          case VIR_DOMAIN_INPUT_BUS_VIRTIO:

>>>>> +            switch ((virDomainInputModel) dev->data.input->model) {

>>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:

>>>>> +                return pciFlags;

>>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO:

>>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:

>>>>> +            case VIR_DOMAIN_INPUT_MODEL_DEFAULT:

>>>>> +            case VIR_DOMAIN_INPUT_MODEL_LAST:

>>>>> +                break;

>>>>> +            }

>>>>>              return virtioFlags;

>>>>

>>>> VIR_DOMAIN_INPUT_MODEL_DEFAULT and VIR_DOMAIN_INPUT_MODEL_LAST

>>>> should result in 0 rather than virtioFlags being returned.

>>>

>>> Oops good catch

>>

>> Actually this missed a case: at least DEFAULT (meaning no model=

>> specified) should return virtioFlags, since this block is already

>> conditional on bus='virtio'. I'll have DEFAULT return virtioFlags and

>> LAST return 0;

> 

> Now that you mention it, for devices such as <input> and <disk>,

> where we're introducing the model attribute just now, we should make

> sure that we expand the default correctly, such that eg.

> 

>   <disk type='file' device='disk'>

>     <target dev='vda' bus='virtio'/>

>   </disk>

> 

> is formatted back as

> 

>   <disk type='file' device='disk' model='virtio'>

>     <target dev='vda' bus='virtio'/>

>   </disk>

> 

> We might have to be careful when formatting the XML for migration

> and scrub model='virtio' in that case in order not to break

> backwards migration, as we already do for plenty of other cases.

> 

> Once that's done, then you'll be guaranteed the model above is

> never going to be DEFAULT.

> 


IMO extending the XML we generate by default should only be done for
very good reasons, the migration compat issue is not a trivial extension
and comes with issues of its own.

For example, say we turn model=DEFAULT into model=VIRTIO in the qemu
driver. In generic domain format code, we would do:

if (!MIGRATABLE || disk->model != VIRTIO)
    <format disk->model in the XML>

But the MIGRATABLE checking is very crude. It doesn't consider version
or capabilities of the remote libvirt. We could be migrating to an
identical qemu and libvirt version, but we still drop the model=VIRTIO
from the XML. Basically for the indefinite future, disk->model=VIRTIO in
the XML is never going to be accounted for in migration. This can cause
theoretical future issues if non-qemu drivers (or other qemu configs)
support model=VIRTIO but it's not their default. For those non-qemu
drivers, if disk->model=VIRTIO is present in the XML, the user
explicitly requested it, but generic formatting code will never send the
value during migration.

Let's say we skip the MIGRATABLE check. In fact for most of the cases in
this patch series (except maybe for <controller> devs), this would work
fine. If we are migrating to same or newer libvirt+qemu, all is good. If
we migrate to older libvirt, it won't know to check disk->model at all
so won't complain, maybe migration would fail due to qemu compat issues
but that's unavoidable.

However, the NEXT time we make a similar change, let's say filling in
disk->model=FOO for bus=ide, or more likely, input->model=QEMUTABLET for
input->bus=usb + input->type=tablet. Now we have to blacklist
model=QEMUTABLET from the migratable XML for the indefinite future,
because older libvirt _may_ know enough to parse input->model, but
doesn't have QEMUTABLET in the enum, and parsing fails. Okay, this is
what the MIGRATABLE flag is all about. But it's exposing us to the issue
with other drivers mentioned above.

And this model=QEMUTABLET MIGRATABLE issue is theoretical but it's an
exact case that has happened before, with controller models at least.
Basically once you open that door of adding new default output to the
XML for common cases, you can't ever close it again for that enum property.

Plus there's other downsides for outputting new properties by default:

* massive test suite churn: every single virtio disk in the XML2XML
tests will now spit out model='virtio', plus all the other devices. This
makes backporting suck

* libvirt downgrade issues. Previous times we added new output, like
input type=keyboard, there were lots of complaints like: I upgraded
libvirt via virt-preview to see if it fixed a bug, type=keyboard was
added by default, I downgraded libvirt back to regular distro version,
my VM disappeared, because the XML parser choked on unknown
type=keyboard. Again in this particular case it likely won't cause those
issues because model= is new everywhere, but the pattern basically can't
be extended forward beyond this one instance.

Here's the benefits I see of outputing model=virtio by default

* Reports more accurate XML. Minor IMO.

* Better chance of maintaining qemu compat across libvirt/qemu upgrades.
Encoding model=virtio means we will always try that. Hypothetically 5
years from now libvirt starts defaulting to
model=virtio-non-transitional, you might upgrade libvirt and your
windows virtio VM doesn't boot. Yeah that's not nice, but it's fixable
(user sets model=virtio in the XML). Same reasoning basically applies to
migration compat as well

I don't think the benefits outweigh the downsides. Of course I may be
missing plenty of things so please correct me if I'm wrong

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani Jan. 28, 2019, 4:17 p.m. | #6
On Thu, 2019-01-24 at 12:05 -0500, Cole Robinson wrote:
> On 01/24/2019 09:48 AM, Andrea Bolognani wrote:

> > Now that you mention it, for devices such as <input> and <disk>,

> > where we're introducing the model attribute just now, we should make

> > sure that we expand the default correctly, such that eg.

> > 

> >   <disk type='file' device='disk'>

> >     <target dev='vda' bus='virtio'/>

> >   </disk>

> > 

> > is formatted back as

> > 

> >   <disk type='file' device='disk' model='virtio'>

> >     <target dev='vda' bus='virtio'/>

> >   </disk>

> > 

> > We might have to be careful when formatting the XML for migration

> > and scrub model='virtio' in that case in order not to break

> > backwards migration, as we already do for plenty of other cases.

> > 

> > Once that's done, then you'll be guaranteed the model above is

> > never going to be DEFAULT.

> 

> IMO extending the XML we generate by default should only be done for

> very good reasons, the migration compat issue is not a trivial extension

> and comes with issues of its own.

> 

> For example, say we turn model=DEFAULT into model=VIRTIO in the qemu

> driver. In generic domain format code, we would do:

> 

> if (!MIGRATABLE || disk->model != VIRTIO)

>     <format disk->model in the XML>

> 

> But the MIGRATABLE checking is very crude. It doesn't consider version

> or capabilities of the remote libvirt. We could be migrating to an

> identical qemu and libvirt version, but we still drop the model=VIRTIO

> from the XML. Basically for the indefinite future, disk->model=VIRTIO in

> the XML is never going to be accounted for in migration. This can cause

> theoretical future issues if non-qemu drivers (or other qemu configs)

> support model=VIRTIO but it's not their default. For those non-qemu

> drivers, if disk->model=VIRTIO is present in the XML, the user

> explicitly requested it, but generic formatting code will never send the

> value during migration.

> 

> Let's say we skip the MIGRATABLE check. In fact for most of the cases in

> this patch series (except maybe for <controller> devs), this would work

> fine. If we are migrating to same or newer libvirt+qemu, all is good. If

> we migrate to older libvirt, it won't know to check disk->model at all

> so won't complain, maybe migration would fail due to qemu compat issues

> but that's unavoidable.

> 

> However, the NEXT time we make a similar change, let's say filling in

> disk->model=FOO for bus=ide, or more likely, input->model=QEMUTABLET for

> input->bus=usb + input->type=tablet. Now we have to blacklist

> model=QEMUTABLET from the migratable XML for the indefinite future,

> because older libvirt _may_ know enough to parse input->model, but

> doesn't have QEMUTABLET in the enum, and parsing fails. Okay, this is

> what the MIGRATABLE flag is all about. But it's exposing us to the issue

> with other drivers mentioned above.


We have qemuDomainDefFormatBufInternal(), where at least some of
the MIGRATABLE tweaks (PCI and USB controllers, serial devices) are
performed. I haven't looked into the call graph but I think it's
fair to assume that what's safe for pci-root is safe for pretty
much anything else :) So that should rule out at least the concern
with messing with other drivers.

> And this model=QEMUTABLET MIGRATABLE issue is theoretical but it's an

> exact case that has happened before, with controller models at least.

> Basically once you open that door of adding new default output to the

> XML for common cases, you can't ever close it again for that enum property.

> 

> Plus there's other downsides for outputting new properties by default:

> 

> * massive test suite churn: every single virtio disk in the XML2XML

> tests will now spit out model='virtio', plus all the other devices. This

> makes backporting suck


We cause test suite churn all the time, and while it's true that it
makes backporting more annoying most of the time it's just a matter
of using REGENERATE_OUTPUT=1 and notifying the reviewer.

> * libvirt downgrade issues. Previous times we added new output, like

> input type=keyboard, there were lots of complaints like: I upgraded

> libvirt via virt-preview to see if it fixed a bug, type=keyboard was

> added by default, I downgraded libvirt back to regular distro version,

> my VM disappeared, because the XML parser choked on unknown

> type=keyboard. Again in this particular case it likely won't cause those

> issues because model= is new everywhere, but the pattern basically can't

> be extended forward beyond this one instance.


People who install from virt-preview are going to be either
developers, who should have no problem dealing with the situation,
or users that are being guided through the analysis process in the
first place, so they won't be on their own when guests disappear.

> Here's the benefits I see of outputing model=virtio by default

> 

> * Reports more accurate XML. Minor IMO.


I remember various occasions in which not formatting a value caused
grief down the line, but I can't recall a single time when the
opposite was true. So maybe I'm focusing on this more than I should,
but past experiences have certainly colored my perception :)

> * Better chance of maintaining qemu compat across libvirt/qemu upgrades.

> Encoding model=virtio means we will always try that. Hypothetically 5

> years from now libvirt starts defaulting to

> model=virtio-non-transitional, you might upgrade libvirt and your

> windows virtio VM doesn't boot. Yeah that's not nice, but it's fixable

> (user sets model=virtio in the XML). Same reasoning basically applies to

> migration compat as well


If we don't format the value from day one, then we *cannot ever*
change the default. But I guess that ship has sailed because the
XML missing model='...' might come from an old libvirt version, so
we can't have that mean anything but model='virtio'.

> I don't think the benefits outweigh the downsides. Of course I may be

> missing plenty of things so please correct me if I'm wrong


I still think we should format the model, but let Dan (now CC'd)
weigh in as well :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrangé Feb. 6, 2019, 4:15 p.m. | #7
On Mon, Jan 28, 2019 at 05:17:38PM +0100, Andrea Bolognani wrote:
> On Thu, 2019-01-24 at 12:05 -0500, Cole Robinson wrote:


> > * libvirt downgrade issues. Previous times we added new output, like

> > input type=keyboard, there were lots of complaints like: I upgraded

> > libvirt via virt-preview to see if it fixed a bug, type=keyboard was

> > added by default, I downgraded libvirt back to regular distro version,

> > my VM disappeared, because the XML parser choked on unknown

> > type=keyboard. Again in this particular case it likely won't cause those

> > issues because model= is new everywhere, but the pattern basically can't

> > be extended forward beyond this one instance.

> 

> People who install from virt-preview are going to be either

> developers, who should have no problem dealing with the situation,

> or users that are being guided through the analysis process in the

> first place, so they won't be on their own when guests disappear.


NB, downgrading of libvirt is explicitly unsupported. You might
get lucky and have it work between some versions, but this is
not something we ever care about guaranteeing.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrangé Feb. 6, 2019, 5:03 p.m. | #8
On Thu, Jan 24, 2019 at 12:05:11PM -0500, Cole Robinson wrote:
> On 01/24/2019 09:48 AM, Andrea Bolognani wrote:

> > On Wed, 2019-01-23 at 13:30 -0500, Cole Robinson wrote:

> >> On 01/22/2019 12:32 PM, Cole Robinson wrote:

> >>> On 01/21/2019 11:20 AM, Andrea Bolognani wrote:

> >>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

> >>>>> @@ -917,6 +917,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,

> >>>>>      case VIR_DOMAIN_DEVICE_INPUT:

> >>>>>          switch ((virDomainInputBus) dev->data.input->bus) {

> >>>>>          case VIR_DOMAIN_INPUT_BUS_VIRTIO:

> >>>>> +            switch ((virDomainInputModel) dev->data.input->model) {

> >>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:

> >>>>> +                return pciFlags;

> >>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO:

> >>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:

> >>>>> +            case VIR_DOMAIN_INPUT_MODEL_DEFAULT:

> >>>>> +            case VIR_DOMAIN_INPUT_MODEL_LAST:

> >>>>> +                break;

> >>>>> +            }

> >>>>>              return virtioFlags;

> >>>>

> >>>> VIR_DOMAIN_INPUT_MODEL_DEFAULT and VIR_DOMAIN_INPUT_MODEL_LAST

> >>>> should result in 0 rather than virtioFlags being returned.

> >>>

> >>> Oops good catch

> >>

> >> Actually this missed a case: at least DEFAULT (meaning no model=

> >> specified) should return virtioFlags, since this block is already

> >> conditional on bus='virtio'. I'll have DEFAULT return virtioFlags and

> >> LAST return 0;

> > 

> > Now that you mention it, for devices such as <input> and <disk>,

> > where we're introducing the model attribute just now, we should make

> > sure that we expand the default correctly, such that eg.

> > 

> >   <disk type='file' device='disk'>

> >     <target dev='vda' bus='virtio'/>

> >   </disk>

> > 

> > is formatted back as

> > 

> >   <disk type='file' device='disk' model='virtio'>

> >     <target dev='vda' bus='virtio'/>

> >   </disk>

> > 

> > We might have to be careful when formatting the XML for migration

> > and scrub model='virtio' in that case in order not to break

> > backwards migration, as we already do for plenty of other cases.

> > 

> > Once that's done, then you'll be guaranteed the model above is

> > never going to be DEFAULT.

> > 

> 

> IMO extending the XML we generate by default should only be done for

> very good reasons, the migration compat issue is not a trivial extension

> and comes with issues of its own.

> 

> For example, say we turn model=DEFAULT into model=VIRTIO in the qemu

> driver. In generic domain format code, we would do:

> 

> if (!MIGRATABLE || disk->model != VIRTIO)

>     <format disk->model in the XML>

> 

> But the MIGRATABLE checking is very crude. It doesn't consider version

> or capabilities of the remote libvirt. We could be migrating to an

> identical qemu and libvirt version, but we still drop the model=VIRTIO

> from the XML. Basically for the indefinite future, disk->model=VIRTIO in

> the XML is never going to be accounted for in migration. This can cause

> theoretical future issues if non-qemu drivers (or other qemu configs)

> support model=VIRTIO but it's not their default. For those non-qemu

> drivers, if disk->model=VIRTIO is present in the XML, the user

> explicitly requested it, but generic formatting code will never send the

> value during migration.

> 

> Let's say we skip the MIGRATABLE check. In fact for most of the cases in

> this patch series (except maybe for <controller> devs), this would work

> fine. If we are migrating to same or newer libvirt+qemu, all is good. If

> we migrate to older libvirt, it won't know to check disk->model at all

> so won't complain, maybe migration would fail due to qemu compat issues

> but that's unavoidable.

> 

> However, the NEXT time we make a similar change, let's say filling in

> disk->model=FOO for bus=ide, or more likely, input->model=QEMUTABLET for

> input->bus=usb + input->type=tablet. Now we have to blacklist

> model=QEMUTABLET from the migratable XML for the indefinite future,

> because older libvirt _may_ know enough to parse input->model, but

> doesn't have QEMUTABLET in the enum, and parsing fails. Okay, this is

> what the MIGRATABLE flag is all about. But it's exposing us to the issue

> with other drivers mentioned above.

> 

> And this model=QEMUTABLET MIGRATABLE issue is theoretical but it's an

> exact case that has happened before, with controller models at least.

> Basically once you open that door of adding new default output to the

> XML for common cases, you can't ever close it again for that enum property.

> 

> Plus there's other downsides for outputting new properties by default:

> 

> * massive test suite churn: every single virtio disk in the XML2XML

> tests will now spit out model='virtio', plus all the other devices. This

> makes backporting suck

> 

> * libvirt downgrade issues. Previous times we added new output, like

> input type=keyboard, there were lots of complaints like: I upgraded

> libvirt via virt-preview to see if it fixed a bug, type=keyboard was

> added by default, I downgraded libvirt back to regular distro version,

> my VM disappeared, because the XML parser choked on unknown

> type=keyboard. Again in this particular case it likely won't cause those

> issues because model= is new everywhere, but the pattern basically can't

> be extended forward beyond this one instance.

> 

> Here's the benefits I see of outputing model=virtio by default

> 

> * Reports more accurate XML. Minor IMO.

> 

> * Better chance of maintaining qemu compat across libvirt/qemu upgrades.

> Encoding model=virtio means we will always try that. Hypothetically 5

> years from now libvirt starts defaulting to

> model=virtio-non-transitional, you might upgrade libvirt and your

> windows virtio VM doesn't boot. Yeah that's not nice, but it's fixable

> (user sets model=virtio in the XML). Same reasoning basically applies to

> migration compat as well


Realistically we can never change the defaults for anything in libvirt,
it has to be done at a layer above.

Previously when we've added elements to the XML that were not previously
present, we have done this to capture some aspect of the guest ABI that
was not previously recorded by libvirt. I see value in that.

I don't think that is actually the case this time though. The model=virtio
addition is just duplcating what is already perfectly well expressed in
the existing  bus=virtio attribute. There's no aspect of the guest ABI
that was missing that we've added here.

Only model=virtio-transitional / model=virtio-non-transitional add new
information wrt the ABI. model=virtio is merely a redundant convenience
we've added for sake of symmetry.

So on this basis, I think there's not a compelling reason to output it
in the XML if the user didn't explicitly set it themselves.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Feb. 6, 2019, 5:54 p.m. | #9
On 2/6/19 12:03 PM, Daniel P. Berrangé wrote:
> On Thu, Jan 24, 2019 at 12:05:11PM -0500, Cole Robinson wrote:
>> On 01/24/2019 09:48 AM, Andrea Bolognani wrote:
>>> On Wed, 2019-01-23 at 13:30 -0500, Cole Robinson wrote:
>>>> On 01/22/2019 12:32 PM, Cole Robinson wrote:
>>>>> On 01/21/2019 11:20 AM, Andrea Bolognani wrote:
>>>>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>>>>>>> @@ -917,6 +917,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>>>>>>>      case VIR_DOMAIN_DEVICE_INPUT:
>>>>>>>          switch ((virDomainInputBus) dev->data.input->bus) {
>>>>>>>          case VIR_DOMAIN_INPUT_BUS_VIRTIO:
>>>>>>> +            switch ((virDomainInputModel) dev->data.input->model) {
>>>>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:
>>>>>>> +                return pciFlags;
>>>>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO:
>>>>>>> +            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:
>>>>>>> +            case VIR_DOMAIN_INPUT_MODEL_DEFAULT:
>>>>>>> +            case VIR_DOMAIN_INPUT_MODEL_LAST:
>>>>>>> +                break;
>>>>>>> +            }
>>>>>>>              return virtioFlags;
>>>>>>
>>>>>> VIR_DOMAIN_INPUT_MODEL_DEFAULT and VIR_DOMAIN_INPUT_MODEL_LAST
>>>>>> should result in 0 rather than virtioFlags being returned.
>>>>>
>>>>> Oops good catch
>>>>
>>>> Actually this missed a case: at least DEFAULT (meaning no model=
>>>> specified) should return virtioFlags, since this block is already
>>>> conditional on bus='virtio'. I'll have DEFAULT return virtioFlags and
>>>> LAST return 0;
>>>
>>> Now that you mention it, for devices such as <input> and <disk>,
>>> where we're introducing the model attribute just now, we should make
>>> sure that we expand the default correctly, such that eg.
>>>
>>>   <disk type='file' device='disk'>
>>>     <target dev='vda' bus='virtio'/>
>>>   </disk>
>>>
>>> is formatted back as
>>>
>>>   <disk type='file' device='disk' model='virtio'>
>>>     <target dev='vda' bus='virtio'/>
>>>   </disk>
>>>
>>> We might have to be careful when formatting the XML for migration
>>> and scrub model='virtio' in that case in order not to break
>>> backwards migration, as we already do for plenty of other cases.
>>>
>>> Once that's done, then you'll be guaranteed the model above is
>>> never going to be DEFAULT.
>>>
>>
>> IMO extending the XML we generate by default should only be done for
>> very good reasons, the migration compat issue is not a trivial extension
>> and comes with issues of its own.
>>
>> For example, say we turn model=DEFAULT into model=VIRTIO in the qemu
>> driver. In generic domain format code, we would do:
>>
>> if (!MIGRATABLE || disk->model != VIRTIO)
>>     <format disk->model in the XML>
>>
>> But the MIGRATABLE checking is very crude. It doesn't consider version
>> or capabilities of the remote libvirt. We could be migrating to an
>> identical qemu and libvirt version, but we still drop the model=VIRTIO
>> from the XML. Basically for the indefinite future, disk->model=VIRTIO in
>> the XML is never going to be accounted for in migration. This can cause
>> theoretical future issues if non-qemu drivers (or other qemu configs)
>> support model=VIRTIO but it's not their default. For those non-qemu
>> drivers, if disk->model=VIRTIO is present in the XML, the user
>> explicitly requested it, but generic formatting code will never send the
>> value during migration.
>>
>> Let's say we skip the MIGRATABLE check. In fact for most of the cases in
>> this patch series (except maybe for <controller> devs), this would work
>> fine. If we are migrating to same or newer libvirt+qemu, all is good. If
>> we migrate to older libvirt, it won't know to check disk->model at all
>> so won't complain, maybe migration would fail due to qemu compat issues
>> but that's unavoidable.
>>
>> However, the NEXT time we make a similar change, let's say filling in
>> disk->model=FOO for bus=ide, or more likely, input->model=QEMUTABLET for
>> input->bus=usb + input->type=tablet. Now we have to blacklist
>> model=QEMUTABLET from the migratable XML for the indefinite future,
>> because older libvirt _may_ know enough to parse input->model, but
>> doesn't have QEMUTABLET in the enum, and parsing fails. Okay, this is
>> what the MIGRATABLE flag is all about. But it's exposing us to the issue
>> with other drivers mentioned above.
>>
>> And this model=QEMUTABLET MIGRATABLE issue is theoretical but it's an
>> exact case that has happened before, with controller models at least.
>> Basically once you open that door of adding new default output to the
>> XML for common cases, you can't ever close it again for that enum property.
>>
>> Plus there's other downsides for outputting new properties by default:
>>
>> * massive test suite churn: every single virtio disk in the XML2XML
>> tests will now spit out model='virtio', plus all the other devices. This
>> makes backporting suck
>>
>> * libvirt downgrade issues. Previous times we added new output, like
>> input type=keyboard, there were lots of complaints like: I upgraded
>> libvirt via virt-preview to see if it fixed a bug, type=keyboard was
>> added by default, I downgraded libvirt back to regular distro version,
>> my VM disappeared, because the XML parser choked on unknown
>> type=keyboard. Again in this particular case it likely won't cause those
>> issues because model= is new everywhere, but the pattern basically can't
>> be extended forward beyond this one instance.
>>
>> Here's the benefits I see of outputing model=virtio by default
>>
>> * Reports more accurate XML. Minor IMO.
>>
>> * Better chance of maintaining qemu compat across libvirt/qemu upgrades.
>> Encoding model=virtio means we will always try that. Hypothetically 5
>> years from now libvirt starts defaulting to
>> model=virtio-non-transitional, you might upgrade libvirt and your
>> windows virtio VM doesn't boot. Yeah that's not nice, but it's fixable
>> (user sets model=virtio in the XML). Same reasoning basically applies to
>> migration compat as well
> 
> Realistically we can never change the defaults for anything in libvirt,
> it has to be done at a layer above.
> 
> Previously when we've added elements to the XML that were not previously
> present, we have done this to capture some aspect of the guest ABI that
> was not previously recorded by libvirt. I see value in that.
> 
> I don't think that is actually the case this time though. The model=virtio
> addition is just duplcating what is already perfectly well expressed in
> the existing  bus=virtio attribute. There's no aspect of the guest ABI
> that was missing that we've added here.
> 
> Only model=virtio-transitional / model=virtio-non-transitional add new
> information wrt the ABI. model=virtio is merely a redundant convenience
> we've added for sake of symmetry.
> 
> So on this basis, I think there's not a compelling reason to output it
> in the XML if the user didn't explicitly set it themselves.
> 

Okay thanks for the response, I'll skip this for my series then. FWIW if
we change our minds later I don't think there's any special downsides to
doing it in a follow up series.

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 954ba5a171..f5b9bc6937 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -540,6 +540,8 @@  VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
 
               /* 340 */
               "vhost-vsock-pci-non-transitional",
+              "virtio-input-host-pci-transitional",
+              "virtio-input-host-pci-non-transitional",
     );
 
 
@@ -1142,6 +1144,8 @@  struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
     {"virtio-balloon-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BALLOON_NON_TRANSITIONAL},
     {"vhost-vsock-pci-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL},
     {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},
+    {"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},
+    {"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 99e3b3c5ca..91aae8df5b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -524,6 +524,8 @@  typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
 
     /* 340 */
     QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL, /* -device vhost-vsock-pci-non-transitional */
+    QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL, /* -device virtio-input-host-pci-transitional */
+    QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL, /* -device virtio-input-host-pci-non-transitional */
 
     QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 67e54d42b8..4d14ac9e5d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -501,9 +501,14 @@  qemuBuildVirtioTransitional(virBufferPtr buf,
             tmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_TRANSITIONAL;
             ntmodel_cap = QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL;
             break;
+        case VIR_DOMAIN_DEVICE_INPUT:
+            has_tmodel = model == VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL;
+            has_ntmodel = model == VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL;
+            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL;
+            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL;
+            break;
 
         case VIR_DOMAIN_DEVICE_LEASE:
-        case VIR_DOMAIN_DEVICE_INPUT:
         case VIR_DOMAIN_DEVICE_SOUND:
         case VIR_DOMAIN_DEVICE_VIDEO:
         case VIR_DOMAIN_DEVICE_WATCHDOG:
@@ -4286,7 +4291,10 @@  qemuBuildVirtioInputDevStr(const virDomainDef *def,
             goto error;
         break;
     case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-input-host", dev->info.type) < 0)
+        if (qemuBuildVirtioTransitional(&buf, "virtio-input-host", qemuCaps,
+                                        dev->info.type,
+                                        dev->model, NULL,
+                                        VIR_DOMAIN_DEVICE_INPUT) < 0)
             goto error;
         break;
     case VIR_DOMAIN_INPUT_TYPE_LAST:
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 1b476bbc15..70591dce25 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -917,6 +917,15 @@  qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
     case VIR_DOMAIN_DEVICE_INPUT:
         switch ((virDomainInputBus) dev->data.input->bus) {
         case VIR_DOMAIN_INPUT_BUS_VIRTIO:
+            switch ((virDomainInputModel) dev->data.input->model) {
+            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:
+                return pciFlags;
+            case VIR_DOMAIN_INPUT_MODEL_VIRTIO:
+            case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:
+            case VIR_DOMAIN_INPUT_MODEL_DEFAULT:
+            case VIR_DOMAIN_INPUT_MODEL_LAST:
+                break;
+            }
             return virtioFlags;
 
         case VIR_DOMAIN_INPUT_BUS_PS2:
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
index b5861b1982..d5bbbc40f7 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
@@ -226,6 +226,8 @@ 
   <flag name='virtio-balloon-pci-non-transitional'/>
   <flag name='vhost-vsock-pci-transitional'/>
   <flag name='vhost-vsock-pci-non-transitional'/>
+  <flag name='virtio-input-host-pci-transitional'/>
+  <flag name='virtio-input-host-pci-non-transitional'/>
   <version>3001050</version>
   <kvmVersion>0</kvmVersion>
   <microcodeVersion>446361</microcodeVersion>
diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args
index 5952008b28..fc183757b0 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args
@@ -42,8 +42,8 @@  bus=pci.1,addr=0x0 \
 -netdev user,id=hostnet0 \
 -device virtio-net-pci,disable-legacy=on,netdev=hostnet0,id=net0,\
 mac=00:11:22:33:44:55,bus=pci.2,addr=0x0 \
--device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.7,\
-addr=0x0 \
+-device virtio-input-host-pci,disable-legacy=on,id=input0,\
+evdev=/dev/input/event1234,bus=pci.7,addr=0x0 \
 -device vhost-scsi-pci,disable-legacy=on,wwpn=naa.5123456789abcde0,vhostfd=3,\
 id=hostdev0,bus=pci.4,addr=0x0 \
 -device virtio-balloon-pci,disable-legacy=on,id=balloon0,bus=pci.5,addr=0x0 \
diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
index 8e0709816b..383b29f629 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
@@ -42,8 +42,8 @@  bus=pci.1,addr=0x0 \
 -netdev user,id=hostnet0 \
 -device virtio-net-pci-non-transitional,netdev=hostnet0,id=net0,\
 mac=00:11:22:33:44:55,bus=pci.2,addr=0x0 \
--device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.7,\
-addr=0x0 \
+-device virtio-input-host-pci-non-transitional,id=input0,\
+evdev=/dev/input/event1234,bus=pci.7,addr=0x0 \
 -device vhost-scsi-pci-non-transitional,wwpn=naa.5123456789abcde0,vhostfd=3,\
 id=hostdev0,bus=pci.4,addr=0x0 \
 -device virtio-balloon-pci-non-transitional,id=balloon0,bus=pci.5,addr=0x0 \
diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
index 06fe70bb11..8046e5c102 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
+++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
@@ -27,7 +27,6 @@  file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 addr=0x1 \
 -device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \
 -device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \
--device pcie-root-port,port=0xa,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x2 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
 -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,\
 id=virtio-disk0,bootindex=1 \
@@ -36,8 +35,8 @@  id=virtio-disk0,bootindex=1 \
 -netdev user,id=hostnet0 \
 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=00:11:22:33:44:55,bus=pci.2,\
 addr=0x2 \
--device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.3,\
-addr=0x0 \
+-device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.2,\
+addr=0x7 \
 -device vhost-scsi-pci,wwpn=naa.5123456789abcde0,vhostfd=3,id=hostdev0,\
 bus=pci.2,addr=0x4 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x5 \
@@ -45,5 +44,5 @@  bus=pci.2,addr=0x4 \
 -device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x6 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
--device vhost-vsock-pci,id=vsock0,guest-cid=4,vhostfd=6789,bus=pci.2,addr=0x7 \
+-device vhost-vsock-pci,id=vsock0,guest-cid=4,vhostfd=6789,bus=pci.2,addr=0x8 \
 -msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
index 24b49e6009..410eb28f0a 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
@@ -27,7 +27,6 @@  file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 addr=0x1 \
 -device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \
 -device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \
--device pcie-root-port,port=0xa,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x2 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
 -device virtio-blk-pci-transitional,scsi=off,bus=pci.2,addr=0x3,\
 drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
@@ -37,8 +36,8 @@  bus=pci.2,addr=0x1 \
 -netdev user,id=hostnet0 \
 -device virtio-net-pci-transitional,netdev=hostnet0,id=net0,\
 mac=00:11:22:33:44:55,bus=pci.2,addr=0x2 \
--device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.3,\
-addr=0x0 \
+-device virtio-input-host-pci-transitional,id=input0,\
+evdev=/dev/input/event1234,bus=pci.2,addr=0x7 \
 -device vhost-scsi-pci-transitional,wwpn=naa.5123456789abcde0,vhostfd=3,\
 id=hostdev0,bus=pci.2,addr=0x4 \
 -device virtio-balloon-pci-transitional,id=balloon0,bus=pci.2,addr=0x5 \
@@ -47,5 +46,5 @@  id=hostdev0,bus=pci.2,addr=0x4 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
 -device vhost-vsock-pci-transitional,id=vsock0,guest-cid=4,vhostfd=6789,\
-bus=pci.2,addr=0x7 \
+bus=pci.2,addr=0x8 \
 -msg timestamp=on
diff --git a/tests/qemuxml2xmloutdata/virtio-transitional.xml b/tests/qemuxml2xmloutdata/virtio-transitional.xml
index 490b28368a..9fa9732c2d 100644
--- a/tests/qemuxml2xmloutdata/virtio-transitional.xml
+++ b/tests/qemuxml2xmloutdata/virtio-transitional.xml
@@ -39,11 +39,6 @@ 
       <target chassis='3' port='0x9'/>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
     </controller>
-    <controller type='pci' index='4' model='pcie-root-port'>
-      <model name='pcie-root-port'/>
-      <target chassis='4' port='0xa'/>
-      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
-    </controller>
     <filesystem type='mount' accessmode='passthrough' model='virtio-transitional'>
       <source dir='/export/fs1'/>
       <target dir='fs1'/>
@@ -56,7 +51,7 @@ 
     </interface>
     <input type='passthrough' bus='virtio' model='virtio-transitional'>
       <source evdev='/dev/input/event1234'/>
-      <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x07' function='0x0'/>
     </input>
     <input type='mouse' bus='ps2'/>
     <input type='keyboard' bus='ps2'/>
@@ -73,7 +68,7 @@ 
     </rng>
     <vsock model='virtio-transitional'>
       <cid auto='no' address='4'/>
-      <address type='pci' domain='0x0000' bus='0x02' slot='0x07' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x08' function='0x0'/>
     </vsock>
   </devices>
 </domain>