| Message ID | 20200602154624.4460-6-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | plugins/next (bug fixes, hwprofile, lockstep) | expand |
On 6/2/20 8:46 AM, Alex Bennée wrote: > Any write to a device might cause a re-arrangement of memory > triggering a TLB flush and potential re-size of the TLB invalidating > previous entries. This would cause users of qemu_plugin_get_hwaddr() > to see the warning: > > invalid use of qemu_plugin_get_hwaddr > > because of the failed tlb_lookup which should always succeed. We catch > this case by checking to see if the list of entries has been cleared > and if so triggering a re-fill. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > accel/tcg/cputlb.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index eb2cf9de5e6..b7d329f7155 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r, > retaddr); > } > + > + /* > + * The memory_region_dispatch may have triggered a flush/resize > + * so for plugins we need to ensure we have reset the tlb_entry > + * so any later lookup is correct. > + */ > +#ifdef CONFIG_PLUGIN > + if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) { > + int size = op & MO_SIZE; > + tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE, > + mmu_idx, retaddr); Ouch. What if the target has a soft tlb fill, so this requires a call into the OS, so this fill actually raises another exception? This will not be happy fun making. I recall I had objections to recording this translation, saying that "we can always get it back again". Clearly I was wrong, and we should just preserve the required CPUTLBEntry details before they're lost by a device. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 6/2/20 8:46 AM, Alex Bennée wrote: >> Any write to a device might cause a re-arrangement of memory >> triggering a TLB flush and potential re-size of the TLB invalidating >> previous entries. This would cause users of qemu_plugin_get_hwaddr() >> to see the warning: >> >> invalid use of qemu_plugin_get_hwaddr >> >> because of the failed tlb_lookup which should always succeed. We catch >> this case by checking to see if the list of entries has been cleared >> and if so triggering a re-fill. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> accel/tcg/cputlb.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c >> index eb2cf9de5e6..b7d329f7155 100644 >> --- a/accel/tcg/cputlb.c >> +++ b/accel/tcg/cputlb.c >> @@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, >> MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r, >> retaddr); >> } >> + >> + /* >> + * The memory_region_dispatch may have triggered a flush/resize >> + * so for plugins we need to ensure we have reset the tlb_entry >> + * so any later lookup is correct. >> + */ >> +#ifdef CONFIG_PLUGIN >> + if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) { >> + int size = op & MO_SIZE; >> + tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE, >> + mmu_idx, retaddr); > > Ouch. What if the target has a soft tlb fill, so this requires a call into the > OS, so this fill actually raises another exception? This will not be happy fun > making. > > I recall I had objections to recording this translation, saying that "we can > always get it back again". Clearly I was wrong, and we should just preserve > the required CPUTLBEntry details before they're lost by a device. Maybe we could just RCU the old TLB if it gets flushed thus ensuring the whole TLB is preserved until after the critical section (i.e. between the actual store and looking it up). However I don't know if the MemoryRegion will be similarly preserved. Paolo? > > > r~
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index eb2cf9de5e6..b7d329f7155 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r, retaddr); } + + /* + * The memory_region_dispatch may have triggered a flush/resize + * so for plugins we need to ensure we have reset the tlb_entry + * so any later lookup is correct. + */ +#ifdef CONFIG_PLUGIN + if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) { + int size = op & MO_SIZE; + tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE, + mmu_idx, retaddr); + } +#endif + if (locked) { qemu_mutex_unlock_iothread(); }
Any write to a device might cause a re-arrangement of memory triggering a TLB flush and potential re-size of the TLB invalidating previous entries. This would cause users of qemu_plugin_get_hwaddr() to see the warning: invalid use of qemu_plugin_get_hwaddr because of the failed tlb_lookup which should always succeed. We catch this case by checking to see if the list of entries has been cleared and if so triggering a re-fill. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- accel/tcg/cputlb.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)