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

-- 
2.34.1

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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~