virtio-pci: add MMIO property

RFC PATCH Superseded Michael S. Tsirkin 5 participants 18 messages Mar 19, 2012 – Jun 18, 2012

Subsystems (2)

KVM virtio core

lore

Share & tools

lore

Michael S. Tsirkin

author maintainer

2012-03-19 15:56 UTC #0 lore raw

RFC PATCH 1/1 virtio-pci: add MMIO property

Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.

While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.

Add a property to make it possible to tweak the BAR type.

Signed-off-by Michael S. Tsirkin <[email protected]>

This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.

---
 hw/virtio-pci.c |   16 ++++++++++++++--
 hw/virtio-pci.h |    4 ++++
 2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 {
     uint8_t *config;
     uint32_t size;
+    uint8_t bar0_type;
 
     proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
     memory_region_init_io(&proxy->bar, &virtio_pci_config_ops, proxy,
                           "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-                     &proxy->bar);
+
+    if (proxy->flags & VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type, &proxy->bar);
 
     if (!kvm_has_many_ioeventfds()) {
         proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
     DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
 static Property virtio_balloon_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
 static Property virtio_scsi_properties[] = {
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1 << VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
 typedef struct {
     PCIDevice pci_dev;
     VirtIODevice *vdev;

Signature · 1 line

1.7.9.111.gf3fb0

Avi Kivity

2012-03-19 17:58 UTC #1 lore raw

Quoted 9 lines · Michael S. Tsirkin

Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.

Quoted 7 lines

Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin <[email protected]>
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.

Should be done via an extra BAR (with the same layout, perhaps extended) so compatibility is preserved.

Signature · 1 line

error compiling committee.c: too many arguments to function

Michael S. Tsirkin

author maintainer

2012-03-19 18:57 UTC #2 lore raw

Quoted 12 lines · Avi Kivity

Quoted 11 lines · Michael S. Tsirkin
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.
Add a property to make it possible to tweak the BAR type.

Quoted 8 lines

Quoted 5 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.
Should be done via an extra BAR (with the same layout, perhaps extended) so compatibility is preserved.

No, that would need guest changes to be of use. The point of this hack is to make things work for Linux guests where PIO does not work.

Quoted 2 lines

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2012-03-20 14:49 UTC #3 lore raw

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines
Should be done via an extra BAR (with the same layout, perhaps extended) so compatibility is preserved.
No, that would need guest changes to be of use. The point of this hack is to make things work for Linux guests where PIO does not work.

It only works if all guest's PCI layer knows to deal with the bit flip correctly. I imagine it isn't true for at least the seabios virtio drivers.

Signature · 1 line

error compiling committee.c: too many arguments to function

Anthony Liguori

2012-03-19 19:19 UTC #4 lore raw

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.

Quoted 6 lines

Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.

Doesn't this violate the virtio-pci spec?

Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.

Regards,

Anthony Liguori

Quoted 90 lines

---
  hw/virtio-pci.c |   16 ++++++++++++++--
  hw/virtio-pci.h |    4 ++++
  2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
  {
      uint8_t *config;
      uint32_t size;
+    uint8_t bar0_type;
 
      proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
      memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                            "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&  VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
      if (!kvm_has_many_ioeventfds()) {
          proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
      DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
      DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
      DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
  static Property virtio_balloon_properties[] = {
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
  static Property virtio_scsi_properties[] = {
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<  VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<  VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
  typedef struct {
      PCIDevice pci_dev;
      VirtIODevice *vdev;

Anthony Liguori

2012-03-19 19:19 UTC #5 lore raw

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.

Quoted 6 lines

Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.

Doesn't this violate the virtio-pci spec?

Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.

Regards,

Anthony Liguori

Quoted 90 lines

---
  hw/virtio-pci.c |   16 ++++++++++++++--
  hw/virtio-pci.h |    4 ++++
  2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
  {
      uint8_t *config;
      uint32_t size;
+    uint8_t bar0_type;
 
      proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
      memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                            "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&  VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
      if (!kvm_has_many_ioeventfds()) {
          proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
      DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
      DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
      DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
  static Property virtio_balloon_properties[] = {
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
  static Property virtio_scsi_properties[] = {
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<  VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<  VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
  typedef struct {
      PCIDevice pci_dev;
      VirtIODevice *vdev;

Michael S. Tsirkin

author maintainer

2012-03-19 20:49 UTC #6 lore raw

Quoted 10 lines · Anthony Liguori

Quoted 9 lines · Michael S. Tsirkin
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.

Quoted 9 lines

Quoted 6 lines
Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.
Doesn't this violate the virtio-pci spec?

The point is to change the BAR type depending on the architecture. IO is fastest on x86 but maybe not on other architectures.

Quoted 6 lines

Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.
Regards,
Anthony Liguori

We do this with MSI-X so why not the BAR type?

Quoted 90 lines

---
 hw/virtio-pci.c |   16 ++++++++++++++--
 hw/virtio-pci.h |    4 ++++
 2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 {
     uint8_t *config;
     uint32_t size;
+    uint8_t bar0_type;
 
     proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
     memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                           "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&  VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
     if (!kvm_has_many_ioeventfds()) {
         proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
     DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
 static Property virtio_balloon_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
 static Property virtio_scsi_properties[] = {
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<  VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<  VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
 typedef struct {
     PCIDevice pci_dev;
     VirtIODevice *vdev;

Anthony Liguori

2012-03-19 21:07 UTC #7 lore raw

Quoted 15 lines · Michael S. Tsirkin

Quoted 14 lines · Anthony Liguori
Quoted 13 lines · Michael S. Tsirkin
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.
Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.
Doesn't this violate the virtio-pci spec?
The point is to change the BAR type depending on the architecture. IO is fastest on x86 but maybe not on other architectures.

Are we going to document that the BAR is X on architecture Y in the spec?

I think the better way to do this is to use a separate device id range for MMIO virtio-pci. You can make the same driver hand both ranges and that way the device is presented consistently to the guest regardless of what the architecture is.

Quoted 8 lines

Quoted 6 lines
Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.
Regards,
Anthony Liguori
We do this with MSI-X so why not the BAR type?

We extend the bar size with MSI-X and use a transport flag to indicate that it's available, right?

Regards,

Anthony LIguori

Quoted 92 lines

Quoted 91 lines


---
  hw/virtio-pci.c |   16 ++++++++++++++--
  hw/virtio-pci.h |    4 ++++
  2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
  {
      uint8_t *config;
      uint32_t size;
+    uint8_t bar0_type;
 
      proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
      memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                            "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&   VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
      if (!kvm_has_many_ioeventfds()) {
          proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
      DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
      DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
      DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
  static Property virtio_balloon_properties[] = {
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
  static Property virtio_scsi_properties[] = {
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<   VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<   VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
  typedef struct {
      PCIDevice pci_dev;
      VirtIODevice *vdev;

Michael S. Tsirkin

author maintainer

2012-03-19 21:20 UTC #8 lore raw

Quoted 25 lines · Anthony Liguori

Quoted 15 lines · Michael S. Tsirkin
Quoted 14 lines · Anthony Liguori
Quoted 13 lines · Michael S. Tsirkin
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.
Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.
Doesn't this violate the virtio-pci spec?
The point is to change the BAR type depending on the architecture. IO is fastest on x86 but maybe not on other architectures.

Quoted 6 lines

Are we going to document that the BAR is X on architecture Y in the spec?
I think the better way to do this is to use a separate device id range for MMIO virtio-pci. You can make the same driver hand both ranges and that way the device is presented consistently to the guest regardless of what the architecture is.

Yes there are endless ways to do this.
This specific hack is good for making existing linux drivers on ppc, arm etc work.

Quoted 11 lines

Quoted 8 lines
Quoted 6 lines
Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.
Regards,
Anthony Liguori
We do this with MSI-X so why not the BAR type?
We extend the bar size with MSI-X and use a transport flag to indicate that it's available, right?

No, we use regular pci capability. Just like BAR type is a regular PCI register :)

Quoted 96 lines

Regards,

Anthony LIguori

Quoted 92 lines

Quoted 91 lines


---
 hw/virtio-pci.c |   16 ++++++++++++++--
 hw/virtio-pci.h |    4 ++++
 2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 {
     uint8_t *config;
     uint32_t size;
+    uint8_t bar0_type;
 
     proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
     memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                           "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&   VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
     if (!kvm_has_many_ioeventfds()) {
         proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
     DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
 static Property virtio_balloon_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
 static Property virtio_scsi_properties[] = {
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<   VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<   VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
 typedef struct {
     PCIDevice pci_dev;
     VirtIODevice *vdev;

Michael S. Tsirkin

author maintainer

2012-03-19 21:29 UTC #9 lore raw

In reply to Anthony Liguori

Quoted 25 lines · Anthony Liguori

Quoted 15 lines · Michael S. Tsirkin
Quoted 14 lines · Anthony Liguori
Quoted 13 lines · Michael S. Tsirkin
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.
Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.
Doesn't this violate the virtio-pci spec?
The point is to change the BAR type depending on the architecture. IO is fastest on x86 but maybe not on other architectures.

Quoted 6 lines

Are we going to document that the BAR is X on architecture Y in the spec?
I think the better way to do this is to use a separate device id range for MMIO virtio-pci. You can make the same driver hand both ranges and that way the device is presented consistently to the guest regardless of what the architecture is.

Maybe just make this a hidden option like x-miio? This will ensure people dont turn it on by mistake on e.g. x86.

Quoted 108 lines

Quoted 8 lines

Quoted 6 lines
Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.
Regards,
Anthony Liguori
We do this with MSI-X so why not the BAR type?

We extend the bar size with MSI-X and use a transport flag to indicate that it's available, right?

Regards,

Anthony LIguori

Quoted 92 lines

Quoted 91 lines


---
 hw/virtio-pci.c |   16 ++++++++++++++--
 hw/virtio-pci.h |    4 ++++
 2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 {
     uint8_t *config;
     uint32_t size;
+    uint8_t bar0_type;
 
     proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
     memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                           "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&   VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
     if (!kvm_has_many_ioeventfds()) {
         proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
     DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
 static Property virtio_balloon_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
 static Property virtio_scsi_properties[] = {
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
     DEFINE_PROP_END_OF_LIST(),
 };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<   VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<   VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
 typedef struct {
     PCIDevice pci_dev;
     VirtIODevice *vdev;

Anthony Liguori

2012-03-19 22:13 UTC #10 lore raw

Quoted 26 lines · Michael S. Tsirkin

Quoted 25 lines · Anthony Liguori
Quoted 23 lines · Michael S. Tsirkin
Quoted 10 lines · Anthony Liguori
Quoted 9 lines · Michael S. Tsirkin
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.
Quoted 9 lines
Quoted 6 lines
Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.
Doesn't this violate the virtio-pci spec?
The point is to change the BAR type depending on the architecture. IO is fastest on x86 but maybe not on other architectures.

Quoted 8 lines

Quoted 6 lines
Are we going to document that the BAR is X on architecture Y in the spec?
I think the better way to do this is to use a separate device id range for MMIO virtio-pci. You can make the same driver hand both ranges and that way the device is presented consistently to the guest regardless of what the architecture is.
Maybe just make this a hidden option like x-miio?

x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power?

Really, aren't we just being too clever here? From a practical perspective, I doubt anyone is ever going to support a driver that has *never* been tested on the platform just because it was accidentally compiled and happens to be there.

If we just do use a device PCI device id range for this, it's a 1-line patch that can be provided via an update to existing guests.

Regards,

Anthony Liguori

Quoted 111 lines

This will ensure people dont turn it on by mistake on e.g. x86.

Quoted 108 lines

Quoted 8 lines

Quoted 6 lines
Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.
Regards,
Anthony Liguori
We do this with MSI-X so why not the BAR type?

We extend the bar size with MSI-X and use a transport flag to indicate that it's available, right?

Regards,

Anthony LIguori

Quoted 92 lines

Quoted 91 lines


---
  hw/virtio-pci.c |   16 ++++++++++++++--
  hw/virtio-pci.h |    4 ++++
  2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
  {
      uint8_t *config;
      uint32_t size;
+    uint8_t bar0_type;
 
      proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
      memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                            "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&    VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
      if (!kvm_has_many_ioeventfds()) {
          proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
      DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
      DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
      DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
  static Property virtio_balloon_properties[] = {
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
  static Property virtio_scsi_properties[] = {
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<    VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<    VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
  typedef struct {
      PCIDevice pci_dev;
      VirtIODevice *vdev;

Anthony Liguori

2012-03-19 22:13 UTC #11 lore raw

In reply to Michael S. Tsirkin

Quoted 26 lines · Michael S. Tsirkin

Quoted 25 lines · Anthony Liguori
Quoted 23 lines · Michael S. Tsirkin
Quoted 10 lines · Anthony Liguori
Quoted 9 lines · Michael S. Tsirkin
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.
Quoted 9 lines
Quoted 6 lines
Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.
Doesn't this violate the virtio-pci spec?
The point is to change the BAR type depending on the architecture. IO is fastest on x86 but maybe not on other architectures.

Quoted 8 lines

Quoted 6 lines
Are we going to document that the BAR is X on architecture Y in the spec?
I think the better way to do this is to use a separate device id range for MMIO virtio-pci. You can make the same driver hand both ranges and that way the device is presented consistently to the guest regardless of what the architecture is.
Maybe just make this a hidden option like x-miio?

x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power?

If we just do use a device PCI device id range for this, it's a 1-line patch that can be provided via an update to existing guests.

Regards,

Anthony Liguori

Quoted 111 lines

This will ensure people dont turn it on by mistake on e.g. x86.

Quoted 108 lines

Quoted 8 lines

Quoted 6 lines
Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.
Regards,
Anthony Liguori
We do this with MSI-X so why not the BAR type?

We extend the bar size with MSI-X and use a transport flag to indicate that it's available, right?

Regards,

Anthony LIguori

Quoted 92 lines

Quoted 91 lines


---
  hw/virtio-pci.c |   16 ++++++++++++++--
  hw/virtio-pci.h |    4 ++++
  2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
  {
      uint8_t *config;
      uint32_t size;
+    uint8_t bar0_type;
 
      proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
      memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                            "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&    VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
      if (!kvm_has_many_ioeventfds()) {
          proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
      DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
      DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
      DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
  static Property virtio_balloon_properties[] = {
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
  static Property virtio_scsi_properties[] = {
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<    VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<    VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
  typedef struct {
      PCIDevice pci_dev;
      VirtIODevice *vdev;

Rusty Russell

2012-03-19 23:52 UTC #12 lore raw

Quoted 3 lines · Anthony Liguori

Quoted 1 line
Maybe just make this a hidden option like x-miio?
x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power?

"To configure the device, we use the first I/O region of the PCI device."

Meh, it does sound a little like we are specifying that it's an PCI I/O bar.

Let's resurrect the PCI-v2 idea, which is ready to implement now, and a nice cleanup? Detach it from the change-of-ring-format idea which is turning out to be a tarpit.

Thanks,
Rusty.

Signature · 1 line

  How could I marry someone with more hair than me?  http://baldalex.org

Anthony Liguori

2012-03-20 00:39 UTC #13 lore raw

Quoted 8 lines · Rusty Russell

Quoted 3 lines · Anthony Liguori
Quoted 1 line
Maybe just make this a hidden option like x-miio?
x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power?
"To configure the device, we use the first I/O region of the PCI device."

Quoted 6 lines

Meh, it does sound a little like we are specifying that it's an PCI I/O bar.
Let's resurrect the PCI-v2 idea, which is ready to implement now, and a nice cleanup? Detach it from the change-of-ring-format idea which is turning out to be a tarpit.

I think that's a sensible approach.

Regards,

Anthony Liguori

Quoted 2 lines

Thanks,
Rusty.

Anthony Liguori

2012-03-20 00:39 UTC #14 lore raw

In reply to Rusty Russell

Quoted 8 lines · Rusty Russell

Quoted 3 lines · Anthony Liguori
Quoted 1 line
Maybe just make this a hidden option like x-miio?
x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power?
"To configure the device, we use the first I/O region of the PCI device."

Quoted 6 lines

Meh, it does sound a little like we are specifying that it's an PCI I/O bar.
Let's resurrect the PCI-v2 idea, which is ready to implement now, and a nice cleanup? Detach it from the change-of-ring-format idea which is turning out to be a tarpit.

I think that's a sensible approach.

Regards,

Anthony Liguori

Quoted 2 lines

Thanks,
Rusty.

Michael S. Tsirkin

author maintainer

2012-06-18 12:05 UTC #15 lore raw

In reply to Rusty Russell

Quoted 11 lines · Rusty Russell

Quoted 3 lines · Anthony Liguori
Quoted 1 line
Maybe just make this a hidden option like x-miio?
x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power?
"To configure the device, we use the first I/O region of the PCI device."
Meh, it does sound a little like we are specifying that it's an PCI I/O bar.

Quoted 6 lines

Let's resurrect the PCI-v2 idea, which is ready to implement now, and a nice cleanup? Detach it from the change-of-ring-format idea which is turning out to be a tarpit.
Thanks,
Rusty.

Yes. But it seems silly to even write code to play with device config in memory when we agreed the right thing to do is to use a config vq everywhere.

Now a question: does a oconfig vq look like a PCI specific feature to you, a work-around for lack of multibyte atomic accesses? If yes it's sane to make it a PCI capability. Or is it something most transports would need? If yes we need a feature bit and this is a chicken and egg problem ...

Quoted 2 lines

Signature · 1 line

  How could I marry someone with more hair than me?  http://baldalex.org

Rusty Russell

2012-03-19 23:52 UTC #16 lore raw

In reply to Anthony Liguori

Quoted 3 lines · Anthony Liguori

Quoted 1 line
Maybe just make this a hidden option like x-miio?
x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power?

"To configure the device, we use the first I/O region of the PCI device."

Meh, it does sound a little like we are specifying that it's an PCI I/O bar.

Let's resurrect the PCI-v2 idea, which is ready to implement now, and a nice cleanup? Detach it from the change-of-ring-format idea which is turning out to be a tarpit.

Thanks,
Rusty.

Signature · 1 line

  How could I marry someone with more hair than me?  http://baldalex.org

Anthony Liguori

2012-03-19 21:07 UTC #17 lore raw

In reply to Michael S. Tsirkin

Quoted 15 lines · Michael S. Tsirkin

Quoted 14 lines · Anthony Liguori
Quoted 13 lines · Michael S. Tsirkin
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs.
While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO.
Add a property to make it possible to tweak the BAR type.
Signed-off-by Michael S. Tsirkin<[email protected]>

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong.
Doesn't this violate the virtio-pci spec?
The point is to change the BAR type depending on the architecture. IO is fastest on x86 but maybe not on other architectures.

Are we going to document that the BAR is X on architecture Y in the spec?

Quoted 8 lines

Quoted 6 lines
Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me.
Regards,
Anthony Liguori
We do this with MSI-X so why not the BAR type?

We extend the bar size with MSI-X and use a transport flag to indicate that it's available, right?

Regards,

Anthony LIguori

Quoted 92 lines

Quoted 91 lines


---
  hw/virtio-pci.c |   16 ++++++++++++++--
  hw/virtio-pci.h |    4 ++++
  2 files changed, 18 insertions(+), 2 deletions(-)

hw/virtio-pci.c +14 -2

index 28498ec..6f338d2 100644
@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
  {
      uint8_t *config;
      uint32_t size;
+    uint8_t bar0_type;
 
      proxy->vdev = vdev;

@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
 
      memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy,
                            "virtio-pci", size);
-    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
-&proxy->bar);
+
+    if (proxy->flags&   VIRTIO_PCI_FLAG_USE_MMIO) {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY;
+    } else {
+        bar0_type = PCI_BASE_ADDRESS_SPACE_IO;
+    }
+
+    pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar);
 
      if (!kvm_has_many_ioeventfds()) {
          proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;

@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = {
      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = {
      DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL),
      DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST),
      DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = {
      DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
      DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = {
 
  static Property virtio_balloon_properties[] = {
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
  static Property virtio_scsi_properties[] = {
      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
      DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false),
      DEFINE_PROP_END_OF_LIST(),
  };

hw/virtio-pci.h +4 -0

index e560428..e6a8861 100644
@@ -24,6 +24,10 @@
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1
  #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1<<   VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
 
+/* Some guests don't support port IO. Use MMIO instead. */
+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2
+#define VIRTIO_PCI_FLAG_USE_MMIO   (1<<   VIRTIO_PCI_FLAG_USE_MMIO_BIT)
+
  typedef struct {
      PCIDevice pci_dev;
      VirtIODevice *vdev;