[Qemu-devel] [PATCH V4 00/19] Support more virtio queues

[Qemu-devel] [PATCH V4 00/19] Support more virtio queues

[Jason Wang]

We current limit the max virtio queues to 64. This is not sufficient
to support multiqueue devices (e.g recent Linux support up to 256
tap queues). So this series tries to let virtio to support more queues.

No much works need to be done except:

- Introducing transport specific queue limitation.
- Let each virtio transport to use specific limit.
- Speedup the MSI-X masking and unmasking through per vector queue
  list, and increase the maximum MSI-X vectors supported by qemu.
- With the above enhancements, increase the maximum number of
  virtqueues supported by PCI from 64 to 513.
- Compat the changes for legacy machine types.

With this patch, we can support up to 256 queues. Since x86 can only
allow about 240 interrupt vectors for MSI-X, current Linux driver can
only have about 80 queue pairs has their private MSI-X interrupt
vectors. With sharing IRQ with queue pairs (RFC posted in
https://lkml.org/lkml/2014/12/25/169), Linux driver can have up
to about 186 queue pairs has their private MSI-X interrupt vectors.

Stress/migration test on virtio-pci, compile test on other
targets. And make check on s390x-softmmu and ppc64-softmmu.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Amit Shah <amit.shah@redhat.com>
Cc: qemu-ppc@nongnu.org

Please review

Thanks

Changes from V3:
- rebase to master and target to 2.4
- handling compat issues for spapr
- fixes for hmp command completion when we have a nic with 256 queues
- using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue in
  virtio-ccw
- fix compile issues for ppc64-softmmu
- don't export VIRTIO_CCW_QUEUE_MAX by introducing a gsi limit and
  inherit in ccw
- use transport specific queue limit in virtio-serial
- correct the stale comment for AdapterRoutes and move it to ccw patch
- replace 128 with queue_max + 64 and add a comment for this in
  virtio_ccw_notify()

Changes from V2:
- move transport specific limitation to their implementation. (The
  left is VIRTIO_CCW_QUEUE_MAX since I fail to find a common header
  files other than virtio.h)
- use virtio_get_queue_max() if possible in virtio.c
- AdapterRoutes should use ccw limit
- introduce a vector to queue mapping for virito devices and speedup
  the MSI-X masking and unmasking through this.

Changes from V1:
- add a validation against the bus limitation
- switch to use a bus specific queue limit instead of a global one,
  this will allow us to just increase the limit of one transport
  without disturbing others.
- only increase the queue limit of virtio-pci
- limit the maximum number of virtio queues to 64 for legacy machine
  types

Jason Wang (19):
  pc: add 2.4 machine types
  spapr: add machine type specific instance init function
  ppc: spapr: add 2.4 machine type
  monitor: replace the magic number 255 with MAX_QUEUE_NUM
  monitor: check return value of qemu_find_net_clients_except()
  virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue
  virtio-net: validate backend queue numbers against bus limitation
  virtio-net: fix the upper bound when trying to delete queues
  virito: introduce bus specific queue limit
  virtio-ccw: introduce ccw specific queue limit
  virtio-s390: switch to bus specific queue limit
  virtio-mmio: switch to bus specific queue limit
  virtio-pci: switch to use bus specific queue limit
  virtio: introduce vector to virtqueues mapping
  virtio: introduce virtio_queue_get_index()
  virtio-pci: speedup MSI-X masking and unmasking
  virtio-pci: increase the maximum number of virtqueues to 513
  pci: remove hard-coded bar size in msix_init_exclusive_bar()
  virtio-pci: introduce auto_msix_bar_size property

 hw/block/nvme.c                |  2 +-
 hw/char/virtio-serial-bus.c    |  2 +-
 hw/i386/pc_piix.c              | 42 ++++++++++++++++++++---
 hw/i386/pc_q35.c               | 39 +++++++++++++++++++--
 hw/misc/ivshmem.c              |  2 +-
 hw/net/virtio-net.c            |  9 ++++-
 hw/pci/msix.c                  | 18 ++++------
 hw/ppc/spapr.c                 | 70 +++++++++++++++++++++++++++++++++++--
 hw/s390x/s390-virtio-bus.c     |  7 ++--
 hw/s390x/s390-virtio-ccw.c     |  7 ++--
 hw/s390x/virtio-ccw.c          | 20 +++++++----
 hw/scsi/virtio-scsi.c          |  4 +--
 hw/virtio/virtio-mmio.c        |  7 ++--
 hw/virtio/virtio-pci.c         | 78 ++++++++++++++++++++++++++++--------------
 hw/virtio/virtio-pci.h         |  3 ++
 hw/virtio/virtio.c             | 75 +++++++++++++++++++++++++++++++---------
 include/hw/compat.h            | 11 ++++++
 include/hw/pci/msix.h          |  2 +-
 include/hw/s390x/s390_flic.h   |  4 ++-
 include/hw/virtio/virtio-bus.h |  2 ++
 include/hw/virtio/virtio.h     |  7 ++--
 monitor.c                      | 25 ++++++++------
 22 files changed, 339 insertions(+), 97 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH V4 01/19] pc: add 2.4 machine types

[Jason Wang]

The following patches will limit the following things to legacy
machine type:

- maximum number of virtqueues for virtio-pci were limited to 64
- auto msix bar size for virtio-net-pci were disabled by default

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/pc_piix.c | 29 +++++++++++++++++++++++++----
 hw/i386/pc_q35.c  | 26 +++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 36c69d7..175e582 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -310,8 +310,13 @@ static void pc_init_pci(MachineState *machine)
     pc_init1(machine, 1, 1);
 }
 
+static void pc_compat_2_3(MachineState *machine)
+{
+}
+
 static void pc_compat_2_2(MachineState *machine)
 {
+    pc_compat_2_3(machine);
     rsdp_in_ram = false;
     x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
     x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
@@ -417,6 +422,12 @@ static void pc_compat_1_2(MachineState *machine)
     x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
 }
 
+static void pc_init_pci_2_3(MachineState *machine)
+{
+    pc_compat_2_3(machine);
+    pc_init_pci(machine);
+}
+
 static void pc_init_pci_2_2(MachineState *machine)
 {
     pc_compat_2_2(machine);
@@ -516,19 +527,28 @@ static void pc_xen_hvm_init(MachineState *machine)
     .desc = "Standard PC (i440FX + PIIX, 1996)", \
     .hot_add_cpu = pc_hot_add_cpu
 
-#define PC_I440FX_2_3_MACHINE_OPTIONS                           \
+#define PC_I440FX_2_4_MACHINE_OPTIONS                           \
     PC_I440FX_MACHINE_OPTIONS,                                  \
     .default_machine_opts = "firmware=bios-256k.bin",           \
     .default_display = "std"
 
-static QEMUMachine pc_i440fx_machine_v2_3 = {
-    PC_I440FX_2_3_MACHINE_OPTIONS,
-    .name = "pc-i440fx-2.3",
+
+static QEMUMachine pc_i440fx_machine_v2_4 = {
+    PC_I440FX_2_4_MACHINE_OPTIONS,
+    .name = "pc-i440fx-2.4",
     .alias = "pc",
     .init = pc_init_pci,
     .is_default = 1,
 };
 
+#define PC_I440FX_2_3_MACHINE_OPTIONS PC_I440FX_2_4_MACHINE_OPTIONS
+
+static QEMUMachine pc_i440fx_machine_v2_3 = {
+    PC_I440FX_2_3_MACHINE_OPTIONS,
+    .name = "pc-i440fx-2.3",
+    .init = pc_init_pci_2_3,
+};
+
 #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
 
 static QEMUMachine pc_i440fx_machine_v2_2 = {
@@ -974,6 +994,7 @@ static QEMUMachine xenfv_machine = {
 
 static void pc_machine_init(void)
 {
+    qemu_register_pc_machine(&pc_i440fx_machine_v2_4);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_3);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_2);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_1);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index bc40537..df44d25 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -289,8 +289,13 @@ static void pc_q35_init(MachineState *machine)
     }
 }
 
+static void pc_compat_2_3(MachineState *machine)
+{
+}
+
 static void pc_compat_2_2(MachineState *machine)
 {
+    pc_compat_2_3(machine);
     rsdp_in_ram = false;
     x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
     x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
@@ -365,6 +370,12 @@ static void pc_compat_1_4(MachineState *machine)
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
 }
 
+static void pc_q35_init_2_3(MachineState *machine)
+{
+    pc_compat_2_3(machine);
+    pc_q35_init(machine);
+}
+
 static void pc_q35_init_2_2(MachineState *machine)
 {
     pc_compat_2_2(machine);
@@ -414,16 +425,24 @@ static void pc_q35_init_1_4(MachineState *machine)
     .hot_add_cpu = pc_hot_add_cpu, \
     .units_per_default_bus = 1
 
-#define PC_Q35_2_3_MACHINE_OPTIONS                      \
+#define PC_Q35_2_4_MACHINE_OPTIONS                      \
     PC_Q35_MACHINE_OPTIONS,                             \
     .default_machine_opts = "firmware=bios-256k.bin",   \
     .default_display = "std"
 
+static QEMUMachine pc_q35_machine_v2_4 = {
+    PC_Q35_2_4_MACHINE_OPTIONS,
+    .name = "pc-q35-2.4",
+    .alias = "q35",
+    .init = pc_q35_init,
+};
+
+#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
+
 static QEMUMachine pc_q35_machine_v2_3 = {
     PC_Q35_2_3_MACHINE_OPTIONS,
     .name = "pc-q35-2.3",
-    .alias = "q35",
-    .init = pc_q35_init,
+    .init = pc_q35_init_2_3,
 };
 
 #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
@@ -510,6 +529,7 @@ static QEMUMachine pc_q35_machine_v1_4 = {
 
 static void pc_q35_machine_init(void)
 {
+    qemu_register_pc_machine(&pc_q35_machine_v2_4);
     qemu_register_pc_machine(&pc_q35_machine_v2_3);
     qemu_register_pc_machine(&pc_q35_machine_v2_2);
     qemu_register_pc_machine(&pc_q35_machine_v2_1);
-- 
2.1.0

[Qemu-devel] [PATCH V4 02/19] spapr: add machine type specific instance init function

[Jason Wang]

This patches adds machine type specific instance initialization
functions. Those functions will be used by following patches to compat
class properties for legacy machine types.

Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/ppc/spapr.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0487f52..7fb174f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1803,6 +1803,27 @@ static const TypeInfo spapr_machine_info = {
 #define SPAPR_COMPAT_2_1 \
         SPAPR_COMPAT_2_2
 
+static void spapr_compat_2_2(Object *obj)
+{
+}
+
+static void spapr_compat_2_1(Object *obj)
+{
+    spapr_compat_2_2(obj);
+}
+
+static void spapr_machine_2_2_instance_init(Object *obj)
+{
+    spapr_compat_2_2(obj);
+    spapr_machine_initfn(obj);
+}
+
+static void spapr_machine_2_1_instance_init(Object *obj)
+{
+    spapr_compat_2_1(obj);
+    spapr_machine_initfn(obj);
+}
+
 static void spapr_machine_2_1_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1821,6 +1842,7 @@ static const TypeInfo spapr_machine_2_1_info = {
     .name          = TYPE_SPAPR_MACHINE "2.1",
     .parent        = TYPE_SPAPR_MACHINE,
     .class_init    = spapr_machine_2_1_class_init,
+    .instance_init = spapr_machine_2_1_instance_init,
 };
 
 static void spapr_machine_2_2_class_init(ObjectClass *oc, void *data)
@@ -1840,6 +1862,7 @@ static const TypeInfo spapr_machine_2_2_info = {
     .name          = TYPE_SPAPR_MACHINE "2.2",
     .parent        = TYPE_SPAPR_MACHINE,
     .class_init    = spapr_machine_2_2_class_init,
+    .instance_init = spapr_machine_2_2_instance_init,
 };
 
 static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data)
-- 
2.1.0

[Qemu-devel] [PATCH V4 03/19] ppc: spapr: add 2.4 machine type

[Jason Wang]

The following patches will limit the following things to legacy
machine type:

- maximum number of virtqueues for virtio-pci were limited to 64
- auto msix bar size for virtio-net-pci were disabled by default

Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7fb174f..22f4ae4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1803,8 +1803,13 @@ static const TypeInfo spapr_machine_info = {
 #define SPAPR_COMPAT_2_1 \
         SPAPR_COMPAT_2_2
 
+static void spapr_compat_2_3(Object *obj)
+{
+}
+
 static void spapr_compat_2_2(Object *obj)
 {
+    spapr_compat_2_3(obj);
 }
 
 static void spapr_compat_2_1(Object *obj)
@@ -1812,6 +1817,12 @@ static void spapr_compat_2_1(Object *obj)
     spapr_compat_2_2(obj);
 }
 
+static void spapr_machine_2_3_instance_init(Object *obj)
+{
+    spapr_compat_2_3(obj);
+    spapr_machine_initfn(obj);
+}
+
 static void spapr_machine_2_2_instance_init(Object *obj)
 {
     spapr_compat_2_2(obj);
@@ -1871,14 +1882,29 @@ static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data)
 
     mc->name = "pseries-2.3";
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
-    mc->alias = "pseries";
-    mc->is_default = 1;
 }
 
 static const TypeInfo spapr_machine_2_3_info = {
     .name          = TYPE_SPAPR_MACHINE "2.3",
     .parent        = TYPE_SPAPR_MACHINE,
     .class_init    = spapr_machine_2_3_class_init,
+    .instance_init = spapr_machine_2_3_instance_init,
+};
+
+static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->name = "pseries-2.4";
+    mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4";
+    mc->alias = "pseries";
+    mc->is_default = 1;
+}
+
+static const TypeInfo spapr_machine_2_4_info = {
+    .name          = TYPE_SPAPR_MACHINE "2.4",
+    .parent        = TYPE_SPAPR_MACHINE,
+    .class_init    = spapr_machine_2_4_class_init,
 };
 
 static void spapr_machine_register_types(void)
@@ -1887,6 +1913,7 @@ static void spapr_machine_register_types(void)
     type_register_static(&spapr_machine_2_1_info);
     type_register_static(&spapr_machine_2_2_info);
     type_register_static(&spapr_machine_2_3_info);
+    type_register_static(&spapr_machine_2_4_info);
 }
 
 type_init(spapr_machine_register_types)
-- 
2.1.0

[Qemu-devel] [PATCH V4 04/19] monitor: replace the magic number 255 with MAX_QUEUE_NUM

[Jason Wang]

This patch replace the magic number 255, and increase it to
MAX_QUEUE_NUM which is maximum number of queues supported by a nic.

Cc: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 monitor.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index 42116a9..07dfed0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4475,10 +4475,11 @@ void set_link_completion(ReadLineState *rs, int nb_args, const char *str)
     len = strlen(str);
     readline_set_completion_index(rs, len);
     if (nb_args == 2) {
-        NetClientState *ncs[255];
+        NetClientState *ncs[MAX_QUEUE_NUM];
         int count, i;
         count = qemu_find_net_clients_except(NULL, ncs,
-                                             NET_CLIENT_OPTIONS_KIND_NONE, 255);
+                                             NET_CLIENT_OPTIONS_KIND_NONE,
+                                             MAX_QUEUE_NUM);
         for (i = 0; i < count; i++) {
             const char *name = ncs[i]->name;
             if (!strncmp(str, name, len)) {
@@ -4494,7 +4495,7 @@ void set_link_completion(ReadLineState *rs, int nb_args, const char *str)
 void netdev_del_completion(ReadLineState *rs, int nb_args, const char *str)
 {
     int len, count, i;
-    NetClientState *ncs[255];
+    NetClientState *ncs[MAX_QUEUE_NUM];
 
     if (nb_args != 2) {
         return;
@@ -4503,7 +4504,7 @@ void netdev_del_completion(ReadLineState *rs, int nb_args, const char *str)
     len = strlen(str);
     readline_set_completion_index(rs, len);
     count = qemu_find_net_clients_except(NULL, ncs, NET_CLIENT_OPTIONS_KIND_NIC,
-                                         255);
+                                         MAX_QUEUE_NUM);
     for (i = 0; i < count; i++) {
         QemuOpts *opts;
         const char *name = ncs[i]->name;
@@ -4569,14 +4570,15 @@ void host_net_add_completion(ReadLineState *rs, int nb_args, const char *str)
 
 void host_net_remove_completion(ReadLineState *rs, int nb_args, const char *str)
 {
-    NetClientState *ncs[255];
+    NetClientState *ncs[MAX_QUEUE_NUM];
     int count, i, len;
 
     len = strlen(str);
     readline_set_completion_index(rs, len);
     if (nb_args == 2) {
         count = qemu_find_net_clients_except(NULL, ncs,
-                                             NET_CLIENT_OPTIONS_KIND_NONE, 255);
+                                             NET_CLIENT_OPTIONS_KIND_NONE,
+                                             MAX_QUEUE_NUM);
         for (i = 0; i < count; i++) {
             int id;
             char name[16];
@@ -4592,7 +4594,8 @@ void host_net_remove_completion(ReadLineState *rs, int nb_args, const char *str)
         return;
     } else if (nb_args == 3) {
         count = qemu_find_net_clients_except(NULL, ncs,
-                                             NET_CLIENT_OPTIONS_KIND_NIC, 255);
+                                             NET_CLIENT_OPTIONS_KIND_NIC,
+                                             MAX_QUEUE_NUM);
         for (i = 0; i < count; i++) {
             int id;
             const char *name;
-- 
2.1.0

[Qemu-devel] [PATCH V4 05/19] monitor: check return value of qemu_find_net_clients_except()

[Jason Wang]

qemu_find_net_clients_except() may return a value which is greater
than the size of array we provided. So we should check this value
before using it, otherwise this may cause unexpected memory access.

This patch fixes the net related command completion when we have a
virtio-net nic with more than 255 queues.

Cc: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 monitor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 07dfed0..3c0abfd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4480,7 +4480,7 @@ void set_link_completion(ReadLineState *rs, int nb_args, const char *str)
         count = qemu_find_net_clients_except(NULL, ncs,
                                              NET_CLIENT_OPTIONS_KIND_NONE,
                                              MAX_QUEUE_NUM);
-        for (i = 0; i < count; i++) {
+        for (i = 0; i < MIN(count, MAX_QUEUE_NUM); i++) {
             const char *name = ncs[i]->name;
             if (!strncmp(str, name, len)) {
                 readline_add_completion(rs, name);
@@ -4505,7 +4505,7 @@ void netdev_del_completion(ReadLineState *rs, int nb_args, const char *str)
     readline_set_completion_index(rs, len);
     count = qemu_find_net_clients_except(NULL, ncs, NET_CLIENT_OPTIONS_KIND_NIC,
                                          MAX_QUEUE_NUM);
-    for (i = 0; i < count; i++) {
+    for (i = 0; i < MIN(count, MAX_QUEUE_NUM); i++) {
         QemuOpts *opts;
         const char *name = ncs[i]->name;
         if (strncmp(str, name, len)) {
@@ -4579,7 +4579,7 @@ void host_net_remove_completion(ReadLineState *rs, int nb_args, const char *str)
         count = qemu_find_net_clients_except(NULL, ncs,
                                              NET_CLIENT_OPTIONS_KIND_NONE,
                                              MAX_QUEUE_NUM);
-        for (i = 0; i < count; i++) {
+        for (i = 0; i < MIN(count, MAX_QUEUE_NUM); i++) {
             int id;
             char name[16];
 
@@ -4596,7 +4596,7 @@ void host_net_remove_completion(ReadLineState *rs, int nb_args, const char *str)
         count = qemu_find_net_clients_except(NULL, ncs,
                                              NET_CLIENT_OPTIONS_KIND_NIC,
                                              MAX_QUEUE_NUM);
-        for (i = 0; i < count; i++) {
+        for (i = 0; i < MIN(count, MAX_QUEUE_NUM); i++) {
             int id;
             const char *name;
 
-- 
2.1.0

[Qemu-devel] [PATCH V4 06/19] virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue

[Jason Wang]

There's no need to use vector 0 for invalid virtqueue. So this patch
changes to use VIRTIO_NO_VECTOR instead.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/s390x/virtio-ccw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 130535c..c8b87aa 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -281,7 +281,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
 
     virtio_queue_set_addr(vdev, index, addr);
     if (!addr) {
-        virtio_queue_set_vector(vdev, index, 0);
+        virtio_queue_set_vector(vdev, index, VIRTIO_NO_VECTOR);
     } else {
         /* Fail if we don't have a big enough queue. */
         /* TODO: Add interface to handle vring.num changing */
-- 
2.1.0

[Qemu-devel] [PATCH V4 07/19] virtio-net: validate backend queue numbers against bus limitation

[Jason Wang]

We don't validate the backend queue numbers against bus limitation,
this will easily crash qemu if it exceeds the limitation. Fixing this
by doing the validation and fail early.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 27adcc5..59f76bc 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1588,6 +1588,13 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
 
     n->max_queues = MAX(n->nic_conf.peers.queues, 1);
+    if (n->max_queues * 2 + 1 > VIRTIO_PCI_QUEUE_MAX) {
+        error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
+                   "must be a postive integer less than %d.",
+                   n->max_queues, (VIRTIO_PCI_QUEUE_MAX - 1) / 2);
+        virtio_cleanup(vdev);
+        return;
+    }
     n->vqs = g_malloc0(sizeof(VirtIONetQueue) * n->max_queues);
     n->vqs[0].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx);
     n->curr_queues = 1;
-- 
2.1.0

[Qemu-devel] [PATCH V4 08/19] virtio-net: fix the upper bound when trying to delete queues

[Jason Wang]

Virtqueue were indexed from zero, so don't delete virtqueue whose
index is n->max_queues * 2 + 1.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 59f76bc..b6fac9c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1309,7 +1309,7 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
 
     n->multiqueue = multiqueue;
 
-    for (i = 2; i <= n->max_queues * 2 + 1; i++) {
+    for (i = 2; i < n->max_queues * 2 + 1; i++) {
         virtio_del_queue(vdev, i);
     }
 
-- 
2.1.0

[Qemu-devel] [PATCH V4 09/19] virito: introduce bus specific queue limit

[Jason Wang]

This patch introduces a bus specific queue limitation. It will be
useful for increasing the limit for one of the bus without disturbing
other buses.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/char/virtio-serial-bus.c    |  2 +-
 hw/net/virtio-net.c            |  4 ++--
 hw/s390x/s390-virtio-bus.c     |  1 +
 hw/s390x/virtio-ccw.c          |  1 +
 hw/scsi/virtio-scsi.c          |  4 ++--
 hw/virtio/virtio-mmio.c        |  1 +
 hw/virtio/virtio-pci.c         |  1 +
 hw/virtio/virtio.c             | 40 +++++++++++++++++++++++++---------------
 include/hw/virtio/virtio-bus.h |  1 +
 include/hw/virtio/virtio.h     |  1 +
 10 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index c86814f..dec2f2d 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -942,7 +942,7 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
     }
 
     /* Each port takes 2 queues, and one pair is for the control queue */
-    max_supported_ports = VIRTIO_PCI_QUEUE_MAX / 2 - 1;
+    max_supported_ports = virtio_get_queue_max(vdev) / 2 - 1;
 
     if (vser->serial.max_virtserial_ports > max_supported_ports) {
         error_setg(errp, "maximum ports supported: %u", max_supported_ports);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index b6fac9c..bf286f5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1588,10 +1588,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
 
     n->max_queues = MAX(n->nic_conf.peers.queues, 1);
-    if (n->max_queues * 2 + 1 > VIRTIO_PCI_QUEUE_MAX) {
+    if (n->max_queues * 2 + 1 > virtio_get_queue_max(vdev)) {
         error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
                    "must be a postive integer less than %d.",
-                   n->max_queues, (VIRTIO_PCI_QUEUE_MAX - 1) / 2);
+                   n->max_queues, (virtio_get_queue_max(vdev) - 1) / 2);
         virtio_cleanup(vdev);
         return;
     }
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 047c963..2b41e32 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -715,6 +715,7 @@ static void virtio_s390_bus_class_init(ObjectClass *klass, void *data)
     bus_class->max_dev = 1;
     k->notify = virtio_s390_notify;
     k->get_features = virtio_s390_get_features;
+    k->queue_max = VIRTIO_PCI_QUEUE_MAX;
 }
 
 static const TypeInfo virtio_s390_bus_info = {
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index c8b87aa..dfe6ded 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1711,6 +1711,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->load_queue = virtio_ccw_load_queue;
     k->save_config = virtio_ccw_save_config;
     k->load_config = virtio_ccw_load_config;
+    k->queue_max = VIRTIO_PCI_QUEUE_MAX;
 }
 
 static const TypeInfo virtio_ccw_bus_info = {
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index da0cff8..feb5e07 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -822,10 +822,10 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
                 sizeof(VirtIOSCSIConfig));
 
     if (s->conf.num_queues == 0 ||
-            s->conf.num_queues > VIRTIO_PCI_QUEUE_MAX - 2) {
+            s->conf.num_queues > virtio_get_queue_max(vdev) - 2) {
         error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
                          "must be a positive integer less than %d.",
-                   s->conf.num_queues, VIRTIO_PCI_QUEUE_MAX - 2);
+                   s->conf.num_queues, virtio_get_queue_max(vdev) - 2);
         virtio_cleanup(vdev);
         return;
     }
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 10123f3..2ae6942 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -403,6 +403,7 @@ static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data)
     k->device_plugged = virtio_mmio_device_plugged;
     k->has_variable_vring_alignment = true;
     bus_class->max_dev = 1;
+    k->queue_max = VIRTIO_PCI_QUEUE_MAX;
 }
 
 static const TypeInfo virtio_mmio_bus_info = {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c7c3f72..075b13b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1498,6 +1498,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->vmstate_change = virtio_pci_vmstate_change;
     k->device_plugged = virtio_pci_device_plugged;
     k->device_unplugged = virtio_pci_device_unplugged;
+    k->queue_max = VIRTIO_PCI_QUEUE_MAX;
 }
 
 static const TypeInfo virtio_pci_bus_info = {
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3c6e430..023eb04 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -541,6 +541,14 @@ void virtio_update_irq(VirtIODevice *vdev)
     virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
 }
 
+int virtio_get_queue_max(VirtIODevice *vdev)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+
+    return k->queue_max;
+}
+
 void virtio_set_status(VirtIODevice *vdev, uint8_t val)
 {
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
@@ -599,7 +607,7 @@ void virtio_reset(void *opaque)
     vdev->config_vector = VIRTIO_NO_VECTOR;
     virtio_notify_vector(vdev, vdev->config_vector);
 
-    for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+    for (i = 0; i < virtio_get_queue_max(vdev); i++) {
         vdev->vq[i].vring.desc = 0;
         vdev->vq[i].vring.avail = 0;
         vdev->vq[i].vring.used = 0;
@@ -738,7 +746,8 @@ int virtio_queue_get_num(VirtIODevice *vdev, int n)
 int virtio_queue_get_id(VirtQueue *vq)
 {
     VirtIODevice *vdev = vq->vdev;
-    assert(vq >= &vdev->vq[0] && vq < &vdev->vq[VIRTIO_PCI_QUEUE_MAX]);
+
+    assert(vq >= &vdev->vq[0] && vq < &vdev->vq[virtio_get_queue_max(vdev)]);
     return vq - &vdev->vq[0];
 }
 
@@ -773,28 +782,29 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
 
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
 {
-    return n < VIRTIO_PCI_QUEUE_MAX ? vdev->vq[n].vector :
+    return n < virtio_get_queue_max(vdev) ? vdev->vq[n].vector :
         VIRTIO_NO_VECTOR;
 }
 
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector)
 {
-    if (n < VIRTIO_PCI_QUEUE_MAX)
+    if (n < virtio_get_queue_max(vdev))
         vdev->vq[n].vector = vector;
 }
 
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             void (*handle_output)(VirtIODevice *, VirtQueue *))
 {
-    int i;
+    int i, queue_max = virtio_get_queue_max(vdev);
 
-    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+    for (i = 0; i < queue_max; i++) {
         if (vdev->vq[i].vring.num == 0)
             break;
     }
 
-    if (i == VIRTIO_PCI_QUEUE_MAX || queue_size > VIRTQUEUE_MAX_SIZE)
+    if (i == queue_max || queue_size > VIRTQUEUE_MAX_SIZE) {
         abort();
+    }
 
     vdev->vq[i].vring.num = queue_size;
     vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
@@ -805,7 +815,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
 
 void virtio_del_queue(VirtIODevice *vdev, int n)
 {
-    if (n < 0 || n >= VIRTIO_PCI_QUEUE_MAX) {
+    if (n < 0 || n >= virtio_get_queue_max(vdev)) {
         abort();
     }
 
@@ -915,14 +925,14 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_be32(f, vdev->config_len);
     qemu_put_buffer(f, vdev->config, vdev->config_len);
 
-    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+    for (i = 0; i < k->queue_max; i++) {
         if (vdev->vq[i].vring.num == 0)
             break;
     }
 
     qemu_put_be32(f, i);
 
-    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+    for (i = 0; i < k->queue_max; i++) {
         if (vdev->vq[i].vring.num == 0)
             break;
 
@@ -987,7 +997,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     qemu_get_8s(f, &vdev->status);
     qemu_get_8s(f, &vdev->isr);
     qemu_get_be16s(f, &vdev->queue_sel);
-    if (vdev->queue_sel >= VIRTIO_PCI_QUEUE_MAX) {
+    if (vdev->queue_sel >= k->queue_max) {
         return -1;
     }
     qemu_get_be32s(f, &features);
@@ -1014,7 +1024,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
 
     num = qemu_get_be32(f);
 
-    if (num > VIRTIO_PCI_QUEUE_MAX) {
+    if (num > k->queue_max) {
         error_report("Invalid number of PCI queues: 0x%x", num);
         return -1;
     }
@@ -1124,15 +1134,15 @@ void virtio_instance_init_common(Object *proxy_obj, void *data,
 void virtio_init(VirtIODevice *vdev, const char *name,
                  uint16_t device_id, size_t config_size)
 {
-    int i;
+    int i, queue_max = virtio_get_queue_max(vdev);
     vdev->device_id = device_id;
     vdev->status = 0;
     vdev->isr = 0;
     vdev->queue_sel = 0;
     vdev->config_vector = VIRTIO_NO_VECTOR;
-    vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
+    vdev->vq = g_malloc0(sizeof(VirtQueue) * queue_max);
     vdev->vm_running = runstate_is_running();
-    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+    for (i = 0; i < queue_max; i++) {
         vdev->vq[i].vector = VIRTIO_NO_VECTOR;
         vdev->vq[i].vdev = vdev;
         vdev->vq[i].queue_index = i;
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 0d2e7b4..4da8022 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -68,6 +68,7 @@ typedef struct VirtioBusClass {
      * Note that changing this will break migration for this transport.
      */
     bool has_variable_vring_alignment;
+    uint16_t queue_max;
 } VirtioBusClass;
 
 struct VirtioBusState {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d95f8b6..91fd673 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -179,6 +179,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
 void virtio_set_status(VirtIODevice *vdev, uint8_t val);
+int virtio_get_queue_max(VirtIODevice *vdev);
 void virtio_reset(void *opaque);
 void virtio_update_irq(VirtIODevice *vdev);
 int virtio_set_features(VirtIODevice *vdev, uint32_t val);
-- 
2.1.0

[Qemu-devel] [PATCH V4 10/19] virtio-ccw: introduce ccw specific queue limit

[Jason Wang]

Instead of depending on marco, using a bus specific limit.

Cc: Alexander Graf <agraf@suse.de>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c   |  7 +++++--
 hw/s390x/virtio-ccw.c        | 19 ++++++++++++-------
 include/hw/s390x/s390_flic.h |  4 +++-
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index afb539a..96b84ab 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -55,6 +55,7 @@ void io_subsystem_reset(void)
 
 static int virtio_ccw_hcall_notify(const uint64_t *args)
 {
+    VirtIODevice *vdev;
     uint64_t subch_id = args[0];
     uint64_t queue = args[1];
     SubchDev *sch;
@@ -67,10 +68,12 @@ static int virtio_ccw_hcall_notify(const uint64_t *args)
     if (!sch || !css_subch_visible(sch)) {
         return -EINVAL;
     }
-    if (queue >= VIRTIO_PCI_QUEUE_MAX) {
+
+    vdev = virtio_ccw_get_vdev(sch);
+    if (queue >= virtio_get_queue_max(vdev)) {
         return -EINVAL;
     }
-    virtio_queue_notify(virtio_ccw_get_vdev(sch), queue);
+    virtio_queue_notify(vdev, queue);
     return 0;
 
 }
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index dfe6ded..935b880 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -32,6 +32,8 @@
 static QTAILQ_HEAD(, IndAddr) indicator_addresses =
     QTAILQ_HEAD_INITIALIZER(indicator_addresses);
 
+#define VIRTIO_CCW_QUEUE_MAX ADAPTER_ROUTES_MAX_GSI
+
 static IndAddr *get_indicator(hwaddr ind_addr, int len)
 {
     IndAddr *indicator;
@@ -170,7 +172,7 @@ static void virtio_ccw_start_ioeventfd(VirtioCcwDevice *dev)
         return;
     }
     vdev = virtio_bus_get_device(&dev->bus);
-    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+    for (n = 0; n < virtio_get_queue_max(vdev); n++) {
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
@@ -205,7 +207,7 @@ static void virtio_ccw_stop_ioeventfd(VirtioCcwDevice *dev)
         return;
     }
     vdev = virtio_bus_get_device(&dev->bus);
-    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+    for (n = 0; n < virtio_get_queue_max(vdev); n++) {
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
@@ -265,8 +267,9 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
                               uint16_t index, uint16_t num)
 {
     VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
+    int queue_max = virtio_get_queue_max(vdev);
 
-    if (index > VIRTIO_PCI_QUEUE_MAX) {
+    if (index > queue_max) {
         return -EINVAL;
     }
 
@@ -291,7 +294,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
         virtio_queue_set_vector(vdev, index, index);
     }
     /* tell notify handler in case of config change */
-    vdev->config_vector = VIRTIO_PCI_QUEUE_MAX;
+    vdev->config_vector = queue_max;
     return 0;
 }
 
@@ -1036,14 +1039,16 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
 static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
 {
     VirtioCcwDevice *dev = to_virtio_ccw_dev_fast(d);
+    VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
     SubchDev *sch = dev->sch;
     uint64_t indicators;
 
-    if (vector >= 128) {
+    /* queue indicators + secondary indicators */
+    if (vector >= virtio_get_queue_max(vdev) + 64) {
         return;
     }
 
-    if (vector < VIRTIO_PCI_QUEUE_MAX) {
+    if (vector < virtio_get_queue_max(vdev)) {
         if (!dev->indicators) {
             return;
         }
@@ -1711,7 +1716,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->load_queue = virtio_ccw_load_queue;
     k->save_config = virtio_ccw_save_config;
     k->load_config = virtio_ccw_load_config;
-    k->queue_max = VIRTIO_PCI_QUEUE_MAX;
+    k->queue_max = VIRTIO_CCW_QUEUE_MAX;
 }
 
 static const TypeInfo virtio_ccw_bus_info = {
diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
index 489d73b..e1b0389 100644
--- a/include/hw/s390x/s390_flic.h
+++ b/include/hw/s390x/s390_flic.h
@@ -17,10 +17,12 @@
 #include "hw/s390x/adapter.h"
 #include "hw/virtio/virtio.h"
 
+#define ADAPTER_ROUTES_MAX_GSI 64
+
 typedef struct AdapterRoutes {
     AdapterInfo adapter;
     int num_routes;
-    int gsi[VIRTIO_PCI_QUEUE_MAX];
+    int gsi[ADAPTER_ROUTES_MAX_GSI];
 } AdapterRoutes;
 
 #define TYPE_S390_FLIC_COMMON "s390-flic"
-- 
2.1.0

[Qemu-devel] [PATCH V4 11/19] virtio-s390: switch to bus specific queue limit

[Jason Wang]

Instead of depending on marco, switch to use a bus specific queue
limit.

Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/s390x/s390-virtio-bus.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 2b41e32..9d6028d 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -45,6 +45,8 @@
     do { } while (0)
 #endif
 
+#define VIRTIO_S390_QUEUE_MAX 64
+
 static void virtio_s390_bus_new(VirtioBusState *bus, size_t bus_size,
                                 VirtIOS390Device *dev);
 
@@ -344,7 +346,7 @@ static ram_addr_t s390_virtio_device_num_vq(VirtIOS390Device *dev)
     VirtIODevice *vdev = dev->vdev;
     int num_vq;
 
-    for (num_vq = 0; num_vq < VIRTIO_PCI_QUEUE_MAX; num_vq++) {
+    for (num_vq = 0; num_vq < virtio_get_queue_max(vdev); num_vq++) {
         if (!virtio_queue_get_num(vdev, num_vq)) {
             break;
         }
@@ -446,7 +448,7 @@ VirtIOS390Device *s390_virtio_bus_find_vring(VirtIOS390Bus *bus,
     QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
         VirtIOS390Device *dev = (VirtIOS390Device *)kid->child;
 
-        for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+        for (i = 0; i < virtio_get_queue_max(dev->vdev); i++) {
             if (!virtio_queue_get_addr(dev->vdev, i))
                 break;
             if (virtio_queue_get_addr(dev->vdev, i) == mem) {
@@ -715,7 +717,7 @@ static void virtio_s390_bus_class_init(ObjectClass *klass, void *data)
     bus_class->max_dev = 1;
     k->notify = virtio_s390_notify;
     k->get_features = virtio_s390_get_features;
-    k->queue_max = VIRTIO_PCI_QUEUE_MAX;
+    k->queue_max = VIRTIO_S390_QUEUE_MAX;
 }
 
 static const TypeInfo virtio_s390_bus_info = {
-- 
2.1.0

[Qemu-devel] [PATCH V4 12/19] virtio-mmio: switch to bus specific queue limit

[Jason Wang]

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-mmio.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 2ae6942..dbd44b6 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -34,6 +34,8 @@ do { printf("virtio_mmio: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do {} while (0)
 #endif
 
+#define VIRTIO_MMIO_QUEUE_MAX 64
+
 /* QOM macros */
 /* virtio-mmio-bus */
 #define TYPE_VIRTIO_MMIO_BUS "virtio-mmio-bus"
@@ -237,7 +239,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
                 proxy->guest_page_shift);
         break;
     case VIRTIO_MMIO_QUEUESEL:
-        if (value < VIRTIO_PCI_QUEUE_MAX) {
+        if (value < virtio_get_queue_max(vdev)) {
             vdev->queue_sel = value;
         }
         break;
@@ -257,7 +259,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
         }
         break;
     case VIRTIO_MMIO_QUEUENOTIFY:
-        if (value < VIRTIO_PCI_QUEUE_MAX) {
+        if (value < virtio_get_queue_max(vdev)) {
             virtio_queue_notify(vdev, value);
         }
         break;
@@ -403,7 +405,7 @@ static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data)
     k->device_plugged = virtio_mmio_device_plugged;
     k->has_variable_vring_alignment = true;
     bus_class->max_dev = 1;
-    k->queue_max = VIRTIO_PCI_QUEUE_MAX;
+    k->queue_max = VIRTIO_MMIO_QUEUE_MAX;
 }
 
 static const TypeInfo virtio_mmio_bus_info = {
-- 
2.1.0

[Qemu-devel] [PATCH V4 13/19] virtio-pci: switch to use bus specific queue limit

[Jason Wang]

Instead of depending on a macro, switch to use a bus specific queue
limit.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-pci.c     | 12 +++++++-----
 include/hw/virtio/virtio.h |  2 --
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 075b13b..e556919 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -42,6 +42,8 @@
  * configuration space */
 #define VIRTIO_PCI_CONFIG_SIZE(dev)     VIRTIO_PCI_CONFIG_OFF(msix_enabled(dev))
 
+#define VIRTIO_PCI_QUEUE_MAX 64
+
 static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtIOPCIProxy *dev);
 
@@ -171,7 +173,7 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
         return;
     }
 
-    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+    for (n = 0; n < virtio_get_queue_max(vdev); n++) {
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
@@ -207,7 +209,7 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
         return;
     }
 
-    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+    for (n = 0; n < virtio_get_queue_max(vdev); n++) {
         if (!virtio_queue_get_num(vdev, n)) {
             continue;
         }
@@ -243,11 +245,11 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             virtio_queue_set_addr(vdev, vdev->queue_sel, pa);
         break;
     case VIRTIO_PCI_QUEUE_SEL:
-        if (val < VIRTIO_PCI_QUEUE_MAX)
+        if (val < virtio_get_queue_max(vdev))
             vdev->queue_sel = val;
         break;
     case VIRTIO_PCI_QUEUE_NOTIFY:
-        if (val < VIRTIO_PCI_QUEUE_MAX) {
+        if (val < virtio_get_queue_max(vdev)) {
             virtio_queue_notify(vdev, val);
         }
         break;
@@ -748,7 +750,7 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
     bool with_irqfd = msix_enabled(&proxy->pci_dev) &&
         kvm_msi_via_irqfd_enabled();
 
-    nvqs = MIN(nvqs, VIRTIO_PCI_QUEUE_MAX);
+    nvqs = MIN(nvqs, virtio_get_queue_max(vdev));
 
     /* When deassigning, pass a consistent nvqs value
      * to avoid leaking notifiers.
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 91fd673..7ff40ac 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -48,8 +48,6 @@ typedef struct VirtQueueElement
     struct iovec out_sg[VIRTQUEUE_MAX_SIZE];
 } VirtQueueElement;
 
-#define VIRTIO_PCI_QUEUE_MAX 64
-
 #define VIRTIO_NO_VECTOR 0xffff
 
 #define TYPE_VIRTIO_DEVICE "virtio-device"
-- 
2.1.0

[Qemu-devel] [PATCH V4 14/19] virtio: introduce vector to virtqueues mapping

[Jason Wang]

Currently we will try to traverse all virtqueues to find a subset that
using a specific vector. This is sub optimal when we will support
hundreds or even thousands of virtqueues. So this patch introduces a
method which could be used by transport to get all virtqueues that
using a same vector. This is done through QLISTs and the number of
QLISTs was queried through a transport specific method. When guest
setting vectors, the virtqueue will be linked and helpers for traverse
the list was also introduced.

The first user will be virtio pci which will use this to speed up
MSI-X masking and unmasking handling.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-pci.c         |  8 ++++++++
 hw/virtio/virtio.c             | 32 ++++++++++++++++++++++++++++++--
 include/hw/virtio/virtio-bus.h |  1 +
 include/hw/virtio/virtio.h     |  3 +++
 4 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e556919..c38f33f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -910,6 +910,13 @@ static const TypeInfo virtio_9p_pci_info = {
  * virtio-pci: This is the PCIDevice which has a virtio-pci-bus.
  */
 
+static int virtio_pci_query_nvectors(DeviceState *d)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+
+    return proxy->nvectors;
+}
+
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_pci_device_plugged(DeviceState *d)
 {
@@ -1500,6 +1507,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->vmstate_change = virtio_pci_vmstate_change;
     k->device_plugged = virtio_pci_device_plugged;
     k->device_unplugged = virtio_pci_device_unplugged;
+    k->query_nvectors = virtio_pci_query_nvectors;
     k->queue_max = VIRTIO_PCI_QUEUE_MAX;
 }
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 023eb04..ca157e8 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -89,6 +89,7 @@ struct VirtQueue
     VirtIODevice *vdev;
     EventNotifier guest_notifier;
     EventNotifier host_notifier;
+    QLIST_ENTRY(VirtQueue) node;
 };
 
 /* virt queue functions */
@@ -613,7 +614,7 @@ void virtio_reset(void *opaque)
         vdev->vq[i].vring.used = 0;
         vdev->vq[i].last_avail_idx = 0;
         vdev->vq[i].pa = 0;
-        vdev->vq[i].vector = VIRTIO_NO_VECTOR;
+        virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
         vdev->vq[i].signalled_used = 0;
         vdev->vq[i].signalled_used_valid = false;
         vdev->vq[i].notification = true;
@@ -738,6 +739,16 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
     virtqueue_init(&vdev->vq[n]);
 }
 
+VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector)
+{
+    return QLIST_FIRST(&vdev->vector_queues[vector]);
+}
+
+VirtQueue *virtio_vector_next_queue(VirtQueue *vq)
+{
+    return QLIST_NEXT(vq, node);
+}
+
 int virtio_queue_get_num(VirtIODevice *vdev, int n)
 {
     return vdev->vq[n].vring.num;
@@ -788,8 +799,17 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
 
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector)
 {
-    if (n < virtio_get_queue_max(vdev))
+    VirtQueue *vq = &vdev->vq[n];
+
+    if (n < virtio_get_queue_max(vdev)) {
+        if (vdev->vq[n].vector != VIRTIO_NO_VECTOR) {
+            QLIST_REMOVE(vq, node);
+        }
         vdev->vq[n].vector = vector;
+        if (vector != VIRTIO_NO_VECTOR) {
+            QLIST_INSERT_HEAD(&vdev->vector_queues[vector], vq, node);
+        }
+    }
 }
 
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
@@ -1097,6 +1117,7 @@ void virtio_cleanup(VirtIODevice *vdev)
     qemu_del_vm_change_state_handler(vdev->vmstate);
     g_free(vdev->config);
     g_free(vdev->vq);
+    g_free(vdev->vector_queues);
 }
 
 static void virtio_vmstate_change(void *opaque, int running, RunState state)
@@ -1134,7 +1155,14 @@ void virtio_instance_init_common(Object *proxy_obj, void *data,
 void virtio_init(VirtIODevice *vdev, const char *name,
                  uint16_t device_id, size_t config_size)
 {
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     int i, queue_max = virtio_get_queue_max(vdev);
+    int nvectors = k->query_nvectors ?
+        k->query_nvectors(qbus->parent) : queue_max;
+
+    vdev->vector_queues = g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
+
     vdev->device_id = device_id;
     vdev->status = 0;
     vdev->isr = 0;
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 4da8022..12386d5 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -62,6 +62,7 @@ typedef struct VirtioBusClass {
      * This is called by virtio-bus just before the device is unplugged.
      */
     void (*device_unplugged)(DeviceState *d);
+    int (*query_nvectors)(DeviceState *d);
     /*
      * Does the transport have variable vring alignment?
      * (ie can it ever call virtio_queue_set_align()?)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 7ff40ac..e3adb1d 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -82,6 +82,7 @@ struct VirtIODevice
     VMChangeStateEntry *vmstate;
     char *bus_name;
     uint8_t device_endian;
+    QLIST_HEAD(, VirtQueue) * vector_queues;
 };
 
 typedef struct VirtioDeviceClass {
@@ -217,6 +218,8 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
+VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
+VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
 
 static inline void virtio_add_feature(uint32_t *features, unsigned int fbit)
 {
-- 
2.1.0

[Qemu-devel] [PATCH V4 15/19] virtio: introduce virtio_queue_get_index()

[Jason Wang]

This patch introduces a helper that can get the queue index of a
VirtQueue. This is useful when traversing the list of VirtQueues.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio.c         | 5 +++++
 include/hw/virtio/virtio.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ca157e8..d4c49ef 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -725,6 +725,11 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
     return vdev->vq[n].pa;
 }
 
+int virtio_queue_get_index(VirtIODevice *vdev, VirtQueue *vq)
+{
+    return vq - vdev->vq;
+}
+
 void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
 {
     /* Don't allow guest to flip queue between existent and
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index e3adb1d..f148590 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -171,6 +171,7 @@ void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data);
 void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data);
 void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
 hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
+int virtio_queue_get_index(VirtIODevice *vdev, VirtQueue *vq);
 void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
 int virtio_queue_get_num(VirtIODevice *vdev, int n);
 void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
-- 
2.1.0

[Qemu-devel] [PATCH V4 16/19] virtio-pci: speedup MSI-X masking and unmasking

[Jason Wang]

This patch tries to speed up the MSI-X masking and unmasking through
the mapping between vector and queues. With this patch it will there's
no need to go through all possible virtqueues, which may help to
reduce the time spent when doing MSI-X masking/unmasking a single
vector when more than hundreds or even thousands of virtqueues were
supported.

Tested with 80 queue pairs virito-net-pci by changing the smp affinity
in the background and doing netperf in the same time:

Before the patch:
5711.70 Gbits/sec
After the patch:
6830.98 Gbits/sec

About 19.6% improvements in throughput.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-pci.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c38f33f..9a5242a 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -632,28 +632,30 @@ static int virtio_pci_vector_unmask(PCIDevice *dev, unsigned vector,
 {
     VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
-    int ret, queue_no;
+    VirtQueue *vq = virtio_vector_first_queue(vdev, vector);
+    int ret, index, unmasked = 0;
 
-    for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers; queue_no++) {
-        if (!virtio_queue_get_num(vdev, queue_no)) {
+    while (vq) {
+        index = virtio_queue_get_index(vdev, vq);
+        if (!virtio_queue_get_num(vdev, index)) {
             break;
         }
-        if (virtio_queue_vector(vdev, queue_no) != vector) {
-            continue;
-        }
-        ret = virtio_pci_vq_vector_unmask(proxy, queue_no, vector, msg);
+        ret = virtio_pci_vq_vector_unmask(proxy, index, vector, msg);
         if (ret < 0) {
             goto undo;
         }
+        vq = virtio_vector_next_queue(vq);
+        ++unmasked;
     }
+
     return 0;
 
 undo:
-    while (--queue_no >= 0) {
-        if (virtio_queue_vector(vdev, queue_no) != vector) {
-            continue;
-        }
-        virtio_pci_vq_vector_mask(proxy, queue_no, vector);
+    vq = virtio_vector_first_queue(vdev, vector);
+    while (vq && --unmasked >= 0) {
+        index = virtio_queue_get_index(vdev, vq);
+        virtio_pci_vq_vector_mask(proxy, index, vector);
+        vq = virtio_vector_next_queue(vq);
     }
     return ret;
 }
@@ -662,16 +664,16 @@ static void virtio_pci_vector_mask(PCIDevice *dev, unsigned vector)
 {
     VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
-    int queue_no;
+    VirtQueue *vq = virtio_vector_first_queue(vdev, vector);
+    int index;
 
-    for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers; queue_no++) {
-        if (!virtio_queue_get_num(vdev, queue_no)) {
+    while (vq) {
+        index = virtio_queue_get_index(vdev, vq);
+        if (!virtio_queue_get_num(vdev, index)) {
             break;
         }
-        if (virtio_queue_vector(vdev, queue_no) != vector) {
-            continue;
-        }
-        virtio_pci_vq_vector_mask(proxy, queue_no, vector);
+        virtio_pci_vq_vector_mask(proxy, index, vector);
+        vq = virtio_vector_next_queue(vq);
     }
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH V4 17/19] virtio-pci: increase the maximum number of virtqueues to 513

[Jason Wang]

This patch increases the maximum number of virtqueues for pci from 64
to 513. This will allow booting a virtio-net-pci device with 256 queue
pairs.

To keep migration compatibility, 64 was kept for legacy machine
types. This is because qemu in fact allows guest to probe the limit of
virtqueues through trying to set queue_sel. So for legacy machine
type, we should make sure setting queue_sel to more than 63 won't
make effect.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/pc_piix.c      | 5 +++++
 hw/i386/pc_q35.c       | 5 +++++
 hw/ppc/spapr.c         | 5 +++++
 hw/virtio/virtio-pci.c | 2 +-
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 175e582..0796719 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -49,6 +49,7 @@
 #include "hw/acpi/acpi.h"
 #include "cpu.h"
 #include "qemu/error-report.h"
+#include "hw/virtio/virtio-bus.h"
 #ifdef CONFIG_XEN
 #  include <xen/hvm/hvm_info_table.h>
 #endif
@@ -312,6 +313,10 @@ static void pc_init_pci(MachineState *machine)
 
 static void pc_compat_2_3(MachineState *machine)
 {
+    ObjectClass *klass = object_class_by_name("virtio-pci-bus");
+    VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
+
+    k->queue_max = 64;
 }
 
 static void pc_compat_2_2(MachineState *machine)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index df44d25..a8a34a4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -45,6 +45,7 @@
 #include "hw/usb.h"
 #include "hw/cpu/icc_bus.h"
 #include "qemu/error-report.h"
+#include "include/hw/virtio/virtio-bus.h"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
@@ -291,6 +292,10 @@ static void pc_q35_init(MachineState *machine)
 
 static void pc_compat_2_3(MachineState *machine)
 {
+    ObjectClass *klass = object_class_by_name("virtio-pci-bus");
+    VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
+
+    k->queue_max = 64;
 }
 
 static void pc_compat_2_2(MachineState *machine)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 22f4ae4..5f25dd3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -50,6 +50,7 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "hw/virtio/virtio-scsi.h"
+#include "hw/virtio/virtio-bus.h"
 
 #include "exec/address-spaces.h"
 #include "hw/usb.h"
@@ -1805,6 +1806,10 @@ static const TypeInfo spapr_machine_info = {
 
 static void spapr_compat_2_3(Object *obj)
 {
+    ObjectClass *klass = object_class_by_name("virtio-pci-bus");
+    VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
+
+    k->queue_max = 64;
 }
 
 static void spapr_compat_2_2(Object *obj)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 9a5242a..02e3ce8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -42,7 +42,7 @@
  * configuration space */
 #define VIRTIO_PCI_CONFIG_SIZE(dev)     VIRTIO_PCI_CONFIG_OFF(msix_enabled(dev))
 
-#define VIRTIO_PCI_QUEUE_MAX 64
+#define VIRTIO_PCI_QUEUE_MAX 513
 
 static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtIOPCIProxy *dev);
-- 
2.1.0

[Qemu-devel] [PATCH V4 18/19] pci: remove hard-coded bar size in msix_init_exclusive_bar()

[Jason Wang]

This patch let msix_init_exclusive_bar() can accept bar_size parameter
other than a hard-coded limit 4096. Then caller can specify a bar_size
depends on msix entries and can use up to 2048 msix entries as PCI
spec allows. To keep migration compatibility, 4096 is used for all
callers and pba were start from half of bar size.

Cc: Keith Busch <keith.busch@intel.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/block/nvme.c        |  2 +-
 hw/misc/ivshmem.c      |  2 +-
 hw/pci/msix.c          | 18 +++++++-----------
 hw/virtio/virtio-pci.c |  2 +-
 include/hw/pci/msix.h  |  2 +-
 5 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 0f3dfb9..09d7884 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -787,7 +787,7 @@ static int nvme_init(PCIDevice *pci_dev)
     pci_register_bar(&n->parent_obj, 0,
         PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
         &n->iomem);
-    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
+    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, 4096);
 
     id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
     id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 5d272c8..3e2a2d4 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -631,7 +631,7 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
 
 static void ivshmem_setup_msi(IVShmemState * s)
 {
-    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
+    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, 4096)) {
         IVSHMEM_DPRINTF("msix initialization failed\n");
         exit(1);
     }
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 24de260..9a1894f 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -291,33 +291,29 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
 }
 
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
-                            uint8_t bar_nr)
+                            uint8_t bar_nr, uint32_t bar_size)
 {
     int ret;
     char *name;
+    uint32_t bar_pba_offset = bar_size / 2;
 
     /*
      * Migration compatibility dictates that this remains a 4k
      * BAR with the vector table in the lower half and PBA in
      * the upper half.  Do not use these elsewhere!
      */
-#define MSIX_EXCLUSIVE_BAR_SIZE 4096
-#define MSIX_EXCLUSIVE_BAR_TABLE_OFFSET 0
-#define MSIX_EXCLUSIVE_BAR_PBA_OFFSET (MSIX_EXCLUSIVE_BAR_SIZE / 2)
-#define MSIX_EXCLUSIVE_CAP_OFFSET 0
-
-    if (nentries * PCI_MSIX_ENTRY_SIZE > MSIX_EXCLUSIVE_BAR_PBA_OFFSET) {
+    if (nentries * PCI_MSIX_ENTRY_SIZE > bar_pba_offset) {
         return -EINVAL;
     }
 
     name = g_strdup_printf("%s-msix", dev->name);
-    memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, MSIX_EXCLUSIVE_BAR_SIZE);
+    memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size);
     g_free(name);
 
     ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
-                    MSIX_EXCLUSIVE_BAR_TABLE_OFFSET, &dev->msix_exclusive_bar,
-                    bar_nr, MSIX_EXCLUSIVE_BAR_PBA_OFFSET,
-                    MSIX_EXCLUSIVE_CAP_OFFSET);
+                    0, &dev->msix_exclusive_bar,
+                    bar_nr, bar_pba_offset,
+                    0);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 02e3ce8..4a5febb 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -937,7 +937,7 @@ static void virtio_pci_device_plugged(DeviceState *d)
     config[PCI_INTERRUPT_PIN] = 1;
 
     if (proxy->nvectors &&
-        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
+        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1, 4096)) {
         error_report("unable to init msix vectors to %" PRIu32,
                      proxy->nvectors);
         proxy->nvectors = 0;
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 954d82b..43edebc 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -11,7 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
               unsigned table_offset, MemoryRegion *pba_bar,
               uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
-                            uint8_t bar_nr);
+                            uint8_t bar_nr, uint32_t bar_size);
 
 void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
 
-- 
2.1.0

[Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Jason Wang]

Currently we don't support more than 128 MSI-X vectors for a pci
devices, trying to use vector=129 for a virtio-net-pci device may get:

qemu-system-x86_64: -device virtio-net-pci,netdev=hn0,vectors=129:
unable to init msix vectors to 129

This this because the MSI-X bar size were hard-coded as 4096. So this
patch introduces boolean auto_msix_bar_size property for
virito-pci devices. Enable this will let the device calculate the msix
bar size based on the number of MSI-X entries instead of previous 4096
hard-coded limit.

This is a must to let virtio-net can up to 256 queues and each queue
were associated with a specific MSI-X entry.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/pc_piix.c      |  8 ++++++++
 hw/i386/pc_q35.c       |  8 ++++++++
 hw/ppc/spapr.c         | 11 ++++++++++-
 hw/virtio/virtio-pci.c | 17 +++++++++++++++--
 hw/virtio/virtio-pci.h |  3 +++
 include/hw/compat.h    | 11 +++++++++++
 6 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 0796719..8808500 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = {
     PC_I440FX_2_3_MACHINE_OPTIONS,
     .name = "pc-i440fx-2.3",
     .init = pc_init_pci_2_3,
+    .compat_props = (GlobalProperty[]) {
+        HW_COMPAT_2_3,
+        { /* end of list */ }
+    },
 };
 
 #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
@@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = {
     PC_I440FX_2_2_MACHINE_OPTIONS,
     .name = "pc-i440fx-2.2",
     .init = pc_init_pci_2_2,
+    .compat_props = (GlobalProperty[]) {
+        HW_COMPAT_2_2,
+        { /* end of list */ }
+    },
 };
 
 #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a8a34a4..4a34349 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
     PC_Q35_2_3_MACHINE_OPTIONS,
     .name = "pc-q35-2.3",
     .init = pc_q35_init_2_3,
+    .compat_props = (GlobalProperty[]) {
+        HW_COMPAT_2_3,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
@@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
     PC_Q35_2_2_MACHINE_OPTIONS,
     .name = "pc-q35-2.2",
     .init = pc_q35_init_2_2,
+    .compat_props = (GlobalProperty[]) {
+        HW_COMPAT_2_2,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_2_1_MACHINE_OPTIONS                      \
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5f25dd3..853a5cc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info = {
     },
 };
 
+#define SPAPR_COMPAT_2_3 \
+        HW_COMPAT_2_3
+
 #define SPAPR_COMPAT_2_2 \
+        SPAPR_COMPAT_2_3, \
         {\
             .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
             .property = "mem_win_size",\
             .value    = "0x20000000",\
-        }
+        } \
 
 #define SPAPR_COMPAT_2_1 \
         SPAPR_COMPAT_2_2
@@ -1883,10 +1887,15 @@ static const TypeInfo spapr_machine_2_2_info = {
 
 static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data)
 {
+    static GlobalProperty compat_props[] = {
+        SPAPR_COMPAT_2_3,
+        { /* end of list */ }
+    };
     MachineClass *mc = MACHINE_CLASS(oc);
 
     mc->name = "pseries-2.3";
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
+    mc->compat_props = compat_props;
 }
 
 static const TypeInfo spapr_machine_2_3_info = {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 4a5febb..f4cd405 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -925,7 +925,7 @@ static void virtio_pci_device_plugged(DeviceState *d)
     VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
     VirtioBusState *bus = &proxy->bus;
     uint8_t *config;
-    uint32_t size;
+    uint32_t size, bar_size;
 
     config = proxy->pci_dev.config;
     if (proxy->class_code) {
@@ -936,8 +936,19 @@ static void virtio_pci_device_plugged(DeviceState *d)
     pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
     config[PCI_INTERRUPT_PIN] = 1;
 
+    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
+        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
+        if (bar_size & (bar_size - 1)) {
+            bar_size = 1 << qemu_fls(bar_size);
+        }
+    } else {
+        /* For migration compatibility */
+        bar_size = 4096;
+    }
+
     if (proxy->nvectors &&
-        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1, 4096)) {
+        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1,
+                                bar_size)) {
         error_report("unable to init msix vectors to %" PRIu32,
                      proxy->nvectors);
         proxy->nvectors = 0;
@@ -1370,6 +1381,8 @@ static const TypeInfo virtio_serial_pci_info = {
 static Property virtio_net_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
+    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
+                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
     DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 3bac016..82a6782 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -62,6 +62,9 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
  * 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)
+#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
+#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
+    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
 
 typedef struct {
     MSIMessage msg;
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 313682a..3186275 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -1,7 +1,18 @@
 #ifndef HW_COMPAT_H
 #define HW_COMPAT_H
 
+#define HW_COMPAT_2_3 \
+        {\
+            .driver   = "virtio-net-pci",\
+            .property = "auto_msix_bar_size",\
+            .value    = "off",\
+        }
+
+#define HW_COMPAT_2_2 \
+        HW_COMPAT_2_3
+
 #define HW_COMPAT_2_1 \
+        HW_COMPAT_2_2, \
         {\
             .driver   = "intel-hda",\
             .property = "old_msi_addr",\
-- 
2.1.0

Re: [Qemu-devel] [PATCH V4 18/19] pci: remove hard-coded bar size in msix_init_exclusive_bar()

[Michael S. Tsirkin]

On Wed, Mar 18, 2015 at 05:35:08PM +0800, Jason Wang wrote:
> This patch let msix_init_exclusive_bar() can accept bar_size parameter
> other than a hard-coded limit 4096. Then caller can specify a bar_size
> depends on msix entries and can use up to 2048 msix entries as PCI
> spec allows. To keep migration compatibility, 4096 is used for all
> callers and pba were start from half of bar size.
> 
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Hmm this API looks strange. Caller will then have to have
msi specific knowledge.
Can't we keep the size iternal to msix.c?

> ---
>  hw/block/nvme.c        |  2 +-
>  hw/misc/ivshmem.c      |  2 +-
>  hw/pci/msix.c          | 18 +++++++-----------
>  hw/virtio/virtio-pci.c |  2 +-
>  include/hw/pci/msix.h  |  2 +-
>  5 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 0f3dfb9..09d7884 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -787,7 +787,7 @@ static int nvme_init(PCIDevice *pci_dev)
>      pci_register_bar(&n->parent_obj, 0,
>          PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
>          &n->iomem);
> -    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
> +    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, 4096);
>  
>      id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
>      id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 5d272c8..3e2a2d4 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -631,7 +631,7 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
>  
>  static void ivshmem_setup_msi(IVShmemState * s)
>  {
> -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> +    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, 4096)) {
>          IVSHMEM_DPRINTF("msix initialization failed\n");
>          exit(1);
>      }
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 24de260..9a1894f 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -291,33 +291,29 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>  }
>  
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> -                            uint8_t bar_nr)
> +                            uint8_t bar_nr, uint32_t bar_size)
>  {
>      int ret;
>      char *name;
> +    uint32_t bar_pba_offset = bar_size / 2;

Spec says we must give BIR 4k of it's own, but for
larger BARs, using up half the BAR just for BIR seems
wasteful.

>  
>      /*
>       * Migration compatibility dictates that this remains a 4k
>       * BAR with the vector table in the lower half and PBA in
>       * the upper half.  Do not use these elsewhere!
>       */

You've left comment here where it no longer makes
any sense.

> -#define MSIX_EXCLUSIVE_BAR_SIZE 4096
> -#define MSIX_EXCLUSIVE_BAR_TABLE_OFFSET 0
> -#define MSIX_EXCLUSIVE_BAR_PBA_OFFSET (MSIX_EXCLUSIVE_BAR_SIZE / 2)
> -#define MSIX_EXCLUSIVE_CAP_OFFSET 0
> -
> -    if (nentries * PCI_MSIX_ENTRY_SIZE > MSIX_EXCLUSIVE_BAR_PBA_OFFSET) {
> +    if (nentries * PCI_MSIX_ENTRY_SIZE > bar_pba_offset) {
>          return -EINVAL;
>      }
>  
>      name = g_strdup_printf("%s-msix", dev->name);
> -    memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, MSIX_EXCLUSIVE_BAR_SIZE);
> +    memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size);
>      g_free(name);
>  
>      ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
> -                    MSIX_EXCLUSIVE_BAR_TABLE_OFFSET, &dev->msix_exclusive_bar,
> -                    bar_nr, MSIX_EXCLUSIVE_BAR_PBA_OFFSET,
> -                    MSIX_EXCLUSIVE_CAP_OFFSET);
> +                    0, &dev->msix_exclusive_bar,
> +                    bar_nr, bar_pba_offset,
> +                    0);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 02e3ce8..4a5febb 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -937,7 +937,7 @@ static void virtio_pci_device_plugged(DeviceState *d)
>      config[PCI_INTERRUPT_PIN] = 1;
>  
>      if (proxy->nvectors &&
> -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
> +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1, 4096)) {
>          error_report("unable to init msix vectors to %" PRIu32,
>                       proxy->nvectors);
>          proxy->nvectors = 0;
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 954d82b..43edebc 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -11,7 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
>                unsigned table_offset, MemoryRegion *pba_bar,
>                uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> -                            uint8_t bar_nr);
> +                            uint8_t bar_nr, uint32_t bar_size);
>  
>  void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
>  
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Michael S. Tsirkin]

On Wed, Mar 18, 2015 at 05:35:09PM +0800, Jason Wang wrote:
> Currently we don't support more than 128 MSI-X vectors for a pci
> devices, trying to use vector=129 for a virtio-net-pci device may get:
> 
> qemu-system-x86_64: -device virtio-net-pci,netdev=hn0,vectors=129:
> unable to init msix vectors to 129
> 
> This this because the MSI-X bar size were hard-coded as 4096. So this
> patch introduces boolean auto_msix_bar_size property for
> virito-pci devices. Enable this will let the device calculate the msix
> bar size based on the number of MSI-X entries instead of previous 4096
> hard-coded limit.
> 
> This is a must to let virtio-net can up to 256 queues and each queue
> were associated with a specific MSI-X entry.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>

I don't understand what this property does.
What if I *don't* set auto_msix_bar_size?
vectors<=128 works exactly like it dif previously, vectors=129 fails?
Does not seem like useful behaviour, to me.


> ---
>  hw/i386/pc_piix.c      |  8 ++++++++
>  hw/i386/pc_q35.c       |  8 ++++++++
>  hw/ppc/spapr.c         | 11 ++++++++++-
>  hw/virtio/virtio-pci.c | 17 +++++++++++++++--
>  hw/virtio/virtio-pci.h |  3 +++
>  include/hw/compat.h    | 11 +++++++++++
>  6 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 0796719..8808500 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = {
>      PC_I440FX_2_3_MACHINE_OPTIONS,
>      .name = "pc-i440fx-2.3",
>      .init = pc_init_pci_2_3,
> +    .compat_props = (GlobalProperty[]) {
> +        HW_COMPAT_2_3,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
> @@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = {
>      PC_I440FX_2_2_MACHINE_OPTIONS,
>      .name = "pc-i440fx-2.2",
>      .init = pc_init_pci_2_2,
> +    .compat_props = (GlobalProperty[]) {
> +        HW_COMPAT_2_2,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a8a34a4..4a34349 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>      PC_Q35_2_3_MACHINE_OPTIONS,
>      .name = "pc-q35-2.3",
>      .init = pc_q35_init_2_3,
> +    .compat_props = (GlobalProperty[]) {
> +        HW_COMPAT_2_3,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
> @@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
>      PC_Q35_2_2_MACHINE_OPTIONS,
>      .name = "pc-q35-2.2",
>      .init = pc_q35_init_2_2,
> +    .compat_props = (GlobalProperty[]) {
> +        HW_COMPAT_2_2,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_Q35_2_1_MACHINE_OPTIONS                      \
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5f25dd3..853a5cc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info = {
>      },
>  };
>  
> +#define SPAPR_COMPAT_2_3 \
> +        HW_COMPAT_2_3
> +
>  #define SPAPR_COMPAT_2_2 \
> +        SPAPR_COMPAT_2_3, \
>          {\
>              .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>              .property = "mem_win_size",\
>              .value    = "0x20000000",\
> -        }
> +        } \
>  
>  #define SPAPR_COMPAT_2_1 \
>          SPAPR_COMPAT_2_2
> @@ -1883,10 +1887,15 @@ static const TypeInfo spapr_machine_2_2_info = {
>  
>  static void spapr_machine_2_3_class_init(ObjectClass *oc, void *data)
>  {
> +    static GlobalProperty compat_props[] = {
> +        SPAPR_COMPAT_2_3,
> +        { /* end of list */ }
> +    };
>      MachineClass *mc = MACHINE_CLASS(oc);
>  
>      mc->name = "pseries-2.3";
>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
> +    mc->compat_props = compat_props;
>  }
>  
>  static const TypeInfo spapr_machine_2_3_info = {
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 4a5febb..f4cd405 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -925,7 +925,7 @@ static void virtio_pci_device_plugged(DeviceState *d)
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>      VirtioBusState *bus = &proxy->bus;
>      uint8_t *config;
> -    uint32_t size;
> +    uint32_t size, bar_size;
>  
>      config = proxy->pci_dev.config;
>      if (proxy->class_code) {
> @@ -936,8 +936,19 @@ static void virtio_pci_device_plugged(DeviceState *d)
>      pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
>      config[PCI_INTERRUPT_PIN] = 1;
>  
> +    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
> +        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
> +        if (bar_size & (bar_size - 1)) {
> +            bar_size = 1 << qemu_fls(bar_size);
> +        }
> +    } else {
> +        /* For migration compatibility */
> +        bar_size = 4096;
> +    }
> +
>      if (proxy->nvectors &&
> -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1, 4096)) {
> +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1,
> +                                bar_size)) {
>          error_report("unable to init msix vectors to %" PRIu32,
>                       proxy->nvectors);
>          proxy->nvectors = 0;


As I expected, msix format stuff spreads out to virtio.
Consider "vectors * PCI_MSIX_ENTRY_SIZE * 2"
That's because you use half the BAR for BIR in msix.c
So any change will have to be done in two places,
that's bad.


> @@ -1370,6 +1381,8 @@ static const TypeInfo virtio_serial_pci_info = {
>  static Property virtio_net_properties[] = {
>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>                      VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
> +    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
> +                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
>      DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 3bac016..82a6782 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -62,6 +62,9 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
>   * 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)
> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
> +    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
>  
>  typedef struct {
>      MSIMessage msg;
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 313682a..3186275 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -1,7 +1,18 @@
>  #ifndef HW_COMPAT_H
>  #define HW_COMPAT_H
>  
> +#define HW_COMPAT_2_3 \
> +        {\
> +            .driver   = "virtio-net-pci",\
> +            .property = "auto_msix_bar_size",\
> +            .value    = "off",\
> +        }
> +
> +#define HW_COMPAT_2_2 \
> +        HW_COMPAT_2_3
> +
>  #define HW_COMPAT_2_1 \
> +        HW_COMPAT_2_2, \
>          {\
>              .driver   = "intel-hda",\
>              .property = "old_msi_addr",\
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V4 00/19] Support more virtio queues

[Michael S. Tsirkin]

On Wed, Mar 18, 2015 at 05:34:50PM +0800, Jason Wang wrote:
> We current limit the max virtio queues to 64. This is not sufficient
> to support multiqueue devices (e.g recent Linux support up to 256
> tap queues). So this series tries to let virtio to support more queues.
> 
> No much works need to be done except:
> 
> - Introducing transport specific queue limitation.
> - Let each virtio transport to use specific limit.
> - Speedup the MSI-X masking and unmasking through per vector queue
>   list, and increase the maximum MSI-X vectors supported by qemu.
> - With the above enhancements, increase the maximum number of
>   virtqueues supported by PCI from 64 to 513.
> - Compat the changes for legacy machine types.

What are the compatibility considerations here?


> With this patch, we can support up to 256 queues. Since x86 can only
> allow about 240 interrupt vectors for MSI-X, current Linux driver can
> only have about 80 queue pairs has their private MSI-X interrupt
> vectors. With sharing IRQ with queue pairs (RFC posted in
> https://lkml.org/lkml/2014/12/25/169), Linux driver can have up
> to about 186 queue pairs has their private MSI-X interrupt vectors.
> 
> Stress/migration test on virtio-pci, compile test on other
> targets. And make check on s390x-softmmu and ppc64-softmmu.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Amit Shah <amit.shah@redhat.com>
> Cc: qemu-ppc@nongnu.org
> 
> Please review
> 
> Thanks
> 
> Changes from V3:
> - rebase to master and target to 2.4
> - handling compat issues for spapr
> - fixes for hmp command completion when we have a nic with 256 queues
> - using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue in
>   virtio-ccw
> - fix compile issues for ppc64-softmmu
> - don't export VIRTIO_CCW_QUEUE_MAX by introducing a gsi limit and
>   inherit in ccw
> - use transport specific queue limit in virtio-serial
> - correct the stale comment for AdapterRoutes and move it to ccw patch
> - replace 128 with queue_max + 64 and add a comment for this in
>   virtio_ccw_notify()
> 
> Changes from V2:
> - move transport specific limitation to their implementation. (The
>   left is VIRTIO_CCW_QUEUE_MAX since I fail to find a common header
>   files other than virtio.h)
> - use virtio_get_queue_max() if possible in virtio.c
> - AdapterRoutes should use ccw limit
> - introduce a vector to queue mapping for virito devices and speedup
>   the MSI-X masking and unmasking through this.
> 
> Changes from V1:
> - add a validation against the bus limitation
> - switch to use a bus specific queue limit instead of a global one,
>   this will allow us to just increase the limit of one transport
>   without disturbing others.
> - only increase the queue limit of virtio-pci
> - limit the maximum number of virtio queues to 64 for legacy machine
>   types
> 
> Jason Wang (19):
>   pc: add 2.4 machine types
>   spapr: add machine type specific instance init function
>   ppc: spapr: add 2.4 machine type
>   monitor: replace the magic number 255 with MAX_QUEUE_NUM
>   monitor: check return value of qemu_find_net_clients_except()
>   virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue
>   virtio-net: validate backend queue numbers against bus limitation
>   virtio-net: fix the upper bound when trying to delete queues
>   virito: introduce bus specific queue limit
>   virtio-ccw: introduce ccw specific queue limit
>   virtio-s390: switch to bus specific queue limit
>   virtio-mmio: switch to bus specific queue limit
>   virtio-pci: switch to use bus specific queue limit
>   virtio: introduce vector to virtqueues mapping
>   virtio: introduce virtio_queue_get_index()
>   virtio-pci: speedup MSI-X masking and unmasking
>   virtio-pci: increase the maximum number of virtqueues to 513
>   pci: remove hard-coded bar size in msix_init_exclusive_bar()
>   virtio-pci: introduce auto_msix_bar_size property
> 
>  hw/block/nvme.c                |  2 +-
>  hw/char/virtio-serial-bus.c    |  2 +-
>  hw/i386/pc_piix.c              | 42 ++++++++++++++++++++---
>  hw/i386/pc_q35.c               | 39 +++++++++++++++++++--
>  hw/misc/ivshmem.c              |  2 +-
>  hw/net/virtio-net.c            |  9 ++++-
>  hw/pci/msix.c                  | 18 ++++------
>  hw/ppc/spapr.c                 | 70 +++++++++++++++++++++++++++++++++++--
>  hw/s390x/s390-virtio-bus.c     |  7 ++--
>  hw/s390x/s390-virtio-ccw.c     |  7 ++--
>  hw/s390x/virtio-ccw.c          | 20 +++++++----
>  hw/scsi/virtio-scsi.c          |  4 +--
>  hw/virtio/virtio-mmio.c        |  7 ++--
>  hw/virtio/virtio-pci.c         | 78 ++++++++++++++++++++++++++++--------------
>  hw/virtio/virtio-pci.h         |  3 ++
>  hw/virtio/virtio.c             | 75 +++++++++++++++++++++++++++++++---------
>  include/hw/compat.h            | 11 ++++++
>  include/hw/pci/msix.h          |  2 +-
>  include/hw/s390x/s390_flic.h   |  4 ++-
>  include/hw/virtio/virtio-bus.h |  2 ++
>  include/hw/virtio/virtio.h     |  7 ++--
>  monitor.c                      | 25 ++++++++------
>  22 files changed, 339 insertions(+), 97 deletions(-)
> 
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V4 07/19] virtio-net: validate backend queue numbers against bus limitation

[Michael S. Tsirkin]

On Wed, Mar 18, 2015 at 05:34:57PM +0800, Jason Wang wrote:
> We don't validate the backend queue numbers against bus limitation,
> this will easily crash qemu if it exceeds the limitation. Fixing this
> by doing the validation and fail early.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Bugfix? needed in 2.3?

> ---
>  hw/net/virtio-net.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 27adcc5..59f76bc 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1588,6 +1588,13 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>      virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>  
>      n->max_queues = MAX(n->nic_conf.peers.queues, 1);
> +    if (n->max_queues * 2 + 1 > VIRTIO_PCI_QUEUE_MAX) {

We have this * 2 + 1 logic in several other places in this file too.
Pls wrap it up in a helper.

> +        error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
> +                   "must be a postive integer less than %d.",
> +                   n->max_queues, (VIRTIO_PCI_QUEUE_MAX - 1) / 2);
> +        virtio_cleanup(vdev);
> +        return;
> +    }
>      n->vqs = g_malloc0(sizeof(VirtIONetQueue) * n->max_queues);
>      n->vqs[0].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx);
>      n->curr_queues = 1;
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V4 08/19] virtio-net: fix the upper bound when trying to delete queues

[Michael S. Tsirkin]

On Wed, Mar 18, 2015 at 05:34:58PM +0800, Jason Wang wrote:
> Virtqueue were indexed from zero, so don't delete virtqueue whose
> index is n->max_queues * 2 + 1.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Bugfix? needed on master? stable?

> ---
>  hw/net/virtio-net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 59f76bc..b6fac9c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1309,7 +1309,7 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
>  
>      n->multiqueue = multiqueue;
>  
> -    for (i = 2; i <= n->max_queues * 2 + 1; i++) {
> +    for (i = 2; i < n->max_queues * 2 + 1; i++) {
>          virtio_del_queue(vdev, i);
>      }
>  
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V4 06/19] virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue

[Michael S. Tsirkin]

On Wed, Mar 18, 2015 at 05:34:56PM +0800, Jason Wang wrote:
> There's no need to use vector 0 for invalid virtqueue. So this patch
> changes to use VIRTIO_NO_VECTOR instead.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> CC: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

I don't know what does this actually do.
Cornelia?

> ---
>  hw/s390x/virtio-ccw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 130535c..c8b87aa 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -281,7 +281,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
>  
>      virtio_queue_set_addr(vdev, index, addr);
>      if (!addr) {
> -        virtio_queue_set_vector(vdev, index, 0);
> +        virtio_queue_set_vector(vdev, index, VIRTIO_NO_VECTOR);
>      } else {
>          /* Fail if we don't have a big enough queue. */
>          /* TODO: Add interface to handle vring.num changing */

Right below this, we have
    /* tell notify handler in case of config change */
    vdev->config_vector = VIRTIO_PCI_QUEUE_MAX;

which also does not seem to make sense.

These changes need some testing though.

> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V4 18/19] pci: remove hard-coded bar size in msix_init_exclusive_bar()

[Jason Wang]

On Wed, Mar 18, 2015 at 8:52 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Wed, Mar 18, 2015 at 05:35:08PM +0800, Jason Wang wrote:
>>  This patch let msix_init_exclusive_bar() can accept bar_size 
>> parameter
>>  other than a hard-coded limit 4096. Then caller can specify a 
>> bar_size
>>  depends on msix entries and can use up to 2048 msix entries as PCI
>>  spec allows. To keep migration compatibility, 4096 is used for all
>>  callers and pba were start from half of bar size.
>>  
>>  Cc: Keith Busch <keith.busch@intel.com>
>>  Cc: Kevin Wolf <kwolf@redhat.com>
>>  Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> Hmm this API looks strange. Caller will then have to have
> msi specific knowledge.
> Can't we keep the size iternal to msix.c?

Looks like we can, but still need an extra parameter e.g a boolean 
compat_size to let msix_init_exclusive_bar() to use 4096 as bar size.

> 
> 
>>  ---
>>   hw/block/nvme.c        |  2 +-
>>   hw/misc/ivshmem.c      |  2 +-
>>   hw/pci/msix.c          | 18 +++++++-----------
>>   hw/virtio/virtio-pci.c |  2 +-
>>   include/hw/pci/msix.h  |  2 +-
>>   5 files changed, 11 insertions(+), 15 deletions(-)
>>  
>>  diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>  index 0f3dfb9..09d7884 100644
>>  --- a/hw/block/nvme.c
>>  +++ b/hw/block/nvme.c
>>  @@ -787,7 +787,7 @@ static int nvme_init(PCIDevice *pci_dev)
>>       pci_register_bar(&n->parent_obj, 0,
>>           PCI_BASE_ADDRESS_SPACE_MEMORY | 
>> PCI_BASE_ADDRESS_MEM_TYPE_64,
>>           &n->iomem);
>>  -    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
>>  +    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, 
>> 4096);
>>   
>>       id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
>>       id->ssvid = cpu_to_le16(pci_get_word(pci_conf + 
>> PCI_SUBSYSTEM_VENDOR_ID));
>>  diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>  index 5d272c8..3e2a2d4 100644
>>  --- a/hw/misc/ivshmem.c
>>  +++ b/hw/misc/ivshmem.c
>>  @@ -631,7 +631,7 @@ static uint64_t ivshmem_get_size(IVShmemState * 
>> s) {
>>   
>>   static void ivshmem_setup_msi(IVShmemState * s)
>>   {
>>  -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
>>  +    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, 
>> 4096)) {
>>           IVSHMEM_DPRINTF("msix initialization failed\n");
>>           exit(1);
>>       }
>>  diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>>  index 24de260..9a1894f 100644
>>  --- a/hw/pci/msix.c
>>  +++ b/hw/pci/msix.c
>>  @@ -291,33 +291,29 @@ int msix_init(struct PCIDevice *dev, unsigned 
>> short nentries,
>>   }
>>   
>>   int msix_init_exclusive_bar(PCIDevice *dev, unsigned short 
>> nentries,
>>  -                            uint8_t bar_nr)
>>  +                            uint8_t bar_nr, uint32_t bar_size)
>>   {
>>       int ret;
>>       char *name;
>>  +    uint32_t bar_pba_offset = bar_size / 2;
> 
> Spec says we must give BIR 4k of it's own, but for
> larger BARs, using up half the BAR just for BIR seems
> wasteful.

I see, I will make PBA size more accurate.

> 
> 
>>   
>>       /*
>>        * Migration compatibility dictates that this remains a 4k
>>        * BAR with the vector table in the lower half and PBA in
>>        * the upper half.  Do not use these elsewhere!
>>        */
> 
> You've left comment here where it no longer makes
> any sense.

Will remove this.

> 
> 
>>  -#define MSIX_EXCLUSIVE_BAR_SIZE 4096
>>  -#define MSIX_EXCLUSIVE_BAR_TABLE_OFFSET 0
>>  -#define MSIX_EXCLUSIVE_BAR_PBA_OFFSET (MSIX_EXCLUSIVE_BAR_SIZE / 2)
>>  -#define MSIX_EXCLUSIVE_CAP_OFFSET 0
>>  -
>>  -    if (nentries * PCI_MSIX_ENTRY_SIZE > 
>> MSIX_EXCLUSIVE_BAR_PBA_OFFSET) {
>>  +    if (nentries * PCI_MSIX_ENTRY_SIZE > bar_pba_offset) {
>>           return -EINVAL;
>>       }
>>   
>>       name = g_strdup_printf("%s-msix", dev->name);
>>  -    memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), 
>> name, MSIX_EXCLUSIVE_BAR_SIZE);
>>  +    memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), 
>> name, bar_size);
>>       g_free(name);
>>   
>>       ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, 
>> bar_nr,
>>  -                    MSIX_EXCLUSIVE_BAR_TABLE_OFFSET, 
>> &dev->msix_exclusive_bar,
>>  -                    bar_nr, MSIX_EXCLUSIVE_BAR_PBA_OFFSET,
>>  -                    MSIX_EXCLUSIVE_CAP_OFFSET);
>>  +                    0, &dev->msix_exclusive_bar,
>>  +                    bar_nr, bar_pba_offset,
>>  +                    0);
>>       if (ret) {
>>           return ret;
>>       }
>>  diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>  index 02e3ce8..4a5febb 100644
>>  --- a/hw/virtio/virtio-pci.c
>>  +++ b/hw/virtio/virtio-pci.c
>>  @@ -937,7 +937,7 @@ static void 
>> virtio_pci_device_plugged(DeviceState *d)
>>       config[PCI_INTERRUPT_PIN] = 1;
>>   
>>       if (proxy->nvectors &&
>>  -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 
>> 1)) {
>>  +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 
>> 1, 4096)) {
>>           error_report("unable to init msix vectors to %" PRIu32,
>>                        proxy->nvectors);
>>           proxy->nvectors = 0;
>>  diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
>>  index 954d82b..43edebc 100644
>>  --- a/include/hw/pci/msix.h
>>  +++ b/include/hw/pci/msix.h
>>  @@ -11,7 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short 
>> nentries,
>>                 unsigned table_offset, MemoryRegion *pba_bar,
>>                 uint8_t pba_bar_nr, unsigned pba_offset, uint8_t 
>> cap_pos);
>>   int msix_init_exclusive_bar(PCIDevice *dev, unsigned short 
>> nentries,
>>  -                            uint8_t bar_nr);
>>  +                            uint8_t bar_nr, uint32_t bar_size);
>>   
>>   void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t 
>> val, int len);
>>   
>>  -- 
>>  2.1.0

Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Jason Wang]

On Wed, Mar 18, 2015 at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Wed, Mar 18, 2015 at 05:35:09PM +0800, Jason Wang wrote:
>>  Currently we don't support more than 128 MSI-X vectors for a pci
>>  devices, trying to use vector=129 for a virtio-net-pci device may 
>> get:
>>  
>>  qemu-system-x86_64: -device virtio-net-pci,netdev=hn0,vectors=129:
>>  unable to init msix vectors to 129
>>  
>>  This this because the MSI-X bar size were hard-coded as 4096. So 
>> this
>>  patch introduces boolean auto_msix_bar_size property for
>>  virito-pci devices. Enable this will let the device calculate the 
>> msix
>>  bar size based on the number of MSI-X entries instead of previous 
>> 4096
>>  hard-coded limit.
>>  
>>  This is a must to let virtio-net can up to 256 queues and each queue
>>  were associated with a specific MSI-X entry.
>>  
>>  Cc: Paolo Bonzini <pbonzini@redhat.com>
>>  Cc: Richard Henderson <rth@twiddle.net>
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Cc: Alexander Graf <agraf@suse.de>
>>  Cc: qemu-ppc@nongnu.org
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> I don't understand what this property does.
> What if I *don't* set auto_msix_bar_size?
> vectors<=128 works exactly like it dif previously, vectors=129 fails?

Yes, but there looks like a bug in the code which can lead a bar size 
less than 4K.

> 
> Does not seem like useful behaviour, to me.

This property allows to have more than 128 vectors to be used. We 
disable this for legacy machine types to stick bar size to 4K to keep 
the migration compatibility.


> 
> 
>>  ---
>>   hw/i386/pc_piix.c      |  8 ++++++++
>>   hw/i386/pc_q35.c       |  8 ++++++++
>>   hw/ppc/spapr.c         | 11 ++++++++++-
>>   hw/virtio/virtio-pci.c | 17 +++++++++++++++--
>>   hw/virtio/virtio-pci.h |  3 +++
>>   include/hw/compat.h    | 11 +++++++++++
>>   6 files changed, 55 insertions(+), 3 deletions(-)
>>  
>>  diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>  index 0796719..8808500 100644
>>  --- a/hw/i386/pc_piix.c
>>  +++ b/hw/i386/pc_piix.c
>>  @@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = {
>>       PC_I440FX_2_3_MACHINE_OPTIONS,
>>       .name = "pc-i440fx-2.3",
>>       .init = pc_init_pci_2_3,
>>  +    .compat_props = (GlobalProperty[]) {
>>  +        HW_COMPAT_2_3,
>>  +        { /* end of list */ }
>>  +    },
>>   };
>>   
>>   #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
>>  @@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = {
>>       PC_I440FX_2_2_MACHINE_OPTIONS,
>>       .name = "pc-i440fx-2.2",
>>       .init = pc_init_pci_2_2,
>>  +    .compat_props = (GlobalProperty[]) {
>>  +        HW_COMPAT_2_2,
>>  +        { /* end of list */ }
>>  +    },
>>   };
>>   
>>   #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
>>  diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>  index a8a34a4..4a34349 100644
>>  --- a/hw/i386/pc_q35.c
>>  +++ b/hw/i386/pc_q35.c
>>  @@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>>       PC_Q35_2_3_MACHINE_OPTIONS,
>>       .name = "pc-q35-2.3",
>>       .init = pc_q35_init_2_3,
>>  +    .compat_props = (GlobalProperty[]) {
>>  +        HW_COMPAT_2_3,
>>  +        { /* end of list */ }
>>  +    },
>>   };
>>   
>>   #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
>>  @@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
>>       PC_Q35_2_2_MACHINE_OPTIONS,
>>       .name = "pc-q35-2.2",
>>       .init = pc_q35_init_2_2,
>>  +    .compat_props = (GlobalProperty[]) {
>>  +        HW_COMPAT_2_2,
>>  +        { /* end of list */ }
>>  +    },
>>   };
>>   
>>   #define PC_Q35_2_1_MACHINE_OPTIONS                      \
>>  diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>  index 5f25dd3..853a5cc 100644
>>  --- a/hw/ppc/spapr.c
>>  +++ b/hw/ppc/spapr.c
>>  @@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info = {
>>       },
>>   };
>>   
>>  +#define SPAPR_COMPAT_2_3 \
>>  +        HW_COMPAT_2_3
>>  +
>>   #define SPAPR_COMPAT_2_2 \
>>  +        SPAPR_COMPAT_2_3, \
>>           {\
>>               .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>>               .property = "mem_win_size",\
>>               .value    = "0x20000000",\
>>  -        }
>>  +        } \
>>   
>>   #define SPAPR_COMPAT_2_1 \
>>           SPAPR_COMPAT_2_2
>>  @@ -1883,10 +1887,15 @@ static const TypeInfo 
>> spapr_machine_2_2_info = {
>>   
>>   static void spapr_machine_2_3_class_init(ObjectClass *oc, void 
>> *data)
>>   {
>>  +    static GlobalProperty compat_props[] = {
>>  +        SPAPR_COMPAT_2_3,
>>  +        { /* end of list */ }
>>  +    };
>>       MachineClass *mc = MACHINE_CLASS(oc);
>>   
>>       mc->name = "pseries-2.3";
>>       mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
>>  +    mc->compat_props = compat_props;
>>   }
>>   
>>   static const TypeInfo spapr_machine_2_3_info = {
>>  diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>  index 4a5febb..f4cd405 100644
>>  --- a/hw/virtio/virtio-pci.c
>>  +++ b/hw/virtio/virtio-pci.c
>>  @@ -925,7 +925,7 @@ static void 
>> virtio_pci_device_plugged(DeviceState *d)
>>       VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>>       VirtioBusState *bus = &proxy->bus;
>>       uint8_t *config;
>>  -    uint32_t size;
>>  +    uint32_t size, bar_size;
>>   
>>       config = proxy->pci_dev.config;
>>       if (proxy->class_code) {
>>  @@ -936,8 +936,19 @@ static void 
>> virtio_pci_device_plugged(DeviceState *d)
>>       pci_set_word(config + PCI_SUBSYSTEM_ID, 
>> virtio_bus_get_vdev_id(bus));
>>       config[PCI_INTERRUPT_PIN] = 1;
>>   
>>  +    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
>>  +        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
>>  +        if (bar_size & (bar_size - 1)) {
>>  +            bar_size = 1 << qemu_fls(bar_size);
>>  +        }
>>  +    } else {
>>  +        /* For migration compatibility */
>>  +        bar_size = 4096;
>>  +    }
>>  +
>>       if (proxy->nvectors &&
>>  -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 
>> 1, 4096)) {
>>  +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 
>> 1,
>>  +                                bar_size)) {
>>           error_report("unable to init msix vectors to %" PRIu32,
>>                        proxy->nvectors);
>>           proxy->nvectors = 0;
> 
> 
> As I expected, msix format stuff spreads out to virtio.
> Consider "vectors * PCI_MSIX_ENTRY_SIZE * 2"
> That's because you use half the BAR for BIR in msix.c
> So any change will have to be done in two places,
> that's bad.
> 
> 
>>  @@ -1370,6 +1381,8 @@ static const TypeInfo virtio_serial_pci_info 
>> = {
>>   static Property virtio_net_properties[] = {
>>       DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>>                       VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
>>  +    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
>>  +                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
>>       DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
>>       DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
>>       DEFINE_PROP_END_OF_LIST(),
>>  diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>>  index 3bac016..82a6782 100644
>>  --- a/hw/virtio/virtio-pci.h
>>  +++ b/hw/virtio/virtio-pci.h
>>  @@ -62,6 +62,9 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
>>    * 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)
>>  +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
>>  +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
>>  +    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
>>   
>>   typedef struct {
>>       MSIMessage msg;
>>  diff --git a/include/hw/compat.h b/include/hw/compat.h
>>  index 313682a..3186275 100644
>>  --- a/include/hw/compat.h
>>  +++ b/include/hw/compat.h
>>  @@ -1,7 +1,18 @@
>>   #ifndef HW_COMPAT_H
>>   #define HW_COMPAT_H
>>   
>>  +#define HW_COMPAT_2_3 \
>>  +        {\
>>  +            .driver   = "virtio-net-pci",\
>>  +            .property = "auto_msix_bar_size",\
>>  +            .value    = "off",\
>>  +        }
>>  +
>>  +#define HW_COMPAT_2_2 \
>>  +        HW_COMPAT_2_3
>>  +
>>   #define HW_COMPAT_2_1 \
>>  +        HW_COMPAT_2_2, \
>>           {\
>>               .driver   = "intel-hda",\
>>               .property = "old_msi_addr",\
>>  -- 
>>  2.1.0

Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Jason Wang]

On Wed, Mar 18, 2015 at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Wed, Mar 18, 2015 at 05:35:09PM +0800, Jason Wang wrote:
>>  Currently we don't support more than 128 MSI-X vectors for a pci
>>  devices, trying to use vector=129 for a virtio-net-pci device may 
>> get:
>>  
>>  qemu-system-x86_64: -device virtio-net-pci,netdev=hn0,vectors=129:
>>  unable to init msix vectors to 129
>>  
>>  This this because the MSI-X bar size were hard-coded as 4096. So 
>> this
>>  patch introduces boolean auto_msix_bar_size property for
>>  virito-pci devices. Enable this will let the device calculate the 
>> msix
>>  bar size based on the number of MSI-X entries instead of previous 
>> 4096
>>  hard-coded limit.
>>  
>>  This is a must to let virtio-net can up to 256 queues and each queue
>>  were associated with a specific MSI-X entry.
>>  
>>  Cc: Paolo Bonzini <pbonzini@redhat.com>
>>  Cc: Richard Henderson <rth@twiddle.net>
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Cc: Alexander Graf <agraf@suse.de>
>>  Cc: qemu-ppc@nongnu.org
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> I don't understand what this property does.
> What if I *don't* set auto_msix_bar_size?
> vectors<=128 works exactly like it dif previously, vectors=129 fails?
> Does not seem like useful behaviour, to me.
> 
> 
>>  ---
>>   hw/i386/pc_piix.c      |  8 ++++++++
>>   hw/i386/pc_q35.c       |  8 ++++++++
>>   hw/ppc/spapr.c         | 11 ++++++++++-
>>   hw/virtio/virtio-pci.c | 17 +++++++++++++++--
>>   hw/virtio/virtio-pci.h |  3 +++
>>   include/hw/compat.h    | 11 +++++++++++
>>   6 files changed, 55 insertions(+), 3 deletions(-)
>>  
>>  diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>  index 0796719..8808500 100644
>>  --- a/hw/i386/pc_piix.c
>>  +++ b/hw/i386/pc_piix.c
>>  @@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = {
>>       PC_I440FX_2_3_MACHINE_OPTIONS,
>>       .name = "pc-i440fx-2.3",
>>       .init = pc_init_pci_2_3,
>>  +    .compat_props = (GlobalProperty[]) {
>>  +        HW_COMPAT_2_3,
>>  +        { /* end of list */ }
>>  +    },
>>   };
>>   
>>   #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
>>  @@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = {
>>       PC_I440FX_2_2_MACHINE_OPTIONS,
>>       .name = "pc-i440fx-2.2",
>>       .init = pc_init_pci_2_2,
>>  +    .compat_props = (GlobalProperty[]) {
>>  +        HW_COMPAT_2_2,
>>  +        { /* end of list */ }
>>  +    },
>>   };
>>   
>>   #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
>>  diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>  index a8a34a4..4a34349 100644
>>  --- a/hw/i386/pc_q35.c
>>  +++ b/hw/i386/pc_q35.c
>>  @@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>>       PC_Q35_2_3_MACHINE_OPTIONS,
>>       .name = "pc-q35-2.3",
>>       .init = pc_q35_init_2_3,
>>  +    .compat_props = (GlobalProperty[]) {
>>  +        HW_COMPAT_2_3,
>>  +        { /* end of list */ }
>>  +    },
>>   };
>>   
>>   #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
>>  @@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
>>       PC_Q35_2_2_MACHINE_OPTIONS,
>>       .name = "pc-q35-2.2",
>>       .init = pc_q35_init_2_2,
>>  +    .compat_props = (GlobalProperty[]) {
>>  +        HW_COMPAT_2_2,
>>  +        { /* end of list */ }
>>  +    },
>>   };
>>   
>>   #define PC_Q35_2_1_MACHINE_OPTIONS                      \
>>  diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>  index 5f25dd3..853a5cc 100644
>>  --- a/hw/ppc/spapr.c
>>  +++ b/hw/ppc/spapr.c
>>  @@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info = {
>>       },
>>   };
>>   
>>  +#define SPAPR_COMPAT_2_3 \
>>  +        HW_COMPAT_2_3
>>  +
>>   #define SPAPR_COMPAT_2_2 \
>>  +        SPAPR_COMPAT_2_3, \
>>           {\
>>               .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>>               .property = "mem_win_size",\
>>               .value    = "0x20000000",\
>>  -        }
>>  +        } \
>>   
>>   #define SPAPR_COMPAT_2_1 \
>>           SPAPR_COMPAT_2_2
>>  @@ -1883,10 +1887,15 @@ static const TypeInfo 
>> spapr_machine_2_2_info = {
>>   
>>   static void spapr_machine_2_3_class_init(ObjectClass *oc, void 
>> *data)
>>   {
>>  +    static GlobalProperty compat_props[] = {
>>  +        SPAPR_COMPAT_2_3,
>>  +        { /* end of list */ }
>>  +    };
>>       MachineClass *mc = MACHINE_CLASS(oc);
>>   
>>       mc->name = "pseries-2.3";
>>       mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
>>  +    mc->compat_props = compat_props;
>>   }
>>   
>>   static const TypeInfo spapr_machine_2_3_info = {
>>  diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>  index 4a5febb..f4cd405 100644
>>  --- a/hw/virtio/virtio-pci.c
>>  +++ b/hw/virtio/virtio-pci.c
>>  @@ -925,7 +925,7 @@ static void 
>> virtio_pci_device_plugged(DeviceState *d)
>>       VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>>       VirtioBusState *bus = &proxy->bus;
>>       uint8_t *config;
>>  -    uint32_t size;
>>  +    uint32_t size, bar_size;
>>   
>>       config = proxy->pci_dev.config;
>>       if (proxy->class_code) {
>>  @@ -936,8 +936,19 @@ static void 
>> virtio_pci_device_plugged(DeviceState *d)
>>       pci_set_word(config + PCI_SUBSYSTEM_ID, 
>> virtio_bus_get_vdev_id(bus));
>>       config[PCI_INTERRUPT_PIN] = 1;
>>   
>>  +    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
>>  +        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
>>  +        if (bar_size & (bar_size - 1)) {
>>  +            bar_size = 1 << qemu_fls(bar_size);
>>  +        }
>>  +    } else {
>>  +        /* For migration compatibility */
>>  +        bar_size = 4096;
>>  +    }
>>  +
>>       if (proxy->nvectors &&
>>  -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 
>> 1, 4096)) {
>>  +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 
>> 1,
>>  +                                bar_size)) {
>>           error_report("unable to init msix vectors to %" PRIu32,
>>                        proxy->nvectors);
>>           proxy->nvectors = 0;
> 
> 
> As I expected, msix format stuff spreads out to virtio.
> Consider "vectors * PCI_MSIX_ENTRY_SIZE * 2"
> That's because you use half the BAR for BIR in msix.c
> So any change will have to be done in two places,
> that's bad.

Yes, will move to msix.c by passing the compat flags to 
msix_init_exclusive_bar().

> 
> 
>>  @@ -1370,6 +1381,8 @@ static const TypeInfo virtio_serial_pci_info 
>> = {
>>   static Property virtio_net_properties[] = {
>>       DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>>                       VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
>>  +    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
>>  +                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
>>       DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
>>       DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
>>       DEFINE_PROP_END_OF_LIST(),
>>  diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>>  index 3bac016..82a6782 100644
>>  --- a/hw/virtio/virtio-pci.h
>>  +++ b/hw/virtio/virtio-pci.h
>>  @@ -62,6 +62,9 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
>>    * 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)
>>  +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
>>  +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
>>  +    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
>>   
>>   typedef struct {
>>       MSIMessage msg;
>>  diff --git a/include/hw/compat.h b/include/hw/compat.h
>>  index 313682a..3186275 100644
>>  --- a/include/hw/compat.h
>>  +++ b/include/hw/compat.h
>>  @@ -1,7 +1,18 @@
>>   #ifndef HW_COMPAT_H
>>   #define HW_COMPAT_H
>>   
>>  +#define HW_COMPAT_2_3 \
>>  +        {\
>>  +            .driver   = "virtio-net-pci",\
>>  +            .property = "auto_msix_bar_size",\
>>  +            .value    = "off",\
>>  +        }
>>  +
>>  +#define HW_COMPAT_2_2 \
>>  +        HW_COMPAT_2_3
>>  +
>>   #define HW_COMPAT_2_1 \
>>  +        HW_COMPAT_2_2, \
>>           {\
>>               .driver   = "intel-hda",\
>>               .property = "old_msi_addr",\
>>  -- 
>>  2.1.0

Re: [Qemu-devel] [PATCH V4 00/19] Support more virtio queues

[Jason Wang]

On Wed, Mar 18, 2015 at 8:58 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Wed, Mar 18, 2015 at 05:34:50PM +0800, Jason Wang wrote:
>>  We current limit the max virtio queues to 64. This is not sufficient
>>  to support multiqueue devices (e.g recent Linux support up to 256
>>  tap queues). So this series tries to let virtio to support more 
>> queues.
>>  
>>  No much works need to be done except:
>>  
>>  - Introducing transport specific queue limitation.
>>  - Let each virtio transport to use specific limit.
>>  - Speedup the MSI-X masking and unmasking through per vector queue
>>    list, and increase the maximum MSI-X vectors supported by qemu.
>>  - With the above enhancements, increase the maximum number of
>>    virtqueues supported by PCI from 64 to 513.
>>  - Compat the changes for legacy machine types.
> 
> What are the compatibility considerations here?

Two considerations:
1) To keep msix bar size to 4K for legacy machine types
2) Limit the pci queue max to 64 for legacy machine types

> 
> 
>>  With this patch, we can support up to 256 queues. Since x86 can only
>>  allow about 240 interrupt vectors for MSI-X, current Linux driver 
>> can
>>  only have about 80 queue pairs has their private MSI-X interrupt
>>  vectors. With sharing IRQ with queue pairs (RFC posted in
>>  https://lkml.org/lkml/2014/12/25/169), Linux driver can have up
>>  to about 186 queue pairs has their private MSI-X interrupt vectors.
>>  
>>  Stress/migration test on virtio-pci, compile test on other
>>  targets. And make check on s390x-softmmu and ppc64-softmmu.
>>  
>>  Cc: Paolo Bonzini <pbonzini@redhat.com>
>>  Cc: Richard Henderson <rth@twiddle.net>
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Cc: Alexander Graf <agraf@suse.de>
>>  Cc: Keith Busch <keith.busch@intel.com>
>>  Cc: Kevin Wolf <kwolf@redhat.com>
>>  Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>  Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>>  Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>  Cc: Amit Shah <amit.shah@redhat.com>
>>  Cc: qemu-ppc@nongnu.org
>>  
>>  Please review
>>  
>>  Thanks
>>  
>>  Changes from V3:
>>  - rebase to master and target to 2.4
>>  - handling compat issues for spapr
>>  - fixes for hmp command completion when we have a nic with 256 
>> queues
>>  - using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue in
>>    virtio-ccw
>>  - fix compile issues for ppc64-softmmu
>>  - don't export VIRTIO_CCW_QUEUE_MAX by introducing a gsi limit and
>>    inherit in ccw
>>  - use transport specific queue limit in virtio-serial
>>  - correct the stale comment for AdapterRoutes and move it to ccw 
>> patch
>>  - replace 128 with queue_max + 64 and add a comment for this in
>>    virtio_ccw_notify()
>>  
>>  Changes from V2:
>>  - move transport specific limitation to their implementation. (The
>>    left is VIRTIO_CCW_QUEUE_MAX since I fail to find a common header
>>    files other than virtio.h)
>>  - use virtio_get_queue_max() if possible in virtio.c
>>  - AdapterRoutes should use ccw limit
>>  - introduce a vector to queue mapping for virito devices and speedup
>>    the MSI-X masking and unmasking through this.
>>  
>>  Changes from V1:
>>  - add a validation against the bus limitation
>>  - switch to use a bus specific queue limit instead of a global one,
>>    this will allow us to just increase the limit of one transport
>>    without disturbing others.
>>  - only increase the queue limit of virtio-pci
>>  - limit the maximum number of virtio queues to 64 for legacy machine
>>    types
>>  
>>  Jason Wang (19):
>>    pc: add 2.4 machine types
>>    spapr: add machine type specific instance init function
>>    ppc: spapr: add 2.4 machine type
>>    monitor: replace the magic number 255 with MAX_QUEUE_NUM
>>    monitor: check return value of qemu_find_net_clients_except()
>>    virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid 
>> virtqueue
>>    virtio-net: validate backend queue numbers against bus limitation
>>    virtio-net: fix the upper bound when trying to delete queues
>>    virito: introduce bus specific queue limit
>>    virtio-ccw: introduce ccw specific queue limit
>>    virtio-s390: switch to bus specific queue limit
>>    virtio-mmio: switch to bus specific queue limit
>>    virtio-pci: switch to use bus specific queue limit
>>    virtio: introduce vector to virtqueues mapping
>>    virtio: introduce virtio_queue_get_index()
>>    virtio-pci: speedup MSI-X masking and unmasking
>>    virtio-pci: increase the maximum number of virtqueues to 513
>>    pci: remove hard-coded bar size in msix_init_exclusive_bar()
>>    virtio-pci: introduce auto_msix_bar_size property
>>  
>>   hw/block/nvme.c                |  2 +-
>>   hw/char/virtio-serial-bus.c    |  2 +-
>>   hw/i386/pc_piix.c              | 42 ++++++++++++++++++++---
>>   hw/i386/pc_q35.c               | 39 +++++++++++++++++++--
>>   hw/misc/ivshmem.c              |  2 +-
>>   hw/net/virtio-net.c            |  9 ++++-
>>   hw/pci/msix.c                  | 18 ++++------
>>   hw/ppc/spapr.c                 | 70 
>> +++++++++++++++++++++++++++++++++++--
>>   hw/s390x/s390-virtio-bus.c     |  7 ++--
>>   hw/s390x/s390-virtio-ccw.c     |  7 ++--
>>   hw/s390x/virtio-ccw.c          | 20 +++++++----
>>   hw/scsi/virtio-scsi.c          |  4 +--
>>   hw/virtio/virtio-mmio.c        |  7 ++--
>>   hw/virtio/virtio-pci.c         | 78 
>> ++++++++++++++++++++++++++++--------------
>>   hw/virtio/virtio-pci.h         |  3 ++
>>   hw/virtio/virtio.c             | 75 
>> +++++++++++++++++++++++++++++++---------
>>   include/hw/compat.h            | 11 ++++++
>>   include/hw/pci/msix.h          |  2 +-
>>   include/hw/s390x/s390_flic.h   |  4 ++-
>>   include/hw/virtio/virtio-bus.h |  2 ++
>>   include/hw/virtio/virtio.h     |  7 ++--
>>   monitor.c                      | 25 ++++++++------
>>   22 files changed, 339 insertions(+), 97 deletions(-)
>>  
>>  -- 
>>  2.1.0

Re: [Qemu-devel] [PATCH V4 07/19] virtio-net: validate backend queue numbers against bus limitation

[Jason Wang]

On Wed, Mar 18, 2015 at 9:05 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Wed, Mar 18, 2015 at 05:34:57PM +0800, Jason Wang wrote:
>>  We don't validate the backend queue numbers against bus limitation,
>>  this will easily crash qemu if it exceeds the limitation. Fixing 
>> this
>>  by doing the validation and fail early.
>>  
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> Bugfix? needed in 2.3?

Yes, a bugfix for the host only support more than 8 tuntap queues. Not 
sure it was necessary to 2.3.

> 
> 
>>  ---
>>   hw/net/virtio-net.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>  
>>  diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>  index 27adcc5..59f76bc 100644
>>  --- a/hw/net/virtio-net.c
>>  +++ b/hw/net/virtio-net.c
>>  @@ -1588,6 +1588,13 @@ static void 
>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>>       virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>>   
>>       n->max_queues = MAX(n->nic_conf.peers.queues, 1);
>>  +    if (n->max_queues * 2 + 1 > VIRTIO_PCI_QUEUE_MAX) {
> 
> We have this * 2 + 1 logic in several other places in this file too.
> Pls wrap it up in a helper.

Ok.

> 
> 
>>  +        error_setg(errp, "Invalid number of queues (= %" PRIu32 
>> "), "
>>  +                   "must be a postive integer less than %d.",
>>  +                   n->max_queues, (VIRTIO_PCI_QUEUE_MAX - 1) / 2);
>>  +        virtio_cleanup(vdev);
>>  +        return;
>>  +    }
>>       n->vqs = g_malloc0(sizeof(VirtIONetQueue) * n->max_queues);
>>       n->vqs[0].rx_vq = virtio_add_queue(vdev, 256, 
>> virtio_net_handle_rx);
>>       n->curr_queues = 1;
>>  -- 
>>  2.1.0

Re: [Qemu-devel] [PATCH V4 08/19] virtio-net: fix the upper bound when trying to delete queues

[Jason Wang]

On Wed, Mar 18, 2015 at 9:06 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Wed, Mar 18, 2015 at 05:34:58PM +0800, Jason Wang wrote:
>>  Virtqueue were indexed from zero, so don't delete virtqueue whose
>>  index is n->max_queues * 2 + 1.
>>  
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> Bugfix? needed on master? stable?

Only for host that supports more than 8 tuntap queues. But seems a 
candidate for both master and stable.

I will post this with patch 7 as an independent patches just for 2.3.

> 
>>  ---
>>   hw/net/virtio-net.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>  
>>  diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>  index 59f76bc..b6fac9c 100644
>>  --- a/hw/net/virtio-net.c
>>  +++ b/hw/net/virtio-net.c
>>  @@ -1309,7 +1309,7 @@ static void 
>> virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)
>>   
>>       n->multiqueue = multiqueue;
>>   
>>  -    for (i = 2; i <= n->max_queues * 2 + 1; i++) {
>>  +    for (i = 2; i < n->max_queues * 2 + 1; i++) {
>>           virtio_del_queue(vdev, i);
>>       }
>>   
>>  -- 
>>  2.1.0
> 

Re: [Qemu-devel] [PATCH V4 00/19] Support more virtio queues

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 01:24:53PM +0800, Jason Wang wrote:
> 
> 
> On Wed, Mar 18, 2015 at 8:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Wed, Mar 18, 2015 at 05:34:50PM +0800, Jason Wang wrote:
> >> We current limit the max virtio queues to 64. This is not sufficient
> >> to support multiqueue devices (e.g recent Linux support up to 256
> >> tap queues). So this series tries to let virtio to support more queues.
> >> No much works need to be done except:
> >> - Introducing transport specific queue limitation.
> >> - Let each virtio transport to use specific limit.
> >> - Speedup the MSI-X masking and unmasking through per vector queue
> >>   list, and increase the maximum MSI-X vectors supported by qemu.
> >> - With the above enhancements, increase the maximum number of
> >>   virtqueues supported by PCI from 64 to 513.
> >> - Compat the changes for legacy machine types.
> >
> >What are the compatibility considerations here?
> 
> Two considerations:
> 1) To keep msix bar size to 4K for legacy machine types
> 2) Limit the pci queue max to 64 for legacy machine types

2 seems not relevant to me.

> >
> >
> >> With this patch, we can support up to 256 queues. Since x86 can only
> >> allow about 240 interrupt vectors for MSI-X, current Linux driver can
> >> only have about 80 queue pairs has their private MSI-X interrupt
> >> vectors. With sharing IRQ with queue pairs (RFC posted in
> >> https://lkml.org/lkml/2014/12/25/169), Linux driver can have up
> >> to about 186 queue pairs has their private MSI-X interrupt vectors.
> >> Stress/migration test on virtio-pci, compile test on other
> >> targets. And make check on s390x-softmmu and ppc64-softmmu.
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Richard Henderson <rth@twiddle.net>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Alexander Graf <agraf@suse.de>
> >> Cc: Keith Busch <keith.busch@intel.com>
> >> Cc: Kevin Wolf <kwolf@redhat.com>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> Cc: Amit Shah <amit.shah@redhat.com>
> >> Cc: qemu-ppc@nongnu.org
> >> Please review
> >> Thanks
> >> Changes from V3:
> >> - rebase to master and target to 2.4
> >> - handling compat issues for spapr
> >> - fixes for hmp command completion when we have a nic with 256 queues
> >> - using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue in
> >>   virtio-ccw
> >> - fix compile issues for ppc64-softmmu
> >> - don't export VIRTIO_CCW_QUEUE_MAX by introducing a gsi limit and
> >>   inherit in ccw
> >> - use transport specific queue limit in virtio-serial
> >> - correct the stale comment for AdapterRoutes and move it to ccw patch
> >> - replace 128 with queue_max + 64 and add a comment for this in
> >>   virtio_ccw_notify()
> >> Changes from V2:
> >> - move transport specific limitation to their implementation. (The
> >>   left is VIRTIO_CCW_QUEUE_MAX since I fail to find a common header
> >>   files other than virtio.h)
> >> - use virtio_get_queue_max() if possible in virtio.c
> >> - AdapterRoutes should use ccw limit
> >> - introduce a vector to queue mapping for virito devices and speedup
> >>   the MSI-X masking and unmasking through this.
> >> Changes from V1:
> >> - add a validation against the bus limitation
> >> - switch to use a bus specific queue limit instead of a global one,
> >>   this will allow us to just increase the limit of one transport
> >>   without disturbing others.
> >> - only increase the queue limit of virtio-pci
> >> - limit the maximum number of virtio queues to 64 for legacy machine
> >>   types
> >> Jason Wang (19):
> >>   pc: add 2.4 machine types
> >>   spapr: add machine type specific instance init function
> >>   ppc: spapr: add 2.4 machine type
> >>   monitor: replace the magic number 255 with MAX_QUEUE_NUM
> >>   monitor: check return value of qemu_find_net_clients_except()
> >>   virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue
> >>   virtio-net: validate backend queue numbers against bus limitation
> >>   virtio-net: fix the upper bound when trying to delete queues
> >>   virito: introduce bus specific queue limit
> >>   virtio-ccw: introduce ccw specific queue limit
> >>   virtio-s390: switch to bus specific queue limit
> >>   virtio-mmio: switch to bus specific queue limit
> >>   virtio-pci: switch to use bus specific queue limit
> >>   virtio: introduce vector to virtqueues mapping
> >>   virtio: introduce virtio_queue_get_index()
> >>   virtio-pci: speedup MSI-X masking and unmasking
> >>   virtio-pci: increase the maximum number of virtqueues to 513
> >>   pci: remove hard-coded bar size in msix_init_exclusive_bar()
> >>   virtio-pci: introduce auto_msix_bar_size property
> >>  hw/block/nvme.c                |  2 +-
> >>  hw/char/virtio-serial-bus.c    |  2 +-
> >>  hw/i386/pc_piix.c              | 42 ++++++++++++++++++++---
> >>  hw/i386/pc_q35.c               | 39 +++++++++++++++++++--
> >>  hw/misc/ivshmem.c              |  2 +-
> >>  hw/net/virtio-net.c            |  9 ++++-
> >>  hw/pci/msix.c                  | 18 ++++------
> >>  hw/ppc/spapr.c                 | 70
> >>+++++++++++++++++++++++++++++++++++--
> >>  hw/s390x/s390-virtio-bus.c     |  7 ++--
> >>  hw/s390x/s390-virtio-ccw.c     |  7 ++--
> >>  hw/s390x/virtio-ccw.c          | 20 +++++++----
> >>  hw/scsi/virtio-scsi.c          |  4 +--
> >>  hw/virtio/virtio-mmio.c        |  7 ++--
> >>  hw/virtio/virtio-pci.c         | 78
> >>++++++++++++++++++++++++++++--------------
> >>  hw/virtio/virtio-pci.h         |  3 ++
> >>  hw/virtio/virtio.c             | 75
> >>+++++++++++++++++++++++++++++++---------
> >>  include/hw/compat.h            | 11 ++++++
> >>  include/hw/pci/msix.h          |  2 +-
> >>  include/hw/s390x/s390_flic.h   |  4 ++-
> >>  include/hw/virtio/virtio-bus.h |  2 ++
> >>  include/hw/virtio/virtio.h     |  7 ++--
> >>  monitor.c                      | 25 ++++++++------
> >>  22 files changed, 339 insertions(+), 97 deletions(-)
> >>   --  2.1.0

Re: [Qemu-devel] [PATCH V4 00/19] Support more virtio queues

[Jason Wang]

On Thu, Mar 19, 2015 at 3:32 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Thu, Mar 19, 2015 at 01:24:53PM +0800, Jason Wang wrote:
>>  
>>  
>>  On Wed, Mar 18, 2015 at 8:58 PM, Michael S. Tsirkin 
>> <mst@redhat.com> wrote:
>>  >On Wed, Mar 18, 2015 at 05:34:50PM +0800, Jason Wang wrote:
>>  >> We current limit the max virtio queues to 64. This is not 
>> sufficient
>>  >> to support multiqueue devices (e.g recent Linux support up to 256
>>  >> tap queues). So this series tries to let virtio to support more 
>> queues.
>>  >> No much works need to be done except:
>>  >> - Introducing transport specific queue limitation.
>>  >> - Let each virtio transport to use specific limit.
>>  >> - Speedup the MSI-X masking and unmasking through per vector 
>> queue
>>  >>   list, and increase the maximum MSI-X vectors supported by qemu.
>>  >> - With the above enhancements, increase the maximum number of
>>  >>   virtqueues supported by PCI from 64 to 513.
>>  >> - Compat the changes for legacy machine types.
>>  >
>>  >What are the compatibility considerations here?
>>  
>>  Two considerations:
>>  1) To keep msix bar size to 4K for legacy machine types
>>  2) Limit the pci queue max to 64 for legacy machine types
> 
> 2 seems not relevant to me.

If we don't limit this. Consider migration from 2.4 to 2.3

Before migration

write 0 to queue_sel
write 100 to queue_sel
read queue_sel will get 100

but after migration 

write 0 to queue_sel
write 100 to queue_sel
read queue_sel will get 0

The hardware behavior was changed after migration.

Another reason is not wasting memory for the extra virtqueues allocated 
for legacy machine types.

Re: [Qemu-devel] [PATCH V4 00/19] Support more virtio queues

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 03:42:56PM +0800, Jason Wang wrote:
> 
> 
> On Thu, Mar 19, 2015 at 3:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Thu, Mar 19, 2015 at 01:24:53PM +0800, Jason Wang wrote:
> >>     On Wed, Mar 18, 2015 at 8:58 PM, Michael S. Tsirkin
> >><mst@redhat.com> wrote:
> >> >On Wed, Mar 18, 2015 at 05:34:50PM +0800, Jason Wang wrote:
> >> >> We current limit the max virtio queues to 64. This is not sufficient
> >> >> to support multiqueue devices (e.g recent Linux support up to 256
> >> >> tap queues). So this series tries to let virtio to support more
> >>queues.
> >> >> No much works need to be done except:
> >> >> - Introducing transport specific queue limitation.
> >> >> - Let each virtio transport to use specific limit.
> >> >> - Speedup the MSI-X masking and unmasking through per vector queue
> >> >>   list, and increase the maximum MSI-X vectors supported by qemu.
> >> >> - With the above enhancements, increase the maximum number of
> >> >>   virtqueues supported by PCI from 64 to 513.
> >> >> - Compat the changes for legacy machine types.
> >> >
> >> >What are the compatibility considerations here?
> >> Two considerations:
> >> 1) To keep msix bar size to 4K for legacy machine types
> >> 2) Limit the pci queue max to 64 for legacy machine types
> >
> >2 seems not relevant to me.
> 
> If we don't limit this. Consider migration from 2.4 to 2.3
> 
> Before migration
> 
> write 0 to queue_sel
> write 100 to queue_sel
> read queue_sel will get 100
> 
> but after migration
> 
> write 0 to queue_sel
> write 100 to queue_sel
> read queue_sel will get 0
> 
> The hardware behavior was changed after migration.

But this driver is out of spec - drivers are not supposed to select
non-existent queues. So this doesn't matter.

> Another reason is not wasting memory for the extra virtqueues allocated for
> legacy machine types.

If that's a significant amount of memory, we need to work
to reduce memory consumption for new machine types.
Not many people use the compat machine types, especially
upstream.

-- 
MST

Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 01:23:12PM +0800, Jason Wang wrote:
> 
> 
> On Wed, Mar 18, 2015 at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Wed, Mar 18, 2015 at 05:35:09PM +0800, Jason Wang wrote:
> >> Currently we don't support more than 128 MSI-X vectors for a pci
> >> devices, trying to use vector=129 for a virtio-net-pci device may get:
> >> qemu-system-x86_64: -device virtio-net-pci,netdev=hn0,vectors=129:
> >> unable to init msix vectors to 129
> >>   This this because the MSI-X bar size were hard-coded as 4096. So this
> >> patch introduces boolean auto_msix_bar_size property for
> >> virito-pci devices. Enable this will let the device calculate the msix
> >> bar size based on the number of MSI-X entries instead of previous 4096
> >> hard-coded limit.
> >> This is a must to let virtio-net can up to 256 queues and each queue
> >> were associated with a specific MSI-X entry.
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Richard Henderson <rth@twiddle.net>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Alexander Graf <agraf@suse.de>
> >> Cc: qemu-ppc@nongnu.org
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> >I don't understand what this property does.
> >What if I *don't* set auto_msix_bar_size?
> >vectors<=128 works exactly like it dif previously, vectors=129 fails?
> 
> Yes, but there looks like a bug in the code which can lead a bar size less
> than 4K.
> 
> >
> >Does not seem like useful behaviour, to me.
> 
> This property allows to have more than 128 vectors to be used. We disable
> this for legacy machine types to stick bar size to 4K to keep the migration
> compatibility.

Compatibility between which two configurations?
qemu 2.3 can not run when > 128 vectors are requested.
So there is no need to worry about what happens when you
request > 128 vectors and an old machine type.
qemu exiting is not guest visible behaviour.

I can imagine two reasonable solutions:
1. increase bar size for everyone. make it 8k for compat machine types
	simple but wastes memory
2. make bar size depend on # of vectors
	as # of vectors is user-specified, there's no problem
	with compatibility, so no need to tweak machine types,
	but layout is dynamic so more complex.


> 
> >
> >
> >> ---
> >>  hw/i386/pc_piix.c      |  8 ++++++++
> >>  hw/i386/pc_q35.c       |  8 ++++++++
> >>  hw/ppc/spapr.c         | 11 ++++++++++-
> >>  hw/virtio/virtio-pci.c | 17 +++++++++++++++--
> >>  hw/virtio/virtio-pci.h |  3 +++
> >>  include/hw/compat.h    | 11 +++++++++++
> >>  6 files changed, 55 insertions(+), 3 deletions(-)
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 0796719..8808500 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = {
> >>      PC_I440FX_2_3_MACHINE_OPTIONS,
> >>      .name = "pc-i440fx-2.3",
> >>      .init = pc_init_pci_2_3,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_3,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>  #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
> >> @@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = {
> >>      PC_I440FX_2_2_MACHINE_OPTIONS,
> >>      .name = "pc-i440fx-2.2",
> >>      .init = pc_init_pci_2_2,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_2,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>  #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index a8a34a4..4a34349 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
> >>      PC_Q35_2_3_MACHINE_OPTIONS,
> >>      .name = "pc-q35-2.3",
> >>      .init = pc_q35_init_2_3,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_3,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
> >> @@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
> >>      PC_Q35_2_2_MACHINE_OPTIONS,
> >>      .name = "pc-q35-2.2",
> >>      .init = pc_q35_init_2_2,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_2,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>  #define PC_Q35_2_1_MACHINE_OPTIONS                      \
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 5f25dd3..853a5cc 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info = {
> >>      },
> >>  };
> >> +#define SPAPR_COMPAT_2_3 \
> >> +        HW_COMPAT_2_3
> >> +
> >>  #define SPAPR_COMPAT_2_2 \
> >> +        SPAPR_COMPAT_2_3, \
> >>          {\
> >>              .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
> >>              .property = "mem_win_size",\
> >>              .value    = "0x20000000",\
> >> -        }
> >> +        } \
> >>  #define SPAPR_COMPAT_2_1 \
> >>          SPAPR_COMPAT_2_2
> >> @@ -1883,10 +1887,15 @@ static const TypeInfo spapr_machine_2_2_info =
> >>{
> >>     static void spapr_machine_2_3_class_init(ObjectClass *oc, void
> >>*data)
> >>  {
> >> +    static GlobalProperty compat_props[] = {
> >> +        SPAPR_COMPAT_2_3,
> >> +        { /* end of list */ }
> >> +    };
> >>      MachineClass *mc = MACHINE_CLASS(oc);
> >>      mc->name = "pseries-2.3";
> >>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
> >> +    mc->compat_props = compat_props;
> >>  }
> >>  static const TypeInfo spapr_machine_2_3_info = {
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index 4a5febb..f4cd405 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -925,7 +925,7 @@ static void virtio_pci_device_plugged(DeviceState
> >>*d)
> >>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> >>      VirtioBusState *bus = &proxy->bus;
> >>      uint8_t *config;
> >> -    uint32_t size;
> >> +    uint32_t size, bar_size;
> >>      config = proxy->pci_dev.config;
> >>      if (proxy->class_code) {
> >> @@ -936,8 +936,19 @@ static void virtio_pci_device_plugged(DeviceState
> >>*d)
> >>      pci_set_word(config + PCI_SUBSYSTEM_ID,
> >>virtio_bus_get_vdev_id(bus));
> >>      config[PCI_INTERRUPT_PIN] = 1;
> >> +    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
> >> +        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
> >> +        if (bar_size & (bar_size - 1)) {
> >> +            bar_size = 1 << qemu_fls(bar_size);
> >> +        }
> >> +    } else {
> >> +        /* For migration compatibility */
> >> +        bar_size = 4096;
> >> +    }
> >> +
> >>      if (proxy->nvectors &&
> >> -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1,
> >>4096)) {
> >> +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1,
> >> +                                bar_size)) {
> >>          error_report("unable to init msix vectors to %" PRIu32,
> >>                       proxy->nvectors);
> >>          proxy->nvectors = 0;
> >
> >
> >As I expected, msix format stuff spreads out to virtio.
> >Consider "vectors * PCI_MSIX_ENTRY_SIZE * 2"
> >That's because you use half the BAR for BIR in msix.c
> >So any change will have to be done in two places,
> >that's bad.
> >
> >
> >> @@ -1370,6 +1381,8 @@ static const TypeInfo virtio_serial_pci_info = {
> >>  static Property virtio_net_properties[] = {
> >>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
> >>                      VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
> >> +    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
> >> +                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
> >>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
> >>      DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
> >>      DEFINE_PROP_END_OF_LIST(),
> >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> >> index 3bac016..82a6782 100644
> >> --- a/hw/virtio/virtio-pci.h
> >> +++ b/hw/virtio/virtio-pci.h
> >> @@ -62,6 +62,9 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
> >>   * 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)
> >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
> >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
> >> +    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
> >>  typedef struct {
> >>      MSIMessage msg;
> >> diff --git a/include/hw/compat.h b/include/hw/compat.h
> >> index 313682a..3186275 100644
> >> --- a/include/hw/compat.h
> >> +++ b/include/hw/compat.h
> >> @@ -1,7 +1,18 @@
> >>  #ifndef HW_COMPAT_H
> >>  #define HW_COMPAT_H
> >> +#define HW_COMPAT_2_3 \
> >> +        {\
> >> +            .driver   = "virtio-net-pci",\
> >> +            .property = "auto_msix_bar_size",\
> >> +            .value    = "off",\
> >> +        }
> >> +
> >> +#define HW_COMPAT_2_2 \
> >> +        HW_COMPAT_2_3
> >> +
> >>  #define HW_COMPAT_2_1 \
> >> +        HW_COMPAT_2_2, \
> >>          {\
> >>              .driver   = "intel-hda",\
> >>              .property = "old_msi_addr",\
> >> --  2.1.0

Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 01:23:56PM +0800, Jason Wang wrote:
> 
> 
> On Wed, Mar 18, 2015 at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Wed, Mar 18, 2015 at 05:35:09PM +0800, Jason Wang wrote:
> >> Currently we don't support more than 128 MSI-X vectors for a pci
> >> devices, trying to use vector=129 for a virtio-net-pci device may get:
> >> qemu-system-x86_64: -device virtio-net-pci,netdev=hn0,vectors=129:
> >> unable to init msix vectors to 129
> >>   This this because the MSI-X bar size were hard-coded as 4096. So this
> >> patch introduces boolean auto_msix_bar_size property for
> >> virito-pci devices. Enable this will let the device calculate the msix
> >> bar size based on the number of MSI-X entries instead of previous 4096
> >> hard-coded limit.
> >> This is a must to let virtio-net can up to 256 queues and each queue
> >> were associated with a specific MSI-X entry.
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Richard Henderson <rth@twiddle.net>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Alexander Graf <agraf@suse.de>
> >> Cc: qemu-ppc@nongnu.org
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> >I don't understand what this property does.
> >What if I *don't* set auto_msix_bar_size?
> >vectors<=128 works exactly like it dif previously, vectors=129 fails?
> >Does not seem like useful behaviour, to me.
> >
> >
> >> ---
> >>  hw/i386/pc_piix.c      |  8 ++++++++
> >>  hw/i386/pc_q35.c       |  8 ++++++++
> >>  hw/ppc/spapr.c         | 11 ++++++++++-
> >>  hw/virtio/virtio-pci.c | 17 +++++++++++++++--
> >>  hw/virtio/virtio-pci.h |  3 +++
> >>  include/hw/compat.h    | 11 +++++++++++
> >>  6 files changed, 55 insertions(+), 3 deletions(-)
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 0796719..8808500 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = {
> >>      PC_I440FX_2_3_MACHINE_OPTIONS,
> >>      .name = "pc-i440fx-2.3",
> >>      .init = pc_init_pci_2_3,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_3,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>  #define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
> >> @@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = {
> >>      PC_I440FX_2_2_MACHINE_OPTIONS,
> >>      .name = "pc-i440fx-2.2",
> >>      .init = pc_init_pci_2_2,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_2,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>  #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index a8a34a4..4a34349 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
> >>      PC_Q35_2_3_MACHINE_OPTIONS,
> >>      .name = "pc-q35-2.3",
> >>      .init = pc_q35_init_2_3,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_3,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
> >> @@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
> >>      PC_Q35_2_2_MACHINE_OPTIONS,
> >>      .name = "pc-q35-2.2",
> >>      .init = pc_q35_init_2_2,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        HW_COMPAT_2_2,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>  #define PC_Q35_2_1_MACHINE_OPTIONS                      \
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 5f25dd3..853a5cc 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info = {
> >>      },
> >>  };
> >> +#define SPAPR_COMPAT_2_3 \
> >> +        HW_COMPAT_2_3
> >> +
> >>  #define SPAPR_COMPAT_2_2 \
> >> +        SPAPR_COMPAT_2_3, \
> >>          {\
> >>              .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
> >>              .property = "mem_win_size",\
> >>              .value    = "0x20000000",\
> >> -        }
> >> +        } \
> >>  #define SPAPR_COMPAT_2_1 \
> >>          SPAPR_COMPAT_2_2
> >> @@ -1883,10 +1887,15 @@ static const TypeInfo spapr_machine_2_2_info =
> >>{
> >>     static void spapr_machine_2_3_class_init(ObjectClass *oc, void
> >>*data)
> >>  {
> >> +    static GlobalProperty compat_props[] = {
> >> +        SPAPR_COMPAT_2_3,
> >> +        { /* end of list */ }
> >> +    };
> >>      MachineClass *mc = MACHINE_CLASS(oc);
> >>      mc->name = "pseries-2.3";
> >>      mc->desc = "pSeries Logical Partition (PAPR compliant) v2.3";
> >> +    mc->compat_props = compat_props;
> >>  }
> >>  static const TypeInfo spapr_machine_2_3_info = {
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index 4a5febb..f4cd405 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -925,7 +925,7 @@ static void virtio_pci_device_plugged(DeviceState
> >>*d)
> >>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> >>      VirtioBusState *bus = &proxy->bus;
> >>      uint8_t *config;
> >> -    uint32_t size;
> >> +    uint32_t size, bar_size;
> >>      config = proxy->pci_dev.config;
> >>      if (proxy->class_code) {
> >> @@ -936,8 +936,19 @@ static void virtio_pci_device_plugged(DeviceState
> >>*d)
> >>      pci_set_word(config + PCI_SUBSYSTEM_ID,
> >>virtio_bus_get_vdev_id(bus));
> >>      config[PCI_INTERRUPT_PIN] = 1;
> >> +    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
> >> +        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
> >> +        if (bar_size & (bar_size - 1)) {
> >> +            bar_size = 1 << qemu_fls(bar_size);
> >> +        }
> >> +    } else {
> >> +        /* For migration compatibility */
> >> +        bar_size = 4096;
> >> +    }
> >> +
> >>      if (proxy->nvectors &&
> >> -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1,
> >>4096)) {
> >> +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1,
> >> +                                bar_size)) {
> >>          error_report("unable to init msix vectors to %" PRIu32,
> >>                       proxy->nvectors);
> >>          proxy->nvectors = 0;
> >
> >
> >As I expected, msix format stuff spreads out to virtio.
> >Consider "vectors * PCI_MSIX_ENTRY_SIZE * 2"
> >That's because you use half the BAR for BIR in msix.c
> >So any change will have to be done in two places,
> >that's bad.
> 
> Yes, will move to msix.c by passing the compat flags to
> msix_init_exclusive_bar().

I really wonder why isn't just specifying # of vectors enough.


> >
> >
> >> @@ -1370,6 +1381,8 @@ static const TypeInfo virtio_serial_pci_info = {
> >>  static Property virtio_net_properties[] = {
> >>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
> >>                      VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
> >> +    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
> >> +                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
> >>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
> >>      DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
> >>      DEFINE_PROP_END_OF_LIST(),
> >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> >> index 3bac016..82a6782 100644
> >> --- a/hw/virtio/virtio-pci.h
> >> +++ b/hw/virtio/virtio-pci.h
> >> @@ -62,6 +62,9 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
> >>   * 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)
> >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
> >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
> >> +    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
> >>  typedef struct {
> >>      MSIMessage msg;
> >> diff --git a/include/hw/compat.h b/include/hw/compat.h
> >> index 313682a..3186275 100644
> >> --- a/include/hw/compat.h
> >> +++ b/include/hw/compat.h
> >> @@ -1,7 +1,18 @@
> >>  #ifndef HW_COMPAT_H
> >>  #define HW_COMPAT_H
> >> +#define HW_COMPAT_2_3 \
> >> +        {\
> >> +            .driver   = "virtio-net-pci",\
> >> +            .property = "auto_msix_bar_size",\
> >> +            .value    = "off",\
> >> +        }
> >> +
> >> +#define HW_COMPAT_2_2 \
> >> +        HW_COMPAT_2_3
> >> +
> >>  #define HW_COMPAT_2_1 \
> >> +        HW_COMPAT_2_2, \
> >>          {\
> >>              .driver   = "intel-hda",\
> >>              .property = "old_msi_addr",\
> >> --  2.1.0

Re: [Qemu-devel] [PATCH V4 18/19] pci: remove hard-coded bar size in msix_init_exclusive_bar()

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 01:19:23PM +0800, Jason Wang wrote:
> 
> 
> On Wed, Mar 18, 2015 at 8:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Wed, Mar 18, 2015 at 05:35:08PM +0800, Jason Wang wrote:
> >> This patch let msix_init_exclusive_bar() can accept bar_size parameter
> >> other than a hard-coded limit 4096. Then caller can specify a bar_size
> >> depends on msix entries and can use up to 2048 msix entries as PCI
> >> spec allows. To keep migration compatibility, 4096 is used for all
> >> callers and pba were start from half of bar size.
> >> Cc: Keith Busch <keith.busch@intel.com>
> >> Cc: Kevin Wolf <kwolf@redhat.com>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> >Hmm this API looks strange. Caller will then have to have
> >msi specific knowledge.
> >Can't we keep the size iternal to msix.c?
> 
> Looks like we can, but still need an extra parameter e.g a boolean
> compat_size to let msix_init_exclusive_bar() to use 4096 as bar size.

Why not just figure the size out from # of vectors?

> >
> >
> >> ---
> >>  hw/block/nvme.c        |  2 +-
> >>  hw/misc/ivshmem.c      |  2 +-
> >>  hw/pci/msix.c          | 18 +++++++-----------
> >>  hw/virtio/virtio-pci.c |  2 +-
> >>  include/hw/pci/msix.h  |  2 +-
> >>  5 files changed, 11 insertions(+), 15 deletions(-)
> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >> index 0f3dfb9..09d7884 100644
> >> --- a/hw/block/nvme.c
> >> +++ b/hw/block/nvme.c
> >> @@ -787,7 +787,7 @@ static int nvme_init(PCIDevice *pci_dev)
> >>      pci_register_bar(&n->parent_obj, 0,
> >>          PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
> >>          &n->iomem);
> >> -    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4);
> >> +    msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, 4096);
> >>      id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
> >>      id->ssvid = cpu_to_le16(pci_get_word(pci_conf +
> >>PCI_SUBSYSTEM_VENDOR_ID));
> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> index 5d272c8..3e2a2d4 100644
> >> --- a/hw/misc/ivshmem.c
> >> +++ b/hw/misc/ivshmem.c
> >> @@ -631,7 +631,7 @@ static uint64_t ivshmem_get_size(IVShmemState * s)
> >>{
> >>  static void ivshmem_setup_msi(IVShmemState * s)
> >>  {
> >> -    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) {
> >> +    if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, 4096)) {
> >>          IVSHMEM_DPRINTF("msix initialization failed\n");
> >>          exit(1);
> >>      }
> >> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> >> index 24de260..9a1894f 100644
> >> --- a/hw/pci/msix.c
> >> +++ b/hw/pci/msix.c
> >> @@ -291,33 +291,29 @@ int msix_init(struct PCIDevice *dev, unsigned
> >>short nentries,
> >>  }
> >>     int msix_init_exclusive_bar(PCIDevice *dev, unsigned short
> >>nentries,
> >> -                            uint8_t bar_nr)
> >> +                            uint8_t bar_nr, uint32_t bar_size)
> >>  {
> >>      int ret;
> >>      char *name;
> >> +    uint32_t bar_pba_offset = bar_size / 2;
> >
> >Spec says we must give BIR 4k of it's own, but for
> >larger BARs, using up half the BAR just for BIR seems
> >wasteful.
> 
> I see, I will make PBA size more accurate.
> 
> >
> >
> >>      /*
> >>       * Migration compatibility dictates that this remains a 4k
> >>       * BAR with the vector table in the lower half and PBA in
> >>       * the upper half.  Do not use these elsewhere!
> >>       */
> >
> >You've left comment here where it no longer makes
> >any sense.
> 
> Will remove this.
> 
> >
> >
> >> -#define MSIX_EXCLUSIVE_BAR_SIZE 4096
> >> -#define MSIX_EXCLUSIVE_BAR_TABLE_OFFSET 0
> >> -#define MSIX_EXCLUSIVE_BAR_PBA_OFFSET (MSIX_EXCLUSIVE_BAR_SIZE / 2)
> >> -#define MSIX_EXCLUSIVE_CAP_OFFSET 0
> >> -
> >> -    if (nentries * PCI_MSIX_ENTRY_SIZE >
> >>MSIX_EXCLUSIVE_BAR_PBA_OFFSET) {
> >> +    if (nentries * PCI_MSIX_ENTRY_SIZE > bar_pba_offset) {
> >>          return -EINVAL;
> >>      }
> >>      name = g_strdup_printf("%s-msix", dev->name);
> >> -    memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name,
> >>MSIX_EXCLUSIVE_BAR_SIZE);
> >> +    memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name,
> >>bar_size);
> >>      g_free(name);
> >>         ret = msix_init(dev, nentries, &dev->msix_exclusive_bar,
> >>bar_nr,
> >> -                    MSIX_EXCLUSIVE_BAR_TABLE_OFFSET,
> >>&dev->msix_exclusive_bar,
> >> -                    bar_nr, MSIX_EXCLUSIVE_BAR_PBA_OFFSET,
> >> -                    MSIX_EXCLUSIVE_CAP_OFFSET);
> >> +                    0, &dev->msix_exclusive_bar,
> >> +                    bar_nr, bar_pba_offset,
> >> +                    0);
> >>      if (ret) {
> >>          return ret;
> >>      }
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index 02e3ce8..4a5febb 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -937,7 +937,7 @@ static void virtio_pci_device_plugged(DeviceState
> >>*d)
> >>      config[PCI_INTERRUPT_PIN] = 1;
> >>      if (proxy->nvectors &&
> >> -        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1))
> >>{
> >> +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1,
> >>4096)) {
> >>          error_report("unable to init msix vectors to %" PRIu32,
> >>                       proxy->nvectors);
> >>          proxy->nvectors = 0;
> >> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> >> index 954d82b..43edebc 100644
> >> --- a/include/hw/pci/msix.h
> >> +++ b/include/hw/pci/msix.h
> >> @@ -11,7 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short
> >>nentries,
> >>                unsigned table_offset, MemoryRegion *pba_bar,
> >>                uint8_t pba_bar_nr, unsigned pba_offset, uint8_t
> >>cap_pos);
> >>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >> -                            uint8_t bar_nr);
> >> +                            uint8_t bar_nr, uint32_t bar_size);
> >>     void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t
> >>val, int len);
> >>    --  2.1.0

Re: [Qemu-devel] [PATCH V4 00/19] Support more virtio queues

[Jason Wang]

On Thu, Mar 19, 2015 at 5:23 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Thu, Mar 19, 2015 at 03:42:56PM +0800, Jason Wang wrote:
>>  
>>  
>>  On Thu, Mar 19, 2015 at 3:32 PM, Michael S. Tsirkin 
>> <mst@redhat.com> wrote:
>>  >On Thu, Mar 19, 2015 at 01:24:53PM +0800, Jason Wang wrote:
>>  >>     On Wed, Mar 18, 2015 at 8:58 PM, Michael S. Tsirkin
>>  >><mst@redhat.com> wrote:
>>  >> >On Wed, Mar 18, 2015 at 05:34:50PM +0800, Jason Wang wrote:
>>  >> >> We current limit the max virtio queues to 64. This is not 
>> sufficient
>>  >> >> to support multiqueue devices (e.g recent Linux support up to 
>> 256
>>  >> >> tap queues). So this series tries to let virtio to support 
>> more
>>  >>queues.
>>  >> >> No much works need to be done except:
>>  >> >> - Introducing transport specific queue limitation.
>>  >> >> - Let each virtio transport to use specific limit.
>>  >> >> - Speedup the MSI-X masking and unmasking through per vector 
>> queue
>>  >> >>   list, and increase the maximum MSI-X vectors supported by 
>> qemu.
>>  >> >> - With the above enhancements, increase the maximum number of
>>  >> >>   virtqueues supported by PCI from 64 to 513.
>>  >> >> - Compat the changes for legacy machine types.
>>  >> >
>>  >> >What are the compatibility considerations here?
>>  >> Two considerations:
>>  >> 1) To keep msix bar size to 4K for legacy machine types
>>  >> 2) Limit the pci queue max to 64 for legacy machine types
>>  >
>>  >2 seems not relevant to me.
>>  
>>  If we don't limit this. Consider migration from 2.4 to 2.3
>>  
>>  Before migration
>>  
>>  write 0 to queue_sel
>>  write 100 to queue_sel
>>  read queue_sel will get 100
>>  
>>  but after migration
>>  
>>  write 0 to queue_sel
>>  write 100 to queue_sel
>>  read queue_sel will get 0
>>  
>>  The hardware behavior was changed after migration.
> 
> But this driver is out of spec - drivers are not supposed to select
> non-existent queues. So this doesn't matter.

Technically, we need make sure there's not change in the behavior after 
migration. The fix is not hard and leaving thing likes this will make 
it hard to debug the issue after migration and it will be too late to 
fix if we find a 'buggy' driver in the future.

For the spec, we have already had a lot of examples that the driver or 
device was out of spec, we could not make sure that there will be no 
driver who depends on undocumented behavior. 
  
> 
> 
>>  Another reason is not wasting memory for the extra virtqueues 
>> allocated for
>>  legacy machine types.
> 
> If that's a significant amount of memory, we need to work
> to reduce memory consumption for new machine types.

It will save about 38K if 513 is queue max, and will save more if we 
want to increase the limit in the future or new member was added to 
VirtQueue.

> 
> Not many people use the compat machine types, especially
> upstream.
> 
> -- 
> MST

But there's still user and we do a lot to maintain compatibility even 
in upstream. Another side, this can also reduce the downstream specific 
codes.

Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Jason Wang]

On Thu, Mar 19, 2015 at 6:01 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Thu, Mar 19, 2015 at 01:23:12PM +0800, Jason Wang wrote:
>>  
>>  
>>  On Wed, Mar 18, 2015 at 8:57 PM, Michael S. Tsirkin 
>> <mst@redhat.com> wrote:
>>  >On Wed, Mar 18, 2015 at 05:35:09PM +0800, Jason Wang wrote:
>>  >> Currently we don't support more than 128 MSI-X vectors for a pci
>>  >> devices, trying to use vector=129 for a virtio-net-pci device 
>> may get:
>>  >> qemu-system-x86_64: -device 
>> virtio-net-pci,netdev=hn0,vectors=129:
>>  >> unable to init msix vectors to 129
>>  >>   This this because the MSI-X bar size were hard-coded as 4096. 
>> So this
>>  >> patch introduces boolean auto_msix_bar_size property for
>>  >> virito-pci devices. Enable this will let the device calculate 
>> the msix
>>  >> bar size based on the number of MSI-X entries instead of 
>> previous 4096
>>  >> hard-coded limit.
>>  >> This is a must to let virtio-net can up to 256 queues and each 
>> queue
>>  >> were associated with a specific MSI-X entry.
>>  >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>  >> Cc: Richard Henderson <rth@twiddle.net>
>>  >> Cc: Michael S. Tsirkin <mst@redhat.com>
>>  >> Cc: Alexander Graf <agraf@suse.de>
>>  >> Cc: qemu-ppc@nongnu.org
>>  >> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>  >
>>  >I don't understand what this property does.
>>  >What if I *don't* set auto_msix_bar_size?
>>  >vectors<=128 works exactly like it dif previously, vectors=129 
>> fails?
>>  
>>  Yes, but there looks like a bug in the code which can lead a bar 
>> size less
>>  than 4K.
>>  
>>  >
>>  >Does not seem like useful behaviour, to me.
>>  
>>  This property allows to have more than 128 vectors to be used. We 
>> disable
>>  this for legacy machine types to stick bar size to 4K to keep the 
>> migration
>>  compatibility.
> 
> Compatibility between which two configurations?

I mean migration from 2.4 to 2.3 with e.g vectors=129.

> 
> qemu 2.3 can not run when > 128 vectors are requested.

Unfortunately not, qemu can run with just a warning like:

qemu-system-x86_64: -device virtio-net-pci,vectors=129: unable to init 
msix vectors to 129

vectors will reset to zero in this case.
> 
> So there is no need to worry about what happens when you
> request > 128 vectors and an old machine type.
> qemu exiting is not guest visible behaviour.
> 
> I can imagine two reasonable solutions:
> 1. increase bar size for everyone. make it 8k for compat machine types
> 	simple but wastes memory
> 2. make bar size depend on # of vectors
> 	as # of vectors is user-specified, there's no problem
> 	with compatibility, so no need to tweak machine types,
> 	but layout is dynamic so more complex.

This is what I want to do. And since vectors=129 won't crash for 2.3. 
Still need to keep the 4k for compat machine types.
> 
> 
> 
>>  
>>  >
>>  >
>>  >> ---
>>  >>  hw/i386/pc_piix.c      |  8 ++++++++
>>  >>  hw/i386/pc_q35.c       |  8 ++++++++
>>  >>  hw/ppc/spapr.c         | 11 ++++++++++-
>>  >>  hw/virtio/virtio-pci.c | 17 +++++++++++++++--
>>  >>  hw/virtio/virtio-pci.h |  3 +++
>>  >>  include/hw/compat.h    | 11 +++++++++++
>>  >>  6 files changed, 55 insertions(+), 3 deletions(-)
>>  >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>  >> index 0796719..8808500 100644
>>  >> --- a/hw/i386/pc_piix.c
>>  >> +++ b/hw/i386/pc_piix.c
>>  >> @@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = 
>> {
>>  >>      PC_I440FX_2_3_MACHINE_OPTIONS,
>>  >>      .name = "pc-i440fx-2.3",
>>  >>      .init = pc_init_pci_2_3,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_I440FX_2_2_MACHINE_OPTIONS 
>> PC_I440FX_2_3_MACHINE_OPTIONS
>>  >> @@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = 
>> {
>>  >>      PC_I440FX_2_2_MACHINE_OPTIONS,
>>  >>      .name = "pc-i440fx-2.2",
>>  >>      .init = pc_init_pci_2_2,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_2,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_I440FX_2_1_MACHINE_OPTIONS                           
>> \
>>  >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>  >> index a8a34a4..4a34349 100644
>>  >> --- a/hw/i386/pc_q35.c
>>  >> +++ b/hw/i386/pc_q35.c
>>  >> @@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>>  >>      PC_Q35_2_3_MACHINE_OPTIONS,
>>  >>      .name = "pc-q35-2.3",
>>  >>      .init = pc_q35_init_2_3,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
>>  >> @@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
>>  >>      PC_Q35_2_2_MACHINE_OPTIONS,
>>  >>      .name = "pc-q35-2.2",
>>  >>      .init = pc_q35_init_2_2,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_2,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_Q35_2_1_MACHINE_OPTIONS                      \
>>  >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>  >> index 5f25dd3..853a5cc 100644
>>  >> --- a/hw/ppc/spapr.c
>>  >> +++ b/hw/ppc/spapr.c
>>  >> @@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info 
>> = {
>>  >>      },
>>  >>  };
>>  >> +#define SPAPR_COMPAT_2_3 \
>>  >> +        HW_COMPAT_2_3
>>  >> +
>>  >>  #define SPAPR_COMPAT_2_2 \
>>  >> +        SPAPR_COMPAT_2_3, \
>>  >>          {\
>>  >>              .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>>  >>              .property = "mem_win_size",\
>>  >>              .value    = "0x20000000",\
>>  >> -        }
>>  >> +        } \
>>  >>  #define SPAPR_COMPAT_2_1 \
>>  >>          SPAPR_COMPAT_2_2
>>  >> @@ -1883,10 +1887,15 @@ static const TypeInfo 
>> spapr_machine_2_2_info =
>>  >>{
>>  >>     static void spapr_machine_2_3_class_init(ObjectClass *oc, 
>> void
>>  >>*data)
>>  >>  {
>>  >> +    static GlobalProperty compat_props[] = {
>>  >> +        SPAPR_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    };
>>  >>      MachineClass *mc = MACHINE_CLASS(oc);
>>  >>      mc->name = "pseries-2.3";
>>  >>      mc->desc = "pSeries Logical Partition (PAPR compliant) 
>> v2.3";
>>  >> +    mc->compat_props = compat_props;
>>  >>  }
>>  >>  static const TypeInfo spapr_machine_2_3_info = {
>>  >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>  >> index 4a5febb..f4cd405 100644
>>  >> --- a/hw/virtio/virtio-pci.c
>>  >> +++ b/hw/virtio/virtio-pci.c
>>  >> @@ -925,7 +925,7 @@ static void 
>> virtio_pci_device_plugged(DeviceState
>>  >>*d)
>>  >>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>>  >>      VirtioBusState *bus = &proxy->bus;
>>  >>      uint8_t *config;
>>  >> -    uint32_t size;
>>  >> +    uint32_t size, bar_size;
>>  >>      config = proxy->pci_dev.config;
>>  >>      if (proxy->class_code) {
>>  >> @@ -936,8 +936,19 @@ static void 
>> virtio_pci_device_plugged(DeviceState
>>  >>*d)
>>  >>      pci_set_word(config + PCI_SUBSYSTEM_ID,
>>  >>virtio_bus_get_vdev_id(bus));
>>  >>      config[PCI_INTERRUPT_PIN] = 1;
>>  >> +    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
>>  >> +        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
>>  >> +        if (bar_size & (bar_size - 1)) {
>>  >> +            bar_size = 1 << qemu_fls(bar_size);
>>  >> +        }
>>  >> +    } else {
>>  >> +        /* For migration compatibility */
>>  >> +        bar_size = 4096;
>>  >> +    }
>>  >> +
>>  >>      if (proxy->nvectors &&
>>  >> -        msix_init_exclusive_bar(&proxy->pci_dev, 
>> proxy->nvectors, 1,
>>  >>4096)) {
>>  >> +        msix_init_exclusive_bar(&proxy->pci_dev, 
>> proxy->nvectors, 1,
>>  >> +                                bar_size)) {
>>  >>          error_report("unable to init msix vectors to %" PRIu32,
>>  >>                       proxy->nvectors);
>>  >>          proxy->nvectors = 0;
>>  >
>>  >
>>  >As I expected, msix format stuff spreads out to virtio.
>>  >Consider "vectors * PCI_MSIX_ENTRY_SIZE * 2"
>>  >That's because you use half the BAR for BIR in msix.c
>>  >So any change will have to be done in two places,
>>  >that's bad.
>>  >
>>  >
>>  >> @@ -1370,6 +1381,8 @@ static const TypeInfo 
>> virtio_serial_pci_info = {
>>  >>  static Property virtio_net_properties[] = {
>>  >>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>>  >>                      VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
>>  >> +    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
>>  >> +                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
>>  >>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
>>  >>      DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
>>  >>      DEFINE_PROP_END_OF_LIST(),
>>  >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>>  >> index 3bac016..82a6782 100644
>>  >> --- a/hw/virtio/virtio-pci.h
>>  >> +++ b/hw/virtio/virtio-pci.h
>>  >> @@ -62,6 +62,9 @@ typedef struct VirtioBusClass 
>> VirtioPCIBusClass;
>>  >>   * 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)
>>  >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
>>  >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
>>  >> +    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
>>  >>  typedef struct {
>>  >>      MSIMessage msg;
>>  >> diff --git a/include/hw/compat.h b/include/hw/compat.h
>>  >> index 313682a..3186275 100644
>>  >> --- a/include/hw/compat.h
>>  >> +++ b/include/hw/compat.h
>>  >> @@ -1,7 +1,18 @@
>>  >>  #ifndef HW_COMPAT_H
>>  >>  #define HW_COMPAT_H
>>  >> +#define HW_COMPAT_2_3 \
>>  >> +        {\
>>  >> +            .driver   = "virtio-net-pci",\
>>  >> +            .property = "auto_msix_bar_size",\
>>  >> +            .value    = "off",\
>>  >> +        }
>>  >> +
>>  >> +#define HW_COMPAT_2_2 \
>>  >> +        HW_COMPAT_2_3
>>  >> +
>>  >>  #define HW_COMPAT_2_1 \
>>  >> +        HW_COMPAT_2_2, \
>>  >>          {\
>>  >>              .driver   = "intel-hda",\
>>  >>              .property = "old_msi_addr",\
>>  >> --  2.1.0

Re: [Qemu-devel] [PATCH V4 19/19] virtio-pci: introduce auto_msix_bar_size property

[Jason Wang]

On Thu, Mar 19, 2015 at 6:02 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Thu, Mar 19, 2015 at 01:23:56PM +0800, Jason Wang wrote:
>>  
>>  
>>  On Wed, Mar 18, 2015 at 8:57 PM, Michael S. Tsirkin 
>> <mst@redhat.com> wrote:
>>  >On Wed, Mar 18, 2015 at 05:35:09PM +0800, Jason Wang wrote:
>>  >> Currently we don't support more than 128 MSI-X vectors for a pci
>>  >> devices, trying to use vector=129 for a virtio-net-pci device 
>> may get:
>>  >> qemu-system-x86_64: -device 
>> virtio-net-pci,netdev=hn0,vectors=129:
>>  >> unable to init msix vectors to 129
>>  >>   This this because the MSI-X bar size were hard-coded as 4096. 
>> So this
>>  >> patch introduces boolean auto_msix_bar_size property for
>>  >> virito-pci devices. Enable this will let the device calculate 
>> the msix
>>  >> bar size based on the number of MSI-X entries instead of 
>> previous 4096
>>  >> hard-coded limit.
>>  >> This is a must to let virtio-net can up to 256 queues and each 
>> queue
>>  >> were associated with a specific MSI-X entry.
>>  >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>  >> Cc: Richard Henderson <rth@twiddle.net>
>>  >> Cc: Michael S. Tsirkin <mst@redhat.com>
>>  >> Cc: Alexander Graf <agraf@suse.de>
>>  >> Cc: qemu-ppc@nongnu.org
>>  >> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>  >
>>  >I don't understand what this property does.
>>  >What if I *don't* set auto_msix_bar_size?
>>  >vectors<=128 works exactly like it dif previously, vectors=129 
>> fails?
>>  >Does not seem like useful behaviour, to me.
>>  >
>>  >
>>  >> ---
>>  >>  hw/i386/pc_piix.c      |  8 ++++++++
>>  >>  hw/i386/pc_q35.c       |  8 ++++++++
>>  >>  hw/ppc/spapr.c         | 11 ++++++++++-
>>  >>  hw/virtio/virtio-pci.c | 17 +++++++++++++++--
>>  >>  hw/virtio/virtio-pci.h |  3 +++
>>  >>  include/hw/compat.h    | 11 +++++++++++
>>  >>  6 files changed, 55 insertions(+), 3 deletions(-)
>>  >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>  >> index 0796719..8808500 100644
>>  >> --- a/hw/i386/pc_piix.c
>>  >> +++ b/hw/i386/pc_piix.c
>>  >> @@ -552,6 +552,10 @@ static QEMUMachine pc_i440fx_machine_v2_3 = 
>> {
>>  >>      PC_I440FX_2_3_MACHINE_OPTIONS,
>>  >>      .name = "pc-i440fx-2.3",
>>  >>      .init = pc_init_pci_2_3,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_I440FX_2_2_MACHINE_OPTIONS 
>> PC_I440FX_2_3_MACHINE_OPTIONS
>>  >> @@ -560,6 +564,10 @@ static QEMUMachine pc_i440fx_machine_v2_2 = 
>> {
>>  >>      PC_I440FX_2_2_MACHINE_OPTIONS,
>>  >>      .name = "pc-i440fx-2.2",
>>  >>      .init = pc_init_pci_2_2,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_2,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_I440FX_2_1_MACHINE_OPTIONS                           
>> \
>>  >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>  >> index a8a34a4..4a34349 100644
>>  >> --- a/hw/i386/pc_q35.c
>>  >> +++ b/hw/i386/pc_q35.c
>>  >> @@ -448,6 +448,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>>  >>      PC_Q35_2_3_MACHINE_OPTIONS,
>>  >>      .name = "pc-q35-2.3",
>>  >>      .init = pc_q35_init_2_3,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
>>  >> @@ -456,6 +460,10 @@ static QEMUMachine pc_q35_machine_v2_2 = {
>>  >>      PC_Q35_2_2_MACHINE_OPTIONS,
>>  >>      .name = "pc-q35-2.2",
>>  >>      .init = pc_q35_init_2_2,
>>  >> +    .compat_props = (GlobalProperty[]) {
>>  >> +        HW_COMPAT_2_2,
>>  >> +        { /* end of list */ }
>>  >> +    },
>>  >>  };
>>  >>  #define PC_Q35_2_1_MACHINE_OPTIONS                      \
>>  >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>  >> index 5f25dd3..853a5cc 100644
>>  >> --- a/hw/ppc/spapr.c
>>  >> +++ b/hw/ppc/spapr.c
>>  >> @@ -1794,12 +1794,16 @@ static const TypeInfo spapr_machine_info 
>> = {
>>  >>      },
>>  >>  };
>>  >> +#define SPAPR_COMPAT_2_3 \
>>  >> +        HW_COMPAT_2_3
>>  >> +
>>  >>  #define SPAPR_COMPAT_2_2 \
>>  >> +        SPAPR_COMPAT_2_3, \
>>  >>          {\
>>  >>              .driver   = TYPE_SPAPR_PCI_HOST_BRIDGE,\
>>  >>              .property = "mem_win_size",\
>>  >>              .value    = "0x20000000",\
>>  >> -        }
>>  >> +        } \
>>  >>  #define SPAPR_COMPAT_2_1 \
>>  >>          SPAPR_COMPAT_2_2
>>  >> @@ -1883,10 +1887,15 @@ static const TypeInfo 
>> spapr_machine_2_2_info =
>>  >>{
>>  >>     static void spapr_machine_2_3_class_init(ObjectClass *oc, 
>> void
>>  >>*data)
>>  >>  {
>>  >> +    static GlobalProperty compat_props[] = {
>>  >> +        SPAPR_COMPAT_2_3,
>>  >> +        { /* end of list */ }
>>  >> +    };
>>  >>      MachineClass *mc = MACHINE_CLASS(oc);
>>  >>      mc->name = "pseries-2.3";
>>  >>      mc->desc = "pSeries Logical Partition (PAPR compliant) 
>> v2.3";
>>  >> +    mc->compat_props = compat_props;
>>  >>  }
>>  >>  static const TypeInfo spapr_machine_2_3_info = {
>>  >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>  >> index 4a5febb..f4cd405 100644
>>  >> --- a/hw/virtio/virtio-pci.c
>>  >> +++ b/hw/virtio/virtio-pci.c
>>  >> @@ -925,7 +925,7 @@ static void 
>> virtio_pci_device_plugged(DeviceState
>>  >>*d)
>>  >>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>>  >>      VirtioBusState *bus = &proxy->bus;
>>  >>      uint8_t *config;
>>  >> -    uint32_t size;
>>  >> +    uint32_t size, bar_size;
>>  >>      config = proxy->pci_dev.config;
>>  >>      if (proxy->class_code) {
>>  >> @@ -936,8 +936,19 @@ static void 
>> virtio_pci_device_plugged(DeviceState
>>  >>*d)
>>  >>      pci_set_word(config + PCI_SUBSYSTEM_ID,
>>  >>virtio_bus_get_vdev_id(bus));
>>  >>      config[PCI_INTERRUPT_PIN] = 1;
>>  >> +    if (proxy->flags & VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE) {
>>  >> +        bar_size = proxy->nvectors * PCI_MSIX_ENTRY_SIZE * 2;
>>  >> +        if (bar_size & (bar_size - 1)) {
>>  >> +            bar_size = 1 << qemu_fls(bar_size);
>>  >> +        }
>>  >> +    } else {
>>  >> +        /* For migration compatibility */
>>  >> +        bar_size = 4096;
>>  >> +    }
>>  >> +
>>  >>      if (proxy->nvectors &&
>>  >> -        msix_init_exclusive_bar(&proxy->pci_dev, 
>> proxy->nvectors, 1,
>>  >>4096)) {
>>  >> +        msix_init_exclusive_bar(&proxy->pci_dev, 
>> proxy->nvectors, 1,
>>  >> +                                bar_size)) {
>>  >>          error_report("unable to init msix vectors to %" PRIu32,
>>  >>                       proxy->nvectors);
>>  >>          proxy->nvectors = 0;
>>  >
>>  >
>>  >As I expected, msix format stuff spreads out to virtio.
>>  >Consider "vectors * PCI_MSIX_ENTRY_SIZE * 2"
>>  >That's because you use half the BAR for BIR in msix.c
>>  >So any change will have to be done in two places,
>>  >that's bad.
>>  
>>  Yes, will move to msix.c by passing the compat flags to
>>  msix_init_exclusive_bar().
> 
> I really wonder why isn't just specifying # of vectors enough.
> 

As I replied, since vectors=129 won't get crash in 2.3. We need to keep 
the layout for legacy machine type.

> 
>>  >
>>  >
>>  >> @@ -1370,6 +1381,8 @@ static const TypeInfo 
>> virtio_serial_pci_info = {
>>  >>  static Property virtio_net_properties[] = {
>>  >>      DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
>>  >>                      VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
>>  >> +    DEFINE_PROP_BIT("auto_msix_bar_size", VirtIOPCIProxy, flags,
>>  >> +                    VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT, true),
>>  >>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
>>  >>      DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
>>  >>      DEFINE_PROP_END_OF_LIST(),
>>  >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>>  >> index 3bac016..82a6782 100644
>>  >> --- a/hw/virtio/virtio-pci.h
>>  >> +++ b/hw/virtio/virtio-pci.h
>>  >> @@ -62,6 +62,9 @@ typedef struct VirtioBusClass 
>> VirtioPCIBusClass;
>>  >>   * 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)
>>  >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT 2
>>  >> +#define VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE \
>>  >> +    (1 << VIRTIO_PCI_FLAG_AUTO_MSIX_SIZE_BIT)
>>  >>  typedef struct {
>>  >>      MSIMessage msg;
>>  >> diff --git a/include/hw/compat.h b/include/hw/compat.h
>>  >> index 313682a..3186275 100644
>>  >> --- a/include/hw/compat.h
>>  >> +++ b/include/hw/compat.h
>>  >> @@ -1,7 +1,18 @@
>>  >>  #ifndef HW_COMPAT_H
>>  >>  #define HW_COMPAT_H
>>  >> +#define HW_COMPAT_2_3 \
>>  >> +        {\
>>  >> +            .driver   = "virtio-net-pci",\
>>  >> +            .property = "auto_msix_bar_size",\
>>  >> +            .value    = "off",\
>>  >> +        }
>>  >> +
>>  >> +#define HW_COMPAT_2_2 \
>>  >> +        HW_COMPAT_2_3
>>  >> +
>>  >>  #define HW_COMPAT_2_1 \
>>  >> +        HW_COMPAT_2_2, \
>>  >>          {\
>>  >>              .driver   = "intel-hda",\
>>  >>              .property = "old_msi_addr",\
>>  >> --  2.1.0

Re: [Qemu-devel] [PATCH V4 18/19] pci: remove hard-coded bar size in msix_init_exclusive_bar()

[Jason Wang]

On Thu, Mar 19, 2015 at 6:09 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Thu, Mar 19, 2015 at 01:19:23PM +0800, Jason Wang wrote:
>>  
>>  
>>  On Wed, Mar 18, 2015 at 8:52 PM, Michael S. Tsirkin 
>> <mst@redhat.com> wrote:
>>  >On Wed, Mar 18, 2015 at 05:35:08PM +0800, Jason Wang wrote:
>>  >> This patch let msix_init_exclusive_bar() can accept bar_size 
>> parameter
>>  >> other than a hard-coded limit 4096. Then caller can specify a 
>> bar_size
>>  >> depends on msix entries and can use up to 2048 msix entries as 
>> PCI
>>  >> spec allows. To keep migration compatibility, 4096 is used for 
>> all
>>  >> callers and pba were start from half of bar size.
>>  >> Cc: Keith Busch <keith.busch@intel.com>
>>  >> Cc: Kevin Wolf <kwolf@redhat.com>
>>  >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>  >> Cc: Michael S. Tsirkin <mst@redhat.com>
>>  >> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>  >
>>  >Hmm this API looks strange. Caller will then have to have
>>  >msi specific knowledge.
>>  >Can't we keep the size iternal to msix.c?
>>  
>>  Looks like we can, but still need an extra parameter e.g a boolean
>>  compat_size to let msix_init_exclusive_bar() to use 4096 as bar 
>> size.
> 
> Why not just figure the size out from # of vectors?

Yes, for 2.4 we can figure the size out from the number of vectors.

Re: [Qemu-devel] [PATCH V4 06/19] virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue

[Cornelia Huck]

On Wed, 18 Mar 2015 14:08:56 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Mar 18, 2015 at 05:34:56PM +0800, Jason Wang wrote:
> > There's no need to use vector 0 for invalid virtqueue. So this patch
> > changes to use VIRTIO_NO_VECTOR instead.
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > CC: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> I don't know what does this actually do.
> Cornelia?

I actually have the same patch somewhere in my queue. The point here is
that 0 is plain wrong (it's a valid queue), while VIRTIO_NO_VECTOR is
most certainly no valid queue.

> 
> > ---
> >  hw/s390x/virtio-ccw.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index 130535c..c8b87aa 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -281,7 +281,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
> >  
> >      virtio_queue_set_addr(vdev, index, addr);
> >      if (!addr) {
> > -        virtio_queue_set_vector(vdev, index, 0);
> > +        virtio_queue_set_vector(vdev, index, VIRTIO_NO_VECTOR);
> >      } else {
> >          /* Fail if we don't have a big enough queue. */
> >          /* TODO: Add interface to handle vring.num changing */
> 
> Right below this, we have
>     /* tell notify handler in case of config change */
>     vdev->config_vector = VIRTIO_PCI_QUEUE_MAX;
> 
> which also does not seem to make sense.

Basically we have:

- at most 64 virtqueues with their own indicators (always 64 indicator
bits when using classic I/O interrupts, up to 64 indicator bits when
using adapter interrupts)
- another indicator bit for configuration changes (bit 0 of the
secondary indicator bits)

That way, the configuration change indicator is always one bit behind
the last possible queue indicator.

> 
> These changes need some testing though.

My identical patch seemed to work for me.

Re: [Qemu-devel] [PATCH V4 09/19] virito: introduce bus specific queue limit

[Cornelia Huck]

On Wed, 18 Mar 2015 17:34:59 +0800
Jason Wang <jasowang@redhat.com> wrote:

Typo in subject: s/virito/virtio/

> This patch introduces a bus specific queue limitation. It will be
> useful for increasing the limit for one of the bus without disturbing
> other buses.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/char/virtio-serial-bus.c    |  2 +-
>  hw/net/virtio-net.c            |  4 ++--
>  hw/s390x/s390-virtio-bus.c     |  1 +
>  hw/s390x/virtio-ccw.c          |  1 +
>  hw/scsi/virtio-scsi.c          |  4 ++--
>  hw/virtio/virtio-mmio.c        |  1 +
>  hw/virtio/virtio-pci.c         |  1 +
>  hw/virtio/virtio.c             | 40 +++++++++++++++++++++++++---------------
>  include/hw/virtio/virtio-bus.h |  1 +
>  include/hw/virtio/virtio.h     |  1 +
>  10 files changed, 36 insertions(+), 20 deletions(-)

> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 047c963..2b41e32 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -715,6 +715,7 @@ static void virtio_s390_bus_class_init(ObjectClass *klass, void *data)
>      bus_class->max_dev = 1;
>      k->notify = virtio_s390_notify;
>      k->get_features = virtio_s390_get_features;
> +    k->queue_max = VIRTIO_PCI_QUEUE_MAX;

I'm wondering whether we should initialize queue_max to smth like
VIRTIO_QUEUE_MAX_DEFAULT (== 64) in virtio-bus.c instead and have the
individual classes override that value once they want to use a
different value?

But I'm fine with this patch as it stands as well.

>  }

Re: [Qemu-devel] [PATCH V4 10/19] virtio-ccw: introduce ccw specific queue limit

[Cornelia Huck]

On Wed, 18 Mar 2015 17:35:00 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Instead of depending on marco, using a bus specific limit.

You should probably also mention the adapter routes change in your
changelog:

"Also make it clear that the number of gsis per I/O adapter is not
directly depending on the number of virtio queues, but rather the other
way around."

> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c   |  7 +++++--
>  hw/s390x/virtio-ccw.c        | 19 ++++++++++++-------
>  include/hw/s390x/s390_flic.h |  4 +++-
>  3 files changed, 20 insertions(+), 10 deletions(-)
> 

Looks good to me.

Re: [Qemu-devel] [PATCH V4 11/19] virtio-s390: switch to bus specific queue limit

[Cornelia Huck]

On Wed, 18 Mar 2015 17:35:01 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Instead of depending on marco, switch to use a bus specific queue
> limit.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/s390x/s390-virtio-bus.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Looks good to me.

Re: [Qemu-devel] [PATCH V4 14/19] virtio: introduce vector to virtqueues mapping

[Cornelia Huck]

On Wed, 18 Mar 2015 17:35:04 +0800
Jason Wang <jasowang@redhat.com> wrote:

> Currently we will try to traverse all virtqueues to find a subset that
> using a specific vector. This is sub optimal when we will support
> hundreds or even thousands of virtqueues. So this patch introduces a
> method which could be used by transport to get all virtqueues that
> using a same vector. This is done through QLISTs and the number of
> QLISTs was queried through a transport specific method. When guest
> setting vectors, the virtqueue will be linked and helpers for traverse
> the list was also introduced.
> 
> The first user will be virtio pci which will use this to speed up
> MSI-X masking and unmasking handling.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio-pci.c         |  8 ++++++++
>  hw/virtio/virtio.c             | 32 ++++++++++++++++++++++++++++++--
>  include/hw/virtio/virtio-bus.h |  1 +
>  include/hw/virtio/virtio.h     |  3 +++
>  4 files changed, 42 insertions(+), 2 deletions(-)

I'm still not too happy introducing this overhead for all devices when
only pci will make use of it.

Could we perhaps make the queue handling dependant on whether the
transport actually provides the query_nvectors callback?

Re: [Qemu-devel] [PATCH V4 06/19] virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue

[Michael S. Tsirkin]

On Fri, Mar 20, 2015 at 08:39:24AM +0100, Cornelia Huck wrote:
> On Wed, 18 Mar 2015 14:08:56 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Mar 18, 2015 at 05:34:56PM +0800, Jason Wang wrote:
> > > There's no need to use vector 0 for invalid virtqueue. So this patch
> > > changes to use VIRTIO_NO_VECTOR instead.
> > > 
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > CC: Christian Borntraeger <borntraeger@de.ibm.com>
> > > Cc: Richard Henderson <rth@twiddle.net>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > 
> > I don't know what does this actually do.
> > Cornelia?
> 
> I actually have the same patch somewhere in my queue. The point here is
> that 0 is plain wrong (it's a valid queue), while VIRTIO_NO_VECTOR is
> most certainly no valid queue.
> 
> > 
> > > ---
> > >  hw/s390x/virtio-ccw.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > > index 130535c..c8b87aa 100644
> > > --- a/hw/s390x/virtio-ccw.c
> > > +++ b/hw/s390x/virtio-ccw.c
> > > @@ -281,7 +281,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
> > >  
> > >      virtio_queue_set_addr(vdev, index, addr);
> > >      if (!addr) {
> > > -        virtio_queue_set_vector(vdev, index, 0);
> > > +        virtio_queue_set_vector(vdev, index, VIRTIO_NO_VECTOR);
> > >      } else {
> > >          /* Fail if we don't have a big enough queue. */
> > >          /* TODO: Add interface to handle vring.num changing */
> > 
> > Right below this, we have
> >     /* tell notify handler in case of config change */
> >     vdev->config_vector = VIRTIO_PCI_QUEUE_MAX;
> > 
> > which also does not seem to make sense.
> 
> Basically we have:
> 
> - at most 64 virtqueues with their own indicators (always 64 indicator
> bits when using classic I/O interrupts, up to 64 indicator bits when
> using adapter interrupts)
> - another indicator bit for configuration changes (bit 0 of the
> secondary indicator bits)
> 
> That way, the configuration change indicator is always one bit behind
> the last possible queue indicator.

But VIRTIO_PCI_QUEUE_MAX only makes sense as a VQ number.
Why does it make sense as a vector number?
Jason's patches actually change VIRTIO_PCI_QUEUE_MAX
so we need to figure our what to do for this code.


> > 
> > These changes need some testing though.
> 
> My identical patch seemed to work for me.

Re: [Qemu-devel] [PATCH V4 06/19] virtio-ccw: using VIRTIO_NO_VECTOR instead of 0 for invalid virtqueue

[Cornelia Huck]

On Sat, 21 Mar 2015 19:27:49 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Mar 20, 2015 at 08:39:24AM +0100, Cornelia Huck wrote:
> > On Wed, 18 Mar 2015 14:08:56 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Mar 18, 2015 at 05:34:56PM +0800, Jason Wang wrote:
> > > > There's no need to use vector 0 for invalid virtqueue. So this patch
> > > > changes to use VIRTIO_NO_VECTOR instead.
> > > > 
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > > CC: Christian Borntraeger <borntraeger@de.ibm.com>
> > > > Cc: Richard Henderson <rth@twiddle.net>
> > > > Cc: Alexander Graf <agraf@suse.de>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > 
> > > I don't know what does this actually do.
> > > Cornelia?
> > 
> > I actually have the same patch somewhere in my queue. The point here is
> > that 0 is plain wrong (it's a valid queue), while VIRTIO_NO_VECTOR is
> > most certainly no valid queue.
> > 
> > > 
> > > > ---
> > > >  hw/s390x/virtio-ccw.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > > > index 130535c..c8b87aa 100644
> > > > --- a/hw/s390x/virtio-ccw.c
> > > > +++ b/hw/s390x/virtio-ccw.c
> > > > @@ -281,7 +281,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
> > > >  
> > > >      virtio_queue_set_addr(vdev, index, addr);
> > > >      if (!addr) {
> > > > -        virtio_queue_set_vector(vdev, index, 0);
> > > > +        virtio_queue_set_vector(vdev, index, VIRTIO_NO_VECTOR);
> > > >      } else {
> > > >          /* Fail if we don't have a big enough queue. */
> > > >          /* TODO: Add interface to handle vring.num changing */
> > > 
> > > Right below this, we have
> > >     /* tell notify handler in case of config change */
> > >     vdev->config_vector = VIRTIO_PCI_QUEUE_MAX;
> > > 
> > > which also does not seem to make sense.
> > 
> > Basically we have:
> > 
> > - at most 64 virtqueues with their own indicators (always 64 indicator
> > bits when using classic I/O interrupts, up to 64 indicator bits when
> > using adapter interrupts)
> > - another indicator bit for configuration changes (bit 0 of the
> > secondary indicator bits)
> > 
> > That way, the configuration change indicator is always one bit behind
> > the last possible queue indicator.
> 
> But VIRTIO_PCI_QUEUE_MAX only makes sense as a VQ number.
> Why does it make sense as a vector number?
> Jason's patches actually change VIRTIO_PCI_QUEUE_MAX
> so we need to figure our what to do for this code.

We don't have "vectors" for virtio-ccw. It's just a concept mandated by
common code, we just do an identity mapping. Using this value just
pushes the notifier bits for config changes to behind the highest
possible virtqueue.

Re: [Qemu-devel] [PATCH V4 09/19] virito: introduce bus specific queue limit

[Jason Wang]

On Fri, Mar 20, 2015 at 6:20 PM, Cornelia Huck 
<cornelia.huck@de.ibm.com> wrote:
> On Wed, 18 Mar 2015 17:34:59 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> Typo in subject: s/virito/virtio/

Will correct this.
> 
> 
>>  This patch introduces a bus specific queue limitation. It will be
>>  useful for increasing the limit for one of the bus without 
>> disturbing
>>  other buses.
>>  
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Cc: Alexander Graf <agraf@suse.de>
>>  Cc: Richard Henderson <rth@twiddle.net>
>>  Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>  Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>>  Cc: Paolo Bonzini <pbonzini@redhat.com>
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
>>  ---
>>   hw/char/virtio-serial-bus.c    |  2 +-
>>   hw/net/virtio-net.c            |  4 ++--
>>   hw/s390x/s390-virtio-bus.c     |  1 +
>>   hw/s390x/virtio-ccw.c          |  1 +
>>   hw/scsi/virtio-scsi.c          |  4 ++--
>>   hw/virtio/virtio-mmio.c        |  1 +
>>   hw/virtio/virtio-pci.c         |  1 +
>>   hw/virtio/virtio.c             | 40 
>> +++++++++++++++++++++++++---------------
>>   include/hw/virtio/virtio-bus.h |  1 +
>>   include/hw/virtio/virtio.h     |  1 +
>>   10 files changed, 36 insertions(+), 20 deletions(-)
> 
>>  diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>>  index 047c963..2b41e32 100644
>>  --- a/hw/s390x/s390-virtio-bus.c
>>  +++ b/hw/s390x/s390-virtio-bus.c
>>  @@ -715,6 +715,7 @@ static void 
>> virtio_s390_bus_class_init(ObjectClass *klass, void *data)
>>       bus_class->max_dev = 1;
>>       k->notify = virtio_s390_notify;
>>       k->get_features = virtio_s390_get_features;
>>  +    k->queue_max = VIRTIO_PCI_QUEUE_MAX;
> 
> I'm wondering whether we should initialize queue_max to smth like
> VIRTIO_QUEUE_MAX_DEFAULT (== 64) in virtio-bus.c instead and have the
> individual classes override that value once they want to use a
> different value?

May work now, but consider in the future, each transport will finally 
have their own limitation, so VIRTIO_QUEUE_MAX_DEFAULT will finally be 
removed some day.
> 
> 
> But I'm fine with this patch as it stands as well.

Thanks, so I will keep this patch as is.
> 
>>   }
> 

Re: [Qemu-devel] [PATCH V4 10/19] virtio-ccw: introduce ccw specific queue limit

[Jason Wang]

On Fri, Mar 20, 2015 at 7:33 PM, Cornelia Huck 
<cornelia.huck@de.ibm.com> wrote:
> On Wed, 18 Mar 2015 17:35:00 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
>>  Instead of depending on marco, using a bus specific limit.
> 
> You should probably also mention the adapter routes change in your
> changelog:
> 
> "Also make it clear that the number of gsis per I/O adapter is not
> directly depending on the number of virtio queues, but rather the 
> other
> way around."

Will add this in next version.
> 
>>  
>>  Cc: Alexander Graf <agraf@suse.de>
>>  Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>  Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>>  Cc: Richard Henderson <rth@twiddle.net>
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
>>  ---
>>   hw/s390x/s390-virtio-ccw.c   |  7 +++++--
>>   hw/s390x/virtio-ccw.c        | 19 ++++++++++++-------
>>   include/hw/s390x/s390_flic.h |  4 +++-
>>   3 files changed, 20 insertions(+), 10 deletions(-)
>>  
> 
> Looks good to me.

Thanks

Re: [Qemu-devel] [PATCH V4 14/19] virtio: introduce vector to virtqueues mapping

[Jason Wang]

On Fri, Mar 20, 2015 at 7:39 PM, Cornelia Huck 
<cornelia.huck@de.ibm.com> wrote:
> On Wed, 18 Mar 2015 17:35:04 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
>>  Currently we will try to traverse all virtqueues to find a subset 
>> that
>>  using a specific vector. This is sub optimal when we will support
>>  hundreds or even thousands of virtqueues. So this patch introduces a
>>  method which could be used by transport to get all virtqueues that
>>  using a same vector. This is done through QLISTs and the number of
>>  QLISTs was queried through a transport specific method. When guest
>>  setting vectors, the virtqueue will be linked and helpers for 
>> traverse
>>  the list was also introduced.
>>  
>>  The first user will be virtio pci which will use this to speed up
>>  MSI-X masking and unmasking handling.
>>  
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
>>  ---
>>   hw/virtio/virtio-pci.c         |  8 ++++++++
>>   hw/virtio/virtio.c             | 32 
>> ++++++++++++++++++++++++++++++--
>>   include/hw/virtio/virtio-bus.h |  1 +
>>   include/hw/virtio/virtio.h     |  3 +++
>>   4 files changed, 42 insertions(+), 2 deletions(-)
> 
> I'm still not too happy introducing this overhead for all devices when
> only pci will make use of it.
> 
> Could we perhaps make the queue handling dependant on whether the
> transport actually provides the query_nvectors callback?

Though I don't think it will introduce noticeable overhead. But looks 
like we can do as you suggest. Will do it in next version.

Thanks