configure | 8 + scripts/qemu-gdb.py | 7 +- scripts/qemugdb/coroutine.py | 114 +++++--------- scripts/qemugdb/coroutine_ucontext.py | 69 +++++++++ scripts/qemugdb/coroutine_x86.py | 21 +++ scripts/qemugdb/mtree.py | 7 +- scripts/qemugdb/tcg.py | 7 +- util/coroutine-x86.c | 212 ++++++++++++++++++++++++++ 8 files changed, 350 insertions(+), 95 deletions(-) create mode 100644 scripts/qemugdb/coroutine_ucontext.py create mode 100644 scripts/qemugdb/coroutine_x86.py create mode 100644 util/coroutine-x86.c
This is in preparation for CET support for x86. I know this is very late, but if possible I'd like to have this in 4.0 as an experimental backend. Paolo Bonzini (3): qemugdb: fix licensing qemugdb: allow adding support for other coroutine backends coroutine: add x86 specific coroutine backend configure | 8 + scripts/qemu-gdb.py | 7 +- scripts/qemugdb/coroutine.py | 114 +++++--------- scripts/qemugdb/coroutine_ucontext.py | 69 +++++++++ scripts/qemugdb/coroutine_x86.py | 21 +++ scripts/qemugdb/mtree.py | 7 +- scripts/qemugdb/tcg.py | 7 +- util/coroutine-x86.c | 212 ++++++++++++++++++++++++++ 8 files changed, 350 insertions(+), 95 deletions(-) create mode 100644 scripts/qemugdb/coroutine_ucontext.py create mode 100644 scripts/qemugdb/coroutine_x86.py create mode 100644 util/coroutine-x86.c -- 2.20.1
Patchew URL: https://patchew.org/QEMU/20190311123507.24867-1-pbonzini@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20190311123507.24867-1-pbonzini@redhat.com
Subject: [Qemu-devel] [PATCH 0/2] coroutine: add x86 specific coroutine backend
=== 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
e2a18635a4..336cfef495 master -> master
* [new tag] patchew/20190311123507.24867-1-pbonzini@redhat.com -> patchew/20190311123507.24867-1-pbonzini@redhat.com
Switched to a new branch 'test'
e241487774 coroutine: add x86 specific coroutine backend
5b3b779548 qemugdb: allow adding support for other coroutine backends
83841cb732 qemugdb: fix licensing
=== OUTPUT BEGIN ===
1/3 Checking commit 83841cb73288 (qemugdb: fix licensing)
2/3 Checking commit 5b3b77954837 (qemugdb: allow adding support for other coroutine backends)
WARNING: line over 80 characters
#147: FILE: scripts/qemugdb/coroutine.py:65:
+ return coroutine_backend().get_coroutine_regs(addr)['rsp'].cast(VOID_PTR)
WARNING: line over 80 characters
#155: FILE: scripts/qemugdb/coroutine.py:72:
+ return coroutine_backend().get_coroutine_regs(addr)['rip'].cast(VOID_PTR)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#157:
new file mode 100644
ERROR: line over 90 characters
#206: FILE: scripts/qemugdb/coroutine_ucontext.py:45:
+ return gdb.parse_and_eval('(((uint64_t)%s >> 0x11) | ((uint64_t)%s << (64 - 0x11))) ^ (uint64_t)%s' % (val, val, pointer_guard))
total: 1 errors, 3 warnings, 205 lines checked
Patch 2/3 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/3 Checking commit e24148777414 (coroutine: add x86 specific coroutine backend)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#56:
new file mode 100644
WARNING: architecture specific defines should be avoided
#121: FILE: util/coroutine-x86.c:34:
+#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
WARNING: line over 80 characters
#169: FILE: util/coroutine-x86.c:82:
+#define CO_SWITCH(from, to, action, jump) ({ \
WARNING: line over 80 characters
#170: FILE: util/coroutine-x86.c:83:
+ int ret = action; \
WARNING: line over 80 characters
#171: FILE: util/coroutine-x86.c:84:
+ void *from_ = from; \
WARNING: line over 80 characters
#172: FILE: util/coroutine-x86.c:85:
+ void *to_ = to; \
WARNING: line over 80 characters
#173: FILE: util/coroutine-x86.c:86:
+ asm volatile( \
WARNING: line over 80 characters
#174: FILE: util/coroutine-x86.c:87:
+ ".cfi_remember_state\n" \
WARNING: line over 80 characters
#175: FILE: util/coroutine-x86.c:88:
+ "pushq %%rbp\n" /* save scratch register on source stack */ \
WARNING: Block comments use a leading /* on a separate line
#175: FILE: util/coroutine-x86.c:88:
+ "pushq %%rbp\n" /* save scratch register on source stack */ \
WARNING: line over 80 characters
#176: FILE: util/coroutine-x86.c:89:
+ ".cfi_adjust_cfa_offset 8\n" \
WARNING: line over 80 characters
#177: FILE: util/coroutine-x86.c:90:
+ ".cfi_rel_offset %%rbp, 0\n" \
WARNING: line over 80 characters
#178: FILE: util/coroutine-x86.c:91:
+ "call 1f\n" /* switch continues at label 1 */ \
WARNING: Block comments use a leading /* on a separate line
#178: FILE: util/coroutine-x86.c:91:
+ "call 1f\n" /* switch continues at label 1 */ \
WARNING: line over 80 characters
#179: FILE: util/coroutine-x86.c:92:
+ ".cfi_adjust_cfa_offset 8\n" \
WARNING: line over 80 characters
#180: FILE: util/coroutine-x86.c:93:
+ "jmp 2f\n" /* switch back continues at label 2 */ \
WARNING: Block comments use a leading /* on a separate line
#180: FILE: util/coroutine-x86.c:93:
+ "jmp 2f\n" /* switch back continues at label 2 */ \
WARNING: line over 80 characters
#181: FILE: util/coroutine-x86.c:94:
+ "1: movq (%%rsp), %%rbp\n" /* save source IP for debugging */ \
WARNING: Block comments use a leading /* on a separate line
#181: FILE: util/coroutine-x86.c:94:
+ "1: movq (%%rsp), %%rbp\n" /* save source IP for debugging */ \
WARNING: line over 80 characters
#182: FILE: util/coroutine-x86.c:95:
+ "movq %%rsp, %c[sp](%[FROM])\n" /* save source SP */ \
WARNING: Block comments use a leading /* on a separate line
#182: FILE: util/coroutine-x86.c:95:
+ "movq %%rsp, %c[sp](%[FROM])\n" /* save source SP */ \
WARNING: line over 80 characters
#183: FILE: util/coroutine-x86.c:96:
+ "movq %c[sp](%[TO]), %%rsp\n" /* load destination SP */ \
WARNING: Block comments use a leading /* on a separate line
#183: FILE: util/coroutine-x86.c:96:
+ "movq %c[sp](%[TO]), %%rsp\n" /* load destination SP */ \
WARNING: line over 80 characters
#184: FILE: util/coroutine-x86.c:97:
+ jump "\n" /* coroutine switch */ \
WARNING: Block comments use a leading /* on a separate line
#184: FILE: util/coroutine-x86.c:97:
+ jump "\n" /* coroutine switch */ \
WARNING: line over 80 characters
#185: FILE: util/coroutine-x86.c:98:
+ "2:" \
WARNING: line over 80 characters
#186: FILE: util/coroutine-x86.c:99:
+ ".cfi_adjust_cfa_offset -8\n" \
WARNING: line over 80 characters
#187: FILE: util/coroutine-x86.c:100:
+ "popq %%rbp\n" \
WARNING: line over 80 characters
#188: FILE: util/coroutine-x86.c:101:
+ ".cfi_adjust_cfa_offset -8\n" \
WARNING: line over 80 characters
#189: FILE: util/coroutine-x86.c:102:
+ ".cfi_restore_state\n" \
WARNING: line over 80 characters
#190: FILE: util/coroutine-x86.c:103:
+ : "+a" (ret), [FROM] "+b" (from_), [TO] "+D" (to_) \
WARNING: line over 80 characters
#191: FILE: util/coroutine-x86.c:104:
+ : [sp] "i" (offsetof(CoroutineX86, sp)) \
WARNING: line over 80 characters
#192: FILE: util/coroutine-x86.c:105:
+ : "rcx", "rdx", "rsi", "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15", \
WARNING: line over 80 characters
#193: FILE: util/coroutine-x86.c:106:
+ "memory"); \
WARNING: line over 80 characters
#225: FILE: util/coroutine-x86.c:138:
+ /* Immediately enter the coroutine once to pass it its address as the argument */
WARNING: architecture specific defines should be avoided
#236: FILE: util/coroutine-x86.c:149:
+#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
WARNING: architecture specific defines should be avoided
#245: FILE: util/coroutine-x86.c:158:
+#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
total: 0 errors, 37 warnings, 265 lines checked
Patch 3/3 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190311123507.24867-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Mon, Mar 11, 2019 at 01:35:04PM +0100, Paolo Bonzini wrote: > This is in preparation for CET support for x86. > > I know this is very late, but if possible I'd like to have this in 4.0 > as an experimental backend. > > Paolo Bonzini (3): > qemugdb: fix licensing > qemugdb: allow adding support for other coroutine backends > coroutine: add x86 specific coroutine backend > > configure | 8 + > scripts/qemu-gdb.py | 7 +- > scripts/qemugdb/coroutine.py | 114 +++++--------- > scripts/qemugdb/coroutine_ucontext.py | 69 +++++++++ > scripts/qemugdb/coroutine_x86.py | 21 +++ > scripts/qemugdb/mtree.py | 7 +- > scripts/qemugdb/tcg.py | 7 +- > util/coroutine-x86.c | 212 ++++++++++++++++++++++++++ > 8 files changed, 350 insertions(+), 95 deletions(-) > create mode 100644 scripts/qemugdb/coroutine_ucontext.py > create mode 100644 scripts/qemugdb/coroutine_x86.py > create mode 100644 util/coroutine-x86.c > > -- > 2.20.1 > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Mon, 11 Mar 2019 at 12:49, Paolo Bonzini <pbonzini@redhat.com> wrote: > > This is in preparation for CET support for x86. What's CET ? > I know this is very late, but if possible I'd like to have this in 4.0 > as an experimental backend. > > Paolo Bonzini (3): > qemugdb: fix licensing > qemugdb: allow adding support for other coroutine backends > coroutine: add x86 specific coroutine backend I can't say I'm terribly enthusiastic about having native-asm specific versions of the coroutine code. It seems like the kind of thing that's going to be fragile against changes in libc implementation, calling convention, etc. thanks -- PMM
On 13/03/19 12:49, Peter Maydell wrote: > On Mon, 11 Mar 2019 at 12:49, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> This is in preparation for CET support for x86. > > What's CET ? It's basically shadow stacks and tracking of indirect branch targets---hardware support for what others calls control-flow integrity. I've now sent the full series, which consists of these patches (with a few cleanups) followed by CET support. >> I know this is very late, but if possible I'd like to have this in 4.0 >> as an experimental backend. >> >> Paolo Bonzini (3): >> qemugdb: fix licensing >> qemugdb: allow adding support for other coroutine backends >> coroutine: add x86 specific coroutine backend > > I can't say I'm terribly enthusiastic about having > native-asm specific versions of the coroutine code. > It seems like the kind of thing that's going to be > fragile against changes in libc implementation, calling > convention, etc. I thought the same before I started writing it, but surprisingly it does not require any of that. Since the asm clobbers all registers except the frame and stack pointer, the compiler will just push/pop all caller-save registers automatically in qemu_coroutine_switch and qemu_coroutine_new's prolog and epilog. This also means that porting it to other architectures should require only a rewrite of the CO_SWITCH macro. I can try porting it to ARM and/or aarch64 to prove that point, or perhaps David will enjoy beating me and doing an s390 port. :) Paolo
On 13.03.19 14:24, Paolo Bonzini wrote: > On 13/03/19 12:49, Peter Maydell wrote: >> On Mon, 11 Mar 2019 at 12:49, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> This is in preparation for CET support for x86. >> >> What's CET ? > > It's basically shadow stacks and tracking of indirect branch > targets---hardware support for what others calls control-flow integrity. > I've now sent the full series, which consists of these patches (with a > few cleanups) followed by CET support. > >>> I know this is very late, but if possible I'd like to have this in 4.0 >>> as an experimental backend. >>> >>> Paolo Bonzini (3): >>> qemugdb: fix licensing >>> qemugdb: allow adding support for other coroutine backends >>> coroutine: add x86 specific coroutine backend >> >> I can't say I'm terribly enthusiastic about having >> native-asm specific versions of the coroutine code. >> It seems like the kind of thing that's going to be >> fragile against changes in libc implementation, calling >> convention, etc. > > I thought the same before I started writing it, but surprisingly it does > not require any of that. Since the asm clobbers all registers except > the frame and stack pointer, the compiler will just push/pop all > caller-save registers automatically in qemu_coroutine_switch and > qemu_coroutine_new's prolog and epilog. This also means that porting it > to other architectures should require only a rewrite of the CO_SWITCH macro. So the clobber list in the inline asm actually only hinders the compiler to play cheap tricks when trying to optimize - so it is forced to save/restore all caller-save register, right? Sounds sane to me, porting it shouldn't be too hard (however I can't tell when I will have time to look into the details). > > I can try porting it to ARM and/or aarch64 to prove that point, or > perhaps David will enjoy beating me and doing an s390 port. :) > > Paolo > -- Thanks, David / dhildenb
© 2016 - 2025 Red Hat, Inc.