Existing modern-only device types are not being touched by v3, as
they don't need separate variants.  However, I plan to implement
separate cleanups in the code that calls virtio_pci_force_virtio_1(),
first, and then propose additional changes (e.g. deprecating
disable-legacy and disable-modern in those device types).

Changes v3 -> v4:
* Trivial comment fixes (Cornelia Huck)
* Test code update (Caio Carrara)

Changes v2 -> v3:
* Split into two separate patches (type registration helper
  and introduction of new types)
* Rewrote virtio_pci_types_register() completely:
  * Replaced magic generation of type names with explicit fields in
    VirtioPCIDeviceTypeInfo
  * Removed modern_only field (not necessary anymore)
  * Don't register a separate base type unless necessary

Changes v1 -> v2:
* Removed *-0.9 devices.  Nobody will want to use them, if
  transitional devices work with legacy drivers
  (Gerd Hoffmann, Michael S. Tsirkin)
* Drop virtio version from name: rename -1.0-transitional to
  -transitional (Michael S. Tsirkin)
* Renamed -1.0 to -non-transitional
* Don't add any extra variants to modern-only device types
  (they don't need it)
* Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
  Cornelia Huck)
* No need to change cast macros for modern-only devices
* Rename virtio_register_types() to virtio_pci_types_register()

Original patch description:

Many of 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 plugged.

This patch adds separate device types for each of those virtio
device flavors:

* virtio-*-pci: the existing multi-purpose device types
* virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
* virtio-*-pci-non-transitional: modern-only

Reference to previous discussion that originated this idea:
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html

Eduardo Habkost (2):
  virtio: Helper for registering virtio device types
  virtio: Provide version-specific variants of virtio PCI devices

 hw/virtio/virtio-pci.h             |  78 +++++++--
 hw/display/virtio-gpu-pci.c        |   7 +-
 hw/display/virtio-vga.c            |   7 +-
 hw/virtio/virtio-crypto-pci.c      |   7 +-
 hw/virtio/virtio-pci.c             | 267 ++++++++++++++++++++++-------
 tests/acceptance/virtio_version.py | 176 +++++++++++++++++++
 6 files changed, 452 insertions(+), 90 deletions(-)
 create mode 100644 tests/acceptance/virtio_version.py

-- 
2.18.0.rc1.1.g3f1ff2140

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

Patchew URL: https://patchew.org/QEMU/20181205195704.17605-1-ehabkost@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20181205195704.17605-1-ehabkost@redhat.com
Subject: [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
486a758 virtio: Provide version-specific variants of virtio PCI devices
85361d9 virtio: Helper for registering virtio device types

=== OUTPUT BEGIN ===
Checking PATCH 1/2: virtio: Helper for registering virtio device types...
WARNING: line over 80 characters
#496: FILE: hw/virtio/virtio-pci.h:443:
+     * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE,

WARNING: line over 80 characters
#505: FILE: hw/virtio/virtio-pci.h:452:
+     * Implements both INTERFACE_PCIE_DEVICE and INTERFACE_CONVENTIONAL_PCI_DEVICE.

total: 0 errors, 2 warnings, 469 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/2: virtio: Provide version-specific variants of virtio PCI devices...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#316: 
new file mode 100644

ERROR: line over 90 characters
#372: FILE: tests/acceptance/virtio_version.py:52:
+    return devtype in [d['name'] for d in vm.command('qom-list-types', implements=implements)]

WARNING: line over 80 characters
#427: FILE: tests/acceptance/virtio_version.py:107:
+        dev_1_0, nt_ifaces = self.run_device('%s-non-transitional' % (qemu_devtype))

WARNING: line over 80 characters
#451: FILE: tests/acceptance/virtio_version.py:131:
+        dev_trans, trans_ifaces = self.run_device('%s-transitional' % (qemu_devtype))

total: 1 errors, 3 warnings, 404 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20181205195704.17605-1-ehabkost@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

Friendly ping.  3.1.0 is tagged now, so there's anything else
blocking this series?


On Wed, Dec 05, 2018 at 05:57:02PM -0200, Eduardo Habkost wrote:
> Existing modern-only device types are not being touched by v3, as
> they don't need separate variants.  However, I plan to implement
> separate cleanups in the code that calls virtio_pci_force_virtio_1(),
> first, and then propose additional changes (e.g. deprecating
> disable-legacy and disable-modern in those device types).
> 
> Changes v3 -> v4:
> * Trivial comment fixes (Cornelia Huck)
> * Test code update (Caio Carrara)
> 
> Changes v2 -> v3:
> * Split into two separate patches (type registration helper
>   and introduction of new types)
> * Rewrote virtio_pci_types_register() completely:
>   * Replaced magic generation of type names with explicit fields in
>     VirtioPCIDeviceTypeInfo
>   * Removed modern_only field (not necessary anymore)
>   * Don't register a separate base type unless necessary
> 
> Changes v1 -> v2:
> * Removed *-0.9 devices.  Nobody will want to use them, if
>   transitional devices work with legacy drivers
>   (Gerd Hoffmann, Michael S. Tsirkin)
> * Drop virtio version from name: rename -1.0-transitional to
>   -transitional (Michael S. Tsirkin)
> * Renamed -1.0 to -non-transitional
> * Don't add any extra variants to modern-only device types
>   (they don't need it)
> * Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
>   Cornelia Huck)
> * No need to change cast macros for modern-only devices
> * Rename virtio_register_types() to virtio_pci_types_register()
> 
> Original patch description:
> 
> Many of 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 plugged.
> 
> This patch adds separate device types for each of those virtio
> device flavors:
> 
> * virtio-*-pci: the existing multi-purpose device types
> * virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> * virtio-*-pci-non-transitional: modern-only
> 
> Reference to previous discussion that originated this idea:
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> 
> Eduardo Habkost (2):
>   virtio: Helper for registering virtio device types
>   virtio: Provide version-specific variants of virtio PCI devices
> 
>  hw/virtio/virtio-pci.h             |  78 +++++++--
>  hw/display/virtio-gpu-pci.c        |   7 +-
>  hw/display/virtio-vga.c            |   7 +-
>  hw/virtio/virtio-crypto-pci.c      |   7 +-
>  hw/virtio/virtio-pci.c             | 267 ++++++++++++++++++++++-------
>  tests/acceptance/virtio_version.py | 176 +++++++++++++++++++
>  6 files changed, 452 insertions(+), 90 deletions(-)
>  create mode 100644 tests/acceptance/virtio_version.py
> 
> -- 
> 2.18.0.rc1.1.g3f1ff2140
> 

-- 
Eduardo

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

Nothing, I'm packing up the 1st pull request.

On Tue, Dec 11, 2018 at 11:18:51PM -0200, Eduardo Habkost wrote:
> Friendly ping.  3.1.0 is tagged now, so there's anything else
> blocking this series?
> 
> 
> On Wed, Dec 05, 2018 at 05:57:02PM -0200, Eduardo Habkost wrote:
> > Existing modern-only device types are not being touched by v3, as
> > they don't need separate variants.  However, I plan to implement
> > separate cleanups in the code that calls virtio_pci_force_virtio_1(),
> > first, and then propose additional changes (e.g. deprecating
> > disable-legacy and disable-modern in those device types).
> > 
> > Changes v3 -> v4:
> > * Trivial comment fixes (Cornelia Huck)
> > * Test code update (Caio Carrara)
> > 
> > Changes v2 -> v3:
> > * Split into two separate patches (type registration helper
> >   and introduction of new types)
> > * Rewrote virtio_pci_types_register() completely:
> >   * Replaced magic generation of type names with explicit fields in
> >     VirtioPCIDeviceTypeInfo
> >   * Removed modern_only field (not necessary anymore)
> >   * Don't register a separate base type unless necessary
> > 
> > Changes v1 -> v2:
> > * Removed *-0.9 devices.  Nobody will want to use them, if
> >   transitional devices work with legacy drivers
> >   (Gerd Hoffmann, Michael S. Tsirkin)
> > * Drop virtio version from name: rename -1.0-transitional to
> >   -transitional (Michael S. Tsirkin)
> > * Renamed -1.0 to -non-transitional
> > * Don't add any extra variants to modern-only device types
> >   (they don't need it)
> > * Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
> >   Cornelia Huck)
> > * No need to change cast macros for modern-only devices
> > * Rename virtio_register_types() to virtio_pci_types_register()
> > 
> > Original patch description:
> > 
> > Many of 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 plugged.
> > 
> > This patch adds separate device types for each of those virtio
> > device flavors:
> > 
> > * virtio-*-pci: the existing multi-purpose device types
> > * virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> > * virtio-*-pci-non-transitional: modern-only
> > 
> > Reference to previous discussion that originated this idea:
> >   https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
> > 
> > Eduardo Habkost (2):
> >   virtio: Helper for registering virtio device types
> >   virtio: Provide version-specific variants of virtio PCI devices
> > 
> >  hw/virtio/virtio-pci.h             |  78 +++++++--
> >  hw/display/virtio-gpu-pci.c        |   7 +-
> >  hw/display/virtio-vga.c            |   7 +-
> >  hw/virtio/virtio-crypto-pci.c      |   7 +-
> >  hw/virtio/virtio-pci.c             | 267 ++++++++++++++++++++++-------
> >  tests/acceptance/virtio_version.py | 176 +++++++++++++++++++
> >  6 files changed, 452 insertions(+), 90 deletions(-)
> >  create mode 100644 tests/acceptance/virtio_version.py
> > 
> > -- 
> > 2.18.0.rc1.1.g3f1ff2140
> > 
> 
> -- 
> Eduardo

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

Sorry to resurrect such an old thread, but I have been wondering...

On Wed, 2018-12-05 at 17:57 -0200, Eduardo Habkost wrote:
[...]
> Changes v1 -> v2:
> * Removed *-0.9 devices.  Nobody will want to use them, if
>   transitional devices work with legacy drivers
>   (Gerd Hoffmann, Michael S. Tsirkin)
> * Drop virtio version from name: rename -1.0-transitional to
>   -transitional (Michael S. Tsirkin)
> * Renamed -1.0 to -non-transitional
> * Don't add any extra variants to modern-only device types
>   (they don't need it)

... if doing this was a good idea after all?

While I understand that something like virtio-gpu, which supports
the 1.0 specification exclusively, only really needs to have a
single device associated with it from the functionality point of
view, looking at it from a user's perspective it seems to me like
providing an explicit non-transitional variant would be appropriate
for consistency reasons, so that your guest could look like

  -device virtio-blk-pci-non-transitional \
  -device virtio-net-pci-non-transitional \
  -device virtio-gpu-pci-non-transitional \

and you wouldn't have to question why you can use the
non-transitional variant for pretty much everything, except for the
few cases where you can't - for no apparent reason...

It would also signal quite clearly which devices support both
transitional and non-transitional variants and which ones don't,
without having to infer that the complete lack of (non-)transitional
variants means that only the non-transitional variant is available -
except you have to use the suffix-less device name to use it.

tl;dr providing the non-transitional variant for virtio 1.0-only
      devices would make using this much more user-friendly.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

  Hi,

>   -device virtio-blk-pci-non-transitional \
>   -device virtio-net-pci-non-transitional \
>   -device virtio-gpu-pci-non-transitional \
> 
> and you wouldn't have to question why you can use the
> non-transitional variant for pretty much everything, except for the
> few cases where you can't - for no apparent reason...

Well, there are no variants, only a single virtio-$foo-pci* device.  So
you don't have to worry about picking one of the available variants,
there is no choice in the first place.

When adding an virtio-gpu-pci-non-transitional variant we'll create
confusion too, because it wouldn't be a real variant.  We would have two
100% identical devices then, and people will probably wonder why they
exist and what the difference is ...

So I can't see how this would be so much better.  We have to document
the mess no matter what.

cheers,
  Gerd

On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >   -device virtio-blk-pci-non-transitional \
> >   -device virtio-net-pci-non-transitional \
> >   -device virtio-gpu-pci-non-transitional \
> > 
> > and you wouldn't have to question why you can use the
> > non-transitional variant for pretty much everything, except for the
> > few cases where you can't - for no apparent reason...
> 
> Well, there are no variants, only a single virtio-$foo-pci* device.  So
> you don't have to worry about picking one of the available variants,
> there is no choice in the first place.
> 
> When adding an virtio-gpu-pci-non-transitional variant we'll create
> confusion too, because it wouldn't be a real variant.  We would have two
> 100% identical devices then, and people will probably wonder why they
> exist and what the difference is ...

When looking at a single device, I mostly agree with your assessment;
however, when looking at the overall situation with VirtIO devices,
one might quite reasonably infer the following rules:

  * devices marked as (non-)transitional are going to show up as
    (non-)transitional;

  * unmarked devices might show up as either one, depending on some
    factor which is not immediately obvious.

So if you knew you wanted non-transitional devices you would expect
to just use the non-transitional variant for *all* VirtIO devices,
including virtio-gpu, without necessarily caring whether the unmarked
devices behaves any differently; if you tried to use the transitional
device, you'd get an error message telling you that device doesn't
exist, which is pretty reasonable and easy to research / understand.

With the current situation, once you've tried using non-transitional
virtio-gpu and gotten back an error message, there's quite a bit more
digging required to figure out *why* the device is not there in the
first place.

So I agree neither scenario is exactly perfect, but I still think
adding non-transitional alias devices would overall be more
user-friendly.

> So I can't see how this would be so much better.  We have to document
> the mess no matter what.

We have some documentation in libvirt:

  https://libvirt.org/formatdomain.html#elementsVirtioTransitional

Not that more / improved documentation is ever a bad idea :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
> On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:

[...]

> So I agree neither scenario is exactly perfect, but I still think
> adding non-transitional alias devices would overall be more
> user-friendly.

I don't think it makes sense to add it at the qemu level. From libvirt's
point of view users should be shielded from any qemu impl detail or
inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
devices would be the same and thus does not make sense to do that
because it would be more confusing.

You can argue that we should add the alias at the libvirt level though.

[1] Yes. I'm aware that statement is quite ironical.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Mar 06, 2019 at 08:41:48AM +0100, Peter Krempa wrote:
>On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
>> On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:
>
>[...]
>
>> So I agree neither scenario is exactly perfect, but I still think
>> adding non-transitional alias devices would overall be more
>> user-friendly.
>
>I don't think it makes sense to add it at the qemu level. From libvirt's
>point of view users should be shielded from any qemu impl detail or
>inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
>devices would be the same and thus does not make sense to do that
>because it would be more confusing.
>
>You can argue that we should add the alias at the libvirt level though.
>

You can, but please don't.

Jano

>[1] Yes. I'm aware that statement is quite ironical.



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

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

On Wed, 2019-03-06 at 09:30 +0100, Ján Tomko wrote:
> On Wed, Mar 06, 2019 at 08:41:48AM +0100, Peter Krempa wrote:
> > On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
> > > So I agree neither scenario is exactly perfect, but I still think
> > > adding non-transitional alias devices would overall be more
> > > user-friendly.
> > 
> > I don't think it makes sense to add it at the qemu level. From libvirt's
> > point of view users should be shielded from any qemu impl detail or
> > inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
> > devices would be the same and thus does not make sense to do that
> > because it would be more confusing.
> > 
> > You can argue that we should add the alias at the libvirt level though.
> 
> You can, but please don't.

It would seem nobody except me thinks this is a good idea, so I
guess I'll just drop it now.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Wed, Mar 06, 2019 at 09:30:32AM +0100, Ján Tomko wrote:
> On Wed, Mar 06, 2019 at 08:41:48AM +0100, Peter Krempa wrote:
> > On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
> > > On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:
> > 
> > [...]
> > 
> > > So I agree neither scenario is exactly perfect, but I still think
> > > adding non-transitional alias devices would overall be more
> > > user-friendly.
> > 
> > I don't think it makes sense to add it at the qemu level. From libvirt's
> > point of view users should be shielded from any qemu impl detail or
> > inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
> > devices would be the same and thus does not make sense to do that
> > because it would be more confusing.
> > 
> > You can argue that we should add the alias at the libvirt level though.
> > 
> 
> You can, but please don't.

Indeed, at the libvirt level we've always tried to take the view that
there should be one way to expressing each concept/feature. Adding
new names / xml elements that duplicate existing supported concepts
to make things "consistent" is a slippery slope becasue there are
100's of places to which that can apply when you have hindsight.

It is not going to make a significant difference to how "user friendly"
libvirt is - that is not a core goal in its own right at the API / XML
schema level. It is can be a factor in deciding between multiple competing
designs when first adding a feature, but it isn't a reason to add duplication
in the API / XML.

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