Re: [Qemu-devel] [PATCH 4/5] tcg: reorder removal from lists in tb_phys_invalidate

[Paolo Bonzini]

On 28/03/2016 17:18, Sergey Fedorov wrote:
> The use pattern of 'tb_invalidated_flag' is a bit intricate; correct me,
> if I'm wrong about the following. Basically, 'tb_invalidated_flag' was
> meant to catch two events:
>  * some TB has been invalidated by tb_phys_invalidate();

This is patch 4.

>  * the whole translation buffer has been flushed by tb_flush().

This is patch 5.

> Then it is checked to ensure:
>  * the last executed TB can be safely patched to directly call the next
>    one in cpu_exec();
>  * the original TB should be provided for further possible invalidation
>    along with the temporarily generated TB when in cpu_exec_nocache().
>
> [...] I would suggest the following solution:
>  (1) Use 'tb->pc' as an indicator of whether TB is valid; check for it
>      in cpu_exec() when deciding on whether to patch the last executed
>      TB or not
>  (2) Use 'tcg_ctx.tb_ctx.tb_flush_count' to check for translation buffer
>      flushes; capture it before calling tb_gen_code() and compare to it
>      afterwards to check if tb_flush() has been called in between

Of course that would work, but it would be slower.  I think it is
unnecessary for two reasons:

1) There are two calls to cpu_exec_nocache.  One exits immediately with
"break;", the other always sets "next_tb = 0;".  Therefore it is safe in
both cases for cpu_exec_nocache to hijack cpu->tb_invalidated_flag.

2) if it were broken, it would _also_ be broken before these patches
because cpu_exec_nocache always runs with tb_lock taken.  So I think
documenting the assumptions is better than changing them at the same
time as doing other changes.


Your observation that tb->pc==-1 is not necessarily safe still holds of
course.  Probably the best thing is an inline that can do one of:

1) set cs_base to an invalid value (anything nonzero is enough except on
x86 and SPARC; SPARC can use all-ones)

2) sets the flags to an invalid combination (x86 can use all ones)

3) sets the PC to an invalid value (no one really needs it)

Paolo