From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54334) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2M5V-0004SV-8G for qemu-devel@nongnu.org; Mon, 16 May 2016 13:13:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b2M5Q-00053G-Vj for qemu-devel@nongnu.org; Mon, 16 May 2016 13:13:49 -0400 Received: from mail-lb0-x241.google.com ([2a00:1450:4010:c04::241]:36600) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2M5Q-00052U-06 for qemu-devel@nongnu.org; Mon, 16 May 2016 13:13:44 -0400 Received: by mail-lb0-x241.google.com with SMTP id r5so610152lbj.3 for ; Mon, 16 May 2016 10:13:43 -0700 (PDT) References: <1463414992-8357-1-git-send-email-peter.maydell@linaro.org> <1463414992-8357-2-git-send-email-peter.maydell@linaro.org> From: Sergey Fedorov Message-ID: <5739FFC5.9010706@gmail.com> Date: Mon, 16 May 2016 20:13:41 +0300 MIME-Version: 1.0 In-Reply-To: <1463414992-8357-2-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] translate-all.c: Don't pass puc, locked to tb_invalidate_phys_page() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , qemu-devel@nongnu.org Cc: Paolo Bonzini , Riku Voipio , Richard Henderson , Eduardo Habkost , patches@linaro.org On 16/05/16 19:09, Peter Maydell wrote: > The user-mode-only function tb_invalidate_phys_page() is only > called from two places: > * page_unprotect(), which passes in a non-zero pc, a puc pointer > and the value 'true' for the locked argument > * page_set_flags(), which passes in a zero pc, a NULL puc pointer > and a 'false' locked argument > > If the pc is non-zero then we may call cpu_resume_from_signal(), > which does a longjmp out of the calling code (and out of the > signal handler); this is to cover the case of a target CPU with > "precise self-modifying code" (currently only x86) executing > a store instruction which modifies code in the same TB as the > store itself. Rather than doing the longjump directly here, > return a flag to the caller which indicates whether the current > TB was modified, and move the longjump to page_unprotect. > > Signed-off-by: Peter Maydell > --- > translate-all.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/translate-all.c b/translate-all.c > index b54f472..4820d2e 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -1438,10 +1438,13 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len) > } > } > #else > -/* Called with mmap_lock held. */ > -static void tb_invalidate_phys_page(tb_page_addr_t addr, > - uintptr_t pc, void *puc, > - bool locked) > +/* Called with mmap_lock held. If pc is not 0 then it indicates the > + * host PC of the faulting store instruction that caused this invalidate. > + * Returns true if the caller needs to abort execution of the current > + * TB (because it was modified by this store and the guest CPU has > + * precise-SMC semantics). > + */ > +static bool tb_invalidate_phys_page(tb_page_addr_t addr, uintptr_t pc) > { > TranslationBlock *tb; > PageDesc *p; > @@ -1459,7 +1462,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr, > addr &= TARGET_PAGE_MASK; > p = page_find(addr >> TARGET_PAGE_BITS); > if (!p) { > - return; > + return false; > } > tb = p->first_tb; > #ifdef TARGET_HAS_PRECISE_SMC > @@ -1498,12 +1501,10 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr, > modifying the memory. It will ensure that it cannot modify > itself */ > tb_gen_code(cpu, current_pc, current_cs_base, current_flags, 1); > - if (locked) { > - mmap_unlock(); > - } > - cpu_resume_from_signal(cpu, puc); > + return true; > } > #endif > + return false; > } > #endif > > @@ -1902,7 +1903,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags) > if (!(p->flags & PAGE_WRITE) && > (flags & PAGE_WRITE) && > p->first_tb) { > - tb_invalidate_phys_page(addr, 0, NULL, false); > + tb_invalidate_phys_page(addr, 0); > } > p->flags = flags; > } > @@ -1996,7 +1997,10 @@ int page_unprotect(target_ulong address, uintptr_t pc, void *puc) > > /* and since the content will be modified, we must invalidate > the corresponding translated code. */ > - tb_invalidate_phys_page(addr, pc, puc, true); > + if (tb_invalidate_phys_page(addr, pc)) { > + mmap_unlock(); > + cpu_resume_from_signal(current_cpu, puc); > + } > #ifdef DEBUG_TB_CHECK > tb_invalidate_check(addr); > #endif Just my 2 cents: we could allow that cpu_resume_from_signal() call and add mmap_lock_reset() similar to tb_lock_reset() to handle resetting mmap_lock after a long jump. Kind regards, Sergey