On 03/05/16 03:02, Peter Maydell wrote:
> 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().
Thank you for the detailed explanation. Now, in the morning, I can see
it :) I'll prepare a patch for this.
Kind regards,
Sergey
