| Message ID | 20240416040609.1313605-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | plugins: Use unwind info for special gdb registers | expand |
On 4/15/24 21:06, Richard Henderson wrote: > Based-on: 20240404230611.21231-1-richard.henderson@linaro.org > ("[PATCH v2 00/21] Rewrite plugin code generation") > > This is an attempt to fix > https://gitlab.com/qemu-project/qemu/-/issues/2208 > ("PC is not updated for each instruction in TCG plugins") > > I have only updated target/i386 so far, but basically all targets > need updating for the new callbacks. Extra points to anyone who > sees how to avoid the extra code duplication. :-) > Thanks for the series Richard. It looks good to me. Besides reviewing individual commits, I have a more general question. From some discussions we had, it seems like that previously gdb stub was correctly updating all register values, and that it has been dropped at some point. Was it for performance reasons, or an architectural change in QEMU? Is gdb stub the right way to poke register values for plugins? I don't know exactly why some registers are not updated correctly in this context, but it seems like we are trying to fix this afterward, instead of identifying root cause. Sorry if my question is irrelevant, I'm trying to understand the full context here. Thanks, Pierrick > > r~ > > > Richard Henderson (7): > tcg: Introduce INDEX_op_plugin_pc > accel/tcg: Set CPUState.plugin_ra before all plugin callbacks > accel/tcg: Return the TranslationBlock from cpu_unwind_state_data > plugins: Introduce TCGCPUOps callbacks for mid-tb register reads > target/i386: Split out gdb-internal.h > target/i386: Introduce cpu_compute_eflags_ccop > target/i386: Implement TCGCPUOps for plugin register reads > > include/exec/cpu-common.h | 9 +++-- > include/hw/core/cpu.h | 1 + > include/hw/core/tcg-cpu-ops.h | 13 +++++++ > include/tcg/tcg-op-common.h | 1 + > include/tcg/tcg-opc.h | 1 + > target/i386/cpu.h | 2 + > target/i386/gdb-internal.h | 65 +++++++++++++++++++++++++++++++ > accel/tcg/plugin-gen.c | 50 +++++++++++++++++++++--- > accel/tcg/translate-all.c | 9 +++-- > plugins/api.c | 36 +++++++++++++++++- > target/i386/gdbstub.c | 1 + > target/i386/helper.c | 6 ++- > target/i386/tcg/cc_helper.c | 10 +++++ > target/i386/tcg/tcg-cpu.c | 72 +++++++++++++++++++++++++++-------- > tcg/tcg-op.c | 5 +++ > tcg/tcg.c | 10 +++++ > 16 files changed, 258 insertions(+), 33 deletions(-) > create mode 100644 target/i386/gdb-internal.h >
On 4/16/24 17:35, Pierrick Bouvier wrote: > On 4/15/24 21:06, Richard Henderson wrote: >> Based-on: 20240404230611.21231-1-richard.henderson@linaro.org >> ("[PATCH v2 00/21] Rewrite plugin code generation") >> >> This is an attempt to fix >> https://gitlab.com/qemu-project/qemu/-/issues/2208 >> ("PC is not updated for each instruction in TCG plugins") >> >> I have only updated target/i386 so far, but basically all targets >> need updating for the new callbacks. Extra points to anyone who >> sees how to avoid the extra code duplication. :-) >> > > Thanks for the series Richard. It looks good to me. > > Besides reviewing individual commits, I have a more general question. > From some discussions we had, it seems like that previously gdb stub was correctly > updating all register values, and that it has been dropped at some point. Normally gdbstub does not run in the middle of a TB -- we end normally (single-step, breakpoint) or raise an exception (watchpoint). Only then, after TCG state has been made consistent, does gdbstub have access to the CPUState. > Was it for performance reasons, or an architectural change in QEMU? > Is gdb stub the right way to poke register values for plugins? > > I don't know exactly why some registers are not updated correctly in this context, but it > seems like we are trying to fix this afterward, instead of identifying root cause. The one or two registers are not updated continuously for performance reasons. And because they are not updated during initial code generation, it's not easy to do so later with plugin injection. But recovering that data is what the unwind info is for -- a bit expensive to access that way, but overall less, with the expectation that it is rare. r~
On 4/16/24 19:40, Richard Henderson wrote: > On 4/16/24 17:35, Pierrick Bouvier wrote: >> On 4/15/24 21:06, Richard Henderson wrote: >>> Based-on: 20240404230611.21231-1-richard.henderson@linaro.org >>> ("[PATCH v2 00/21] Rewrite plugin code generation") >>> >>> This is an attempt to fix >>> https://gitlab.com/qemu-project/qemu/-/issues/2208 >>> ("PC is not updated for each instruction in TCG plugins") >>> >>> I have only updated target/i386 so far, but basically all targets >>> need updating for the new callbacks. Extra points to anyone who >>> sees how to avoid the extra code duplication. :-) >>> >> >> Thanks for the series Richard. It looks good to me. >> >> Besides reviewing individual commits, I have a more general question. >> From some discussions we had, it seems like that previously gdb stub was correctly >> updating all register values, and that it has been dropped at some point. > > Normally gdbstub does not run in the middle of a TB -- we end normally (single-step, > breakpoint) or raise an exception (watchpoint). Only then, after TCG state has been made > consistent, does gdbstub have access to the CPUState. > That makes sense. > >> Was it for performance reasons, or an architectural change in QEMU? >> Is gdb stub the right way to poke register values for plugins? >> >> I don't know exactly why some registers are not updated correctly in this context, but it >> seems like we are trying to fix this afterward, instead of identifying root cause. > > The one or two registers are not updated continuously for performance reasons. And > because they are not updated during initial code generation, it's not easy to do so later > with plugin injection. But recovering that data is what the unwind info is for -- a bit > expensive to access that way, but overall less, with the expectation that it is rare. > Thanks for the description, I understand better the approach you picked for that issue. > > r~
Richard Henderson <richard.henderson@linaro.org> writes: > Based-on: 20240404230611.21231-1-richard.henderson@linaro.org > ("[PATCH v2 00/21] Rewrite plugin code generation") I'm getting code conflicts w.r.t to the above (which is already merged?) so it would be helpful to get a re-base. > > This is an attempt to fix > https://gitlab.com/qemu-project/qemu/-/issues/2208 > ("PC is not updated for each instruction in TCG plugins") The issue raises another question about PCREL support which makes me wonder if we need to deprecate get_vaddr at translation time and make it a run time only value? > > I have only updated target/i386 so far, but basically all targets > need updating for the new callbacks. Extra points to anyone who > sees how to avoid the extra code duplication. :-) > > > r~ > > > Richard Henderson (7): > tcg: Introduce INDEX_op_plugin_pc > accel/tcg: Set CPUState.plugin_ra before all plugin callbacks > accel/tcg: Return the TranslationBlock from cpu_unwind_state_data > plugins: Introduce TCGCPUOps callbacks for mid-tb register reads > target/i386: Split out gdb-internal.h > target/i386: Introduce cpu_compute_eflags_ccop > target/i386: Implement TCGCPUOps for plugin register reads > > include/exec/cpu-common.h | 9 +++-- > include/hw/core/cpu.h | 1 + > include/hw/core/tcg-cpu-ops.h | 13 +++++++ > include/tcg/tcg-op-common.h | 1 + > include/tcg/tcg-opc.h | 1 + > target/i386/cpu.h | 2 + > target/i386/gdb-internal.h | 65 +++++++++++++++++++++++++++++++ > accel/tcg/plugin-gen.c | 50 +++++++++++++++++++++--- > accel/tcg/translate-all.c | 9 +++-- > plugins/api.c | 36 +++++++++++++++++- > target/i386/gdbstub.c | 1 + > target/i386/helper.c | 6 ++- > target/i386/tcg/cc_helper.c | 10 +++++ > target/i386/tcg/tcg-cpu.c | 72 +++++++++++++++++++++++++++-------- > tcg/tcg-op.c | 5 +++ > tcg/tcg.c | 10 +++++ > 16 files changed, 258 insertions(+), 33 deletions(-) > create mode 100644 target/i386/gdb-internal.h
Based-on: 20240404230611.21231-1-richard.henderson@linaro.org ("[PATCH v2 00/21] Rewrite plugin code generation") This is an attempt to fix https://gitlab.com/qemu-project/qemu/-/issues/2208 ("PC is not updated for each instruction in TCG plugins") I have only updated target/i386 so far, but basically all targets need updating for the new callbacks. Extra points to anyone who sees how to avoid the extra code duplication. :-) r~ Richard Henderson (7): tcg: Introduce INDEX_op_plugin_pc accel/tcg: Set CPUState.plugin_ra before all plugin callbacks accel/tcg: Return the TranslationBlock from cpu_unwind_state_data plugins: Introduce TCGCPUOps callbacks for mid-tb register reads target/i386: Split out gdb-internal.h target/i386: Introduce cpu_compute_eflags_ccop target/i386: Implement TCGCPUOps for plugin register reads include/exec/cpu-common.h | 9 +++-- include/hw/core/cpu.h | 1 + include/hw/core/tcg-cpu-ops.h | 13 +++++++ include/tcg/tcg-op-common.h | 1 + include/tcg/tcg-opc.h | 1 + target/i386/cpu.h | 2 + target/i386/gdb-internal.h | 65 +++++++++++++++++++++++++++++++ accel/tcg/plugin-gen.c | 50 +++++++++++++++++++++--- accel/tcg/translate-all.c | 9 +++-- plugins/api.c | 36 +++++++++++++++++- target/i386/gdbstub.c | 1 + target/i386/helper.c | 6 ++- target/i386/tcg/cc_helper.c | 10 +++++ target/i386/tcg/tcg-cpu.c | 72 +++++++++++++++++++++++++++-------- tcg/tcg-op.c | 5 +++ tcg/tcg.c | 10 +++++ 16 files changed, 258 insertions(+), 33 deletions(-) create mode 100644 target/i386/gdb-internal.h