* [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; 8+ 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] 8+ 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 2020-02-05 11:00 ` Richard Henderson 2019-11-27 22:06 ` [PATCH 2/2] exec: drop tb_invalidate_phys_addr Max Filippov 1 sibling, 2 replies; 8+ 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 related [flat|nested] 8+ 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 2020-02-05 11:00 ` Richard Henderson 1 sibling, 1 reply; 8+ 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] 8+ 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; 8+ 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] 8+ 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 @ 2020-02-05 11:00 ` Richard Henderson 2020-02-05 21:14 ` Max Filippov 1 sibling, 1 reply; 8+ messages in thread From: Richard Henderson @ 2020-02-05 11:00 UTC (permalink / raw) To: Max Filippov, qemu-devel; +Cc: Paolo Bonzini, Alex Bennée, changbin.du 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 I know I had reservations about this, but we now have two patches on list that fix the problem in this way. What I would *like* is for each CPUBreakpoint to maintain a list of the TBs to which it has been applied, so that each can be invalidated. Our current management of breakpoints are IMO sloppy. That said, I don't really have time to work on cleaning this up myself in the short term, and this is fixing a real bug. Therefore, I am going to queue this to tcg-next. I would still like patch 2/2 to be split, and that can probably go through an xtensa branch. r~ > > 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] exec: flush CPU TB cache in breakpoint_invalidate 2020-02-05 11:00 ` Richard Henderson @ 2020-02-05 21:14 ` Max Filippov 0 siblings, 0 replies; 8+ messages in thread From: Max Filippov @ 2020-02-05 21:14 UTC (permalink / raw) To: Richard Henderson Cc: Paolo Bonzini, Alex Bennée, qemu-devel, changbin.du On Wed, Feb 5, 2020 at 3:00 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > 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 > > I know I had reservations about this, but we now have two patches on list that > fix the problem in this way. > > What I would *like* is for each CPUBreakpoint to maintain a list of the TBs to > which it has been applied, so that each can be invalidated. I don't see how this can fix this issue: it's not the list of TBs that we want to invalidate, it's the TBs that get associated with new virtual addresses that are currently causing the issue, right? > Our current > management of breakpoints are IMO sloppy. > > That said, I don't really have time to work on cleaning this up myself in the > short term, and this is fixing a real bug. Therefore, I am going to queue this > to tcg-next. > > I would still like patch 2/2 to be split, and that can probably go through an > xtensa branch. Will do. -- Thanks. -- Max ^ permalink raw reply [flat|nested] 8+ 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; 8+ 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 related [flat|nested] 8+ 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; 8+ 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] 8+ messages in thread
end of thread, other threads:[~2020-02-05 21:15 UTC | newest] Thread overview: 8+ 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 2020-02-05 11:00 ` Richard Henderson 2020-02-05 21:14 ` Max Filippov 2019-11-27 22:06 ` [PATCH 2/2] exec: drop tb_invalidate_phys_addr Max Filippov 2019-12-01 23:53 ` Richard Henderson
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).