| Message ID | 20190731160719.11396-1-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | plugins for TCG | expand |
Patchew URL: https://patchew.org/QEMU/20190731160719.11396-1-alex.bennee@linaro.org/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH v4 00/54] plugins for TCG
Message-id: 20190731160719.11396-1-alex.bennee@linaro.org
=== 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
- [tag update] patchew/20190731075652.17053-1-thuth@redhat.com -> patchew/20190731075652.17053-1-thuth@redhat.com
* [new tag] patchew/20190731160719.11396-1-alex.bennee@linaro.org -> patchew/20190731160719.11396-1-alex.bennee@linaro.org
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out 'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3'
Cloning into 'roms/edk2'...
Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
Cloning into 'boringssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
Cloning into 'krb5'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
Cloning into 'pyca-cryptography'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/opensbi'...
Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'slirp'...
Submodule path 'slirp': checked out 'f0da6726207b740f6101028b2992f918477a4b08'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
4618434 include/exec/cpu-defs.h: fix typo
d260302 include/exec: wrap cpu_ldst.h in CONFIG_TCG
4a08e81 accel/stubs: reduce headers from tcg-stub
dc047a8 tests/plugin: add hotpages plugin to breakdown memory access patterns
0031177 tests/plugin: add instruction execution breakdown
bd83073 plugin: add qemu_plugin_insn_disas helper
719f27a tests/plugin: add a hotblocks plugin
0c2193f tests/tcg: enable plugin testing
4fda032 tests/plugin: add sample plugins
fd7f586 linux-user: support -plugin option
b484c3f vl: support -plugin option
720fb2d plugin: add API symbols to qemu-plugins.symbols
75048c5 translator: inject instrumentation from plugins
90d92af target/openrisc: fetch code with translator_ld
bf3c443 target/xtensa: fetch code with translator_ld
1b3b92b target/sparc: fetch code with translator_ld
f41eb29 target/riscv: fetch code with translator_ld
35e338c target/alpha: fetch code with translator_ld
9014aab target/m68k: fetch code with translator_ld
604625d target/hppa: fetch code with translator_ld
f86b279 target/i386: fetch code with translator_ld
5bfcc20 target/sh4: fetch code with translator_ld
65656f9 target/ppc: fetch code with translator_ld
1338aac target/arm: fetch code with translator_ld
6ad7980 translator: add translator_ld{ub, sw, uw, l, q}
123a7b0 plugin-gen: add plugin_insn_append
2fc0fb1 cpu: hook plugin vcpu events
2ad986c *-user: plugin syscalls
12cf148 *-user: notify plugin of exit
5ef3c3d translate-all: notify plugin code of tb_flush
ceffbf5 plugins: implement helpers for resolving hwaddr
bfd0bef tcg: let plugins instrument virtual memory accesses
8c70962 atomic_template: add inline trace/plugin helpers
b6d0321 atomic_template: fix indentation in GEN_ATOMIC_HELPER
59e11a6 plugin-gen: add module for TCG-related code
b7ff8fb tcg: add tcg_gen_st_ptr
57dce44 cputlb: introduce get_page_addr_code_hostp
c21fd8d cputlb: document get_page_addr_code
0156ea7 queue: add QTAILQ_REMOVE_SEVERAL
98bb420 plugin: add implementation of the api
90dff04 plugin: add core code
dfc8157 plugin: add user-facing API
c508aec configure: add --enable-plugins (MOVE TO END)
efd9ca8 docs/devel: add plugins.rst design document
7e445c2 translate-all: use cpu_in_exclusive_work_context() in tb_flush
e3927dd cpu: introduce cpu_in_exclusive_context()
2c1c22b tcg/README: fix typo s/afterwise/afterwards/
0082b05 trace: add mmu_index to mem_info
43a9cd6 trace: expand mem_info:size_shift to 4 bits
92f8098 includes: remove stale [smp|max]_cpus externs
b12d00f target/arm: remove run time semihosting checks
3964cfe target/arm: handle A-profile A32 semihosting at translate time
354ccbf target/arm: handle A-profile T32 semihosting at translate time
c736511 target/arm: handle M-profile semihosting at translate time
=== OUTPUT BEGIN ===
1/54 Checking commit c736511afb73 (target/arm: handle M-profile semihosting at translate time)
2/54 Checking commit 354ccbf7836a (target/arm: handle A-profile T32 semihosting at translate time)
3/54 Checking commit 3964cfed151e (target/arm: handle A-profile A32 semihosting at translate time)
4/54 Checking commit b12d00f866ff (target/arm: remove run time semihosting checks)
5/54 Checking commit 92f80985c6b7 (includes: remove stale [smp|max]_cpus externs)
6/54 Checking commit 43a9cd64afe9 (trace: expand mem_info:size_shift to 4 bits)
7/54 Checking commit 0082b05e9d82 (trace: add mmu_index to mem_info)
ERROR: line over 90 characters
#24: FILE: accel/tcg/atomic_template.h:63:
+ uint16_t info = glue(trace_mem_build_info_no_se, MEND)(SHIFT, false, ATOMIC_MMU_IDX); \
ERROR: line over 90 characters
#33: FILE: accel/tcg/atomic_template.h:71:
+ uint16_t info = glue(trace_mem_build_info_no_se, MEND)(SHIFT, false, ATOMIC_MMU_IDX); \
ERROR: line over 90 characters
#40: FILE: accel/tcg/atomic_template.h:77:
+ uint16_t info = glue(trace_mem_build_info_no_se, MEND)(SHIFT, true, ATOMIC_MMU_IDX); \
WARNING: line over 80 characters
#321: FILE: trace/mem.h:21:
+static uint16_t trace_mem_get_info(TCGMemOp op, unsigned int mmu_idx, bool store);
total: 3 errors, 1 warnings, 261 lines checked
Patch 7/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/54 Checking commit 2c1c22bc16e9 (tcg/README: fix typo s/afterwise/afterwards/)
9/54 Checking commit e3927dda7720 (cpu: introduce cpu_in_exclusive_context())
10/54 Checking commit 7e445c2738b4 (translate-all: use cpu_in_exclusive_work_context() in tb_flush)
11/54 Checking commit efd9ca87d2ad (docs/devel: add plugins.rst design document)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#24:
new file mode 100644
total: 0 errors, 1 warnings, 111 lines checked
Patch 11/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/54 Checking commit c508aec11118 (configure: add --enable-plugins (MOVE TO END))
13/54 Checking commit dfc81576c19a (plugin: add user-facing API)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15:
new file mode 100644
WARNING: architecture specific defines should be avoided
#41: FILE: include/qemu/qemu-plugin.h:22:
+#if defined _WIN32 || defined __CYGWIN__
WARNING: architecture specific defines should be avoided
#49: FILE: include/qemu/qemu-plugin.h:30:
+ #if __GNUC__ >= 4
total: 0 errors, 3 warnings, 351 lines checked
Patch 13/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
14/54 Checking commit 90dff041feaf (plugin: add core code)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#54:
new file mode 100644
ERROR: "foo * bar" should be "foo *bar"
#190: FILE: include/qemu/plugin.h:132:
+static inline struct qemu_plugin_insn * qemu_plugin_insn_alloc(void)
ERROR: named QLIST_HEAD should be typedefed separately
#399: FILE: plugins/core.c:37:
+QLIST_HEAD(qemu_plugin_cb_head, qemu_plugin_cb);
WARNING: line over 80 characters
#618: FILE: plugins/core.c:256:
+ cbs = g_array_sized_new(false, false, sizeof(struct qemu_plugin_dyn_cb), 1);
WARNING: Block comments use a leading /* on a separate line
#909: FILE: plugins/loader.c:42:
+ { /* end of list */ }
ERROR: externs should be avoided in .c files
#915: FILE: plugins/loader.c:48:
+extern struct qemu_plugin_state plugin;
total: 3 errors, 3 warnings, 1261 lines checked
Patch 14/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
15/54 Checking commit 98bb4205f8d1 (plugin: add implementation of the api)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#22:
new file mode 100644
ERROR: if this code is redundant consider removing it
#282: FILE: plugins/api.c:256:
+#if 0 /* XXX FIXME should be SOFTMMU */
ERROR: "foo * bar" should be "foo *bar"
#303: FILE: plugins/api.c:277:
+static MachineState * get_ms(void)
total: 2 errors, 1 warnings, 303 lines checked
Patch 15/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
16/54 Checking commit 0156ea779e67 (queue: add QTAILQ_REMOVE_SEVERAL)
WARNING: Block comments use a leading /* on a separate line
#31: FILE: include/qemu/queue.h:433:
+ } while (/*CONSTCOND*/0)
total: 0 errors, 1 warnings, 16 lines checked
Patch 16/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
17/54 Checking commit c21fd8dfb8e0 (cputlb: document get_page_addr_code)
18/54 Checking commit 57dce445a292 (cputlb: introduce get_page_addr_code_hostp)
19/54 Checking commit b7ff8fb737a6 (tcg: add tcg_gen_st_ptr)
20/54 Checking commit 59e11a6c6781 (plugin-gen: add module for TCG-related code)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#48:
new file mode 100644
ERROR: if this code is redundant consider removing it
#420: FILE: accel/tcg/plugin-gen.c:368:
+#if 0 /* XXX: no longer needed? */
total: 1 errors, 1 warnings, 1168 lines checked
Patch 20/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
21/54 Checking commit b6d032138a56 (atomic_template: fix indentation in GEN_ATOMIC_HELPER)
22/54 Checking commit 8c70962547d5 (atomic_template: add inline trace/plugin helpers)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15:
new file mode 100644
total: 0 errors, 1 warnings, 296 lines checked
Patch 22/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
23/54 Checking commit bfd0bef6abd9 (tcg: let plugins instrument virtual memory accesses)
WARNING: line over 80 characters
#171: FILE: include/exec/cpu_ldst_template.h:91:
+ uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false, mmu_idx);
total: 0 errors, 1 warnings, 372 lines checked
Patch 23/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
24/54 Checking commit ceffbf57661b (plugins: implement helpers for resolving hwaddr)
25/54 Checking commit 5ef3c3d45dd0 (translate-all: notify plugin code of tb_flush)
26/54 Checking commit 12cf1480ae24 (*-user: notify plugin of exit)
27/54 Checking commit 2ad986cc69bd (*-user: plugin syscalls)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#93:
new file mode 100644
total: 0 errors, 1 warnings, 127 lines checked
Patch 27/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
28/54 Checking commit 2fc0fb11c7c5 (cpu: hook plugin vcpu events)
29/54 Checking commit 123a7b0c3078 (plugin-gen: add plugin_insn_append)
30/54 Checking commit 6ad7980f6247 (translator: add translator_ld{ub, sw, uw, l, q})
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#76: FILE: include/exec/translator.h:161:
+#define DO_LOAD(type, name, shift) \
+ set_helper_retaddr(1); \
+ ret = name ## _p(g2h(pc)); \
+ clear_helper_retaddr();
WARNING: Block comments use a leading /* on a separate line
#109: FILE: include/exec/translator.h:194:
+GEN_TRANSLATOR_LD(translator_ldub, ldb, uint8_t, 1, /* no swap needed */)
total: 1 errors, 1 warnings, 116 lines checked
Patch 30/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
31/54 Checking commit 1338aac82722 (target/arm: fetch code with translator_ld)
32/54 Checking commit 65656f9c3eea (target/ppc: fetch code with translator_ld)
33/54 Checking commit 5bfcc20d11c6 (target/sh4: fetch code with translator_ld)
34/54 Checking commit f86b27916216 (target/i386: fetch code with translator_ld)
35/54 Checking commit 604625d4780e (target/hppa: fetch code with translator_ld)
36/54 Checking commit 9014aab24680 (target/m68k: fetch code with translator_ld)
37/54 Checking commit 35e338c60b8f (target/alpha: fetch code with translator_ld)
38/54 Checking commit f41eb298b626 (target/riscv: fetch code with translator_ld)
39/54 Checking commit 1b3b92b10053 (target/sparc: fetch code with translator_ld)
40/54 Checking commit bf3c443ec239 (target/xtensa: fetch code with translator_ld)
41/54 Checking commit 90d92af84114 (target/openrisc: fetch code with translator_ld)
42/54 Checking commit 75048c56cd83 (translator: inject instrumentation from plugins)
43/54 Checking commit 720fb2d03f34 (plugin: add API symbols to qemu-plugins.symbols)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#120:
new file mode 100644
total: 0 errors, 1 warnings, 151 lines checked
Patch 43/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
44/54 Checking commit b484c3fd133c (vl: support -plugin option)
45/54 Checking commit fd7f58630fad (linux-user: support -plugin option)
46/54 Checking commit 4fda03223154 (tests/plugin: add sample plugins)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#36:
new file mode 100644
total: 0 errors, 1 warnings, 296 lines checked
Patch 46/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
47/54 Checking commit 0c2193fd7aa2 (tests/tcg: enable plugin testing)
48/54 Checking commit 719f27a8e85d (tests/plugin: add a hotblocks plugin)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#32:
new file mode 100644
total: 0 errors, 1 warnings, 153 lines checked
Patch 48/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
49/54 Checking commit bd8307360460 (plugin: add qemu_plugin_insn_disas helper)
ERROR: "foo * bar" should be "foo *bar"
#74: FILE: disas.c:530:
+char * plugin_disas(CPUState *cpu, uint64_t addr, size_t size)
ERROR: "foo * bar" should be "foo *bar"
#136: FILE: include/disas/disas.h:16:
+char * plugin_disas(CPUState *cpu, uint64_t addr, size_t size);
ERROR: "foo * bar" should be "foo *bar"
#155: FILE: include/qemu/qemu-plugin.h:334:
+char * qemu_plugin_insn_disas(const struct qemu_plugin_insn *insn);
total: 3 errors, 0 warnings, 158 lines checked
Patch 49/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
50/54 Checking commit 0031177b24bf (tests/plugin: add instruction execution breakdown)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26:
new file mode 100644
ERROR: space required after that ',' (ctx:VxV)
#99: FILE: tests/plugin/howvec.c:69:
+ { " Add/Sub (imm,tags)","asit", 0x1f800000, 0x11800000, COUNT_CLASS},
^
ERROR: space required after that ',' (ctx:VxV)
#122: FILE: tests/plugin/howvec.c:92:
+ { " AdvSimd ldstmult++","advlsmp",0xbfb00000, 0x0c800000, COUNT_CLASS},
^
ERROR: space required after that ',' (ctx:VxV)
#122: FILE: tests/plugin/howvec.c:92:
+ { " AdvSimd ldstmult++","advlsmp",0xbfb00000, 0x0c800000, COUNT_CLASS},
^
ERROR: space required after that ',' (ctx:VxV)
#124: FILE: tests/plugin/howvec.c:94:
+ { " AdvSimd ldst++", "advlssp",0xbf800000, 0x0d800000, COUNT_CLASS},
^
ERROR: space required after that ',' (ctx:VxV)
#128: FILE: tests/plugin/howvec.c:98:
+ { " ldst noalloc pair", "ldstnap",0x3b800000, 0x28000000, COUNT_CLASS},
^
ERROR: space required after that ',' (ctx:VxV)
#132: FILE: tests/plugin/howvec.c:102:
+ { " ldst reg (reg off)","ldstro", 0x3b200b00, 0x38200800, COUNT_CLASS},
^
WARNING: line over 80 characters
#167: FILE: tests/plugin/howvec.c:137:
+ g_string_append_printf(report, "Class: %-24s\tcounted individually\n",
ERROR: space required after that ',' (ctx:VxV)
#183: FILE: tests/plugin/howvec.c:153:
+ g_string_append_printf(report,"Individual Instructions:\n");
^
WARNING: line over 80 characters
#189: FILE: tests/plugin/howvec.c:159:
+ g_string_append_printf(report, "Instr: %-24s\t(%ld hits)\t(op=%#08x/%s)\n",
ERROR: "foo * bar" should be "foo *bar"
#214: FILE: tests/plugin/howvec.c:184:
+static uint64_t * find_counter(struct qemu_plugin_insn *insn)
WARNING: line over 80 characters
#246: FILE: tests/plugin/howvec.c:216:
+ GUINT_TO_POINTER(opcode));
total: 8 errors, 4 warnings, 308 lines checked
Patch 50/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
51/54 Checking commit dc047a82f004 (tests/plugin: add hotpages plugin to breakdown memory access patterns)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26:
new file mode 100644
total: 0 errors, 1 warnings, 186 lines checked
Patch 51/54 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
52/54 Checking commit 4a08e81629be (accel/stubs: reduce headers from tcg-stub)
53/54 Checking commit d260302d62f5 (include/exec: wrap cpu_ldst.h in CONFIG_TCG)
54/54 Checking commit 4618434e2366 (include/exec/cpu-defs.h: fix typo)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190731160719.11396-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
Alex Bennée <alex.bennee@linaro.org> writes: > Hi, > > This is the latest iteration of the plugins series. The main changes > from the last version are: > > - dropped passing of haddr to plugins > > This makes the code for handling the plugins less invasive in the > softmmu path at the cost of offloading processing to the plugin if it > wants the value. We rely on the fact that the TLB is per vCPU so a > helper can just trigger a re-query of the TLB to get the final > address. > > Part of that change involved embedding the MMU index in the meminfo > field for tracing. I see there are some other patches on the list for > messing with TCGMemOp so there might be a clash coming up. > > - translator_ld goes direct to softmmu/user functions > > I also mark the [SOFTMMU_]CODE_ACCESS helpers as deprecated. There is > more work to be done to clean up all the current uses of code access > helpers but ideally the only thing that should be peaking at code is > the translator loop itself. However a bunch of helpers have taken to > using code loading functions to peak at the instruction just executed > to figure out what to do. Once those have been fixed then we can > remove those helpers. > > Other more minor fixes can be found documented bellow the --- in the > individual patches. > > This series also includes the semihosting patches as they are a > pre-requisite for the translator_ld patches for ARM. > > Once the tree opens up for development again I hope to get the > semihosting and trivial clean-up patches merged quickly so the patch > count for the plugins patches proper can be reduced a bit. Next time, please explain briefly what TCG plugins are about right in your cover letter. I had to go hunting for this. Found "[PATCH v4 11/54] docs/devel: add plugins.rst design document". Please advise why TCG plugins don't undermine the GPL. Any proposal to add a plugin interface needs to do that.
Patchew URL: https://patchew.org/QEMU/20190731160719.11396-1-alex.bennee@linaro.org/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
CC x86_64-softmmu/cpus.o
CC x86_64-softmmu/gdbstub.o
CC x86_64-softmmu/balloon.o
/tmp/qemu-test/src/disas.c:503:5: error: use of undeclared identifier 'csh'
csh handle;
^
/tmp/qemu-test/src/disas.c:504:5: error: use of undeclared identifier 'cs_insn'
cs_insn *insn;
^
/tmp/qemu-test/src/disas.c:504:14: error: use of undeclared identifier 'insn'; did you mean 'info'?
cs_insn *insn;
^~~~
info
/tmp/qemu-test/src/disas.c:500:46: note: 'info' declared here
bool plugin_cap_disas_insn(disassemble_info *info, uint64_t pc, size_t size)
^
/tmp/qemu-test/src/disas.c:509:9: error: implicit declaration of function 'cap_disas_start' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
if (cap_disas_start(info, &handle) != CS_ERR_OK) {
^
/tmp/qemu-test/src/disas.c:509:9: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/tmp/qemu-test/src/disas.c:509:32: error: use of undeclared identifier 'handle'
if (cap_disas_start(info, &handle) != CS_ERR_OK) {
^
/tmp/qemu-test/src/disas.c:509:43: error: use of undeclared identifier 'CS_ERR_OK'
if (cap_disas_start(info, &handle) != CS_ERR_OK) {
^
/tmp/qemu-test/src/disas.c:512:5: error: use of undeclared identifier 'insn'
insn = cap_insn;
^
/tmp/qemu-test/src/disas.c:512:12: error: use of undeclared identifier 'cap_insn'
insn = cap_insn;
^
/tmp/qemu-test/src/disas.c:518:13: error: implicit declaration of function 'cs_disasm' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
count = cs_disasm(handle, cbuf, size, 0, 1, &insn);
^
/tmp/qemu-test/src/disas.c:518:13: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/tmp/qemu-test/src/disas.c:518:23: error: use of undeclared identifier 'handle'
count = cs_disasm(handle, cbuf, size, 0, 1, &insn);
^
/tmp/qemu-test/src/disas.c:518:50: error: use of undeclared identifier 'insn'
count = cs_disasm(handle, cbuf, size, 0, 1, &insn);
^
/tmp/qemu-test/src/disas.c:521:37: error: use of undeclared identifier 'insn'
g_string_printf(s, "%s %s", insn->mnemonic, insn->op_str);
^
/tmp/qemu-test/src/disas.c:521:53: error: use of undeclared identifier 'insn'
g_string_printf(s, "%s %s", insn->mnemonic, insn->op_str);
^
/tmp/qemu-test/src/disas.c:526:5: error: implicit declaration of function 'cs_close' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
cs_close(&handle);
^
/tmp/qemu-test/src/disas.c:526:5: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/tmp/qemu-test/src/disas.c:526:15: error: use of undeclared identifier 'handle'
cs_close(&handle);
^
18 errors generated.
The full log is available at
http://patchew.org/logs/20190731160719.11396-1-alex.bennee@linaro.org/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Markus Armbruster <armbru@redhat.com> writes: > Alex Bennée <alex.bennee@linaro.org> writes: > >> Hi, >> >> This is the latest iteration of the plugins series. The main changes >> from the last version are: >> >> - dropped passing of haddr to plugins >> >> This makes the code for handling the plugins less invasive in the >> softmmu path at the cost of offloading processing to the plugin if it >> wants the value. We rely on the fact that the TLB is per vCPU so a >> helper can just trigger a re-query of the TLB to get the final >> address. >> >> Part of that change involved embedding the MMU index in the meminfo >> field for tracing. I see there are some other patches on the list for >> messing with TCGMemOp so there might be a clash coming up. >> >> - translator_ld goes direct to softmmu/user functions >> >> I also mark the [SOFTMMU_]CODE_ACCESS helpers as deprecated. There is >> more work to be done to clean up all the current uses of code access >> helpers but ideally the only thing that should be peaking at code is >> the translator loop itself. However a bunch of helpers have taken to >> using code loading functions to peak at the instruction just executed >> to figure out what to do. Once those have been fixed then we can >> remove those helpers. >> >> Other more minor fixes can be found documented bellow the --- in the >> individual patches. >> >> This series also includes the semihosting patches as they are a >> pre-requisite for the translator_ld patches for ARM. >> >> Once the tree opens up for development again I hope to get the >> semihosting and trivial clean-up patches merged quickly so the patch >> count for the plugins patches proper can be reduced a bit. > > Next time, please explain briefly what TCG plugins are about right in > your cover letter. I had to go hunting for this. Found "[PATCH v4 > 11/54] docs/devel: add plugins.rst design document". I'll provide a better overview in my next cover letter. > Please advise why TCG plugins don't undermine the GPL. Any proposal to > add a plugin interface needs to do that. I'm not sure what we can say about this apart from "ask your lawyer". I'm certainly not proposing we add any sort of language about what should and shouldn't be allowed to use the plugin interface. I find it hard to see how anyone could argue code written to interface with the plugin API couldn't be considered a derived work. There are two use cases I have in mind: The first is FLOSS developers writing interesting tools that can take advantage of QEMU's control of the system to do experiments that are tricky with other setups (Valgrind is limited to same-arch, Dynamo/Pin are user-space only). I want these experiments to be easy to do without having to keep hacking and re-hacking QEMU's core code. I would hope QEMU developers would up-stream theirs into the QEMU source tree but I can imagine academics will have open source code that will only ever sit in their paper's repository. The other is users who currently maintain hacked up internal copies of QEMU as a test bed for whatever piece of silicon they are brewing behind closed doors. This code would never be distributed (hence never be a GPL issue) and is generally kept private because it's IP sensitive (e.g: experimenting with different cache models). If we can provide an interface that allows them to keep their experiments private and separate from changes to the core code then maybe apart from making their lives a bit easier we will see some non-IP sensitive contributions come back to the upstream. I live in hope ;-) -- Alex Bennée
On Sep 06 20:52, Alex Bennée wrote: > > Markus Armbruster <armbru@redhat.com> writes: > > Please advise why TCG plugins don't undermine the GPL. Any proposal to > > add a plugin interface needs to do that. > > I'm not sure what we can say about this apart from "ask your lawyer". > I'm certainly not proposing we add any sort of language about what > should and shouldn't be allowed to use the plugin interface. I find it > hard to see how anyone could argue code written to interface with the > plugin API couldn't be considered a derived work. I am not a lawyer, but I would not have expected software merely using a well-defined API to be considered a derivative work of the software defining it. Unless, of course, it is a derivative work of another plugin using the same interface in a way that is not necessitated by the structure of the API. What's your reasoning for why it would be a derivative work? Is your belief that the plugin API is complex enough that anything using it has to be a derivative work, or something else? That said, I'm not sure I understand in what way adding a plugin interface would undermine the GPL, so maybe I'm missing the point. -Aaron
On Fri, 6 Sep 2019 at 20:53, Alex Bennée <alex.bennee@linaro.org> wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > Please advise why TCG plugins don't undermine the GPL. Any proposal to > > add a plugin interface needs to do that. > > I'm not sure what we can say about this apart from "ask your lawyer". I suspect the underlying confusion here is "the term 'plugin' has a very wide scope and you and Markus are probably not on the same page about what this API/ABI is actually doing". Specifically, this is not the 'plugin ABI that lets you write device models out of tree' that is sometimes mooted, and the ABI does not (unless it's changed since I last looked) expose arbitrary bits of QEMU internals to the plugin or let plugins make calls to random QEMU functions. It's a very limited scope ABI that allows QEMU users to write code that can do useful introspection on what running guest code is doing without having to modify QEMU itself in non-sustainable non-upstreamable ways. [If it doesn't have that kind of protection against misuse then we should add it.] More generally, your cover letter really needs to be much more descriptive about what we're trying to do here, and what the purpose, limitations, etc are here. If you say "plugins" without giving any detail then you're going to trigger a lot of (reasonable) reactions from people who associate that word with a much more generalized "provide arbitrary bits of QEMU functionality as 3rd-party blobs" concept... thanks -- PMM
Aaron Lindsay OS <aaron@os.amperecomputing.com> writes: > On Sep 06 20:52, Alex Bennée wrote: >> >> Markus Armbruster <armbru@redhat.com> writes: >> > Please advise why TCG plugins don't undermine the GPL. Any proposal to >> > add a plugin interface needs to do that. >> >> I'm not sure what we can say about this apart from "ask your lawyer". >> I'm certainly not proposing we add any sort of language about what >> should and shouldn't be allowed to use the plugin interface. I find it >> hard to see how anyone could argue code written to interface with the >> plugin API couldn't be considered a derived work. > > I am not a lawyer, but I would not have expected software merely using a > well-defined API to be considered a derivative work of the software > defining it. Unless, of course, it is a derivative work of another > plugin using the same interface in a way that is not necessitated by the > structure of the API. > > What's your reasoning for why it would be a derivative work? Is your > belief that the plugin API is complex enough that anything using it has > to be a derivative work, or something else? Well it's derivative if the code couldn't have been written on it's own - so is "derived" from a design in GPL code. The endless arguments about derivation w.r.t the nvidia kernel drivers hinge on the fact the drivers are multi-OS and shimmed to into the kernel - not explicitly written for Linux. However no one really knows where the line is because it's the courts that will ultimately decide. OTOH the kernel has explicit language about the syscall layer: NOTE! This copyright does *not* cover user programs that use kernel services by normal system calls - this is merely considered normal use of the kernel, and does *not* fall under the heading of "derived work". Anyway I don't really care either way and don't intend to become an arm chair lawyer as the arguments can become rather circular and tedious. We might want to make an explicit statement in the QEMU tree about reserving the right to change the API in the future and the best way to keep plugins in sync is to have them in the upstream source tree. > That said, I'm not sure I understand in what way adding a plugin > interface would undermine the GPL, so maybe I'm missing the point. I suspect there is a similar concern as GCC had when they were asked to expose their IR to tools. They were explicitly concerned that people would ship proprietary compilers consisting of a GPL GCC shell with all the clever optimisations in proprietary plugins that glued the front and backends together. I don't think this is a problem for QEMU because at the moment plugins can't be used to improve code generation - just observe it. You could theoretically implement device models this way but there are definitely easier more efficient ways of doing it - as shown by other 3rd party forks of QEMU. As I mentioned in the other message I'm sure there will be proprietary - or rather secret behind closed doors plugins but I don't view them any differently to all the various secret and external forks of the main QEMU source. > > -Aaron -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Markus Armbruster <armbru@redhat.com> writes: [...] >> Please advise why TCG plugins don't undermine the GPL. Any proposal to >> add a plugin interface needs to do that. > > I'm not sure what we can say about this apart from "ask your lawyer". I'm not asking for a legal argument, I'm asking for a pragmatic one. > I'm certainly not proposing we add any sort of language about what > should and shouldn't be allowed to use the plugin interface. I find it > hard to see how anyone could argue code written to interface with the > plugin API couldn't be considered a derived work. What makes that so? Is writing a plugin without linking with QEMU code impractical? > There are two use cases I have in mind: > > The first is FLOSS developers writing interesting tools that can take > advantage of QEMU's control of the system to do experiments that are > tricky with other setups (Valgrind is limited to same-arch, Dynamo/Pin > are user-space only). I want these experiments to be easy to do without > having to keep hacking and re-hacking QEMU's core code. I would hope > QEMU developers would up-stream theirs into the QEMU source tree but I > can imagine academics will have open source code that will only ever sit > in their paper's repository. GPL'ed code that's not for upstream is 100% legitimate. > The other is users who currently maintain hacked up internal copies of > QEMU as a test bed for whatever piece of silicon they are brewing behind > closed doors. This code would never be distributed (hence never be a GPL > issue) Correct. We can't force anybody to distribute, and that's only proper. > and is generally kept private because it's IP sensitive > (e.g: experimenting with different cache models). If we can provide an > interface that allows them to keep their experiments private and > separate from changes to the core code then maybe apart from making > their lives a bit easier we will see some non-IP sensitive contributions > come back to the upstream. I live in hope ;-) I'm concerned about a third case: imlementing stuff as a plugin so you can distribute it with a GPL-incompatible license. Particularly pernicious when that stuff could be useful upstream. Are there any technical difficulties that could make distributing a plugins in binary form impractical?
Markus Armbruster <armbru@redhat.com> writes: > Alex Bennée <alex.bennee@linaro.org> writes: > >> Markus Armbruster <armbru@redhat.com> writes: > [...] >>> Please advise why TCG plugins don't undermine the GPL. Any proposal to >>> add a plugin interface needs to do that. >> >> I'm not sure what we can say about this apart from "ask your lawyer". > > I'm not asking for a legal argument, I'm asking for a pragmatic one. > >> I'm certainly not proposing we add any sort of language about what >> should and shouldn't be allowed to use the plugin interface. I find it >> hard to see how anyone could argue code written to interface with the >> plugin API couldn't be considered a derived work. > > What makes that so? Is writing a plugin without linking with QEMU code > impractical? The way a plugin works is by linking. The plugin itself would be useless unless combined with the QEMU code to do its thing. It is a more intimate binding than an IPC interface using some sort of protocol. The argument goes that pretty much any kernel module is derived code - but again it has never really been litigated in the courts which would be the people to set the precedent. > >> There are two use cases I have in mind: >> >> The first is FLOSS developers writing interesting tools that can take >> advantage of QEMU's control of the system to do experiments that are >> tricky with other setups (Valgrind is limited to same-arch, Dynamo/Pin >> are user-space only). I want these experiments to be easy to do without >> having to keep hacking and re-hacking QEMU's core code. I would hope >> QEMU developers would up-stream theirs into the QEMU source tree but I >> can imagine academics will have open source code that will only ever sit >> in their paper's repository. > > GPL'ed code that's not for upstream is 100% legitimate. > >> The other is users who currently maintain hacked up internal copies of >> QEMU as a test bed for whatever piece of silicon they are brewing behind >> closed doors. This code would never be distributed (hence never be a GPL >> issue) > > Correct. We can't force anybody to distribute, and that's only proper. > >> and is generally kept private because it's IP sensitive >> (e.g: experimenting with different cache models). If we can provide an >> interface that allows them to keep their experiments private and >> separate from changes to the core code then maybe apart from making >> their lives a bit easier we will see some non-IP sensitive contributions >> come back to the upstream. I live in hope ;-) > > I'm concerned about a third case: imlementing stuff as a plugin so you > can distribute it with a GPL-incompatible license. Particularly > pernicious when that stuff could be useful upstream. If someone were to do that it would depend on a copyright holder (i.e. one of us) being willing to challenge that licensing. AIUI GCC used additional language in the runtime exception clause: https://www.gnu.org/licenses/gcc-exception-3.1.html which only allows use of the runtime exception if the code has gone through GPL compatible code: A Compilation Process is "Eligible" if it is done using GCC, alone or with other GPL-compatible software, or if it is done without using any work based on GCC. For example, using non-GPL-compatible Software to optimize any GCC intermediate representations would not qualify as an Eligible Compilation Process. > Are there any technical difficulties that could make distributing a > plugins in binary form impractical? Well the first thing will be we are not intending to offer a guaranteed ABI. While we don't want to be changing it at a whim there shouldn't be an expectation that the plugin interface will maintain backwards compatibility (unlike the command line interface ;-). There should be an expectation that plugins will likely need to be rebuilt against the current source tree from time to time. We could implement a more technical measure analogous to the kernels module signing that would require the plugin to be rebuilt with reference to the current QEMU source tree although that will be a pain even for internally distributed blobs. I'm loathed to implement such a system from v1 though given the problem of publicly distributed binary blobs is currently only a theoretical problem. -- Alex Bennée
On Thu, 12 Sep 2019 at 10:03, Alex Bennée <alex.bennee@linaro.org> wrote: > > Well the first thing will be we are not intending to offer a guaranteed > ABI. While we don't want to be changing it at a whim there shouldn't be > an expectation that the plugin interface will maintain backwards > compatibility (unlike the command line interface ;-). There should be an > expectation that plugins will likely need to be rebuilt against the > current source tree from time to time. Wait, what? From my perspective the whole point of the plugin interface is that it should be stable, in that at least there's a good chance that a plugin you built will work against multiple versions of QEMU, and if it doesn't then it should fail with a reasonable error message telling you to update. I'm not sure we should be landing the plugins infrastructure if we don't have that much stability. thanks -- PMM
On Thu, Sep 12, 2019 at 10:03:48AM +0100, Alex Bennée wrote: > > Markus Armbruster <armbru@redhat.com> writes: > > > Alex Bennée <alex.bennee@linaro.org> writes: > > > >> Markus Armbruster <armbru@redhat.com> writes: > > [...] > >>> Please advise why TCG plugins don't undermine the GPL. Any proposal to > >>> add a plugin interface needs to do that. > >> > >> I'm not sure what we can say about this apart from "ask your lawyer". > > > > I'm not asking for a legal argument, I'm asking for a pragmatic one. > > > >> I'm certainly not proposing we add any sort of language about what > >> should and shouldn't be allowed to use the plugin interface. I find it > >> hard to see how anyone could argue code written to interface with the > >> plugin API couldn't be considered a derived work. > > > > What makes that so? Is writing a plugin without linking with QEMU code > > impractical? > > The way a plugin works is by linking. The plugin itself would be useless > unless combined with the QEMU code to do its thing. It is a more > intimate binding than an IPC interface using some sort of protocol. The > argument goes that pretty much any kernel module is derived code - but > again it has never really been litigated in the courts which would be > the people to set the precedent. Part of the problem with the kernel is the historical precedent they had set. The issue of GPL compliance only become prominent much later after (closed source) loadable modules were already widely in use. They tried to lock the door after the horse had already bolted by adding EXPORT_SYMBOL_GPL. We can avoid this trapped by clearly documenting our license expectations from the very start. ie state that we consider any plugins to be derived works and to be bound by the terms of the GPL. This doesn't mean the plugins themselves have to be GPL, but they have to be under terms that allow relicensing to the GPL, in order to be license compatible. We could even go as far as having the plugin registration API require that the plugin explicitly declare its license and we can log this license at time of loading. If people use a non-GPL compatible license it will be clearly visible as non-compatible, or if they lie and pretend to be GPL then they would be willfully violating. > >> There are two use cases I have in mind: > >> > >> The first is FLOSS developers writing interesting tools that can take > >> advantage of QEMU's control of the system to do experiments that are > >> tricky with other setups (Valgrind is limited to same-arch, Dynamo/Pin > >> are user-space only). I want these experiments to be easy to do without > >> having to keep hacking and re-hacking QEMU's core code. I would hope > >> QEMU developers would up-stream theirs into the QEMU source tree but I > >> can imagine academics will have open source code that will only ever sit > >> in their paper's repository. > > > > GPL'ed code that's not for upstream is 100% legitimate. Yep, the code only has to be provided to the people who receive the plugin binary. Those people can't be prevented from redistributing it further though. > >> The other is users who currently maintain hacked up internal copies of > >> QEMU as a test bed for whatever piece of silicon they are brewing behind > >> closed doors. This code would never be distributed (hence never be a GPL > >> issue) > > > > Correct. We can't force anybody to distribute, and that's only proper. > > > > Are there any technical difficulties that could make distributing a > > plugins in binary form impractical? > > Well the first thing will be we are not intending to offer a guaranteed > ABI. While we don't want to be changing it at a whim there shouldn't be > an expectation that the plugin interface will maintain backwards > compatibility (unlike the command line interface ;-). There should be an > expectation that plugins will likely need to be rebuilt against the > current source tree from time to time. > > We could implement a more technical measure analogous to the kernels > module signing that would require the plugin to be rebuilt with > reference to the current QEMU source tree although that will be a pain > even for internally distributed blobs. I'm loathed to implement such a > system from v1 though given the problem of publicly distributed binary > blobs is currently only a theoretical problem. The problem with waiting for a problem to arise is that you have set a precedent that its ok. We are in a stronger enforcement position if we can set expectations accurately from day one, avoiding the trap the kernel had which needed to try to enforce after the fact. So from that POV, I'd be strongly in favour of technical measures that force the plugin to be rebuilt against each new QEMU version. This doesn't need to be module signing - it could be as simple as requiring the plugin to export a symbol "module_version" which must exactly match the QEMU version, or refuse to load it. If we want to be even more strict we could generate a random hash in each QEMU rebuild, which is the similar level of strictness to kernel signing Regards, Daniel
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 12 Sep 2019 at 10:03, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Well the first thing will be we are not intending to offer a guaranteed >> ABI. While we don't want to be changing it at a whim there shouldn't be >> an expectation that the plugin interface will maintain backwards >> compatibility (unlike the command line interface ;-). There should be an >> expectation that plugins will likely need to be rebuilt against the >> current source tree from time to time. > > Wait, what? From my perspective the whole point of the plugin > interface is that it should be stable, in that at least there's > a good chance that a plugin you built will work against multiple > versions of QEMU, and if it doesn't then it should fail with > a reasonable error message telling you to update. I'm not > sure we should be landing the plugins infrastructure if we > don't have that much stability. There is a big fat blurry line between "set in stone" and "not requiring you to re-engineer the plugin every QEMU release". I'm saying we should reserve the right to extend and change the plugin API as required but the expectation would be the plugins will continue to work the same way but maybe with tweaks to the API hooks to support additional features. It's also a pretty young interface so I would expect some evolution once it is released into the field. One problem with the anti-license circumvention measures being suggested is it will mean having to recompile plugins for any given release. This isn't a problem for plugins we write but it does add an extra step for out of tree plugins. Maybe being forced to re-compile (but not change the source) is a hurdle we are willing to accept? -- Alex Bennée
On Thu, Sep 12, 2019 at 11:07:07AM +0100, Alex Bennée wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Thu, 12 Sep 2019 at 10:03, Alex Bennée <alex.bennee@linaro.org> wrote: > >> > >> Well the first thing will be we are not intending to offer a guaranteed > >> ABI. While we don't want to be changing it at a whim there shouldn't be > >> an expectation that the plugin interface will maintain backwards > >> compatibility (unlike the command line interface ;-). There should be an > >> expectation that plugins will likely need to be rebuilt against the > >> current source tree from time to time. > > > > Wait, what? From my perspective the whole point of the plugin > > interface is that it should be stable, in that at least there's > > a good chance that a plugin you built will work against multiple > > versions of QEMU, and if it doesn't then it should fail with > > a reasonable error message telling you to update. I'm not > > sure we should be landing the plugins infrastructure if we > > don't have that much stability. > > There is a big fat blurry line between "set in stone" and "not requiring > you to re-engineer the plugin every QEMU release". I'm saying we should > reserve the right to extend and change the plugin API as required but > the expectation would be the plugins will continue to work the same way > but maybe with tweaks to the API hooks to support additional features. > > It's also a pretty young interface so I would expect some evolution once > it is released into the field. > > One problem with the anti-license circumvention measures being suggested > is it will mean having to recompile plugins for any given release. This > isn't a problem for plugins we write but it does add an extra step for > out of tree plugins. Maybe being forced to re-compile (but not change > the source) is a hurdle we are willing to accept? I can understand & totally support not wishing to break the compilation of plugins for every release, by having a reasonably stable API. I think forcing recompile for each release is reasonable protection to make it less atttractive to create license violating closed source plugins. Regards, Daniel
On Thu, 12 Sep 2019 at 11:07, Alex Bennée <alex.bennee@linaro.org> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > > Wait, what? From my perspective the whole point of the plugin > > interface is that it should be stable, in that at least there's > > a good chance that a plugin you built will work against multiple > > versions of QEMU, and if it doesn't then it should fail with > > a reasonable error message telling you to update. I'm not > > sure we should be landing the plugins infrastructure if we > > don't have that much stability. > > There is a big fat blurry line between "set in stone" and "not requiring > you to re-engineer the plugin every QEMU release". I'm saying we should > reserve the right to extend and change the plugin API as required but > the expectation would be the plugins will continue to work the same way > but maybe with tweaks to the API hooks to support additional features. > > It's also a pretty young interface so I would expect some evolution once > it is released into the field. Sure. But I think we should document that we at least intend to have some approximation to a compatability/deprecation policy here, and some mechanisms for versioning so you get a helpful error rather than weird misbehaviour if your plugin is too old. > One problem with the anti-license circumvention measures being suggested > is it will mean having to recompile plugins for any given release. Why should we do this? I think this is making life hard for our users for no good reason. We *do* have this check for modules, because a module is just a random .so that can do anything in QEMU. I thought we had the TCG-plugin interface much more locked down than that? thanks -- PMM
On Thu, 12 Sep 2019 at 11:17, Daniel P. Berrangé <berrange@redhat.com> wrote: > I think forcing recompile for each release is reasonable protection > to make it less atttractive to create license violating closed source > plugins. I'm just not sure that a plugin that, for instance, does "whenever the guest makes a memory access print the address and data", is in any reasonable sense a derived work of QEMU that it makes sense to extend the GPL to. We're providing a convenient introspection interface here, similar to but different from the way gdbstub lets you introspect guest binary behaviour, not an arbitrary mechanism for extending QEMU itself. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 12 Sep 2019 at 11:07, Alex Bennée <alex.bennee@linaro.org> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> > Wait, what? From my perspective the whole point of the plugin >> > interface is that it should be stable, in that at least there's >> > a good chance that a plugin you built will work against multiple >> > versions of QEMU, and if it doesn't then it should fail with >> > a reasonable error message telling you to update. I'm not >> > sure we should be landing the plugins infrastructure if we >> > don't have that much stability. >> >> There is a big fat blurry line between "set in stone" and "not requiring >> you to re-engineer the plugin every QEMU release". I'm saying we should >> reserve the right to extend and change the plugin API as required but >> the expectation would be the plugins will continue to work the same way >> but maybe with tweaks to the API hooks to support additional features. >> >> It's also a pretty young interface so I would expect some evolution once >> it is released into the field. > > Sure. But I think we should document that we at least intend to > have some approximation to a compatability/deprecation policy > here, and some mechanisms for versioning so you get a helpful > error rather than weird misbehaviour if your plugin is too old. > >> One problem with the anti-license circumvention measures being suggested >> is it will mean having to recompile plugins for any given release. > > Why should we do this? I think this is making life hard for our > users for no good reason. We *do* have this check for modules, > because a module is just a random .so that can do anything in > QEMU. I thought we had the TCG-plugin interface much more locked > down than that? It is, there are only a few set calls the plugin can make into QEMU, mostly to register callbacks to events. Currently it can examine the state of the system (again through the API) but can't change it's behaviour (although a register access interface has been requested although I'd initially intended to make it read only). > > thanks > -- PMM -- Alex Bennée
Hi, This is the latest iteration of the plugins series. The main changes from the last version are: - dropped passing of haddr to plugins This makes the code for handling the plugins less invasive in the softmmu path at the cost of offloading processing to the plugin if it wants the value. We rely on the fact that the TLB is per vCPU so a helper can just trigger a re-query of the TLB to get the final address. Part of that change involved embedding the MMU index in the meminfo field for tracing. I see there are some other patches on the list for messing with TCGMemOp so there might be a clash coming up. - translator_ld goes direct to softmmu/user functions I also mark the [SOFTMMU_]CODE_ACCESS helpers as deprecated. There is more work to be done to clean up all the current uses of code access helpers but ideally the only thing that should be peaking at code is the translator loop itself. However a bunch of helpers have taken to using code loading functions to peak at the instruction just executed to figure out what to do. Once those have been fixed then we can remove those helpers. Other more minor fixes can be found documented bellow the --- in the individual patches. This series also includes the semihosting patches as they are a pre-requisite for the translator_ld patches for ARM. Once the tree opens up for development again I hope to get the semihosting and trivial clean-up patches merged quickly so the patch count for the plugins patches proper can be reduced a bit. The following patches need review patch 0004/target arm remove run time semihosting checks.patch patch 0005/includes remove stale smp max _cpus externs.patch patch 0007/trace add mmu_index to mem_info.patch patch 0011/docs devel add plugins.rst design document.patch patch 0012/configure add enable plugins MOVE TO END.patch patch 0015/plugin add implementation of the api.patch patch 0024/plugins implement helpers for resolving hwaddr.patch patch 0047/tests tcg enable plugin testing.patch patch 0048/tests plugin add a hotblocks plugin.patch patch 0050/tests plugin add instruction execution breakdown.patch patch 0051/tests plugin add hotpages plugin to breakdown mem.patch patch 0053/include exec wrap cpu_ldst.h in CONFIG_TCG.patch Alex Bennée (18): target/arm: handle M-profile semihosting at translate time target/arm: handle A-profile T32 semihosting at translate time target/arm: handle A-profile A32 semihosting at translate time target/arm: remove run time semihosting checks includes: remove stale [smp|max]_cpus externs trace: add mmu_index to mem_info docs/devel: add plugins.rst design document configure: add --enable-plugins (MOVE TO END) plugin: add implementation of the api plugins: implement helpers for resolving hwaddr tests/tcg: enable plugin testing tests/plugin: add a hotblocks plugin plugin: add qemu_plugin_insn_disas helper tests/plugin: add instruction execution breakdown tests/plugin: add hotpages plugin to breakdown memory access patterns accel/stubs: reduce headers from tcg-stub include/exec: wrap cpu_ldst.h in CONFIG_TCG include/exec/cpu-defs.h: fix typo Emilio G. Cota (34): trace: expand mem_info:size_shift to 4 bits tcg/README: fix typo s/afterwise/afterwards/ cpu: introduce cpu_in_exclusive_context() translate-all: use cpu_in_exclusive_work_context() in tb_flush plugin: add user-facing API plugin: add core code queue: add QTAILQ_REMOVE_SEVERAL cputlb: document get_page_addr_code cputlb: introduce get_page_addr_code_hostp tcg: add tcg_gen_st_ptr plugin-gen: add module for TCG-related code atomic_template: fix indentation in GEN_ATOMIC_HELPER atomic_template: add inline trace/plugin helpers tcg: let plugins instrument virtual memory accesses translate-all: notify plugin code of tb_flush *-user: notify plugin of exit *-user: plugin syscalls cpu: hook plugin vcpu events plugin-gen: add plugin_insn_append translator: add translator_ld{ub,sw,uw,l,q} target/arm: fetch code with translator_ld target/ppc: fetch code with translator_ld target/sh4: fetch code with translator_ld target/i386: fetch code with translator_ld target/hppa: fetch code with translator_ld target/m68k: fetch code with translator_ld target/alpha: fetch code with translator_ld target/riscv: fetch code with translator_ld target/sparc: fetch code with translator_ld target/xtensa: fetch code with translator_ld target/openrisc: fetch code with translator_ld translator: inject instrumentation from plugins plugin: add API symbols to qemu-plugins.symbols tests/plugin: add sample plugins Lluís Vilanova (2): vl: support -plugin option linux-user: support -plugin option Makefile | 10 +- Makefile.target | 2 + accel/stubs/tcg-stub.c | 3 - accel/tcg/Makefile.objs | 1 + accel/tcg/atomic_common.inc.c | 54 ++ accel/tcg/atomic_template.h | 96 ++- accel/tcg/cpu-exec.c | 8 +- accel/tcg/cputlb.c | 69 +- accel/tcg/plugin-gen.c | 977 ++++++++++++++++++++++ accel/tcg/plugin-helpers.h | 5 + accel/tcg/translate-all.c | 15 +- accel/tcg/translator.c | 20 + accel/tcg/user-exec.c | 3 + bsd-user/syscall.c | 24 +- configure | 86 +- cpus-common.c | 4 + cpus.c | 10 + disas.c | 103 +++ docs/devel/index.rst | 1 + docs/devel/plugins.rst | 107 +++ exec.c | 2 + include/disas/disas.h | 2 + include/exec/cpu-defs.h | 3 +- include/exec/cpu_ldst.h | 11 + include/exec/cpu_ldst_template.h | 37 +- include/exec/cpu_ldst_useronly_template.h | 29 +- include/exec/exec-all.h | 81 +- include/exec/helper-gen.h | 1 + include/exec/helper-proto.h | 1 + include/exec/helper-tcg.h | 1 + include/exec/plugin-gen.h | 71 ++ include/exec/translator.h | 58 +- include/qemu/bswap.h | 5 + include/qemu/plugin.h | 261 ++++++ include/qemu/qemu-plugin.h | 360 ++++++++ include/qemu/queue.h | 10 + include/qom/cpu.h | 19 + include/sysemu/sysemu.h | 2 - include/user/syscall-trace.h | 40 + linux-user/exit.c | 1 + linux-user/main.c | 18 + linux-user/syscall.c | 7 +- plugins/.gitignore | 2 + plugins/Makefile.objs | 21 + plugins/api.c | 338 ++++++++ plugins/core.c | 499 +++++++++++ plugins/loader.c | 353 ++++++++ plugins/plugin.h | 95 +++ plugins/qemu-plugins.symbols | 39 + qemu-options.hx | 17 + qom/cpu.c | 2 + scripts/tracetool/transform.py | 1 + target/alpha/translate.c | 2 +- target/arm/arm_ldst.h | 15 +- target/arm/helper.c | 90 +- target/arm/m_helper.c | 18 +- target/arm/translate.c | 64 +- target/hppa/translate.c | 2 +- target/i386/translate.c | 10 +- target/m68k/translate.c | 2 +- target/openrisc/translate.c | 2 +- target/ppc/translate.c | 8 +- target/riscv/translate.c | 2 +- target/sh4/translate.c | 4 +- target/sparc/translate.c | 2 +- target/xtensa/translate.c | 4 +- tcg/README | 2 +- tcg/tcg-op.c | 40 +- tcg/tcg-op.h | 16 + tcg/tcg-opc.h | 3 + tcg/tcg.c | 22 + tcg/tcg.h | 23 + tests/Makefile.include | 10 +- tests/plugin/Makefile | 31 + tests/plugin/bb.c | 65 ++ tests/plugin/empty.c | 29 + tests/plugin/hotblocks.c | 146 ++++ tests/plugin/hotpages.c | 179 ++++ tests/plugin/howvec.c | 301 +++++++ tests/plugin/insn.c | 62 ++ tests/plugin/mem.c | 96 +++ tests/tcg/Makefile | 34 + tests/tcg/arm/Makefile.softmmu-target | 1 + trace-events | 8 +- trace/mem-internal.h | 31 +- trace/mem.h | 7 +- vl.c | 11 + 87 files changed, 5067 insertions(+), 260 deletions(-) create mode 100644 accel/tcg/atomic_common.inc.c create mode 100644 accel/tcg/plugin-gen.c create mode 100644 accel/tcg/plugin-helpers.h create mode 100644 docs/devel/plugins.rst create mode 100644 include/exec/plugin-gen.h create mode 100644 include/qemu/plugin.h create mode 100644 include/qemu/qemu-plugin.h create mode 100644 include/user/syscall-trace.h create mode 100644 plugins/.gitignore create mode 100644 plugins/Makefile.objs create mode 100644 plugins/api.c create mode 100644 plugins/core.c create mode 100644 plugins/loader.c create mode 100644 plugins/plugin.h create mode 100644 plugins/qemu-plugins.symbols create mode 100644 tests/plugin/Makefile create mode 100644 tests/plugin/bb.c create mode 100644 tests/plugin/empty.c create mode 100644 tests/plugin/hotblocks.c create mode 100644 tests/plugin/hotpages.c create mode 100644 tests/plugin/howvec.c create mode 100644 tests/plugin/insn.c create mode 100644 tests/plugin/mem.c