[PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Ilya Leoshkevich]

Currently single-insn TBs created from I/O memory are not added to
region_trees. Therefore, when they generate exceptions, they are not
handled by cpu_restore_state_from_tb(). For x86 this is not a problem,
because x86_restore_state_to_opc() only restores pc and cc, which are
already correct. However, on several other architectures,
restore_state_to_opc() restores more registers, and guests can notice
incorrect values.

Fix by always calling tcg_tb_insert(). This may increase the size of
region_trees, but tcg_region_reset_all() clears it once code_gen_buffer
fills up, so it will not grow uncontrollably.

Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 accel/tcg/translate-all.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 453eb20ec95..6333302813e 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -531,23 +531,23 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
         tb_reset_jump(tb, 1);
     }
 
+    /*
+     * Insert TB into the corresponding region tree before publishing it
+     * through QHT. Otherwise rewinding happened in the TB might fail to
+     * lookup itself using host PC.
+     */
+    tcg_tb_insert(tb);
+
     /*
      * If the TB is not associated with a physical RAM page then it must be
      * a temporary one-insn TB, and we have nothing left to do. Return early
-     * before attempting to link to other TBs or add to the lookup table.
+     * before attempting to link to other TBs.
      */
     if (tb_page_addr0(tb) == -1) {
         assert_no_pages_locked();
         return tb;
     }
 
-    /*
-     * Insert TB into the corresponding region tree before publishing it
-     * through QHT. Otherwise rewinding happened in the TB might fail to
-     * lookup itself using host PC.
-     */
-    tcg_tb_insert(tb);
-
     /*
      * No explicit memory barrier is required -- tb_link_page() makes the
      * TB visible in a consistent state.
-- 
2.47.1

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Richard Henderson]

On 1/15/25 15:20, Ilya Leoshkevich wrote:
> Currently single-insn TBs created from I/O memory are not added to
> region_trees. Therefore, when they generate exceptions, they are not
> handled by cpu_restore_state_from_tb(). For x86 this is not a problem,
> because x86_restore_state_to_opc() only restores pc and cc, which are
> already correct. However, on several other architectures,
> restore_state_to_opc() restores more registers, and guests can notice
> incorrect values.
> 
> Fix by always calling tcg_tb_insert(). This may increase the size of
> region_trees, but tcg_region_reset_all() clears it once code_gen_buffer
> fills up, so it will not grow uncontrollably.
> 
> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---

This needs something else.  The reason why they're not insertted is that they're not valid 
for a second execution.  We need to not find them in the search tree.


r~

>   accel/tcg/translate-all.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 453eb20ec95..6333302813e 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -531,23 +531,23 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>           tb_reset_jump(tb, 1);
>       }
>   
> +    /*
> +     * Insert TB into the corresponding region tree before publishing it
> +     * through QHT. Otherwise rewinding happened in the TB might fail to
> +     * lookup itself using host PC.
> +     */
> +    tcg_tb_insert(tb);
> +
>       /*
>        * If the TB is not associated with a physical RAM page then it must be
>        * a temporary one-insn TB, and we have nothing left to do. Return early
> -     * before attempting to link to other TBs or add to the lookup table.
> +     * before attempting to link to other TBs.
>        */
>       if (tb_page_addr0(tb) == -1) {
>           assert_no_pages_locked();
>           return tb;
>       }
>   
> -    /*
> -     * Insert TB into the corresponding region tree before publishing it
> -     * through QHT. Otherwise rewinding happened in the TB might fail to
> -     * lookup itself using host PC.
> -     */
> -    tcg_tb_insert(tb);
> -
>       /*
>        * No explicit memory barrier is required -- tb_link_page() makes the
>        * TB visible in a consistent state.

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Ilya Leoshkevich]

On Wed, 2025-01-15 at 16:08 -0800, Richard Henderson wrote:
> On 1/15/25 15:20, Ilya Leoshkevich wrote:
> > Currently single-insn TBs created from I/O memory are not added to
> > region_trees. Therefore, when they generate exceptions, they are
> > not
> > handled by cpu_restore_state_from_tb(). For x86 this is not a
> > problem,
> > because x86_restore_state_to_opc() only restores pc and cc, which
> > are
> > already correct. However, on several other architectures,
> > restore_state_to_opc() restores more registers, and guests can
> > notice
> > incorrect values.
> > 
> > Fix by always calling tcg_tb_insert(). This may increase the size
> > of
> > region_trees, but tcg_region_reset_all() clears it once
> > code_gen_buffer
> > fills up, so it will not grow uncontrollably.
> > 
> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> 
> This needs something else.  The reason why they're not insertted is
> that they're not valid 
> for a second execution.  We need to not find them in the search tree.

I have the impression that code_gen_buffer is append-only, so after we
create a new TB for the second execution, the first TB should not
be deleted - is this correct? At least I haven't found code_gen_ptr
decrements, besides the rollback at the end of tb_gen_code(). Then,
since region_trees are indexed by code_gen_buffer pointers, and not
guest pointers, this should not introduce any stale entries.

While we might not need to find the ones created for the first
execution, we still need to find the ones for executions that fail -
and there is no way to tell in advance, which ones these are going to
be, so the idea here is to register all of them.

Am I missing something?

> r~

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> On Wed, 2025-01-15 at 16:08 -0800, Richard Henderson wrote:
>> On 1/15/25 15:20, Ilya Leoshkevich wrote:
>> > Currently single-insn TBs created from I/O memory are not added to
>> > region_trees. Therefore, when they generate exceptions, they are
>> > not
>> > handled by cpu_restore_state_from_tb(). For x86 this is not a
>> > problem,
>> > because x86_restore_state_to_opc() only restores pc and cc, which
>> > are
>> > already correct. However, on several other architectures,
>> > restore_state_to_opc() restores more registers, and guests can
>> > notice
>> > incorrect values.
>> > 
>> > Fix by always calling tcg_tb_insert(). This may increase the size
>> > of
>> > region_trees, but tcg_region_reset_all() clears it once
>> > code_gen_buffer
>> > fills up, so it will not grow uncontrollably.
>> > 
>> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> > ---
>> 
>> This needs something else.  The reason why they're not insertted is
>> that they're not valid 
>> for a second execution.  We need to not find them in the search tree.
>
> I have the impression that code_gen_buffer is append-only, so after we
> create a new TB for the second execution, the first TB should not
> be deleted - is this correct? At least I haven't found code_gen_ptr
> decrements, besides the rollback at the end of tb_gen_code(). Then,
> since region_trees are indexed by code_gen_buffer pointers, and not
> guest pointers, this should not introduce any stale entries.

We used to generate a temporary TB, execute it and then wind codeptr
back. We simplified the code to generate the TB but then not add it to
QHT - I think the original reasoning was saving on scarce CF_ flags and
not over complicating the tb_gen_code function. See around 873d64ac30
(accel/tcg: re-factor non-RAM execution code).

This does have the effect of potentially regenerating the same TB
over and over again but usually there only a few insns executed out of
IO space.

However some arches do more of this than others. And now we have devices
such as CXL where people do want to run large amounts of code directly
out of io address space. This eventually exhausts the codegen buffer so
we do a tb_flush() and start again. This is obviously sloooooow.

>
> While we might not need to find the ones created for the first
> execution, we still need to find the ones for executions that fail -
> and there is no way to tell in advance, which ones these are going to
> be, so the idea here is to register all of them.
>
> Am I missing something?
>
>> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Peter Maydell]

On Thu, 16 Jan 2025 at 10:52, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Ilya Leoshkevich <iii@linux.ibm.com> writes:
>
> > On Wed, 2025-01-15 at 16:08 -0800, Richard Henderson wrote:
> >> On 1/15/25 15:20, Ilya Leoshkevich wrote:
> >> > Currently single-insn TBs created from I/O memory are not added to
> >> > region_trees. Therefore, when they generate exceptions, they are
> >> > not
> >> > handled by cpu_restore_state_from_tb(). For x86 this is not a
> >> > problem,
> >> > because x86_restore_state_to_opc() only restores pc and cc, which
> >> > are
> >> > already correct. However, on several other architectures,
> >> > restore_state_to_opc() restores more registers, and guests can
> >> > notice
> >> > incorrect values.
> >> >
> >> > Fix by always calling tcg_tb_insert(). This may increase the size
> >> > of
> >> > region_trees, but tcg_region_reset_all() clears it once
> >> > code_gen_buffer
> >> > fills up, so it will not grow uncontrollably.
> >> >
> >> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> >> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> >> > ---
> >>
> >> This needs something else.  The reason why they're not insertted is
> >> that they're not valid
> >> for a second execution.  We need to not find them in the search tree.
> >
> > I have the impression that code_gen_buffer is append-only, so after we
> > create a new TB for the second execution, the first TB should not
> > be deleted - is this correct? At least I haven't found code_gen_ptr
> > decrements, besides the rollback at the end of tb_gen_code(). Then,
> > since region_trees are indexed by code_gen_buffer pointers, and not
> > guest pointers, this should not introduce any stale entries.
>
> We used to generate a temporary TB, execute it and then wind codeptr
> back. We simplified the code to generate the TB but then not add it to
> QHT - I think the original reasoning was saving on scarce CF_ flags and
> not over complicating the tb_gen_code function. See around 873d64ac30
> (accel/tcg: re-factor non-RAM execution code).
>
> This does have the effect of potentially regenerating the same TB
> over and over again but usually there only a few insns executed out of
> IO space.

You can't avoid regenerating the TB unless you somehow put the
entire bytes of the instruction into the hash you look up,
because you must re-read the MMIO region, and when you do the
second read you might read a different value than the first
time you read and created the TB.

The original reported problem here seems to me like it's a
problem with whatever target's frontend code this is.
This is a single instruction TB, so either:
 * the generated code for it completes the insn without
   raising an exception (no problem)
 * the generated code for it should raise an exception
   without having modified the CPU state (so there would
   be nothing to do for restore_state_to_opc)

It sounds like the target is generating code which does
something like:
 * do part of the instruction, including updating some of
   the CPU state
 * then decide it needs to raise an exception, and rely on
   the restore_state_to_opc handling to undo the state updates
   it did previously

The assumption of the "throwaway single insn TB" is that
you don't do that (i.e. that restore_state_to_opc is only
there for the benefit of multi-insn TBs).

thanks
-- PMM

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Ilya Leoshkevich]

On Thu, 2025-01-16 at 11:06 +0000, Peter Maydell wrote:
> On Thu, 16 Jan 2025 at 10:52, Alex Bennée <alex.bennee@linaro.org>
> wrote:
> > 
> > Ilya Leoshkevich <iii@linux.ibm.com> writes:
> > 
> > > On Wed, 2025-01-15 at 16:08 -0800, Richard Henderson wrote:
> > > > On 1/15/25 15:20, Ilya Leoshkevich wrote:
> > > > > Currently single-insn TBs created from I/O memory are not
> > > > > added to
> > > > > region_trees. Therefore, when they generate exceptions, they
> > > > > are
> > > > > not
> > > > > handled by cpu_restore_state_from_tb(). For x86 this is not a
> > > > > problem,
> > > > > because x86_restore_state_to_opc() only restores pc and cc,
> > > > > which
> > > > > are
> > > > > already correct. However, on several other architectures,
> > > > > restore_state_to_opc() restores more registers, and guests
> > > > > can
> > > > > notice
> > > > > incorrect values.
> > > > > 
> > > > > Fix by always calling tcg_tb_insert(). This may increase the
> > > > > size
> > > > > of
> > > > > region_trees, but tcg_region_reset_all() clears it once
> > > > > code_gen_buffer
> > > > > fills up, so it will not grow uncontrollably.
> > > > > 
> > > > > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > > ---
> > > > 
> > > > This needs something else.  The reason why they're not
> > > > insertted is
> > > > that they're not valid
> > > > for a second execution.  We need to not find them in the search
> > > > tree.
> > > 
> > > I have the impression that code_gen_buffer is append-only, so
> > > after we
> > > create a new TB for the second execution, the first TB should not
> > > be deleted - is this correct? At least I haven't found
> > > code_gen_ptr
> > > decrements, besides the rollback at the end of tb_gen_code().
> > > Then,
> > > since region_trees are indexed by code_gen_buffer pointers, and
> > > not
> > > guest pointers, this should not introduce any stale entries.
> > 
> > We used to generate a temporary TB, execute it and then wind
> > codeptr
> > back. We simplified the code to generate the TB but then not add it
> > to
> > QHT - I think the original reasoning was saving on scarce CF_ flags
> > and
> > not over complicating the tb_gen_code function. See around
> > 873d64ac30
> > (accel/tcg: re-factor non-RAM execution code).
> > 
> > This does have the effect of potentially regenerating the same TB
> > over and over again but usually there only a few insns executed out
> > of
> > IO space.
> 
> You can't avoid regenerating the TB unless you somehow put the
> entire bytes of the instruction into the hash you look up,
> because you must re-read the MMIO region, and when you do the
> second read you might read a different value than the first
> time you read and created the TB.
> 
> The original reported problem here seems to me like it's a
> problem with whatever target's frontend code this is.
> This is a single instruction TB, so either:
>  * the generated code for it completes the insn without
>    raising an exception (no problem)
>  * the generated code for it should raise an exception
>    without having modified the CPU state (so there would
>    be nothing to do for restore_state_to_opc)
> 
> It sounds like the target is generating code which does
> something like:
>  * do part of the instruction, including updating some of
>    the CPU state
>  * then decide it needs to raise an exception, and rely on
>    the restore_state_to_opc handling to undo the state updates
>    it did previously
> 
> The assumption of the "throwaway single insn TB" is that
> you don't do that (i.e. that restore_state_to_opc is only
> there for the benefit of multi-insn TBs).
> 
> thanks
> -- PMM

The problem is not a partial state update in an instruction, but rather
that on some targets restore_state_to_opc is more than just a
"restore" - it is also "prepare for handling an exception", i.e.:

- arm: exception.syndrome
- hppa: unwind_breg, psw_n
- mips: btarget
- openrisc: ppc
- riscv: excp_uw2
- s390x: int_pgm_ilen

Some of these may be wrong due to unfamiliarity with the respective
architectures, sorry - but this illustrates the idea.

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Peter Maydell]

On Thu, 16 Jan 2025 at 11:48, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Thu, 2025-01-16 at 11:06 +0000, Peter Maydell wrote:
> > The original reported problem here seems to me like it's a
> > problem with whatever target's frontend code this is.
> > This is a single instruction TB, so either:
> >  * the generated code for it completes the insn without
> >    raising an exception (no problem)
> >  * the generated code for it should raise an exception
> >    without having modified the CPU state (so there would
> >    be nothing to do for restore_state_to_opc)
> >
> > It sounds like the target is generating code which does
> > something like:
> >  * do part of the instruction, including updating some of
> >    the CPU state
> >  * then decide it needs to raise an exception, and rely on
> >    the restore_state_to_opc handling to undo the state updates
> >    it did previously
> >
> > The assumption of the "throwaway single insn TB" is that
> > you don't do that (i.e. that restore_state_to_opc is only
> > there for the benefit of multi-insn TBs).

> The problem is not a partial state update in an instruction, but rather
> that on some targets restore_state_to_opc is more than just a
> "restore" - it is also "prepare for handling an exception", i.e.:
>
> - arm: exception.syndrome
> - hppa: unwind_breg, psw_n
> - mips: btarget
> - openrisc: ppc
> - riscv: excp_uw2
> - s390x: int_pgm_ilen
>
> Some of these may be wrong due to unfamiliarity with the respective
> architectures, sorry - but this illustrates the idea.

Ah, yes, thanks for the clear explanation. The "throw away
the TB" design didn't consider that (or vice-versa).

thanks
-- PMM

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 16 Jan 2025 at 11:48, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>
>> On Thu, 2025-01-16 at 11:06 +0000, Peter Maydell wrote:
>> > The original reported problem here seems to me like it's a
>> > problem with whatever target's frontend code this is.
>> > This is a single instruction TB, so either:
>> >  * the generated code for it completes the insn without
>> >    raising an exception (no problem)
>> >  * the generated code for it should raise an exception
>> >    without having modified the CPU state (so there would
>> >    be nothing to do for restore_state_to_opc)
>> >
>> > It sounds like the target is generating code which does
>> > something like:
>> >  * do part of the instruction, including updating some of
>> >    the CPU state
>> >  * then decide it needs to raise an exception, and rely on
>> >    the restore_state_to_opc handling to undo the state updates
>> >    it did previously
>> >
>> > The assumption of the "throwaway single insn TB" is that
>> > you don't do that (i.e. that restore_state_to_opc is only
>> > there for the benefit of multi-insn TBs).
>
>> The problem is not a partial state update in an instruction, but rather
>> that on some targets restore_state_to_opc is more than just a
>> "restore" - it is also "prepare for handling an exception", i.e.:
>>
>> - arm: exception.syndrome
>> - hppa: unwind_breg, psw_n
>> - mips: btarget
>> - openrisc: ppc
>> - riscv: excp_uw2
>> - s390x: int_pgm_ilen
>>
>> Some of these may be wrong due to unfamiliarity with the respective
>> architectures, sorry - but this illustrates the idea.
>
> Ah, yes, thanks for the clear explanation. The "throw away
> the TB" design didn't consider that (or vice-versa).

We can certainly do with better docstrings for tcg_tb_lookup (via the
region tree) and tb_lookup (using cache and/or QHT) to make it clear the
difference between the two. I don't think we should ever use
tcg_tb_lookup for the purposes of executing a TB, just for resolution.

We have a few spare CF_ flags so maybe we could have a CF_RUNONCE flag
which is set for these TBs and assert its not set in tb_lookup along
with the current CF_INVALID flag. We could possibly set CF_INVALID
before executing the TB as we don't check the tb state from
tb_gen_code() before executing it but I guess that might be a little too
magic.

Rich, WDYT?

>
> thanks
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Philippe Mathieu-Daudé]

On 16/1/25 16:09, Alex Bennée wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Thu, 16 Jan 2025 at 11:48, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>>
>>> On Thu, 2025-01-16 at 11:06 +0000, Peter Maydell wrote:
>>>> The original reported problem here seems to me like it's a
>>>> problem with whatever target's frontend code this is.
>>>> This is a single instruction TB, so either:
>>>>   * the generated code for it completes the insn without
>>>>     raising an exception (no problem)
>>>>   * the generated code for it should raise an exception
>>>>     without having modified the CPU state (so there would
>>>>     be nothing to do for restore_state_to_opc)
>>>>
>>>> It sounds like the target is generating code which does
>>>> something like:
>>>>   * do part of the instruction, including updating some of
>>>>     the CPU state
>>>>   * then decide it needs to raise an exception, and rely on
>>>>     the restore_state_to_opc handling to undo the state updates
>>>>     it did previously
>>>>
>>>> The assumption of the "throwaway single insn TB" is that
>>>> you don't do that (i.e. that restore_state_to_opc is only
>>>> there for the benefit of multi-insn TBs).
>>
>>> The problem is not a partial state update in an instruction, but rather
>>> that on some targets restore_state_to_opc is more than just a
>>> "restore" - it is also "prepare for handling an exception", i.e.:
>>>
>>> - arm: exception.syndrome
>>> - hppa: unwind_breg, psw_n
>>> - mips: btarget
>>> - openrisc: ppc
>>> - riscv: excp_uw2
>>> - s390x: int_pgm_ilen

Should we move that to another TCGCPUOps handler?

>>> Some of these may be wrong due to unfamiliarity with the respective
>>> architectures, sorry - but this illustrates the idea.
>>
>> Ah, yes, thanks for the clear explanation. The "throw away
>> the TB" design didn't consider that (or vice-versa).
> 
> We can certainly do with better docstrings for tcg_tb_lookup (via the
> region tree) and tb_lookup (using cache and/or QHT) to make it clear the
> difference between the two. I don't think we should ever use
> tcg_tb_lookup for the purposes of executing a TB, just for resolution.
> 
> We have a few spare CF_ flags so maybe we could have a CF_RUNONCE flag
> which is set for these TBs and assert its not set in tb_lookup along
> with the current CF_INVALID flag. We could possibly set CF_INVALID
> before executing the TB as we don't check the tb state from
> tb_gen_code() before executing it but I guess that might be a little too
> magic.
> 
> Rich, WDYT?
> 
>>
>> thanks
>> -- PMM
> 

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Richard Henderson]

On 1/15/25 15:20, Ilya Leoshkevich wrote:
> Currently single-insn TBs created from I/O memory are not added to
> region_trees. Therefore, when they generate exceptions, they are not
> handled by cpu_restore_state_from_tb(). For x86 this is not a problem,
> because x86_restore_state_to_opc() only restores pc and cc, which are
> already correct. However, on several other architectures,
> restore_state_to_opc() restores more registers, and guests can notice
> incorrect values.
> 
> Fix by always calling tcg_tb_insert(). This may increase the size of
> region_trees, but tcg_region_reset_all() clears it once code_gen_buffer
> fills up, so it will not grow uncontrollably.
> 
> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   accel/tcg/translate-all.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 453eb20ec95..6333302813e 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -531,23 +531,23 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>           tb_reset_jump(tb, 1);
>       }
>   
> +    /*
> +     * Insert TB into the corresponding region tree before publishing it
> +     * through QHT. Otherwise rewinding happened in the TB might fail to
> +     * lookup itself using host PC.
> +     */
> +    tcg_tb_insert(tb);

I think what we need is to mark the tb CF_INVALID before inserting it. That way we'll 
never match in tb_lookup (comparing guest state, including cflags), but *will* find it in 
tcg_tb_lookup (comparing host_pc).


r~

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Ilya Leoshkevich]

On Thu, 2025-01-16 at 06:54 -0800, Richard Henderson wrote:
> On 1/15/25 15:20, Ilya Leoshkevich wrote:
> > Currently single-insn TBs created from I/O memory are not added to
> > region_trees. Therefore, when they generate exceptions, they are
> > not
> > handled by cpu_restore_state_from_tb(). For x86 this is not a
> > problem,
> > because x86_restore_state_to_opc() only restores pc and cc, which
> > are
> > already correct. However, on several other architectures,
> > restore_state_to_opc() restores more registers, and guests can
> > notice
> > incorrect values.
> > 
> > Fix by always calling tcg_tb_insert(). This may increase the size
> > of
> > region_trees, but tcg_region_reset_all() clears it once
> > code_gen_buffer
> > fills up, so it will not grow uncontrollably.
> > 
> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   accel/tcg/translate-all.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > index 453eb20ec95..6333302813e 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -531,23 +531,23 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> >           tb_reset_jump(tb, 1);
> >       }
> >   
> > +    /*
> > +     * Insert TB into the corresponding region tree before
> > publishing it
> > +     * through QHT. Otherwise rewinding happened in the TB might
> > fail to
> > +     * lookup itself using host PC.
> > +     */
> > +    tcg_tb_insert(tb);
> 
> I think what we need is to mark the tb CF_INVALID before inserting
> it. That way we'll 
> never match in tb_lookup (comparing guest state, including cflags),
> but *will* find it in 
> tcg_tb_lookup (comparing host_pc).
> 
> 
> r~

How can tb_lookup() find it? With this change, it is inserted into
region_trees, but not into tb_ctx.htable - this is done by
tb_link_page(), which is not called. And because it's not in
tb_ctx.htable, it can't end up in tb_jmp_cache either.

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Richard Henderson]

On 1/16/25 07:06, Ilya Leoshkevich wrote:
> On Thu, 2025-01-16 at 06:54 -0800, Richard Henderson wrote:
>> On 1/15/25 15:20, Ilya Leoshkevich wrote:
>>> Currently single-insn TBs created from I/O memory are not added to
>>> region_trees. Therefore, when they generate exceptions, they are
>>> not
>>> handled by cpu_restore_state_from_tb(). For x86 this is not a
>>> problem,
>>> because x86_restore_state_to_opc() only restores pc and cc, which
>>> are
>>> already correct. However, on several other architectures,
>>> restore_state_to_opc() restores more registers, and guests can
>>> notice
>>> incorrect values.
>>>
>>> Fix by always calling tcg_tb_insert(). This may increase the size
>>> of
>>> region_trees, but tcg_region_reset_all() clears it once
>>> code_gen_buffer
>>> fills up, so it will not grow uncontrollably.
>>>
>>> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    accel/tcg/translate-all.c | 16 ++++++++--------
>>>    1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>>> index 453eb20ec95..6333302813e 100644
>>> --- a/accel/tcg/translate-all.c
>>> +++ b/accel/tcg/translate-all.c
>>> @@ -531,23 +531,23 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>>>            tb_reset_jump(tb, 1);
>>>        }
>>>    
>>> +    /*
>>> +     * Insert TB into the corresponding region tree before
>>> publishing it
>>> +     * through QHT. Otherwise rewinding happened in the TB might
>>> fail to
>>> +     * lookup itself using host PC.
>>> +     */
>>> +    tcg_tb_insert(tb);
>>
>> I think what we need is to mark the tb CF_INVALID before inserting
>> it. That way we'll
>> never match in tb_lookup (comparing guest state, including cflags),
>> but *will* find it in
>> tcg_tb_lookup (comparing host_pc).
>>
>>
>> r~
> 
> How can tb_lookup() find it? With this change, it is inserted into
> region_trees, but not into tb_ctx.htable - this is done by
> tb_link_page(), which is not called. And because it's not in
> tb_ctx.htable, it can't end up in tb_jmp_cache either.

You're absolutely right.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 1/16/25 07:06, Ilya Leoshkevich wrote:
>> On Thu, 2025-01-16 at 06:54 -0800, Richard Henderson wrote:
>>> On 1/15/25 15:20, Ilya Leoshkevich wrote:
>>>> Currently single-insn TBs created from I/O memory are not added to
>>>> region_trees. Therefore, when they generate exceptions, they are
>>>> not
>>>> handled by cpu_restore_state_from_tb(). For x86 this is not a
>>>> problem,
>>>> because x86_restore_state_to_opc() only restores pc and cc, which
>>>> are
>>>> already correct. However, on several other architectures,
>>>> restore_state_to_opc() restores more registers, and guests can
>>>> notice
>>>> incorrect values.
>>>>
>>>> Fix by always calling tcg_tb_insert(). This may increase the size
>>>> of
>>>> region_trees, but tcg_region_reset_all() clears it once
>>>> code_gen_buffer
>>>> fills up, so it will not grow uncontrollably.
>>>>
>>>> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>>> ---
>>>>    accel/tcg/translate-all.c | 16 ++++++++--------
>>>>    1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>>>> index 453eb20ec95..6333302813e 100644
>>>> --- a/accel/tcg/translate-all.c
>>>> +++ b/accel/tcg/translate-all.c
>>>> @@ -531,23 +531,23 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>>>>            tb_reset_jump(tb, 1);
>>>>        }
>>>>    +    /*
>>>> +     * Insert TB into the corresponding region tree before
>>>> publishing it
>>>> +     * through QHT. Otherwise rewinding happened in the TB might
>>>> fail to
>>>> +     * lookup itself using host PC.
>>>> +     */
>>>> +    tcg_tb_insert(tb);
>>>
>>> I think what we need is to mark the tb CF_INVALID before inserting
>>> it. That way we'll
>>> never match in tb_lookup (comparing guest state, including cflags),
>>> but *will* find it in
>>> tcg_tb_lookup (comparing host_pc).
>>>
>>>
>>> r~
>> How can tb_lookup() find it? With this change, it is inserted into
>> region_trees, but not into tb_ctx.htable - this is done by
>> tb_link_page(), which is not called. And because it's not in
>> tb_ctx.htable, it can't end up in tb_jmp_cache either.
>
> You're absolutely right.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

It would still be nice to update the docstrings on the two lookup
functions to make it clear what they are for though.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Peter Maydell]

On Thu, 16 Jan 2025 at 15:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
> > On 1/16/25 07:06, Ilya Leoshkevich wrote:
> >> On Thu, 2025-01-16 at 06:54 -0800, Richard Henderson wrote:
> >>> On 1/15/25 15:20, Ilya Leoshkevich wrote:
> >>>> Currently single-insn TBs created from I/O memory are not added to
> >>>> region_trees. Therefore, when they generate exceptions, they are
> >>>> not
> >>>> handled by cpu_restore_state_from_tb(). For x86 this is not a
> >>>> problem,
> >>>> because x86_restore_state_to_opc() only restores pc and cc, which
> >>>> are
> >>>> already correct. However, on several other architectures,
> >>>> restore_state_to_opc() restores more registers, and guests can
> >>>> notice
> >>>> incorrect values.
> >>>>
> >>>> Fix by always calling tcg_tb_insert(). This may increase the size
> >>>> of
> >>>> region_trees, but tcg_region_reset_all() clears it once
> >>>> code_gen_buffer
> >>>> fills up, so it will not grow uncontrollably.
> >>>>
> >>>> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> >>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> >>>> ---
> >>>>    accel/tcg/translate-all.c | 16 ++++++++--------
> >>>>    1 file changed, 8 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> >>>> index 453eb20ec95..6333302813e 100644
> >>>> --- a/accel/tcg/translate-all.c
> >>>> +++ b/accel/tcg/translate-all.c
> >>>> @@ -531,23 +531,23 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> >>>>            tb_reset_jump(tb, 1);
> >>>>        }
> >>>>    +    /*
> >>>> +     * Insert TB into the corresponding region tree before
> >>>> publishing it
> >>>> +     * through QHT. Otherwise rewinding happened in the TB might
> >>>> fail to
> >>>> +     * lookup itself using host PC.
> >>>> +     */
> >>>> +    tcg_tb_insert(tb);
> >>>
> >>> I think what we need is to mark the tb CF_INVALID before inserting
> >>> it. That way we'll
> >>> never match in tb_lookup (comparing guest state, including cflags),
> >>> but *will* find it in
> >>> tcg_tb_lookup (comparing host_pc).
> >>>
> >>>
> >>> r~
> >> How can tb_lookup() find it? With this change, it is inserted into
> >> region_trees, but not into tb_ctx.htable - this is done by
> >> tb_link_page(), which is not called. And because it's not in
> >> tb_ctx.htable, it can't end up in tb_jmp_cache either.
> >
> > You're absolutely right.
> >
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> It would still be nice to update the docstrings on the two lookup
> functions to make it clear what they are for though.

Given this thread, perhaps also consider briefly mentioning some
of the details in the commit message and/or the comment?

-- PMM

Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Ilya Leoshkevich]

On Thu, 2025-01-16 at 15:41 +0000, Peter Maydell wrote:
> On Thu, 16 Jan 2025 at 15:40, Alex Bennée <alex.bennee@linaro.org>
> wrote:
> > 
> > Richard Henderson <richard.henderson@linaro.org> writes:
> > 
> > > On 1/16/25 07:06, Ilya Leoshkevich wrote:
> > > > On Thu, 2025-01-16 at 06:54 -0800, Richard Henderson wrote:
> > > > > On 1/15/25 15:20, Ilya Leoshkevich wrote:
> > > > > > Currently single-insn TBs created from I/O memory are not
> > > > > > added to
> > > > > > region_trees. Therefore, when they generate exceptions,
> > > > > > they are
> > > > > > not
> > > > > > handled by cpu_restore_state_from_tb(). For x86 this is not
> > > > > > a
> > > > > > problem,
> > > > > > because x86_restore_state_to_opc() only restores pc and cc,
> > > > > > which
> > > > > > are
> > > > > > already correct. However, on several other architectures,
> > > > > > restore_state_to_opc() restores more registers, and guests
> > > > > > can
> > > > > > notice
> > > > > > incorrect values.
> > > > > > 
> > > > > > Fix by always calling tcg_tb_insert(). This may increase
> > > > > > the size
> > > > > > of
> > > > > > region_trees, but tcg_region_reset_all() clears it once
> > > > > > code_gen_buffer
> > > > > > fills up, so it will not grow uncontrollably.
> > > > > > 
> > > > > > Co-developed-by: Nina Schoetterl-Glausch
> > > > > > <nsg@linux.ibm.com>
> > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > > > ---
> > > > > >    accel/tcg/translate-all.c | 16 ++++++++--------
> > > > > >    1 file changed, 8 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/accel/tcg/translate-all.c
> > > > > > b/accel/tcg/translate-all.c
> > > > > > index 453eb20ec95..6333302813e 100644
> > > > > > --- a/accel/tcg/translate-all.c
> > > > > > +++ b/accel/tcg/translate-all.c
> > > > > > @@ -531,23 +531,23 @@ TranslationBlock
> > > > > > *tb_gen_code(CPUState *cpu,
> > > > > >            tb_reset_jump(tb, 1);
> > > > > >        }
> > > > > >    +    /*
> > > > > > +     * Insert TB into the corresponding region tree before
> > > > > > publishing it
> > > > > > +     * through QHT. Otherwise rewinding happened in the TB
> > > > > > might
> > > > > > fail to
> > > > > > +     * lookup itself using host PC.
> > > > > > +     */
> > > > > > +    tcg_tb_insert(tb);
> > > > > 
> > > > > I think what we need is to mark the tb CF_INVALID before
> > > > > inserting
> > > > > it. That way we'll
> > > > > never match in tb_lookup (comparing guest state, including
> > > > > cflags),
> > > > > but *will* find it in
> > > > > tcg_tb_lookup (comparing host_pc).
> > > > > 
> > > > > 
> > > > > r~
> > > > How can tb_lookup() find it? With this change, it is inserted
> > > > into
> > > > region_trees, but not into tb_ctx.htable - this is done by
> > > > tb_link_page(), which is not called. And because it's not in
> > > > tb_ctx.htable, it can't end up in tb_jmp_cache either.
> > > 
> > > You're absolutely right.
> > > 
> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > 
> > It would still be nice to update the docstrings on the two lookup
> > functions to make it clear what they are for though.
> 
> Given this thread, perhaps also consider briefly mentioning some
> of the details in the commit message and/or the comment?
> 
> -- PMM

Ok. I will send a v2 with docstring additions in a separate patch.