Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag

[Sergey Fedorov]

On 18/04/16 17:09, Alex Bennée wrote:

Sergey Fedorov <address@hidden> writes:

From: Sergey Fedorov <address@hidden>

'tb_invalidated_flag' was meant to catch two events:
 * some TB has been invalidated by tb_phys_invalidate();
 * the whole translation buffer has been flushed by tb_flush().

Then it was checked:
 * in cpu_exec() to ensure that the last executed TB can be safely
   linked to directly call the next one;
 * in cpu_exec_nocache() to decide if the original TB should be provided
   for further possible invalidation along with the temporarily
   generated TB.

It is always safe to patch an invalidated TB since it is not going to be
used anyway.

Wouldn't that have implications for code searching through the linked
list of jump patched TBs?

The only implication I can see is that the jump in that already invalidated TB could just get reset back later on in tb_phys_invalidate(). We could keep track of invalidated TB's and skip patching those but it's also some overhead in the main CPU execution loop wich I'm not sure is worth to be introduced.

(snip)

diff --git a/cpu-exec.c b/cpu-exec.c

(snip)

@@ -507,14 +510,12 @@ int cpu_exec(CPUState *cpu)
                 }
                 tb_lock();
                 tb = tb_find_fast(cpu);
-                /* Note: we do it here to avoid a gcc bug on Mac OS X when
-                   doing it in tb_find_slow */

Is this still true? Would it make more sense to push the patching down
to the gen_code?

This comment comes up to the commit:

commit 1538800276aa7228d74f9d00bf275f54dc9e9b43              
Author: bellard <address@hidden>
Date:   Mon Dec 19 01:42:32 2005 +0000                       

    workaround for gcc bug on PowerPC

It was added more than ten years ago. Anyway, now this code is here not because of the bug: we need to reset 'next_tb' which is a local variable in cpu_exec(). Personally, I don't think it would be neater to hide it into gen_code(). Do you have some thoughts on how we could benefit from doing so? BTW, I had a feeling that it may be useful to reorganize cpu_exec() a bit, although I don't have a solid idea of how to do this so far.

I got slightly confused as to what next_tb ends up meaning at what point
in the run loop.

Yes, it seems to be a misleading name for this variable. As it was discussed on IRC, I'd like to break it into two variables, say 'last_tb' and 'tb_exit_idx', as soon as cpu_tb_exec() returns. Probably this series could also include such a patch.

Kind regards,
Sergey