mbox series

[v1,0/9] plugins/next (bug fixes, hwprofile, lockstep)

Message ID 20200602154624.4460-1-alex.bennee@linaro.org (mailing list archive)
Headers show
Series plugins/next (bug fixes, hwprofile, lockstep) | expand

Message

Alex Bennée June 2, 2020, 3:46 p.m. UTC 
Hi,

This is the current state of my plugins tree. It has a few minor fixes
to the headers as well as a bug I found in the cputlb which was
triggered when a pci_config write via io_writex causes the memory
regions to be reset and as a result a flush and potential re-sizing of
the TLB entries. This meant when a plugin looked up details of the
address later there was no TLB entry with the information which got
flagged as an error on the plugins part.

I found the bug while I was looking around trying to figure out what
was going wrong with my virtio code so I implemented a "hwprofile"
plugin to track exactly what devices where seeing writes. There are
some minor associated tweaks to the virtio PCI code to better name the
MemoryRegions and a new helper API which exposes the region names to
the plugin.

The lockstep plugin is essentially unchanged from previous postings
but hasn't seen any review. I'm minded to include it in the next PR
anyway just so it isn't lost next time I need to do an A-B comparison
on something that only diverges in behaviour several million
instructions into an execution.

The following need review:

 - .travis.yml: allow failure for unreliable hosts
 - plugins: new hwprofile plugin
 - plugins: add API to return a name for a IO device
 - hw/virtio/pci: include vdev name in registered PCI sections
 - cputlb: ensure we re-fill the TLB if it has reset
 - tests/plugin: correctly honour io_count
 - plugins: new lockstep plugin for debugging TCG changes

Alex Bennée (7):
  plugins: new lockstep plugin for debugging TCG changes
  tests/plugin: correctly honour io_count
  cputlb: ensure we re-fill the TLB if it has reset
  hw/virtio/pci: include vdev name in registered PCI sections
  plugins: add API to return a name for a IO device
  plugins: new hwprofile plugin
  .travis.yml: allow failure for unreliable hosts

Emilio G. Cota (1):
  qemu-plugin.h: add missing include <stddef.h> to define size_t

Philippe Mathieu-Daudé (1):
  scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header

 include/qemu/qemu-plugin.h |   6 +
 accel/tcg/cputlb.c         |  14 ++
 hw/virtio/virtio-pci.c     |  21 ++-
 plugins/api.c              |  18 ++
 tests/plugin/hwprofile.c   | 248 +++++++++++++++++++++++++++
 tests/plugin/lockstep.c    | 340 +++++++++++++++++++++++++++++++++++++
 tests/plugin/mem.c         |   2 +-
 .travis.yml                |   5 +
 scripts/clean-includes     |   1 +
 tests/plugin/Makefile      |   2 +
 tests/tcg/Makefile.target  |   2 +-
 11 files changed, 649 insertions(+), 10 deletions(-)
 create mode 100644 tests/plugin/hwprofile.c
 create mode 100644 tests/plugin/lockstep.c

Comments

no-reply@patchew.org June 2, 2020, 5:03 p.m. UTC | #1 
Patchew URL: https://patchew.org/QEMU/20200602154624.4460-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200602154624.4460-1-alex.bennee@linaro.org
Subject: [PATCH  v1 0/9] plugins/next (bug fixes, hwprofile, lockstep)
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200602164911.5706-1-alex.bennee@linaro.org -> patchew/20200602164911.5706-1-alex.bennee@linaro.org
Switched to a new branch 'test'
54f2fc0 .travis.yml: allow failure for unreliable hosts
e3ba90e plugins: new hwprofile plugin
084ad01 plugins: add API to return a name for a IO device
77a2374 hw/virtio/pci: include vdev name in registered PCI sections
18ec36e cputlb: ensure we re-fill the TLB if it has reset
e341c5e tests/plugin: correctly honour io_count
cf0780c scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header
1ec5362 qemu-plugin.h: add missing include <stddef.h> to define size_t
3e2c523 plugins: new lockstep plugin for debugging TCG changes

=== OUTPUT BEGIN ===
1/9 Checking commit 3e2c523716d9 (plugins: new lockstep plugin for debugging TCG changes)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#55: 
new file mode 100644

total: 0 errors, 1 warnings, 355 lines checked

Patch 1/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/9 Checking commit 1ec536281b2d (qemu-plugin.h: add missing include <stddef.h> to define size_t)
3/9 Checking commit cf0780c17d3c (scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header)
4/9 Checking commit e341c5e754f4 (tests/plugin: correctly honour io_count)
5/9 Checking commit 18ec36e424f5 (cputlb: ensure we re-fill the TLB if it has reset)
6/9 Checking commit 77a2374e2c88 (hw/virtio/pci: include vdev name in registered PCI sections)
WARNING: line over 80 characters
#23: FILE: hw/virtio/virtio-pci.c:1393:
+static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy, const char *vdev_name)

total: 0 errors, 1 warnings, 63 lines checked

Patch 6/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/9 Checking commit 084ad019dd0b (plugins: add API to return a name for a IO device)
ERROR: "foo * bar" should be "foo *bar"
#23: FILE: include/qemu/qemu-plugin.h:341:
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);

ERROR: "foo * bar" should be "foo *bar"
#36: FILE: plugins/api.c:306:
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)

WARNING: line over 80 characters
#42: FILE: plugins/api.c:312:
+            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);

total: 2 errors, 1 warnings, 35 lines checked

Patch 7/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

8/9 Checking commit e3ba90ecbf5b (plugins: new hwprofile plugin)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#30: 
new file mode 100644

ERROR: line over 90 characters
#141: FILE: tests/plugin/hwprofile.c:107:
+                g_string_append_printf(report, "%s @ 0x%"PRIx64"\n", rec->name, rec->base);

WARNING: line over 80 characters
#144: FILE: tests/plugin/hwprofile.c:110:
+                    g_string_append_printf(report, "  off:%08"PRIx64, loc->offset);

ERROR: suspect code indent for conditional statements (20, 23)
#149: FILE: tests/plugin/hwprofile.c:115:
+                    if (track_writes()) {
+                       g_string_append_printf(report, ", 0x%04x, %"PRId64,

ERROR: space required after that ',' (ctx:VxV)
#153: FILE: tests/plugin/hwprofile.c:119:
+                    g_string_append_c(report,'\n');
                                             ^

ERROR: "foo * bar" should be "foo *bar"
#177: FILE: tests/plugin/hwprofile.c:143:
+static DeviceCounts * new_count(char *name, uint64_t base)

ERROR: "foo * bar" should be "foo *bar"
#189: FILE: tests/plugin/hwprofile.c:155:
+static IOLocationCounts * new_location(uint64_t offset)

ERROR: line over 90 characters
#219: FILE: tests/plugin/hwprofile.c:185:
+            IOLocationCounts *io_count = g_hash_table_lookup(counts->access_pattern, &off);

total: 6 errors, 2 warnings, 255 lines checked

Patch 8/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

9/9 Checking commit 54f2fc0935c4 (.travis.yml: allow failure for unreliable hosts)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200602154624.4460-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
no-reply@patchew.org June 2, 2020, 7:16 p.m. UTC | #2 
Patchew URL: https://patchew.org/QEMU/20200602154624.4460-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200602154624.4460-1-alex.bennee@linaro.org
Subject: [PATCH  v1 0/9] plugins/next (bug fixes, hwprofile, lockstep)
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
5e3c9f6 .travis.yml: allow failure for unreliable hosts
a829a1c plugins: new hwprofile plugin
fb39889 plugins: add API to return a name for a IO device
2d8b57c hw/virtio/pci: include vdev name in registered PCI sections
c275a76 cputlb: ensure we re-fill the TLB if it has reset
2ce5135 tests/plugin: correctly honour io_count
fbdf670 scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header
42a59cf qemu-plugin.h: add missing include <stddef.h> to define size_t
e3dc316 plugins: new lockstep plugin for debugging TCG changes

=== OUTPUT BEGIN ===
1/9 Checking commit e3dc3166ed69 (plugins: new lockstep plugin for debugging TCG changes)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#55: 
new file mode 100644

total: 0 errors, 1 warnings, 355 lines checked

Patch 1/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/9 Checking commit 42a59cf04068 (qemu-plugin.h: add missing include <stddef.h> to define size_t)
3/9 Checking commit fbdf67007614 (scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header)
4/9 Checking commit 2ce513535a41 (tests/plugin: correctly honour io_count)
5/9 Checking commit c275a76fea13 (cputlb: ensure we re-fill the TLB if it has reset)
6/9 Checking commit 2d8b57ced74c (hw/virtio/pci: include vdev name in registered PCI sections)
WARNING: line over 80 characters
#23: FILE: hw/virtio/virtio-pci.c:1393:
+static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy, const char *vdev_name)

total: 0 errors, 1 warnings, 63 lines checked

Patch 6/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/9 Checking commit fb3988932bba (plugins: add API to return a name for a IO device)
ERROR: "foo * bar" should be "foo *bar"
#23: FILE: include/qemu/qemu-plugin.h:341:
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);

ERROR: "foo * bar" should be "foo *bar"
#36: FILE: plugins/api.c:306:
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)

WARNING: line over 80 characters
#42: FILE: plugins/api.c:312:
+            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);

total: 2 errors, 1 warnings, 35 lines checked

Patch 7/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

8/9 Checking commit a829a1ca4c20 (plugins: new hwprofile plugin)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#30: 
new file mode 100644

ERROR: line over 90 characters
#141: FILE: tests/plugin/hwprofile.c:107:
+                g_string_append_printf(report, "%s @ 0x%"PRIx64"\n", rec->name, rec->base);

WARNING: line over 80 characters
#144: FILE: tests/plugin/hwprofile.c:110:
+                    g_string_append_printf(report, "  off:%08"PRIx64, loc->offset);

ERROR: suspect code indent for conditional statements (20, 23)
#149: FILE: tests/plugin/hwprofile.c:115:
+                    if (track_writes()) {
+                       g_string_append_printf(report, ", 0x%04x, %"PRId64,

ERROR: space required after that ',' (ctx:VxV)
#153: FILE: tests/plugin/hwprofile.c:119:
+                    g_string_append_c(report,'\n');
                                             ^

ERROR: "foo * bar" should be "foo *bar"
#177: FILE: tests/plugin/hwprofile.c:143:
+static DeviceCounts * new_count(char *name, uint64_t base)

ERROR: "foo * bar" should be "foo *bar"
#189: FILE: tests/plugin/hwprofile.c:155:
+static IOLocationCounts * new_location(uint64_t offset)

ERROR: line over 90 characters
#219: FILE: tests/plugin/hwprofile.c:185:
+            IOLocationCounts *io_count = g_hash_table_lookup(counts->access_pattern, &off);

total: 6 errors, 2 warnings, 255 lines checked

Patch 8/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

9/9 Checking commit 5e3c9f6b37c9 (.travis.yml: allow failure for unreliable hosts)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200602154624.4460-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com