| Message ID | 20191127220602.10827-2-jcmvbkbc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | flush CPU TB cache in breakpoint_invalidate | expand |
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~
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~
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 > >
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.
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
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(-)