Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Thu, Oct 18, 2018 at 12:25:12PM +0200, Andrea Bolognani wrote:
> On Wed, 2018-10-17 at 12:01 -0300, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote:
> > > The proposal doesn't directly address the interaction between virtio
> > > protocol version and slot type. [...]
> >
> > It does. See the interface names added to each device type.
>
> Sorry, I might be blind but I'm still not seeing it... I see a bunch
> of *-pci devices and exactly zero *-pcie devices. See below.
>
> [...]
> > The difference between the devices is not just the bus type: it
> > is a different type of device with different behavior. Using the
> > bus type to differentiate them would be misleading.
>
> I'm not arguing that we should use the bus type *only* to
> differentiate them, but rather that we should *also* differentiate
> them by bus type; failing to do so will mean that...
>
> > e.g. both modern and transitional virtio devices can be plugged
> > to Conventional PCI buses, but they have different PCI IDs.
>
> ... even people who should be very familiar with the topic by now,
> like you and me, will get it wrong from time to time, as Michael
> helpfully pointed out ;)
>
> > I'm considering doing this in v2:
> >
> > * Remove the -0.9 device type, because nobody seems to need it
> > * Add two device types:
> > * virtio-1-blk-pci-non-transitional
> > * virtio-1-blk-pci-transitional
> >
> > This way, we:
> > * Include only the major version of the spec (so
> > we don't require new device types for virtio 1.1, 1.2, etc),
> > * Use terms that come from the Virtio spec itself, to avoid
> > ambiguity.
>
> That's quite a mouthful :)
>
> Anyway, whether a device or not is transitional should go next to
> the spec version rather than at the end: this will also help with
> consistency because we will need -device and -ccw variants of all
> these, no?
Answering this question: we won't need this on the other variants
they don't have the code that silently breaks compatibility with
legacy drivers depending on the bus it's plugged. The device is
either always transitional or always non-transitional.
The root of the problem is in virtio-pci. More precisely, it
begins at virtio_pci_realize():
if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
}
combined with virtio_pci_device_plugged():
if (legacy) {
[...]
pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
} else {
pci_set_word(config + PCI_VENDOR_ID,
PCI_VENDOR_ID_REDHAT_QUMRANET);
pci_set_word(config + PCI_DEVICE_ID,
0x1040 + virtio_bus_get_vdev_id(bus));
pci_config_set_revision(config, 1);
}
>
> Can we assume if and when virtio 2.0 comes around it will also have
> both a pure variant and a transitional one which is compatible with
> virtio 1.0? If so, I would suggest the names should be
>
> virtio-1-blk-pcie (1.x only, PCIe slot)
> virtio-1-transitional-blk-pci (transitional, PCI slot)
> virtio-1-transitional-blk-pcie (transitional, PCIe slot)
I don't see any need to make separate transitional-pci and
transitional-pcie device types. A -transitional device type that
supports both Conventional PCI and PCI Express will work and I
don't see any problem with that.
For reference, the next version of this patch will have 3 device
variants:
* virtio-blk-pci (existing device, for compatibility. Contains
magic transitional/non-transitional logic depending on bus)
* virtio-blk-pci-transitional (1.x transitional, supports legacy
drivers, Conventional PCI only)
* virtio-blk-pci-non-transitional (1.x non-transitional, no
legacy driver support, supports both Conventional PCI and PCI
Express)
--
Eduardo
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Thu, Oct 18, 2018 at 12:25:12PM +0200, Andrea Bolognani wrote: > On Wed, 2018-10-17 at 12:01 -0300, Eduardo Habkost wrote: > > On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote: > > > The proposal doesn't directly address the interaction between virtio > > > protocol version and slot type. [...] > > > > It does. See the interface names added to each device type. > > Sorry, I might be blind but I'm still not seeing it... I see a bunch > of *-pci devices and exactly zero *-pcie devices. See below. > > [...] > > The difference between the devices is not just the bus type: it > > is a different type of device with different behavior. Using the > > bus type to differentiate them would be misleading. > > I'm not arguing that we should use the bus type *only* to > differentiate them, but rather that we should *also* differentiate > them by bus type; failing to do so will mean that... > > > e.g. both modern and transitional virtio devices can be plugged > > to Conventional PCI buses, but they have different PCI IDs. > > ... even people who should be very familiar with the topic by now, > like you and me, will get it wrong from time to time, as Michael > helpfully pointed out ;) > > > I'm considering doing this in v2: > > > > * Remove the -0.9 device type, because nobody seems to need it > > * Add two device types: > > * virtio-1-blk-pci-non-transitional > > * virtio-1-blk-pci-transitional > > > > This way, we: > > * Include only the major version of the spec (so > > we don't require new device types for virtio 1.1, 1.2, etc), > > * Use terms that come from the Virtio spec itself, to avoid > > ambiguity. > > That's quite a mouthful :) > > Anyway, whether a device or not is transitional should go next to > the spec version rather than at the end: this will also help with > consistency because we will need -device and -ccw variants of all > these, no? > > Can we assume if and when virtio 2.0 comes around it will also have > both a pure variant and a transitional one which is compatible with > virtio 1.0? If so, I would suggest the names should be > > virtio-1-blk-pcie (1.x only, PCIe slot) > virtio-1-transitional-blk-pci (transitional, PCI slot) > virtio-1-transitional-blk-pcie (transitional, PCIe slot) > > [...] > > > At the same time, we should not fool ourselves into thinking it will > > > take less than *years* before applications such as virt-manager can > > > actually take advantage of the new devices without compromising their > > > support for old libvirt and QEMU versions too much. > > > > > > So if we're doing this to rectify some questionable design choices > > > with the goal of having a better situation in the long run, then by > > > all means we should go ahead; but if we think this will allow us to > > > run RHEL 6 on q35 in the short term, then our efforts are probably > > > misguided. > > > > Good point. You might be right about oVirt and OpenStack, but > > I'm assuming at least some applications (maybe KubeVirt?) don't > > care about supporting old libvirt/QEMU versions and won't have > > that problem. > > AFAIK oVirt and OpenStack have been bumping their requirements quite > aggressively with each subsequent release, so it might not take them > that much time to catch up; I was thinking more about applications > like virt-manager, gnome-boxes and libguestfs, which historically > have maintained compatibility with old libvirt/QEMU releases for the > longest time. OpenStack isn't that aggressive - its min is libvirt 1.3.1 and QEMU 2.5.0 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 :|
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Wed, 2018-10-17 at 12:01 -0300, Eduardo Habkost wrote: > On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote: > > The proposal doesn't directly address the interaction between virtio > > protocol version and slot type. [...] > > It does. See the interface names added to each device type. Sorry, I might be blind but I'm still not seeing it... I see a bunch of *-pci devices and exactly zero *-pcie devices. See below. [...] > The difference between the devices is not just the bus type: it > is a different type of device with different behavior. Using the > bus type to differentiate them would be misleading. I'm not arguing that we should use the bus type *only* to differentiate them, but rather that we should *also* differentiate them by bus type; failing to do so will mean that... > e.g. both modern and transitional virtio devices can be plugged > to Conventional PCI buses, but they have different PCI IDs. ... even people who should be very familiar with the topic by now, like you and me, will get it wrong from time to time, as Michael helpfully pointed out ;) > I'm considering doing this in v2: > > * Remove the -0.9 device type, because nobody seems to need it > * Add two device types: > * virtio-1-blk-pci-non-transitional > * virtio-1-blk-pci-transitional > > This way, we: > * Include only the major version of the spec (so > we don't require new device types for virtio 1.1, 1.2, etc), > * Use terms that come from the Virtio spec itself, to avoid > ambiguity. That's quite a mouthful :) Anyway, whether a device or not is transitional should go next to the spec version rather than at the end: this will also help with consistency because we will need -device and -ccw variants of all these, no? Can we assume if and when virtio 2.0 comes around it will also have both a pure variant and a transitional one which is compatible with virtio 1.0? If so, I would suggest the names should be virtio-1-blk-pcie (1.x only, PCIe slot) virtio-1-transitional-blk-pci (transitional, PCI slot) virtio-1-transitional-blk-pcie (transitional, PCIe slot) [...] > > At the same time, we should not fool ourselves into thinking it will > > take less than *years* before applications such as virt-manager can > > actually take advantage of the new devices without compromising their > > support for old libvirt and QEMU versions too much. > > > > So if we're doing this to rectify some questionable design choices > > with the goal of having a better situation in the long run, then by > > all means we should go ahead; but if we think this will allow us to > > run RHEL 6 on q35 in the short term, then our efforts are probably > > misguided. > > Good point. You might be right about oVirt and OpenStack, but > I'm assuming at least some applications (maybe KubeVirt?) don't > care about supporting old libvirt/QEMU versions and won't have > that problem. AFAIK oVirt and OpenStack have been bumping their requirements quite aggressively with each subsequent release, so it might not take them that much time to catch up; I was thinking more about applications like virt-manager, gnome-boxes and libguestfs, which historically have maintained compatibility with old libvirt/QEMU releases for the longest time. -- Andrea Bolognani / Red Hat / Virtualization
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Wed, Oct 17, 2018 at 11:06:31AM -0400, Michael S. Tsirkin wrote: > On Wed, Oct 17, 2018 at 12:01:37PM -0300, Eduardo Habkost wrote: [...] > > I'm considering doing this in v2: > > > > * Remove the -0.9 device type, because nobody seems to need it > > * Add two device types: > > * virtio-1-blk-pci-non-transitional > > * virtio-1-blk-pci-transitional > > > > This way, we: > > * Include only the major version of the spec (so > > we don't require new device types for virtio 1.1, 1.2, etc), > > * Use terms that come from the Virtio spec itself, to avoid > > ambiguity. > > I'd say just drop "1" completely then. E.g. transitional and legacy > have same ID's, differences are internal and not interesting to users. > If spec comes up with a new type of device it will come up with a new > term for it, I am sure. Sounds good to me. I'll do that in v2. -- Eduardo
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Wed, Oct 17, 2018 at 12:10:04PM -0300, Eduardo Habkost wrote: > On Wed, Oct 17, 2018 at 10:22:37AM +0100, Stefan Hajnoczi wrote: > > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote: > > > This patch adds separate device types for each of those virtio > > > device flavors: > > > > > > - virtio-*-pci: the existing multi-purpose device types > > > - Configurable using `disable-legacy` and `disable-modern` > > > properties > > > - Legacy driver support is automatically enabled/disabled > > > depending on the bus where it is plugged > > > - Supports Conventional PCI and PCI Express buses > > > (but Conventional PCI is incompatible with > > > disable-legacy=off) > > > - Changes PCI vendor/device IDs at runtime > > > - virtio-*-pci-0.9: legacy virtio device > > > - Supports Conventional PCI buses only, because > > > it has a PIO BAR > > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy > > > drivers > > > - Supports Conventional PCI buses only, because > > > it has a PIO BAR > > > - virtio-*-pci-1.0: modern-only > > > - Supports both Conventional PCI and PCI Express buses > > > > If new device types are being created, is it time to decouple the VIRTIO > > PCI transport from the actual VIRTIO device (blk, net, scsi) like we > > already have with virtio-mmio? > > > > -device virtio-pci-1.0,id=virtio-pci-0 > > -device virtio-blk,bus=virtio-pci-0,drive=drive0,serial=mydisk > > > > That way we avoid lots of boilerplate code and an explosion of new > > device types. > > I don't think that would work. This would in turn make > virtio-pci-1.0 a chameleon device that changes PCI ID depending > on the backend device plugged to it. > > Also, PCI IDs are not the only information contained inside the > virtio-pci subclasses. See all the PCI-specific + > device-type-specific knowledge encoded inside the class_init and > instance_init functions of each virtio-pci subclass. I think we need to draw the line somewhere. We can't make new classes and bother users each time there is a behaviour change in the device. But I also think the transport/device split it also an internal virtio thing, no one who has not read the spec cares about it. > -- > Eduardo
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Wed, Oct 17, 2018 at 10:22:37AM +0100, Stefan Hajnoczi wrote: > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote: > > This patch adds separate device types for each of those virtio > > device flavors: > > > > - virtio-*-pci: the existing multi-purpose device types > > - Configurable using `disable-legacy` and `disable-modern` > > properties > > - Legacy driver support is automatically enabled/disabled > > depending on the bus where it is plugged > > - Supports Conventional PCI and PCI Express buses > > (but Conventional PCI is incompatible with > > disable-legacy=off) > > - Changes PCI vendor/device IDs at runtime > > - virtio-*-pci-0.9: legacy virtio device > > - Supports Conventional PCI buses only, because > > it has a PIO BAR > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers > > - Supports Conventional PCI buses only, because > > it has a PIO BAR > > - virtio-*-pci-1.0: modern-only > > - Supports both Conventional PCI and PCI Express buses > > If new device types are being created, is it time to decouple the VIRTIO > PCI transport from the actual VIRTIO device (blk, net, scsi) like we > already have with virtio-mmio? > > -device virtio-pci-1.0,id=virtio-pci-0 > -device virtio-blk,bus=virtio-pci-0,drive=drive0,serial=mydisk > > That way we avoid lots of boilerplate code and an explosion of new > device types. I don't think that would work. This would in turn make virtio-pci-1.0 a chameleon device that changes PCI ID depending on the backend device plugged to it. Also, PCI IDs are not the only information contained inside the virtio-pci subclasses. See all the PCI-specific + device-type-specific knowledge encoded inside the class_init and instance_init functions of each virtio-pci subclass. -- Eduardo
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Wed, Oct 17, 2018 at 12:01:37PM -0300, Eduardo Habkost wrote: > On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote: > > On Tue, 2018-10-16 at 15:12 -0400, Laine Stump wrote: > > > On 10/16/2018 01:02 PM, Daniel P. Berrangé wrote: > > > > On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote: > > > > How about using only the major digit in the device names eg > > > > > > > > 'virtio-blk-0.x' > > > > 'virtio-blk-1.x' > > > > > > > > to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc > > > > by the same device. > > > > > > +1 from me on either "-1" or "-1.x", and a -1 on "-1.0" or "-modern". > > > > Agreed on using the major version number rather than a non-specific > > string, and since the number refers to the virtio protocol version > > I would expect the result to be > > > > virtio-0-blk-pci > > virtio-1-blk-pci > > > > and so on. > > > > The proposal doesn't directly address the interaction between virtio > > protocol version and slot type. [...] > > It does. See the interface names added to each device type. > > > > [...] Admittedly, I don't recall all the > > details myself, but the point is that I would like to see the slot > > type mentioned explicitly in the device name to avoid confusion, so > > the above might end up looking more like > > > > virtio-0-blk-pci > > virtio-1-blk-pci > > virtio-1-blk-pcie > > > > details myself, but the point is that I would like to see the slot > > type mentioned explicitly in the device name to avoid confusion, so > > the above might end up looking more like > > > > virtio-0-blk-pci > > virtio-1-blk-pci > > virtio-1-blk-pcie > > > > with the last one very clearly not being usable on i440fx. I might > > have gotten some details wrong in the example, but you get the idea. > > The difference between the devices is not just the bus type: it > is a different type of device with different behavior. Using the > bus type to differentiate them would be misleading. > > e.g. both modern and transitional virtio devices can be plugged > to Conventional PCI buses, but they have different PCI IDs. > > I'm considering doing this in v2: > > * Remove the -0.9 device type, because nobody seems to need it > * Add two device types: > * virtio-1-blk-pci-non-transitional > * virtio-1-blk-pci-transitional > > This way, we: > * Include only the major version of the spec (so > we don't require new device types for virtio 1.1, 1.2, etc), > * Use terms that come from the Virtio spec itself, to avoid > ambiguity. I'd say just drop "1" completely then. E.g. transitional and legacy have same ID's, differences are internal and not interesting to users. If spec comes up with a new type of device it will come up with a new term for it, I am sure. > > > > [...] > > > > Apps using the new device model names would either make themselves > > > > incompatible with older libvirt/QEMU, or they would increase their > > > > code complexity & testing matrix by having to support both old & new > > > > names. The usage would also harm migration to older hosts. > > > > > > > > This just to be able to switch from i440fx to q35 for OS which don't > > > > support virtio-1.0, but for such old OS, q35 isn't offering any > > > > compelling features, so they might as well stick with the thing that > > > > is known to work well. > > > > > > The *current* compelling reason is to permit management apps to use Q35 > > > for "old" OSes that don't have a driver for virtio-1.0, (and especially > > > *new* management apps that want to support only Q35 from the start), but > > > there are other future advantages that will make us appreciate that this > > > was done. For example, libosinfo currently reports separately that an > > > supports virtio-0.9 devices and/or virtio-1.0 devices, but a management > > > app would need to have extra logic to take account of the fact that the > > > only way to get a virtio-0.9 device would be to place it on a > > > conventional PCI bus; if qemu offers the two as separate devices then > > > all the management app has to do is use the device that libosinfo tells > > > it to use, and it will automatically be placed on the right kind of bus. > > > (and I've heard from Eduardo that eventually we'll be able to learn the > > > PCI ID of the devices from qmp introspection, so the management app will > > > be able to just look for a device ID that is on both the qemu and the OS > > > list, and use that). > > > > > > Obviously using these devices will make it impossible to migrate a guest > > > that uses them to an older host that doesn't have "new" qemu + libvirt, > > > but if that's important to a management app, then they can just do > > > things in the more complicated manner needed by the "combined" virtio > > > device variants. (Again, if a management app is just being > > > designed/written now, it can assume these new devices from the start and > > > ignore the older combined
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Wed, Oct 17, 2018 at 12:43:02PM +0200, Andrea Bolognani wrote: > On Tue, 2018-10-16 at 15:12 -0400, Laine Stump wrote: > > On 10/16/2018 01:02 PM, Daniel P. Berrangé wrote: > > > On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote: > > > How about using only the major digit in the device names eg > > > > > > 'virtio-blk-0.x' > > > 'virtio-blk-1.x' > > > > > > to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc > > > by the same device. > > > > +1 from me on either "-1" or "-1.x", and a -1 on "-1.0" or "-modern". > > Agreed on using the major version number rather than a non-specific > string, and since the number refers to the virtio protocol version > I would expect the result to be > > virtio-0-blk-pci > virtio-1-blk-pci > > and so on. > > The proposal doesn't directly address the interaction between virtio > protocol version and slot type. [...] It does. See the interface names added to each device type. > [...] Admittedly, I don't recall all the > details myself, but the point is that I would like to see the slot > type mentioned explicitly in the device name to avoid confusion, so > the above might end up looking more like > > virtio-0-blk-pci > virtio-1-blk-pci > virtio-1-blk-pcie > > details myself, but the point is that I would like to see the slot > type mentioned explicitly in the device name to avoid confusion, so > the above might end up looking more like > > virtio-0-blk-pci > virtio-1-blk-pci > virtio-1-blk-pcie > > with the last one very clearly not being usable on i440fx. I might > have gotten some details wrong in the example, but you get the idea. The difference between the devices is not just the bus type: it is a different type of device with different behavior. Using the bus type to differentiate them would be misleading. e.g. both modern and transitional virtio devices can be plugged to Conventional PCI buses, but they have different PCI IDs. I'm considering doing this in v2: * Remove the -0.9 device type, because nobody seems to need it * Add two device types: * virtio-1-blk-pci-non-transitional * virtio-1-blk-pci-transitional This way, we: * Include only the major version of the spec (so we don't require new device types for virtio 1.1, 1.2, etc), * Use terms that come from the Virtio spec itself, to avoid ambiguity. > > [...] > > > Apps using the new device model names would either make themselves > > > incompatible with older libvirt/QEMU, or they would increase their > > > code complexity & testing matrix by having to support both old & new > > > names. The usage would also harm migration to older hosts. > > > > > > This just to be able to switch from i440fx to q35 for OS which don't > > > support virtio-1.0, but for such old OS, q35 isn't offering any > > > compelling features, so they might as well stick with the thing that > > > is known to work well. > > > > The *current* compelling reason is to permit management apps to use Q35 > > for "old" OSes that don't have a driver for virtio-1.0, (and especially > > *new* management apps that want to support only Q35 from the start), but > > there are other future advantages that will make us appreciate that this > > was done. For example, libosinfo currently reports separately that an > > supports virtio-0.9 devices and/or virtio-1.0 devices, but a management > > app would need to have extra logic to take account of the fact that the > > only way to get a virtio-0.9 device would be to place it on a > > conventional PCI bus; if qemu offers the two as separate devices then > > all the management app has to do is use the device that libosinfo tells > > it to use, and it will automatically be placed on the right kind of bus. > > (and I've heard from Eduardo that eventually we'll be able to learn the > > PCI ID of the devices from qmp introspection, so the management app will > > be able to just look for a device ID that is on both the qemu and the OS > > list, and use that). > > > > Obviously using these devices will make it impossible to migrate a guest > > that uses them to an older host that doesn't have "new" qemu + libvirt, > > but if that's important to a management app, then they can just do > > things in the more complicated manner needed by the "combined" virtio > > device variants. (Again, if a management app is just being > > designed/written now, it can assume these new devices from the start and > > ignore the older combined device). > > > > In the end, having a device that changed PCI ID depending on what kind > > of slot it was plugged into was an idea "too clever for its own good", > > should be avoided when new devices are added in the future, and we > > should at least provide an alternative that doesn't do that for existing > > devices. > > Agreed, the current situation is kind of a mess and taking steps > towards solving it will pay off in the long term. > > At the same time, we should not fool ourselves into
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Tue, 2018-10-16 at 15:12 -0400, Laine Stump wrote: > On 10/16/2018 01:02 PM, Daniel P. Berrangé wrote: > > On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote: > > How about using only the major digit in the device names eg > > > > 'virtio-blk-0.x' > > 'virtio-blk-1.x' > > > > to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc > > by the same device. > > +1 from me on either "-1" or "-1.x", and a -1 on "-1.0" or "-modern". Agreed on using the major version number rather than a non-specific string, and since the number refers to the virtio protocol version I would expect the result to be virtio-0-blk-pci virtio-1-blk-pci and so on. The proposal doesn't directly address the interaction between virtio protocol version and slot type. Admittedly, I don't recall all the details myself, but the point is that I would like to see the slot type mentioned explicitly in the device name to avoid confusion, so the above might end up looking more like virtio-0-blk-pci virtio-1-blk-pci virtio-1-blk-pcie with the last one very clearly not being usable on i440fx. I might have gotten some details wrong in the example, but you get the idea. [...] > > Apps using the new device model names would either make themselves > > incompatible with older libvirt/QEMU, or they would increase their > > code complexity & testing matrix by having to support both old & new > > names. The usage would also harm migration to older hosts. > > > > This just to be able to switch from i440fx to q35 for OS which don't > > support virtio-1.0, but for such old OS, q35 isn't offering any > > compelling features, so they might as well stick with the thing that > > is known to work well. > > The *current* compelling reason is to permit management apps to use Q35 > for "old" OSes that don't have a driver for virtio-1.0, (and especially > *new* management apps that want to support only Q35 from the start), but > there are other future advantages that will make us appreciate that this > was done. For example, libosinfo currently reports separately that an > supports virtio-0.9 devices and/or virtio-1.0 devices, but a management > app would need to have extra logic to take account of the fact that the > only way to get a virtio-0.9 device would be to place it on a > conventional PCI bus; if qemu offers the two as separate devices then > all the management app has to do is use the device that libosinfo tells > it to use, and it will automatically be placed on the right kind of bus. > (and I've heard from Eduardo that eventually we'll be able to learn the > PCI ID of the devices from qmp introspection, so the management app will > be able to just look for a device ID that is on both the qemu and the OS > list, and use that). > > Obviously using these devices will make it impossible to migrate a guest > that uses them to an older host that doesn't have "new" qemu + libvirt, > but if that's important to a management app, then they can just do > things in the more complicated manner needed by the "combined" virtio > device variants. (Again, if a management app is just being > designed/written now, it can assume these new devices from the start and > ignore the older combined device). > > In the end, having a device that changed PCI ID depending on what kind > of slot it was plugged into was an idea "too clever for its own good", > should be avoided when new devices are added in the future, and we > should at least provide an alternative that doesn't do that for existing > devices. Agreed, the current situation is kind of a mess and taking steps towards solving it will pay off in the long term. At the same time, we should not fool ourselves into thinking it will take less than *years* before applications such as virt-manager can actually take advantage of the new devices without compromising their support for old libvirt and QEMU versions too much. So if we're doing this to rectify some questionable design choices with the goal of having a better situation in the long run, then by all means we should go ahead; but if we think this will allow us to run RHEL 6 on q35 in the short term, then our efforts are probably misguided. -- Andrea Bolognani / Red Hat / Virtualization
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote: > This patch adds separate device types for each of those virtio > device flavors: > > - virtio-*-pci: the existing multi-purpose device types > - Configurable using `disable-legacy` and `disable-modern` > properties > - Legacy driver support is automatically enabled/disabled > depending on the bus where it is plugged > - Supports Conventional PCI and PCI Express buses > (but Conventional PCI is incompatible with > disable-legacy=off) > - Changes PCI vendor/device IDs at runtime > - virtio-*-pci-0.9: legacy virtio device > - Supports Conventional PCI buses only, because > it has a PIO BAR > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers > - Supports Conventional PCI buses only, because > it has a PIO BAR > - virtio-*-pci-1.0: modern-only > - Supports both Conventional PCI and PCI Express buses If new device types are being created, is it time to decouple the VIRTIO PCI transport from the actual VIRTIO device (blk, net, scsi) like we already have with virtio-mmio? -device virtio-pci-1.0,id=virtio-pci-0 -device virtio-blk,bus=virtio-pci-0,drive=drive0,serial=mydisk That way we avoid lots of boilerplate code and an explosion of new device types. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On 10/16/2018 01:02 PM, Daniel P. Berrangé wrote:
> On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote:
>> On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
>>> On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
The current virtio-*-pci device types actually represent 3
different types of devices:
* virtio 1.0 non-transitional devices
* virtio 1.0 transitional devices
* virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus
compatibility QMP interfaces. With this multi-purpose device
type, there's no way to tell management software that
transitional devices and legacy devices require a Conventional
PCI bus.
The multi-purpose device types would also prevent us from telling
management software what's the PCI vendor/device ID for them,
because their PCI IDs change at runtime depending on the bus
where they were is plugged.
This patch adds separate device types for each of those virtio
device flavors:
- virtio-*-pci: the existing multi-purpose device types
- Configurable using `disable-legacy` and `disable-modern`
properties
- Legacy driver support is automatically enabled/disabled
depending on the bus where it is plugged
- Supports Conventional PCI and PCI Express buses
(but Conventional PCI is incompatible with
disable-legacy=off)
- Changes PCI vendor/device IDs at runtime
- virtio-*-pci-0.9: legacy virtio device
- Supports Conventional PCI buses only, because
it has a PIO BAR
- virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy
drivers
- Supports Conventional PCI buses only, because
it has a PIO BAR
- virtio-*-pci-1.0: modern-only
- Supports both Conventional PCI and PCI Express buses
>>>
>>> I would prefer a "modern" suffix since it will likely cover future
>>> revisions as well.
>>
>> That's on purpose. The new device types don't cover future
>> revisions, otherwise we'll have exactly the same ambiguity
>> problems in the future.
>>
>> The moment we have a new virtio specification released, a device
>> type called "modern" will automatically become ambiguous.
>
> Agreed, I don't want to see us back in the same mess, if a
> virtio-2.0 ever gets released with non-backcompatible
> changes.
>
> How about using only the major digit in the device names eg
>
> 'virtio-blk-0.x'
> 'virtio-blk-1.x'
>
> to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc
> by the same device.
+1 from me on either "-1" or "-1.x", and a -1 on "-1.0" or "-modern".
>
> This assumes that virtio authors are indeed using semantic
> versioning where minor digit indicates backcompat changes
> and major digit indicates breaking compat changes.
>
>
>>> Besides, I don't have a problem with this but I'd like an
>>> ack from someone on the management side, confirming
>>> these new interfaces are actually going to be used.
>>>
>>> Could you copy some relevant people as well pls?
>>
>> CCing Andrea, Daniel, and Laine from the libvirt side.
>
> I don't have a objection from libvirt side.
>
> Last time, I suggested/discussed this I was not convinced that the benefit
> was compelling enough to justify the work across all levels in the stack.
>
> Apps using the new device model names would either make themselves
> incompatible with older libvirt/QEMU, or they would increase their
> code complexity & testing matrix by having to support both old & new
> names. The usage would also harm migration to older hosts.
>
> This just to be able to switch from i440fx to q35 for OS which don't
> support virtio-1.0, but for such old OS, q35 isn't offering any
> compelling features, so they might as well stick with the thing that
> is known to work well.
The *current* compelling reason is to permit management apps to use Q35
for "old" OSes that don't have a driver for virtio-1.0, (and especially
*new* management apps that want to support only Q35 from the start), but
there are other future advantages that will make us appreciate that this
was done. For example, libosinfo currently reports separately that an
supports virtio-0.9 devices and/or virtio-1.0 devices, but a management
app would need to have extra logic to take account of the fact that the
only way to get a virtio-0.9 device would be to place it on a
conventional PCI bus; if qemu offers the two as separate devices then
all the management app has to do is use the device that libosinfo tells
it to use, and it will automatically be placed on the right kind of bus.
(and I've heard from Eduardo that eventually we'll be able to learn the
PCI ID of the devices from qmp introspection, so the management app will
be able to just look for a device ID that is on both the qemu and the OS
list, and use that).
Obviously using these devices
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
Hi, > > See above. We can't drop disable-modern. > > Good point. But this doesn't require it to be a supported device > option for users/management. Maybe we should rename it to > "x-disable-modern" (but that's a separate discussion). I think it would be more useful to allow properties being tagged as "no-user", simliar to devices which we do not want be created via -device. > > > If we still want to provide a legacy-only virtio device, it should be > > > a separate device type. > > > > I can't see a reason why we should provide legacy-only virtio devices. > > If the only reason for disable-modern to exist is for internal > usage by machine-type code, I agree we don't need the -0.9 device > types. In the early virtio-1.0 days it was required for testing. We had one or two releases with virtio-1.0 support merged but not (yet) turned on by default, so you had to use disable-modern=off to enable virtio-1.0 support. That use case is gone though since we flipped the default. cheers, Gerd
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Mon, Oct 15, 2018 at 03:14:04PM -0300, Eduardo Habkost wrote:
> On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
> > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > The current virtio-*-pci device types actually represent 3
> > > different types of devices:
> > > * virtio 1.0 non-transitional devices
> > > * virtio 1.0 transitional devices
> > > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> > >
> > > That would be just an annoyance if it didn't break our device/bus
> > > compatibility QMP interfaces. With this multi-purpose device
> > > type, there's no way to tell management software that
> > > transitional devices and legacy devices require a Conventional
> > > PCI bus.
> > >
> > > The multi-purpose device types would also prevent us from telling
> > > management software what's the PCI vendor/device ID for them,
> > > because their PCI IDs change at runtime depending on the bus
> > > where they were is plugged.
> > >
> > > This patch adds separate device types for each of those virtio
> > > device flavors:
> > >
> > > - virtio-*-pci: the existing multi-purpose device types
> > > - Configurable using `disable-legacy` and `disable-modern`
> > > properties
> > > - Legacy driver support is automatically enabled/disabled
> > > depending on the bus where it is plugged
> > > - Supports Conventional PCI and PCI Express buses
> > > (but Conventional PCI is incompatible with
> > > disable-legacy=off)
> > > - Changes PCI vendor/device IDs at runtime
> > > - virtio-*-pci-0.9: legacy virtio device
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy
> > > drivers
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0: modern-only
> > > - Supports both Conventional PCI and PCI Express buses
> >
> > I would prefer a "modern" suffix since it will likely cover future
> > revisions as well.
>
> That's on purpose. The new device types don't cover future
> revisions, otherwise we'll have exactly the same ambiguity
> problems in the future.
>
> The moment we have a new virtio specification released, a device
> type called "modern" will automatically become ambiguous.
Agreed, I don't want to see us back in the same mess, if a
virtio-2.0 ever gets released with non-backcompatible
changes.
How about using only the major digit in the device names eg
'virtio-blk-0.x'
'virtio-blk-1.x'
to make it clearer that we cover 1.0 and 1.1 (and 1.2, etc
by the same device.
This assumes that virtio authors are indeed using semantic
versioning where minor digit indicates backcompat changes
and major digit indicates breaking compat changes.
> > Besides, I don't have a problem with this but I'd like an
> > ack from someone on the management side, confirming
> > these new interfaces are actually going to be used.
> >
> > Could you copy some relevant people as well pls?
>
> CCing Andrea, Daniel, and Laine from the libvirt side.
I don't have a objection from libvirt side.
Last time, I suggested/discussed this I was not convinced that the benefit
was compelling enough to justify the work across all levels in the stack.
Apps using the new device model names would either make themselves
incompatible with older libvirt/QEMU, or they would increase their
code complexity & testing matrix by having to support both old & new
names. The usage would also harm migration to older hosts.
This just to be able to switch from i440fx to q35 for OS which don't
support virtio-1.0, but for such old OS, q35 isn't offering any
compelling features, so they might as well stick with the thing that
is known to work well.
If QEMU supports this, we'd support it in libvirt, but my recommendation
to apps would still likely be to not use it and simply stick with i440fx
for such older OS.
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 :|
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Tue, 16 Oct 2018 10:32:20 -0300 Eduardo Habkost wrote: > On Tue, Oct 16, 2018 at 10:39:30AM +0200, Cornelia Huck wrote: > > So, what I'd propose is: > > - virtio-*-pci-standard: compliant with the virtio standard 1.0 or > > later; no legacy fallback > > - virtio-*-pci-transitional: compliant with the virtio standard 1.0 or > > later; fallback to legacy included, as specified by the standard > > (- virtio-*-pci-legacy: legacy devices, should we need that for compat > > reasons) > > > > We could also use '-virtio-1' instead of '-standard', if we do a major > > break with a 2.x standard (I don't see it yet). But having a new type > > for 1.1 sounds wrong. > > That's true: adding a new type for 1.1 is probably going to be > wrong. > > But how can I make any promises about the existing device type > being compatible with 1.1 (or 1.2, 1.3...), if the 1.1 (or 1.2, > 1.3...) specification wasn't released yet? I think the *goal* is to keep them compatible. We'll probably want to switch to virtio-2 should we want to do an explicit break in the future. > > Maybe using "-virtio-1" would be a good way to be clear we're > talking about virtio-1.x without making any promises about 1.1, > 1.2, 1.3, etc. It would fit the way this is supposed to work, yes.
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Tue, Oct 16, 2018 at 08:48:16AM +0200, Gerd Hoffmann wrote: > Hi, > > > I don't know. The `disable-modern` option already exists but I > > don't know who would want to use it. > > Compat property for old (2.6 & older) machine types. > > > > Right - maybe a flag to disable modern interface is enough. > > > We thus get two types: > > > - transitional (legacy) > > > - non-transitional (modern) > > > which matches spec nicely. > > > > I would agree with the removal of virtio-*-0.9 only if we decide > > to drop support for disable-modern=true. > > See above. We can't drop disable-modern. Good point. But this doesn't require it to be a supported device option for users/management. Maybe we should rename it to "x-disable-modern" (but that's a separate discussion). > > > If we still want to provide a legacy-only virtio device, it should be > > a separate device type. > > I can't see a reason why we should provide legacy-only virtio devices. If the only reason for disable-modern to exist is for internal usage by machine-type code, I agree we don't need the -0.9 device types. -- Eduardo
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Tue, Oct 16, 2018 at 10:39:30AM +0200, Cornelia Huck wrote:
> On Mon, 15 Oct 2018 15:14:04 -0300
> Eduardo Habkost wrote:
>
> > On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > > The current virtio-*-pci device types actually represent 3
> > > > different types of devices:
> > > > * virtio 1.0 non-transitional devices
> > > > * virtio 1.0 transitional devices
> > > > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> > > >
> > > > That would be just an annoyance if it didn't break our device/bus
> > > > compatibility QMP interfaces. With this multi-purpose device
> > > > type, there's no way to tell management software that
> > > > transitional devices and legacy devices require a Conventional
> > > > PCI bus.
> > > >
> > > > The multi-purpose device types would also prevent us from telling
> > > > management software what's the PCI vendor/device ID for them,
> > > > because their PCI IDs change at runtime depending on the bus
> > > > where they were is plugged.
> > > >
> > > > This patch adds separate device types for each of those virtio
> > > > device flavors:
> > > >
> > > > - virtio-*-pci: the existing multi-purpose device types
> > > > - Configurable using `disable-legacy` and `disable-modern`
> > > > properties
> > > > - Legacy driver support is automatically enabled/disabled
> > > > depending on the bus where it is plugged
> > > > - Supports Conventional PCI and PCI Express buses
> > > > (but Conventional PCI is incompatible with
> > > > disable-legacy=off)
> > > > - Changes PCI vendor/device IDs at runtime
> > > > - virtio-*-pci-0.9: legacy virtio device
> > > > - Supports Conventional PCI buses only, because
> > > > it has a PIO BAR
> > > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy
> > > > drivers
> > > > - Supports Conventional PCI buses only, because
> > > > it has a PIO BAR
> > > > - virtio-*-pci-1.0: modern-only
> > > > - Supports both Conventional PCI and PCI Express buses
> > >
> > > I would prefer a "modern" suffix since it will likely cover future
> > > revisions as well.
> >
> > That's on purpose. The new device types don't cover future
> > revisions, otherwise we'll have exactly the same ambiguity
> > problems in the future.
> >
> > The moment we have a new virtio specification released, a device
> > type called "modern" will automatically become ambiguous.
>
> I don't think that's a problem.
>
> Most of the issues come from the major changes that virtio-pci did when
> moving to the 1.0 standard (like that PIO BAR, or the changed ids). The
> 1.0 standard allows for gradual enhancements and changes guarded by
> feature bits.
>
> Consider the packed ring layout, which is one of the major changes for
> 1.1: It is completely guarded by feature bits, and it does not come
> with any changes down in the pci area. If we want to fence it off for
> compat machines, we need to fence off offering the bits (for _any_
> transport), which does not imply a new type of virtio-pci device.
>
> There are other, pci-specific changes which are also guarded by feature
> bits and don't need a new device type.
>
> Also, many of the enhancements of the virtio standard are likely to be
> new device types and new features for existing device types. None of
> that is pci specific.
>
> So, what I'd propose is:
> - virtio-*-pci-standard: compliant with the virtio standard 1.0 or
> later; no legacy fallback
> - virtio-*-pci-transitional: compliant with the virtio standard 1.0 or
> later; fallback to legacy included, as specified by the standard
> (- virtio-*-pci-legacy: legacy devices, should we need that for compat
> reasons)
>
> We could also use '-virtio-1' instead of '-standard', if we do a major
> break with a 2.x standard (I don't see it yet). But having a new type
> for 1.1 sounds wrong.
That's true: adding a new type for 1.1 is probably going to be
wrong.
But how can I make any promises about the existing device type
being compatible with 1.1 (or 1.2, 1.3...), if the 1.1 (or 1.2,
1.3...) specification wasn't released yet?
Maybe using "-virtio-1" would be a good way to be clear we're
talking about virtio-1.x without making any promises about 1.1,
1.2, 1.3, etc.
>
> Also note that virtio-ccw does not need this (we managed to avoid many
> of the issues virtio-pci had due to being late to the party :). I'm not
> sure what virtio-mmio does, or if we even care.
>
--
Eduardo
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Mon, 15 Oct 2018 15:14:04 -0300
Eduardo Habkost wrote:
> On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
> > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > The current virtio-*-pci device types actually represent 3
> > > different types of devices:
> > > * virtio 1.0 non-transitional devices
> > > * virtio 1.0 transitional devices
> > > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> > >
> > > That would be just an annoyance if it didn't break our device/bus
> > > compatibility QMP interfaces. With this multi-purpose device
> > > type, there's no way to tell management software that
> > > transitional devices and legacy devices require a Conventional
> > > PCI bus.
> > >
> > > The multi-purpose device types would also prevent us from telling
> > > management software what's the PCI vendor/device ID for them,
> > > because their PCI IDs change at runtime depending on the bus
> > > where they were is plugged.
> > >
> > > This patch adds separate device types for each of those virtio
> > > device flavors:
> > >
> > > - virtio-*-pci: the existing multi-purpose device types
> > > - Configurable using `disable-legacy` and `disable-modern`
> > > properties
> > > - Legacy driver support is automatically enabled/disabled
> > > depending on the bus where it is plugged
> > > - Supports Conventional PCI and PCI Express buses
> > > (but Conventional PCI is incompatible with
> > > disable-legacy=off)
> > > - Changes PCI vendor/device IDs at runtime
> > > - virtio-*-pci-0.9: legacy virtio device
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy
> > > drivers
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> > > - virtio-*-pci-1.0: modern-only
> > > - Supports both Conventional PCI and PCI Express buses
> >
> > I would prefer a "modern" suffix since it will likely cover future
> > revisions as well.
>
> That's on purpose. The new device types don't cover future
> revisions, otherwise we'll have exactly the same ambiguity
> problems in the future.
>
> The moment we have a new virtio specification released, a device
> type called "modern" will automatically become ambiguous.
I don't think that's a problem.
Most of the issues come from the major changes that virtio-pci did when
moving to the 1.0 standard (like that PIO BAR, or the changed ids). The
1.0 standard allows for gradual enhancements and changes guarded by
feature bits.
Consider the packed ring layout, which is one of the major changes for
1.1: It is completely guarded by feature bits, and it does not come
with any changes down in the pci area. If we want to fence it off for
compat machines, we need to fence off offering the bits (for _any_
transport), which does not imply a new type of virtio-pci device.
There are other, pci-specific changes which are also guarded by feature
bits and don't need a new device type.
Also, many of the enhancements of the virtio standard are likely to be
new device types and new features for existing device types. None of
that is pci specific.
So, what I'd propose is:
- virtio-*-pci-standard: compliant with the virtio standard 1.0 or
later; no legacy fallback
- virtio-*-pci-transitional: compliant with the virtio standard 1.0 or
later; fallback to legacy included, as specified by the standard
(- virtio-*-pci-legacy: legacy devices, should we need that for compat
reasons)
We could also use '-virtio-1' instead of '-standard', if we do a major
break with a 2.x standard (I don't see it yet). But having a new type
for 1.1 sounds wrong.
Also note that virtio-ccw does not need this (we managed to avoid many
of the issues virtio-pci had due to being late to the party :). I'm not
sure what virtio-mmio does, or if we even care.
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
Hi, > I don't know. The `disable-modern` option already exists but I > don't know who would want to use it. Compat property for old (2.6 & older) machine types. > > Right - maybe a flag to disable modern interface is enough. > > We thus get two types: > > - transitional (legacy) > > - non-transitional (modern) > > which matches spec nicely. > > I would agree with the removal of virtio-*-0.9 only if we decide > to drop support for disable-modern=true. See above. We can't drop disable-modern. > If we still want to provide a legacy-only virtio device, it should be > a separate device type. I can't see a reason why we should provide legacy-only virtio devices. cheers, Gerd
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Mon, Oct 15, 2018 at 11:45:56AM +0200, Cornelia Huck wrote:
> On Fri, 12 Oct 2018 23:54:35 -0300
> Eduardo Habkost wrote:
>
> > The current virtio-*-pci device types actually represent 3
> > different types of devices:
> > * virtio 1.0 non-transitional devices
> > * virtio 1.0 transitional devices
> > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> >
> > That would be just an annoyance if it didn't break our device/bus
> > compatibility QMP interfaces. With this multi-purpose device
> > type, there's no way to tell management software that
> > transitional devices and legacy devices require a Conventional
> > PCI bus.
> >
> > The multi-purpose device types would also prevent us from telling
> > management software what's the PCI vendor/device ID for them,
> > because their PCI IDs change at runtime depending on the bus
> > where they were is plugged.
> >
> > This patch adds separate device types for each of those virtio
> > device flavors:
> >
> > - virtio-*-pci: the existing multi-purpose device types
> > - Configurable using `disable-legacy` and `disable-modern`
> > properties
> > - Legacy driver support is automatically enabled/disabled
> > depending on the bus where it is plugged
> > - Supports Conventional PCI and PCI Express buses
> > (but Conventional PCI is incompatible with
> > disable-legacy=off)
> > - Changes PCI vendor/device IDs at runtime
> > - virtio-*-pci-0.9: legacy virtio device
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0: modern-only
> > - Supports both Conventional PCI and PCI Express buses
> >
> > All the types above will inherit from an abstract
> > "virtio-*-pci-base" type, so existing code that doesn't care
> > about the virtio version can use "virtio-*-pci-base" on type
> > casts.
>
> I get a crash when running 'make check'; might be missing a change to
> virtio-input-host?
>
> TEST: tests/device-introspect-test... (pid=28626)
> /s390x/device/introspect/list: OK
> /s390x/device/introspect/list-fields:OK
> /s390x/device/introspect/none: OK
> /s390x/device/introspect/abstract: OK
> /s390x/device/introspect/abstract-interfaces:OK
> /s390x/device/introspect/concrete/defaults/none:
> /home/cohuck/git/qemu/hw/virtio/virtio-pci.c:2730:virtio_host_initfn: Object
> 0x5633a43a1480 is not an instance of type virtio-input-host-device-base
> Broken pipe
> /home/cohuck/git/qemu/tests/libqtest.c:125: kill_qemu() detected QEMU death
> from signal 6 (Aborted) (core dumped)
Oops. I will fix it in v2, thanks.
>
[...]
> >
> > A simple test script (tests/acceptance/virtio_version.py) is
> > included, to check if the new device types are equivalent to
> > using the `disable-legacy` and `disable-modern` options.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > Reference to previous discussion that originated this idea:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> > ---
> > hw/virtio/virtio-pci.h | 93 +---
> > hw/display/virtio-gpu-pci.c| 8 +-
> > hw/display/virtio-vga.c| 11 +-
> > hw/virtio/virtio-crypto-pci.c | 8 +-
> > hw/virtio/virtio-pci.c | 225 +
> > tests/acceptance/virtio_version.py | 138 ++
> > 6 files changed, 390 insertions(+), 93 deletions(-)
> > create mode 100644 tests/acceptance/virtio_version.py
>
> The approach makes sense to me, but I second the suggestion to use
> something like 'modern' (or 'standard'?) instead of '1.0'.
As noted on the reply I sent to Michael: the whole point of this
patch is to provide unambiguous device type names. What would be
the advantage of having another ambiguous device type named
"virtio-*-modern"?
>
> Also, before someone asks: I don't think we need something like that
> for virtio-ccw, as we (a) only support fencing newer revisions, not
> older ones, and (b) the implications of using different virtio
> revisions are localized to virtio-ccw only and do not have further
> implications as for virtio-pci.
Thanks! I was planning to ask about it later. :)
--
Eduardo
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Mon, Oct 15, 2018 at 06:27:21AM -0400, Michael S. Tsirkin wrote:
> On Mon, Oct 15, 2018 at 10:16:41AM +0200, Gerd Hoffmann wrote:
> > On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > > The current virtio-*-pci device types actually represent 3
> > > different types of devices:
> > > * virtio 1.0 non-transitional devices
> > > * virtio 1.0 transitional devices
> >
> > Ok.
> >
> > > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> >
> > Why? Is there any reason to prefer 0.9 over transitional?
I don't know. The `disable-modern` option already exists but I
don't know who would want to use it. I'm just adding a way to
represent that existing feature without having another set of
device types with multiple-personality disorder.
> >
> > cheers,
> > Gerd
>
> Right - maybe a flag to disable modern interface is enough.
> We thus get two types:
> - transitional (legacy)
> - non-transitional (modern)
> which matches spec nicely.
I would agree with the removal of virtio-*-0.9 only if we decide
to drop support for disable-modern=true. If we still want to
provide a legacy-only virtio device, it should be a separate
device type.
Without separate device types, it becomes impossible to provide
meaningful device type information to management software,
including but not limited to:
* removal/deprecation of device types (especially important if we
plan to remove virtio 0.9 support from QEMU one day)
* PCI IDs
* Bus compatibility information
* Schema/introspection for 0.9-only or 1.0-only device options
--
Eduardo
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Sun, Oct 14, 2018 at 05:35:12PM -0400, Michael S. Tsirkin wrote:
> On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > The current virtio-*-pci device types actually represent 3
> > different types of devices:
> > * virtio 1.0 non-transitional devices
> > * virtio 1.0 transitional devices
> > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> >
> > That would be just an annoyance if it didn't break our device/bus
> > compatibility QMP interfaces. With this multi-purpose device
> > type, there's no way to tell management software that
> > transitional devices and legacy devices require a Conventional
> > PCI bus.
> >
> > The multi-purpose device types would also prevent us from telling
> > management software what's the PCI vendor/device ID for them,
> > because their PCI IDs change at runtime depending on the bus
> > where they were is plugged.
> >
> > This patch adds separate device types for each of those virtio
> > device flavors:
> >
> > - virtio-*-pci: the existing multi-purpose device types
> > - Configurable using `disable-legacy` and `disable-modern`
> > properties
> > - Legacy driver support is automatically enabled/disabled
> > depending on the bus where it is plugged
> > - Supports Conventional PCI and PCI Express buses
> > (but Conventional PCI is incompatible with
> > disable-legacy=off)
> > - Changes PCI vendor/device IDs at runtime
> > - virtio-*-pci-0.9: legacy virtio device
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-1.0: modern-only
> > - Supports both Conventional PCI and PCI Express buses
>
> I would prefer a "modern" suffix since it will likely cover future
> revisions as well.
That's on purpose. The new device types don't cover future
revisions, otherwise we'll have exactly the same ambiguity
problems in the future.
The moment we have a new virtio specification released, a device
type called "modern" will automatically become ambiguous.
>
>
> Besides, I don't have a problem with this but I'd like an
> ack from someone on the management side, confirming
> these new interfaces are actually going to be used.
>
> Could you copy some relevant people as well pls?
CCing Andrea, Daniel, and Laine from the libvirt side.
CCing Fabian from the KubeVirt side.
>
> > All the types above will inherit from an abstract
> > "virtio-*-pci-base" type, so existing code that doesn't care
> > about the virtio version can use "virtio-*-pci-base" on type
> > casts.
> >
> > A simple test script (tests/acceptance/virtio_version.py) is
> > included, to check if the new device types are equivalent to
> > using the `disable-legacy` and `disable-modern` options.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > Reference to previous discussion that originated this idea:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> > ---
> > hw/virtio/virtio-pci.h | 93 +---
> > hw/display/virtio-gpu-pci.c| 8 +-
> > hw/display/virtio-vga.c| 11 +-
> > hw/virtio/virtio-crypto-pci.c | 8 +-
> > hw/virtio/virtio-pci.c | 225 +
> > tests/acceptance/virtio_version.py | 138 ++
> > 6 files changed, 390 insertions(+), 93 deletions(-)
> > create mode 100644 tests/acceptance/virtio_version.py
> >
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index 813082b0d7..f1cfb60277 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
> > @@ -216,7 +216,8 @@ static inline void
> > virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
> > /*
> > * virtio-scsi-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci"
> > +#define TYPE_VIRTIO_SCSI_PCI_PREFIX "virtio-scsi-pci"
> > +#define TYPE_VIRTIO_SCSI_PCI (TYPE_VIRTIO_SCSI_PCI_PREFIX "-base")
> > #define VIRTIO_SCSI_PCI(obj) \
> > OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
> >
> > @@ -229,7 +230,8 @@ struct VirtIOSCSIPCI {
> > /*
> > * vhost-scsi-pci: This extends VirtioPCIProxy.
> > */
> > -#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
> > +#define TYPE_VHOST_SCSI_PCI_PREFIX "vhost-scsi-pci"
> > +#define TYPE_VHOST_SCSI_PCI (TYPE_VHOST_SCSI_PCI_PREFIX "-base")
> > #define VHOST_SCSI_PCI(obj) \
> > OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHOST_SCSI_PCI)
> >
> > @@ -239,7 +241,8 @@ struct VHostSCSIPCI {
> > };
> > #endif
> >
> > -#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
> > +#define TYPE_VHOST_USER_SCSI_PCI_PREFIX "vhost-user-scsi-pci"
> > +#define TYPE_VHOST_USER_SCSI_PCI (TYPE_VHOST_USER_SCSI_PCI_PREFIX "-base")
> > #define VHOST_USER_SCSI_PCI(obj) \
> > OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
> >
> > @@
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Mon, Oct 15, 2018 at 10:16:41AM +0200, Gerd Hoffmann wrote:
> On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> > The current virtio-*-pci device types actually represent 3
> > different types of devices:
> > * virtio 1.0 non-transitional devices
> > * virtio 1.0 transitional devices
>
> Ok.
>
> > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> Why? Is there any reason to prefer 0.9 over transitional?
>
> cheers,
> Gerd
Right - maybe a flag to disable modern interface is enough.
We thus get two types:
- transitional (legacy)
- non-transitional (modern)
which matches spec nicely.
--
MST
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Fri, 12 Oct 2018 23:54:35 -0300
Eduardo Habkost wrote:
> The current virtio-*-pci device types actually represent 3
> different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> That would be just an annoyance if it didn't break our device/bus
> compatibility QMP interfaces. With this multi-purpose device
> type, there's no way to tell management software that
> transitional devices and legacy devices require a Conventional
> PCI bus.
>
> The multi-purpose device types would also prevent us from telling
> management software what's the PCI vendor/device ID for them,
> because their PCI IDs change at runtime depending on the bus
> where they were is plugged.
>
> This patch adds separate device types for each of those virtio
> device flavors:
>
> - virtio-*-pci: the existing multi-purpose device types
> - Configurable using `disable-legacy` and `disable-modern`
> properties
> - Legacy driver support is automatically enabled/disabled
> depending on the bus where it is plugged
> - Supports Conventional PCI and PCI Express buses
> (but Conventional PCI is incompatible with
> disable-legacy=off)
> - Changes PCI vendor/device IDs at runtime
> - virtio-*-pci-0.9: legacy virtio device
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0: modern-only
> - Supports both Conventional PCI and PCI Express buses
>
> All the types above will inherit from an abstract
> "virtio-*-pci-base" type, so existing code that doesn't care
> about the virtio version can use "virtio-*-pci-base" on type
> casts.
I get a crash when running 'make check'; might be missing a change to
virtio-input-host?
TEST: tests/device-introspect-test... (pid=28626)
/s390x/device/introspect/list: OK
/s390x/device/introspect/list-fields:OK
/s390x/device/introspect/none: OK
/s390x/device/introspect/abstract: OK
/s390x/device/introspect/abstract-interfaces:OK
/s390x/device/introspect/concrete/defaults/none:
/home/cohuck/git/qemu/hw/virtio/virtio-pci.c:2730:virtio_host_initfn: Object
0x5633a43a1480 is not an instance of type virtio-input-host-device-base
Broken pipe
/home/cohuck/git/qemu/tests/libqtest.c:125: kill_qemu() detected QEMU death
from signal 6 (Aborted) (core dumped)
(...)
#2 0x5633a2e92555 in object_dynamic_cast_assert (obj=0x5633a43a1480,
typename=0x5633a30a295c "virtio-input-host-device-base",
file=0x5633a30a2152 "/home/cohuck/git/qemu/hw/virtio/virtio-pci.c",
line=2730, func=0x5633a30a297a "virtio_host_initfn")
at /home/cohuck/git/qemu/qom/object.c:702
#3 0x5633a2e30fa3 in virtio_host_initfn (obj=0x5633a43a1480)
at /home/cohuck/git/qemu/hw/virtio/virtio-pci.c:2730
#4 0x5633a2e966a2 in object_init_with_type (obj=0x5633a43a1480,
ti=0x5633a42eb990) at /home/cohuck/git/qemu/qom/object.c:356
#5 0x5633a2e91433 in object_initialize_with_type (data=0x5633a43a1480,
size=33632, type=0x5633a42eb990) at /home/cohuck/git/qemu/qom/object.c:391
#6 0x5633a2e91db8 in object_new_with_type (type=)
at /home/cohuck/git/qemu/qom/object.c:553
#7 object_new (typename=)
at /home/cohuck/git/qemu/qom/object.c:563
#8 0x5633a2dbfc49 in qmp_device_list_properties (
typename=0x5633a437cb40 "virtio-input-host-pci-1.0-transitional",
errp=0x7ffd66276010) at /home/cohuck/git/qemu/qmp.c:513
(...)
>
> A simple test script (tests/acceptance/virtio_version.py) is
> included, to check if the new device types are equivalent to
> using the `disable-legacy` and `disable-modern` options.
>
> Signed-off-by: Eduardo Habkost
> ---
> Reference to previous discussion that originated this idea:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> ---
> hw/virtio/virtio-pci.h | 93 +---
> hw/display/virtio-gpu-pci.c| 8 +-
> hw/display/virtio-vga.c| 11 +-
> hw/virtio/virtio-crypto-pci.c | 8 +-
> hw/virtio/virtio-pci.c | 225 +
> tests/acceptance/virtio_version.py | 138 ++
> 6 files changed, 390 insertions(+), 93 deletions(-)
> create mode 100644 tests/acceptance/virtio_version.py
The approach makes sense to me, but I second the suggestion to use
something like 'modern' (or 'standard'?) instead of '1.0'.
Also, before someone asks: I don't think we need something like that
for virtio-ccw, as we (a) only support fencing newer revisions, not
older ones, and (b) the implications of using different virtio
revisions are localized to virtio-ccw only
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> The current virtio-*-pci device types actually represent 3
> different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
Ok.
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
Why? Is there any reason to prefer 0.9 over transitional?
cheers,
Gerd
Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices
On Fri, Oct 12, 2018 at 11:54:35PM -0300, Eduardo Habkost wrote:
> The current virtio-*-pci device types actually represent 3
> different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> That would be just an annoyance if it didn't break our device/bus
> compatibility QMP interfaces. With this multi-purpose device
> type, there's no way to tell management software that
> transitional devices and legacy devices require a Conventional
> PCI bus.
>
> The multi-purpose device types would also prevent us from telling
> management software what's the PCI vendor/device ID for them,
> because their PCI IDs change at runtime depending on the bus
> where they were is plugged.
>
> This patch adds separate device types for each of those virtio
> device flavors:
>
> - virtio-*-pci: the existing multi-purpose device types
> - Configurable using `disable-legacy` and `disable-modern`
> properties
> - Legacy driver support is automatically enabled/disabled
> depending on the bus where it is plugged
> - Supports Conventional PCI and PCI Express buses
> (but Conventional PCI is incompatible with
> disable-legacy=off)
> - Changes PCI vendor/device IDs at runtime
> - virtio-*-pci-0.9: legacy virtio device
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-1.0: modern-only
> - Supports both Conventional PCI and PCI Express buses
I would prefer a "modern" suffix since it will likely cover future
revisions as well.
Besides, I don't have a problem with this but I'd like an
ack from someone on the management side, confirming
these new interfaces are actually going to be used.
Could you copy some relevant people as well pls?
> All the types above will inherit from an abstract
> "virtio-*-pci-base" type, so existing code that doesn't care
> about the virtio version can use "virtio-*-pci-base" on type
> casts.
>
> A simple test script (tests/acceptance/virtio_version.py) is
> included, to check if the new device types are equivalent to
> using the `disable-legacy` and `disable-modern` options.
>
> Signed-off-by: Eduardo Habkost
> ---
> Reference to previous discussion that originated this idea:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> ---
> hw/virtio/virtio-pci.h | 93 +---
> hw/display/virtio-gpu-pci.c| 8 +-
> hw/display/virtio-vga.c| 11 +-
> hw/virtio/virtio-crypto-pci.c | 8 +-
> hw/virtio/virtio-pci.c | 225 +
> tests/acceptance/virtio_version.py | 138 ++
> 6 files changed, 390 insertions(+), 93 deletions(-)
> create mode 100644 tests/acceptance/virtio_version.py
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 813082b0d7..f1cfb60277 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -216,7 +216,8 @@ static inline void
> virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
> /*
> * virtio-scsi-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci"
> +#define TYPE_VIRTIO_SCSI_PCI_PREFIX "virtio-scsi-pci"
> +#define TYPE_VIRTIO_SCSI_PCI (TYPE_VIRTIO_SCSI_PCI_PREFIX "-base")
> #define VIRTIO_SCSI_PCI(obj) \
> OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
>
> @@ -229,7 +230,8 @@ struct VirtIOSCSIPCI {
> /*
> * vhost-scsi-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
> +#define TYPE_VHOST_SCSI_PCI_PREFIX "vhost-scsi-pci"
> +#define TYPE_VHOST_SCSI_PCI (TYPE_VHOST_SCSI_PCI_PREFIX "-base")
> #define VHOST_SCSI_PCI(obj) \
> OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHOST_SCSI_PCI)
>
> @@ -239,7 +241,8 @@ struct VHostSCSIPCI {
> };
> #endif
>
> -#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
> +#define TYPE_VHOST_USER_SCSI_PCI_PREFIX "vhost-user-scsi-pci"
> +#define TYPE_VHOST_USER_SCSI_PCI (TYPE_VHOST_USER_SCSI_PCI_PREFIX "-base")
> #define VHOST_USER_SCSI_PCI(obj) \
> OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
>
> @@ -252,7 +255,8 @@ struct VHostUserSCSIPCI {
> /*
> * vhost-user-blk-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci"
> +#define TYPE_VHOST_USER_BLK_PCI_PREFIX "vhost-user-blk-pci"
> +#define TYPE_VHOST_USER_BLK_PCI (TYPE_VHOST_USER_BLK_PCI_PREFIX "-base")
> #define VHOST_USER_BLK_PCI(obj) \
> OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI)
>
> @@ -265,7 +269,8 @@ struct VHostUserBlkPCI {
> /*
> * virtio-blk-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci"
> +#define TYPE_VIRTIO_BLK_PCI_PREFIX "virtio-blk-pci"
> +#define
