* [Qemu-devel] [4352] Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries .
@ 2008-05-06 8:44 Edgar E. Iglesias
2008-05-23 23:31 ` Fabrice Bellard
0 siblings, 1 reply; 3+ messages in thread
From: Edgar E. Iglesias @ 2008-05-06 8:44 UTC (permalink / raw)
To: qemu-devel
Revision: 4352
http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4352
Author: edgar_igl
Date: 2008-05-06 08:44:21 +0000 (Tue, 06 May 2008)
Log Message:
-----------
Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries.
Modified Paths:
--------------
trunk/exec.c
Modified: trunk/exec.c
===================================================================
--- trunk/exec.c 2008-05-06 08:38:22 UTC (rev 4351)
+++ trunk/exec.c 2008-05-06 08:44:21 UTC (rev 4352)
@@ -1366,6 +1366,21 @@
#if !defined(CONFIG_USER_ONLY)
+static inline void tlb_flush_jmp_cache(CPUState *env, target_ulong addr)
+{
+ unsigned int i;
+
+ /* Discard jump cache entries for any tb which might potentially
+ overlap the flushed page. */
+ i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
+ memset (&env->tb_jmp_cache[i], 0,
+ TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
+
+ i = tb_jmp_cache_hash_page(addr);
+ memset (&env->tb_jmp_cache[i], 0,
+ TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
+}
+
/* NOTE: if flush_global is true, also flush global entries (not
implemented yet) */
void tlb_flush(CPUState *env, int flush_global)
@@ -1428,7 +1443,6 @@
void tlb_flush_page(CPUState *env, target_ulong addr)
{
int i;
- TranslationBlock *tb;
#if defined(DEBUG_TLB)
printf("tlb_flush_page: " TARGET_FMT_lx "\n", addr);
@@ -1448,14 +1462,8 @@
#endif
#endif
- /* Discard jump cache entries for any tb which might potentially
- overlap the flushed page. */
- i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
- memset (&env->tb_jmp_cache[i], 0, TB_JMP_PAGE_SIZE * sizeof(tb));
+ tlb_flush_jmp_cache(env, addr);
- i = tb_jmp_cache_hash_page(addr);
- memset (&env->tb_jmp_cache[i], 0, TB_JMP_PAGE_SIZE * sizeof(tb));
-
#if !defined(CONFIG_SOFTMMU)
if (addr < MMAP_AREA_END)
munmap((void *)addr, TARGET_PAGE_SIZE);
@@ -1706,6 +1714,10 @@
} else {
te->addr_read = -1;
}
+
+ if (te->addr_code != -1) {
+ tlb_flush_jmp_cache(env, te->addr_code);
+ }
if (prot & PAGE_EXEC) {
te->addr_code = address;
} else {
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [4352] Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries .
2008-05-06 8:44 [Qemu-devel] [4352] Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries Edgar E. Iglesias
@ 2008-05-23 23:31 ` Fabrice Bellard
2008-05-24 18:01 ` Edgar E. Iglesias
0 siblings, 1 reply; 3+ messages in thread
From: Fabrice Bellard @ 2008-05-23 23:31 UTC (permalink / raw)
To: edgar.iglesias; +Cc: qemu-devel
Please revert this patch: it has a major performance hit because the
tb_jmp_cache is flushed too often. Flushing the tb_jmp_cache when
overriding a tlb_cache entry is not necessary, provided a given virtual
address is always remapped at the same physical address with the same
rights, which is the assumed API here. Detecting possible
inconsistencies is useful, but I am sure there is a solution without
such a performance hit.
Please avoid doing such modifications without doing regressions tests on
performance (e.g. a compilation with gcc).
Regards,
Fabrice.
Edgar E. Iglesias wrote:
> Revision: 4352
> http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4352
> Author: edgar_igl
> Date: 2008-05-06 08:44:21 +0000 (Tue, 06 May 2008)
>
> Log Message:
> -----------
> Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries.
>
> Modified Paths:
> --------------
> trunk/exec.c
>
> Modified: trunk/exec.c
> ===================================================================
> --- trunk/exec.c 2008-05-06 08:38:22 UTC (rev 4351)
> +++ trunk/exec.c 2008-05-06 08:44:21 UTC (rev 4352)
> @@ -1366,6 +1366,21 @@
>
> #if !defined(CONFIG_USER_ONLY)
>
> +static inline void tlb_flush_jmp_cache(CPUState *env, target_ulong addr)
> +{
> + unsigned int i;
> +
> + /* Discard jump cache entries for any tb which might potentially
> + overlap the flushed page. */
> + i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
> + memset (&env->tb_jmp_cache[i], 0,
> + TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
> +
> + i = tb_jmp_cache_hash_page(addr);
> + memset (&env->tb_jmp_cache[i], 0,
> + TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
> +}
> +
> /* NOTE: if flush_global is true, also flush global entries (not
> implemented yet) */
> void tlb_flush(CPUState *env, int flush_global)
> @@ -1428,7 +1443,6 @@
> void tlb_flush_page(CPUState *env, target_ulong addr)
> {
> int i;
> - TranslationBlock *tb;
>
> #if defined(DEBUG_TLB)
> printf("tlb_flush_page: " TARGET_FMT_lx "\n", addr);
> @@ -1448,14 +1462,8 @@
> #endif
> #endif
>
> - /* Discard jump cache entries for any tb which might potentially
> - overlap the flushed page. */
> - i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
> - memset (&env->tb_jmp_cache[i], 0, TB_JMP_PAGE_SIZE * sizeof(tb));
> + tlb_flush_jmp_cache(env, addr);
>
> - i = tb_jmp_cache_hash_page(addr);
> - memset (&env->tb_jmp_cache[i], 0, TB_JMP_PAGE_SIZE * sizeof(tb));
> -
> #if !defined(CONFIG_SOFTMMU)
> if (addr < MMAP_AREA_END)
> munmap((void *)addr, TARGET_PAGE_SIZE);
> @@ -1706,6 +1714,10 @@
> } else {
> te->addr_read = -1;
> }
> +
> + if (te->addr_code != -1) {
> + tlb_flush_jmp_cache(env, te->addr_code);
> + }
> if (prot & PAGE_EXEC) {
> te->addr_code = address;
> } else {
>
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [4352] Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries .
2008-05-23 23:31 ` Fabrice Bellard
@ 2008-05-24 18:01 ` Edgar E. Iglesias
0 siblings, 0 replies; 3+ messages in thread
From: Edgar E. Iglesias @ 2008-05-24 18:01 UTC (permalink / raw)
To: Fabrice Bellard; +Cc: edgar.iglesias, qemu-devel
On Sat, May 24, 2008 at 01:31:21AM +0200, Fabrice Bellard wrote:
> Please revert this patch: it has a major performance hit because the
> tb_jmp_cache is flushed too often. Flushing the tb_jmp_cache when
> overriding a tlb_cache entry is not necessary, provided a given virtual
> address is always remapped at the same physical address with the same
> rights, which is the assumed API here. Detecting possible
> inconsistencies is useful, but I am sure there is a solution without
> such a performance hit.
>
> Please avoid doing such modifications without doing regressions tests on
> performance (e.g. a compilation with gcc).
My misstake, unfortunately I only ran performance tests on CRIS. That flush
was needed at the time in order to avoid a complete tlb_flush() every time the
guest touched the TLB. IIRC those changes improved performance and got rid of
bugs were TBs got executed even if their addresses were not in the TLB.
The flush might not be needed once the tb jump cache hash function got fixed,
but I havent had time to verify that.
Thanks for clarifying the API.
Best regards
>
> Regards,
>
> Fabrice.
>
> Edgar E. Iglesias wrote:
> > Revision: 4352
> > http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4352
> > Author: edgar_igl
> > Date: 2008-05-06 08:44:21 +0000 (Tue, 06 May 2008)
> >
> > Log Message:
> > -----------
> > Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries.
> >
> > Modified Paths:
> > --------------
> > trunk/exec.c
> >
> > Modified: trunk/exec.c
> > ===================================================================
> > --- trunk/exec.c 2008-05-06 08:38:22 UTC (rev 4351)
> > +++ trunk/exec.c 2008-05-06 08:44:21 UTC (rev 4352)
> > @@ -1366,6 +1366,21 @@
> >
> > #if !defined(CONFIG_USER_ONLY)
> >
> > +static inline void tlb_flush_jmp_cache(CPUState *env, target_ulong addr)
> > +{
> > + unsigned int i;
> > +
> > + /* Discard jump cache entries for any tb which might potentially
> > + overlap the flushed page. */
> > + i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
> > + memset (&env->tb_jmp_cache[i], 0,
> > + TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
> > +
> > + i = tb_jmp_cache_hash_page(addr);
> > + memset (&env->tb_jmp_cache[i], 0,
> > + TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
> > +}
> > +
> > /* NOTE: if flush_global is true, also flush global entries (not
> > implemented yet) */
> > void tlb_flush(CPUState *env, int flush_global)
> > @@ -1428,7 +1443,6 @@
> > void tlb_flush_page(CPUState *env, target_ulong addr)
> > {
> > int i;
> > - TranslationBlock *tb;
> >
> > #if defined(DEBUG_TLB)
> > printf("tlb_flush_page: " TARGET_FMT_lx "\n", addr);
> > @@ -1448,14 +1462,8 @@
> > #endif
> > #endif
> >
> > - /* Discard jump cache entries for any tb which might potentially
> > - overlap the flushed page. */
> > - i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
> > - memset (&env->tb_jmp_cache[i], 0, TB_JMP_PAGE_SIZE * sizeof(tb));
> > + tlb_flush_jmp_cache(env, addr);
> >
> > - i = tb_jmp_cache_hash_page(addr);
> > - memset (&env->tb_jmp_cache[i], 0, TB_JMP_PAGE_SIZE * sizeof(tb));
> > -
> > #if !defined(CONFIG_SOFTMMU)
> > if (addr < MMAP_AREA_END)
> > munmap((void *)addr, TARGET_PAGE_SIZE);
> > @@ -1706,6 +1714,10 @@
> > } else {
> > te->addr_read = -1;
> > }
> > +
> > + if (te->addr_code != -1) {
> > + tlb_flush_jmp_cache(env, te->addr_code);
> > + }
> > if (prot & PAGE_EXEC) {
> > te->addr_code = address;
> > } else {
> >
> >
> >
> >
> >
>
>
--
Edgar E. Iglesias
Axis Communications AB
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-05-24 16:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-06 8:44 [Qemu-devel] [4352] Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries Edgar E. Iglesias
2008-05-23 23:31 ` Fabrice Bellard
2008-05-24 18:01 ` Edgar E. Iglesias
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).