From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49028) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axJw9-0007hz-LG for qemu-devel@nongnu.org; Mon, 02 May 2016 15:55:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axJvy-0000Mf-0n for qemu-devel@nongnu.org; Mon, 02 May 2016 15:55:16 -0400 Received: from mail-lf0-x22b.google.com ([2a00:1450:4010:c07::22b]:35450) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axJvw-0000Fn-20 for qemu-devel@nongnu.org; Mon, 02 May 2016 15:55:09 -0400 Received: by mail-lf0-x22b.google.com with SMTP id j8so68217667lfd.2 for ; Mon, 02 May 2016 12:54:53 -0700 (PDT) From: Sergey Fedorov Message-ID: <5727B088.1090400@gmail.com> Date: Mon, 2 May 2016 22:54:48 +0300 MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="------------020209020406000709020004" Subject: [Qemu-devel] tcg: How CPUState::current_tb is used? List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: QEMU Developers Cc: =?UTF-8?Q?Alex_Benn=c3=a9e?= , Paolo Bonzini , Richard Henderson , Blue Swirl , Riku Voipio This is a multi-part message in MIME format. --------------020209020406000709020004 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 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 --------------020209020406000709020004 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit 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
--------------020209020406000709020004-- From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axKJI-0003UL-TS for qemu-devel@nongnu.org; Mon, 02 May 2016 16:19:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axKJ7-0005q4-4B for qemu-devel@nongnu.org; Mon, 02 May 2016 16:19:11 -0400 Received: from mail-lf0-x22c.google.com ([2a00:1450:4010:c07::22c]:33404) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axKJ5-0005mj-3D for qemu-devel@nongnu.org; Mon, 02 May 2016 16:19:05 -0400 Received: by mail-lf0-x22c.google.com with SMTP id y84so201056257lfc.0 for ; Mon, 02 May 2016 13:18:49 -0700 (PDT) References: <5727B088.1090400@gmail.com> From: Sergey Fedorov Message-ID: <5727B623.2070901@gmail.com> Date: Mon, 2 May 2016 23:18:43 +0300 MIME-Version: 1.0 In-Reply-To: <5727B088.1090400@gmail.com> Content-Type: multipart/alternative; boundary="------------070403090005090900000706" Subject: Re: [Qemu-devel] tcg: How 'CPUState::current_tb' is used? List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: QEMU Developers Cc: =?UTF-8?Q?Alex_Benn=c3=a9e?= , Paolo Bonzini , Richard Henderson , Blue Swirl , Riku Voipio This is a multi-part message in MIME format. --------------070403090005090900000706 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > 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 --------------070403090005090900000706 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit
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
--------------070403090005090900000706-- From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39342) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axNo9-0001g9-02 for qemu-devel@nongnu.org; Mon, 02 May 2016 20:03:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axNnx-0003RU-4N for qemu-devel@nongnu.org; Mon, 02 May 2016 20:03:15 -0400 Received: from mail-vk0-x232.google.com ([2607:f8b0:400c:c05::232]:34053) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axNnv-0003Le-QB for qemu-devel@nongnu.org; Mon, 02 May 2016 20:03:08 -0400 Received: by mail-vk0-x232.google.com with SMTP id m188so4924591vka.1 for ; Mon, 02 May 2016 17:02:49 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5727B623.2070901@gmail.com> References: <5727B088.1090400@gmail.com> <5727B623.2070901@gmail.com> From: Peter Maydell Date: Tue, 3 May 2016 01:02:25 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] tcg: How 'CPUState::current_tb' is used? List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Fedorov Cc: QEMU Developers , Blue Swirl , Paolo Bonzini , Riku Voipio , =?UTF-8?B?QWxleCBCZW5uw6ll?= , Richard Henderson On 2 May 2016 at 21:18, Sergey Fedorov 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 > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axX4J-0004hB-6Y for qemu-devel@nongnu.org; Tue, 03 May 2016 05:56:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axX47-0001an-9g for qemu-devel@nongnu.org; Tue, 03 May 2016 05:56:33 -0400 Received: from mail-lf0-x22a.google.com ([2a00:1450:4010:c07::22a]:36282) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axX45-0001NP-Kp for qemu-devel@nongnu.org; Tue, 03 May 2016 05:56:27 -0400 Received: by mail-lf0-x22a.google.com with SMTP id u64so15554133lff.3 for ; Tue, 03 May 2016 02:56:11 -0700 (PDT) References: <5727B088.1090400@gmail.com> <5727B623.2070901@gmail.com> From: Sergey Fedorov Message-ID: <572875B6.5090705@gmail.com> Date: Tue, 3 May 2016 12:56:06 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] tcg: How 'CPUState::current_tb' is used? List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers , Blue Swirl , Paolo Bonzini , Riku Voipio , =?UTF-8?Q?Alex_Benn=c3=a9e?= , Richard Henderson On 03/05/16 03:02, Peter Maydell wrote: > On 2 May 2016 at 21:18, Sergey Fedorov 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 >> 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