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

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

[Sergey Fedorov]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

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 :)

Thanks,
Sergey

[-- Attachment #2: Type: text/html, Size: 998 bytes --]

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

[Sergey Fedorov]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

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 :)

Kind regards,
Sergey

[-- Attachment #2: Type: text/html, Size: 2723 bytes --]

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

[Peter Maydell]

On 2 May 2016 at 21:18, Sergey Fedorov <serge.fdrv@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

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

[Sergey Fedorov]

On 03/05/16 03:02, Peter Maydell wrote:
> On 2 May 2016 at 21:18, Sergey Fedorov <serge.fdrv@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().

Thank you for the detailed explanation. Now, in the morning, I can see
it :) I'll prepare a patch for this.

Kind regards,
Sergey