[PATCH v1 0/4] virtio: fix IO request length in virtio SCSI/block

[PATCH v1 0/4] virtio: fix IO request length in virtio SCSI/block

[Denis Plotnikov]

v1:
  * make seg_max size dependent on virtuqueue size
  * don't expose seg_max as property
  * add new machine types with increased queue size
  * add test to check the new machine types
  * check queue size for non-modern virtio devices
---
From: "Denis V. Lunev" <den@openvz.org>

Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
field reported by SCSI controler. Thus typical sequential read with
1 MB size results in the following pattern of the IO from the guest:
  8,16   1    15754     2.766095122  2071  D   R 2095104 + 1008 [dd]
  8,16   1    15755     2.766108785  2071  D   R 2096112 + 1008 [dd]
  8,16   1    15756     2.766113486  2071  D   R 2097120 + 32 [dd]
  8,16   1    15757     2.767668961     0  C   R 2095104 + 1008 [0]
  8,16   1    15758     2.768534315     0  C   R 2096112 + 1008 [0]
  8,16   1    15759     2.768539782     0  C   R 2097120 + 32 [0]
The IO was generated by
  dd if=/dev/sda of=/dev/null bs=1024 iflag=direct

This effectively means that on rotational disks we will observe 3 IOPS
for each 2 MBs processed. This definitely negatively affects both
guest and host IO performance.

The cure is relatively simple - we should report lengthy scatter-gather
ability of the SCSI controller. Fortunately the situation here is very
good. VirtIO transport layer can accomodate 1024 items in one request
while we are using only 128. This situation is present since almost
very beginning. 2 items are dedicated for request metadata thus we
should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.

The following pattern is observed after the patch:
  8,16   1     9921     2.662721340  2063  D   R 2095104 + 1024 [dd]
  8,16   1     9922     2.662737585  2063  D   R 2096128 + 1024 [dd]
  8,16   1     9923     2.665188167     0  C   R 2095104 + 1024 [0]
  8,16   1     9924     2.665198777     0  C   R 2096128 + 1024 [0]
which is much better.

The dark side of this patch is that we are tweaking guest visible
parameter, though this should be relatively safe as above transport
layer support is present in QEMU/host Linux for a very long time.
The patch adds configurable property for VirtIO SCSI with a new default
and hardcode option for VirtBlock which does not provide good
configurable framework.

Unfortunately the commit can not be applied as is. For the real cure we
need guest to be fixed to accomodate that queue length, which is done
only in the latest 4.14 kernel. Thus we are going to expose the property
and tweak it on machine type level.

The problem with the old kernels is that they have
max_segments <= virtqueue_size restriction which cause the guest
crashing in the case of violation.
To fix the case described above in the old kernels we can increase
virtqueue_size to 256 and max_segments to 254. The pitfall here is
that seabios allows the virtqueue_size-s < 128, however, the seabios
patch extending that value to 256 is pending.

Denis Plotnikov (4):
  virtio: protect non-modern devices from too big virtqueue size setting
  virtio: make seg_max virtqueue size dependent
  virtio: increase virtuqueue sizes in new machine types
  iotests: add test for virtio-scsi and virtio-blk machine type settings

 hw/block/virtio-blk.c       |   2 +-
 hw/core/machine.c           |  14 ++++
 hw/i386/pc_piix.c           |  16 +++-
 hw/i386/pc_q35.c            |  14 +++-
 hw/scsi/virtio-scsi.c       |   2 +-
 hw/virtio/virtio-blk-pci.c  |   9 +++
 hw/virtio/virtio-scsi-pci.c |  10 +++
 include/hw/boards.h         |   6 ++
 tests/qemu-iotests/267      | 154 ++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/267.out  |   1 +
 tests/qemu-iotests/group    |   1 +
 11 files changed, 222 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/267
 create mode 100644 tests/qemu-iotests/267.out

-- 
2.17.0

[PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Denis Plotnikov]

The patch protects from creating illegal virtio device configuration
via direct virtqueue size property setting.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 hw/virtio/virtio-blk-pci.c  |  9 +++++++++
 hw/virtio/virtio-scsi-pci.c | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
index 60c9185c39..6177ff1df8 100644
--- a/hw/virtio/virtio-blk-pci.c
+++ b/hw/virtio/virtio-blk-pci.c
@@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 {
     VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
+    bool modern = virtio_pci_modern(vpci_dev);
+    uint32_t queue_size = dev->vdev.conf.queue_size;
+
+    if (!modern && queue_size > 128) {
+        error_setg(errp,
+                   "too big queue size (%u, max: 128) "
+                   "for non-modern virtio device", queue_size);
+        return;
+    }
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
         vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
index 2830849729..6e6790fda5 100644
--- a/hw/virtio/virtio-scsi-pci.c
+++ b/hw/virtio/virtio-scsi-pci.c
@@ -17,6 +17,7 @@
 
 #include "hw/virtio/virtio-scsi.h"
 #include "virtio-pci.h"
+#include "qapi/error.h"
 
 typedef struct VirtIOSCSIPCI VirtIOSCSIPCI;
 
@@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
     DeviceState *proxy = DEVICE(vpci_dev);
     char *bus_name;
+    bool modern = virtio_pci_modern(vpci_dev);
+    uint32_t virtqueue_size = vs->conf.virtqueue_size;
+
+    if (!modern && virtqueue_size > 128) {
+        error_setg(errp,
+                   "too big virtqueue size (%u, max: 128) "
+                   "for non-modern virtio device", virtqueue_size);
+        return;
+    }
 
     if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
         vpci_dev->nvectors = vs->conf.num_queues + 3;
-- 
2.17.0

[PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Denis Plotnikov]

seg_max has a restriction to be less or equal to virtqueue size
according to Virtio 1.0 specification

Although seg_max can't be set directly, it's worth to express this
dependancy directly in the code for sanity purpose.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 hw/block/virtio-blk.c | 2 +-
 hw/scsi/virtio-scsi.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 06e57a4d39..21530304cf 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     blk_get_geometry(s->blk, &capacity);
     memset(&blkcfg, 0, sizeof(blkcfg));
     virtio_stq_p(vdev, &blkcfg.capacity, capacity);
-    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
+    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
     virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
     virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
     virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 839f120256..f7e5533cd5 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
     VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
     virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
-    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
+    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.virtqueue_size - 2);
     virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
     virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
     virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
-- 
2.17.0

[PATCH v1 3/4] virtio: increase virtuqueue sizes in new machine types

[Denis Plotnikov]

Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
field reported by SCSI controler. Thus typical sequential read with
1 MB size results in the following pattern of the IO from the guest:
  8,16   1    15754     2.766095122  2071  D   R 2095104 + 1008 [dd]
  8,16   1    15755     2.766108785  2071  D   R 2096112 + 1008 [dd]
  8,16   1    15756     2.766113486  2071  D   R 2097120 + 32 [dd]
  8,16   1    15757     2.767668961     0  C   R 2095104 + 1008 [0]
  8,16   1    15758     2.768534315     0  C   R 2096112 + 1008 [0]
  8,16   1    15759     2.768539782     0  C   R 2097120 + 32 [0]
The IO was generated by
  dd if=/dev/sda of=/dev/null bs=1024 iflag=direct

This effectively means that on rotational disks we will observe 3 IOPS
for each 2 MBs processed. This definitely negatively affects both
guest and host IO performance.

The cure is relatively simple - we should report lengthy scatter-gather
ability of the SCSI controller. Fortunately the situation here is very
good. VirtIO transport layer can accomodate 1024 items in one request
while we are using only 128. This situation is present since almost
very beginning. 2 items are dedicated for request metadata thus we
should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.

The following pattern is observed after the patch:
  8,16   1     9921     2.662721340  2063  D   R 2095104 + 1024 [dd]
  8,16   1     9922     2.662737585  2063  D   R 2096128 + 1024 [dd]
  8,16   1     9923     2.665188167     0  C   R 2095104 + 1024 [0]
  8,16   1     9924     2.665198777     0  C   R 2096128 + 1024 [0]
which is much better.

To fix this particular case, the patch adds new machine types with
extended virtqueue sizes to 256 which also increases max_seg to 254
implicitly.

Suggested-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 hw/core/machine.c   | 14 ++++++++++++++
 hw/i386/pc_piix.c   | 16 +++++++++++++---
 hw/i386/pc_q35.c    | 14 ++++++++++++--
 include/hw/boards.h |  6 ++++++
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 55b08f1466..28013a0e3f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -24,6 +24,13 @@
 #include "hw/pci/pci.h"
 #include "hw/mem/nvdimm.h"
 
+GlobalProperty hw_compat_4_0_1[] = {
+    { "virtio-blk-device", "queue-size", "128" },
+    { "virtio-scsi-device", "virtqueue_size", "128" },
+    { "vhost-scsi-device", "virtqueue_size", "128" },
+};
+const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
+
 GlobalProperty hw_compat_4_0[] = {
     { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
 };
@@ -157,6 +164,13 @@ GlobalProperty hw_compat_2_1[] = {
 };
 const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
 
+GlobalProperty hw_compat[] = {
+    { "virtio-blk-device", "queue-size", "256" },
+    { "virtio-scsi-device", "virtqueue_size", "256" },
+    { "vhost-scsi-device", "virtqueue_size", "256" },
+};
+const size_t hw_compat_len = G_N_ELEMENTS(hw_compat);
+
 static char *machine_get_accel(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8ad8e885c6..2260a61b1b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -426,15 +426,27 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
+    compat_props_add(m->compat_props, hw_compat, hw_compat_len);
 }
 
-static void pc_i440fx_4_0_machine_options(MachineClass *m)
+static void pc_i440fx_4_0_2_machine_options(MachineClass *m)
 {
     pc_i440fx_machine_options(m);
     m->alias = "pc";
     m->is_default = 1;
 }
 
+DEFINE_I440FX_MACHINE(v4_0_2, "pc-i440fx-4.0.2", NULL,
+                      pc_i440fx_4_0_2_machine_options);
+
+static void pc_i440fx_4_0_machine_options(MachineClass *m)
+{
+    pc_i440fx_4_0_2_machine_options(m);
+    m->alias = NULL;
+    m->is_default = 0;
+    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
+}
+
 DEFINE_I440FX_MACHINE(v4_0, "pc-i440fx-4.0", NULL,
                       pc_i440fx_4_0_machine_options);
 
@@ -443,9 +455,7 @@ static void pc_i440fx_3_1_machine_options(MachineClass *m)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
 
     pc_i440fx_4_0_machine_options(m);
-    m->is_default = 0;
     m->smbus_no_migration_support = true;
-    m->alias = NULL;
     pcmc->pvh_enabled = false;
     compat_props_add(m->compat_props, hw_compat_3_1, hw_compat_3_1_len);
     compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 45cc29d1ad..50ccd9ebcf 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -363,14 +363,25 @@ static void pc_q35_machine_options(MachineClass *m)
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
     m->max_cpus = 288;
+    compat_props_add(m->compat_props, hw_compat, hw_compat_len);
 }
 
-static void pc_q35_4_0_1_machine_options(MachineClass *m)
+static void pc_q35_4_0_2_machine_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
     m->alias = "q35";
 }
 
+DEFINE_Q35_MACHINE(v4_0_2, "pc-q35-4.0.2", NULL,
+                   pc_q35_4_0_2_machine_options);
+
+static void pc_q35_4_0_1_machine_options(MachineClass *m)
+{
+    pc_q35_4_0_2_machine_options(m);
+    m->alias = NULL;
+    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
+}
+
 DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
                    pc_q35_4_0_1_machine_options);
 
@@ -378,7 +389,6 @@ static void pc_q35_4_0_machine_options(MachineClass *m)
 {
     pc_q35_4_0_1_machine_options(m);
     m->default_kernel_irqchip_split = true;
-    m->alias = NULL;
     compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
     compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
 }
diff --git a/include/hw/boards.h b/include/hw/boards.h
index fe1885cbff..cf10632dac 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -293,6 +293,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_4_0_1[];
+extern const size_t hw_compat_4_0_1_len;
+
 extern GlobalProperty hw_compat_4_0[];
 extern const size_t hw_compat_4_0_len;
 
@@ -338,4 +341,7 @@ extern const size_t hw_compat_2_2_len;
 extern GlobalProperty hw_compat_2_1[];
 extern const size_t hw_compat_2_1_len;
 
+extern GlobalProperty hw_compat[];
+extern const size_t hw_compat_len;
+
 #endif
-- 
2.17.0

[PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Denis Plotnikov]

It tests proper queue size settings for all available machine types.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 tests/qemu-iotests/267     | 154 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/267.out |   1 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 156 insertions(+)
 create mode 100755 tests/qemu-iotests/267
 create mode 100644 tests/qemu-iotests/267.out

diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
new file mode 100755
index 0000000000..6d3cc574b3
--- /dev/null
+++ b/tests/qemu-iotests/267
@@ -0,0 +1,154 @@
+#!/usr/bin/env python
+#
+# Test virtio-scsi and virtio-blk queue settings for all machine types
+#
+# Copyright (c) 2019 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import sys
+import os
+import re
+import iotests
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
+import qemu
+
+# list of device types and virtqueue properties to test
+# this is a generalized approach
+# for now we just check queue length
+# more properties can be added in each list
+virtio_scsi_props = {'vq_size': 'virtqueue_size'}
+virtio_blk_props = {'vq_size': 'queue-size'}
+
+dev_types = {'virtio-scsi-pci': virtio_scsi_props,
+             'virtio-blk-pci': virtio_blk_props}
+
+vm_dev_params = {'virtio-scsi-pci': ['-device', 'virtio-scsi-pci,id=scsi0'],
+                 'virtio-blk-pci': ['-device',
+                                    'virtio-blk-pci,id=scsi0,drive=drive0',
+                                    '-drive',
+                                    'driver=null-co,id=drive0,if=none']}
+
+def make_pattern(props):
+     pattern_items = ['{0} = \d+'.format(prop) for prop in props]
+     return '|'.join(pattern_items)
+
+
+def query_virtqueue_props(vm, dev_type_name):
+    output = vm.qmp('human-monitor-command', command_line='info qtree')
+    output = output['return']
+
+    props_list = dev_types[dev_type_name].values();
+
+    pattern = make_pattern(props_list)
+
+    res = re.findall(pattern, output)
+
+    if len(res) != len(props_list):
+        not_found = props_list.difference(set(res))
+
+        ret = (0, '({0}): The following properties not found: {1}'
+                  .format(dev_type_name, ', '.join(not_found)))
+    else:
+        props = dict()
+        for prop in res:
+            p = prop.split(' = ')
+            props[p[0]] = int(p[1])
+        ret = (1, props)
+
+    return ret
+
+
+def check_mt(mt, dev_type_name):
+    vm_params = ['-machine', mt['name']] + vm_dev_params[dev_type_name]
+
+    vm = qemu.QEMUMachine(iotests.qemu_prog, vm_params)
+    vm.launch()
+    ret = query_virtqueue_props(vm, dev_type_name)
+    vm.shutdown()
+
+    if ret[0] == 0:
+        print('Error ({0}): {1}'.format(mt['name'], ret[1]))
+        return 1
+
+    errors = 0
+    props = ret[1]
+
+    for prop_name, prop_val in props.items():
+        if mt[prop_name] != prop_val:
+            print('Error [{0}, {1}]: {2}={3} (expected {4})'.
+                  format(mt['name'], dev_type_name, prop_name, prop_val,
+                         mt[prop_name]))
+            errors += 1
+
+    return errors
+
+def is_256_virtqueue_size_mt(mt):
+    mt = mt.split("-")
+
+    # machine types like pc-x.x
+    if len(mt) == 2:
+        return False
+
+    # machine types like pc-<chip_name>-x.x[.x]
+    ver = mt[2]
+
+    ver = ver.split(".");
+
+    # all versions greater than 4.0.1 goes with 256 queue size
+    if int(ver[0]) >= 4:
+        major = int(ver[1])
+        minor = 0
+        if len(ver) > 2:
+            minor = int(ver[2])
+
+        if major > 0 or minor > 1:
+             return True
+
+    return False
+
+
+# collect all machine types except 'none'
+vm = iotests.VM()
+vm.launch()
+machines = [m['name'] for m in vm.qmp('query-machines')['return']]
+vm.shutdown()
+machines.remove('none')
+machines.remove('isapc')
+
+failed = 0
+
+for dev_type in dev_types:
+    # create a list of machine types and their parameters
+    # machine types vz8.X.X must have virtqueue_length=256
+    # others must have virtqueue_length=128
+    mtypes = list()
+    for m in machines:
+        if is_256_virtqueue_size_mt(m):
+            vq_size = 256
+        else:
+            vq_size = 128
+
+        mtypes.append({'name': m, dev_types[dev_type]['vq_size']: vq_size})
+
+    # test each machine type
+    for mt in mtypes:
+        failed += check_mt(mt, dev_type)
+
+if failed > 0:
+    print('Failed')
+else:
+    print('Success')
diff --git a/tests/qemu-iotests/267.out b/tests/qemu-iotests/267.out
new file mode 100644
index 0000000000..35821117c8
--- /dev/null
+++ b/tests/qemu-iotests/267.out
@@ -0,0 +1 @@
+Success
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3605796bb2..ab8523ad60 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -252,3 +252,4 @@
 255 rw auto quick
 256 rw auto quick
 266 rw quick
+267 auto quick
-- 
2.17.0

Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Michael S. Tsirkin]

On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
> seg_max has a restriction to be less or equal to virtqueue size
> according to Virtio 1.0 specification
> 
> Although seg_max can't be set directly, it's worth to express this
> dependancy directly in the code for sanity purpose.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>

This is guest visible so needs to be machine type dependent, right?

> ---
>  hw/block/virtio-blk.c | 2 +-
>  hw/scsi/virtio-scsi.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 06e57a4d39..21530304cf 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      blk_get_geometry(s->blk, &capacity);
>      memset(&blkcfg, 0, sizeof(blkcfg));
>      virtio_stq_p(vdev, &blkcfg.capacity, capacity);
> -    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
> +    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
>      virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
>      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
>      virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 839f120256..f7e5533cd5 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>  
>      virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
> -    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> +    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.virtqueue_size - 2);
>      virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
>      virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
>      virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> -- 
> 2.17.0

Re: [PATCH v1 3/4] virtio: increase virtuqueue sizes in new machine types

[Michael S. Tsirkin]

On Tue, Nov 05, 2019 at 07:11:04PM +0300, Denis Plotnikov wrote:
> Linux guests submit IO requests no longer than PAGE_SIZE * max_seg
> field reported by SCSI controler. Thus typical sequential read with
> 1 MB size results in the following pattern of the IO from the guest:
>   8,16   1    15754     2.766095122  2071  D   R 2095104 + 1008 [dd]
>   8,16   1    15755     2.766108785  2071  D   R 2096112 + 1008 [dd]
>   8,16   1    15756     2.766113486  2071  D   R 2097120 + 32 [dd]
>   8,16   1    15757     2.767668961     0  C   R 2095104 + 1008 [0]
>   8,16   1    15758     2.768534315     0  C   R 2096112 + 1008 [0]
>   8,16   1    15759     2.768539782     0  C   R 2097120 + 32 [0]
> The IO was generated by
>   dd if=/dev/sda of=/dev/null bs=1024 iflag=direct
> 
> This effectively means that on rotational disks we will observe 3 IOPS
> for each 2 MBs processed. This definitely negatively affects both
> guest and host IO performance.
> 
> The cure is relatively simple - we should report lengthy scatter-gather
> ability of the SCSI controller. Fortunately the situation here is very
> good. VirtIO transport layer can accomodate 1024 items in one request
> while we are using only 128. This situation is present since almost
> very beginning. 2 items are dedicated for request metadata thus we
> should publish VIRTQUEUE_MAX_SIZE - 2 as max_seg.
> 
> The following pattern is observed after the patch:
>   8,16   1     9921     2.662721340  2063  D   R 2095104 + 1024 [dd]
>   8,16   1     9922     2.662737585  2063  D   R 2096128 + 1024 [dd]
>   8,16   1     9923     2.665188167     0  C   R 2095104 + 1024 [0]
>   8,16   1     9924     2.665198777     0  C   R 2096128 + 1024 [0]
> which is much better.
> 
> To fix this particular case, the patch adds new machine types with
> extended virtqueue sizes to 256 which also increases max_seg to 254
> implicitly.
> 
> Suggested-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---



the way we normally do this is change the defaults to 256.
what is wrong with doing that?


>  hw/core/machine.c   | 14 ++++++++++++++
>  hw/i386/pc_piix.c   | 16 +++++++++++++---
>  hw/i386/pc_q35.c    | 14 ++++++++++++--
>  include/hw/boards.h |  6 ++++++
>  4 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 55b08f1466..28013a0e3f 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -24,6 +24,13 @@
>  #include "hw/pci/pci.h"
>  #include "hw/mem/nvdimm.h"
>  
> +GlobalProperty hw_compat_4_0_1[] = {
> +    { "virtio-blk-device", "queue-size", "128" },
> +    { "virtio-scsi-device", "virtqueue_size", "128" },
> +    { "vhost-scsi-device", "virtqueue_size", "128" },
> +};
> +const size_t hw_compat_4_0_1_len = G_N_ELEMENTS(hw_compat_4_0_1);
> +
>  GlobalProperty hw_compat_4_0[] = {
>      { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
>  };
> @@ -157,6 +164,13 @@ GlobalProperty hw_compat_2_1[] = {
>  };
>  const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
>  
> +GlobalProperty hw_compat[] = {
> +    { "virtio-blk-device", "queue-size", "256" },
> +    { "virtio-scsi-device", "virtqueue_size", "256" },
> +    { "vhost-scsi-device", "virtqueue_size", "256" },
> +};
> +const size_t hw_compat_len = G_N_ELEMENTS(hw_compat);
> +
>  static char *machine_get_accel(Object *obj, Error **errp)
>  {
>      MachineState *ms = MACHINE(obj);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 8ad8e885c6..2260a61b1b 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -426,15 +426,27 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->default_machine_opts = "firmware=bios-256k.bin";
>      m->default_display = "std";
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
> +    compat_props_add(m->compat_props, hw_compat, hw_compat_len);
>  }
>  
> -static void pc_i440fx_4_0_machine_options(MachineClass *m)
> +static void pc_i440fx_4_0_2_machine_options(MachineClass *m)
>  {
>      pc_i440fx_machine_options(m);
>      m->alias = "pc";
>      m->is_default = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v4_0_2, "pc-i440fx-4.0.2", NULL,
> +                      pc_i440fx_4_0_2_machine_options);
> +
> +static void pc_i440fx_4_0_machine_options(MachineClass *m)
> +{
> +    pc_i440fx_4_0_2_machine_options(m);
> +    m->alias = NULL;
> +    m->is_default = 0;
> +    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
> +}
> +
>  DEFINE_I440FX_MACHINE(v4_0, "pc-i440fx-4.0", NULL,
>                        pc_i440fx_4_0_machine_options);
>  
> @@ -443,9 +455,7 @@ static void pc_i440fx_3_1_machine_options(MachineClass *m)
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>  
>      pc_i440fx_4_0_machine_options(m);
> -    m->is_default = 0;
>      m->smbus_no_migration_support = true;
> -    m->alias = NULL;
>      pcmc->pvh_enabled = false;
>      compat_props_add(m->compat_props, hw_compat_3_1, hw_compat_3_1_len);
>      compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 45cc29d1ad..50ccd9ebcf 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -363,14 +363,25 @@ static void pc_q35_machine_options(MachineClass *m)
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
>      m->max_cpus = 288;
> +    compat_props_add(m->compat_props, hw_compat, hw_compat_len);
>  }
>  
> -static void pc_q35_4_0_1_machine_options(MachineClass *m)
> +static void pc_q35_4_0_2_machine_options(MachineClass *m)
>  {
>      pc_q35_machine_options(m);
>      m->alias = "q35";
>  }
>  
> +DEFINE_Q35_MACHINE(v4_0_2, "pc-q35-4.0.2", NULL,
> +                   pc_q35_4_0_2_machine_options);
> +
> +static void pc_q35_4_0_1_machine_options(MachineClass *m)
> +{
> +    pc_q35_4_0_2_machine_options(m);
> +    m->alias = NULL;
> +    compat_props_add(m->compat_props, hw_compat_4_0_1, hw_compat_4_0_1_len);
> +}
> +
>  DEFINE_Q35_MACHINE(v4_0_1, "pc-q35-4.0.1", NULL,
>                     pc_q35_4_0_1_machine_options);
>  
> @@ -378,7 +389,6 @@ static void pc_q35_4_0_machine_options(MachineClass *m)
>  {
>      pc_q35_4_0_1_machine_options(m);
>      m->default_kernel_irqchip_split = true;
> -    m->alias = NULL;
>      compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
>      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
>  }
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index fe1885cbff..cf10632dac 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -293,6 +293,9 @@ struct MachineState {
>      } \
>      type_init(machine_initfn##_register_types)
>  
> +extern GlobalProperty hw_compat_4_0_1[];
> +extern const size_t hw_compat_4_0_1_len;
> +
>  extern GlobalProperty hw_compat_4_0[];
>  extern const size_t hw_compat_4_0_len;
>  
> @@ -338,4 +341,7 @@ extern const size_t hw_compat_2_2_len;
>  extern GlobalProperty hw_compat_2_1[];
>  extern const size_t hw_compat_2_1_len;
>  
> +extern GlobalProperty hw_compat[];
> +extern const size_t hw_compat_len;
> +
>  #endif
> -- 
> 2.17.0

Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Michael S. Tsirkin]

On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
> The patch protects from creating illegal virtio device configuration
> via direct virtqueue size property setting.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  hw/virtio/virtio-blk-pci.c  |  9 +++++++++
>  hw/virtio/virtio-scsi-pci.c | 10 ++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
> index 60c9185c39..6177ff1df8 100644
> --- a/hw/virtio/virtio-blk-pci.c
> +++ b/hw/virtio/virtio-blk-pci.c
> @@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>  {
>      VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
>      DeviceState *vdev = DEVICE(&dev->vdev);
> +    bool modern = virtio_pci_modern(vpci_dev);
> +    uint32_t queue_size = dev->vdev.conf.queue_size;
> +
> +    if (!modern && queue_size > 128) {
> +        error_setg(errp,
> +                   "too big queue size (%u, max: 128) "
> +                   "for non-modern virtio device", queue_size);
> +        return;
> +    }


this enables for transitional so still visible to legacy
interface. I am guessing you want to check whether
device is accessed through the modern interface instead.

>      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>          vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;

> diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> index 2830849729..6e6790fda5 100644
> --- a/hw/virtio/virtio-scsi-pci.c
> +++ b/hw/virtio/virtio-scsi-pci.c
> @@ -17,6 +17,7 @@
>  
>  #include "hw/virtio/virtio-scsi.h"
>  #include "virtio-pci.h"
> +#include "qapi/error.h"
>  
>  typedef struct VirtIOSCSIPCI VirtIOSCSIPCI;
>  
> @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>      DeviceState *proxy = DEVICE(vpci_dev);
>      char *bus_name;
> +    bool modern = virtio_pci_modern(vpci_dev);
> +    uint32_t virtqueue_size = vs->conf.virtqueue_size;
> +
> +    if (!modern && virtqueue_size > 128) {
> +        error_setg(errp,
> +                   "too big virtqueue size (%u, max: 128) "
> +                   "for non-modern virtio device", virtqueue_size);
> +        return;
> +    }

why? what is illegal about 256 for legacy?

>  
>      if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>          vpci_dev->nvectors = vs->conf.num_queues + 3;
> -- 
> 2.17.0

Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Denis Plotnikov]

On 05.11.2019 23:56, Michael S. Tsirkin wrote:
> On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
>> The patch protects from creating illegal virtio device configuration
>> via direct virtqueue size property setting.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>>   hw/virtio/virtio-blk-pci.c  |  9 +++++++++
>>   hw/virtio/virtio-scsi-pci.c | 10 ++++++++++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
>> index 60c9185c39..6177ff1df8 100644
>> --- a/hw/virtio/virtio-blk-pci.c
>> +++ b/hw/virtio/virtio-blk-pci.c
>> @@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>>   {
>>       VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
>>       DeviceState *vdev = DEVICE(&dev->vdev);
>> +    bool modern = virtio_pci_modern(vpci_dev);
>> +    uint32_t queue_size = dev->vdev.conf.queue_size;
>> +
>> +    if (!modern && queue_size > 128) {
>> +        error_setg(errp,
>> +                   "too big queue size (%u, max: 128) "
>> +                   "for non-modern virtio device", queue_size);
>> +        return;
>> +    }
>
> this enables for transitional so still visible to legacy
> interface. I am guessing you want to check whether
> device is accessed through the modern interface instead.

My goal is to not break something when I'm setting the queue size > 128 
(taking into account the current seabios queue size restriction to 128). 
I'm not quite sure what to check. Could I ask why one want to the check 
whether accessing through the modern interface and how it could be checked?

Thanks!

Denis

>>       if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>>           vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
>> diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
>> index 2830849729..6e6790fda5 100644
>> --- a/hw/virtio/virtio-scsi-pci.c
>> +++ b/hw/virtio/virtio-scsi-pci.c
>> @@ -17,6 +17,7 @@
>>   
>>   #include "hw/virtio/virtio-scsi.h"
>>   #include "virtio-pci.h"
>> +#include "qapi/error.h"
>>   
>>   typedef struct VirtIOSCSIPCI VirtIOSCSIPCI;
>>   
>> @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>>       VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>>       DeviceState *proxy = DEVICE(vpci_dev);
>>       char *bus_name;
>> +    bool modern = virtio_pci_modern(vpci_dev);
>> +    uint32_t virtqueue_size = vs->conf.virtqueue_size;
>> +
>> +    if (!modern && virtqueue_size > 128) {
>> +        error_setg(errp,
>> +                   "too big virtqueue size (%u, max: 128) "
>> +                   "for non-modern virtio device", virtqueue_size);
>> +        return;
>> +    }
> why? what is illegal about 256 for legacy?
>
>>   
>>       if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
>>           vpci_dev->nvectors = vs->conf.num_queues + 3;
>> -- 
>> 2.17.0

Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

On Wed, Nov 06, 2019 at 07:46:31AM +0000, Denis Plotnikov wrote:
> 
> On 05.11.2019 23:56, Michael S. Tsirkin wrote:
> > On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
> >> The patch protects from creating illegal virtio device configuration
> >> via direct virtqueue size property setting.
> >>
> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >> ---
> >>   hw/virtio/virtio-blk-pci.c  |  9 +++++++++
> >>   hw/virtio/virtio-scsi-pci.c | 10 ++++++++++
> >>   2 files changed, 19 insertions(+)
> >>
> >> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
> >> index 60c9185c39..6177ff1df8 100644
> >> --- a/hw/virtio/virtio-blk-pci.c
> >> +++ b/hw/virtio/virtio-blk-pci.c
> >> @@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> >>   {
> >>       VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
> >>       DeviceState *vdev = DEVICE(&dev->vdev);
> >> +    bool modern = virtio_pci_modern(vpci_dev);
> >> +    uint32_t queue_size = dev->vdev.conf.queue_size;
> >> +
> >> +    if (!modern && queue_size > 128) {
> >> +        error_setg(errp,
> >> +                   "too big queue size (%u, max: 128) "
> >> +                   "for non-modern virtio device", queue_size);
> >> +        return;
> >> +    }
> >
> > this enables for transitional so still visible to legacy
> > interface. I am guessing you want to check whether
> > device is accessed through the modern interface instead.
> 
> My goal is to not break something when I'm setting the queue size > 128 
> (taking into account the current seabios queue size restriction to 128). 
> I'm not quite sure what to check. Could I ask why one want to the check 
> whether accessing through the modern interface and how it could be checked?

virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

On Tue, Nov 05, 2019 at 03:56:43PM -0500, Michael S. Tsirkin wrote:
> On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
> > @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> >      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
> >      DeviceState *proxy = DEVICE(vpci_dev);
> >      char *bus_name;
> > +    bool modern = virtio_pci_modern(vpci_dev);
> > +    uint32_t virtqueue_size = vs->conf.virtqueue_size;
> > +
> > +    if (!modern && virtqueue_size > 128) {
> > +        error_setg(errp,
> > +                   "too big virtqueue size (%u, max: 128) "
> > +                   "for non-modern virtio device", virtqueue_size);
> > +        return;
> > +    }
> 
> why? what is illegal about 256 for legacy?

I think it was mentioned that this limit is specific to SeaBIOS
src/hw/virtio-pci.c:vp_find_vq():

  #define MAX_QUEUE_NUM      (128)
  ...
  if (num > MAX_QUEUE_NUM) {
      dprintf(1, "ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM);
      goto fail;
  }

I'm not sure there is anything we can do in QEMU.  Either you can let
SeaBIOS fail, or if you want something more user-friendly, then the
management tool can implement a check based on the SeaBIOS version and
the -device virtio-blk-pci,queue-size=SIZE property value.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

On Wed, Nov 06, 2019 at 07:46:31AM +0000, Denis Plotnikov wrote:
> Could I ask why one want to the check 
> whether accessing through the modern interface and how it could be checked?

BTW the VERSION_1 check I mentioned won't work in .realize() since
feature negotiation happens later at guest run-time.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> It tests proper queue size settings for all available machine types.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  tests/qemu-iotests/267     | 154 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/267.out |   1 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 156 insertions(+)
>  create mode 100755 tests/qemu-iotests/267
>  create mode 100644 tests/qemu-iotests/267.out

The qemu-iotests maintainers might prefer for this to be at the
top-level in tests/ since it's not really an iotest, but the code itself
looks fine to me:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1163 bytes --]

On 06.11.19 10:24, Stefan Hajnoczi wrote:
> On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
>> It tests proper queue size settings for all available machine types.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>>  tests/qemu-iotests/267     | 154 +++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/267.out |   1 +
>>  tests/qemu-iotests/group   |   1 +
>>  3 files changed, 156 insertions(+)
>>  create mode 100755 tests/qemu-iotests/267
>>  create mode 100644 tests/qemu-iotests/267.out
> 
> The qemu-iotests maintainers might prefer for this to be at the
> top-level in tests/ since it's not really an iotest, but the code itself
> looks fine to me:
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Good question.  I don’t really mind, but it would be weird if started
adding all kinds of “external” qemu tests (i.e. that use QMP) in the
iotests directory.

What is the alternative?  Just putting it in a different directory
doesn’t sound that appealing to me either, because it would still depend
on the iotests infrastructure, right?  (i.e., iotests.py and check)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Denis Lunev]

On 11/5/19 9:51 PM, Michael S. Tsirkin wrote:
> On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
>> seg_max has a restriction to be less or equal to virtqueue size
>> according to Virtio 1.0 specification
>>
>> Although seg_max can't be set directly, it's worth to express this
>> dependancy directly in the code for sanity purpose.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> This is guest visible so needs to be machine type dependent, right?

we have discussed this verbally with Stefan and think that
there is no need to add that to the machine type as:

- original default was 126, which matches 128 as queue
  length in old machine types
- queue length > 128 is not observed in the field as
  SeaBios has quirk that asserts
- if queue length will be set to something < 128 - linux
  guest will crash

If we really need to preserve original __buggy__ behavior -
we can add boolean property, pls let us know.

Den

>
>> ---
>>  hw/block/virtio-blk.c | 2 +-
>>  hw/scsi/virtio-scsi.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 06e57a4d39..21530304cf 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>>      blk_get_geometry(s->blk, &capacity);
>>      memset(&blkcfg, 0, sizeof(blkcfg));
>>      virtio_stq_p(vdev, &blkcfg.capacity, capacity);
>> -    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
>> +    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
>>      virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
>>      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
>>      virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 839f120256..f7e5533cd5 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>>  
>>      virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
>> -    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
>> +    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.virtqueue_size - 2);
>>      virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
>>      virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
>>      virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
>> -- 
>> 2.17.0

Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Michael S. Tsirkin]

On Wed, Nov 06, 2019 at 07:46:31AM +0000, Denis Plotnikov wrote:
> 
> On 05.11.2019 23:56, Michael S. Tsirkin wrote:
> > On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
> >> The patch protects from creating illegal virtio device configuration
> >> via direct virtqueue size property setting.
> >>
> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >> ---
> >>   hw/virtio/virtio-blk-pci.c  |  9 +++++++++
> >>   hw/virtio/virtio-scsi-pci.c | 10 ++++++++++
> >>   2 files changed, 19 insertions(+)
> >>
> >> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
> >> index 60c9185c39..6177ff1df8 100644
> >> --- a/hw/virtio/virtio-blk-pci.c
> >> +++ b/hw/virtio/virtio-blk-pci.c
> >> @@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> >>   {
> >>       VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
> >>       DeviceState *vdev = DEVICE(&dev->vdev);
> >> +    bool modern = virtio_pci_modern(vpci_dev);
> >> +    uint32_t queue_size = dev->vdev.conf.queue_size;
> >> +
> >> +    if (!modern && queue_size > 128) {
> >> +        error_setg(errp,
> >> +                   "too big queue size (%u, max: 128) "
> >> +                   "for non-modern virtio device", queue_size);
> >> +        return;
> >> +    }
> >
> > this enables for transitional so still visible to legacy
> > interface. I am guessing you want to check whether
> > device is accessed through the modern interface instead.
> 
> My goal is to not break something when I'm setting the queue size > 128 
> (taking into account the current seabios queue size restriction to 128). 
> I'm not quite sure what to check. Could I ask why one want to the check 
> whether accessing through the modern interface

Well now that you say that I don't really know why did you put this test
in here.  I was guessing you wanted modern because with modern queue
size is not forced by the host, guest can always use a smaller queue.
So it's safe to have a large queue.  But if not maybe you can comment on
why this is limited like this, and add a code comment here.

> and how it could be checked?


As Stefan said, you can look at the features.
But you can't do it from realize, you need to do it after guest
from the set features or validate features or set status callback.
I think validate features is the easiest to use of the three.

This calls for an API to resize queues which we
do not have now, but it's not hard to add.


> Thanks!
> 
> Denis
> 
> >>       if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> >>           vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
> >> diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> >> index 2830849729..6e6790fda5 100644
> >> --- a/hw/virtio/virtio-scsi-pci.c
> >> +++ b/hw/virtio/virtio-scsi-pci.c
> >> @@ -17,6 +17,7 @@
> >>   
> >>   #include "hw/virtio/virtio-scsi.h"
> >>   #include "virtio-pci.h"
> >> +#include "qapi/error.h"
> >>   
> >>   typedef struct VirtIOSCSIPCI VirtIOSCSIPCI;
> >>   
> >> @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> >>       VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
> >>       DeviceState *proxy = DEVICE(vpci_dev);
> >>       char *bus_name;
> >> +    bool modern = virtio_pci_modern(vpci_dev);
> >> +    uint32_t virtqueue_size = vs->conf.virtqueue_size;
> >> +
> >> +    if (!modern && virtqueue_size > 128) {
> >> +        error_setg(errp,
> >> +                   "too big virtqueue size (%u, max: 128) "
> >> +                   "for non-modern virtio device", virtqueue_size);
> >> +        return;
> >> +    }
> > why? what is illegal about 256 for legacy?
> >
> >>   
> >>       if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> >>           vpci_dev->nvectors = vs->conf.num_queues + 3;
> >> -- 
> >> 2.17.0

Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Michael S. Tsirkin]

On Wed, Nov 06, 2019 at 10:18:12AM +0100, Stefan Hajnoczi wrote:
> On Tue, Nov 05, 2019 at 03:56:43PM -0500, Michael S. Tsirkin wrote:
> > On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
> > > @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> > >      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
> > >      DeviceState *proxy = DEVICE(vpci_dev);
> > >      char *bus_name;
> > > +    bool modern = virtio_pci_modern(vpci_dev);
> > > +    uint32_t virtqueue_size = vs->conf.virtqueue_size;
> > > +
> > > +    if (!modern && virtqueue_size > 128) {
> > > +        error_setg(errp,
> > > +                   "too big virtqueue size (%u, max: 128) "
> > > +                   "for non-modern virtio device", virtqueue_size);
> > > +        return;
> > > +    }
> > 
> > why? what is illegal about 256 for legacy?
> 
> I think it was mentioned that this limit is specific to SeaBIOS
> src/hw/virtio-pci.c:vp_find_vq():
> 
>   #define MAX_QUEUE_NUM      (128)
>   ...
>   if (num > MAX_QUEUE_NUM) {
>       dprintf(1, "ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM);
>       goto fail;
>   }

OK I see. It's worth documenting this (with version of seabios
that has the issue).
And yes virtio_pci_modern will not do the right thing.
This checks whether device has the modern interface, but
an old seabios will not use the modern interface even
if it's there.

You want to start with small queues and then check after features have
been negotiated with firmware, and make them bigger.


But even then I am not so sure we should just block
bigger queues by default. kernel can use bigger queues fine,
and not all disks are necessarily used by seabios.


If you want a user friendly option, we can add a flag that tells
qemu to adjust the size to a known safe value.
Then we'd start small and make it bigger if guest is a modern one.




> I'm not sure there is anything we can do in QEMU.  Either you can let
> SeaBIOS fail, or if you want something more user-friendly, then the
> management tool can implement a check based on the SeaBIOS version and
> the -device virtio-blk-pci,queue-size=SIZE property value.
> 
> Stefan

Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Michael S. Tsirkin]

On Wed, Nov 06, 2019 at 10:07:02AM +0000, Denis Lunev wrote:
> On 11/5/19 9:51 PM, Michael S. Tsirkin wrote:
> > On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
> >> seg_max has a restriction to be less or equal to virtqueue size
> >> according to Virtio 1.0 specification
> >>
> >> Although seg_max can't be set directly, it's worth to express this
> >> dependancy directly in the code for sanity purpose.
> >>
> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > This is guest visible so needs to be machine type dependent, right?
> 
> we have discussed this verbally with Stefan and think that
> there is no need to add that to the machine type as:
> 
> - original default was 126, which matches 128 as queue
>   length in old machine types
> - queue length > 128 is not observed in the field as
>   SeaBios has quirk that asserts

Well that's just the SeaBios virtio driver. Not everyone's using that to
drive their devices.

> - if queue length will be set to something < 128 - linux
>   guest will crash

Again that's just one guest driver. Not everyone is using that either.


> If we really need to preserve original __buggy__ behavior -
> we can add boolean property, pls let us know.
> 
> Den

Looks like some drivers are buggy but I'm not sure it's
the same as saying the behavior is buggy.
So yes, I'd say it's preferable to be compatible.


> >> ---
> >>  hw/block/virtio-blk.c | 2 +-
> >>  hw/scsi/virtio-scsi.c | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 06e57a4d39..21530304cf 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
> >>      blk_get_geometry(s->blk, &capacity);
> >>      memset(&blkcfg, 0, sizeof(blkcfg));
> >>      virtio_stq_p(vdev, &blkcfg.capacity, capacity);
> >> -    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
> >> +    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
> >>      virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
> >>      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
> >>      virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> >> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> >> index 839f120256..f7e5533cd5 100644
> >> --- a/hw/scsi/virtio-scsi.c
> >> +++ b/hw/scsi/virtio-scsi.c
> >> @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
> >>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> >>  
> >>      virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
> >> -    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> >> +    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.virtqueue_size - 2);
> >>      virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
> >>      virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
> >>      virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> >> -- 
> >> 2.17.0
> 

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Eduardo Habkost]

On Wed, Nov 06, 2019 at 11:04:16AM +0100, Max Reitz wrote:
> On 06.11.19 10:24, Stefan Hajnoczi wrote:
> > On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> >> It tests proper queue size settings for all available machine types.
> >>
> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >> ---
> >>  tests/qemu-iotests/267     | 154 +++++++++++++++++++++++++++++++++++++
> >>  tests/qemu-iotests/267.out |   1 +
> >>  tests/qemu-iotests/group   |   1 +
> >>  3 files changed, 156 insertions(+)
> >>  create mode 100755 tests/qemu-iotests/267
> >>  create mode 100644 tests/qemu-iotests/267.out
> > 
> > The qemu-iotests maintainers might prefer for this to be at the
> > top-level in tests/ since it's not really an iotest, but the code itself
> > looks fine to me:
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Good question.  I don’t really mind, but it would be weird if started
> adding all kinds of “external” qemu tests (i.e. that use QMP) in the
> iotests directory.
> 
> What is the alternative?  Just putting it in a different directory
> doesn’t sound that appealing to me either, because it would still depend
> on the iotests infrastructure, right?  (i.e., iotests.py and check)

We do have tests/acceptance for simple test cases written in
Python.  What's the reason for this test case to depend on the
iotests infrastructure?

-- 
Eduardo

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Cleber Rosa]

On Wed, Nov 06, 2019 at 04:26:41PM -0300, Eduardo Habkost wrote:
> On Wed, Nov 06, 2019 at 11:04:16AM +0100, Max Reitz wrote:
> > On 06.11.19 10:24, Stefan Hajnoczi wrote:
> > > On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> > >> It tests proper queue size settings for all available machine types.
> > >>
> > >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > >> ---
> > >>  tests/qemu-iotests/267     | 154 +++++++++++++++++++++++++++++++++++++
> > >>  tests/qemu-iotests/267.out |   1 +
> > >>  tests/qemu-iotests/group   |   1 +
> > >>  3 files changed, 156 insertions(+)
> > >>  create mode 100755 tests/qemu-iotests/267
> > >>  create mode 100644 tests/qemu-iotests/267.out
> > > 
> > > The qemu-iotests maintainers might prefer for this to be at the
> > > top-level in tests/ since it's not really an iotest, but the code itself
> > > looks fine to me:
> > > 
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > Good question.  I don’t really mind, but it would be weird if started
> > adding all kinds of “external” qemu tests (i.e. that use QMP) in the
> > iotests directory.
> > 
> > What is the alternative?  Just putting it in a different directory
> > doesn’t sound that appealing to me either, because it would still depend
> > on the iotests infrastructure, right?  (i.e., iotests.py and check)
> 
> We do have tests/acceptance for simple test cases written in
> Python.  What's the reason for this test case to depend on the
> iotests infrastructure?
> 
> -- 
> Eduardo

This test does look similar in spirit to "tests/acceptance/virtio_version.py".

Denis,

If you think this is more of a generic test than an IO test, and would
rather want to have it a more agnostic location, I can provide you
with tips (or a patch) to do so.

Cheers,
- Cleber.

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Denis Plotnikov]

On 07.11.2019 19:30, Cleber Rosa wrote:
> On Wed, Nov 06, 2019 at 04:26:41PM -0300, Eduardo Habkost wrote:
>> On Wed, Nov 06, 2019 at 11:04:16AM +0100, Max Reitz wrote:
>>> On 06.11.19 10:24, Stefan Hajnoczi wrote:
>>>> On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
>>>>> It tests proper queue size settings for all available machine types.
>>>>>
>>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>>>> ---
>>>>>   tests/qemu-iotests/267     | 154 +++++++++++++++++++++++++++++++++++++
>>>>>   tests/qemu-iotests/267.out |   1 +
>>>>>   tests/qemu-iotests/group   |   1 +
>>>>>   3 files changed, 156 insertions(+)
>>>>>   create mode 100755 tests/qemu-iotests/267
>>>>>   create mode 100644 tests/qemu-iotests/267.out
>>>> The qemu-iotests maintainers might prefer for this to be at the
>>>> top-level in tests/ since it's not really an iotest, but the code itself
>>>> looks fine to me:
>>>>
>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Good question.  I don’t really mind, but it would be weird if started
>>> adding all kinds of “external” qemu tests (i.e. that use QMP) in the
>>> iotests directory.
>>>
>>> What is the alternative?  Just putting it in a different directory
>>> doesn’t sound that appealing to me either, because it would still depend
>>> on the iotests infrastructure, right?  (i.e., iotests.py and check)
>> We do have tests/acceptance for simple test cases written in
>> Python.  What's the reason for this test case to depend on the
>> iotests infrastructure?
>>
>> -- 
>> Eduardo
> This test does look similar in spirit to "tests/acceptance/virtio_version.py".
>
> Denis,
>
> If you think this is more of a generic test than an IO test, and would
> rather want to have it a more agnostic location, I can provide you
> with tips (or a patch) to do so.

It would be great! Thanks!

Denis

>
> Cheers,
> - Cleber.
>

Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Denis Plotnikov]

The 1st patch from the series seems to be useless. The patch extending 
queue length by adding machine type may break vm-s which use seabios 
with max queue size = 128.

Looks like only this patch doesn't break anything and helps to express 
queue size and seg max dependency (the specification constraint) 
explicitly. So, I would like to re-send this patch as a standalone one 
and send other patches including the test later, when we all agree on 
how exactly to deal with issues posted in the thread.

Any objections are welcome.

Denis

On 06.11.2019 14:54, Michael S. Tsirkin wrote:
> On Wed, Nov 06, 2019 at 10:07:02AM +0000, Denis Lunev wrote:
>> On 11/5/19 9:51 PM, Michael S. Tsirkin wrote:
>>> On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
>>>> seg_max has a restriction to be less or equal to virtqueue size
>>>> according to Virtio 1.0 specification
>>>>
>>>> Although seg_max can't be set directly, it's worth to express this
>>>> dependancy directly in the code for sanity purpose.
>>>>
>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>> This is guest visible so needs to be machine type dependent, right?
>> we have discussed this verbally with Stefan and think that
>> there is no need to add that to the machine type as:
>>
>> - original default was 126, which matches 128 as queue
>>    length in old machine types
>> - queue length > 128 is not observed in the field as
>>    SeaBios has quirk that asserts
> Well that's just the SeaBios virtio driver. Not everyone's using that to
> drive their devices.
>
>> - if queue length will be set to something < 128 - linux
>>    guest will crash
> Again that's just one guest driver. Not everyone is using that either.
>
>
>> If we really need to preserve original __buggy__ behavior -
>> we can add boolean property, pls let us know.
>>
>> Den
> Looks like some drivers are buggy but I'm not sure it's
> the same as saying the behavior is buggy.
> So yes, I'd say it's preferable to be compatible.
>
>
>>>> ---
>>>>   hw/block/virtio-blk.c | 2 +-
>>>>   hw/scsi/virtio-scsi.c | 2 +-
>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>>>> index 06e57a4d39..21530304cf 100644
>>>> --- a/hw/block/virtio-blk.c
>>>> +++ b/hw/block/virtio-blk.c
>>>> @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>>>>       blk_get_geometry(s->blk, &capacity);
>>>>       memset(&blkcfg, 0, sizeof(blkcfg));
>>>>       virtio_stq_p(vdev, &blkcfg.capacity, capacity);
>>>> -    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
>>>> +    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
>>>>       virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
>>>>       virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
>>>>       virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
>>>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>>>> index 839f120256..f7e5533cd5 100644
>>>> --- a/hw/scsi/virtio-scsi.c
>>>> +++ b/hw/scsi/virtio-scsi.c
>>>> @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>>>>       VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>>>>   
>>>>       virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
>>>> -    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
>>>> +    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.virtqueue_size - 2);
>>>>       virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
>>>>       virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
>>>>       virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
>>>> -- 
>>>> 2.17.0

Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Michael S. Tsirkin]

On Fri, Nov 08, 2019 at 07:43:22AM +0000, Denis Plotnikov wrote:
> The 1st patch from the series seems to be useless. The patch extending 
> queue length by adding machine type may break vm-s which use seabios 
> with max queue size = 128.
> 
> Looks like only this patch doesn't break anything and helps to express 
> queue size and seg max dependency (the specification constraint) 
> explicitly. So, I would like to re-send this patch as a standalone one 
> and send other patches including the test later, when we all agree on 
> how exactly to deal with issues posted in the thread.

OK, and I think we should make it machine type dependent.

> Any objections are welcome.
> 
> Denis
> 
> On 06.11.2019 14:54, Michael S. Tsirkin wrote:
> > On Wed, Nov 06, 2019 at 10:07:02AM +0000, Denis Lunev wrote:
> >> On 11/5/19 9:51 PM, Michael S. Tsirkin wrote:
> >>> On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
> >>>> seg_max has a restriction to be less or equal to virtqueue size
> >>>> according to Virtio 1.0 specification
> >>>>
> >>>> Although seg_max can't be set directly, it's worth to express this
> >>>> dependancy directly in the code for sanity purpose.
> >>>>
> >>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >>> This is guest visible so needs to be machine type dependent, right?
> >> we have discussed this verbally with Stefan and think that
> >> there is no need to add that to the machine type as:
> >>
> >> - original default was 126, which matches 128 as queue
> >>    length in old machine types
> >> - queue length > 128 is not observed in the field as
> >>    SeaBios has quirk that asserts
> > Well that's just the SeaBios virtio driver. Not everyone's using that to
> > drive their devices.
> >
> >> - if queue length will be set to something < 128 - linux
> >>    guest will crash
> > Again that's just one guest driver. Not everyone is using that either.
> >
> >
> >> If we really need to preserve original __buggy__ behavior -
> >> we can add boolean property, pls let us know.
> >>
> >> Den
> > Looks like some drivers are buggy but I'm not sure it's
> > the same as saying the behavior is buggy.
> > So yes, I'd say it's preferable to be compatible.
> >
> >
> >>>> ---
> >>>>   hw/block/virtio-blk.c | 2 +-
> >>>>   hw/scsi/virtio-scsi.c | 2 +-
> >>>>   2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >>>> index 06e57a4d39..21530304cf 100644
> >>>> --- a/hw/block/virtio-blk.c
> >>>> +++ b/hw/block/virtio-blk.c
> >>>> @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
> >>>>       blk_get_geometry(s->blk, &capacity);
> >>>>       memset(&blkcfg, 0, sizeof(blkcfg));
> >>>>       virtio_stq_p(vdev, &blkcfg.capacity, capacity);
> >>>> -    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
> >>>> +    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
> >>>>       virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
> >>>>       virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
> >>>>       virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> >>>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> >>>> index 839f120256..f7e5533cd5 100644
> >>>> --- a/hw/scsi/virtio-scsi.c
> >>>> +++ b/hw/scsi/virtio-scsi.c
> >>>> @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
> >>>>       VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> >>>>   
> >>>>       virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
> >>>> -    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> >>>> +    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.virtqueue_size - 2);
> >>>>       virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
> >>>>       virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
> >>>>       virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> >>>> -- 
> >>>> 2.17.0