QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] flush CPU TB cache in breakpoint_invalidate
@ 2019-11-27 22:06 Max Filippov
  2019-11-27 22:06 ` [PATCH 1/2] exec: " Max Filippov
  2019-11-27 22:06 ` [PATCH 2/2] exec: drop tb_invalidate_phys_addr Max Filippov
  0 siblings, 2 replies; 6+ messages in thread
From: Max Filippov @ 2019-11-27 22:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Max Filippov, Paolo Bonzini, Alex Bennée, Richard Henderson

Hello,

this series changes breakpoint_invalidate to unconditionally do
tb_flush and then changes remaining users of tb_invalidate_phys_addr
to do the same and removes tb_invalidate_phys_addr.

Changes RFC->v1:
- do tb_flush in breakpoint_invalidate unconditionally
- add tb_invalidate_phys_addr cleanup.

Max Filippov (2):
  exec: flush CPU TB cache in breakpoint_invalidate
  exec: drop tb_invalidate_phys_addr

 exec.c                     | 44 ++++++++------------------------------
 include/exec/exec-all.h    |  3 ---
 target/xtensa/dbg_helper.c | 19 +++-------------
 3 files changed, 12 insertions(+), 54 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] exec: flush CPU TB cache in breakpoint_invalidate
  2019-11-27 22:06 [PATCH 0/2] flush CPU TB cache in breakpoint_invalidate Max Filippov
@ 2019-11-27 22:06 ` " Max Filippov
  2019-12-01 23:48   ` Richard Henderson
  2019-11-27 22:06 ` [PATCH 2/2] exec: drop tb_invalidate_phys_addr Max Filippov
  1 sibling, 1 reply; 6+ messages in thread
From: Max Filippov @ 2019-11-27 22:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Max Filippov, Paolo Bonzini, Alex Bennée, Richard Henderson

When a breakpoint is inserted at location for which there's currently no
virtual to physical translation no action is taken on CPU TB cache. If a
TB for that virtual address already exists but is not visible ATM the
breakpoint won't be hit next time an instruction at that address will be
executed.

Flush entire CPU TB cache in breakpoint_invalidate to force
re-translation of all TBs for the breakpoint address.

This change fixes the following scenario:
- linux user application is running
- a breakpoint is inserted from QEMU gdbstub for a user address that is
  not currently present in the target CPU TLB
- an instruction at that address is executed, but the external debugger
  doesn't get control.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
Changes RFC->v1:
- do tb_flush in breakpoint_invalidate unconditionally

 exec.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index ffdb5185353b..1709b760edc1 100644
--- a/exec.c
+++ b/exec.c
@@ -1017,14 +1017,13 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs)
 
 static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 {
-    MemTxAttrs attrs;
-    hwaddr phys = cpu_get_phys_page_attrs_debug(cpu, pc, &attrs);
-    int asidx = cpu_asidx_from_attrs(cpu, attrs);
-    if (phys != -1) {
-        /* Locks grabbed by tb_invalidate_phys_addr */
-        tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as,
-                                phys | (pc & ~TARGET_PAGE_MASK), attrs);
-    }
+    /*
+     * There may not be a virtual to physical translation for the pc
+     * right now, but there may exist cached TB for this pc.
+     * Flush the whole TB cache to force re-translation of such TBs.
+     * This is heavyweight, but we're debugging anyway.
+     */
+    tb_flush(cpu);
 }
 #endif
 
-- 
2.20.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] exec: drop tb_invalidate_phys_addr
  2019-11-27 22:06 [PATCH 0/2] flush CPU TB cache in breakpoint_invalidate Max Filippov
  2019-11-27 22:06 ` [PATCH 1/2] exec: " Max Filippov
@ 2019-11-27 22:06 ` Max Filippov
  2019-12-01 23:53   ` Richard Henderson
  1 sibling, 1 reply; 6+ messages in thread
From: Max Filippov @ 2019-11-27 22:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Max Filippov, Paolo Bonzini, Alex Bennée, Richard Henderson

The only remaining user of tb_invalidate_phys_addr is target/xtensa
instruction breakpoint code and it is better to use tb_flush there.

Drop tb_invalidate_phys_addr implementations and declarations.
Use tb_flush in xtensa IBREAK helpers.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 exec.c                     | 29 ++---------------------------
 include/exec/exec-all.h    |  3 ---
 target/xtensa/dbg_helper.c | 19 +++----------------
 3 files changed, 5 insertions(+), 46 deletions(-)

diff --git a/exec.c b/exec.c
index 1709b760edc1..4d20fc005520 100644
--- a/exec.c
+++ b/exec.c
@@ -983,38 +983,13 @@ const char *parse_cpu_option(const char *cpu_option)
 }
 
 #if defined(CONFIG_USER_ONLY)
-void tb_invalidate_phys_addr(target_ulong addr)
+static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 {
     mmap_lock();
-    tb_invalidate_phys_page_range(addr, addr + 1);
+    tb_invalidate_phys_page_range(pc, pc + 1);
     mmap_unlock();
 }
-
-static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
-{
-    tb_invalidate_phys_addr(pc);
-}
 #else
-void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs)
-{
-    ram_addr_t ram_addr;
-    MemoryRegion *mr;
-    hwaddr l = 1;
-
-    if (!tcg_enabled()) {
-        return;
-    }
-
-    RCU_READ_LOCK_GUARD();
-    mr = address_space_translate(as, addr, &addr, &l, false, attrs);
-    if (!(memory_region_is_ram(mr)
-          || memory_region_is_romd(mr))) {
-        return;
-    }
-    ram_addr = memory_region_get_ram_addr(mr) + addr;
-    tb_invalidate_phys_page_range(ram_addr, ram_addr + 1);
-}
-
 static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 {
     /*
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index d85e610e85b9..585fe7ff430c 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -449,10 +449,7 @@ static inline uint32_t curr_cflags(void)
 
 /* TranslationBlock invalidate API */
 #if defined(CONFIG_USER_ONLY)
-void tb_invalidate_phys_addr(target_ulong addr);
 void tb_invalidate_phys_range(target_ulong start, target_ulong end);
-#else
-void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs);
 #endif
 void tb_flush(CPUState *cpu);
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
diff --git a/target/xtensa/dbg_helper.c b/target/xtensa/dbg_helper.c
index be1f81107b43..2481dc326fba 100644
--- a/target/xtensa/dbg_helper.c
+++ b/target/xtensa/dbg_helper.c
@@ -33,19 +33,6 @@
 #include "exec/exec-all.h"
 #include "exec/address-spaces.h"
 
-static void tb_invalidate_virtual_addr(CPUXtensaState *env, uint32_t vaddr)
-{
-    uint32_t paddr;
-    uint32_t page_size;
-    unsigned access;
-    int ret = xtensa_get_physical_addr(env, false, vaddr, 2, 0,
-                                       &paddr, &page_size, &access);
-    if (ret == 0) {
-        tb_invalidate_phys_addr(&address_space_memory, paddr,
-                                MEMTXATTRS_UNSPECIFIED);
-    }
-}
-
 void HELPER(wsr_ibreakenable)(CPUXtensaState *env, uint32_t v)
 {
     uint32_t change = v ^ env->sregs[IBREAKENABLE];
@@ -53,7 +40,8 @@ void HELPER(wsr_ibreakenable)(CPUXtensaState *env, uint32_t v)
 
     for (i = 0; i < env->config->nibreak; ++i) {
         if (change & (1 << i)) {
-            tb_invalidate_virtual_addr(env, env->sregs[IBREAKA + i]);
+            tb_flush(env_cpu(env));
+            break;
         }
     }
     env->sregs[IBREAKENABLE] = v & ((1 << env->config->nibreak) - 1);
@@ -62,8 +50,7 @@ void HELPER(wsr_ibreakenable)(CPUXtensaState *env, uint32_t v)
 void HELPER(wsr_ibreaka)(CPUXtensaState *env, uint32_t i, uint32_t v)
 {
     if (env->sregs[IBREAKENABLE] & (1 << i) && env->sregs[IBREAKA + i] != v) {
-        tb_invalidate_virtual_addr(env, env->sregs[IBREAKA + i]);
-        tb_invalidate_virtual_addr(env, v);
+        tb_flush(env_cpu(env));
     }
     env->sregs[IBREAKA + i] = v;
 }
-- 
2.20.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] exec: flush CPU TB cache in breakpoint_invalidate
  2019-11-27 22:06 ` [PATCH 1/2] exec: " Max Filippov
@ 2019-12-01 23:48   ` Richard Henderson
  2019-12-03 11:16     ` Alex Bennée
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2019-12-01 23:48 UTC (permalink / raw)
  To: Max Filippov, qemu-devel
  Cc: Paolo Bonzini, Alex Bennée, Richard Henderson

On 11/27/19 10:06 PM, Max Filippov wrote:
> When a breakpoint is inserted at location for which there's currently no
> virtual to physical translation no action is taken on CPU TB cache. If a
> TB for that virtual address already exists but is not visible ATM the
> breakpoint won't be hit next time an instruction at that address will be
> executed.
> 
> Flush entire CPU TB cache in breakpoint_invalidate to force
> re-translation of all TBs for the breakpoint address.
> 
> This change fixes the following scenario:
> - linux user application is running
> - a breakpoint is inserted from QEMU gdbstub for a user address that is
>   not currently present in the target CPU TLB
> - an instruction at that address is executed, but the external debugger
>   doesn't get control.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
> Changes RFC->v1:
> - do tb_flush in breakpoint_invalidate unconditionally

Unlike Paolo, I don't think this is a good idea.

If I was going to change anything here, I'd change this to not use
cpu_get_phys_page_attrs_debug but using the caching available from the actual
cputlb, using cc->tlb_fill() in probe mode -- something akin to probe_access(),
but not returning a host address, nor handling watchpoints nor notdirty.

This would help flushing too much by distinguishing different tbs for the same
virtual address mapping to a different physical address.


r~


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] exec: drop tb_invalidate_phys_addr
  2019-11-27 22:06 ` [PATCH 2/2] exec: drop tb_invalidate_phys_addr Max Filippov
@ 2019-12-01 23:53   ` Richard Henderson
  0 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2019-12-01 23:53 UTC (permalink / raw)
  To: Max Filippov, qemu-devel
  Cc: Paolo Bonzini, Alex Bennée, Richard Henderson

On 11/27/19 10:06 PM, Max Filippov wrote:
> The only remaining user of tb_invalidate_phys_addr is target/xtensa
> instruction breakpoint code and it is better to use tb_flush there.
> 
> Drop tb_invalidate_phys_addr implementations and declarations.
> Use tb_flush in xtensa IBREAK helpers.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  exec.c                     | 29 ++---------------------------
>  include/exec/exec-all.h    |  3 ---
>  target/xtensa/dbg_helper.c | 19 +++----------------
>  3 files changed, 5 insertions(+), 46 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Though perhaps split in half, so the xtensa patch comes first.


r~


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] exec: flush CPU TB cache in breakpoint_invalidate
  2019-12-01 23:48   ` Richard Henderson
@ 2019-12-03 11:16     ` Alex Bennée
  0 siblings, 0 replies; 6+ messages in thread
From: Alex Bennée @ 2019-12-03 11:16 UTC (permalink / raw)
  To: Richard Henderson
  Cc: Max Filippov, Richard Henderson, qemu-devel, Paolo Bonzini


Richard Henderson <richard.henderson@linaro.org> writes:

> On 11/27/19 10:06 PM, Max Filippov wrote:
>> When a breakpoint is inserted at location for which there's currently no
>> virtual to physical translation no action is taken on CPU TB cache. If a
>> TB for that virtual address already exists but is not visible ATM the
>> breakpoint won't be hit next time an instruction at that address will be
>> executed.
>> 
>> Flush entire CPU TB cache in breakpoint_invalidate to force
>> re-translation of all TBs for the breakpoint address.
>> 
>> This change fixes the following scenario:
>> - linux user application is running
>> - a breakpoint is inserted from QEMU gdbstub for a user address that is
>>   not currently present in the target CPU TLB
>> - an instruction at that address is executed, but the external debugger
>>   doesn't get control.
>> 
>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>> ---
>> Changes RFC->v1:
>> - do tb_flush in breakpoint_invalidate unconditionally
>
> Unlike Paolo, I don't think this is a good idea.

We previously had a general tb_flush during the MTTCG implementation as
a temporary fix. It was changed back in 406bc339b0 and it would be nice
to minimise the flushing of code if we can. While most interactive users
aren't going to notice the temporary slow down it would suck for any
automated gdb scripting.

>
> If I was going to change anything here, I'd change this to not use
> cpu_get_phys_page_attrs_debug but using the caching available from the actual
> cputlb, using cc->tlb_fill() in probe mode -- something akin to probe_access(),
> but not returning a host address, nor handling watchpoints nor notdirty.
>
> This would help flushing too much by distinguishing different tbs for the same
> virtual address mapping to a different physical address.
>
>
> r~


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-27 22:06 [PATCH 0/2] flush CPU TB cache in breakpoint_invalidate Max Filippov
2019-11-27 22:06 ` [PATCH 1/2] exec: " Max Filippov
2019-12-01 23:48   ` Richard Henderson
2019-12-03 11:16     ` Alex Bennée
2019-11-27 22:06 ` [PATCH 2/2] exec: drop tb_invalidate_phys_addr Max Filippov
2019-12-01 23:53   ` Richard Henderson

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git