[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] tcg: How 'CPUState::current_tb' is used?

From: Peter Maydell
Subject: Re: [Qemu-devel] tcg: How 'CPUState::current_tb' is used?
Date: Tue, 3 May 2016 01:02:25 +0100

On 2 May 2016 at 21:18, Sergey Fedorov <address@hidden> 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 <address@hidden>
> 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()

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;

            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().

-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]