* [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent
@ 2019-12-20 14:09 Denis Plotnikov
2019-12-20 14:09 ` [PATCH v5 1/2] " Denis Plotnikov
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Denis Plotnikov @ 2019-12-20 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, fam, ehabkost, mst, mreitz, stefanha, pbonzini, den
v5:
* rebased on the recent master [MST]
* NOTE: the test doesn't pass because 5.0 machine type use 4.2 compat
instead of it's own or no compat at all. The test will pass
once the new 5.0 compat is used.
v4:
* rebased on 4.2 [MST]
v3:
* add property to set in machine type [MST]
* add min queue size check [Stefan]
* add avocado based test [Max, Stefan, Eduardo, Cleber]
v2:
* the standalone patch to make seg_max virtqueue size dependent
* other patches are postponed
v1:
the initial series
Denis Plotnikov (2):
virtio: make seg_max virtqueue size dependent
tests: add virtio-scsi and virtio-blk seg_max_adjust test
hw/block/virtio-blk.c | 9 +-
hw/core/machine.c | 3 +
hw/scsi/vhost-scsi.c | 2 +
hw/scsi/virtio-scsi.c | 10 +-
include/hw/virtio/virtio-blk.h | 1 +
include/hw/virtio/virtio-scsi.h | 1 +
tests/acceptance/virtio_seg_max_adjust.py | 134 ++++++++++++++++++++++
7 files changed, 158 insertions(+), 2 deletions(-)
create mode 100755 tests/acceptance/virtio_seg_max_adjust.py
--
2.17.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v5 1/2] virtio: make seg_max virtqueue size dependent 2019-12-20 14:09 [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent Denis Plotnikov @ 2019-12-20 14:09 ` Denis Plotnikov 2019-12-20 14:09 ` [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test Denis Plotnikov 2019-12-22 13:31 ` [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent Michael S. Tsirkin 2 siblings, 0 replies; 9+ messages in thread From: Denis Plotnikov @ 2019-12-20 14:09 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, fam, ehabkost, mst, mreitz, stefanha, pbonzini, den Before the patch, seg_max parameter was immutable and hardcoded to 126 (128 - 2) without respect to queue size. This has two negative effects: 1. when queue size is < 128, we have Virtio 1.1 specfication violation: (2.6.5.3.1 Driver Requirements) seq_max must be <= queue_size. This violation affects the old Linux guests (ver < 4.14). These guests crash on these queue_size setups. 2. when queue_size > 128, as was pointed out by Denis Lunev <den@virtuozzo.com>, seg_max restrics guest's block request length which affects guests' performance making them issues more block request than needed. https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg03721.html To mitigate this two effects, the patch adds the property adjusting seg_max to queue size automaticaly. Since seg_max is a guest visible parameter, the property is machine type managable and allows to choose between old (seg_max = 126 always) and new (seg_max = queue_size - 2) behaviors. Not to change the behavior of the older VMs, prevent setting the default seg_max_adjust value for older machine types. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> --- hw/block/virtio-blk.c | 9 ++++++++- hw/core/machine.c | 3 +++ hw/scsi/vhost-scsi.c | 2 ++ hw/scsi/virtio-scsi.c | 10 +++++++++- include/hw/virtio/virtio-blk.h | 1 + include/hw/virtio/virtio-scsi.h | 1 + 6 files changed, 24 insertions(+), 2 deletions(-) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index d62e6377c2..0f6f8113b7 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -908,7 +908,8 @@ 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.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 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); @@ -1133,6 +1134,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) error_setg(errp, "num-queues property must be larger than 0"); return; } + if (conf->queue_size <= 2) { + error_setg(errp, "invalid queue-size property (%" PRIu16 "), " + "must be > 2", conf->queue_size); + return; + } if (!is_power_of_2(conf->queue_size) || conf->queue_size > VIRTQUEUE_MAX_SIZE) { error_setg(errp, "invalid queue-size property (%" PRIu16 "), " @@ -1262,6 +1268,7 @@ static Property virtio_blk_properties[] = { true), DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), + DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, IOThread *), DEFINE_PROP_BIT64("discard", VirtIOBlock, host_features, diff --git a/hw/core/machine.c b/hw/core/machine.c index 023548b4f3..bfa320387e 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -29,6 +29,9 @@ GlobalProperty hw_compat_4_2[] = { { "virtio-blk-device", "x-enable-wce-if-config-wce", "off" }, + { "virtio-blk-device", "seg-max-adjust", "off"}, + { "virtio-scsi-device", "seg_max_adjust", "off"}, + { "vhost-blk-device", "seg_max_adjust", "off"}, }; const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index c693fc748a..26f710d3ec 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -275,6 +275,8 @@ static Property vhost_scsi_properties[] = { DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size, 128), + DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSICommon, conf.seg_max_adjust, + true), DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors, 0xFFFF), DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128), diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index e8b2b64d09..405cb6c953 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -654,7 +654,8 @@ 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.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 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)); @@ -893,6 +894,11 @@ void virtio_scsi_common_realize(DeviceState *dev, virtio_cleanup(vdev); return; } + if (s->conf.virtqueue_size <= 2) { + error_setg(errp, "invalid virtqueue_size property (= %" PRIu16 "), " + "must be > 2", s->conf.virtqueue_size); + return; + } s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues); s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE; s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE; @@ -949,6 +955,8 @@ static Property virtio_scsi_properties[] = { DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, parent_obj.conf.virtqueue_size, 128), + DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, + parent_obj.conf.seg_max_adjust, true), DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, 0xFFFF), DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSI, parent_obj.conf.cmd_per_lun, diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index 9c19f5b634..1e62f869b2 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -38,6 +38,7 @@ struct VirtIOBlkConf uint32_t request_merging; uint16_t num_queues; uint16_t queue_size; + bool seg_max_adjust; uint32_t max_discard_sectors; uint32_t max_write_zeroes_sectors; bool x_enable_wce_if_config_wce; diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 122f7c4b6f..24e768909d 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -48,6 +48,7 @@ typedef struct virtio_scsi_config VirtIOSCSIConfig; struct VirtIOSCSIConf { uint32_t num_queues; uint32_t virtqueue_size; + bool seg_max_adjust; uint32_t max_sectors; uint32_t cmd_per_lun; #ifdef CONFIG_VHOST_SCSI -- 2.17.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test 2019-12-20 14:09 [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent Denis Plotnikov 2019-12-20 14:09 ` [PATCH v5 1/2] " Denis Plotnikov @ 2019-12-20 14:09 ` Denis Plotnikov 2020-01-22 20:56 ` Philippe Mathieu-Daudé 2019-12-22 13:31 ` [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent Michael S. Tsirkin 2 siblings, 1 reply; 9+ messages in thread From: Denis Plotnikov @ 2019-12-20 14:09 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, fam, ehabkost, mst, mreitz, stefanha, pbonzini, den It tests proper seg_max_adjust settings for all machine types except 'none', 'isapc', 'microvm' Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> --- tests/acceptance/virtio_seg_max_adjust.py | 134 ++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100755 tests/acceptance/virtio_seg_max_adjust.py diff --git a/tests/acceptance/virtio_seg_max_adjust.py b/tests/acceptance/virtio_seg_max_adjust.py new file mode 100755 index 0000000000..5458573138 --- /dev/null +++ b/tests/acceptance/virtio_seg_max_adjust.py @@ -0,0 +1,134 @@ +#!/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 + +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python')) +from qemu.machine import QEMUMachine +from avocado_qemu import Test + +#list of machine types and virtqueue properties to test +VIRTIO_SCSI_PROPS = {'seg_max_adjust': 'seg_max_adjust'} +VIRTIO_BLK_PROPS = {'seg_max_adjust': 'seg-max-adjust'} + +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']} + + +class VirtioMaxSegSettingsCheck(Test): + @staticmethod + def make_pattern(props): + pattern_items = ['{0} = \w+'.format(prop) for prop in props] + return '|'.join(pattern_items) + + def query_virtqueue(self, vm, dev_type_name): + query_ok = False + error = None + props = None + + output = vm.command('human-monitor-command', + command_line = 'info qtree') + props_list = DEV_TYPES[dev_type_name].values(); + pattern = self.make_pattern(props_list) + res = re.findall(pattern, output) + + if len(res) != len(props_list): + props_list = set(props_list) + res = set(res) + not_found = props_list.difference(res) + not_found = ', '.join(not_found) + error = '({0}): The following properties not found: {1}'\ + .format(dev_type_name, not_found) + else: + query_ok = True + props = dict() + for prop in res: + p = prop.split(' = ') + props[p[0]] = p[1] + return query_ok, props, error + + def check_mt(self, mt, dev_type_name): + with QEMUMachine(self.qemu_bin) as vm: + vm.set_machine(mt["name"]) + for s in VM_DEV_PARAMS[dev_type_name]: + vm.add_args(s) + vm.launch() + query_ok, props, error = self.query_virtqueue(vm, dev_type_name) + + if not query_ok: + self.fail('machine type {0}: {1}'.format(mt['name'], error)) + + for prop_name, prop_val in props.items(): + expected_val = mt[prop_name] + self.assertEqual(expected_val, prop_val) + + @staticmethod + def seg_max_adjust_enabled(mt): + # machine types >= 5.0 should have seg_max_adjust = true + # others seg_max_adjust = false + mt = mt.split("-") + + # machine types with one line name and name 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("."); + + # versions >= 5.0 goes with seg_max_adjust enabled + major = int(ver[0]) + + if major >= 5: + return True + return False + + def test_machine_types(self): + # collect all machine types except 'none', 'isapc', 'microvm' + with QEMUMachine(self.qemu_bin) as vm: + vm.launch() + machines = [m['name'] for m in vm.command('query-machines')] + vm.shutdown() + machines.remove('none') + machines.remove('isapc') + machines.remove('microvm') + + for dev_type in DEV_TYPES: + # create the list of machine types and their parameters. + mtypes = list() + for m in machines: + if self.seg_max_adjust_enabled(m): + enabled = 'true' + else: + enabled = 'false' + mtypes.append({'name': m, + DEV_TYPES[dev_type]['seg_max_adjust']: enabled}) + + # test each machine type for a device type + for mt in mtypes: + self.check_mt(mt, dev_type) -- 2.17.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test 2019-12-20 14:09 ` [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test Denis Plotnikov @ 2020-01-22 20:56 ` Philippe Mathieu-Daudé 2020-01-22 21:47 ` Philippe Mathieu-Daudé 2020-01-23 12:43 ` Wainer dos Santos Moschetta 0 siblings, 2 replies; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2020-01-22 20:56 UTC (permalink / raw) To: Denis Plotnikov, qemu-devel, ehabkost, mst, stefanha, Markus Armbruster Cc: kwolf, fam, Hannes Reinecke, den, mreitz, Cleber Rosa, pbonzini, Christoph Hellwig Hello, On 12/20/19 3:09 PM, Denis Plotnikov wrote: > It tests proper seg_max_adjust settings for all machine types except > 'none', 'isapc', 'microvm' > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > --- > tests/acceptance/virtio_seg_max_adjust.py | 134 ++++++++++++++++++++++ > 1 file changed, 134 insertions(+) > create mode 100755 tests/acceptance/virtio_seg_max_adjust.py > > diff --git a/tests/acceptance/virtio_seg_max_adjust.py b/tests/acceptance/virtio_seg_max_adjust.py > new file mode 100755 > index 0000000000..5458573138 > --- /dev/null > +++ b/tests/acceptance/virtio_seg_max_adjust.py > @@ -0,0 +1,134 @@ > +#!/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 > + > +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python')) > +from qemu.machine import QEMUMachine > +from avocado_qemu import Test > + > +#list of machine types and virtqueue properties to test > +VIRTIO_SCSI_PROPS = {'seg_max_adjust': 'seg_max_adjust'} > +VIRTIO_BLK_PROPS = {'seg_max_adjust': 'seg-max-adjust'} > + > +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']} > + > + > +class VirtioMaxSegSettingsCheck(Test): > + @staticmethod > + def make_pattern(props): > + pattern_items = ['{0} = \w+'.format(prop) for prop in props] > + return '|'.join(pattern_items) > + > + def query_virtqueue(self, vm, dev_type_name): > + query_ok = False > + error = None > + props = None > + > + output = vm.command('human-monitor-command', > + command_line = 'info qtree') > + props_list = DEV_TYPES[dev_type_name].values(); > + pattern = self.make_pattern(props_list) > + res = re.findall(pattern, output) > + > + if len(res) != len(props_list): > + props_list = set(props_list) > + res = set(res) > + not_found = props_list.difference(res) > + not_found = ', '.join(not_found) > + error = '({0}): The following properties not found: {1}'\ > + .format(dev_type_name, not_found) > + else: > + query_ok = True > + props = dict() > + for prop in res: > + p = prop.split(' = ') > + props[p[0]] = p[1] > + return query_ok, props, error > + > + def check_mt(self, mt, dev_type_name): > + with QEMUMachine(self.qemu_bin) as vm: > + vm.set_machine(mt["name"]) > + for s in VM_DEV_PARAMS[dev_type_name]: > + vm.add_args(s) > + vm.launch() > + query_ok, props, error = self.query_virtqueue(vm, dev_type_name) > + > + if not query_ok: > + self.fail('machine type {0}: {1}'.format(mt['name'], error)) > + > + for prop_name, prop_val in props.items(): > + expected_val = mt[prop_name] > + self.assertEqual(expected_val, prop_val) > + > + @staticmethod > + def seg_max_adjust_enabled(mt): > + # machine types >= 5.0 should have seg_max_adjust = true > + # others seg_max_adjust = false > + mt = mt.split("-") > + > + # machine types with one line name and name 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("."); > + > + # versions >= 5.0 goes with seg_max_adjust enabled > + major = int(ver[0]) > + > + if major >= 5: > + return True > + return False > + > + def test_machine_types(self): > + # collect all machine types except 'none', 'isapc', 'microvm' > + with QEMUMachine(self.qemu_bin) as vm: > + vm.launch() > + machines = [m['name'] for m in vm.command('query-machines')] > + vm.shutdown() > + machines.remove('none') > + machines.remove('isapc') > + machines.remove('microvm') > + > + for dev_type in DEV_TYPES: > + # create the list of machine types and their parameters. > + mtypes = list() > + for m in machines: > + if self.seg_max_adjust_enabled(m): > + enabled = 'true' > + else: > + enabled = 'false' > + mtypes.append({'name': m, > + DEV_TYPES[dev_type]['seg_max_adjust']: enabled}) > + > + # test each machine type for a device type > + for mt in mtypes: > + self.check_mt(mt, dev_type) This test is failing on OSX: TestFail: machine type pc-i440fx-2.0: <class 'TypeError'> Looking at my job-results/job-2020-01-22T17.54-92b7fae/job.log: Unexpected error in object_property_find() at qom/object.c:1201: qemu-system-x86_64: -device virtio-blk-pci,id=scsi0,drive=drive0: can't apply global virtio-blk-device.scsi=true: Property '.scsi' not found Which makes sense looking at hw/block/virtio-blk.c: 1261 static Property virtio_blk_properties[] = { 1262 DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf), ... 1268 #ifdef __linux__ 1269 DEFINE_PROP_BIT64("scsi", VirtIOBlock, host_features, 1270 VIRTIO_BLK_F_SCSI, false), 1271 #endif Except code moved around, origin is: $ git show 1063b8b15 commit 1063b8b15fb49fcf88ffa282b19aaaf7ca9c678c Author: Christoph Hellwig <hch@lst.de> Date: Mon Apr 27 10:29:14 2009 +0200 virtio-blk: add SGI_IO passthru support Add support for SG_IO passthru (packet commands) to the virtio-blk backend. Conceptually based on an older patch from Hannes Reinecke but largely rewritten to match the code structure and layering in virtio-blk. Note that currently we issue the hose SG_IO synchronously. We could easily switch to async I/O, but that would required either bloating the VirtIOBlockReq by the size of struct sg_io_hdr or an additional memory allocation for each SG_IO request. I'm not sure what is the correct way to fix this. Regards, Phil. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test 2020-01-22 20:56 ` Philippe Mathieu-Daudé @ 2020-01-22 21:47 ` Philippe Mathieu-Daudé 2020-01-23 11:14 ` Cornelia Huck 2020-01-23 12:43 ` Wainer dos Santos Moschetta 1 sibling, 1 reply; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2020-01-22 21:47 UTC (permalink / raw) To: Denis Plotnikov, qemu-devel, ehabkost, mst, stefanha, Markus Armbruster, Cornelia Huck Cc: kwolf, fam, Hannes Reinecke, den, mreitz, Cleber Rosa, pbonzini, Christoph Hellwig Cc'ing Cornelia now ... On 1/22/20 9:56 PM, Philippe Mathieu-Daudé wrote: > Hello, > > On 12/20/19 3:09 PM, Denis Plotnikov wrote: >> It tests proper seg_max_adjust settings for all machine types except >> 'none', 'isapc', 'microvm' >> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >> --- >> tests/acceptance/virtio_seg_max_adjust.py | 134 ++++++++++++++++++++++ >> 1 file changed, 134 insertions(+) >> create mode 100755 tests/acceptance/virtio_seg_max_adjust.py >> >> diff --git a/tests/acceptance/virtio_seg_max_adjust.py >> b/tests/acceptance/virtio_seg_max_adjust.py >> new file mode 100755 >> index 0000000000..5458573138 >> --- /dev/null >> +++ b/tests/acceptance/virtio_seg_max_adjust.py >> @@ -0,0 +1,134 @@ >> +#!/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 >> + >> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', >> 'python')) >> +from qemu.machine import QEMUMachine >> +from avocado_qemu import Test >> + >> +#list of machine types and virtqueue properties to test >> +VIRTIO_SCSI_PROPS = {'seg_max_adjust': 'seg_max_adjust'} >> +VIRTIO_BLK_PROPS = {'seg_max_adjust': 'seg-max-adjust'} >> + >> +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']} >> + >> + >> +class VirtioMaxSegSettingsCheck(Test): >> + @staticmethod >> + def make_pattern(props): >> + pattern_items = ['{0} = \w+'.format(prop) for prop in props] >> + return '|'.join(pattern_items) >> + >> + def query_virtqueue(self, vm, dev_type_name): >> + query_ok = False >> + error = None >> + props = None >> + >> + output = vm.command('human-monitor-command', >> + command_line = 'info qtree') >> + props_list = DEV_TYPES[dev_type_name].values(); >> + pattern = self.make_pattern(props_list) >> + res = re.findall(pattern, output) >> + >> + if len(res) != len(props_list): >> + props_list = set(props_list) >> + res = set(res) >> + not_found = props_list.difference(res) >> + not_found = ', '.join(not_found) >> + error = '({0}): The following properties not found: {1}'\ >> + .format(dev_type_name, not_found) >> + else: >> + query_ok = True >> + props = dict() >> + for prop in res: >> + p = prop.split(' = ') >> + props[p[0]] = p[1] >> + return query_ok, props, error >> + >> + def check_mt(self, mt, dev_type_name): >> + with QEMUMachine(self.qemu_bin) as vm: >> + vm.set_machine(mt["name"]) >> + for s in VM_DEV_PARAMS[dev_type_name]: >> + vm.add_args(s) >> + vm.launch() >> + query_ok, props, error = self.query_virtqueue(vm, >> dev_type_name) >> + >> + if not query_ok: >> + self.fail('machine type {0}: {1}'.format(mt['name'], error)) >> + >> + for prop_name, prop_val in props.items(): >> + expected_val = mt[prop_name] >> + self.assertEqual(expected_val, prop_val) >> + >> + @staticmethod >> + def seg_max_adjust_enabled(mt): >> + # machine types >= 5.0 should have seg_max_adjust = true >> + # others seg_max_adjust = false >> + mt = mt.split("-") >> + >> + # machine types with one line name and name 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("."); >> + >> + # versions >= 5.0 goes with seg_max_adjust enabled >> + major = int(ver[0]) >> + >> + if major >= 5: >> + return True >> + return False >> + >> + def test_machine_types(self): >> + # collect all machine types except 'none', 'isapc', 'microvm' >> + with QEMUMachine(self.qemu_bin) as vm: >> + vm.launch() >> + machines = [m['name'] for m in vm.command('query-machines')] >> + vm.shutdown() >> + machines.remove('none') >> + machines.remove('isapc') >> + machines.remove('microvm') >> + >> + for dev_type in DEV_TYPES: >> + # create the list of machine types and their parameters. >> + mtypes = list() >> + for m in machines: >> + if self.seg_max_adjust_enabled(m): >> + enabled = 'true' >> + else: >> + enabled = 'false' >> + mtypes.append({'name': m, >> + DEV_TYPES[dev_type]['seg_max_adjust']: >> enabled}) >> + >> + # test each machine type for a device type >> + for mt in mtypes: >> + self.check_mt(mt, dev_type) > > This test is failing on OSX: > > TestFail: machine type pc-i440fx-2.0: <class 'TypeError'> > > Looking at my job-results/job-2020-01-22T17.54-92b7fae/job.log: > > Unexpected error in object_property_find() at qom/object.c:1201: > qemu-system-x86_64: -device virtio-blk-pci,id=scsi0,drive=drive0: can't > apply global virtio-blk-device.scsi=true: Property '.scsi' not found > > Which makes sense looking at hw/block/virtio-blk.c: > > 1261 static Property virtio_blk_properties[] = { > 1262 DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf), > ... > 1268 #ifdef __linux__ > 1269 DEFINE_PROP_BIT64("scsi", VirtIOBlock, host_features, > 1270 VIRTIO_BLK_F_SCSI, false), > 1271 #endif > > Except code moved around, origin is: > > $ git show 1063b8b15 > commit 1063b8b15fb49fcf88ffa282b19aaaf7ca9c678c > Author: Christoph Hellwig <hch@lst.de> > Date: Mon Apr 27 10:29:14 2009 +0200 > > virtio-blk: add SGI_IO passthru support > > Add support for SG_IO passthru (packet commands) to the virtio-blk > backend. Conceptually based on an older patch from Hannes Reinecke > but largely rewritten to match the code structure and layering in > virtio-blk. > > Note that currently we issue the hose SG_IO synchronously. We could > easily switch to async I/O, but that would required either bloating > the VirtIOBlockReq by the size of struct sg_io_hdr or an additional > memory allocation for each SG_IO request. > > I'm not sure what is the correct way to fix this. ... because of: $ git show ed65fd1a27 commit ed65fd1a2750d24290354cc7ea49caec7c13e30b Author: Cornelia Huck <cornelia.huck@de.ibm.com> Date: Fri Oct 16 12:25:54 2015 +0200 virtio-blk: switch off scsi-passthrough by default Devices that are compliant with virtio-1 do not support scsi passthrough any more (and it has not been a recommended setup anyway for quite some time). To avoid having to switch it off explicitly in newer qemus that turn on virtio-1 by default, let's switch the default to scsi=false for 2.5. Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> Message-id: 1444991154-79217-4-git-send-email-cornelia.huck@de.ibm.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> diff --git a/include/hw/compat.h b/include/hw/compat.h index 095de5d12f..93e71afb4a 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -2,7 +2,11 @@ #define HW_COMPAT_H #define HW_COMPAT_2_4 \ - /* empty */ + {\ + .driver = "virtio-blk-device",\ + .property = "scsi",\ + .value = "true",\ + }, #define HW_COMPAT_2_3 \ {\ diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 3e230debb8..45a24e4fa6 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -972,7 +972,7 @@ static Property virtio_blk_properties[] = { DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial), DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true), #ifdef __linux__ - DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, true), + DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, false), #endif DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, true), Should this HW_COMPAT_2_4 entry be guarded with ifdef __linux__? Probably nobody ran a pre-2.4 machine out of Linux =) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test 2020-01-22 21:47 ` Philippe Mathieu-Daudé @ 2020-01-23 11:14 ` Cornelia Huck 0 siblings, 0 replies; 9+ messages in thread From: Cornelia Huck @ 2020-01-23 11:14 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: kwolf, fam, Hannes Reinecke, ehabkost, mst, Markus Armbruster, qemu-devel, Denis Plotnikov, stefanha, Cleber Rosa, pbonzini, mreitz, Christoph Hellwig, den On Wed, 22 Jan 2020 22:47:42 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > This test is failing on OSX: > > > > TestFail: machine type pc-i440fx-2.0: <class 'TypeError'> > > > > Looking at my job-results/job-2020-01-22T17.54-92b7fae/job.log: > > > > Unexpected error in object_property_find() at qom/object.c:1201: > > qemu-system-x86_64: -device virtio-blk-pci,id=scsi0,drive=drive0: can't > > apply global virtio-blk-device.scsi=true: Property '.scsi' not found > > > > Which makes sense looking at hw/block/virtio-blk.c: > > > > 1261 static Property virtio_blk_properties[] = { > > 1262 DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf), > > ... > > 1268 #ifdef __linux__ > > 1269 DEFINE_PROP_BIT64("scsi", VirtIOBlock, host_features, > > 1270 VIRTIO_BLK_F_SCSI, false), > > 1271 #endif > > > > Except code moved around, origin is: > > > > $ git show 1063b8b15 > > commit 1063b8b15fb49fcf88ffa282b19aaaf7ca9c678c > > Author: Christoph Hellwig <hch@lst.de> > > Date: Mon Apr 27 10:29:14 2009 +0200 > > > > virtio-blk: add SGI_IO passthru support > > > > Add support for SG_IO passthru (packet commands) to the virtio-blk > > backend. Conceptually based on an older patch from Hannes Reinecke > > but largely rewritten to match the code structure and layering in > > virtio-blk. > > > > Note that currently we issue the hose SG_IO synchronously. We could > > easily switch to async I/O, but that would required either bloating > > the VirtIOBlockReq by the size of struct sg_io_hdr or an additional > > memory allocation for each SG_IO request. > > > > I'm not sure what is the correct way to fix this. > > ... because of: > > $ git show ed65fd1a27 > commit ed65fd1a2750d24290354cc7ea49caec7c13e30b > Author: Cornelia Huck <cornelia.huck@de.ibm.com> > Date: Fri Oct 16 12:25:54 2015 +0200 > > virtio-blk: switch off scsi-passthrough by default > > Devices that are compliant with virtio-1 do not support scsi > passthrough any more (and it has not been a recommended setup > anyway for quite some time). To avoid having to switch it off > explicitly in newer qemus that turn on virtio-1 by default, let's > switch the default to scsi=false for 2.5. > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > Message-id: 1444991154-79217-4-git-send-email-cornelia.huck@de.ibm.com > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 095de5d12f..93e71afb4a 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,11 @@ > #define HW_COMPAT_H > > #define HW_COMPAT_2_4 \ > - /* empty */ > + {\ > + .driver = "virtio-blk-device",\ > + .property = "scsi",\ > + .value = "true",\ > + }, This code has changed a lot in the meantime... > > #define HW_COMPAT_2_3 \ > {\ > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 3e230debb8..45a24e4fa6 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -972,7 +972,7 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial), > DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true), > #ifdef __linux__ > - DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, true), > + DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, false), > #endif > DEFINE_PROP_BIT("request-merging", VirtIOBlock, > conf.request_merging, 0, > true), > > Should this HW_COMPAT_2_4 entry be guarded with ifdef __linux__? ... so something like the following might do the trick: diff --git a/hw/core/machine.c b/hw/core/machine.c index 3e288bfceb7f..d8e30e4895d8 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -148,7 +148,8 @@ GlobalProperty hw_compat_2_5[] = { const size_t hw_compat_2_5_len = G_N_ELEMENTS(hw_compat_2_5); GlobalProperty hw_compat_2_4[] = { - { "virtio-blk-device", "scsi", "true" }, + /* Optional because the 'scsi' property is Linux-only */ + { "virtio-blk-device", "scsi", "true", .optional = true }, { "e1000", "extra_mac_registers", "off" }, { "virtio-pci", "x-disable-pcie", "on" }, { "virtio-pci", "migrate-extra", "off" }, > > Probably nobody ran a pre-2.4 machine out of Linux =) > Yeah. I'm wondering if there's more compat stuff in there that should be optional. Devices that simply do not exist are not a problem, but properties that not always exist are. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test 2020-01-22 20:56 ` Philippe Mathieu-Daudé 2020-01-22 21:47 ` Philippe Mathieu-Daudé @ 2020-01-23 12:43 ` Wainer dos Santos Moschetta 1 sibling, 0 replies; 9+ messages in thread From: Wainer dos Santos Moschetta @ 2020-01-23 12:43 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Denis Plotnikov, qemu-devel, ehabkost, mst, stefanha, Markus Armbruster Cc: kwolf, fam, Hannes Reinecke, den, mreitz, Cleber Rosa, pbonzini, Christoph Hellwig On 1/22/20 6:56 PM, Philippe Mathieu-Daudé wrote: > Hello, > > On 12/20/19 3:09 PM, Denis Plotnikov wrote: >> It tests proper seg_max_adjust settings for all machine types except >> 'none', 'isapc', 'microvm' >> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >> --- >> tests/acceptance/virtio_seg_max_adjust.py | 134 ++++++++++++++++++++++ >> 1 file changed, 134 insertions(+) >> create mode 100755 tests/acceptance/virtio_seg_max_adjust.py >> >> diff --git a/tests/acceptance/virtio_seg_max_adjust.py >> b/tests/acceptance/virtio_seg_max_adjust.py >> new file mode 100755 >> index 0000000000..5458573138 >> --- /dev/null >> +++ b/tests/acceptance/virtio_seg_max_adjust.py >> @@ -0,0 +1,134 @@ >> +#!/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 >> + >> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', >> 'python')) >> +from qemu.machine import QEMUMachine >> +from avocado_qemu import Test >> + >> +#list of machine types and virtqueue properties to test >> +VIRTIO_SCSI_PROPS = {'seg_max_adjust': 'seg_max_adjust'} >> +VIRTIO_BLK_PROPS = {'seg_max_adjust': 'seg-max-adjust'} >> + >> +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']} >> + >> + >> +class VirtioMaxSegSettingsCheck(Test): >> + @staticmethod >> + def make_pattern(props): >> + pattern_items = ['{0} = \w+'.format(prop) for prop in props] >> + return '|'.join(pattern_items) >> + >> + def query_virtqueue(self, vm, dev_type_name): >> + query_ok = False >> + error = None >> + props = None >> + >> + output = vm.command('human-monitor-command', >> + command_line = 'info qtree') >> + props_list = DEV_TYPES[dev_type_name].values(); >> + pattern = self.make_pattern(props_list) >> + res = re.findall(pattern, output) >> + >> + if len(res) != len(props_list): >> + props_list = set(props_list) >> + res = set(res) >> + not_found = props_list.difference(res) >> + not_found = ', '.join(not_found) >> + error = '({0}): The following properties not found: {1}'\ >> + .format(dev_type_name, not_found) >> + else: >> + query_ok = True >> + props = dict() >> + for prop in res: >> + p = prop.split(' = ') >> + props[p[0]] = p[1] >> + return query_ok, props, error >> + >> + def check_mt(self, mt, dev_type_name): >> + with QEMUMachine(self.qemu_bin) as vm: >> + vm.set_machine(mt["name"]) >> + for s in VM_DEV_PARAMS[dev_type_name]: >> + vm.add_args(s) >> + vm.launch() >> + query_ok, props, error = self.query_virtqueue(vm, >> dev_type_name) >> + >> + if not query_ok: >> + self.fail('machine type {0}: {1}'.format(mt['name'], >> error)) >> + >> + for prop_name, prop_val in props.items(): >> + expected_val = mt[prop_name] >> + self.assertEqual(expected_val, prop_val) >> + >> + @staticmethod >> + def seg_max_adjust_enabled(mt): >> + # machine types >= 5.0 should have seg_max_adjust = true >> + # others seg_max_adjust = false >> + mt = mt.split("-") >> + >> + # machine types with one line name and name 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("."); >> + >> + # versions >= 5.0 goes with seg_max_adjust enabled >> + major = int(ver[0]) >> + >> + if major >= 5: >> + return True >> + return False >> + >> + def test_machine_types(self): >> + # collect all machine types except 'none', 'isapc', 'microvm' >> + with QEMUMachine(self.qemu_bin) as vm: >> + vm.launch() >> + machines = [m['name'] for m in >> vm.command('query-machines')] >> + vm.shutdown() >> + machines.remove('none') >> + machines.remove('isapc') >> + machines.remove('microvm') >> + >> + for dev_type in DEV_TYPES: >> + # create the list of machine types and their parameters. >> + mtypes = list() >> + for m in machines: >> + if self.seg_max_adjust_enabled(m): >> + enabled = 'true' >> + else: >> + enabled = 'false' >> + mtypes.append({'name': m, >> + DEV_TYPES[dev_type]['seg_max_adjust']: enabled}) >> + >> + # test each machine type for a device type >> + for mt in mtypes: >> + self.check_mt(mt, dev_type) > > This test is failing on OSX: Kind related... Just yesterday I found a bug on the test case code itself that makes it fail on ppc64le (likely to fail on other arches). I'm working on a fix. - Wainer > > TestFail: machine type pc-i440fx-2.0: <class 'TypeError'> > > Looking at my job-results/job-2020-01-22T17.54-92b7fae/job.log: > > Unexpected error in object_property_find() at qom/object.c:1201: > qemu-system-x86_64: -device virtio-blk-pci,id=scsi0,drive=drive0: > can't apply global virtio-blk-device.scsi=true: Property '.scsi' not > found > > Which makes sense looking at hw/block/virtio-blk.c: > > 1261 static Property virtio_blk_properties[] = { > 1262 DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf), > ... > 1268 #ifdef __linux__ > 1269 DEFINE_PROP_BIT64("scsi", VirtIOBlock, host_features, > 1270 VIRTIO_BLK_F_SCSI, false), > 1271 #endif > > Except code moved around, origin is: > > $ git show 1063b8b15 > commit 1063b8b15fb49fcf88ffa282b19aaaf7ca9c678c > Author: Christoph Hellwig <hch@lst.de> > Date: Mon Apr 27 10:29:14 2009 +0200 > > virtio-blk: add SGI_IO passthru support > > Add support for SG_IO passthru (packet commands) to the virtio-blk > backend. Conceptually based on an older patch from Hannes Reinecke > but largely rewritten to match the code structure and layering in > virtio-blk. > > Note that currently we issue the hose SG_IO synchronously. We could > easily switch to async I/O, but that would required either bloating > the VirtIOBlockReq by the size of struct sg_io_hdr or an additional > memory allocation for each SG_IO request. > > I'm not sure what is the correct way to fix this. > > Regards, > > Phil. > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent 2019-12-20 14:09 [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent Denis Plotnikov 2019-12-20 14:09 ` [PATCH v5 1/2] " Denis Plotnikov 2019-12-20 14:09 ` [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test Denis Plotnikov @ 2019-12-22 13:31 ` Michael S. Tsirkin 2 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2019-12-22 13:31 UTC (permalink / raw) To: Denis Plotnikov Cc: kwolf, fam, ehabkost, qemu-devel, mreitz, stefanha, pbonzini, den On Fri, Dec 20, 2019 at 05:09:03PM +0300, Denis Plotnikov wrote: > v5: > * rebased on the recent master [MST] > * NOTE: the test doesn't pass because 5.0 machine type use 4.2 compat > instead of it's own or no compat at all. The test will pass > once the new 5.0 compat is used. So please fix that up first (in a separate patch). > v4: > * rebased on 4.2 [MST] > > v3: > * add property to set in machine type [MST] > * add min queue size check [Stefan] > * add avocado based test [Max, Stefan, Eduardo, Cleber] > > v2: > * the standalone patch to make seg_max virtqueue size dependent > * other patches are postponed > > v1: > the initial series > > Denis Plotnikov (2): > virtio: make seg_max virtqueue size dependent > tests: add virtio-scsi and virtio-blk seg_max_adjust test > > hw/block/virtio-blk.c | 9 +- > hw/core/machine.c | 3 + > hw/scsi/vhost-scsi.c | 2 + > hw/scsi/virtio-scsi.c | 10 +- > include/hw/virtio/virtio-blk.h | 1 + > include/hw/virtio/virtio-scsi.h | 1 + > tests/acceptance/virtio_seg_max_adjust.py | 134 ++++++++++++++++++++++ > 7 files changed, 158 insertions(+), 2 deletions(-) > create mode 100755 tests/acceptance/virtio_seg_max_adjust.py > > -- > 2.17.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent
@ 2019-12-20 14:04 Denis Plotnikov
2019-12-20 14:04 ` [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test Denis Plotnikov
0 siblings, 1 reply; 9+ messages in thread
From: Denis Plotnikov @ 2019-12-20 14:04 UTC (permalink / raw)
To: qemu-devel; +Cc: fam, ehabkost, kwolf, mreitz, stefanha, pbonzini, mst, den
v5:
* rebased on the recent master [MST]
* NOTE: the test doesn't pass because 5.0 machine type use 4.2 compat
instead of it's own or no compat at all. The test will pass
once the new 5.0 compat is used.
v4:
* rebased on 4.2 [MST]
v3:
* add property to set in machine type [MST]
* add min queue size check [Stefan]
* add avocado based test [Max, Stefan, Eduardo, Cleber]
v2:
* the standalone patch to make seg_max virtqueue size dependent
* other patches are postponed
v1:
the initial series
Denis Plotnikov (2):
virtio: make seg_max virtqueue size dependent
tests: add virtio-scsi and virtio-blk seg_max_adjust test
hw/block/virtio-blk.c | 9 +-
hw/core/machine.c | 3 +
hw/scsi/vhost-scsi.c | 2 +
hw/scsi/virtio-scsi.c | 10 +-
include/hw/virtio/virtio-blk.h | 1 +
include/hw/virtio/virtio-scsi.h | 1 +
tests/acceptance/virtio_seg_max_adjust.py | 134 ++++++++++++++++++++++
7 files changed, 158 insertions(+), 2 deletions(-)
create mode 100755 tests/acceptance/virtio_seg_max_adjust.py
--
2.17.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test 2019-12-20 14:04 Denis Plotnikov @ 2019-12-20 14:04 ` Denis Plotnikov 0 siblings, 0 replies; 9+ messages in thread From: Denis Plotnikov @ 2019-12-20 14:04 UTC (permalink / raw) To: qemu-devel; +Cc: fam, ehabkost, kwolf, mreitz, stefanha, pbonzini, mst, den It tests proper seg_max_adjust settings for all machine types except 'none', 'isapc', 'microvm' Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> --- tests/acceptance/virtio_seg_max_adjust.py | 134 ++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100755 tests/acceptance/virtio_seg_max_adjust.py diff --git a/tests/acceptance/virtio_seg_max_adjust.py b/tests/acceptance/virtio_seg_max_adjust.py new file mode 100755 index 0000000000..5458573138 --- /dev/null +++ b/tests/acceptance/virtio_seg_max_adjust.py @@ -0,0 +1,134 @@ +#!/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 + +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python')) +from qemu.machine import QEMUMachine +from avocado_qemu import Test + +#list of machine types and virtqueue properties to test +VIRTIO_SCSI_PROPS = {'seg_max_adjust': 'seg_max_adjust'} +VIRTIO_BLK_PROPS = {'seg_max_adjust': 'seg-max-adjust'} + +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']} + + +class VirtioMaxSegSettingsCheck(Test): + @staticmethod + def make_pattern(props): + pattern_items = ['{0} = \w+'.format(prop) for prop in props] + return '|'.join(pattern_items) + + def query_virtqueue(self, vm, dev_type_name): + query_ok = False + error = None + props = None + + output = vm.command('human-monitor-command', + command_line = 'info qtree') + props_list = DEV_TYPES[dev_type_name].values(); + pattern = self.make_pattern(props_list) + res = re.findall(pattern, output) + + if len(res) != len(props_list): + props_list = set(props_list) + res = set(res) + not_found = props_list.difference(res) + not_found = ', '.join(not_found) + error = '({0}): The following properties not found: {1}'\ + .format(dev_type_name, not_found) + else: + query_ok = True + props = dict() + for prop in res: + p = prop.split(' = ') + props[p[0]] = p[1] + return query_ok, props, error + + def check_mt(self, mt, dev_type_name): + with QEMUMachine(self.qemu_bin) as vm: + vm.set_machine(mt["name"]) + for s in VM_DEV_PARAMS[dev_type_name]: + vm.add_args(s) + vm.launch() + query_ok, props, error = self.query_virtqueue(vm, dev_type_name) + + if not query_ok: + self.fail('machine type {0}: {1}'.format(mt['name'], error)) + + for prop_name, prop_val in props.items(): + expected_val = mt[prop_name] + self.assertEqual(expected_val, prop_val) + + @staticmethod + def seg_max_adjust_enabled(mt): + # machine types >= 5.0 should have seg_max_adjust = true + # others seg_max_adjust = false + mt = mt.split("-") + + # machine types with one line name and name 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("."); + + # versions >= 5.0 goes with seg_max_adjust enabled + major = int(ver[0]) + + if major >= 5: + return True + return False + + def test_machine_types(self): + # collect all machine types except 'none', 'isapc', 'microvm' + with QEMUMachine(self.qemu_bin) as vm: + vm.launch() + machines = [m['name'] for m in vm.command('query-machines')] + vm.shutdown() + machines.remove('none') + machines.remove('isapc') + machines.remove('microvm') + + for dev_type in DEV_TYPES: + # create the list of machine types and their parameters. + mtypes = list() + for m in machines: + if self.seg_max_adjust_enabled(m): + enabled = 'true' + else: + enabled = 'false' + mtypes.append({'name': m, + DEV_TYPES[dev_type]['seg_max_adjust']: enabled}) + + # test each machine type for a device type + for mt in mtypes: + self.check_mt(mt, dev_type) -- 2.17.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-01-23 14:44 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-20 14:09 [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent Denis Plotnikov 2019-12-20 14:09 ` [PATCH v5 1/2] " Denis Plotnikov 2019-12-20 14:09 ` [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test Denis Plotnikov 2020-01-22 20:56 ` Philippe Mathieu-Daudé 2020-01-22 21:47 ` Philippe Mathieu-Daudé 2020-01-23 11:14 ` Cornelia Huck 2020-01-23 12:43 ` Wainer dos Santos Moschetta 2019-12-22 13:31 ` [PATCH v5 0/2] virtio: make seg_max virtqueue size dependent Michael S. Tsirkin -- strict thread matches above, loose matches on Subject: below -- 2019-12-20 14:04 Denis Plotnikov 2019-12-20 14:04 ` [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test Denis Plotnikov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).