On 2 May 2016 at 21:18, Sergey Fedorov <serge.f...@gmail.com> wrote:
> On 02/05/16 22:54, Sergey Fedorov wrote:
>
> Hi,
>
> I can't figure out how this field is used. The comment says it's "Currently
> executing TB", but actually it's the first TB in a chain of TBs executed.
> Grep shows the only place it is really checked is
> tb_invalidate_phys_page_range(). That code seems to be introduced long ago
> in:
>
> commit ea1c18022edd0e2c45552d6fc2da6e15a3486b33
> Author: bellard <bellard@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date: Mon Jun 14 18:56:36 2004 +0000
>
> fixed self modifying code in case of asynchronous interrupt
>
>
> I suspect it's only related to user emulation. But I would appreciate if
> someone could give me an idea of how this really works :)
>
>
> UPD: 'CPUState::current_tb' was used in that version of QEMU by this code:
>
> /* mask must never be zero, except for A20 change call */
> void cpu_interrupt(CPUState *env, int mask)
> {
> TranslationBlock *tb;
> static int interrupt_lock;
>
> env->interrupt_request |= mask;
> /* if the cpu is currently executing code, we must unlink it and
> all the potentially executing TB */
> tb = env->current_tb;
> if (tb && !testandset(&interrupt_lock)) {
> env->current_tb = NULL;
> tb_reset_jump_recursive(tb);
> interrupt_lock = 0;
> }
> }
>
>
> cpu_interrupt() has changed almost completely since that time. I'm wondering
> if checking 'cpu->current_tb' by this code in
> tb_invalidate_phys_page_range() still makes any sense:
>
> if (cpu->interrupt_request && cpu->current_tb) {
> cpu_interrupt(cpu, cpu->interrupt_request);
> }
>
>
> BTW, I'm not sure about the purpose of this piece of code either :)
I think it's now obsolete. When cpu_interrupt() worked
by unlinking the TB being executed and all the ones that it
chained to, then (as you see in the code you quote) cpu_interrupt()
only did actual work if env->current_tb was set. The code in
tb_invalidate_phys_page_range() doesn't want that work to happen
while it's in tb_phys_invalidate() [it would have tried to
modify the TB graph in the signal handler in the middle of
tb_phys_invalidate also modifying the graph and corrupted it],
so it sets cpu->current_tb to NULL to suppress this. However
that then meant that if we had an asynchronous interrupt
(ie executed cpu_interrupt() in a signal handler) it would
have done nothing, so the tb_invalidate_phys_page_range()
code now has to say "if we did get an interrupt, do the work
now" after it restores the current_tb pointer.
Since cpu_interrupt() no longer does complicated TB graph
modification it now does it unconditionally, so the work
done by tb_invalidate_phys_page_range() to clear cpu->current_tb
is unnecessary and so is the extra call to cpu_interrupt()
afterwards.
So I think the current_tb field can be deleted, and so
can the code fragments
/* we need to do that to handle the case where a signal
occurs while doing tb_phys_invalidate() */
saved_tb = NULL;
if (cpu != NULL) {
saved_tb = cpu->current_tb;
cpu->current_tb = NULL;
}
and
if (cpu != NULL) {
cpu->current_tb = saved_tb;
if (cpu->interrupt_request && cpu->current_tb) {
cpu_interrupt(cpu, cpu->interrupt_request);
}
}
because with our current code a signal and resulting
call to cpu_interrupt() is perfectly safe even if it
happens while we're executing tb_phys_invalidate().
thanks
-- PMM
