hw/virtio: fix double use of a virtio flag

PATCH In review Marcel Apfelbaum 4 participants 7 messages Feb 10, 2016 – Mar 1, 2016

Share & tools

Marcel Apfelbaum

author

2016-02-10 13:31 UTC #0 lore raw

v2 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 feature 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.

v1 -> v2:
- Addressed Laurent Vivier's comment:
- forgot to remove a flag
- Added teset-by to the first patch

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 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Signature · 1 line

2.4.3

Marcel Apfelbaum

author

2016-02-10 13:31 UTC #1 lore raw

PATCH v2 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.

Reported-by Laurent Vivier <[email protected]>

Tested-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 +1 -1

index e096e98..6686b10 100644
@@ -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

Jason Wang

Acked-by on v2 1/2

2016-02-23 03:42 UTC #2 lore raw

Quoted 17 lines · Marcel Apfelbaum

Commits 1811e64c and a6df8adf use the same virtio feature bit 4 for different features.
Fix it by using different bits.
Reported-by Laurent Vivier <[email protected]>
Tested-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
index e096e98..6686b10 100644

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)

Acked-by Jason Wang <[email protected]>

Marcel Apfelbaum

author

2016-02-10 13:31 UTC #3 lore raw

PATCH v2 2/2 hw/virtio: group virtio flags into an enum

In reply to 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 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

hw/virtio/virtio-pci.h +10 -7

index 6686b10..e4548c2 100644
@@ -58,30 +58,33 @@ 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 \
     (1 << VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT)
 
 /* 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

Laurent Vivier

Reviewed-by on v2 2/2

2016-02-10 13:37 UTC #4 lore raw

Re: [Qemu-devel] [PATCH V2 2/2] hw/virtio: group virtio flags into an enum

Quoted 4 lines · Marcel Apfelbaum

Minimizes the possibility to assign
the same bit to different features.
Signed-off-by Marcel Apfelbaum <[email protected]>

Reviewed-by Laurent Vivier <[email protected]>

Jason Wang

Acked-by on v2 2/2

2016-02-23 03:43 UTC #5 lore raw

Re: [Qemu-devel] [PATCH V2 2/2] hw/virtio: group virtio flags into an enum

In reply to Marcel Apfelbaum

Quoted 43 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 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

hw/virtio/virtio-pci.h +10 -5

index 6686b10..e4548c2 100644
@@ -58,30 +58,33 @@ 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 \
     (1 << VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT)
 
 /* 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)

hw/virtio/virtio-pci.h typedef struct VirtioBusClass VirtioPCIBusClass; · line 90

Quoted 10 lines

hw/virtio/virtio-pci.h +0 -2

 
 /* 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)

Acked-by Jason Wang <[email protected]>

Marcel Apfelbaum

2016-03-01 17:01 UTC #6 lore raw

In reply to Marcel Apfelbaum

Quoted 5 lines · Marcel Apfelbaum

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 feature in the same time, and they were both merged in the same pull request. We both saw BIT 3 as the last used

Quoted 9 lines

     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.

ping

Thanks,
Marcel

Quoted 13 lines

v1 -> v2:
- Addressed Laurent Vivier's comment:
- forgot to remove a flag
- Added teset-by to the first patch
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 | 17 ++++++++++-------
  1 file changed, 10 insertions(+), 7 deletions(-)