* [PATCH v1 01/13] docs/devel: convert and update MTTCG design document
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-11 20:38 ` Emilio G. Cota
2020-07-09 14:13 ` [PATCH v1 02/13] docs/devel: add some notes on tcg-icount for developers Alex Bennée
` (11 subsequent siblings)
12 siblings, 1 reply; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, aaron, cota, kuhn.chenqun, peter.puhov,
aurelien
Do a light conversion to .rst and clean-up some of the language at the
start now MTTCG has been merged for a while.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
docs/devel/index.rst | 1 +
...ti-thread-tcg.txt => multi-thread-tcg.rst} | 52 ++++++++++++-------
2 files changed, 34 insertions(+), 19 deletions(-)
rename docs/devel/{multi-thread-tcg.txt => multi-thread-tcg.rst} (90%)
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index bb8238c5d6de..4ecaea3643fb 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -23,6 +23,7 @@ Contents:
decodetree
secure-coding-practices
tcg
+ multi-thread-tcg
tcg-plugins
bitops
reset
diff --git a/docs/devel/multi-thread-tcg.txt b/docs/devel/multi-thread-tcg.rst
similarity index 90%
rename from docs/devel/multi-thread-tcg.txt
rename to docs/devel/multi-thread-tcg.rst
index 3c85ac0eab9b..42158b77c707 100644
--- a/docs/devel/multi-thread-tcg.txt
+++ b/docs/devel/multi-thread-tcg.rst
@@ -1,15 +1,17 @@
-Copyright (c) 2015-2016 Linaro Ltd.
+..
+ Copyright (c) 2015-2020 Linaro Ltd.
-This work is licensed under the terms of the GNU GPL, version 2 or
-later. See the COPYING file in the top-level directory.
+ This work is licensed under the terms of the GNU GPL, version 2 or
+ later. See the COPYING file in the top-level directory.
Introduction
============
-This document outlines the design for multi-threaded TCG system-mode
-emulation. The current user-mode emulation mirrors the thread
-structure of the translated executable. Some of the work will be
-applicable to both system and linux-user emulation.
+This document outlines the design for multi-threaded TCG (a.k.a MTTCG)
+system-mode emulation. user-mode emulation has always mirrored the
+thread structure of the translated executable although some of the
+changes done for MTTCG system emulation have improved the stability of
+linux-user emulation.
The original system-mode TCG implementation was single threaded and
dealt with multiple CPUs with simple round-robin scheduling. This
@@ -21,9 +23,18 @@ vCPU Scheduling
===============
We introduce a new running mode where each vCPU will run on its own
-user-space thread. This will be enabled by default for all FE/BE
-combinations that have had the required work done to support this
-safely.
+user-space thread. This is enabled by default for all FE/BE
+combinations where the host memory model is able to accommodate the
+guest (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO is zero) and the
+guest has had the required work done to support this safely
+(TARGET_SUPPORTS_MTTCG).
+
+System emulation will fall back to the original round robin approach
+if:
+
+* forced by --accel tcg,thread=single
+* enabling --icount mode
+* 64 bit guests on 32 bit hosts (TCG_OVERSIZED_GUEST)
In the general case of running translated code there should be no
inter-vCPU dependencies and all vCPUs should be able to run at full
@@ -61,7 +72,9 @@ have their block-to-block jumps patched.
Global TCG State
----------------
-### User-mode emulation
+User-mode emulation
+~~~~~~~~~~~~~~~~~~~
+
We need to protect the entire code generation cycle including any post
generation patching of the translated code. This also implies a shared
translation buffer which contains code running on all cores. Any
@@ -78,9 +91,11 @@ patching.
Code generation is serialised with mmap_lock().
-### !User-mode emulation
+!User-mode emulation
+~~~~~~~~~~~~~~~~~~~~
+
Each vCPU has its own TCG context and associated TCG region, thereby
-requiring no locking.
+requiring no locking during translation.
Translation Blocks
------------------
@@ -92,6 +107,7 @@ including:
- debugging operations (breakpoint insertion/removal)
- some CPU helper functions
+ - linux-user spawning it's first thread
This is done with the async_safe_run_on_cpu() mechanism to ensure all
vCPUs are quiescent when changes are being made to shared global
@@ -250,8 +266,10 @@ to enforce a particular ordering of memory operations from the point
of view of external observers (e.g. another processor core). They can
apply to any memory operations as well as just loads or stores.
-The Linux kernel has an excellent write-up on the various forms of
-memory barrier and the guarantees they can provide [1].
+The Linux kernel has an excellent `write-up
+<https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/Documentation/memory-barriers.txt>`
+on the various forms of memory barrier and the guarantees they can
+provide.
Barriers are often wrapped around synchronisation primitives to
provide explicit memory ordering semantics. However they can be used
@@ -352,7 +370,3 @@ an exclusive lock which ensures all emulation is serialised.
While the atomic helpers look good enough for now there may be a need
to look at solutions that can more closely model the guest
architectures semantics.
-
-==========
-
-[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/Documentation/memory-barriers.txt
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 01/13] docs/devel: convert and update MTTCG design document
2020-07-09 14:13 ` [PATCH v1 01/13] docs/devel: convert and update MTTCG design document Alex Bennée
@ 2020-07-11 20:38 ` Emilio G. Cota
0 siblings, 0 replies; 37+ messages in thread
From: Emilio G. Cota @ 2020-07-11 20:38 UTC (permalink / raw)
To: Alex Bennée
Cc: fam, berrange, robert.foley, richard.henderson, qemu-devel,
robhenry, f4bug, aaron, kuhn.chenqun, peter.puhov, aurelien
On Thu, Jul 09, 2020 at 15:13:15 +0100, Alex Bennée wrote:
> @@ -92,6 +107,7 @@ including:
>
> - debugging operations (breakpoint insertion/removal)
> - some CPU helper functions
> + - linux-user spawning it's first thread
s/it's/its/
Reviewed-by: Emilio G. Cota <cota@braap.org>
Thanks,
E.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 02/13] docs/devel: add some notes on tcg-icount for developers
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
2020-07-09 14:13 ` [PATCH v1 01/13] docs/devel: convert and update MTTCG design document Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-11 20:41 ` Emilio G. Cota
2020-07-09 14:13 ` [PATCH v1 03/13] docs: Add to gdbstub documentation the PhyMemMode Alex Bennée
` (10 subsequent siblings)
12 siblings, 1 reply; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, Peter Maydell, berrange, robert.foley, Paolo Bonzini,
Alex Bennée, richard.henderson, f4bug, robhenry, aaron,
cota, Pavel Dovgalyuk, kuhn.chenqun, peter.puhov, aurelien
This attempts to bring together my understanding of the requirements
for icount behaviour into one reference document for our developer
notes.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Pavel Dovgalyuk <dovgaluk@ispras.ru>
Cc: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20200619135844.23307-1-alex.bennee@linaro.org>
---
v2
- fix copyright date
- it's -> its
- drop mentioned of gen_io_end()
- remove and correct original conjecture
v3
- include link in index
v4
- Grammar fixes from Peter
- re-worded final section
---
docs/devel/index.rst | 1 +
docs/devel/tcg-icount.rst | 97 +++++++++++++++++++++++++++++++++++++++
2 files changed, 98 insertions(+)
create mode 100644 docs/devel/tcg-icount.rst
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 4ecaea3643fb..ae6eac7c9c66 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -23,6 +23,7 @@ Contents:
decodetree
secure-coding-practices
tcg
+ tcg-icount
multi-thread-tcg
tcg-plugins
bitops
diff --git a/docs/devel/tcg-icount.rst b/docs/devel/tcg-icount.rst
new file mode 100644
index 000000000000..8d67b6c076a7
--- /dev/null
+++ b/docs/devel/tcg-icount.rst
@@ -0,0 +1,97 @@
+..
+ Copyright (c) 2020, Linaro Limited
+ Written by Alex Bennée
+
+
+========================
+TCG Instruction Counting
+========================
+
+TCG has long supported a feature known as icount which allows for
+instruction counting during execution. This should not be confused
+with cycle accurate emulation - QEMU does not attempt to emulate how
+long an instruction would take on real hardware. That is a job for
+other more detailed (and slower) tools that simulate the rest of a
+micro-architecture.
+
+This feature is only available for system emulation and is
+incompatible with multi-threaded TCG. It can be used to better align
+execution time with wall-clock time so a "slow" device doesn't run too
+fast on modern hardware. It can also provides for a degree of
+deterministic execution and is an essential part of the record/replay
+support in QEMU.
+
+Core Concepts
+=============
+
+At its heart icount is simply a count of executed instructions which
+is stored in the TimersState of QEMU's timer sub-system. The number of
+executed instructions can then be used to calculate QEMU_CLOCK_VIRTUAL
+which represents the amount of elapsed time in the system since
+execution started. Depending on the icount mode this may either be a
+fixed number of ns per instruction or adjusted as execution continues
+to keep wall clock time and virtual time in sync.
+
+To be able to calculate the number of executed instructions the
+translator starts by allocating a budget of instructions to be
+executed. The budget of instructions is limited by how long it will be
+until the next timer will expire. We store this budget as part of a
+vCPU icount_decr field which shared with the machinery for handling
+cpu_exit(). The whole field is checked at the start of every
+translated block and will cause a return to the outer loop to deal
+with whatever caused the exit.
+
+In the case of icount, before the flag is checked we subtract the
+number of instructions the translation block would execute. If this
+would cause the instruction budget to go negative we exit the main
+loop and regenerate a new translation block with exactly the right
+number of instructions to take the budget to 0 meaning whatever timer
+was due to expire will expire exactly when we exit the main run loop.
+
+Dealing with MMIO
+-----------------
+
+While we can adjust the instruction budget for known events like timer
+expiry we cannot do the same for MMIO. Every load/store we execute
+might potentially trigger an I/O event, at which point we will need an
+up to date and accurate reading of the icount number.
+
+To deal with this case, when an I/O access is made we:
+
+ - restore un-executed instructions to the icount budget
+ - re-compile a single [1]_ instruction block for the current PC
+ - exit the cpu loop and execute the re-compiled block
+
+The new block is created with the CF_LAST_IO compile flag which
+ensures the final instruction translation starts with a call to
+gen_io_start() so we don't enter a perpetual loop constantly
+recompiling a single instruction block. For translators using the
+common translator_loop this is done automatically.
+
+.. [1] sometimes two instructions if dealing with delay slots
+
+Other I/O operations
+--------------------
+
+MMIO isn't the only type of operation for which we might need a
+correct and accurate clock. IO port instructions and accesses to
+system registers are the common examples here. These instructions have
+to be handled by the individual translators which have the knowledge
+of which operations are I/O operations.
+
+When the translator is handling an instruction of this kind:
+
+* it must call gen_io_start() if icount is enabled, at some
+ point before the generation of the code which actually does
+ the I/O, using a code fragment similar to:
+
+.. code:: c
+
+ if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
+ gen_io_start();
+ }
+
+* it must end the TB immediately after this instruction
+
+Note that some older front-ends call a "gen_io_end()" function:
+this is obsolete and should not be used.
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 02/13] docs/devel: add some notes on tcg-icount for developers
2020-07-09 14:13 ` [PATCH v1 02/13] docs/devel: add some notes on tcg-icount for developers Alex Bennée
@ 2020-07-11 20:41 ` Emilio G. Cota
0 siblings, 0 replies; 37+ messages in thread
From: Emilio G. Cota @ 2020-07-11 20:41 UTC (permalink / raw)
To: Alex Bennée
Cc: fam, Peter Maydell, berrange, robert.foley, richard.henderson,
qemu-devel, robhenry, f4bug, aaron, Paolo Bonzini,
Pavel Dovgalyuk, kuhn.chenqun, peter.puhov, aurelien
On Thu, Jul 09, 2020 at 15:13:16 +0100, Alex Bennée wrote:
> This attempts to bring together my understanding of the requirements
> for icount behaviour into one reference document for our developer
> notes.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Emilio G. Cota <cota@braap.org>
Thanks,
Emilio
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 03/13] docs: Add to gdbstub documentation the PhyMemMode
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
2020-07-09 14:13 ` [PATCH v1 01/13] docs/devel: convert and update MTTCG design document Alex Bennée
2020-07-09 14:13 ` [PATCH v1 02/13] docs/devel: add some notes on tcg-icount for developers Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-09 14:40 ` Philippe Mathieu-Daudé
2020-07-09 14:13 ` [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset Alex Bennée
` (9 subsequent siblings)
12 siblings, 1 reply; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, aaron, cota, kuhn.chenqun, peter.puhov,
aurelien, Jon Doron
From: Jon Doron <arilou@gmail.com>
The PhyMemMode gdb extension command was missing from the gdb.rst
document.
Signed-off-by: Jon Doron <arilou@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200601171609.1665397-1-arilou@gmail.com>
---
docs/system/gdb.rst | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/docs/system/gdb.rst b/docs/system/gdb.rst
index a40145fcf849..abda961e2b49 100644
--- a/docs/system/gdb.rst
+++ b/docs/system/gdb.rst
@@ -87,3 +87,23 @@ three commands you can query and set the single step behavior:
(gdb) maintenance packet Qqemu.sstep=0x5
sending: "qemu.sstep=0x5"
received: "OK"
+
+
+Another feature that QEMU gdbstub provides is to toggle the memory GDB
+works with, by default GDB will show the current process memory respecting
+the virtual address translation.
+
+If you want to examine/change the physical memory you can set the gdbstub
+to work with the physical memory rather with the virtual one.
+
+The memory mode can be checked by sending the following command:
+
+``maintenance packet qqemu.PhyMemMode``
+ This will return either 0 or 1, 1 indicates you are currently in the
+ physical memory mode.
+
+``maintenance packet Qqemu.PhyMemMode:1``
+ This will change the memory mode to physical memory.
+
+``maintenance packet Qqemu.PhyMemMode:0``
+ This will change it back to normal memory mode.
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 03/13] docs: Add to gdbstub documentation the PhyMemMode
2020-07-09 14:13 ` [PATCH v1 03/13] docs: Add to gdbstub documentation the PhyMemMode Alex Bennée
@ 2020-07-09 14:40 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 37+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-09 14:40 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: fam, berrange, robert.foley, richard.henderson, robhenry, aaron,
cota, peter.puhov, kuhn.chenqun, aurelien, Jon Doron
On 7/9/20 4:13 PM, Alex Bennée wrote:
> From: Jon Doron <arilou@gmail.com>
>
> The PhyMemMode gdb extension command was missing from the gdb.rst
> document.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20200601171609.1665397-1-arilou@gmail.com>
> ---
> docs/system/gdb.rst | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/docs/system/gdb.rst b/docs/system/gdb.rst
> index a40145fcf849..abda961e2b49 100644
> --- a/docs/system/gdb.rst
> +++ b/docs/system/gdb.rst
> @@ -87,3 +87,23 @@ three commands you can query and set the single step behavior:
> (gdb) maintenance packet Qqemu.sstep=0x5
> sending: "qemu.sstep=0x5"
> received: "OK"
> +
> +
> +Another feature that QEMU gdbstub provides is to toggle the memory GDB
> +works with,
Maybe start a new sentence?
Otherwise looks good:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> by default GDB will show the current process memory respecting
> +the virtual address translation.
> +
> +If you want to examine/change the physical memory you can set the gdbstub
> +to work with the physical memory rather with the virtual one.
> +
> +The memory mode can be checked by sending the following command:
> +
> +``maintenance packet qqemu.PhyMemMode``
> + This will return either 0 or 1, 1 indicates you are currently in the
> + physical memory mode.
> +
> +``maintenance packet Qqemu.PhyMemMode:1``
> + This will change the memory mode to physical memory.
> +
> +``maintenance packet Qqemu.PhyMemMode:0``
> + This will change it back to normal memory mode.
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (2 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 03/13] docs: Add to gdbstub documentation the PhyMemMode Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-10 21:03 ` Richard Henderson
2020-07-11 21:10 ` Emilio G. Cota
2020-07-09 14:13 ` [PATCH v1 05/13] hw/virtio/pci: include vdev name in registered PCI sections Alex Bennée
` (8 subsequent siblings)
12 siblings, 2 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Paolo Bonzini, Alex Bennée,
richard.henderson, f4bug, robhenry, aaron, cota, kuhn.chenqun,
peter.puhov, Eduardo Habkost, aurelien, Richard Henderson
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)
+{
+ 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;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset
2020-07-09 14:13 ` [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset Alex Bennée
@ 2020-07-10 21:03 ` Richard Henderson
2020-07-11 21:30 ` Emilio G. Cota
2020-07-11 21:10 ` Emilio G. Cota
1 sibling, 1 reply; 37+ messages in thread
From: Richard Henderson @ 2020-07-10 21:03 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: fam, berrange, robert.foley, Paolo Bonzini, f4bug, robhenry,
aaron, cota, kuhn.chenqun, peter.puhov, Eduardo Habkost,
aurelien, Richard Henderson
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~
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset
2020-07-10 21:03 ` Richard Henderson
@ 2020-07-11 21:30 ` Emilio G. Cota
2020-07-12 9:58 ` Alex Bennée
0 siblings, 1 reply; 37+ messages in thread
From: Emilio G. Cota @ 2020-07-11 21:30 UTC (permalink / raw)
To: Richard Henderson
Cc: fam, berrange, robert.foley, Alex Bennée, qemu-devel,
robhenry, f4bug, aaron, Paolo Bonzini, kuhn.chenqun, peter.puhov,
Eduardo Habkost, aurelien, Richard Henderson
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.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset
2020-07-11 21:30 ` Emilio G. Cota
@ 2020-07-12 9:58 ` Alex Bennée
0 siblings, 0 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-12 9:58 UTC (permalink / raw)
To: Emilio G. Cota
Cc: fam, Peter Maydell, berrange, robert.foley, Richard Henderson,
qemu-devel, robhenry, f4bug, aaron, Paolo Bonzini, kuhn.chenqun,
peter.puhov, Eduardo Habkost, aurelien, Richard Henderson
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset
2020-07-09 14:13 ` [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset Alex Bennée
2020-07-10 21:03 ` Richard Henderson
@ 2020-07-11 21:10 ` Emilio G. Cota
1 sibling, 0 replies; 37+ messages in thread
From: Emilio G. Cota @ 2020-07-11 21:10 UTC (permalink / raw)
To: Alex Bennée
Cc: fam, berrange, robert.foley, richard.henderson, qemu-devel,
robhenry, f4bug, aaron, Paolo Bonzini, kuhn.chenqun, peter.puhov,
Eduardo Habkost, aurelien, Richard Henderson
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 05/13] hw/virtio/pci: include vdev name in registered PCI sections
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (3 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-27 13:32 ` Michael S. Tsirkin
2020-07-09 14:13 ` [PATCH v1 06/13] plugins: add API to return a name for a IO device Alex Bennée
` (7 subsequent siblings)
12 siblings, 1 reply; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Michael S . Tsirkin,
Alex Bennée, richard.henderson, f4bug, robhenry,
Philippe Mathieu-Daudé,
aaron, cota, kuhn.chenqun, peter.puhov, aurelien
When viewing/debugging memory regions it is sometimes hard to figure
out which PCI device something belongs to. Make the names unique by
including the vdev name in the name string.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
v2
- swap ()'s for an extra -
---
hw/virtio/virtio-pci.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 8554cf2a038e..215e680c71f4 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1406,7 +1406,8 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr,
}
}
-static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
+static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy,
+ const char *vdev_name)
{
static const MemoryRegionOps common_ops = {
.read = virtio_pci_common_read,
@@ -1453,36 +1454,41 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
},
.endianness = DEVICE_LITTLE_ENDIAN,
};
+ g_autoptr(GString) name = g_string_new(NULL);
-
+ g_string_printf(name, "virtio-pci-common-%s", vdev_name);
memory_region_init_io(&proxy->common.mr, OBJECT(proxy),
&common_ops,
proxy,
- "virtio-pci-common",
+ name->str,
proxy->common.size);
+ g_string_printf(name, "virtio-pci-isr-%s", vdev_name);
memory_region_init_io(&proxy->isr.mr, OBJECT(proxy),
&isr_ops,
proxy,
- "virtio-pci-isr",
+ name->str,
proxy->isr.size);
+ g_string_printf(name, "virtio-pci-device-%s", vdev_name);
memory_region_init_io(&proxy->device.mr, OBJECT(proxy),
&device_ops,
virtio_bus_get_device(&proxy->bus),
- "virtio-pci-device",
+ name->str,
proxy->device.size);
+ g_string_printf(name, "virtio-pci-notify-%s", vdev_name);
memory_region_init_io(&proxy->notify.mr, OBJECT(proxy),
¬ify_ops,
virtio_bus_get_device(&proxy->bus),
- "virtio-pci-notify",
+ name->str,
proxy->notify.size);
+ g_string_printf(name, "virtio-pci-notify-pio-%s", vdev_name);
memory_region_init_io(&proxy->notify_pio.mr, OBJECT(proxy),
¬ify_pio_ops,
virtio_bus_get_device(&proxy->bus),
- "virtio-pci-notify-pio",
+ name->str,
proxy->notify_pio.size);
}
@@ -1623,7 +1629,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
struct virtio_pci_cfg_cap *cfg_mask;
- virtio_pci_modern_regions_init(proxy);
+ virtio_pci_modern_regions_init(proxy, vdev->name);
virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap);
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 05/13] hw/virtio/pci: include vdev name in registered PCI sections
2020-07-09 14:13 ` [PATCH v1 05/13] hw/virtio/pci: include vdev name in registered PCI sections Alex Bennée
@ 2020-07-27 13:32 ` Michael S. Tsirkin
2020-07-27 16:22 ` Alex Bennée
0 siblings, 1 reply; 37+ messages in thread
From: Michael S. Tsirkin @ 2020-07-27 13:32 UTC (permalink / raw)
To: Alex Bennée
Cc: fam, berrange, robert.foley, Philippe Mathieu-Daudé,
richard.henderson, qemu-devel, robhenry, f4bug, aaron, cota,
kuhn.chenqun, peter.puhov, aurelien
On Thu, Jul 09, 2020 at 03:13:19PM +0100, Alex Bennée wrote:
> When viewing/debugging memory regions it is sometimes hard to figure
> out which PCI device something belongs to. Make the names unique by
> including the vdev name in the name string.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
So I don't know what's the plan here. I think ideally core would
just do it for us automatically. Why not?
If it can't my ack stands, anyway, pls
merge with rest of the patches if that is deemed appropriate.
> ---
> v2
> - swap ()'s for an extra -
> ---
> hw/virtio/virtio-pci.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 8554cf2a038e..215e680c71f4 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1406,7 +1406,8 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr,
> }
> }
>
> -static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
> +static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy,
> + const char *vdev_name)
> {
> static const MemoryRegionOps common_ops = {
> .read = virtio_pci_common_read,
> @@ -1453,36 +1454,41 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
> },
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
> + g_autoptr(GString) name = g_string_new(NULL);
>
> -
> + g_string_printf(name, "virtio-pci-common-%s", vdev_name);
> memory_region_init_io(&proxy->common.mr, OBJECT(proxy),
> &common_ops,
> proxy,
> - "virtio-pci-common",
> + name->str,
> proxy->common.size);
>
> + g_string_printf(name, "virtio-pci-isr-%s", vdev_name);
> memory_region_init_io(&proxy->isr.mr, OBJECT(proxy),
> &isr_ops,
> proxy,
> - "virtio-pci-isr",
> + name->str,
> proxy->isr.size);
>
> + g_string_printf(name, "virtio-pci-device-%s", vdev_name);
> memory_region_init_io(&proxy->device.mr, OBJECT(proxy),
> &device_ops,
> virtio_bus_get_device(&proxy->bus),
> - "virtio-pci-device",
> + name->str,
> proxy->device.size);
>
> + g_string_printf(name, "virtio-pci-notify-%s", vdev_name);
> memory_region_init_io(&proxy->notify.mr, OBJECT(proxy),
> ¬ify_ops,
> virtio_bus_get_device(&proxy->bus),
> - "virtio-pci-notify",
> + name->str,
> proxy->notify.size);
>
> + g_string_printf(name, "virtio-pci-notify-pio-%s", vdev_name);
> memory_region_init_io(&proxy->notify_pio.mr, OBJECT(proxy),
> ¬ify_pio_ops,
> virtio_bus_get_device(&proxy->bus),
> - "virtio-pci-notify-pio",
> + name->str,
> proxy->notify_pio.size);
> }
>
> @@ -1623,7 +1629,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>
> struct virtio_pci_cfg_cap *cfg_mask;
>
> - virtio_pci_modern_regions_init(proxy);
> + virtio_pci_modern_regions_init(proxy, vdev->name);
>
> virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
> virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap);
> --
> 2.20.1
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v1 05/13] hw/virtio/pci: include vdev name in registered PCI sections
2020-07-27 13:32 ` Michael S. Tsirkin
@ 2020-07-27 16:22 ` Alex Bennée
0 siblings, 0 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-27 16:22 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: fam, berrange, robert.foley, Philippe Mathieu-Daudé,
richard.henderson, qemu-devel, robhenry, f4bug, aaron, cota,
kuhn.chenqun, peter.puhov, aurelien
Michael S. Tsirkin <mst@redhat.com> writes:
> On Thu, Jul 09, 2020 at 03:13:19PM +0100, Alex Bennée wrote:
>> When viewing/debugging memory regions it is sometimes hard to figure
>> out which PCI device something belongs to. Make the names unique by
>> including the vdev name in the name string.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> So I don't know what's the plan here. I think ideally core would
> just do it for us automatically. Why not?
> If it can't my ack stands, anyway, pls
> merge with rest of the patches if that is deemed appropriate.
Yeah currently it's sitting in my plugins/next queue which isn't
targeting 5.1. As the hwprofile plugin is the only one I know using it I
figured I'd keep it with those.
>
>> ---
>> v2
>> - swap ()'s for an extra -
>> ---
>> hw/virtio/virtio-pci.c | 22 ++++++++++++++--------
>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 8554cf2a038e..215e680c71f4 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1406,7 +1406,8 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr,
>> }
>> }
>>
>> -static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
>> +static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy,
>> + const char *vdev_name)
>> {
>> static const MemoryRegionOps common_ops = {
>> .read = virtio_pci_common_read,
>> @@ -1453,36 +1454,41 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
>> },
>> .endianness = DEVICE_LITTLE_ENDIAN,
>> };
>> + g_autoptr(GString) name = g_string_new(NULL);
>>
>> -
>> + g_string_printf(name, "virtio-pci-common-%s", vdev_name);
>> memory_region_init_io(&proxy->common.mr, OBJECT(proxy),
>> &common_ops,
>> proxy,
>> - "virtio-pci-common",
>> + name->str,
>> proxy->common.size);
>>
>> + g_string_printf(name, "virtio-pci-isr-%s", vdev_name);
>> memory_region_init_io(&proxy->isr.mr, OBJECT(proxy),
>> &isr_ops,
>> proxy,
>> - "virtio-pci-isr",
>> + name->str,
>> proxy->isr.size);
>>
>> + g_string_printf(name, "virtio-pci-device-%s", vdev_name);
>> memory_region_init_io(&proxy->device.mr, OBJECT(proxy),
>> &device_ops,
>> virtio_bus_get_device(&proxy->bus),
>> - "virtio-pci-device",
>> + name->str,
>> proxy->device.size);
>>
>> + g_string_printf(name, "virtio-pci-notify-%s", vdev_name);
>> memory_region_init_io(&proxy->notify.mr, OBJECT(proxy),
>> ¬ify_ops,
>> virtio_bus_get_device(&proxy->bus),
>> - "virtio-pci-notify",
>> + name->str,
>> proxy->notify.size);
>>
>> + g_string_printf(name, "virtio-pci-notify-pio-%s", vdev_name);
>> memory_region_init_io(&proxy->notify_pio.mr, OBJECT(proxy),
>> ¬ify_pio_ops,
>> virtio_bus_get_device(&proxy->bus),
>> - "virtio-pci-notify-pio",
>> + name->str,
>> proxy->notify_pio.size);
>> }
>>
>> @@ -1623,7 +1629,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>>
>> struct virtio_pci_cfg_cap *cfg_mask;
>>
>> - virtio_pci_modern_regions_init(proxy);
>> + virtio_pci_modern_regions_init(proxy, vdev->name);
>>
>> virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
>> virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap);
>> --
>> 2.20.1
--
Alex Bennée
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 06/13] plugins: add API to return a name for a IO device
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (4 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 05/13] hw/virtio/pci: include vdev name in registered PCI sections Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-09 15:03 ` Philippe Mathieu-Daudé
2020-07-09 14:13 ` [PATCH v1 07/13] plugins: new hwprofile plugin Alex Bennée
` (6 subsequent siblings)
12 siblings, 1 reply; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, aaron, cota, kuhn.chenqun, peter.puhov,
Clement Deschamps, aurelien
This may well end up being anonymous but it should always be unique.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
[r-b provisional given change to g_intern_string]
Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>
Reviewed-by: Emilio G. Cota <cota@braap.org>
---
v3
- return a non-freeable const g_intern_string()
- checkpatch cleanups
---
include/qemu/qemu-plugin.h | 6 ++++++
plugins/api.c | 20 ++++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index bab8b0d4b3af..c98c18d6b052 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -335,6 +335,12 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
+/*
+ * Returns a string representing the device. The string is valid for
+ * the lifetime of the plugin.
+ */
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
+
typedef void
(*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
qemu_plugin_meminfo_t info, uint64_t vaddr,
diff --git a/plugins/api.c b/plugins/api.c
index bbdc5a4eb46d..4304e63f0cf8 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
return 0;
}
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
+{
+#ifdef CONFIG_SOFTMMU
+ if (h && h->is_io) {
+ MemoryRegionSection *mrs = h->v.io.section;
+ if (!mrs->mr->name) {
+ unsigned long maddr = 0xffffffff & (uintptr_t) mrs->mr;
+ g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);
+ return g_intern_string(temp);
+ } else {
+ return g_intern_string(mrs->mr->name);
+ }
+ } else {
+ return g_intern_string("RAM");
+ }
+#else
+ return g_intern_string("Invalid");
+#endif
+}
+
/*
* Queries to the number and potential maximum number of vCPUs there
* will be. This helps the plugin dimension per-vcpu arrays.
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 06/13] plugins: add API to return a name for a IO device
2020-07-09 14:13 ` [PATCH v1 06/13] plugins: add API to return a name for a IO device Alex Bennée
@ 2020-07-09 15:03 ` Philippe Mathieu-Daudé
2020-07-09 16:30 ` Alex Bennée
0 siblings, 1 reply; 37+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-09 15:03 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: fam, Daniel P . Berrange, robert.foley, richard.henderson,
robhenry, aaron, cota, peter.puhov, kuhn.chenqun,
Clement Deschamps, aurelien
On 7/9/20 4:13 PM, Alex Bennée wrote:
> This may well end up being anonymous but it should always be unique.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> [r-b provisional given change to g_intern_string]
> Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>
> Reviewed-by: Emilio G. Cota <cota@braap.org>
>
> ---
> v3
> - return a non-freeable const g_intern_string()
> - checkpatch cleanups
> ---
> include/qemu/qemu-plugin.h | 6 ++++++
> plugins/api.c | 20 ++++++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index bab8b0d4b3af..c98c18d6b052 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -335,6 +335,12 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
> bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
> uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
>
> +/*
> + * Returns a string representing the device. The string is valid for
> + * the lifetime of the plugin.
> + */
> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
> +
> typedef void
> (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
> qemu_plugin_meminfo_t info, uint64_t vaddr,
> diff --git a/plugins/api.c b/plugins/api.c
> index bbdc5a4eb46d..4304e63f0cf8 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
> return 0;
> }
>
> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
> +{
> +#ifdef CONFIG_SOFTMMU
> + if (h && h->is_io) {
> + MemoryRegionSection *mrs = h->v.io.section;
> + if (!mrs->mr->name) {
> + unsigned long maddr = 0xffffffff & (uintptr_t) mrs->mr;
Why not use uint32_t & PRIx32?
uint32_t maddr = (uintptr_t) mrs->mr;
> + g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);
> + return g_intern_string(temp);
Isn't this illegal? temp is definitively not const...
> + } else {
> + return g_intern_string(mrs->mr->name);
> + }
> + } else {
> + return g_intern_string("RAM");
> + }
> +#else
> + return g_intern_string("Invalid");
> +#endif
> +}
> +
> /*
> * Queries to the number and potential maximum number of vCPUs there
> * will be. This helps the plugin dimension per-vcpu arrays.
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v1 06/13] plugins: add API to return a name for a IO device
2020-07-09 15:03 ` Philippe Mathieu-Daudé
@ 2020-07-09 16:30 ` Alex Bennée
0 siblings, 0 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 16:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: fam, Daniel P . Berrange, robert.foley, richard.henderson,
qemu-devel, robhenry, aaron, cota, peter.puhov, kuhn.chenqun,
Clement Deschamps, aurelien
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> On 7/9/20 4:13 PM, Alex Bennée wrote:
>> This may well end up being anonymous but it should always be unique.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> [r-b provisional given change to g_intern_string]
>> Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>
>> Reviewed-by: Emilio G. Cota <cota@braap.org>
>>
>> ---
>> v3
>> - return a non-freeable const g_intern_string()
>> - checkpatch cleanups
>> ---
>> include/qemu/qemu-plugin.h | 6 ++++++
>> plugins/api.c | 20 ++++++++++++++++++++
>> 2 files changed, 26 insertions(+)
>>
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index bab8b0d4b3af..c98c18d6b052 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -335,6 +335,12 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
>> bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
>> uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
>>
>> +/*
>> + * Returns a string representing the device. The string is valid for
>> + * the lifetime of the plugin.
>> + */
>> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
>> +
>> typedef void
>> (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
>> qemu_plugin_meminfo_t info, uint64_t vaddr,
>> diff --git a/plugins/api.c b/plugins/api.c
>> index bbdc5a4eb46d..4304e63f0cf8 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>> return 0;
>> }
>>
>> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
>> +{
>> +#ifdef CONFIG_SOFTMMU
>> + if (h && h->is_io) {
>> + MemoryRegionSection *mrs = h->v.io.section;
>> + if (!mrs->mr->name) {
>> + unsigned long maddr = 0xffffffff & (uintptr_t) mrs->mr;
>
> Why not use uint32_t & PRIx32?
>
> uint32_t maddr = (uintptr_t) mrs->mr;
>
>> + g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);
>> + return g_intern_string(temp);
>
> Isn't this illegal? temp is definitively not const...
We don't mess with it, we return a new string which is the canonical
form.
>
>> + } else {
>> + return g_intern_string(mrs->mr->name);
>> + }
>> + } else {
>> + return g_intern_string("RAM");
>> + }
>> +#else
>> + return g_intern_string("Invalid");
>> +#endif
>> +}
>> +
>> /*
>> * Queries to the number and potential maximum number of vCPUs there
>> * will be. This helps the plugin dimension per-vcpu arrays.
>>
--
Alex Bennée
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 07/13] plugins: new hwprofile plugin
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (5 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 06/13] plugins: add API to return a name for a IO device Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-09 14:13 ` [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu Alex Bennée
` (5 subsequent siblings)
12 siblings, 0 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, Philippe Mathieu-Daudé,
aaron, cota, kuhn.chenqun, peter.puhov, aurelien
This is a plugin intended to help with profiling access to various
bits of system hardware. It only really makes sense for system
emulation.
It takes advantage of the recently exposed helper API that allows us
to see the device name (memory region name) associated with a device.
You can specify arg=read or arg=write to limit the tracking to just
reads or writes (by default it does both).
The pattern option:
-plugin ./tests/plugin/libhwprofile.so,arg=pattern
will allow you to see the access pattern to devices, eg:
gic_cpu @ 0xffffffc010040000
off:00000000, 8, 1, 8, 1
off:00000000, 4, 1, 4, 1
off:00000000, 2, 1, 2, 1
off:00000000, 1, 1, 1, 1
The source option:
-plugin ./tests/plugin/libhwprofile.so,arg=source
will track the virtual source address of the instruction making the
access:
pl011 @ 0xffffffc010031000
pc:ffffffc0104c785c, 1, 4, 0, 0
pc:ffffffc0104c7898, 1, 4, 0, 0
pc:ffffffc010512bcc, 2, 1867, 0, 0
You cannot mix source and pattern.
Finally the match option allow you to limit the tracking to just the
devices you care about.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Robert Foley <robert.foley@linaro.org>
Tested-by: Robert Foley <robert.foley@linaro.org>
---
v2
- use 64 bit cpu masks
- warn if we will exceed cpu mask capability
- don't run in linux-user mode
- skip in check-tcg for linux-user test
v3
- update device name API
- re-factor and clean-up
- add source tracking
- use direct hash for source/pattern
- add match function
- expand the commit message
- checkpatch cleanups
---
tests/plugin/hwprofile.c | 305 ++++++++++++++++++++++++++++++++++++++
tests/plugin/Makefile | 1 +
tests/tcg/Makefile.target | 12 +-
3 files changed, 316 insertions(+), 2 deletions(-)
create mode 100644 tests/plugin/hwprofile.c
diff --git a/tests/plugin/hwprofile.c b/tests/plugin/hwprofile.c
new file mode 100644
index 000000000000..6dac1d5f8541
--- /dev/null
+++ b/tests/plugin/hwprofile.c
@@ -0,0 +1,305 @@
+/*
+ * Copyright (C) 2020, Alex Bennée <alex.bennee@linaro.org>
+ *
+ * HW Profile - breakdown access patterns for IO to devices
+ *
+ * License: GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <inttypes.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <inttypes.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <glib.h>
+
+#include <qemu-plugin.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+typedef struct {
+ uint64_t cpu_read;
+ uint64_t cpu_write;
+ uint64_t reads;
+ uint64_t writes;
+} IOCounts;
+
+typedef struct {
+ uint64_t off_or_pc;
+ IOCounts counts;
+} IOLocationCounts;
+
+typedef struct {
+ const char *name;
+ uint64_t base;
+ IOCounts totals;
+ GHashTable *detail;
+} DeviceCounts;
+
+static GMutex lock;
+static GHashTable *devices;
+
+/* track the access pattern to a piece of HW */
+static bool pattern;
+/* track the source address of access to HW */
+static bool source;
+/* track only matched regions of HW */
+static bool check_match;
+static gchar **matches;
+
+static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
+
+static inline bool track_reads(void)
+{
+ return rw == QEMU_PLUGIN_MEM_RW || rw == QEMU_PLUGIN_MEM_R;
+}
+
+static inline bool track_writes(void)
+{
+ return rw == QEMU_PLUGIN_MEM_RW || rw == QEMU_PLUGIN_MEM_W;
+}
+
+static void plugin_init(void)
+{
+ devices = g_hash_table_new(NULL, NULL);
+}
+
+static gint sort_cmp(gconstpointer a, gconstpointer b)
+{
+ DeviceCounts *ea = (DeviceCounts *) a;
+ DeviceCounts *eb = (DeviceCounts *) b;
+ return ea->totals.reads + ea->totals.writes >
+ eb->totals.reads + eb->totals.writes ? -1 : 1;
+}
+
+static gint sort_loc(gconstpointer a, gconstpointer b)
+{
+ IOLocationCounts *ea = (IOLocationCounts *) a;
+ IOLocationCounts *eb = (IOLocationCounts *) b;
+ return ea->off_or_pc > eb->off_or_pc;
+}
+
+static void fmt_iocount_record(GString *s, IOCounts *rec)
+{
+ if (track_reads()) {
+ g_string_append_printf(s, ", %"PRIx64", %"PRId64,
+ rec->cpu_read, rec->reads);
+ }
+ if (track_writes()) {
+ g_string_append_printf(s, ", %"PRIx64", %"PRId64,
+ rec->cpu_write, rec->writes);
+ }
+}
+
+static void fmt_dev_record(GString *s, DeviceCounts *rec)
+{
+ g_string_append_printf(s, "%s, 0x%"PRIx64,
+ rec->name, rec->base);
+ fmt_iocount_record(s, &rec->totals);
+ g_string_append_c(s, '\n');
+}
+
+static void plugin_exit(qemu_plugin_id_t id, void *p)
+{
+ g_autoptr(GString) report = g_string_new("");
+ GList *counts;
+
+ if (!(pattern || source)) {
+ g_string_printf(report, "Device, Address");
+ if (track_reads()) {
+ g_string_append_printf(report, ", RCPUs, Reads");
+ }
+ if (track_writes()) {
+ g_string_append_printf(report, ", WCPUs, Writes");
+ }
+ g_string_append_c(report, '\n');
+ }
+
+ counts = g_hash_table_get_values(devices);
+ if (counts && g_list_next(counts)) {
+ GList *it;
+
+ it = g_list_sort(counts, sort_cmp);
+
+ while (it) {
+ DeviceCounts *rec = (DeviceCounts *) it->data;
+ if (rec->detail) {
+ GList *accesses = g_hash_table_get_values(rec->detail);
+ GList *io_it = g_list_sort(accesses, sort_loc);
+ const char *prefix = pattern ? "off" : "pc";
+ g_string_append_printf(report, "%s @ 0x%"PRIx64"\n",
+ rec->name, rec->base);
+ while (io_it) {
+ IOLocationCounts *loc = (IOLocationCounts *) io_it->data;
+ g_string_append_printf(report, " %s:%08"PRIx64,
+ prefix, loc->off_or_pc);
+ fmt_iocount_record(report, &loc->counts);
+ g_string_append_c(report, '\n');
+ io_it = io_it->next;
+ }
+ } else {
+ fmt_dev_record(report, rec);
+ }
+ it = it->next;
+ };
+ g_list_free(it);
+ }
+
+ qemu_plugin_outs(report->str);
+}
+
+static DeviceCounts *new_count(const char *name, uint64_t base)
+{
+ DeviceCounts *count = g_new0(DeviceCounts, 1);
+ count->name = name;
+ count->base = base;
+ if (pattern || source) {
+ count->detail = g_hash_table_new(NULL, NULL);
+ }
+ g_hash_table_insert(devices, (gpointer) name, count);
+ return count;
+}
+
+static IOLocationCounts *new_location(GHashTable *table, uint64_t off_or_pc)
+{
+ IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
+ loc->off_or_pc = off_or_pc;
+ g_hash_table_insert(table, (gpointer) off_or_pc, loc);
+ return loc;
+}
+
+static void hwprofile_match_hit(DeviceCounts *rec, uint64_t off)
+{
+ g_autoptr(GString) report = g_string_new("hwprofile: match @ offset");
+ g_string_append_printf(report, "%"PRIx64", previous hits\n", off);
+ fmt_dev_record(report, rec);
+ qemu_plugin_outs(report->str);
+}
+
+static void inc_count(IOCounts *count, bool is_write, unsigned int cpu_index)
+{
+ if (is_write) {
+ count->writes++;
+ count->cpu_write |= (1 << cpu_index);
+ } else {
+ count->reads++;
+ count->cpu_read |= (1 << cpu_index);
+ }
+}
+
+static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
+ uint64_t vaddr, void *udata)
+{
+ struct qemu_plugin_hwaddr *hwaddr = qemu_plugin_get_hwaddr(meminfo, vaddr);
+
+ if (!hwaddr || !qemu_plugin_hwaddr_is_io(hwaddr)) {
+ return;
+ } else {
+ const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
+ uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
+ bool is_write = qemu_plugin_mem_is_store(meminfo);
+ DeviceCounts *counts;
+
+ g_mutex_lock(&lock);
+ counts = (DeviceCounts *) g_hash_table_lookup(devices, name);
+
+ if (!counts) {
+ uint64_t base = vaddr - off;
+ counts = new_count(name, base);
+ }
+
+ if (check_match) {
+ if (g_strv_contains((const char * const *)matches, counts->name)) {
+ hwprofile_match_hit(counts, off);
+ inc_count(&counts->totals, is_write, cpu_index);
+ }
+ } else {
+ inc_count(&counts->totals, is_write, cpu_index);
+ }
+
+ /* either track offsets or source of access */
+ if (source) {
+ off = (uint64_t) udata;
+ }
+
+ if (pattern || source) {
+ IOLocationCounts *io_count = g_hash_table_lookup(counts->detail,
+ (gpointer) off);
+ if (!io_count) {
+ io_count = new_location(counts->detail, off);
+ }
+ inc_count(&io_count->counts, is_write, cpu_index);
+ }
+
+ g_mutex_unlock(&lock);
+ }
+}
+
+static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
+{
+ size_t n = qemu_plugin_tb_n_insns(tb);
+ size_t i;
+
+ for (i = 0; i < n; i++) {
+ struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
+ gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
+ qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
+ QEMU_PLUGIN_CB_NO_REGS,
+ rw, udata);
+ }
+}
+
+QEMU_PLUGIN_EXPORT
+int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
+ int argc, char **argv)
+{
+ int i;
+
+ for (i = 0; i < argc; i++) {
+ char *opt = argv[i];
+ if (g_strcmp0(opt, "read") == 0) {
+ rw = QEMU_PLUGIN_MEM_R;
+ } else if (g_strcmp0(opt, "write") == 0) {
+ rw = QEMU_PLUGIN_MEM_W;
+ } else if (g_strcmp0(opt, "pattern") == 0) {
+ pattern = true;
+ } else if (g_strcmp0(opt, "source") == 0) {
+ source = true;
+ } else if (g_str_has_prefix(opt, "match")) {
+ gchar **parts = g_strsplit(opt, "=", 2);
+ check_match = true;
+ matches = g_strsplit(parts[1], ",", -1);
+ g_strfreev(parts);
+ } else {
+ fprintf(stderr, "option parsing failed: %s\n", opt);
+ return -1;
+ }
+ }
+
+ if (source && pattern) {
+ fprintf(stderr, "can only currently track either source or pattern.\n");
+ return -1;
+ }
+
+ if (!info->system_emulation) {
+ fprintf(stderr, "hwprofile: plugin only useful for system emulation\n");
+ return -1;
+ }
+
+ /* Just warn about overflow */
+ if (info->system.smp_vcpus > 64 ||
+ info->system.max_vcpus > 64) {
+ fprintf(stderr, "hwprofile: can only track up to 64 CPUs\n");
+ }
+
+ plugin_init();
+
+ qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
+ qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
+ return 0;
+}
diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
index 3a50451428b4..0cb8e35ae407 100644
--- a/tests/plugin/Makefile
+++ b/tests/plugin/Makefile
@@ -14,6 +14,7 @@ NAMES += hotblocks
NAMES += howvec
NAMES += hotpages
NAMES += lockstep
+NAMES += hwprofile
SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index 4b2b696fcee3..bc595b058845 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -129,8 +129,16 @@ ifeq ($(CONFIG_PLUGIN),y)
PLUGIN_SRC=$(SRC_PATH)/tests/plugin
PLUGIN_LIB=../../plugin
VPATH+=$(PLUGIN_LIB)
-PLUGINS=$(filter-out liblockstep.so,\
- $(patsubst %.c, lib%.so, $(notdir $(wildcard $(PLUGIN_SRC)/*.c))))
+
+# Some plugins aren't testable automatically
+SKIP_PLUGINS=liblockstep.so
+ifdef CONFIG_USER_ONLY
+SKIP_PLUGINS+=libhwprofile.so
+endif
+
+PLUGINS=$(filter-out $(SKIP_PLUGINS),\
+ $(patsubst %.c, lib%.so, \
+ $(notdir $(wildcard $(PLUGIN_SRC)/*.c))))
# We need to ensure expand the run-plugin-TEST-with-PLUGIN
# pre-requistes manually here as we can't use stems to handle it. We
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (6 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 07/13] plugins: new hwprofile plugin Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-09 17:12 ` Robert Foley
2020-07-11 21:56 ` Emilio G. Cota
2020-07-09 14:13 ` [PATCH v1 09/13] target/sh4: revert to using cpu_lduw_code to decode gusa Alex Bennée
` (4 subsequent siblings)
12 siblings, 2 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, aaron, cota, Dave Bort, kuhn.chenqun,
peter.puhov, aurelien
While there isn't any easy way to make the inline counts thread safe
we can ensure the callback based ones are. While we are at it we can
reduce introduce a new option ("idle") to dump a report of the current
bb and insn count each time a vCPU enters the idle state.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Dave Bort <dbort@dbort.com>
---
v2
- fixup for non-inline linux-user case
- minor cleanup and re-factor
---
tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 83 insertions(+), 13 deletions(-)
diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
index df19fd359df3..89c373e19cd8 100644
--- a/tests/plugin/bb.c
+++ b/tests/plugin/bb.c
@@ -16,24 +16,67 @@
QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
-static uint64_t bb_count;
-static uint64_t insn_count;
+typedef struct {
+ GMutex lock;
+ int index;
+ uint64_t bb_count;
+ uint64_t insn_count;
+} CPUCount;
+
+/* Used by the inline & linux-user counts */
static bool do_inline;
+static CPUCount inline_count;
+
+/* Dump running CPU total on idle? */
+static bool idle_report;
+static GPtrArray *counts;
+static int max_cpus;
+
+static void gen_one_cpu_report(CPUCount *count, GString *report)
+{
+ if (count->bb_count) {
+ g_string_append_printf(report, "CPU%d: "
+ "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+ count->index,
+ count->bb_count, count->insn_count);
+ }
+}
static void plugin_exit(qemu_plugin_id_t id, void *p)
{
- g_autofree gchar *out = g_strdup_printf(
- "bb's: %" PRIu64", insns: %" PRIu64 "\n",
- bb_count, insn_count);
- qemu_plugin_outs(out);
+ g_autoptr(GString) report = g_string_new("");
+
+ if (do_inline || !max_cpus) {
+ g_string_printf(report, "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+ inline_count.bb_count, inline_count.insn_count);
+ } else {
+ g_ptr_array_foreach(counts, (GFunc) gen_one_cpu_report, report);
+ }
+ qemu_plugin_outs(report->str);
+}
+
+static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+ CPUCount *count = g_ptr_array_index(counts, cpu_index);
+ g_autoptr(GString) report = g_string_new("");
+ gen_one_cpu_report(count, report);
+
+ if (report->len > 0) {
+ g_string_prepend(report, "Idling ");
+ qemu_plugin_outs(report->str);
+ }
}
static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
{
- unsigned long n_insns = (unsigned long)udata;
+ CPUCount *count = max_cpus ?
+ g_ptr_array_index(counts, cpu_index) : &inline_count;
- insn_count += n_insns;
- bb_count++;
+ unsigned long n_insns = (unsigned long)udata;
+ g_mutex_lock(&count->lock);
+ count->insn_count += n_insns;
+ count->bb_count++;
+ g_mutex_unlock(&count->lock);
}
static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
@@ -42,9 +85,9 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
if (do_inline) {
qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
- &bb_count, 1);
+ &inline_count.bb_count, 1);
qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
- &insn_count, n_insns);
+ &inline_count.insn_count, n_insns);
} else {
qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
QEMU_PLUGIN_CB_NO_REGS,
@@ -56,8 +99,35 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
const qemu_info_t *info,
int argc, char **argv)
{
- if (argc && strcmp(argv[0], "inline") == 0) {
- do_inline = true;
+ int i;
+
+ for (i = 0; i < argc; i++) {
+ char *opt = argv[i];
+ if (g_strcmp0(opt, "inline") == 0) {
+ do_inline = true;
+ } else if (g_strcmp0(opt, "idle") == 0) {
+ idle_report = true;
+ } else {
+ fprintf(stderr, "option parsing failed: %s\n", opt);
+ return -1;
+ }
+ }
+
+ if (info->system_emulation && !do_inline) {
+ max_cpus = info->system.max_vcpus;
+ counts = g_ptr_array_new();
+ for (i = 0; i < max_cpus; i++) {
+ CPUCount *count = g_new0(CPUCount, 1);
+ g_mutex_init(&count->lock);
+ count->index = i;
+ g_ptr_array_add(counts, count);
+ }
+ } else if (!do_inline) {
+ g_mutex_init(&inline_count.lock);
+ }
+
+ if (idle_report) {
+ qemu_plugin_register_vcpu_idle_cb(id, vcpu_idle);
}
qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu
2020-07-09 14:13 ` [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu Alex Bennée
@ 2020-07-09 17:12 ` Robert Foley
2020-07-11 21:56 ` Emilio G. Cota
1 sibling, 0 replies; 37+ messages in thread
From: Robert Foley @ 2020-07-09 17:12 UTC (permalink / raw)
To: Alex Bennée
Cc: Fam Zheng, Daniel P. Berrangé,
Richard Henderson, QEMU Developers, robhenry, f4bug, aaron,
Emilio G. Cota, Dave Bort, kuhn.chenqun, Peter Puhov,
Aurelien Jarno
Reviewed-by: Robert Foley <robert.foley@linaro.org>
On Thu, 9 Jul 2020 at 10:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> While there isn't any easy way to make the inline counts thread safe
> we can ensure the callback based ones are. While we are at it we can
> reduce introduce a new option ("idle") to dump a report of the current
> bb and insn count each time a vCPU enters the idle state.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Dave Bort <dbort@dbort.com>
>
> ---
> v2
> - fixup for non-inline linux-user case
> - minor cleanup and re-factor
> ---
> tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 83 insertions(+), 13 deletions(-)
>
> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
> index df19fd359df3..89c373e19cd8 100644
> --- a/tests/plugin/bb.c
> +++ b/tests/plugin/bb.c
> @@ -16,24 +16,67 @@
>
> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>
> -static uint64_t bb_count;
> -static uint64_t insn_count;
> +typedef struct {
> + GMutex lock;
> + int index;
> + uint64_t bb_count;
> + uint64_t insn_count;
> +} CPUCount;
> +
> +/* Used by the inline & linux-user counts */
> static bool do_inline;
> +static CPUCount inline_count;
> +
> +/* Dump running CPU total on idle? */
> +static bool idle_report;
> +static GPtrArray *counts;
> +static int max_cpus;
> +
> +static void gen_one_cpu_report(CPUCount *count, GString *report)
> +{
> + if (count->bb_count) {
> + g_string_append_printf(report, "CPU%d: "
> + "bb's: %" PRIu64", insns: %" PRIu64 "\n",
> + count->index,
> + count->bb_count, count->insn_count);
> + }
> +}
>
> static void plugin_exit(qemu_plugin_id_t id, void *p)
> {
> - g_autofree gchar *out = g_strdup_printf(
> - "bb's: %" PRIu64", insns: %" PRIu64 "\n",
> - bb_count, insn_count);
> - qemu_plugin_outs(out);
> + g_autoptr(GString) report = g_string_new("");
> +
> + if (do_inline || !max_cpus) {
> + g_string_printf(report, "bb's: %" PRIu64", insns: %" PRIu64 "\n",
> + inline_count.bb_count, inline_count.insn_count);
> + } else {
> + g_ptr_array_foreach(counts, (GFunc) gen_one_cpu_report, report);
> + }
> + qemu_plugin_outs(report->str);
> +}
> +
> +static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
> +{
> + CPUCount *count = g_ptr_array_index(counts, cpu_index);
> + g_autoptr(GString) report = g_string_new("");
> + gen_one_cpu_report(count, report);
> +
> + if (report->len > 0) {
> + g_string_prepend(report, "Idling ");
> + qemu_plugin_outs(report->str);
> + }
> }
>
> static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
> {
> - unsigned long n_insns = (unsigned long)udata;
> + CPUCount *count = max_cpus ?
> + g_ptr_array_index(counts, cpu_index) : &inline_count;
>
> - insn_count += n_insns;
> - bb_count++;
> + unsigned long n_insns = (unsigned long)udata;
> + g_mutex_lock(&count->lock);
> + count->insn_count += n_insns;
> + count->bb_count++;
> + g_mutex_unlock(&count->lock);
> }
>
> static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> @@ -42,9 +85,9 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>
> if (do_inline) {
> qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
> - &bb_count, 1);
> + &inline_count.bb_count, 1);
> qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
> - &insn_count, n_insns);
> + &inline_count.insn_count, n_insns);
> } else {
> qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
> QEMU_PLUGIN_CB_NO_REGS,
> @@ -56,8 +99,35 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> const qemu_info_t *info,
> int argc, char **argv)
> {
> - if (argc && strcmp(argv[0], "inline") == 0) {
> - do_inline = true;
> + int i;
> +
> + for (i = 0; i < argc; i++) {
> + char *opt = argv[i];
> + if (g_strcmp0(opt, "inline") == 0) {
> + do_inline = true;
> + } else if (g_strcmp0(opt, "idle") == 0) {
> + idle_report = true;
> + } else {
> + fprintf(stderr, "option parsing failed: %s\n", opt);
> + return -1;
> + }
> + }
> +
> + if (info->system_emulation && !do_inline) {
> + max_cpus = info->system.max_vcpus;
> + counts = g_ptr_array_new();
> + for (i = 0; i < max_cpus; i++) {
> + CPUCount *count = g_new0(CPUCount, 1);
> + g_mutex_init(&count->lock);
> + count->index = i;
> + g_ptr_array_add(counts, count);
> + }
> + } else if (!do_inline) {
> + g_mutex_init(&inline_count.lock);
> + }
> +
> + if (idle_report) {
> + qemu_plugin_register_vcpu_idle_cb(id, vcpu_idle);
> }
>
> qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu
2020-07-09 14:13 ` [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu Alex Bennée
2020-07-09 17:12 ` Robert Foley
@ 2020-07-11 21:56 ` Emilio G. Cota
2020-07-12 9:48 ` Alex Bennée
1 sibling, 1 reply; 37+ messages in thread
From: Emilio G. Cota @ 2020-07-11 21:56 UTC (permalink / raw)
To: Alex Bennée
Cc: fam, berrange, robert.foley, richard.henderson, qemu-devel,
robhenry, f4bug, aaron, Dave Bort, kuhn.chenqun, peter.puhov,
aurelien
On Thu, Jul 09, 2020 at 15:13:22 +0100, Alex Bennée wrote:
> While there isn't any easy way to make the inline counts thread safe
Why not? At least in 64-bit hosts TCG will emit a single write to
update the 64-bit counter.
> we can ensure the callback based ones are. While we are at it we can
> reduce introduce a new option ("idle") to dump a report of the current
s/reduce//
> bb and insn count each time a vCPU enters the idle state.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Dave Bort <dbort@dbort.com>
>
> ---
> v2
> - fixup for non-inline linux-user case
> - minor cleanup and re-factor
> ---
> tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 83 insertions(+), 13 deletions(-)
>
> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
> index df19fd359df3..89c373e19cd8 100644
> --- a/tests/plugin/bb.c
> +++ b/tests/plugin/bb.c
> @@ -16,24 +16,67 @@
>
> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>
> -static uint64_t bb_count;
> -static uint64_t insn_count;
> +typedef struct {
> + GMutex lock;
> + int index;
> + uint64_t bb_count;
> + uint64_t insn_count;
> +} CPUCount;
Why use a mutex?
Just have a per-vCPU struct that each vCPU thread updates with atomic_write.
Then when we want to print a report we just have to collect the counts
with atomic_read().
Also, consider just adding a comment to bb.c noting that it is not thread-safe,
and having a separate bb-threadsafe.c plugin for patch. The reason is that bb.c is
very simple, which is useful to understand the interface.
Thanks,
E.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu
2020-07-11 21:56 ` Emilio G. Cota
@ 2020-07-12 9:48 ` Alex Bennée
0 siblings, 0 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-12 9:48 UTC (permalink / raw)
To: Emilio G. Cota
Cc: fam, berrange, robert.foley, richard.henderson, qemu-devel,
robhenry, f4bug, aaron, Dave Bort, kuhn.chenqun, peter.puhov,
aurelien
Emilio G. Cota <cota@braap.org> writes:
> On Thu, Jul 09, 2020 at 15:13:22 +0100, Alex Bennée wrote:
>> While there isn't any easy way to make the inline counts thread safe
>
> Why not? At least in 64-bit hosts TCG will emit a single write to
> update the 64-bit counter.
Well the operation is an add so that is a load/add/store cycle on
non-x86. If we want to do it properly we should expose an atomic add
operation which may be helper based or maybe generate an atomic
operation if the backend can support it easily.
>
>> we can ensure the callback based ones are. While we are at it we can
>> reduce introduce a new option ("idle") to dump a report of the current
>
> s/reduce//
>
>> bb and insn count each time a vCPU enters the idle state.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Dave Bort <dbort@dbort.com>
>>
>> ---
>> v2
>> - fixup for non-inline linux-user case
>> - minor cleanup and re-factor
>> ---
>> tests/plugin/bb.c | 96 ++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 83 insertions(+), 13 deletions(-)
>>
>> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
>> index df19fd359df3..89c373e19cd8 100644
>> --- a/tests/plugin/bb.c
>> +++ b/tests/plugin/bb.c
>> @@ -16,24 +16,67 @@
>>
>> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>>
>> -static uint64_t bb_count;
>> -static uint64_t insn_count;
>> +typedef struct {
>> + GMutex lock;
>> + int index;
>> + uint64_t bb_count;
>> + uint64_t insn_count;
>> +} CPUCount;
>
> Why use a mutex?
>
> Just have a per-vCPU struct that each vCPU thread updates with atomic_write.
> Then when we want to print a report we just have to collect the counts
> with atomic_read().
>
> Also, consider just adding a comment to bb.c noting that it is not thread-safe,
> and having a separate bb-threadsafe.c plugin for patch. The reason is that bb.c is
> very simple, which is useful to understand the interface.
>
> Thanks,
> E.
--
Alex Bennée
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 09/13] target/sh4: revert to using cpu_lduw_code to decode gusa
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (7 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-09 14:42 ` Philippe Mathieu-Daudé
2020-07-10 21:26 ` Richard Henderson
2020-07-09 14:13 ` [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi Alex Bennée
` (3 subsequent siblings)
12 siblings, 2 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Yoshinori Sato, Alex Bennée,
richard.henderson, f4bug, robhenry, aaron, cota, Claudio Fontana,
kuhn.chenqun, peter.puhov, aurelien
The translator_ld* functions very much expect us to be decoding one
instruction at a time. Otherwise we will see weirdness such as:
qemu-sh4: warning: plugin_disas: 6 bytes left over
when we use the disas functions. For what SH4 is doing here (scanning
ahead in the instruction stream) this is the right function to use.
Reported-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
target/sh4/translate.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index 6192d83e8c66..919da72a0c98 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -1915,9 +1915,13 @@ static void decode_gusa(DisasContext *ctx, CPUSH4State *env)
goto fail;
}
- /* Read all of the insns for the region. */
+ /*
+ * Read all of the insns for the region. We do this directly with
+ * cpu_lduw_code to avoid confusing the plugins by decoding
+ * multiple instructions.
+ */
for (i = 0; i < max_insns; ++i) {
- insns[i] = translator_lduw(env, pc + i * 2);
+ insns[i] = cpu_lduw_code(env, pc + i * 2);
}
ld_adr = ld_dst = ld_mop = -1;
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 09/13] target/sh4: revert to using cpu_lduw_code to decode gusa
2020-07-09 14:13 ` [PATCH v1 09/13] target/sh4: revert to using cpu_lduw_code to decode gusa Alex Bennée
@ 2020-07-09 14:42 ` Philippe Mathieu-Daudé
2020-07-10 21:26 ` Richard Henderson
1 sibling, 0 replies; 37+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-09 14:42 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: fam, berrange, robert.foley, Yoshinori Sato, richard.henderson,
robhenry, aaron, cota, Claudio Fontana, peter.puhov,
kuhn.chenqun, aurelien
On 7/9/20 4:13 PM, Alex Bennée wrote:
> The translator_ld* functions very much expect us to be decoding one
> instruction at a time. Otherwise we will see weirdness such as:
>
> qemu-sh4: warning: plugin_disas: 6 bytes left over
>
> when we use the disas functions. For what SH4 is doing here (scanning
> ahead in the instruction stream) this is the right function to use.
>
> Reported-by: Claudio Fontana <cfontana@suse.de>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/sh4/translate.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/sh4/translate.c b/target/sh4/translate.c
> index 6192d83e8c66..919da72a0c98 100644
> --- a/target/sh4/translate.c
> +++ b/target/sh4/translate.c
> @@ -1915,9 +1915,13 @@ static void decode_gusa(DisasContext *ctx, CPUSH4State *env)
> goto fail;
> }
>
> - /* Read all of the insns for the region. */
> + /*
> + * Read all of the insns for the region. We do this directly with
> + * cpu_lduw_code to avoid confusing the plugins by decoding
> + * multiple instructions.
> + */
> for (i = 0; i < max_insns; ++i) {
> - insns[i] = translator_lduw(env, pc + i * 2);
> + insns[i] = cpu_lduw_code(env, pc + i * 2);
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> }
>
> ld_adr = ld_dst = ld_mop = -1;
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v1 09/13] target/sh4: revert to using cpu_lduw_code to decode gusa
2020-07-09 14:13 ` [PATCH v1 09/13] target/sh4: revert to using cpu_lduw_code to decode gusa Alex Bennée
2020-07-09 14:42 ` Philippe Mathieu-Daudé
@ 2020-07-10 21:26 ` Richard Henderson
1 sibling, 0 replies; 37+ messages in thread
From: Richard Henderson @ 2020-07-10 21:26 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: fam, berrange, robert.foley, Yoshinori Sato, f4bug, robhenry,
aaron, cota, Claudio Fontana, kuhn.chenqun, peter.puhov,
aurelien
On 7/9/20 7:13 AM, Alex Bennée wrote:
> The translator_ld* functions very much expect us to be decoding one
> instruction at a time. Otherwise we will see weirdness such as:
>
> qemu-sh4: warning: plugin_disas: 6 bytes left over
>
> when we use the disas functions. For what SH4 is doing here (scanning
> ahead in the instruction stream) this is the right function to use.
It is not just scanning ahead. It does that, but after having done so it may
also commit to translating them all at once.
In the case to which you refer, above, we have translated 4 insns into one
operation. Just having plugin_disas see the first one is not really correct
either.
r~
>
> Reported-by: Claudio Fontana <cfontana@suse.de>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/sh4/translate.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/sh4/translate.c b/target/sh4/translate.c
> index 6192d83e8c66..919da72a0c98 100644
> --- a/target/sh4/translate.c
> +++ b/target/sh4/translate.c
> @@ -1915,9 +1915,13 @@ static void decode_gusa(DisasContext *ctx, CPUSH4State *env)
> goto fail;
> }
>
> - /* Read all of the insns for the region. */
> + /*
> + * Read all of the insns for the region. We do this directly with
> + * cpu_lduw_code to avoid confusing the plugins by decoding
> + * multiple instructions.
> + */
> for (i = 0; i < max_insns; ++i) {
> - insns[i] = translator_lduw(env, pc + i * 2);
> + insns[i] = cpu_lduw_code(env, pc + i * 2);
> }
>
> ld_adr = ld_dst = ld_mop = -1;
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (8 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 09/13] target/sh4: revert to using cpu_lduw_code to decode gusa Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-09 17:27 ` Robert Foley
2020-07-10 21:29 ` Richard Henderson
2020-07-09 14:13 ` [PATCH v1 11/13] tests/docker: fall back more gracefully when pull fails Alex Bennée
` (2 subsequent siblings)
12 siblings, 2 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, aaron, cota, kuhn.chenqun, peter.puhov,
aurelien
Not all compilers support the -Wpsabi (clang-9 in my case).
Fixes: bac8d222a
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
tests/plugin/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
index 0cb8e35ae407..dcfbd99b15b8 100644
--- a/tests/plugin/Makefile
+++ b/tests/plugin/Makefile
@@ -18,7 +18,7 @@ NAMES += hwprofile
SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
-QEMU_CFLAGS += -fPIC -Wpsabi
+QEMU_CFLAGS += -fPIC -Wno-unknown-warning-option -Wpsabi
QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu
all: $(SONAMES)
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi
2020-07-09 14:13 ` [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi Alex Bennée
@ 2020-07-09 17:27 ` Robert Foley
2020-07-10 21:29 ` Richard Henderson
1 sibling, 0 replies; 37+ messages in thread
From: Robert Foley @ 2020-07-09 17:27 UTC (permalink / raw)
To: Alex Bennée
Cc: Fam Zheng, Daniel P. Berrangé,
Richard Henderson, QEMU Developers, robhenry, f4bug, aaron,
Emilio G. Cota, kuhn.chenqun, Peter Puhov, Aurelien Jarno
Reviewed-by: Robert Foley <robert.foley@linaro.org>
On Thu, 9 Jul 2020 at 10:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Not all compilers support the -Wpsabi (clang-9 in my case).
>
> Fixes: bac8d222a
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> tests/plugin/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
> index 0cb8e35ae407..dcfbd99b15b8 100644
> --- a/tests/plugin/Makefile
> +++ b/tests/plugin/Makefile
> @@ -18,7 +18,7 @@ NAMES += hwprofile
>
> SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>
> -QEMU_CFLAGS += -fPIC -Wpsabi
> +QEMU_CFLAGS += -fPIC -Wno-unknown-warning-option -Wpsabi
> QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu
>
> all: $(SONAMES)
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi
2020-07-09 14:13 ` [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi Alex Bennée
2020-07-09 17:27 ` Robert Foley
@ 2020-07-10 21:29 ` Richard Henderson
2020-07-13 16:39 ` Alex Bennée
1 sibling, 1 reply; 37+ messages in thread
From: Richard Henderson @ 2020-07-10 21:29 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: fam, berrange, robert.foley, f4bug, robhenry, aaron, cota,
kuhn.chenqun, peter.puhov, aurelien
On 7/9/20 7:13 AM, Alex Bennée wrote:
> Not all compilers support the -Wpsabi (clang-9 in my case).
>
> Fixes: bac8d222a
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> tests/plugin/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
> index 0cb8e35ae407..dcfbd99b15b8 100644
> --- a/tests/plugin/Makefile
> +++ b/tests/plugin/Makefile
> @@ -18,7 +18,7 @@ NAMES += hwprofile
>
> SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>
> -QEMU_CFLAGS += -fPIC -Wpsabi
> +QEMU_CFLAGS += -fPIC -Wno-unknown-warning-option -Wpsabi
Surely -Wno-unknown-warning-option is in the same boat? E.g. I don't see any
version of gcc that supports it.
Originally, I tried to grab -Wno-psabi out of the existing QEMU_CFLAGS and
transforming it, but I couldn't make that work.
r~
> QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu
>
> all: $(SONAMES)
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi
2020-07-10 21:29 ` Richard Henderson
@ 2020-07-13 16:39 ` Alex Bennée
2020-07-13 18:27 ` Thomas Huth
0 siblings, 1 reply; 37+ messages in thread
From: Alex Bennée @ 2020-07-13 16:39 UTC (permalink / raw)
To: Richard Henderson
Cc: fam, berrange, robert.foley, qemu-devel, robhenry, f4bug, aaron,
cota, kuhn.chenqun, peter.puhov, aurelien
Richard Henderson <richard.henderson@linaro.org> writes:
> On 7/9/20 7:13 AM, Alex Bennée wrote:
>> Not all compilers support the -Wpsabi (clang-9 in my case).
>>
>> Fixes: bac8d222a
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> tests/plugin/Makefile | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
>> index 0cb8e35ae407..dcfbd99b15b8 100644
>> --- a/tests/plugin/Makefile
>> +++ b/tests/plugin/Makefile
>> @@ -18,7 +18,7 @@ NAMES += hwprofile
>>
>> SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>>
>> -QEMU_CFLAGS += -fPIC -Wpsabi
>> +QEMU_CFLAGS += -fPIC -Wno-unknown-warning-option -Wpsabi
>
> Surely -Wno-unknown-warning-option is in the same boat? E.g. I don't see any
> version of gcc that supports it.
GCC doesn't seem to complain about it though.
> Originally, I tried to grab -Wno-psabi out of the existing QEMU_CFLAGS and
> transforming it, but I couldn't make that work.
I though the plugin Makefile was meant to be standalone to demonstrate
how you build stuff out of tree but I guess we include some of the main
make machinery in it. I'll see what I can do.
>
>
> r~
>
>> QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu
>>
>> all: $(SONAMES)
>>
--
Alex Bennée
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi
2020-07-13 16:39 ` Alex Bennée
@ 2020-07-13 18:27 ` Thomas Huth
2020-07-13 19:34 ` Alex Bennée
0 siblings, 1 reply; 37+ messages in thread
From: Thomas Huth @ 2020-07-13 18:27 UTC (permalink / raw)
To: Alex Bennée, Richard Henderson
Cc: fam, berrange, robert.foley, f4bug, robhenry, qemu-devel, aaron,
cota, peter.puhov, kuhn.chenqun, aurelien
On 13/07/2020 18.39, Alex Bennée wrote:
>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> On 7/9/20 7:13 AM, Alex Bennée wrote:
>>> Not all compilers support the -Wpsabi (clang-9 in my case).
>>>
>>> Fixes: bac8d222a
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>> tests/plugin/Makefile | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
>>> index 0cb8e35ae407..dcfbd99b15b8 100644
>>> --- a/tests/plugin/Makefile
>>> +++ b/tests/plugin/Makefile
>>> @@ -18,7 +18,7 @@ NAMES += hwprofile
>>>
>>> SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>>>
>>> -QEMU_CFLAGS += -fPIC -Wpsabi
>>> +QEMU_CFLAGS += -fPIC -Wno-unknown-warning-option -Wpsabi
>>
>> Surely -Wno-unknown-warning-option is in the same boat? E.g. I don't see any
>> version of gcc that supports it.
>
> GCC doesn't seem to complain about it though.
Both, GCC and Clang do not complain about unknown -Wno-somthing options
as long as there are no other warnings. They only complain for options
that do not have a "no-" after the "-W".
Thomas
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi
2020-07-13 18:27 ` Thomas Huth
@ 2020-07-13 19:34 ` Alex Bennée
0 siblings, 0 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-13 19:34 UTC (permalink / raw)
To: Thomas Huth
Cc: fam, berrange, robert.foley, Richard Henderson, qemu-devel,
robhenry, f4bug, aaron, cota, peter.puhov, kuhn.chenqun,
aurelien
Thomas Huth <thuth@redhat.com> writes:
> On 13/07/2020 18.39, Alex Bennée wrote:
>>
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>
>>> On 7/9/20 7:13 AM, Alex Bennée wrote:
>>>> Not all compilers support the -Wpsabi (clang-9 in my case).
>>>>
>>>> Fixes: bac8d222a
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> ---
>>>> tests/plugin/Makefile | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
>>>> index 0cb8e35ae407..dcfbd99b15b8 100644
>>>> --- a/tests/plugin/Makefile
>>>> +++ b/tests/plugin/Makefile
>>>> @@ -18,7 +18,7 @@ NAMES += hwprofile
>>>>
>>>> SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>>>>
>>>> -QEMU_CFLAGS += -fPIC -Wpsabi
>>>> +QEMU_CFLAGS += -fPIC -Wno-unknown-warning-option -Wpsabi
>>>
>>> Surely -Wno-unknown-warning-option is in the same boat? E.g. I don't see any
>>> version of gcc that supports it.
>>
>> GCC doesn't seem to complain about it though.
>
> Both, GCC and Clang do not complain about unknown -Wno-somthing options
> as long as there are no other warnings. They only complain for options
> that do not have a "no-" after the "-W".
I've done a little minor surgery on the Makefile to both limit the
pollution of the plugin build from QEMU flags and to only enable the
check if we detect the compiler supports it:
--8<---------------cut here---------------start------------->8---
tests/plugins: don't unconditionally add -Wpsabi
Not all compilers support the -Wpsabi (clang-9 in my case). To handle
this gracefully we pare back the shared build machinery so the
Makefile is relatively "standalone". We still take advantage of
config-host.mak as configure has done a bunch of probing for us but
that is it.
Fixes: bac8d222a
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v2
- separate from main build system and check probe
2 files changed, 21 insertions(+), 4 deletions(-)
configure | 3 +++
tests/plugin/Makefile | 22 ++++++++++++++++++----
modified configure
@@ -7115,6 +7115,9 @@ echo "GIT_UPDATE=$git_update" >> $config_host_mak
echo "ARCH=$ARCH" >> $config_host_mak
+echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
+echo "GLIB_LDFLAGS=$glib_ldflags" >> $config_host_mak
+
if test "$default_devices" = "yes" ; then
echo "CONFIG_MINIKCONF_MODE=--defconfig" >> $config_host_mak
else
modified tests/plugin/Makefile
@@ -1,9 +1,16 @@
+# -*- Mode: makefile -*-
+#
+# This Makefile example is fairly independent from the main makefile
+# so users can take and adapt it for their build. We only really
+# include config-host.mak so we don't have to repeat probing for
+# cflags that the main configure has already done for us.
+#
+
BUILD_DIR := $(CURDIR)/../..
include $(BUILD_DIR)/config-host.mak
-include $(SRC_PATH)/rules.mak
-$(call set-vpath, $(SRC_PATH)/tests/plugin)
+VPATH += $(SRC_PATH)/tests/plugin
NAMES :=
NAMES += bb
@@ -18,11 +25,18 @@ NAMES += hwprofile
SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
-QEMU_CFLAGS += -fPIC -Wpsabi
-QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu
+# The main QEMU uses Glib extensively so it's perfectly fine to use it
+# in plugins (which many example do).
+CFLAGS = $(GLIB_CFLAGS)
+CFLAGS += -fPIC
+CFLAGS += $(if $(findstring no-psabi,$(QEMU_CFLAGS)),-Wpsabi)
+CFLAGS += -I$(SRC_PATH)/include/qemu
all: $(SONAMES)
+%.o: %.c
+ $(CC) $(CFLAGS) -c -o $@ $<
+
lib%.so: %.o
$(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS)
--8<---------------cut here---------------end--------------->8---
--
Alex Bennée
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 11/13] tests/docker: fall back more gracefully when pull fails
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (9 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 10/13] tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-09 14:46 ` Philippe Mathieu-Daudé
2020-07-09 14:13 ` [PATCH v1 12/13] tests/docker: update toolchain set in debian-xtensa-cross Alex Bennée
2020-07-09 14:13 ` [PATCH v1 13/13] configure: remove all dependencies on a (re)configure Alex Bennée
12 siblings, 1 reply; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, Philippe Mathieu-Daudé,
aaron, cota, kuhn.chenqun, peter.puhov, aurelien
I only spotted this in the small window between my testing with my
registry while waiting for the gitlab PR to go in. As we pre-pull the
registry image we know if that fails there isn't any point attempting
to use the cache. Fall back to the way we used to do it at that point.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
tests/docker/docker.py | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 9684f07bdebe..2d67bbd15a5b 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -306,13 +306,14 @@ class Docker(object):
checksum = _text_checksum(_dockerfile_preprocess(dockerfile))
if registry is not None:
- dockerfile = dockerfile.replace("FROM qemu/",
- "FROM %s/qemu/" %
- (registry))
# see if we can fetch a cache copy, may fail...
pull_args = ["pull", "%s/%s" % (registry, tag)]
- self._do(pull_args, quiet=quiet)
-
+ if self._do(pull_args, quiet=quiet) == 0:
+ dockerfile = dockerfile.replace("FROM qemu/",
+ "FROM %s/qemu/" %
+ (registry))
+ else:
+ registry = None
tmp_df = tempfile.NamedTemporaryFile(mode="w+t",
encoding='utf-8',
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 11/13] tests/docker: fall back more gracefully when pull fails
2020-07-09 14:13 ` [PATCH v1 11/13] tests/docker: fall back more gracefully when pull fails Alex Bennée
@ 2020-07-09 14:46 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 37+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-09 14:46 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: fam, berrange, robert.foley, Philippe Mathieu-Daudé,
richard.henderson, robhenry, aaron, cota, kuhn.chenqun,
peter.puhov, aurelien
On 7/9/20 4:13 PM, Alex Bennée wrote:
> I only spotted this in the small window between my testing with my
> registry while waiting for the gitlab PR to go in. As we pre-pull the
> registry image we know if that fails there isn't any point attempting
> to use the cache. Fall back to the way we used to do it at that point.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> tests/docker/docker.py | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index 9684f07bdebe..2d67bbd15a5b 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -306,13 +306,14 @@ class Docker(object):
> checksum = _text_checksum(_dockerfile_preprocess(dockerfile))
>
> if registry is not None:
> - dockerfile = dockerfile.replace("FROM qemu/",
> - "FROM %s/qemu/" %
> - (registry))
> # see if we can fetch a cache copy, may fail...
> pull_args = ["pull", "%s/%s" % (registry, tag)]
> - self._do(pull_args, quiet=quiet)
> -
> + if self._do(pull_args, quiet=quiet) == 0:
Maybe worth defining EXIT_SUCCESS = 0. Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> + dockerfile = dockerfile.replace("FROM qemu/",
> + "FROM %s/qemu/" %
> + (registry))
> + else:
> + registry = None
>
> tmp_df = tempfile.NamedTemporaryFile(mode="w+t",
> encoding='utf-8',
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 12/13] tests/docker: update toolchain set in debian-xtensa-cross
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (10 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 11/13] tests/docker: fall back more gracefully when pull fails Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
2020-07-09 14:54 ` Philippe Mathieu-Daudé
2020-07-09 14:13 ` [PATCH v1 13/13] configure: remove all dependencies on a (re)configure Alex Bennée
12 siblings, 1 reply; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, Philippe Mathieu-Daudé,
Max Filippov, aaron, cota, kuhn.chenqun, peter.puhov, aurelien
From: Max Filippov <jcmvbkbc@gmail.com>
Switch to the prebuilt xtensa toolchains release 2020.07.
Drop csp toolchain as the csp core is not a part of QEMU.
Add de233_fpu and dsp3400 toolchains to enable DFPU and FPU2000 tests.
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200708082347.27318-1-jcmvbkbc@gmail.com>
---
tests/docker/dockerfiles/debian-xtensa-cross.docker | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/docker/dockerfiles/debian-xtensa-cross.docker b/tests/docker/dockerfiles/debian-xtensa-cross.docker
index beb73f46baa6..ba4148299c5a 100644
--- a/tests/docker/dockerfiles/debian-xtensa-cross.docker
+++ b/tests/docker/dockerfiles/debian-xtensa-cross.docker
@@ -18,12 +18,12 @@ RUN apt-get update && \
git \
python3-minimal
-ENV CPU_LIST csp dc232b dc233c
-ENV TOOLCHAIN_RELEASE 2018.02
+ENV CPU_LIST dc232b dc233c de233_fpu dsp3400
+ENV TOOLCHAIN_RELEASE 2020.07
RUN for cpu in $CPU_LIST; do \
curl -#SL http://github.com/foss-xtensa/toolchain/releases/download/$TOOLCHAIN_RELEASE/x86_64-$TOOLCHAIN_RELEASE-xtensa-$cpu-elf.tar.gz \
| tar -xzC /opt; \
done
-ENV PATH $PATH:/opt/$TOOLCHAIN_RELEASE/xtensa-dc232b-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-dc233c-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-csp-elf/bin
+ENV PATH $PATH:/opt/$TOOLCHAIN_RELEASE/xtensa-dc232b-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-dc233c-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-de233_fpu-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-dsp3400-elf/bin
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v1 12/13] tests/docker: update toolchain set in debian-xtensa-cross
2020-07-09 14:13 ` [PATCH v1 12/13] tests/docker: update toolchain set in debian-xtensa-cross Alex Bennée
@ 2020-07-09 14:54 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 37+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-09 14:54 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: fam, berrange, robert.foley, Philippe Mathieu-Daudé,
richard.henderson, robhenry, Max Filippov, aaron, cota,
kuhn.chenqun, peter.puhov, aurelien
On 7/9/20 4:13 PM, Alex Bennée wrote:
> From: Max Filippov <jcmvbkbc@gmail.com>
>
> Switch to the prebuilt xtensa toolchains release 2020.07.
> Drop csp toolchain as the csp core is not a part of QEMU.
> Add de233_fpu and dsp3400 toolchains to enable DFPU and FPU2000 tests.
Yay thanks Max!
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20200708082347.27318-1-jcmvbkbc@gmail.com>
> ---
> tests/docker/dockerfiles/debian-xtensa-cross.docker | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/docker/dockerfiles/debian-xtensa-cross.docker b/tests/docker/dockerfiles/debian-xtensa-cross.docker
> index beb73f46baa6..ba4148299c5a 100644
> --- a/tests/docker/dockerfiles/debian-xtensa-cross.docker
> +++ b/tests/docker/dockerfiles/debian-xtensa-cross.docker
> @@ -18,12 +18,12 @@ RUN apt-get update && \
> git \
> python3-minimal
>
> -ENV CPU_LIST csp dc232b dc233c
> -ENV TOOLCHAIN_RELEASE 2018.02
> +ENV CPU_LIST dc232b dc233c de233_fpu dsp3400
> +ENV TOOLCHAIN_RELEASE 2020.07
>
> RUN for cpu in $CPU_LIST; do \
> curl -#SL http://github.com/foss-xtensa/toolchain/releases/download/$TOOLCHAIN_RELEASE/x86_64-$TOOLCHAIN_RELEASE-xtensa-$cpu-elf.tar.gz \
> | tar -xzC /opt; \
> done
>
> -ENV PATH $PATH:/opt/$TOOLCHAIN_RELEASE/xtensa-dc232b-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-dc233c-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-csp-elf/bin
> +ENV PATH $PATH:/opt/$TOOLCHAIN_RELEASE/xtensa-dc232b-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-dc233c-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-de233_fpu-elf/bin:/opt/$TOOLCHAIN_RELEASE/xtensa-dsp3400-elf/bin
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v1 13/13] configure: remove all dependencies on a (re)configure
2020-07-09 14:13 [PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker) Alex Bennée
` (11 preceding siblings ...)
2020-07-09 14:13 ` [PATCH v1 12/13] tests/docker: update toolchain set in debian-xtensa-cross Alex Bennée
@ 2020-07-09 14:13 ` Alex Bennée
12 siblings, 0 replies; 37+ messages in thread
From: Alex Bennée @ 2020-07-09 14:13 UTC (permalink / raw)
To: qemu-devel
Cc: fam, berrange, robert.foley, Alex Bennée, richard.henderson,
f4bug, robhenry, aaron, cota, kuhn.chenqun, peter.puhov,
aurelien
The previous code was brittle and missed cases such as the mipn32
variants which for some reason has the 64 bit syscalls. This leads to
a number of binary targets having deps lines like:
all.clang-sanitizer/mipsn32el-linux-user/linux-user/signal.d
140: /home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h \
455:/home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h:
all.clang-sanitizer/mipsn32el-linux-user/linux-user/syscall.d
146: /home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h \
485:/home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h:
which in turn would trigger the re-generation of syscall_nr.h in the
source tree (thanks to generic %/syscall_nr.h rules). The previous
code attempts to clean it out but misses edge cases but fails.
After spending a day trying to understand how this was happening I'm
unconvinced that there are not other such breakages possible with this
"caching". As we add more auto-generated code to the build it is likely
to trip up again. Apply a hammer to the problem.
Fixes: 91e5998f18 (which fixes 5f29856b852d and 4d6a835dea47)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
configure | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/configure b/configure
index 1e977601a47a..3c404f31f4f4 100755
--- a/configure
+++ b/configure
@@ -1955,23 +1955,20 @@ EOF
exit 0
fi
-# Remove old dependency files to make sure that they get properly regenerated
-rm -f */config-devices.mak.d
-
# Remove syscall_nr.h to be sure they will be regenerated in the build
# directory, not in the source directory
for arch in alpha hppa m68k xtensa sh4 microblaze arm ppc s390x sparc sparc64 \
i386 x86_64 mips mips64 ; do
# remove the file if it has been generated in the source directory
rm -f "${source_path}/linux-user/${arch}/syscall_nr.h"
- # remove the dependency files
- for target in ${arch}*-linux-user ; do
- test -d "${target}" && find "${target}" -type f -name "*.d" \
- -exec grep -q "${source_path}/linux-user/${arch}/syscall_nr.h" {} \; \
- -print | while read file ; do rm "${file}" "${file%.d}.o" ; done
- done
done
+# Clean out all old dependency files. As more files are generated we
+# run the risk of old dependencies triggering generation in the wrong
+# places. Previous brittle attempts to be surgical tend to miss edge
+# cases leading to wasted time and much confusion.
+find -type f -name "*.d" -exec rm -f {} \;
+
if test -z "$python"
then
error_exit "Python not found. Use --python=/path/to/python"
--
2.20.1
^ permalink raw reply related [flat|nested] 37+ messages in thread