|Subject:||Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag|
|Date:||Mon, 18 Apr 2016 18:05:40 +0300|
|User-agent:||Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0|
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
@@ -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:
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.
|[Prev in Thread]||Current Thread||[Next in Thread]|