hw/virtio: fix double use of a virtio flag
cover hw/virtio: fix double use of a virtio flag
To the obvious question of "how did that happen?"
I can say we had an unlucky break.
Both Jason and me worked on a new different virtio features in the same time, and they were both merged in the same pull request.
We both saw BIT 3 as the last used
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg03041.html
Commits 1811e64c and a6df8adf use the same virtio feature bit 4 for different features.
Fix it by using different bits.
While at it, group all the virtio flags into an enum to avoid that in the feature.
Marcel Apfelbaum (2):
hw/virtio: fix double use of a virtio flag
hw/virtio: group virtio flags into an enum
hw/virtio/virtio-pci.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
Signature · 1 line
2.4.3
PATCH 1/2 hw/virtio: fix double use of a virtio flag
Commits 1811e64c and a6df8adf use the same virtio feature bit 4 for different features.
Fix it by using different bits.
--- hw/virtio/virtio-pci.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
hw/virtio/virtio-pci.h +1 -1
@@ -71,7 +71,7 @@ typedef struct VirtioBusClass VirtioPCIBusClass;/* virtio version flags */ #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 -#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 6#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)
Signature · 1 line
2.4.3
Quoted 17 lines · Marcel Apfelbaum
Commits
1811e64canda6df8adfuse the same virtio feature bit 4 for different features.Fix it by using different bits.
Reported-by Laurent Vivier <[email protected]>Signed-off-by Marcel Apfelbaum <[email protected]>--- hw/virtio/virtio-pci.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/virtio/virtio-pci.h +0
-0
hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 77
Quoted 8 lines
hw/virtio/virtio-pci.h +1
-1@@ -71,7 +71,7 @@ typedef struct VirtioBusClass VirtioPCIBusClass;/* virtio version flags */ #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 -#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 6#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)
Quoted 17 lines · Marcel Apfelbaum
Commits
1811e64canda6df8adfuse the same virtio feature bit 4 for different features.Fix it by using different bits.
Reported-by Laurent Vivier <[email protected]>Signed-off-by Marcel Apfelbaum <[email protected]>--- hw/virtio/virtio-pci.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/virtio/virtio-pci.h +0
-0
hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 77
Quoted 8 lines
hw/virtio/virtio-pci.h +1
-1@@ -71,7 +71,7 @@ typedef struct VirtioBusClass VirtioPCIBusClass;/* virtio version flags */ #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 -#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 6#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)
pc-i440fx-2.4
migrate-extra = false
x-disable-pcie = true
pc-i440fx-2.5
migrate-extra = true
x-disable-pcie = false
This is correct, now.
Thanks,
Laurent
Quoted 16 lines · Marcel Apfelbaum
Commits
1811e64canda6df8adfuse the same virtio feature bit 4 for different features.Fix it by using different bits.
Reported-by Laurent Vivier <[email protected]>Signed-off-by Marcel Apfelbaum <[email protected]>--- hw/virtio/virtio-pci.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/virtio/virtio-pci.h +0
-0
hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 77
Quoted 8 lines
hw/virtio/virtio-pci.h +1
-1@@ -71,7 +71,7 @@ typedef struct VirtioBusClass VirtioPCIBusClass;/* virtio version flags */ #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 -#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 6#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)
PATCH 2/2 hw/virtio: group virtio flags into an enum
In reply to Marcel ApfelbaumMinimizes the possibility to assign
the same bit to different features.
--- hw/virtio/virtio-pci.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
hw/virtio/virtio-pci.h +10 -6
@@ -58,6 +58,16 @@ typedef struct VirtioBusClass VirtioPCIBusClass;#define VIRTIO_PCI_BUS_CLASS (klass) \ OBJECT_CLASS_CHECK(VirtioPCIBusClass, klass, TYPE_VIRTIO_PCI_BUS) +enum { +VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT , +VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT , +VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT , +VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT , +VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT , +VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT , +VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT , +}; +/* Need to activate work-arounds for buggy guests at vmstate load. */ #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT 0#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION \
@@ -65,23 +75,17 @@ typedef struct VirtioBusClass VirtioPCIBusClass;/* Performance improves when virtqueue kick processing is decoupled from the * vcpu thread using ioeventfd for some devices. */ -#define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1#define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)/* virtio version flags */ -#define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 -#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 -#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 6#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)/* migrate extra state */ -#define VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT 4#define VIRTIO_PCI_FLAG_MIGRATE_EXTRA (1 << VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT)/* have pio notification for modern device ? */ -#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT 5#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY \ (1 << VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT)
Signature · 1 line
2.4.3
Re: [Qemu-devel] [PATCH 2/2] hw/virtio: group virtio flags into an enum
Quoted 15 lines · Marcel Apfelbaum
Minimizes the possibility to assign
the same bit to different features.Signed-off-by Marcel Apfelbaum <[email protected]>--- hw/virtio/virtio-pci.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)hw/virtio/virtio-pci.h +0
-0@@ -58,6 +58,16 @@ typedef struct VirtioBusClass VirtioPCIBusClass;#define VIRTIO_PCI_BUS_CLASS (klass) \ OBJECT_CLASS_CHECK(VirtioPCIBusClass, klass, TYPE_VIRTIO_PCI_BUS)
hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 69
Quoted 10 lines
hw/virtio/virtio-pci.h +9
-0+enum { +VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT , +VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT , +VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT , +VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT , +VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT , +VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT , +VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT , +};
You can keep the "#define", doing something like this after the enum:
#define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT
So, if someone includes this ".h" out of qemu and wants to use some "#ifdef" to see if the bit is defined, it always works. (see for an example, /usr/include/linux/rtnetlink.h).
hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 72
Quoted 3 lines
hw/virtio/virtio-pci.h +1
-0+/* Need to activate work-arounds for buggy guests at vmstate load. */ #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT 0
You should remove this one too.
Quoted 26 lines
hw/virtio/virtio-pci.h +0
-6#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION \@@ -65,23 +75,17 @@ typedef struct VirtioBusClass VirtioPCIBusClass;/* Performance improves when virtqueue kick processing is decoupled from the * vcpu thread using ioeventfd for some devices. */ -#define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1#define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)/* virtio version flags */ -#define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 -#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 -#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 6#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)/* migrate extra state */ -#define VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT 4#define VIRTIO_PCI_FLAG_MIGRATE_EXTRA (1 << VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT)/* have pio notification for modern device ? */ -#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT 5#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY \ (1 << VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT)
Re: [Qemu-devel] [PATCH 2/2] hw/virtio: group virtio flags into an enum
Quoted 31 lines · Laurent Vivier
Quoted 15 lines · Marcel Apfelbaum
Minimizes the possibility to assign
the same bit to different features.Signed-off-by Marcel Apfelbaum <[email protected]>--- hw/virtio/virtio-pci.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)hw/virtio/virtio-pci.h +0
-0@@ -58,6 +58,16 @@ typedef struct VirtioBusClass VirtioPCIBusClass;#define VIRTIO_PCI_BUS_CLASS (klass) \ OBJECT_CLASS_CHECK(VirtioPCIBusClass, klass, TYPE_VIRTIO_PCI_BUS)hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 69
Quoted 10 lines
hw/virtio/virtio-pci.h +9
-0+enum { +VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT , +VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT , +VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT , +VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT , +VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT , +VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT , +VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT , +};You can keep the "#define", doing something like this after the enum:
Quoted 5 lines
#define
VIRTIO_PCI_FLAG_USE_IOEVENTFD_BITVIRTIO_PCI_FLAG_USE_IOEVENTFD_BITSo, if someone includes this ".h" out of qemu and wants to use some "#ifdef" to see if the bit is defined, it always works. (see for an example,
/usr/include/linux/rtnetlink.h).
Thanks for the idea, but I don't think anyone would want to use those out of qemu :)
I am also afraid that developers will forget to add it to the enum and will still use the defines only.
Quoted 6 lines
hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 72
Quoted 4 lines
hw/virtio/virtio-pci.h +1
-0+/* Need to activate work-arounds for buggy guests at vmstate load. */ #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT 0You should remove this one too.
How did I manage to miss it? Thanks!
I'll send a v2
Marcel
Quoted 29 lines
Quoted 27 lines
hw/virtio/virtio-pci.h +0
-6#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION \@@ -65,23 +75,17 @@ typedef struct VirtioBusClass VirtioPCIBusClass;/* Performance improves when virtqueue kick processing is decoupled from the * vcpu thread using ioeventfd for some devices. */ -#define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1#define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)/* virtio version flags */ -#define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 -#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 -#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 6#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)/* migrate extra state */ -#define VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT 4#define VIRTIO_PCI_FLAG_MIGRATE_EXTRA (1 << VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT)/* have pio notification for modern device ? */ -#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT 5#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY \ (1 << VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT)
Re: [Qemu-devel] [PATCH 2/2] hw/virtio: group virtio flags into an enum
In reply to Marcel ApfelbaumQuoted 18 lines · Marcel Apfelbaum
Minimizes the possibility to assign
the same bit to different features.Signed-off-by Marcel Apfelbaum <[email protected]>--- hw/virtio/virtio-pci.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)hw/virtio/virtio-pci.h +2
-0@@ -58,6 +58,16 @@ typedef struct VirtioBusClass VirtioPCIBusClass;#define VIRTIO_PCI_BUS_CLASS (klass) \ OBJECT_CLASS_CHECK(VirtioPCIBusClass, klass, TYPE_VIRTIO_PCI_BUS) +enum { +VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT ,
hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 72
Quoted 10 lines
hw/virtio/virtio-pci.h +8
-0+VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT , +VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT , +VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT , +VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT , +VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT , + VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, +}; +/* Need to activate work-arounds for buggy guests at vmstate load. */ #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT 0
Why not remove this line too?
Quoted 25 lines
hw/virtio/virtio-pci.h +0
-6#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION \@@ -65,23 +75,17 @@ typedef struct VirtioBusClass VirtioPCIBusClass;/* Performance improves when virtqueue kick processing is decoupled from the * vcpu thread using ioeventfd for some devices. */ -#define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1#define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)/* virtio version flags */ -#define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 -#define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 -#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 6#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)/* migrate extra state */ -#define VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT 4#define VIRTIO_PCI_FLAG_MIGRATE_EXTRA (1 << VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT)/* have pio notification for modern device ? */ -#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT 5#define VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY \ (1 << VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT)