On 03/19/2012 04:29 PM, Michael S. Tsirkin wrote:

On Mon, Mar 19, 2012 at 04:07:45PM -0500, Anthony Liguori wrote:

quoted

On 03/19/2012 03:49 PM, Michael S. Tsirkin wrote:

quoted

On Mon, Mar 19, 2012 at 02:19:33PM -0500, Anthony Liguori wrote:

quoted

On 03/19/2012 10:56 AM, Michael S. Tsirkin wrote:

quoted

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<mst@redhat.com>

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.

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

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

quoted

quoted

quoted

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

quoted

quoted

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

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 28498ec..6f338d2 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c

@@ -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(),
  };

diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index e560428..e6a8861 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h

@@ -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;