| Message ID | 20200709141327.14631-5-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | misc rc0 fixes (docs, plugins, docker) | expand |
On 7/9/20 7:13 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. To > prevent this we save the IOTLB data in case it is later needed by a > plugin doing a lookup. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > v2 > - save the entry instead of re-running the tlb_fill. > v3 > - don't abuse TLS, use CPUState to store data > - just use g_free_rcu() to avoid ugliness > - verify addr matches before returning data > - ws fix > --- > include/hw/core/cpu.h | 4 +++ > include/qemu/typedefs.h | 1 + > accel/tcg/cputlb.c | 57 +++++++++++++++++++++++++++++++++++++++-- > 3 files changed, 60 insertions(+), 2 deletions(-) > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index b3f4b7931823..bedbf098dc57 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -417,7 +417,11 @@ struct CPUState { > > DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX); > > +#ifdef CONFIG_PLUGIN > GArray *plugin_mem_cbs; > + /* saved iotlb data from io_writex */ > + SavedIOTLB *saved_iotlb; > +#endif > > /* TODO Move common fields from CPUArchState here. */ > int cpu_index; > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index 15f5047bf1dc..427027a9707a 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -116,6 +116,7 @@ typedef struct QObject QObject; > typedef struct QString QString; > typedef struct RAMBlock RAMBlock; > typedef struct Range Range; > +typedef struct SavedIOTLB SavedIOTLB; > typedef struct SHPCDevice SHPCDevice; > typedef struct SSIBus SSIBus; > typedef struct VirtIODevice VirtIODevice; > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index 1e815357c709..8636b66e036a 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -1073,6 +1073,42 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > return val; > } > > +#ifdef CONFIG_PLUGIN > + > +typedef struct SavedIOTLB { > + struct rcu_head rcu; > + hwaddr addr; > + MemoryRegionSection *section; > + hwaddr mr_offset; > +} SavedIOTLB; > + > +/* > + * Save a potentially trashed IOTLB entry for later lookup by plugin. > + * > + * We also need to track the thread storage address because the RCU > + * cleanup that runs when we leave the critical region (the current > + * execution) is actually in a different thread. > + */ > +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection *section, hwaddr mr_offset) Overlong line. > +{ > + SavedIOTLB *old, *new = g_new(SavedIOTLB, 1); > + new->addr = addr; > + new->section = section; > + new->mr_offset = mr_offset; > + old = atomic_rcu_read(&cs->saved_iotlb); > + atomic_rcu_set(&cs->saved_iotlb, new); > + if (old) { > + g_free_rcu(old, rcu); > + } > +} I'm a bit confused by this. Why all the multiple allocation? How many consumers are you expecting, and more are you expecting multiple memory operations in flight at once? If multiple memory operations in flight, then why aren't we chaining them together, so that you can search through multiple alternatives. If only one memory operation in flight, why are you allocating memory at all, much less managing it with rcu? Just put one structure (or a collection of fields) into CPUState and be done. > + > +#else > +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection *section, hwaddr mr_offset) > +{ > + /* do nothing */ > +} > +#endif Surely better to move the ifdef inside the function so that you don't have to replicate the definition? > + SavedIOTLB *saved = atomic_rcu_read(&cpu->saved_iotlb); > + if (saved && saved->addr == tlb_addr) { > + data->is_io = true; > + data->v.io.section = saved->section; > + data->v.io.offset = saved->mr_offset; > + return true; > + } Should that test in fact be an assert? Why would this fail? r~
On Thu, Jul 09, 2020 at 15:13:18 +0100, 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. To > prevent this we save the IOTLB data in case it is later needed by a > plugin doing a lookup. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Emilio G. Cota <cota@braap.org> Some minor comments below. > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -1073,6 +1073,42 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > return val; > } > > +#ifdef CONFIG_PLUGIN > + > +typedef struct SavedIOTLB { > + struct rcu_head rcu; > + hwaddr addr; > + MemoryRegionSection *section; > + hwaddr mr_offset; > +} SavedIOTLB; > + > +/* > + * Save a potentially trashed IOTLB entry for later lookup by plugin. > + * > + * We also need to track the thread storage address because the RCU > + * cleanup that runs when we leave the critical region (the current > + * execution) is actually in a different thread. Mentioning the thread storage is now outdated -- I think this comment (starting from 'We') can be removed. > + */ > +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection *section, hwaddr mr_offset) > +{ > + SavedIOTLB *old, *new = g_new(SavedIOTLB, 1); > + new->addr = addr; > + new->section = section; > + new->mr_offset = mr_offset; > + old = atomic_rcu_read(&cs->saved_iotlb); > + atomic_rcu_set(&cs->saved_iotlb, new); > + if (old) { > + g_free_rcu(old, rcu); > + } Using atomic_rcu_read here is not necessary (only this thread ever writes to this field) and might confuse a reader when trying to find the atomic_rcu_read that matches the atomic_rcu_set (that read is in tlb_plugin_lookup). Consider doing old = cs->saved_iotlb; instead. Thanks, Emilio
On Fri, Jul 10, 2020 at 14:03:27 -0700, Richard Henderson wrote: > On 7/9/20 7:13 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. To > > prevent this we save the IOTLB data in case it is later needed by a > > plugin doing a lookup. > > > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > > > --- > > v2 > > - save the entry instead of re-running the tlb_fill. > > v3 > > - don't abuse TLS, use CPUState to store data > > - just use g_free_rcu() to avoid ugliness > > - verify addr matches before returning data > > - ws fix > > --- > > include/hw/core/cpu.h | 4 +++ > > include/qemu/typedefs.h | 1 + > > accel/tcg/cputlb.c | 57 +++++++++++++++++++++++++++++++++++++++-- > > 3 files changed, 60 insertions(+), 2 deletions(-) > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > > index b3f4b7931823..bedbf098dc57 100644 > > --- a/include/hw/core/cpu.h > > +++ b/include/hw/core/cpu.h > > @@ -417,7 +417,11 @@ struct CPUState { > > > > DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX); > > > > +#ifdef CONFIG_PLUGIN > > GArray *plugin_mem_cbs; > > + /* saved iotlb data from io_writex */ > > + SavedIOTLB *saved_iotlb; > > +#endif > > > > /* TODO Move common fields from CPUArchState here. */ > > int cpu_index; > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > > index 15f5047bf1dc..427027a9707a 100644 > > --- a/include/qemu/typedefs.h > > +++ b/include/qemu/typedefs.h > > @@ -116,6 +116,7 @@ typedef struct QObject QObject; > > typedef struct QString QString; > > typedef struct RAMBlock RAMBlock; > > typedef struct Range Range; > > +typedef struct SavedIOTLB SavedIOTLB; > > typedef struct SHPCDevice SHPCDevice; > > typedef struct SSIBus SSIBus; > > typedef struct VirtIODevice VirtIODevice; > > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > > index 1e815357c709..8636b66e036a 100644 > > --- a/accel/tcg/cputlb.c > > +++ b/accel/tcg/cputlb.c > > @@ -1073,6 +1073,42 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, > > return val; > > } > > > > +#ifdef CONFIG_PLUGIN > > + > > +typedef struct SavedIOTLB { > > + struct rcu_head rcu; > > + hwaddr addr; > > + MemoryRegionSection *section; > > + hwaddr mr_offset; > > +} SavedIOTLB; > > + > > +/* > > + * Save a potentially trashed IOTLB entry for later lookup by plugin. > > + * > > + * We also need to track the thread storage address because the RCU > > + * cleanup that runs when we leave the critical region (the current > > + * execution) is actually in a different thread. > > + */ > > +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection *section, hwaddr mr_offset) > > Overlong line. > > > +{ > > + SavedIOTLB *old, *new = g_new(SavedIOTLB, 1); > > + new->addr = addr; > > + new->section = section; > > + new->mr_offset = mr_offset; > > + old = atomic_rcu_read(&cs->saved_iotlb); > > + atomic_rcu_set(&cs->saved_iotlb, new); > > + if (old) { > > + g_free_rcu(old, rcu); > > + } > > +} > > I'm a bit confused by this. Why all the multiple allocation? How many > consumers are you expecting, and more are you expecting multiple memory > operations in flight at once? > > If multiple memory operations in flight, then why aren't we chaining them > together, so that you can search through multiple alternatives. > > If only one memory operation in flight, why are you allocating memory at all, > much less managing it with rcu? Just put one structure (or a collection of > fields) into CPUState and be done. Oh I just saw this reply. I subscribe all of the above, please shelve my R-b tag until these are resolved. An alternative is to emit the hwaddr directly in the mem_cb -- IIRC this is how I did it originally. The API is a larger/uglier (plugins can subscribe to either hwaddr or vaddr callbacks) but there is no state to keep and no overhead of calling several functions in a hot path. Thanks, E.
Emilio G. Cota <cota@braap.org> writes: > On Fri, Jul 10, 2020 at 14:03:27 -0700, Richard Henderson wrote: >> On 7/9/20 7:13 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. To >> > prevent this we save the IOTLB data in case it is later needed by a >> > plugin doing a lookup. >> > >> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> > >> > --- >> > v2 >> > - save the entry instead of re-running the tlb_fill. >> > v3 >> > - don't abuse TLS, use CPUState to store data >> > - just use g_free_rcu() to avoid ugliness >> > - verify addr matches before returning data >> > - ws fix >> > --- >> > include/hw/core/cpu.h | 4 +++ >> > include/qemu/typedefs.h | 1 + >> > accel/tcg/cputlb.c | 57 +++++++++++++++++++++++++++++++++++++++-- >> > 3 files changed, 60 insertions(+), 2 deletions(-) >> > >> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >> > index b3f4b7931823..bedbf098dc57 100644 >> > --- a/include/hw/core/cpu.h >> > +++ b/include/hw/core/cpu.h >> > @@ -417,7 +417,11 @@ struct CPUState { >> > >> > DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX); >> > >> > +#ifdef CONFIG_PLUGIN >> > GArray *plugin_mem_cbs; >> > + /* saved iotlb data from io_writex */ >> > + SavedIOTLB *saved_iotlb; >> > +#endif >> > >> > /* TODO Move common fields from CPUArchState here. */ >> > int cpu_index; >> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >> > index 15f5047bf1dc..427027a9707a 100644 >> > --- a/include/qemu/typedefs.h >> > +++ b/include/qemu/typedefs.h >> > @@ -116,6 +116,7 @@ typedef struct QObject QObject; >> > typedef struct QString QString; >> > typedef struct RAMBlock RAMBlock; >> > typedef struct Range Range; >> > +typedef struct SavedIOTLB SavedIOTLB; >> > typedef struct SHPCDevice SHPCDevice; >> > typedef struct SSIBus SSIBus; >> > typedef struct VirtIODevice VirtIODevice; >> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c >> > index 1e815357c709..8636b66e036a 100644 >> > --- a/accel/tcg/cputlb.c >> > +++ b/accel/tcg/cputlb.c >> > @@ -1073,6 +1073,42 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, >> > return val; >> > } >> > >> > +#ifdef CONFIG_PLUGIN >> > + >> > +typedef struct SavedIOTLB { >> > + struct rcu_head rcu; >> > + hwaddr addr; >> > + MemoryRegionSection *section; >> > + hwaddr mr_offset; >> > +} SavedIOTLB; >> > + >> > +/* >> > + * Save a potentially trashed IOTLB entry for later lookup by plugin. >> > + * >> > + * We also need to track the thread storage address because the RCU >> > + * cleanup that runs when we leave the critical region (the current >> > + * execution) is actually in a different thread. >> > + */ >> > +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection *section, hwaddr mr_offset) >> >> Overlong line. >> >> > +{ >> > + SavedIOTLB *old, *new = g_new(SavedIOTLB, 1); >> > + new->addr = addr; >> > + new->section = section; >> > + new->mr_offset = mr_offset; >> > + old = atomic_rcu_read(&cs->saved_iotlb); >> > + atomic_rcu_set(&cs->saved_iotlb, new); >> > + if (old) { >> > + g_free_rcu(old, rcu); >> > + } >> > +} >> >> I'm a bit confused by this. Why all the multiple allocation? How many >> consumers are you expecting, and more are you expecting multiple memory >> operations in flight at once? >> >> If multiple memory operations in flight, then why aren't we chaining them >> together, so that you can search through multiple alternatives. >> >> If only one memory operation in flight, why are you allocating memory at all, >> much less managing it with rcu? Just put one structure (or a collection of >> fields) into CPUState and be done. > > Oh I just saw this reply. I subscribe all of the above, please shelve my R-b > tag until these are resolved. I was just conscious the data is not always valid - clearing it up with RCU at least ensured it went away eventually. However we could certainly just park it in the general CPUState and assert the match on the fall through case of tlb_plugin_lookup. My only worry is do we ever see us storing data that is valid but the re-filled data in the real TLB could still match and be incorrect? > An alternative is to emit the hwaddr directly in the mem_cb -- IIRC this is > how I did it originally. The API is a larger/uglier (plugins can subscribe > to either hwaddr or vaddr callbacks) but there is no state to keep and > no overhead of calling several functions in a hot path. I think that is certainly an option for evolving the API but I'd rather just get this fix in for now while we ponder what else will be in v2 of the API. So far my proposal for qemu_plugin_hwaddr_device_name has some mild push back from Peter which I'd like to resolve first: Date: Wed, 3 Jun 2020 16:48:33 +0100 Message-ID: <CAFEAcA-kPZoumxfLgjxPfCPDmPgsAFCjB-zdicsiGeqSOPOH7Q@mail.gmail.com> So far I've been adding stuff to solve any particular problems I've had and I'd like to see what other API/hooks are being proposed so we don't have browser style version ticks ;-) > > Thanks, > E. -- Alex Bennée
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index b3f4b7931823..bedbf098dc57 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -417,7 +417,11 @@ struct CPUState { DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX); +#ifdef CONFIG_PLUGIN GArray *plugin_mem_cbs; + /* saved iotlb data from io_writex */ + SavedIOTLB *saved_iotlb; +#endif /* TODO Move common fields from CPUArchState here. */ int cpu_index; diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index 15f5047bf1dc..427027a9707a 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -116,6 +116,7 @@ typedef struct QObject QObject; typedef struct QString QString; typedef struct RAMBlock RAMBlock; typedef struct Range Range; +typedef struct SavedIOTLB SavedIOTLB; typedef struct SHPCDevice SHPCDevice; typedef struct SSIBus SSIBus; typedef struct VirtIODevice VirtIODevice; diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 1e815357c709..8636b66e036a 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1073,6 +1073,42 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry, return val; } +#ifdef CONFIG_PLUGIN + +typedef struct SavedIOTLB { + struct rcu_head rcu; + hwaddr addr; + MemoryRegionSection *section; + hwaddr mr_offset; +} SavedIOTLB; + +/* + * Save a potentially trashed IOTLB entry for later lookup by plugin. + * + * We also need to track the thread storage address because the RCU + * cleanup that runs when we leave the critical region (the current + * execution) is actually in a different thread. + */ +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection *section, hwaddr mr_offset) +{ + SavedIOTLB *old, *new = g_new(SavedIOTLB, 1); + new->addr = addr; + new->section = section; + new->mr_offset = mr_offset; + old = atomic_rcu_read(&cs->saved_iotlb); + atomic_rcu_set(&cs->saved_iotlb, new); + if (old) { + g_free_rcu(old, rcu); + } +} + +#else +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection *section, hwaddr mr_offset) +{ + /* do nothing */ +} +#endif + static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, int mmu_idx, uint64_t val, target_ulong addr, uintptr_t retaddr, MemOp op) @@ -1092,6 +1128,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry, } cpu->mem_io_pc = retaddr; + /* + * The memory_region_dispatch may trigger a flush/resize + * so for plugins we save the iotlb_data just in case. + */ + save_iotlb_data(cpu, iotlbentry->addr, section, mr_offset); + if (mr->global_locking && !qemu_mutex_iothread_locked()) { qemu_mutex_lock_iothread(); locked = true; @@ -1381,8 +1423,11 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr, * in the softmmu lookup code (or helper). We don't handle re-fills or * checking the victim table. This is purely informational. * - * This should never fail as the memory access being instrumented - * should have just filled the TLB. + * This almost never fails as the memory access being instrumented + * should have just filled the TLB. The one corner case is io_writex + * which can cause TLB flushes and potential resizing of the TLBs + * loosing the information we need. In those cases we need to recover + * data from a copy of the io_tlb entry. */ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx, @@ -1406,6 +1451,14 @@ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx, data->v.ram.hostaddr = addr + tlbe->addend; } return true; + } else { + SavedIOTLB *saved = atomic_rcu_read(&cpu->saved_iotlb); + if (saved && saved->addr == tlb_addr) { + data->is_io = true; + data->v.io.section = saved->section; + data->v.io.offset = saved->mr_offset; + return true; + } } return false; }
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. To prevent this we save the IOTLB data in case it is later needed by a plugin doing a lookup. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- v2 - save the entry instead of re-running the tlb_fill. v3 - don't abuse TLS, use CPUState to store data - just use g_free_rcu() to avoid ugliness - verify addr matches before returning data - ws fix --- include/hw/core/cpu.h | 4 +++ include/qemu/typedefs.h | 1 + accel/tcg/cputlb.c | 57 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 60 insertions(+), 2 deletions(-) -- 2.20.1