[Bug 1735049] Re: Need MTTCG support for x86 guests
OK, looks like I cannot reopen the bug, probably because the bug tracker moved to gitlab. If you care about this feature, please file a bug over there: https://gitlab.com/qemu-project/qemu/-/issues -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1735049 Title: Need MTTCG support for x86 guests Status in QEMU: Fix Released Bug description: MTTCG support is notably absent for x86_64 guests. The last Wiki update on MTTCG was back in 2015, and I am having some difficulty determining the current status of the underlying requirements to enable this feature on x86 hosts. For instance, has support for strong-on-weak memory consistency been added into QEMU GIT at this point? Thanks! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1735049/+subscriptions
[Bug 1735049] Re: Need MTTCG support for x86 guests
Looks like support for this was not fully added; my apologies for closing this bug too early. Adding full support for strong-on-weak emulation would be simple, at least when it comes to memory ordering. The slowdown would be huge though, see Figure 12 in http://www.cs.columbia.edu/~cota/pubs/cota_cgo17.pdf (i.e. ~2x hmean overhead for SPEC). The good news is that with hardware support this overhead is ~0 (see SAO in that figure). The other feature that is not yet implemented in upstream QEMU is the correct emulation of LL/SC, although for most code out there this shouldn't be an issue in practice given that most parallel code relies on cmpxchg, not on LL/SC pairs. I'm reopening this bug an Cc'ing a few people who are more familiar with the current code than I am in case I missed anything. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1735049 Title: Need MTTCG support for x86 guests Status in QEMU: Fix Released Bug description: MTTCG support is notably absent for x86_64 guests. The last Wiki update on MTTCG was back in 2015, and I am having some difficulty determining the current status of the underlying requirements to enable this feature on x86 hosts. For instance, has support for strong-on-weak memory consistency been added into QEMU GIT at this point? Thanks! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1735049/+subscriptions
Re: [PATCH v2 06/11] cputlb: ensure we save the IOTLB data in case of reset
On Mon, Jul 13, 2020 at 21:04:10 +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 (snip) > +/* > + * 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. As I mentioned in the previous iteration of this series, this comment is outdated -- there is no thread storage nor RCU to worry about here. > + * 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. > */ s/loosing/losing/ About the approach in this patch: it works as long as the caller is in the same vCPU thread, otherwise we'd need a seqlock to avoid races between readers and the writing vCPU. I see that qemu_plugin_get_hwaddr does not even take a vCPU index, so this should be OK -- as long as this is called only from a mem callback, it's in the same vCPU thread and it's therefore safe. With the above comments fixed, Reviewed-by: Emilio G. Cota Thanks, Emilio
Re: [PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu
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
> Cc: Dave Bort
>
> ---
> 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.
Re: [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset
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
> >
> > ---
> > 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(>saved_iotlb);
> > +atomic_rcu_set(>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.
Re: [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset
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
Reviewed-by: Emilio G. Cota
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(>saved_iotlb);
> +atomic_rcu_set(>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
Re: [PATCH v1 02/13] docs/devel: add some notes on tcg-icount for developers
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 > Reviewed-by: Richard Henderson Reviewed-by: Emilio G. Cota Thanks, Emilio
Re: [PATCH v1 01/13] docs/devel: convert and update MTTCG design document
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 Thanks, E.
[Bug 1885827] Re: building plugin failed on Windows with mingw
You should then find out why libqemu_plugin.dll.a is not working. It is possible though that your linked is calling the import library something else, for instance adding a .dll extension to it. You will have to run a few tests with your linker (I'd just use the examples from the stackoverflow links I posted above) to see the name of the import library that gets created. My assumption is that some library gets created, otherwise the linked should give you an error and AFAICT it does not. Once you find what the import library us, you should be in good shape to adapt the above for QEMU. Let us know how it goes! -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1885827 Title: building plugin failed on Windows with mingw Status in QEMU: New Bug description: I want to build QEMU 4.2.0's plugin module on Windows 7/10 with Mingw, but the building process faild. The step I follow is listed below: 1. create "dsp_build" diretory under source file folder 2. change directory to dsp_build , and run ../configure --target-list=dsp-softmmu --cross-prefix=x86_64-w64-mingw32- --enable-gtk --enable-sdl --enable-debug --enable-plugins 3. build qemu project 4. switch dir to /dsp_build, make -C tests/plugin, yeilds error: CC bb.o D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:17:24: error: variable 'qemu_plugin_version' definition is marked dllimport 17 | QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; |^~~ D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:17:24: warning: 'qemu_plugin_version' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c: In function 'vcpu_tb_exec': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:33:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] 33 | unsigned long n_insns = (unsigned long)udata; | ^ D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c: In function 'vcpu_tb_trans': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:51:46: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 51 | (void *)n_insns); 5. Then , I modified the QEMU_flags and the compilation command arguments($(CC) ..) in the makefile : BUILD_DIR := $(CURDIR)/../.. include $(BUILD_DIR)/config-host.mak include $(SRC_PATH)/rules.mak $(call set-vpath, $(SRC_PATH)/tests/plugin) NAMES := NAMES += bb NAMES += empty NAMES += insn NAMES += mem NAMES += hotblocks NAMES += howvec NAMES += hotpages SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES))) QEMU_CFLAGS += -fPIC-DBUILDING_DLL #added -DBUILDING_DLL QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu all: $(SONAMES) lib%.so: %.o $(CC) -fPIC -shared -o $@ $^ $(LDLIBS) -L /c/msys64/mingw64/lib/ -lglib-2.0 # original cmd: $(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS) clean: rm -f *.o *.so *.d rm -Rf .libs .PHONY: all clean 6. Executing make yeilds: make: enter “/d/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/build_dsp/tests/plugin” CC bb.o x86_64-w64-mingw32-gcc -fPIC -shared -o libbb.so bb.o -L /c/msys64/mingw64/lib/ -lglib-2.0 C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: bb.o: in function `plugin_exit': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:28: undefined reference to `qemu_plugin_outs' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:29: undefined reference to `__stack_chk_fail' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: bb.o: in function `vcpu_tb_trans': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:41: undefined reference to `qemu_plugin_tb_n_insns' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:44: undefined reference to `qemu_plugin_register_vcpu_tb_exec_inline'
[Bug 1885827] Re: building plugin failed on Windows with mingw
You're getting close. 1. From the build directory, find the import library created, e.g. `find . -name 'libqemu_plugin.dll.a'`. If you're building x86_64-linux-user, it should be in `x86_64-linux-user/libqemu_plugin.dll.a`. 2. Go back to the `build/tests/plugin` directory. 3. Link the empty plugin with: x86_64-w64-mingw32-gcc -shared -Wl,-soname,libempty.so -o libempty.so empty.o -L/path/to/libqemu_plugin.dll.a/directory -lqemu_plugin.dll Note the added -L and -l flags. If that works, move on to the other plugins, although you might have to comment out the glib-dependent code. But let's not go there yet; try to build the empty plugin first. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1885827 Title: building plugin failed on Windows with mingw Status in QEMU: New Bug description: I want to build QEMU 4.2.0's plugin module on Windows 7/10 with Mingw, but the building process faild. The step I follow is listed below: 1. create "dsp_build" diretory under source file folder 2. change directory to dsp_build , and run ../configure --target-list=dsp-softmmu --cross-prefix=x86_64-w64-mingw32- --enable-gtk --enable-sdl --enable-debug --enable-plugins 3. build qemu project 4. switch dir to /dsp_build, make -C tests/plugin, yeilds error: CC bb.o D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:17:24: error: variable 'qemu_plugin_version' definition is marked dllimport 17 | QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; |^~~ D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:17:24: warning: 'qemu_plugin_version' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c: In function 'vcpu_tb_exec': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:33:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] 33 | unsigned long n_insns = (unsigned long)udata; | ^ D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c: In function 'vcpu_tb_trans': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:51:46: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 51 | (void *)n_insns); 5. Then , I modified the QEMU_flags and the compilation command arguments($(CC) ..) in the makefile : BUILD_DIR := $(CURDIR)/../.. include $(BUILD_DIR)/config-host.mak include $(SRC_PATH)/rules.mak $(call set-vpath, $(SRC_PATH)/tests/plugin) NAMES := NAMES += bb NAMES += empty NAMES += insn NAMES += mem NAMES += hotblocks NAMES += howvec NAMES += hotpages SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES))) QEMU_CFLAGS += -fPIC-DBUILDING_DLL #added -DBUILDING_DLL QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu all: $(SONAMES) lib%.so: %.o $(CC) -fPIC -shared -o $@ $^ $(LDLIBS) -L /c/msys64/mingw64/lib/ -lglib-2.0 # original cmd: $(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS) clean: rm -f *.o *.so *.d rm -Rf .libs .PHONY: all clean 6. Executing make yeilds: make: enter “/d/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/build_dsp/tests/plugin” CC bb.o x86_64-w64-mingw32-gcc -fPIC -shared -o libbb.so bb.o -L /c/msys64/mingw64/lib/ -lglib-2.0 C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: bb.o: in function `plugin_exit': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:28: undefined reference to `qemu_plugin_outs' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:29: undefined reference to `__stack_chk_fail' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: bb.o: in function `vcpu_tb_trans': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:41: undefined reference to `qemu_plugin_tb_n_insns' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:44: undefined reference to `qemu_plugin_register_vcpu_tb_exec_inline'
[Bug 1885827] Re: building plugin failed on Windows with mingw
Xiaolei confirmed to me via email that adding -DBUILDING_DLL is not enough to fix the problem. I looked into this a bit further and it looks like we need an "import library" to be created when compiling the QEMU binary. This is accomplished by adding "-Wl,--out-implib,libqemu_plugin.a" to the linker invocation to build the QEMU binary. See these two stackoverflow questions: - https://stackoverflow.com/questions/17601949/building-a-shared-library-using-gcc-on-linux-and-mingw-on-windows - https://stackoverflow.com/questions/39759060/compile-to-dll-with-some-undefined-references-with-mingw It's not clear to me whether any import library with the symbols needed can work, or whether each target binary needs its corresponding import library, e.g. aarch64-linux-user and x86_64-linux-user need different libraries. The below is an attempt at the latter approach; one would just have to link the appropriate import library with "-l" when building the plugin. Please give this a try, and don't forget to also pass -DBUILDING_DLL to the linker. diff --git a/Makefile b/Makefile index b437a346d7..5cc1bc8e23 100644 --- a/Makefile +++ b/Makefile @@ -866,8 +866,14 @@ ICON_SIZES=16x16 24x24 32x32 48x48 64x64 128x128 256x256 512x512 install-includedir: $(INSTALL_DIR) "$(DESTDIR)$(includedir)" +install-libdir: + $(INSTALL_DIR) "$(DESTDIR)$(libdir)" + for d in $(TARGET_DIRS); do \ + $(INSTALL_DIR) "$(DESTDIR)$(libdir)/$$d"; \ + done + install: all $(if $(BUILD_DOCS),install-doc) \ - install-datadir install-localstatedir install-includedir \ + install-datadir install-localstatedir install-includedir install-libdir \ $(if $(INSTALL_BLOBS),$(edk2-decompressed)) \ recurse-install ifneq ($(TOOLS),) @@ -932,6 +938,11 @@ ifdef CONFIG_GTK endif ifeq ($(CONFIG_PLUGIN),y) $(INSTALL_DATA) $(SRC_PATH)/include/qemu/qemu-plugin.h "$(DESTDIR)$(includedir)/qemu-plugin.h" +# ifeq MINGW, WINDOWS or similar. + for d in $(TARGET_DIRS); do \ + $(INSTALL_DATA) "$$d/libqemu_plugin.dll.a" "$(DESTDIR)$(libdir)/$$d/libqemu_plugin.dll.a"; \ + done +# endif endif $(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)/keymaps" set -e; for x in $(KEYMAPS); do \ diff --git a/configure b/configure index 6099be1d84..9606f6e888 100755 --- a/configure +++ b/configure @@ -7455,6 +7455,9 @@ if test "$plugins" = "yes" ; then "If \$plugins=yes, either \$ld_dynamic_list or " \ "\$ld_exported_symbols_list should have been set to 'yes'." fi +# if test "$mingw32" = "yes" ; then # or mingw, or windows; I don't know. + QEMU_LDFLAGS="-Wl,--out-implib,libqemu_plugin.dll.a" +# fi fi if test "$tcg_interpreter" = "yes"; then -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1885827 Title: building plugin failed on Windows with mingw Status in QEMU: New Bug description: I want to build QEMU 4.2.0's plugin module on Windows 7/10 with Mingw, but the building process faild. The step I follow is listed below: 1. create "dsp_build" diretory under source file folder 2. change directory to dsp_build , and run ../configure --target-list=dsp-softmmu --cross-prefix=x86_64-w64-mingw32- --enable-gtk --enable-sdl --enable-debug --enable-plugins 3. build qemu project 4. switch dir to /dsp_build, make -C tests/plugin, yeilds error: CC bb.o D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:17:24: error: variable 'qemu_plugin_version' definition is marked dllimport 17 | QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; |^~~ D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:17:24: warning: 'qemu_plugin_version' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c: In function 'vcpu_tb_exec': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:33:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] 33 | unsigned long n_insns = (unsigned long)udata; | ^ D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c: In function 'vcpu_tb_trans': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:51:46: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 51 | (void *)n_insns); 5. Then , I modified the QEMU_flags and the compilation command arguments($(CC) ..) in the makefile : BUILD_DIR := $(CURDIR)/../.. include $(BUILD_DIR)/config-host.mak include $(SRC_PATH)/rules.mak $(call set-vpath,
[Bug 1885827] Re: building plugin failed on Windows with mingw
I never built the plugin code on anything other than Linux :( All I did for Windows is to try to follow https://gcc.gnu.org/wiki/Visibility Can you please try the following patch (with no other changes)? The patch applies on top of v4.2.0. diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile index 75467b6db8..5b3611ad63 100644 --- a/tests/plugin/Makefile +++ b/tests/plugin/Makefile @@ -16,7 +16,7 @@ NAMES += hotpages SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES))) -QEMU_CFLAGS += -fPIC +QEMU_CFLAGS += -fPIC -DBUILDING_DLL QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu all: $(SONAMES) -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1885827 Title: building plugin failed on Windows with mingw Status in QEMU: New Bug description: I want to build QEMU 4.2.0's plugin module on Windows 7/10 with Mingw, but the building process faild. The step I follow is listed below: 1. create "dsp_build" diretory under source file folder 2. change directory to dsp_build , and run ../configure --target-list=dsp-softmmu --cross-prefix=x86_64-w64-mingw32- --enable-gtk --enable-sdl --enable-debug --enable-plugins 3. build qemu project 4. switch dir to /dsp_build, make -C tests/plugin, yeilds error: CC bb.o D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:17:24: error: variable 'qemu_plugin_version' definition is marked dllimport 17 | QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; |^~~ D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:17:24: warning: 'qemu_plugin_version' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c: In function 'vcpu_tb_exec': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:33:29: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] 33 | unsigned long n_insns = (unsigned long)udata; | ^ D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c: In function 'vcpu_tb_trans': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:51:46: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 51 | (void *)n_insns); 5. Then , I modified the QEMU_flags and the compilation command arguments($(CC) ..) in the makefile : BUILD_DIR := $(CURDIR)/../.. include $(BUILD_DIR)/config-host.mak include $(SRC_PATH)/rules.mak $(call set-vpath, $(SRC_PATH)/tests/plugin) NAMES := NAMES += bb NAMES += empty NAMES += insn NAMES += mem NAMES += hotblocks NAMES += howvec NAMES += hotpages SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES))) QEMU_CFLAGS += -fPIC-DBUILDING_DLL #added -DBUILDING_DLL QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu all: $(SONAMES) lib%.so: %.o $(CC) -fPIC -shared -o $@ $^ $(LDLIBS) -L /c/msys64/mingw64/lib/ -lglib-2.0 # original cmd: $(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS) clean: rm -f *.o *.so *.d rm -Rf .libs .PHONY: all clean 6. Executing make yeilds: make: enter “/d/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/build_dsp/tests/plugin” CC bb.o x86_64-w64-mingw32-gcc -fPIC -shared -o libbb.so bb.o -L /c/msys64/mingw64/lib/ -lglib-2.0 C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: bb.o: in function `plugin_exit': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:28: undefined reference to `qemu_plugin_outs' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:29: undefined reference to `__stack_chk_fail' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: bb.o: in function `vcpu_tb_trans': D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:41: undefined reference to `qemu_plugin_tb_n_insns' C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/emu_devl/qemu_src/qemu-sr-dsp-a/qemu_tidsp_c3x/tests/plugin/bb.c:44: undefined reference to `qemu_plugin_register_vcpu_tb_exec_inline'
Re: [PATCH v2 0/2] tests/qht-bench: Adjust rate/threshold computation
On Fri, Jun 26, 2020 at 13:09:48 -0700, Richard Henderson wrote: > Supercedes: <20200620214551.447392-1-richard.hender...@linaro.org> > > Thanks for Emilio's review of v1. I've split "seed" from "rate" > as suggested, left the comparisons alone, and expanded the comment > in do_threshold. Reviewed-by: Emilio G. Cota for the series. Thanks a lot! E.
Re: [PATCH] tests/qht-bench: Adjust rate computation and comparisons
Cc'ing Philippe, who authored the fix for this in May as I mention below.
Emilio
On Sun, Jun 21, 2020 at 17:28:25 -0400, Emilio G. Cota wrote:
> On Sat, Jun 20, 2020 at 14:45:51 -0700, Richard Henderson wrote:
> > Use <= comparisons vs the threshold, so that threshold UINT64_MAX
> > is always true, corresponding to rate 1.0 being unity. Simplify
> > do_threshold scaling to 2**64, with a special case for 1.0.
> >
> > Cc: Emilio G. Cota
> > Signed-off-by: Richard Henderson
> > ---
> > tests/qht-bench.c | 15 +++
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/qht-bench.c b/tests/qht-bench.c
> > index eb88a90137..21b1b7de82 100644
> > --- a/tests/qht-bench.c
> > +++ b/tests/qht-bench.c
> > @@ -132,7 +132,7 @@ static void do_rz(struct thread_info *info)
> > {
> > struct thread_stats *stats = >stats;
> >
> > -if (info->r < resize_threshold) {
> > +if (info->r <= resize_threshold) {
> > size_t size = info->resize_down ? resize_min : resize_max;
> > bool resized;
>
> This works, but only because info->r cannot be 0 since xorshift never
> returns it. (xorshift returns a random number in the range [1, u64max],
> a fact that I missed when I wrote this code.)
> If r were 0, then we would resize even if resize_threshold == 0.0.
>
> I think it will be easier to reason about this if we rename info->r
> to info->seed, and then have a local r = info->seed - 1. Then we can keep
> the "if random < threshold" form (and its negated "if random >= threshold"
> as below), which (at least to me) is intuitive provided that random's range
> is [0, threshold), e.g. [0.0, 1.0) with drand48(3).
>
> > @@ -154,7 +154,7 @@ static void do_rw(struct thread_info *info)
> > uint32_t hash;
> > long *p;
> >
> > -if (info->r >= update_threshold) {
> > +if (info->r > update_threshold) {
> > bool read;
> >
> > p = [info->r & (lookup_range - 1)];
> > @@ -281,11 +281,18 @@ static void pr_params(void)
> >
> > static void do_threshold(double rate, uint64_t *threshold)
> > {
> > +/*
> > + * For 0 <= rate <= 1, scale to fit in a uint64_t.
> > + *
> > + * For rate == 1, returning UINT64_MAX means 100% certainty: all
> > + * uint64_t will match using <=. The largest representable value
> > + * for rate less than 1 is 0.999889; scaling that
> > + * by 2**64 results in 0xf800.
> > + */
> > if (rate == 1.0) {
> > *threshold = UINT64_MAX;
> > } else {
> > -*threshold = (rate * 0xull)
> > - + (rate * 0xull);
> > +*threshold = rate * 0x1p64;
>
> I'm sorry this caused a breakage for some integration tests; I thought
> this was fixed in May with:
> https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg01477.html
>
> Just for my own education, why isn't nextafter needed here?
>
> Thanks,
> Emilio
Re: [PATCH v2 3/6] cputlb: ensure we save the IOTLB data in case of reset
On Mon, Jun 22, 2020 at 10:02:50 +0100, Alex Bennée wrote:
> Emilio G. Cota writes:
> > On Wed, Jun 10, 2020 at 16:55:06 +0100, Alex Bennée wrote:
(snip)
> >> +#ifdef CONFIG_PLUGIN
> >> +
> >> +typedef struct SavedIOTLB {
> >> +struct rcu_head rcu;
> >> +struct SavedIOTLB **save_loc;
> >> +MemoryRegionSection *section;
> >> +hwaddr mr_offset;
> >> +} SavedIOTLB;
> >> +
> >> +static void clean_saved_entry(SavedIOTLB *s)
> >> +{
> >> +atomic_rcu_set(s->save_loc, NULL);
> >
> > This will race with the CPU thread that sets saved_for_plugin in
> > save_iotlb_data().
>
> Surely that only happens outside the critical section?
I am not sure which critical section you're referring to.
With call_rcu() we defer the execution of the function to the RCU
thread at a later time, where "later time" is defined as any time
after the pre-existing RCU read critical sections have elapsed.
So we could have the RCU thread clearing the variable while the
CPU thread, which is in a _later_ RCU read critical section, is
setting said variable. This is the race I was referring to.
Thanks,
Emilio
Re: [PATCH] tests/qht-bench: Adjust rate computation and comparisons
On Sat, Jun 20, 2020 at 14:45:51 -0700, Richard Henderson wrote:
> Use <= comparisons vs the threshold, so that threshold UINT64_MAX
> is always true, corresponding to rate 1.0 being unity. Simplify
> do_threshold scaling to 2**64, with a special case for 1.0.
>
> Cc: Emilio G. Cota
> Signed-off-by: Richard Henderson
> ---
> tests/qht-bench.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qht-bench.c b/tests/qht-bench.c
> index eb88a90137..21b1b7de82 100644
> --- a/tests/qht-bench.c
> +++ b/tests/qht-bench.c
> @@ -132,7 +132,7 @@ static void do_rz(struct thread_info *info)
> {
> struct thread_stats *stats = >stats;
>
> -if (info->r < resize_threshold) {
> +if (info->r <= resize_threshold) {
> size_t size = info->resize_down ? resize_min : resize_max;
> bool resized;
This works, but only because info->r cannot be 0 since xorshift never
returns it. (xorshift returns a random number in the range [1, u64max],
a fact that I missed when I wrote this code.)
If r were 0, then we would resize even if resize_threshold == 0.0.
I think it will be easier to reason about this if we rename info->r
to info->seed, and then have a local r = info->seed - 1. Then we can keep
the "if random < threshold" form (and its negated "if random >= threshold"
as below), which (at least to me) is intuitive provided that random's range
is [0, threshold), e.g. [0.0, 1.0) with drand48(3).
> @@ -154,7 +154,7 @@ static void do_rw(struct thread_info *info)
> uint32_t hash;
> long *p;
>
> -if (info->r >= update_threshold) {
> +if (info->r > update_threshold) {
> bool read;
>
> p = [info->r & (lookup_range - 1)];
> @@ -281,11 +281,18 @@ static void pr_params(void)
>
> static void do_threshold(double rate, uint64_t *threshold)
> {
> +/*
> + * For 0 <= rate <= 1, scale to fit in a uint64_t.
> + *
> + * For rate == 1, returning UINT64_MAX means 100% certainty: all
> + * uint64_t will match using <=. The largest representable value
> + * for rate less than 1 is 0.999889; scaling that
> + * by 2**64 results in 0xf800.
> + */
> if (rate == 1.0) {
> *threshold = UINT64_MAX;
> } else {
> -*threshold = (rate * 0xull)
> - + (rate * 0xull);
> +*threshold = rate * 0x1p64;
I'm sorry this caused a breakage for some integration tests; I thought
this was fixed in May with:
https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg01477.html
Just for my own education, why isn't nextafter needed here?
Thanks,
Emilio
Re: [PATCH v2 3/6] cputlb: ensure we save the IOTLB data in case of reset
On Wed, Jun 10, 2020 at 16:55:06 +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
>
> ---
> v2
> - save the entry instead of re-running the tlb_fill.
>
> squash! cputlb: ensure we save the IOTLB entry in case of reset
> ---
> accel/tcg/cputlb.c | 63 --
> 1 file changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index eb2cf9de5e6..9bf9e479c7c 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1058,6 +1058,47 @@ static uint64_t io_readx(CPUArchState *env,
> CPUIOTLBEntry *iotlbentry,
> return val;
> }
>
> +#ifdef CONFIG_PLUGIN
> +
> +typedef struct SavedIOTLB {
> +struct rcu_head rcu;
> +struct SavedIOTLB **save_loc;
> +MemoryRegionSection *section;
> +hwaddr mr_offset;
> +} SavedIOTLB;
> +
> +static void clean_saved_entry(SavedIOTLB *s)
> +{
> +atomic_rcu_set(s->save_loc, NULL);
This will race with the CPU thread that sets saved_for_plugin in
save_iotlb_data().
> +g_free(s);
> +}
> +
> +static __thread SavedIOTLB *saved_for_plugin;
Apologies if this has been discussed, but why is this using TLS
variables and not state embedded in CPUState?
I see that qemu_plugin_get_hwaddr does not take a cpu_index, but
maybe it should? We could then just embed the RCU pointer in CPUState.
> +
> +/*
> + * 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(MemoryRegionSection *section, hwaddr mr_offset)
> +{
> +SavedIOTLB *s = g_new(SavedIOTLB, 1);
> +s->save_loc = _for_plugin;
> +s->section = section;
> +s->mr_offset = mr_offset;
> +atomic_rcu_set(_for_plugin, s);
> +call_rcu(s, clean_saved_entry, rcu);
Here we could just publish the new pointer and g_free_rcu the old
one, if any.
> +}
> +
> +#else
> +static void save_iotlb_data(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)
> @@ -1077,6 +1118,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(section, mr_offset);
> +
> if (mr->global_locking && !qemu_mutex_iothread_locked()) {
> qemu_mutex_lock_iothread();
> locked = true;
> @@ -1091,6 +1138,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry
> *iotlbentry,
> MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
> retaddr);
> }
> +
Stray whitespace change.
> if (locked) {
> qemu_mutex_unlock_iothread();
> }
> @@ -1366,8 +1414,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 thread local copy of the io_tlb entry.
> */
>
> bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
> @@ -1391,6 +1442,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(_for_plugin);
> +if (saved) {
> +data->is_io = true;
> +data->v.io.section = saved->section;
> +data->v.io.offset = saved->mr_offset;
> +return true;
> +}
Shouldn't we check that the contents of the saved IOTLB match the
parameters of the lookup? Otherwise passing a random address is likely
to land
Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device
On Tue, Jun 09, 2020 at 12:09:54 +0100, Alex Bennée wrote: > How about a g_intern_string() as a non-freeable const char that can also > be treated as canonical? I like it. Didn't know about g_intern_string (I see it's implemented as an append-only hash table protected by a lock). Cheers, Emilio
Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device
On Mon, Jun 08, 2020 at 09:06:17 +0100, Alex Bennée wrote:
> Emilio G. Cota writes:
> > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
> > have to worry about glib, and on the QEMU side we don't have to worry
> > about plugins calling free() instead of g_free().
>
> AFAIK you can actually mix free/g_free because g_free is just a NULL
> checking wrapper around free.
I was just going with the documentation, but you're right:
https://github.com/GNOME/glib/blob/mainline/glib/gmem.c#L196
> void
> g_free (gpointer mem)
> {
> free (mem);
> TRACE(GLIB_MEM_FREE((void*) mem));
> }
The NULL-pointer check is done by free(3), though.
> However ideally I'd be passing a
> non-freeable const char to the plugin but I didn't want to expose
> pointers deep inside of QEMU's guts although maybe I'm just being
> paranoid there given you can easily gdb the combined operation anyway.
>
> Perhaps there is a need for a separate memory region where we can store
> copies of strings we have made for the plugins?
I agree with the idea of not exposing internal pointers to plugins
(e.g. we don't pass a CPUState *, only an opaque handle) so I'm OK
with returning a dup'ed string here.
(snip)
> That said in another
> thread Peter was uncomfortable about exposing this piece of information
> to plugins. Maybe we should only expose something based on the optional
> -device foo,id=bar parameter?
I have no opinion on whether exposing this is a good idea. If it turns
out that it is, please have my
Reviewed-by: Emilio G. Cota
Thanks,
Emilio
Re: [PATCH v2 13/13] tests: Disable select tests under TSan, which hit TSan issue.
On Fri, Jun 05, 2020 at 13:34:22 -0400, Robert Foley wrote: > Disable a few tests under CONFIG_TSAN, which > run into a known TSan issue that results in a hang. > https://github.com/google/sanitizers/issues/1116 > > The disabled tests under TSan include all the qtests as well as > the test-char, test-qga, and test-qdev-global-props. > > Signed-off-by: Robert Foley Reviewed-by: Emilio G. Cota Thanks, Emilio
Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device
On Tue, Jun 02, 2020 at 16:46:22 +0100, Alex Bennée wrote:
> This may well end up being anonymous but it should always be unique.
>
> Signed-off-by: Alex Bennée
> ---
> include/qemu/qemu-plugin.h | 5 +
> plugins/api.c | 18 ++
> 2 files changed, 23 insertions(+)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index bab8b0d4b3a..43c6a9e857f 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -335,6 +335,11 @@ 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. Plugin must free() it
> + */
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr
> *haddr);
> +
> 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 bbdc5a4eb46..3c73de8c1c2 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct
> qemu_plugin_hwaddr *haddr
> return 0;
> }
>
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
> +{
> +#ifdef CONFIG_SOFTMMU
> +if (haddr && haddr->is_io) {
> +MemoryRegionSection *mrs = haddr->v.io.section;
> +if (!mrs->mr->name) {
> +return g_strdup_printf("anon%08lx", 0x & (uintptr_t)
> mrs->mr);
> +} else {
> +return g_strdup(mrs->mr->name);
> +}
> +} else {
> +return g_strdup("RAM");
> +}
> +#else
> +return g_strdup("Invalid");
> +#endif
> +}
I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
have to worry about glib, and on the QEMU side we don't have to worry
about plugins calling free() instead of g_free().
Or given that this doesn't look perf-critical, perhaps an easier way out
is to wrap the above with:
char *g_str = above();
char *ret = strdup(g_str);
g_free(g_str);
return ret;
Not sure we should NULL-check ret, since I don't know whether
mrs->mr->name is guaranteed to be non-NULL.
Thanks,
Emilio
Re: [PATCH v1 11/12] util: Added tsan annotate for thread name.
On Fri, May 29, 2020 at 09:23:41 -0400, Robert Foley wrote: > This allows us to see the name of the thread in tsan > warning reports such as this: > > Thread T7 'CPU 1/TCG' (tid=24317, running) created by main thread at: > > Signed-off-by: Robert Foley Reviewed-by: Emilio G. Cota Thanks, Emilio
Re: [PATCH] scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header
On Sun, May 24, 2020 at 23:56:54 +0200, Philippe Mathieu-Daudé wrote: > "qemu/qemu-plugin.h" isn't meant to be include by QEMU codebase, > but by 3rd party plugins that QEMU can use. These plugins can be > built out of QEMU and don't include "qemu/osdep.h". > Mark "qemu/qemu-plugin.h" as a special header that doesn't need > to be cleaned for "qemu/osdep.h". > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Emilio G. Cota Thanks, Emilio
[PATCH] qemu-plugin.h: add missing include to define size_t
Signed-off-by: Emilio G. Cota --- include/qemu/qemu-plugin.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h index 89ed579f55..bab8b0d4b3 100644 --- a/include/qemu/qemu-plugin.h +++ b/include/qemu/qemu-plugin.h @@ -12,6 +12,7 @@ #include #include +#include /* * For best performance, build the plugin with -fvisibility=hidden so that -- 2.25.1
Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times
Hi Alex, On Tue, May 12, 2020 at 21:11:46 +0100, Alex Bennée wrote: > > Emilio G. Cota writes: > > > On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote: > >> Attached to the mail counter.c when running with attached test.c compiled > >> to Linux standalone binary shows failing assert, unless the patch is > >> applied. > > > > I didn't get the attachment. Can you paste the code at the end of your > > reply? > > So the problem is we are recycling cpu_index: > > 21:01:54 [alex@zen:~/l/q/b/all.plugin] plugins/next|●2✚3…(+8 insertions(+)) > 127 + ./x86_64-linux-user/qemu-x86_64 > ./tests/tcg/x86_64-linux-user/threadcount > cpu_list_add: 0x5605598579f0 > cpu_get_free_index: 0 > cpu_list_add: 0x5605598c6010 > cpu_get_free_index: 1 > cpu_list_add: 0x5605598ec330 > cpu_get_free_index: 2 > cpu_list_add: 0x560559912a30 > cpu_get_free_index: 3 > cpu_list_add: 0x560559939130 > cpu_get_free_index: 4 > cpu_list_add: 0x56055995d360 > cpu_get_free_index: 1 > ** > > ERROR:/home/alex/lsrc/qemu.git/plugins/core.c:205:qemu_plugin_vcpu_init_hook: > assertion failed: (success) > qemu:handle_cpu_signal received signal outside vCPU context @ > pc=0x7fa29c702613 > > However I'm confused as to how. The cpu_index is really just a function > of the number of CPUs in the cpus list and I can't see where we free > them up. We never call cpu_common_unrealizefn so the list should never > shrink. We should just end up with a slowly growing list of now dead CPU > states. > > I'm giving up for now but will have a look again in the morning if no > one else has figured out what is going on by then. I just got this fixed, only to realise that you already fixed it weeks ago :-) (I didn't see your fixes because I'm not subscribed to the list.) My changes are here https://github.com/cota/qemu/tree/cpu_index Please feel free to take anything from there that you find valuable, e.g. the use of a bitmap for tracking cpu_index'es and the addition of a unit test (note that the test is fixed in the last commit). Thanks, Emilio
Re: [PATCH 10/19] include/qemu: Added tsan.h for annotations.
On Sat, May 23, 2020 at 13:20:15 -0400, Emilio G. Cota wrote: > On Fri, May 22, 2020 at 12:07:46 -0400, Robert Foley wrote: > > These annotations will allow us to give tsan > > additional hints. For example, we can inform > > tsan about reads/writes to ignore to silence certain > > classes of warnings. > > We can also annotate threads so that the proper thread > > naming shows up in tsan warning results. > > > > Signed-off-by: Robert Foley > > Reviewed-by: Emilio G. Cota Although consider prefixing the constants with QEMU_. Thanks, E.
Re: [PATCH 00/19] Add Thread Sanitizer support to QEMU
On Fri, May 22, 2020 at 12:07:36 -0400, Robert Foley wrote: > This patch series continues the work done by Emilio Cota and others to add > Thread Sanitizer (TSan) support to QEMU. > > The starting point for this work was Emilio's branch here: > https://github.com/cota/qemu/commits/tsan > specifically this commit: 0be125fc0afd47218b34d2019abdd19b644f3199 > > The purpose of this patch is not to fix all the TSan warnings, but to enable > the TSan support so that QEMU developers can start using the tool. > We found this tool useful and even ran it on our recent changes in > the cpu-locks series. > Clearly there is work to do here to clean up all the warnings. :) > We have made a start to cleaning up these warnings by getting a VM to boot > cleanly with no TSan warnings. > We have also made an effort to introduce enough of the TSan suppression > mechanisms, so that others can continue this work. Thank you for doing this work! Great to see this finally coming along. What are your plans wrt the per-cpu-lock branch? Given that some of the races reported here are fixed there, I think it would make sense to defer those patches to the per-cpu-lock series (i.e. patches 2/19, parts of 13/19, and 18/19) so that this series goes in first (it is a lot less likely to break anything). Also, I would not worry too much about rushing to bring warnings to 0; what's important is that with the warnings we now know where to focus on, and then we can carefully fix races. In particular I think all annotations we add must have a comment, since otherwise we are at the risk of blindlessly silencing warnings of real races. > Issues: > - When running docker-test-quick under TSan there are several tests which hang > - The unit tests which seem to hang under TSan: > test-char, test-qdev-global-props, and test-qga. > - If we comment out those tests, check-unit finishes, albeit with > a couple of warnings. :) I've noticed another issue (that I did not notice on my previous machine), which is that tsan blows up when in qht we lock all of the bucket's locks. We then get this assert from tsan, since it has a static limit of 64 mutexes held at any given time: FATAL: ThreadSanitizer CHECK failed: /build/llvm-toolchain-10-yegZYJ/llvm-toolchain-10-10.0.0/compiler-rt/lib/sanitizer_common/sanitizer_deadlock_detector.h:67 "((n_all_locks_)) < (((sizeof(all_locks_with_contexts_)/sizeof((all_locks_with_contexts_)[0]" (0x40, 0x40) #0 __tsan::TsanCheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (qemu-system-aarch64+0x49f9f5) #1 __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (qemu-system-aarch64+0x4b651f) #2 __sanitizer::DeadlockDetectorTLS<__sanitizer::TwoLevelBitVector<1ul, __sanitizer::BasicBitVector > >::addLock(unsigned long, unsigned long, unsigned int) (qemu-system-aarch64+0x4aacbc) #3 __sanitizer::DD::MutexAfterLock(__sanitizer::DDCallback*, __sanitizer::DDMutex*, bool, bool) (qemu-system-aarch64+0x4aa22e) #4 __tsan::MutexPostLock(__tsan::ThreadState*, unsigned long, unsigned long, unsigned int, int) (qemu-system-aarch64+0x49ded8) #5 __tsan_mutex_post_lock (qemu-system-aarch64+0x48175a) #6 qemu_spin_lock /home/cota/src/qemu/include/qemu/thread.h:245:5 (qemu-system-aarch64+0xb1283b) #7 qht_map_lock_buckets /home/cota/src/qemu/util/qht.c:253:9 (qemu-system-aarch64+0xb1283b) A workaround for now is to run qemu with TSAN_OPTIONS=detect_deadlocks=0, although selectively disabling tsan for qht_map_lock/unlock_buckets would be ideal (not sure if it's possible). Another warning I'm reliably seen is: WARNING: ThreadSanitizer: double lock of a mutex (pid=489006) #0 pthread_mutex_lock (qemu-system-aarch64+0x457596) #1 qemu_mutex_lock_impl /home/cota/src/qemu/util/qemu-thread-posix.c:79:11 (qemu-system-aarch64+0xaf7e3c) Location is heap block of size 328 at 0x7b4800114900 allocated by main thread: #0 calloc (qemu-system-aarch64+0x438b80) #1 g_malloc0 (libglib-2.0.so.0+0x57d30) Mutex M57 (0x7b4800114960) created at: #0 pthread_mutex_init (qemu-system-aarch64+0x43b74d) #1 qemu_rec_mutex_init /home/cota/src/qemu/util/qemu-thread-posix.c:119:11 (qemu-system-aarch64+0xaf815b) But this one seems safe to ignore. Thanks, E.
Re: [PATCH 15/19] qht: Fix tsan warnings.
On Fri, May 22, 2020 at 12:07:51 -0400, Robert Foley wrote: > For example: > WARNING: ThreadSanitizer: data race (pid=23406) > Atomic read of size 4 at 0x7b13e3c8 by thread T7: > #0 __tsan_atomic32_load (qemu-system-aarch64+0x39a36c) > #1 qht_do_lookup util/qht.c:495:17 (qemu-system-aarch64+0xd82f7a) > #2 qht_lookup_custom util/qht.c:539:11 (qemu-system-aarch64+0xd82f7a) > Previous write of size 8 at 0x7b13e3c8 by thread T6 (mutexes: write > M166769147697783108, write M995435858420506688): > #0 posix_memalign (qemu-system-aarch64+0x350dd1) > #1 qemu_try_memalign util/oslib-posix.c:189:11 > (qemu-system-aarch64+0xd59317) > #2 qemu_memalign util/oslib-posix.c:205:27 (qemu-system-aarch64+0xd5943e) > #3 qht_insert__locked util/qht.c:583:9 (qemu-system-aarch64+0xd837c5) > > Signed-off-by: Robert Foley > --- > util/qht.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/util/qht.c b/util/qht.c > index 67e5d5b916..739a53ced0 100644 > --- a/util/qht.c > +++ b/util/qht.c > @@ -69,6 +69,7 @@ > #include "qemu/qht.h" > #include "qemu/atomic.h" > #include "qemu/rcu.h" > +#include "qemu/tsan.h" > > //#define QHT_DEBUG > > @@ -580,10 +581,12 @@ static void *qht_insert__locked(const struct qht *ht, > struct qht_map *map, > b = b->next; > } while (b); > > +TSAN_ANNOTATE_IGNORE_WRITES_BEGIN(); > b = qemu_memalign(QHT_BUCKET_ALIGN, sizeof(*b)); > memset(b, 0, sizeof(*b)); > new = b; > i = 0; > +TSAN_ANNOTATE_IGNORE_WRITES_END(); I cannot reproduce this warning post-series with detect_deadlocks=0 but my hypothesis is that this is a side effect of tsan not understanding the seqlock: tsan sees that below we "publish" this piece of memory with an atomic write (in atomic_rcu_set), and does not see that with seqlock_write_begin we have a write memory barrier. I wonder if what we need instead is to annotate the seqlock functions, not the callers. Thanks, E.
Re: [PATCH 19/19] docs: Added details on TSan to testing.rst
On Fri, May 22, 2020 at 12:07:55 -0400, Robert Foley wrote: > This includes details on how to build and test with TSan > both inside a docker and outside. > > Signed-off-by: Robert Foley Reviewed-by: Emilio G. Cota E.
Re: [PATCH 17/19] util: Added tsan annotate for thread name.
On Fri, May 22, 2020 at 12:07:53 -0400, Robert Foley wrote: > This allows us to see the name of the thread in tsan > warning reports such as this: > > Thread T7 'CPU 1/TCG' (tid=24317, running) created by main thread at: > > Signed-off-by: Robert Foley Reviewed-by: Emilio G. Cota E.
Re: [PATCH 14/19] util/async: Fixed tsan warnings
On Fri, May 22, 2020 at 12:07:50 -0400, Robert Foley wrote:
> For example:
> Atomic write of size 8 at 0x7b4800113c28 by main thread (mutexes: write
> M30):
> #0 __tsan_atomic64_exchange (qemu-system-aarch64+0x386f85)
> #1 aio_bh_poll util/async.c:146:5 (qemu-system-aarch64+0xcd1f61)
> #2 aio_dispatch util/aio-posix.c:380:5 (qemu-system-aarch64+0xcd8abb)
> #3 aio_ctx_dispatch util/async.c:298:5 (qemu-system-aarch64+0xcd31b0)
> #4 g_main_context_dispatch (libglib-2.0.so.0+0x4c416)
> #5 qemu_main_loop softmmu/vl.c:1664:9 (qemu-system-aarch64+0x5cc6d6)
> #6 main softmmu/main.c:49:5 (qemu-system-aarch64+0xc62857)
>
> Previous read of size 8 at 0x7b4800113c28 by thread T3 (mutexes: write M81):
> #0 aio_bh_enqueue util/async.c:81:9 (qemu-system-aarch64+0xcd2267)
> #1 qemu_bh_schedule util/async.c:181:5 (qemu-system-aarch64+0xcd2267)
> #2 worker_thread util/thread-pool.c:113:9 (qemu-system-aarch64+0xcd473c)
> #3 qemu_thread_start util/qemu-thread-posix.c:519:9
> (qemu-system-aarch64+0xcde280)
>
> Cc: Stefan Hajnoczi
> Cc: Fam Zheng
> Signed-off-by: Robert Foley
> ---
> util/async.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/util/async.c b/util/async.c
> index 1319eee3bc..51e306bf0c 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -33,6 +33,7 @@
> #include "block/raw-aio.h"
> #include "qemu/coroutine_int.h"
> #include "trace.h"
> +#include "qemu/tsan.h"
>
> /***/
> /* bottom halves (can be seen as timers which expire ASAP) */
> @@ -76,10 +77,12 @@ static void aio_bh_enqueue(QEMUBH *bh, unsigned new_flags)
> * 2. ctx is loaded before the callback has a chance to execute and bh
> *could be freed.
> */
> +TSAN_ANNOTATE_IGNORE_WRITES_BEGIN();
Why do we need these annotations here? It looks like the fix for the
race in the commit message is below (i.e. atomic_read).
In general, I'd expect annotations to come with a comment, since
they should be used sparingly. That is, the assumption is that
tsan is almost always right.
> old_flags = atomic_fetch_or(>flags, BH_PENDING | new_flags);
> if (!(old_flags & BH_PENDING)) {
> QSLIST_INSERT_HEAD_ATOMIC(>bh_list, bh, next);
> }
> +TSAN_ANNOTATE_IGNORE_WRITES_END();
>
> aio_notify(ctx);
> }
> @@ -143,7 +146,9 @@ int aio_bh_poll(AioContext *ctx)
> BHListSlice *s;
> int ret = 0;
>
> +TSAN_ANNOTATE_IGNORE_WRITES_BEGIN();
> QSLIST_MOVE_ATOMIC(_list, >bh_list);
> +TSAN_ANNOTATE_IGNORE_WRITES_END();
ditto
> QSIMPLEQ_INSERT_TAIL(>bh_slice_list, , next);
>
> while ((s = QSIMPLEQ_FIRST(>bh_slice_list))) {
> @@ -280,14 +285,16 @@ aio_ctx_check(GSource *source)
> aio_notify_accept(ctx);
>
> QSLIST_FOREACH_RCU(bh, >bh_list, next) {
> -if ((bh->flags & (BH_SCHEDULED | BH_DELETED)) == BH_SCHEDULED) {
> +if ((atomic_read(>flags) & (BH_SCHEDULED | BH_DELETED))
> + == BH_SCHEDULED) {
> return true;
> }
> }
>
> QSIMPLEQ_FOREACH(s, >bh_slice_list, next) {
> QSLIST_FOREACH_RCU(bh, >bh_list, next) {
> -if ((bh->flags & (BH_SCHEDULED | BH_DELETED)) == BH_SCHEDULED) {
> +if ((atomic_read(>flags) & (BH_SCHEDULED | BH_DELETED))
> + == BH_SCHEDULED) {
This hunk like the real fix. Also, I'd put "fix race" in the commit
title as opposed to "fix warning" since fixing races is the goal, not
fixing warnings.
Thanks,
Emilio
Re: [PATCH 13/19] accel/tcg: Fixed tsan warnings.
On Fri, May 22, 2020 at 12:07:49 -0400, Robert Foley wrote:
> For example:
> WARNING: ThreadSanitizer: data race (pid=35425)
> Write of size 4 at 0x7bbc00ac by main thread (mutexes: write M875):
> #0 cpu_reset_interrupt hw/core/cpu.c:107:28 (qemu-system-aarch64+0x843790)
> #1 arm_cpu_set_irq target/arm/cpu.c (qemu-system-aarch64+0x616265)
> #2 qemu_set_irq hw/core/irq.c:44:5 (qemu-system-aarch64+0x8462ca)
> Previous atomic read of size 4 at 0x7bbc00ac by thread T6:
> #0 __tsan_atomic32_load (qemu-system-aarch64+0x394c1c)
> #1 cpu_handle_interrupt accel/tcg/cpu-exec.c:534:9
> (qemu-system-aarch64+0x4b7e79)
> #2 cpu_exec accel/tcg/cpu-exec.c:720:17 (qemu-system-aarch64+0x4b7e79)
> or
> WARNING: ThreadSanitizer: data race (pid=25425)
> Read of size 8 at 0x7f8ad8e138d0 by thread T10:
> #0 tb_lookup_cmp accel/tcg/cpu-exec.c:307:13
> (qemu-system-aarch64+0x4ac4d2)
> #1 qht_do_lookup util/qht.c:502:34 (qemu-system-aarch64+0xd05264)
> Previous write of size 8 at 0x7f8ad8e138d0 by thread T15 (mutexes: write
> M728311726235541804):
> #0 tb_link_page accel/tcg/translate-all.c:1625:26
> (qemu-system-aarch64+0x4b0bf2)
> #1 tb_gen_code accel/tcg/translate-all.c:1865:19
> (qemu-system-aarch64+0x4b0bf2)
> #2 tb_find accel/tcg/cpu-exec.c:407:14 (qemu-system-aarch64+0x4ad77c)
I see you're working through the warnings in this file, but I think it would
be better to forget about files and focus on the data itself.
Therefore this patch should be split in two: (1) cpu- Cc: Richard Henderson
> Cc: Paolo Bonzini
> Signed-off-by: Robert Foley
> ---
> accel/tcg/tcg-all.c | 4 ++--
> accel/tcg/tcg-runtime.c | 7 ++-
> accel/tcg/translate-all.c | 6 +-
> hw/core/cpu.c | 2 +-
> 4 files changed, 14 insertions(+), 5 deletions(-)
>
(snip)
> diff --git a/accel/tcg/tcg-runtime.c b/accel/tcg/tcg-runtime.c
> index 446465a09a..bd0cd77450 100644
> --- a/accel/tcg/tcg-runtime.c
> +++ b/accel/tcg/tcg-runtime.c
> @@ -31,6 +31,7 @@
> #include "disas/disas.h"
> #include "exec/log.h"
> #include "tcg/tcg.h"
> +#include "qemu/tsan.h"
>
> /* 32-bit helpers */
>
> @@ -151,6 +152,7 @@ void *HELPER(lookup_tb_ptr)(CPUArchState *env)
> TranslationBlock *tb;
> target_ulong cs_base, pc;
> uint32_t flags;
> +void *tc_ptr;
>
> tb = tb_lookup__cpu_state(cpu, , _base, , curr_cflags());
> if (tb == NULL) {
> @@ -161,7 +163,10 @@ void *HELPER(lookup_tb_ptr)(CPUArchState *env)
> TARGET_FMT_lx "/" TARGET_FMT_lx "/%#x] %s\n",
> cpu->cpu_index, tb->tc.ptr, cs_base, pc, flags,
> lookup_symbol(pc));
> -return tb->tc.ptr;
> +TSAN_ANNOTATE_IGNORE_READS_BEGIN();
> +tc_ptr = tb->tc.ptr;
> +TSAN_ANNOTATE_IGNORE_READS_END();
> +return tc_ptr;
I'm not sure these are needed. At least after applying all other patches
in this series, I don't get a warning here.
> }
>
> void HELPER(exit_atomic)(CPUArchState *env)
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 3fb71a1503..6c0e61994c 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -58,6 +58,7 @@
> #include "exec/log.h"
> #include "sysemu/cpus.h"
> #include "sysemu/tcg.h"
> +#include "qemu/tsan.h"
>
> /* #define DEBUG_TB_INVALIDATE */
> /* #define DEBUG_TB_FLUSH */
> @@ -1704,6 +1705,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> max_insns = 1;
> }
>
> +TSAN_ANNOTATE_IGNORE_WRITES_BEGIN();
Same here, I don't get a warning in this hunk if I remove these,
except for:
---
WARNING: ThreadSanitizer: data race (pid=445867)
Atomic read of size 1 at 0x7f906e050158 by thread T7:
#0 __tsan_mutex_post_lock (qemu-system-aarch64+0x481721)
#1 qemu_spin_lock /home/cota/src/qemu/include/qemu/thread.h:244:5
(qemu-system-aarch64+0x5578e9)
#2 tb_add_jump /home/cota/src/qemu/accel/tcg/cpu-exec.c:363:5
(qemu-system-aarch64+0x5578e9)
#3 tb_find /home/cota/src/qemu/accel/tcg/cpu-exec.c:423:9
(qemu-system-aarch64+0x5578e9)
Previous write of size 1 at 0x7f906e050158 by thread T8:
#0 __tsan_mutex_create (qemu-system-aarch64+0x481589)
#1 qemu_spin_init /home/cota/src/qemu/include/qemu/thread.h:221:5
(qemu-system-aarch64+0x559a71)
#2 tb_gen_code /home/cota/src/qemu/accel/tcg/translate-all.c:1875:5
(qemu-system-aarch64+0x559a71)
Thread T7 'CPU 0/TCG' (tid=445875, running) created by main thread at:
#0 pthread_create (qemu-system-aarch64+0x43915b)
#1 qemu_thread_create /home/cota/src/qemu/util/qemu-thread-posix.c:558:11
(qemu-system-aarch64+0xaf91ff)
Thread T8 'CPU 1/TCG' (tid=445876, running) created by main thread at:
#0 pthread_create (qemu-system-aarch64+0x43915b)
#1 qemu_thread_create /home/cota/src/qemu/util/qemu-thread-posix.c:558:11
(qemu-system-aarch64+0xaf91ff)
SUMMARY: ThreadSanitizer: data race
Re: [PATCH 12/19] configure: added tsan support for blacklist.
On Fri, May 22, 2020 at 12:07:48 -0400, Robert Foley wrote: > Initially put several files into blacklist that were > causing the most problems, namely bitops.c and bitmap.c. > > Signed-off-by: Robert Foley > --- > configure | 3 ++- > tests/tsan/blacklist.tsan | 5 + > 2 files changed, 7 insertions(+), 1 deletion(-) > create mode 100644 tests/tsan/blacklist.tsan > > diff --git a/configure b/configure > index c95c54fb48..8a86a0638d 100755 > --- a/configure > +++ b/configure > @@ -6306,7 +6306,8 @@ if test "$have_asan" = "yes"; then > fi > if test "$have_tsan" = "yes" ; then >if test "$have_tsan_iface_fiber" = "yes" ; then > -QEMU_CFLAGS="-fsanitize=thread $QEMU_CFLAGS" > +QEMU_CFLAGS="-fsanitize=thread -fsanitize-blacklist="\ > + "\$(SRC_PATH)/tests/tsan/blacklist.tsan $QEMU_CFLAGS" I presume the goal here is to fix these races later (my default assumption is that warnings == races, since most warnings are indeed races). If so, please consider making the suppression optional (via "--extra-cflags=-fsanitize-blacklist=path-to-this-file"), since that way the reports are likely to get more eyeballs. Thanks, E.
Re: [PATCH 11/19] accel/tcg: Fixed tsan warnings related to parallel_cpus
On Fri, May 22, 2020 at 12:07:47 -0400, Robert Foley wrote: > Fixed several tsan warnings. e.g. > > WARNING: ThreadSanitizer: data race (pid=35425) > Read of size 1 at 0x557cd83aee28 by thread T7: > #0 curr_cflags include/exec/exec-all.h:460:13 > (qemu-system-aarch64+0x4b7f27) > #1 cpu_exec accel/tcg/cpu-exec.c:730:26 (qemu-system-aarch64+0x4b7f27) > #2 tcg_cpu_exec cpus.c:1415:11 (qemu-system-aarch64+0x45b9b6) > #3 qemu_tcg_cpu_thread_fn cpus.c:1723:17 (qemu-system-aarch64+0x45b9b6) > #4 qemu_thread_start util/qemu-thread-posix.c:519:9 > (qemu-system-aarch64+0xd431e0) > > Previous write of size 1 at 0x557cd83aee28 by thread T6: > #0 cpu_exec_step_atomic accel/tcg/cpu-exec.c:254:23 > (qemu-system-aarch64+0x4b6caa) > #1 qemu_tcg_cpu_thread_fn cpus.c:1741:17 (qemu-system-aarch64+0x45baca) > #2 qemu_thread_start util/qemu-thread-posix.c:519:9 > (qemu-system-aarch64+0xd431e0) > > Location is global 'parallel_cpus' of size 1 at 0x557cd83aee28 > (qemu-system-aarch64+0x01fb3e28) > > Cc: Richard Henderson > Cc: Paolo Bonzini > Signed-off-by: Robert Foley Reviewed-by: Emilio G. Cota E.
Re: [PATCH 10/19] include/qemu: Added tsan.h for annotations.
On Fri, May 22, 2020 at 12:07:46 -0400, Robert Foley wrote: > These annotations will allow us to give tsan > additional hints. For example, we can inform > tsan about reads/writes to ignore to silence certain > classes of warnings. > We can also annotate threads so that the proper thread > naming shows up in tsan warning results. > > Signed-off-by: Robert Foley Reviewed-by: Emilio G. Cota E.
Re: [PATCH 18/19] target/arm: Fix tsan warning in cpu.c
On Fri, May 22, 2020 at 23:36:18 +0100, Peter Maydell wrote: > So is this: > (a) a TSan false positive, because we've analysed the use > of this struct field and know it's not a race because > [details], but which we're choosing to silence in this way > (b) an actual race for which the correct fix is to make the > accesses atomic because [details] > > ? It is (b), as shown in the per-cpu lock series. In particular, see these two patches: - [PATCH v9 33/74] cpu: define cpu_interrupt_request helpers https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg06322.html - [PATCH v9 39/74] arm: convert to cpu_interrupt_request https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg06328.html Since a more thorough fix is included in that other series, I think this patch should be dropped from this series -- I'll post a reply to patch 00/19 with more details. Thanks, Emilio
Re: [PATCH v8 74/74] cputlb: queue async flush jobs without the BQL
On Mon, May 18, 2020 at 09:46:36 -0400, Robert Foley wrote: > We re-ran the numbers with the latest re-based series. > > We used an aarch64 ubuntu VM image with a host CPU: > Intel(R) Xeon(R) Silver 4114 CPU @ 2.20GHz, 2 CPUs, 10 cores/CPU, > 20 Threads/CPU. 40 cores total. > > For the bare hardware and kvm tests (first chart) the host CPU was: > HiSilicon 1620 CPU 2600 Mhz, 2 CPUs, 64 Cores per CPU, 128 CPUs total. > > First, we ran a test of building the kernel in the VM. > We did not see any major improvements nor major regressions. > We show the results of the Speedup of building the kernel > on bare hardware compared with kvm and QEMU (both the baseline and cpu locks). > > >Speedup vs a single thread for kernel build > > 40 +--+ > | + + + + + + ** | > |bare hardwar* | > | kvm ### | > 35 |-+ baseline $$$-| > |*cpu lock %%% | > | *** | > | ** | > 30 |-+ *** +-| > | *** | > | *** | > |**| > 25 |-+ ***+-| > | *** | > |**| > | ** | > 20 |-+ ** +-| > | **# | > |** | > | ** ## | > |** ###| > 15 |-+ * +-| > | ** ### | > |*### | > | * ### | > 10 |-+ **### +-| > |*## | > | ## | > | #$ | >5 |-+ $%%%%%$%$%$%$%$%$%$%$%$%$%$%$%$%$%$%+-| > | %% % | > | %% | > |%+ + + + + + | >0 +--+ > 0 102030 40506070 >Guest vCPUs > > > After seeing these results and the scaling limits inherent in the build > itself, > we decided to run a test which might show the scaling improvements clearer. Thanks for doing these tests. I know from experience that benchmarking is hard and incredibly time consuming, so please do not be discouraged by my comments below. A couple of points: 1. I am not familiar with aarch64 KVM but I'd expect it to scale almost like the native run. Are you assigning enough RAM to the guest? Also, it can help to run the kernel build in a ramfs in the guest. 2. The build itself does not seem to impose a scaling limit, since it scales very well when run natively (per-thread I presume aarch64 TCG is still slower than native, even if TCG is run on a faster x86 machine). The limit here is probably aarch64 TCG. In particular, last time I checked aarch64 TCG has room for improvement scalability-wise handling interrupts and some TLB operations; this is likely to explain why we see no benefit with per-CPU locks, i.e. the bottleneck is elsewhere. This can be confirmed with the sync profiler. IIRC I originally used ppc64 for this test because ppc64 TCG does not have any other big bottlenecks scalability-wise. I just checked but unfortunately I can't find the ppc64 image I used :( What I can offer is the script I used to run these benchmarks; see the appended. Thanks,
Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times
On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote: > Attached to the mail counter.c when running with attached test.c compiled > to Linux standalone binary shows failing assert, unless the patch is > applied. I didn't get the attachment. Can you paste the code at the end of your reply? Thanks, Emilio
Re: [PATCH 0/3] plugins: Move declarations around and rename 'hwaddr' argument
On Sun, May 10, 2020 at 19:11:16 +0200, Philippe Mathieu-Daudé wrote: > Rename qemu_plugin_hwaddr_is_io() 'hwaddr' argument to later > allow declaration of the 'hwaddr' type to be poisoned (see [*]). > > [*] https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg02254.html > "exec/cpu: Poison 'hwaddr' type in user-mode emulation" > > Philippe Mathieu-Daudé (3): > qemu/plugin: Trivial code movement > qemu/plugin: Move !CONFIG_PLUGIN stubs altogether > qemu/qemu-plugin: Make qemu_plugin_hwaddr_is_io() hwaddr argument > const Reviewed-by: Emilio G. Cota for the series. Thanks, E.
Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times
On Mon, Apr 20, 2020 at 13:04:51 +0300, Nikolay Igotti wrote:
> In linux-user multithreaded scenarious CPU could be inited many times with
> the same id,
> so avoid assertions on already present hashtable entry.
>
> Signed-off-by: Nikolay Igotti
> ---
> plugins/core.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/plugins/core.c b/plugins/core.c
> index 51bfc94787..889cc6441a 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -196,13 +196,10 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum
> qemu_plugin_event ev,
>
> void qemu_plugin_vcpu_init_hook(CPUState *cpu)
> {
> -bool success;
> -
> qemu_rec_mutex_lock();
> plugin_cpu_update__locked(>cpu_index, NULL, NULL);
> -success = g_hash_table_insert(plugin.cpu_ht, >cpu_index,
> +g_hash_table_insert(plugin.cpu_ht, >cpu_index,
>>cpu_index);
> -g_assert(success);
> qemu_rec_mutex_unlock();
Do you have a reproducer for this? I'd expect (1) the g_hash_table_remove
call in qemu_plugin_vcpu_exit_hook to clear this entry upon CPU exit,
and (2) no two live CPUs to have the same cpu_index. But maybe assumption
(2) is wrong, or simply (1) does not get called for some exiting CPUs,
in which case the right fix would be to make sure that it does get called
on CPU exit.
Thanks,
Emilio
Re: [PATCH v3] tests/qht-bench: Fix Clang 'implicit-int-float-conversion' warning
On Mon, May 04, 2020 at 16:43:52 +0200, Philippe Mathieu-Daudé wrote:
> When building with Clang 10 on Fedora 32, we get:
>
> tests/qht-bench.c:287:29: error: implicit conversion from 'unsigned long'
> to 'double' changes value from 18446744073709551615 to 18446744073709551616
> [-Werror,-Wimplicit-int-float-conversion]
(snip)
> @@ -284,7 +285,7 @@ static void do_threshold(double rate, uint64_t *threshold)
> if (rate == 1.0) {
> *threshold = UINT64_MAX;
> } else {
> -*threshold = rate * UINT64_MAX;
> +*threshold = rate * nextafter(0x1p64, 0.0);
Reviewed-by: Emilio G. Cota
Please consider mentioning 25f74087c69 in the commit log -- it clearly
describes the problem.
Thanks,
Emilio
Re: [PATCH v8 00/74] per-CPU locks
(Apologies if I missed some Cc's; I was not Cc'ed in patch 0 so I'm blindly crafting a reply.) On Thu, Mar 26, 2020 at 15:30:43 -0400, Robert Foley wrote: > This is a continuation of the series created by Emilio Cota. > We are picking up this patch set with the goal to apply > any fixes or updates needed to get this accepted. Thanks for picking this up! > Listed below are the changes for this version of the patch, > aside from the merge related changes. > > Changes for V8: > - Fixed issue where in rr mode we could destroy the BQL twice. I remember doing little to no testing in record-replay mode, so there should be more bugs hiding in there :-) > - Found/fixed bug that had been hit in testing previously during > the last consideration of this patch. > We reproduced the issue hit in the qtest: bios-tables-test. > The issue was introduced by dropping the BQL, and found us > (very rarely) missing the condition variable wakeup in > qemu_tcg_rr_cpu_thread_fn(). Aah, this one: https://patchwork.kernel.org/patch/10838149/#22516931 How did you identify the problem? Was it code inspection or using a tool like rr? I remember this being hard to reproduce reliably. On a related note, I've done some work to get QEMU-system to work under thread sanitizer, since tsan now supports our longjmp-based coroutines (hurrah!). My idea was to integrate tsan in QEMU (i.e. bring tsan warnings to 0) before (re)trying to merge the per-CPU lock patchset; this would minimize the potential for regressions, which from my personal viewpoint seems like a reasonable thing to do especially now that I have little time to work on QEMU. If there's interest in doing the tsan work first, then I'd be happy to send to the list as soon as this weekend the changes that I have so far [1]. Thanks, Emilio [1] WIP branch: https://github.com/cota/qemu/commits/tsan
Re: [PATCH v2 17/19] tcg: save vaddr temp for plugin usage
On Thu, Feb 13, 2020 at 22:51:07 +, Alex Bennée wrote: > From: Richard Henderson > > While do_gen_mem_cb does copy (via extu_tl_i64) vaddr into a new temp > this won't help if the vaddr temp gets clobbered by the actual > load/store op. To avoid this clobbering we explicitly copy vaddr > before the op to ensure it is live my the time we do the > instrumentation. s/my the time/by the time/ Reviewed-by: Emilio G. Cota E.
Re: [PATCH] testing: don't nest build for fp-test
On Tue, Jan 07, 2020 at 18:00:03 +, Alex Bennée wrote: > Re-calling the main make is counter-productive and really messes up > with parallel builds. Just ensure we have built the pre-requisites > before we build the fp-test bits. If the user builds manually just > complain if the parent build hasn't got the bits we need. > > Signed-off-by: Alex Bennée Reviewed-by: Emilio G. Cota E.
[PATCH] plugins/core: add missing break in cb_to_tcg_flags
Reported-by: Robert Henry
Signed-off-by: Emilio G. Cota
---
plugins/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/plugins/core.c b/plugins/core.c
index 9e1b9e7a91..ed863011ba 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -286,6 +286,7 @@ static inline uint32_t cb_to_tcg_flags(enum
qemu_plugin_cb_flags flags)
switch (flags) {
case QEMU_PLUGIN_CB_RW_REGS:
ret = 0;
+break;
case QEMU_PLUGIN_CB_R_REGS:
ret = TCG_CALL_NO_WG;
break;
--
2.20.1
Re: [Qemu-devel] [PATCH v5 00/15] demacro softmmu (plus tests/coverage)
On Fri, May 10, 2019 at 11:36:33 +0100, Alex Bennée wrote: > Ping Emilio/Mark > > Would you be able to re-run your tests to check there are no other > regressions? I can then get the PR prepared for merging ;-) I'll try to run some tests next week, but I am not sure I'll have time to do so. If I were you I'd go ahead with the PR -- it's best to have these type of changes merged early in the development cycle. Thanks, Emilio
[Qemu-devel] [PATCH v2 for-4.0] hardfloat: fix float32/64 fused multiply-add
From: Kito Cheng
Before falling back to softfloat FMA, we do not restore the original
values of inputs A and C. Fix it.
This bug was caught by running gcc's testsuite on RISC-V qemu.
Note that this change gives a small perf increase for fp-bench:
Host: Intel(R) Core(TM) i7-4790K CPU @ 4.00GHz
Command: perf stat -r 3 taskset -c 0 ./fp-bench -o mulAdd -p $prec
- $prec = single:
- before:
101.71 MFlops
102.18 MFlops
100.96 MFlops
- after:
103.63 MFlops
103.05 MFlops
102.96 MFlops
- $prec = double:
- before:
173.10 MFlops
173.93 MFlops
172.11 MFlops
- after:
178.49 MFlops
178.88 MFlops
178.66 MFlops
Signed-off-by: Kito Cheng
Signed-off-by: Emilio G. Cota
---
fpu/softfloat.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 4610738ab1..2ba36ec370 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -1596,6 +1596,9 @@ float32_muladd(float32 xa, float32 xb, float32 xc, int
flags, float_status *s)
}
ur.h = up.h + uc.h;
} else {
+union_float32 ua_orig = ua;
+union_float32 uc_orig = uc;
+
if (flags & float_muladd_negate_product) {
ua.h = -ua.h;
}
@@ -1608,6 +1611,8 @@ float32_muladd(float32 xa, float32 xb, float32 xc, int
flags, float_status *s)
if (unlikely(f32_is_inf(ur))) {
s->float_exception_flags |= float_flag_overflow;
} else if (unlikely(fabsf(ur.h) <= FLT_MIN)) {
+ua = ua_orig;
+uc = uc_orig;
goto soft;
}
}
@@ -1662,6 +1667,9 @@ float64_muladd(float64 xa, float64 xb, float64 xc, int
flags, float_status *s)
}
ur.h = up.h + uc.h;
} else {
+union_float64 ua_orig = ua;
+union_float64 uc_orig = uc;
+
if (flags & float_muladd_negate_product) {
ua.h = -ua.h;
}
@@ -1674,6 +1682,8 @@ float64_muladd(float64 xa, float64 xb, float64 xc, int
flags, float_status *s)
if (unlikely(f64_is_inf(ur))) {
s->float_exception_flags |= float_flag_overflow;
} else if (unlikely(fabs(ur.h) <= FLT_MIN)) {
+ua = ua_orig;
+uc = uc_orig;
goto soft;
}
}
--
2.17.1
Re: [Qemu-devel] [PATCH] hardfloat: fix float32/64 fused multiply-add
On Sat, Mar 23, 2019 at 01:39:26 +0800, Kito Cheng wrote: > hardfloat fused multiply-add might fallback to softfloat mode in some > situation, but it might already changed the value of input operands, > so we must restore those value before fallback. > > This bug is catched by running gcc testsuite on RISC-V qemu. > > Signed-off-by: Kito Cheng Good catch! I'll send a v2 shortly with some small changes. Thanks, Emilio
Re: [Qemu-devel] 'make check' error
On Sat, Mar 09, 2019 at 13:53:32 +0800, Li Qiang wrote: > Hi all, > > Today I ‘git clone’ && configure && make && make check > > And get following error, > > fp-test.c:50:10: fatal error: fail.h: No such file or directory > #include "fail.h" > ^~~~ > > I look at the commit: > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3ac1f81329f4dfdc10a51e180f9cf28dbcb02a3c;hp=b44b5abeae4a3b54ccbd7137f59c0a8923cecec9 > > Seems it’s old commit, I think I got ‘make check’ work after this commit. > So I don’t know anywhere wrong. > > Any hints? fail.h is part of berkeley-testfloat-3 -- I suspect the berkeley-testfloat-3 git submodule wasn't checked out. Make sure both berkeley-softfloat-3 and berkeley-testfloat-3 are checked out at $src/tests/fp. If not, you can get them with "git submodule init && git submodule update". Hope that helps, Emilio
Re: [Qemu-devel] x86 segment limits enforcement with TCG
On Thu, Feb 28, 2019 at 10:05:02 -0800, Richard Henderson wrote: > On 2/28/19 9:18 AM, Stephen Checkoway wrote: > > I wonder if it would make sense to maintain a small cache of TLBs. The > > majority of cases are likely to involving setting segment registers to one > > of a handful of segments (e.g., setting es to ds or ss). So it might be nice > > to avoid the flushes entirely. > Hmm. > > The straight-forward approach to this would change the mapping between segment > and mmu index, which would need to force a new translation (since mmu indexes > are built into the generated code as constants). It would be easy for this > scheme to generate too many translations and slow down the system as a whole. > > However, since the change to dynamic tlbs, the actual tlb is now a pointer. > So > it might not be out of the question to simply swap TLB contents around when > changing segment registers. All you would need is N+1 tlbs to support the > (easy?) case of es swapping. > > With some additional work in cputlb, it might even be possible to have > different mmu indexes share the same backing tlb. This would be tricky to > manage during a tlb resize, but perhaps not impossible. > > Emilio, do you have any thoughts here? I like the approach you propose. If I understood correctly, we could: - Let target code manipulate the pointer in TLB[mmu_idx]. This way the target can keep for each idx a set of tables, indexed by segment register value. The target would detect changes to segment registers and update the pointer in TLB[mmu_idx]. - Add a hook from cputlb to target-specific code, so that the latter is notified of flushes and can therefore flush the set of tables. Note that resizes only occur during flushes, which makes things simpler. Seems like a reasonable amount of work and won't affect much the common path (i.e. no segmentation) -- at worst it will add the overhead of using helpers for a small number of instructions. Thanks, Emilio
Re: [Qemu-devel] [PATCH v7 00/73] per-CPU locks
On Wed, Mar 06, 2019 at 11:40:57 -0800, Richard Henderson wrote: > Peter reported to me a hang on the x86_64 > bios-tables-test with an arm32 host. I have been able to replicate this. (snip) I reproduced this on a power7 machine (gcc110). However, after a few git checkout->build cycles I was not able to reproduce this again (!?). I currently have very little time to spend on this. Given how close we are to the soft freeze, I suggest we defer the merge of this series to post-v4.0. Thanks, Emilio
Re: [Qemu-devel] [PULL 00/73] tcg: per-cpu locks
On Tue, Mar 05, 2019 at 16:01:45 +, Peter Maydell wrote:
> On Tue, 5 Mar 2019 at 15:01, Richard Henderson
> wrote:
> >
> > This is Emilio's v7, unchanged, so I'm not re-posting the 73 patches.
> >
> > I also didn't want to add Signed-off-by to 73 patches, so I verified
> > that there was nothing on Emilio's branch that shouldn't be there and
> > then used git-merge -S --signoff.
>
> Hi; I'm afraid this doesn't build on OSX:
>
> /Users/pm215/src/qemu-for-merges/target/i386/hvf/x86hvf.c:361:14:
> error: unused variable 'interrupt_request' [-Werror,-Wunused-variable]
> uint32_t interrupt_request;
Fixed on "i386: convert to cpu_halted".
> /Users/pm215/src/qemu-for-merges/target/i386/hvf/x86hvf.c:465:23:
> error: incompatible pointer types passing 'X86CPU *' (aka 'struct
> X86CPU *') to parameter of type 'CPUState *' (aka 'struct CPUState *')
> [-Werror,-Wincompatible-pointer-types]
> return cpu_halted(cpu);
> ^~~
> /Users/pm215/src/qemu-for-merges/include/qom/cpu.h:510:45: note:
> passing argument to parameter 'cpu' here
> static inline uint32_t cpu_halted(CPUState *cpu)
> ^
Fixed on "i386/hvf: convert to cpu_request_interrupt".
I've pushed a new branch with these two patches fixed up:
https://github.com/cota/qemu/tree/cpu-lock-v7b
$ git diff cpu-lock-v7..cpu-lock-v7b
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index 91feafeedc..c13d144fb3 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -358,7 +358,6 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
uint8_t vector;
uint64_t intr_type;
-uint32_t interrupt_request;
bool have_event = true;
if (env->interrupt_injected != -1) {
vector = env->interrupt_injected;
@@ -462,5 +461,5 @@ int hvf_process_events(CPUState *cpu_state)
apic_handle_tpr_access_report(cpu->apic_state, env->eip,
env->tpr_access_type);
}
-return cpu_halted(cpu);
+return cpu_halted(cpu_state);
}
Thanks,
Emilio
[Qemu-devel] [PATCH v7 44/73] ppc: convert to cpu_interrupt_request
Cc: David Gibson
Cc: qemu-...@nongnu.org
Reviewed-by: Richard Henderson
Acked-by: David Gibson
Signed-off-by: Emilio G. Cota
---
hw/ppc/ppc.c| 2 +-
target/ppc/kvm.c| 4 ++--
target/ppc/translate_init.inc.c | 14 +++---
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 45e212390a..100badefe4 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -89,7 +89,7 @@ void ppc_set_irq(PowerPCCPU *cpu, int n_IRQ, int level)
LOG_IRQ("%s: %p n_IRQ %d level %d => pending %08" PRIx32
"req %08x\n", __func__, env, n_IRQ, level,
-env->pending_interrupts, CPU(cpu)->interrupt_request);
+env->pending_interrupts, cpu_interrupt_request(CPU(cpu)));
if (locked) {
qemu_mutex_unlock_iothread();
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index faa757c417..16f09c504d 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1339,7 +1339,7 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
* interrupt, reset, etc) in PPC-specific env->irq_input_state. */
if (!cap_interrupt_level &&
run->ready_for_interrupt_injection &&
-(cs->interrupt_request & CPU_INTERRUPT_HARD) &&
+(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD) &&
(env->irq_input_state & (1<env;
-if (!(cs->interrupt_request & CPU_INTERRUPT_HARD) && (msr_ee)) {
+if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD) && (msr_ee)) {
cpu_halted_set(cs, 1);
cs->exception_index = EXCP_HLT;
}
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index b2cad43e2a..f387404913 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8469,7 +8469,7 @@ static bool cpu_has_work_POWER7(CPUState *cs)
CPUPPCState *env = >env;
if (cpu_halted(cs)) {
-if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
+if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
return false;
}
if ((env->pending_interrupts & (1u << PPC_INTERRUPT_EXT)) &&
@@ -8493,7 +8493,7 @@ static bool cpu_has_work_POWER7(CPUState *cs)
}
return false;
} else {
-return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
+return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
}
}
@@ -8623,7 +8623,7 @@ static bool cpu_has_work_POWER8(CPUState *cs)
CPUPPCState *env = >env;
if (cpu_halted(cs)) {
-if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
+if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
return false;
}
if ((env->pending_interrupts & (1u << PPC_INTERRUPT_EXT)) &&
@@ -8655,7 +8655,7 @@ static bool cpu_has_work_POWER8(CPUState *cs)
}
return false;
} else {
-return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
+return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
}
}
@@ -8817,7 +8817,7 @@ static bool cpu_has_work_POWER9(CPUState *cs)
if (cpu_halted(cs)) {
uint64_t psscr = env->spr[SPR_PSSCR];
-if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
+if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
return false;
}
@@ -8863,7 +8863,7 @@ static bool cpu_has_work_POWER9(CPUState *cs)
}
return false;
} else {
-return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
+return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
}
}
@@ -10332,7 +10332,7 @@ static bool ppc_cpu_has_work(CPUState *cs)
PowerPCCPU *cpu = POWERPC_CPU(cs);
CPUPPCState *env = >env;
-return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
+return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
}
/* CPUClass::reset() */
--
2.17.1
[Qemu-devel] [PATCH v7 65/73] s390x: convert to cpu_has_work_with_iothread_lock
Soon we will call cpu_has_work without the BQL.
Cc: Cornelia Huck
Cc: David Hildenbrand
Cc: qemu-s3...@nongnu.org
Reviewed-by: Richard Henderson
Reviewed-by: Cornelia Huck
Signed-off-by: Emilio G. Cota
---
target/s390x/cpu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 5169adb6a5..27b76d7d8c 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -56,6 +56,8 @@ static bool s390_cpu_has_work(CPUState *cs)
{
S390CPU *cpu = S390_CPU(cs);
+g_assert(qemu_mutex_iothread_locked());
+
/* STOPPED cpus can never wake up */
if (s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD &&
s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING) {
@@ -470,7 +472,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
scc->initial_cpu_reset = s390_cpu_initial_reset;
cc->reset = s390_cpu_full_reset;
cc->class_by_name = s390_cpu_class_by_name,
-cc->has_work = s390_cpu_has_work;
+cc->has_work_with_iothread_lock = s390_cpu_has_work;
#ifdef CONFIG_TCG
cc->do_interrupt = s390_cpu_do_interrupt;
#endif
--
2.17.1
[Qemu-devel] [PATCH v7 57/73] unicore32: convert to cpu_interrupt_request
Cc: Guan Xuetao
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/unicore32/cpu.c | 2 +-
target/unicore32/softmmu.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c
index 2b49d1ca40..65c5334551 100644
--- a/target/unicore32/cpu.c
+++ b/target/unicore32/cpu.c
@@ -29,7 +29,7 @@ static void uc32_cpu_set_pc(CPUState *cs, vaddr value)
static bool uc32_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request &
+return cpu_interrupt_request(cs) &
(CPU_INTERRUPT_HARD | CPU_INTERRUPT_EXITTB);
}
diff --git a/target/unicore32/softmmu.c b/target/unicore32/softmmu.c
index 00c7e0d028..f58e2361e0 100644
--- a/target/unicore32/softmmu.c
+++ b/target/unicore32/softmmu.c
@@ -119,7 +119,7 @@ void uc32_cpu_do_interrupt(CPUState *cs)
/* The PC already points to the proper instruction. */
env->regs[30] = env->regs[31];
env->regs[31] = addr;
-cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+cpu_interrupt_request_or(cs, CPU_INTERRUPT_EXITTB);
}
static int get_phys_addr_ucv2(CPUUniCore32State *env, uint32_t address,
--
2.17.1
[Qemu-devel] [PATCH v7 46/73] cris: convert to cpu_interrupt_request
Cc: "Edgar E. Iglesias"
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/cris/cpu.c| 2 +-
target/cris/helper.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index a23aba2688..3cdba581e6 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -37,7 +37,7 @@ static void cris_cpu_set_pc(CPUState *cs, vaddr value)
static bool cris_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI);
+return cpu_interrupt_request(cs) & (CPU_INTERRUPT_HARD |
CPU_INTERRUPT_NMI);
}
/* CPUClass::reset() */
diff --git a/target/cris/helper.c b/target/cris/helper.c
index b2dbb2075c..5c453d5221 100644
--- a/target/cris/helper.c
+++ b/target/cris/helper.c
@@ -116,7 +116,7 @@ int cris_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
int size, int rw,
if (r > 0) {
qemu_log_mask(CPU_LOG_MMU,
"%s returns %d irqreq=%x addr=%" VADDR_PRIx " phy=%x vec=%x"
-" pc=%x\n", __func__, r, cs->interrupt_request, address,
+" pc=%x\n", __func__, r, cpu_interrupt_request(cs), address,
res.phy, res.bf_vec, env->pc);
}
return r;
@@ -130,7 +130,7 @@ void crisv10_cpu_do_interrupt(CPUState *cs)
D_LOG("exception index=%d interrupt_req=%d\n",
cs->exception_index,
- cs->interrupt_request);
+ cpu_interrupt_request(cs));
if (env->dslot) {
/* CRISv10 never takes interrupts while in a delay-slot. */
@@ -192,7 +192,7 @@ void cris_cpu_do_interrupt(CPUState *cs)
D_LOG("exception index=%d interrupt_req=%d\n",
cs->exception_index,
- cs->interrupt_request);
+ cpu_interrupt_request(cs));
switch (cs->exception_index) {
case EXCP_BREAK:
--
2.17.1
[Qemu-devel] [PATCH v7 54/73] moxie: convert to cpu_interrupt_request
Cc: Anthony Green
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/moxie/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/moxie/cpu.c b/target/moxie/cpu.c
index 46434e65ba..72206a03ee 100644
--- a/target/moxie/cpu.c
+++ b/target/moxie/cpu.c
@@ -33,7 +33,7 @@ static void moxie_cpu_set_pc(CPUState *cs, vaddr value)
static bool moxie_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & CPU_INTERRUPT_HARD;
+return cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
}
static void moxie_cpu_reset(CPUState *s)
--
2.17.1
[Qemu-devel] [PATCH v7 49/73] m68k: convert to cpu_interrupt_request
Cc: Laurent Vivier
Reviewed-by: Richard Henderson
Reviewed-by: Laurent Vivier
Signed-off-by: Emilio G. Cota
---
target/m68k/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 582e3a73b3..99a7eb4340 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -34,7 +34,7 @@ static void m68k_cpu_set_pc(CPUState *cs, vaddr value)
static bool m68k_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & CPU_INTERRUPT_HARD;
+return cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
}
static void m68k_set_feature(CPUM68KState *env, int feature)
--
2.17.1
[Qemu-devel] [PATCH v7 52/73] s390x: convert to cpu_interrupt_request
Cc: Cornelia Huck
Cc: Christian Borntraeger
Cc: David Hildenbrand
Cc: qemu-s3...@nongnu.org
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Reviewed-by: Cornelia Huck
Signed-off-by: Emilio G. Cota
---
hw/intc/s390_flic.c | 2 +-
target/s390x/cpu.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index bfb5cf1d07..d944824e67 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -189,7 +189,7 @@ static void qemu_s390_flic_notify(uint32_t type)
CPU_FOREACH(cs) {
S390CPU *cpu = S390_CPU(cs);
-cs->interrupt_request |= CPU_INTERRUPT_HARD;
+cpu_interrupt_request_or(cs, CPU_INTERRUPT_HARD);
/* ignore CPUs that are not sleeping */
if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 4e17df5c39..5169adb6a5 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -62,7 +62,7 @@ static bool s390_cpu_has_work(CPUState *cs)
return false;
}
-if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
+if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
return false;
}
--
2.17.1
[Qemu-devel] [PATCH v7 59/73] accel/tcg: convert to cpu_interrupt_request
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
accel/tcg/cpu-exec.c | 15 ---
accel/tcg/tcg-all.c | 12 +---
accel/tcg/translate-all.c | 2 +-
3 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 8af0b5de07..ddd38fe9ac 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -431,7 +431,7 @@ static inline bool cpu_handle_halt_locked(CPUState *cpu)
if (cpu_halted(cpu)) {
#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
-if ((cpu->interrupt_request & CPU_INTERRUPT_POLL)
+if ((cpu_interrupt_request(cpu) & CPU_INTERRUPT_POLL)
&& replay_interrupt()) {
X86CPU *x86_cpu = X86_CPU(cpu);
@@ -544,16 +544,17 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
*/
atomic_mb_set(>icount_decr.u16.high, 0);
-if (unlikely(atomic_read(>interrupt_request))) {
+if (unlikely(cpu_interrupt_request(cpu))) {
int interrupt_request;
+
qemu_mutex_lock_iothread();
-interrupt_request = cpu->interrupt_request;
+interrupt_request = cpu_interrupt_request(cpu);
if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
/* Mask out external interrupts for this step. */
interrupt_request &= ~CPU_INTERRUPT_SSTEP_MASK;
}
if (interrupt_request & CPU_INTERRUPT_DEBUG) {
-cpu->interrupt_request &= ~CPU_INTERRUPT_DEBUG;
+cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG);
cpu->exception_index = EXCP_DEBUG;
qemu_mutex_unlock_iothread();
return true;
@@ -562,7 +563,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
/* Do nothing */
} else if (interrupt_request & CPU_INTERRUPT_HALT) {
replay_interrupt();
-cpu->interrupt_request &= ~CPU_INTERRUPT_HALT;
+cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
cpu_halted_set(cpu, 1);
cpu->exception_index = EXCP_HLT;
qemu_mutex_unlock_iothread();
@@ -599,10 +600,10 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
}
/* The target hook may have updated the 'cpu->interrupt_request';
* reload the 'interrupt_request' value */
-interrupt_request = cpu->interrupt_request;
+interrupt_request = cpu_interrupt_request(cpu);
}
if (interrupt_request & CPU_INTERRUPT_EXITTB) {
-cpu->interrupt_request &= ~CPU_INTERRUPT_EXITTB;
+cpu_reset_interrupt(cpu, CPU_INTERRUPT_EXITTB);
/* ensure that no TB jump will be modified as
the program flow was changed */
*last_tb = NULL;
diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 3d25bdcc17..4e2fe70350 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -39,10 +39,16 @@ unsigned long tcg_tb_size;
static void tcg_handle_interrupt(CPUState *cpu, int mask)
{
int old_mask;
-g_assert(qemu_mutex_iothread_locked());
-old_mask = cpu->interrupt_request;
-cpu->interrupt_request |= mask;
+if (!cpu_mutex_locked(cpu)) {
+cpu_mutex_lock(cpu);
+old_mask = cpu_interrupt_request(cpu);
+cpu_interrupt_request_or(cpu, mask);
+cpu_mutex_unlock(cpu);
+} else {
+old_mask = cpu_interrupt_request(cpu);
+cpu_interrupt_request_or(cpu, mask);
+}
/*
* If called from iothread context, wake the target cpu in
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 8f593b926f..62585fd95c 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2343,7 +2343,7 @@ void dump_opcount_info(FILE *f, fprintf_function
cpu_fprintf)
void cpu_interrupt(CPUState *cpu, int mask)
{
g_assert(qemu_mutex_iothread_locked());
-cpu->interrupt_request |= mask;
+cpu_interrupt_request_or(cpu, mask);
atomic_set(>icount_decr.u16.high, -1);
}
--
2.17.1
[Qemu-devel] [PATCH v7 60/73] cpu: convert to interrupt_request
This finishes the conversion to interrupt_request.
Reviewed-by: Alex Bennée
Reviewed-by: Richard Henderson
Signed-off-by: Emilio G. Cota
---
qom/cpu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qom/cpu.c b/qom/cpu.c
index 00add81a7f..f2695be9b2 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -275,7 +275,7 @@ static void cpu_common_reset(CPUState *cpu)
log_cpu_state(cpu, cc->reset_dump_flags);
}
-cpu->interrupt_request = 0;
+cpu_interrupt_request_set(cpu, 0);
cpu_halted_set(cpu, 0);
cpu->mem_io_pc = 0;
cpu->mem_io_vaddr = 0;
@@ -412,7 +412,7 @@ static vaddr cpu_adjust_watchpoint_address(CPUState *cpu,
vaddr addr, int len)
static void generic_handle_interrupt(CPUState *cpu, int mask)
{
-cpu->interrupt_request |= mask;
+cpu_interrupt_request_or(cpu, mask);
if (!qemu_cpu_is_self(cpu)) {
qemu_cpu_kick(cpu);
--
2.17.1
[Qemu-devel] [PATCH v7 71/73] cpus-common: release BQL earlier in run_on_cpu
After completing the conversion to per-CPU locks, there is no need
to release the BQL after having called cpu_kick.
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
cpus-common.c | 20 +---
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/cpus-common.c b/cpus-common.c
index 62e282bff1..1241024b2c 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -145,6 +145,11 @@ void run_on_cpu(CPUState *cpu, run_on_cpu_func func,
run_on_cpu_data data)
return;
}
+/* We are going to sleep on the CPU lock, so release the BQL */
+if (has_bql) {
+qemu_mutex_unlock_iothread();
+}
+
wi.func = func;
wi.data = data;
wi.done = false;
@@ -153,21 +158,6 @@ void run_on_cpu(CPUState *cpu, run_on_cpu_func func,
run_on_cpu_data data)
cpu_mutex_lock(cpu);
queue_work_on_cpu_locked(cpu, );
-
-/*
- * We are going to sleep on the CPU lock, so release the BQL.
- *
- * During the transition to per-CPU locks, we release the BQL _after_
- * having kicked the destination CPU (from queue_work_on_cpu_locked above).
- * This makes sure that the enqueued work will be seen by the CPU
- * after being woken up from the kick, since the CPU sleeps on the BQL.
- * Once we complete the transition to per-CPU locks, we will release
- * the BQL earlier in this function.
- */
-if (has_bql) {
-qemu_mutex_unlock_iothread();
-}
-
while (!atomic_mb_read()) {
CPUState *self_cpu = current_cpu;
--
2.17.1
[Qemu-devel] [PATCH v7 45/73] sh4: convert to cpu_interrupt_request
Cc: Aurelien Jarno
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/sh4/cpu.c| 2 +-
target/sh4/helper.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index b9f393b7c7..58ea212f53 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -45,7 +45,7 @@ static void superh_cpu_synchronize_from_tb(CPUState *cs,
TranslationBlock *tb)
static bool superh_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & CPU_INTERRUPT_HARD;
+return cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
}
/* CPUClass::reset() */
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index 2ff0cf4060..8463da5bc8 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -83,7 +83,7 @@ void superh_cpu_do_interrupt(CPUState *cs)
{
SuperHCPU *cpu = SUPERH_CPU(cs);
CPUSH4State *env = >env;
-int do_irq = cs->interrupt_request & CPU_INTERRUPT_HARD;
+int do_irq = cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
int do_exp, irq_vector = cs->exception_index;
/* prioritize exceptions over interrupts */
--
2.17.1
[Qemu-devel] [PATCH v7 67/73] sparc: convert to cpu_has_work_with_iothread_lock
Soon we will call cpu_has_work without the BQL. Cc: Mark Cave-Ayland Cc: Artyom Tarasenko Reviewed-by: Richard Henderson Acked-by: Mark Cave-Ayland Signed-off-by: Emilio G. Cota --- target/sparc/cpu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c index 933fd8e954..b15f6ef180 100644 --- a/target/sparc/cpu.c +++ b/target/sparc/cpu.c @@ -708,6 +708,8 @@ static bool sparc_cpu_has_work(CPUState *cs) SPARCCPU *cpu = SPARC_CPU(cs); CPUSPARCState *env = >env; +g_assert(qemu_mutex_iothread_locked()); + return (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD) && cpu_interrupts_enabled(env); } @@ -869,7 +871,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data) cc->class_by_name = sparc_cpu_class_by_name; cc->parse_features = sparc_cpu_parse_features; -cc->has_work = sparc_cpu_has_work; +cc->has_work_with_iothread_lock = sparc_cpu_has_work; cc->do_interrupt = sparc_cpu_do_interrupt; cc->cpu_exec_interrupt = sparc_cpu_exec_interrupt; cc->dump_state = sparc_cpu_dump_state; -- 2.17.1
[Qemu-devel] [PATCH v7 48/73] lm32: convert to cpu_interrupt_request
Cc: Michael Walle
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/lm32/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
index b7499cb627..1508bb6199 100644
--- a/target/lm32/cpu.c
+++ b/target/lm32/cpu.c
@@ -101,7 +101,7 @@ static void lm32_cpu_init_cfg_reg(LM32CPU *cpu)
static bool lm32_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & CPU_INTERRUPT_HARD;
+return cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
}
/* CPUClass::reset() */
--
2.17.1
[Qemu-devel] [PATCH v7 40/73] i386/kvm: convert to cpu_interrupt_request
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/i386/kvm.c | 58 ---
1 file changed, 35 insertions(+), 23 deletions(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index f2e187e40f..44a9e3d243 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2888,11 +2888,14 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
events.smi.smm = !!(env->hflags & HF_SMM_MASK);
events.smi.smm_inside_nmi = !!(env->hflags2 & HF2_SMM_INSIDE_NMI_MASK);
if (kvm_irqchip_in_kernel()) {
+uint32_t interrupt_request;
+
/* As soon as these are moved to the kernel, remove them
* from cs->interrupt_request.
*/
-events.smi.pending = cs->interrupt_request & CPU_INTERRUPT_SMI;
-events.smi.latched_init = cs->interrupt_request &
CPU_INTERRUPT_INIT;
+interrupt_request = cpu_interrupt_request(cs);
+events.smi.pending = interrupt_request & CPU_INTERRUPT_SMI;
+events.smi.latched_init = interrupt_request & CPU_INTERRUPT_INIT;
cpu_reset_interrupt(cs, CPU_INTERRUPT_INIT | CPU_INTERRUPT_SMI);
} else {
/* Keep these in cs->interrupt_request. */
@@ -3183,14 +3186,14 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run
*run)
{
X86CPU *x86_cpu = X86_CPU(cpu);
CPUX86State *env = _cpu->env;
+uint32_t interrupt_request;
int ret;
+interrupt_request = cpu_interrupt_request(cpu);
/* Inject NMI */
-if (cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
-if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
-qemu_mutex_lock_iothread();
+if (interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
+if (interrupt_request & CPU_INTERRUPT_NMI) {
cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
-qemu_mutex_unlock_iothread();
DPRINTF("injected NMI\n");
ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
if (ret < 0) {
@@ -3198,10 +3201,8 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
strerror(-ret));
}
}
-if (cpu->interrupt_request & CPU_INTERRUPT_SMI) {
-qemu_mutex_lock_iothread();
+if (interrupt_request & CPU_INTERRUPT_SMI) {
cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
-qemu_mutex_unlock_iothread();
DPRINTF("injected SMI\n");
ret = kvm_vcpu_ioctl(cpu, KVM_SMI);
if (ret < 0) {
@@ -3215,16 +3216,22 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run
*run)
qemu_mutex_lock_iothread();
}
+/*
+ * We might have cleared some bits in cpu->interrupt_request since reading
+ * it; read it again.
+ */
+interrupt_request = cpu_interrupt_request(cpu);
+
/* Force the VCPU out of its inner loop to process any INIT requests
* or (for userspace APIC, but it is cheap to combine the checks here)
* pending TPR access reports.
*/
-if (cpu->interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
-if ((cpu->interrupt_request & CPU_INTERRUPT_INIT) &&
+if (interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
+if ((interrupt_request & CPU_INTERRUPT_INIT) &&
!(env->hflags & HF_SMM_MASK)) {
cpu->exit_request = 1;
}
-if (cpu->interrupt_request & CPU_INTERRUPT_TPR) {
+if (interrupt_request & CPU_INTERRUPT_TPR) {
cpu->exit_request = 1;
}
}
@@ -3232,7 +3239,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
if (!kvm_pic_in_kernel()) {
/* Try to inject an interrupt if the guest can accept it */
if (run->ready_for_interrupt_injection &&
-(cpu->interrupt_request & CPU_INTERRUPT_HARD) &&
+(interrupt_request & CPU_INTERRUPT_HARD) &&
(env->eflags & IF_MASK)) {
int irq;
@@ -3256,7 +3263,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
* interrupt, request an interrupt window exit. This will
* cause a return to userspace as soon as the guest is ready to
* receive interrupts. */
-if ((cpu->interrupt_request & CPU_INTERRUPT_HARD)) {
+if ((cpu_interrupt_request(cpu) & CPU_INTERRUPT_HARD)) {
run->request_interrupt_window = 1;
} else {
run->request_interrupt_window = 0;
@@ -3302,8 +3309,9 @@ int kvm_arch_process_async_events(CPUState *cs)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = >env;
+uint32_t interrupt_request;
-if (c
[Qemu-devel] [PATCH v7 51/73] nios: convert to cpu_interrupt_request
Cc: Chris Wulff
Cc: Marek Vasut
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/nios2/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index fbfaa2ce26..49a75414d3 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -36,7 +36,7 @@ static void nios2_cpu_set_pc(CPUState *cs, vaddr value)
static bool nios2_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI);
+return cpu_interrupt_request(cs) & (CPU_INTERRUPT_HARD |
CPU_INTERRUPT_NMI);
}
/* CPUClass::reset() */
--
2.17.1
[Qemu-devel] [PATCH v7 63/73] ppc: convert to cpu_has_work_with_iothread_lock
Soon we will call cpu_has_work without the BQL.
Cc: David Gibson
Cc: qemu-...@nongnu.org
Reviewed-by: Richard Henderson
Acked-by: David Gibson
Signed-off-by: Emilio G. Cota
---
target/ppc/translate_init.inc.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index f387404913..8c2ac99707 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8468,6 +8468,8 @@ static bool cpu_has_work_POWER7(CPUState *cs)
PowerPCCPU *cpu = POWERPC_CPU(cs);
CPUPPCState *env = >env;
+g_assert(qemu_mutex_iothread_locked());
+
if (cpu_halted(cs)) {
if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
return false;
@@ -8511,7 +8513,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
pcc->pcr_supported = PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
pcc->init_proc = init_proc_POWER7;
pcc->check_pow = check_pow_nocheck;
-cc->has_work = cpu_has_work_POWER7;
+cc->has_work_with_iothread_lock = cpu_has_work_POWER7;
pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
@@ -8622,6 +8624,8 @@ static bool cpu_has_work_POWER8(CPUState *cs)
PowerPCCPU *cpu = POWERPC_CPU(cs);
CPUPPCState *env = >env;
+g_assert(qemu_mutex_iothread_locked());
+
if (cpu_halted(cs)) {
if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
return false;
@@ -8673,7 +8677,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
pcc->pcr_supported = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
pcc->init_proc = init_proc_POWER8;
pcc->check_pow = check_pow_nocheck;
-cc->has_work = cpu_has_work_POWER8;
+cc->has_work_with_iothread_lock = cpu_has_work_POWER8;
pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
@@ -8814,6 +8818,8 @@ static bool cpu_has_work_POWER9(CPUState *cs)
PowerPCCPU *cpu = POWERPC_CPU(cs);
CPUPPCState *env = >env;
+g_assert(qemu_mutex_iothread_locked());
+
if (cpu_halted(cs)) {
uint64_t psscr = env->spr[SPR_PSSCR];
@@ -8882,7 +,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
PCR_COMPAT_2_05;
pcc->init_proc = init_proc_POWER9;
pcc->check_pow = check_pow_nocheck;
-cc->has_work = cpu_has_work_POWER9;
+cc->has_work_with_iothread_lock = cpu_has_work_POWER9;
pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
@@ -10332,6 +10338,8 @@ static bool ppc_cpu_has_work(CPUState *cs)
PowerPCCPU *cpu = POWERPC_CPU(cs);
CPUPPCState *env = >env;
+g_assert(qemu_mutex_iothread_locked());
+
return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
}
@@ -10531,7 +10539,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void
*data)
cc->class_by_name = ppc_cpu_class_by_name;
pcc->parent_parse_features = cc->parse_features;
cc->parse_features = ppc_cpu_parse_featurestr;
-cc->has_work = ppc_cpu_has_work;
+cc->has_work_with_iothread_lock = ppc_cpu_has_work;
cc->do_interrupt = ppc_cpu_do_interrupt;
cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
cc->dump_state = ppc_cpu_dump_state;
--
2.17.1
[Qemu-devel] [PATCH v7 35/73] i386: use cpu_reset_interrupt
From: Paolo Bonzini
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Paolo Bonzini
Signed-off-by: Emilio G. Cota
---
target/i386/hax-all.c| 4 ++--
target/i386/hvf/x86hvf.c | 8
target/i386/kvm.c| 14 +++---
target/i386/seg_helper.c | 13 ++---
target/i386/svm_helper.c | 2 +-
target/i386/whpx-all.c | 10 +-
6 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index 22951017cf..518c6ff103 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -424,7 +424,7 @@ static int hax_vcpu_interrupt(CPUArchState *env)
irq = cpu_get_pic_interrupt(env);
if (irq >= 0) {
hax_inject_interrupt(env, irq);
-cpu->interrupt_request &= ~CPU_INTERRUPT_HARD;
+cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
}
}
@@ -474,7 +474,7 @@ static int hax_vcpu_hax_exec(CPUArchState *env)
cpu_halted_set(cpu, 0);
if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
-cpu->interrupt_request &= ~CPU_INTERRUPT_POLL;
+cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
apic_poll_irq(x86_cpu->apic_state);
}
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index 163bbed23f..e8b13ed534 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -402,7 +402,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
if (cpu_state->interrupt_request & CPU_INTERRUPT_NMI) {
if (!(env->hflags2 & HF2_NMI_MASK) && !(info & VMCS_INTR_VALID)) {
-cpu_state->interrupt_request &= ~CPU_INTERRUPT_NMI;
+cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_NMI);
info = VMCS_INTR_VALID | VMCS_INTR_T_NMI | NMI_VEC;
wvmcs(cpu_state->hvf_fd, VMCS_ENTRY_INTR_INFO, info);
} else {
@@ -414,7 +414,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
(cpu_state->interrupt_request & CPU_INTERRUPT_HARD) &&
(EFLAGS(env) & IF_MASK) && !(info & VMCS_INTR_VALID)) {
int line = cpu_get_pic_interrupt(>env);
-cpu_state->interrupt_request &= ~CPU_INTERRUPT_HARD;
+cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_HARD);
if (line >= 0) {
wvmcs(cpu_state->hvf_fd, VMCS_ENTRY_INTR_INFO, line |
VMCS_INTR_VALID | VMCS_INTR_T_HWINTR);
@@ -440,7 +440,7 @@ int hvf_process_events(CPUState *cpu_state)
}
if (cpu_state->interrupt_request & CPU_INTERRUPT_POLL) {
-cpu_state->interrupt_request &= ~CPU_INTERRUPT_POLL;
+cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_POLL);
apic_poll_irq(cpu->apic_state);
}
if (((cpu_state->interrupt_request & CPU_INTERRUPT_HARD) &&
@@ -453,7 +453,7 @@ int hvf_process_events(CPUState *cpu_state)
do_cpu_sipi(cpu);
}
if (cpu_state->interrupt_request & CPU_INTERRUPT_TPR) {
-cpu_state->interrupt_request &= ~CPU_INTERRUPT_TPR;
+cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_TPR);
hvf_cpu_synchronize_state(cpu_state);
apic_handle_tpr_access_report(cpu->apic_state, env->eip,
env->tpr_access_type);
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 81e3d65cc9..f2e187e40f 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2893,7 +2893,7 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level)
*/
events.smi.pending = cs->interrupt_request & CPU_INTERRUPT_SMI;
events.smi.latched_init = cs->interrupt_request &
CPU_INTERRUPT_INIT;
-cs->interrupt_request &= ~(CPU_INTERRUPT_INIT | CPU_INTERRUPT_SMI);
+cpu_reset_interrupt(cs, CPU_INTERRUPT_INIT | CPU_INTERRUPT_SMI);
} else {
/* Keep these in cs->interrupt_request. */
events.smi.pending = 0;
@@ -3189,7 +3189,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
if (cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
qemu_mutex_lock_iothread();
-cpu->interrupt_request &= ~CPU_INTERRUPT_NMI;
+cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
qemu_mutex_unlock_iothread();
DPRINTF("injected NMI\n");
ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
@@ -3200,7 +3200,7 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
}
if (cpu->interrupt_request & CPU_INTERRUPT_SMI) {
qemu_mutex_lock_iothread();
-cpu->interrupt_request &= ~CPU_INTERRUPT_SMI;
+cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
qemu_mutex_unlock_iothread();
DP
[Qemu-devel] [PATCH v7 62/73] cpu: introduce cpu_has_work_with_iothread_lock
It will gain some users soon.
Suggested-by: Paolo Bonzini
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
include/qom/cpu.h | 37 ++---
1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 651e1ab4d8..fc768bbe00 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -27,6 +27,7 @@
#include "qapi/qapi-types-run-state.h"
#include "qemu/bitmap.h"
#include "qemu/fprintf-fn.h"
+#include "qemu/main-loop.h"
#include "qemu/rcu_queue.h"
#include "qemu/queue.h"
#include "qemu/thread.h"
@@ -87,6 +88,8 @@ struct TranslationBlock;
* @reset_dump_flags: #CPUDumpFlags to use for reset logging.
* @has_work: Callback for checking if there is work to do. Called with the
* CPU lock held.
+ * @has_work_with_iothread_lock: Callback for checking if there is work to do.
+ * Called with both the BQL and the CPU lock held.
* @do_interrupt: Callback for interrupt handling.
* @do_unassigned_access: Callback for unassigned access handling.
* (this is deprecated: new targets should use do_transaction_failed instead)
@@ -170,6 +173,7 @@ typedef struct CPUClass {
void (*reset)(CPUState *cpu);
int reset_dump_flags;
bool (*has_work)(CPUState *cpu);
+bool (*has_work_with_iothread_lock)(CPUState *cpu);
void (*do_interrupt)(CPUState *cpu);
CPUUnassignedAccess do_unassigned_access;
void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
@@ -809,14 +813,41 @@ const char *parse_cpu_model(const char *cpu_model);
static inline bool cpu_has_work(CPUState *cpu)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
+bool has_cpu_lock = cpu_mutex_locked(cpu);
+bool (*func)(CPUState *cpu);
bool ret;
+/* some targets require us to hold the BQL when checking for work */
+if (cc->has_work_with_iothread_lock) {
+if (qemu_mutex_iothread_locked()) {
+func = cc->has_work_with_iothread_lock;
+goto call_func;
+}
+
+if (has_cpu_lock) {
+/* avoid deadlock by acquiring the locks in order */
+cpu_mutex_unlock(cpu);
+}
+qemu_mutex_lock_iothread();
+cpu_mutex_lock(cpu);
+
+ret = cc->has_work_with_iothread_lock(cpu);
+
+qemu_mutex_unlock_iothread();
+if (!has_cpu_lock) {
+cpu_mutex_unlock(cpu);
+}
+return ret;
+}
+
g_assert(cc->has_work);
-if (cpu_mutex_locked(cpu)) {
-return cc->has_work(cpu);
+func = cc->has_work;
+ call_func:
+if (has_cpu_lock) {
+return func(cpu);
}
cpu_mutex_lock(cpu);
-ret = cc->has_work(cpu);
+ret = func(cpu);
cpu_mutex_unlock(cpu);
return ret;
}
--
2.17.1
[Qemu-devel] [PATCH v7 58/73] microblaze: convert to cpu_interrupt_request
Cc: "Edgar E. Iglesias"
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/microblaze/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 5596cd5485..0cdd7fe917 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -84,7 +84,7 @@ static void mb_cpu_set_pc(CPUState *cs, vaddr value)
static bool mb_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI);
+return cpu_interrupt_request(cs) & (CPU_INTERRUPT_HARD |
CPU_INTERRUPT_NMI);
}
#ifndef CONFIG_USER_ONLY
--
2.17.1
[Qemu-devel] [PATCH v7 39/73] i386: convert to cpu_interrupt_request
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/i386/cpu.c| 2 +-
target/i386/helper.c | 4 ++--
target/i386/svm_helper.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fe23d684db..ec87f611ba 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5686,7 +5686,7 @@ int x86_cpu_pending_interrupt(CPUState *cs, int
interrupt_request)
static bool x86_cpu_has_work(CPUState *cs)
{
-return x86_cpu_pending_interrupt(cs, cs->interrupt_request) != 0;
+return x86_cpu_pending_interrupt(cs, cpu_interrupt_request(cs)) != 0;
}
static void x86_disas_set_info(CPUState *cs, disassemble_info *info)
diff --git a/target/i386/helper.c b/target/i386/helper.c
index a75278f954..9197fb4edc 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -1035,12 +1035,12 @@ void do_cpu_init(X86CPU *cpu)
CPUState *cs = CPU(cpu);
CPUX86State *env = >env;
CPUX86State *save = g_new(CPUX86State, 1);
-int sipi = cs->interrupt_request & CPU_INTERRUPT_SIPI;
+int sipi = cpu_interrupt_request(cs) & CPU_INTERRUPT_SIPI;
*save = *env;
cpu_reset(cs);
-cs->interrupt_request = sipi;
+cpu_interrupt_request_set(cs, sipi);
memcpy(>start_init_save, >start_init_save,
offsetof(CPUX86State, end_init_save) -
offsetof(CPUX86State, start_init_save));
diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
index a6d33e55d8..ebf3643ba7 100644
--- a/target/i386/svm_helper.c
+++ b/target/i386/svm_helper.c
@@ -316,7 +316,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int
next_eip_addend)
if (int_ctl & V_IRQ_MASK) {
CPUState *cs = CPU(x86_env_get_cpu(env));
-cs->interrupt_request |= CPU_INTERRUPT_VIRQ;
+cpu_interrupt_request_or(cs, CPU_INTERRUPT_VIRQ);
}
/* maybe we need to inject an event */
@@ -674,7 +674,7 @@ void do_vmexit(CPUX86State *env, uint32_t exit_code,
uint64_t exit_info_1)
env->vm_vmcb + offsetof(struct vmcb, control.int_ctl));
int_ctl &= ~(V_TPR_MASK | V_IRQ_MASK);
int_ctl |= env->v_tpr & V_TPR_MASK;
-if (cs->interrupt_request & CPU_INTERRUPT_VIRQ) {
+if (cpu_interrupt_request(cs) & CPU_INTERRUPT_VIRQ) {
int_ctl |= V_IRQ_MASK;
}
x86_stl_phys(cs,
--
2.17.1
[Qemu-devel] [PATCH v7 34/73] exec: use cpu_reset_interrupt
Reviewed-by: Richard Henderson Reviewed-by: Alex Bennée Signed-off-by: Emilio G. Cota --- exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exec.c b/exec.c index 518064530b..5ed64e274b 100644 --- a/exec.c +++ b/exec.c @@ -779,7 +779,7 @@ static int cpu_common_post_load(void *opaque, int version_id) /* 0x01 was CPU_INTERRUPT_EXIT. This line can be removed when the version_id is increased. */ -cpu->interrupt_request &= ~0x01; +cpu_reset_interrupt(cpu, 1); tlb_flush(cpu); /* loadvm has just updated the content of RAM, bypassing the -- 2.17.1
[Qemu-devel] [PATCH v7 69/73] cpu: rename all_cpu_threads_idle to qemu_tcg_rr_all_cpu_threads_idle
This function is only called from TCG rr mode, so add
a prefix to mark this as well as an assertion.
Reviewed-by: Alex Bennée
Reviewed-by: Richard Henderson
Signed-off-by: Emilio G. Cota
---
cpus.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/cpus.c b/cpus.c
index c6806184ee..61fb6e033f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -211,10 +211,12 @@ static bool cpu_thread_is_idle(CPUState *cpu)
return true;
}
-static bool all_cpu_threads_idle(void)
+static bool qemu_tcg_rr_all_cpu_threads_idle(void)
{
CPUState *cpu;
+g_assert(qemu_is_tcg_rr());
+
CPU_FOREACH(cpu) {
if (!cpu_thread_is_idle(cpu)) {
return false;
@@ -692,7 +694,7 @@ void qemu_start_warp_timer(void)
}
if (replay_mode != REPLAY_MODE_PLAY) {
-if (!all_cpu_threads_idle()) {
+if (!qemu_tcg_rr_all_cpu_threads_idle()) {
return;
}
@@ -1325,7 +1327,7 @@ static void qemu_tcg_rr_wait_io_event(void)
{
CPUState *cpu;
-while (all_cpu_threads_idle()) {
+while (qemu_tcg_rr_all_cpu_threads_idle()) {
stop_tcg_kick_timer();
qemu_cond_wait(first_cpu->halt_cond, _global_mutex);
}
@@ -1660,7 +1662,7 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
atomic_mb_set(>exit_request, 0);
}
-if (use_icount && all_cpu_threads_idle()) {
+if (use_icount && qemu_tcg_rr_all_cpu_threads_idle()) {
/*
* When all cpus are sleeping (e.g in WFI), to avoid a deadlock
* in the main_loop, wake it up in order to start the warp timer.
--
2.17.1
[Qemu-devel] [PATCH v7 70/73] cpu: protect CPU state with cpu->lock instead of the BQL
Use the per-CPU locks to protect the CPUs' state, instead of
using the BQL. These locks are uncontended (they are mostly
acquired by the corresponding vCPU thread), so acquiring them
is cheaper than acquiring the BQL, which particularly in
MTTCG can be contended at high core counts.
In this conversion we drop qemu_cpu_cond and qemu_pause_cond,
and use cpu->cond instead.
In qom/cpu.c we can finally remove the ugliness that
results from having to hold both the BQL and the CPU lock;
now we just have to grab the CPU lock.
Reviewed-by: Alex Bennée
Reviewed-by: Richard Henderson
Signed-off-by: Emilio G. Cota
---
include/qom/cpu.h | 20 ++--
cpus.c| 281 ++
qom/cpu.c | 29 +
3 files changed, 225 insertions(+), 105 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index fc768bbe00..44e6e83ecd 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -309,10 +309,6 @@ struct qemu_work_item;
* valid under cpu_list_lock.
* @created: Indicates whether the CPU thread has been successfully created.
* @interrupt_request: Indicates a pending interrupt request.
- * @halted: Nonzero if the CPU is in suspended state.
- * @stop: Indicates a pending stop request.
- * @stopped: Indicates the CPU has been artificially stopped.
- * @unplug: Indicates a pending CPU unplug request.
* @crash_occurred: Indicates the OS reported a crash (panic) for this CPU
* @singlestep_enabled: Flags for single-stepping.
* @icount_extra: Instructions until next timer event.
@@ -342,6 +338,10 @@ struct qemu_work_item;
*after the BQL.
* @cond: Condition variable for per-CPU events.
* @work_list: List of pending asynchronous work.
+ * @halted: Nonzero if the CPU is in suspended state.
+ * @stop: Indicates a pending stop request.
+ * @stopped: Indicates the CPU has been artificially stopped.
+ * @unplug: Indicates a pending CPU unplug request.
* @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
*to @trace_dstate).
* @trace_dstate: Dynamic tracing state of events for this vCPU (bitmask).
@@ -365,12 +365,7 @@ struct CPUState {
#endif
int thread_id;
bool running, has_waiter;
-struct QemuCond *halt_cond;
bool thread_kicked;
-bool created;
-bool stop;
-bool stopped;
-bool unplug;
bool crash_occurred;
bool exit_request;
uint32_t cflags_next_tb;
@@ -384,7 +379,13 @@ struct CPUState {
QemuMutex *lock;
/* fields below protected by @lock */
QemuCond cond;
+QemuCond *halt_cond;
QSIMPLEQ_HEAD(, qemu_work_item) work_list;
+uint32_t halted;
+bool created;
+bool stop;
+bool stopped;
+bool unplug;
CPUAddressSpace *cpu_ases;
int num_ases;
@@ -432,7 +433,6 @@ struct CPUState {
/* TODO Move common fields from CPUArchState here. */
int cpu_index;
int cluster_index;
-uint32_t halted;
uint32_t can_do_io;
int32_t exception_index;
diff --git a/cpus.c b/cpus.c
index 61fb6e033f..d3abf5d3dc 100644
--- a/cpus.c
+++ b/cpus.c
@@ -181,24 +181,30 @@ bool cpu_mutex_locked(const CPUState *cpu)
return test_bit(cpu->cpu_index + 1, cpu_lock_bitmap);
}
-bool cpu_is_stopped(CPUState *cpu)
+/* Called with the CPU's lock held */
+static bool cpu_is_stopped_locked(CPUState *cpu)
{
return cpu->stopped || !runstate_is_running();
}
-static inline bool cpu_work_list_empty(CPUState *cpu)
+bool cpu_is_stopped(CPUState *cpu)
{
-bool ret;
+if (!cpu_mutex_locked(cpu)) {
+bool ret;
-cpu_mutex_lock(cpu);
-ret = QSIMPLEQ_EMPTY(>work_list);
-cpu_mutex_unlock(cpu);
-return ret;
+cpu_mutex_lock(cpu);
+ret = cpu_is_stopped_locked(cpu);
+cpu_mutex_unlock(cpu);
+return ret;
+}
+return cpu_is_stopped_locked(cpu);
}
static bool cpu_thread_is_idle(CPUState *cpu)
{
-if (cpu->stop || !cpu_work_list_empty(cpu)) {
+g_assert(cpu_mutex_locked(cpu));
+
+if (cpu->stop || !QSIMPLEQ_EMPTY(>work_list)) {
return false;
}
if (cpu_is_stopped(cpu)) {
@@ -216,9 +222,17 @@ static bool qemu_tcg_rr_all_cpu_threads_idle(void)
CPUState *cpu;
g_assert(qemu_is_tcg_rr());
+g_assert(qemu_mutex_iothread_locked());
+g_assert(no_cpu_mutex_locked());
CPU_FOREACH(cpu) {
-if (!cpu_thread_is_idle(cpu)) {
+bool is_idle;
+
+cpu_mutex_lock(cpu);
+is_idle = cpu_thread_is_idle(cpu);
+cpu_mutex_unlock(cpu);
+
+if (!is_idle) {
return false;
}
}
@@ -780,6 +794,8 @@ void qemu_start_warp_timer(void)
static void qemu_account_warp_timer(void)
{
+g_assert(qemu_mutex_iothread_locked());
+
if (!use_icount || !icount_sleep) {
return;
}
@@ -1090,6 +1106,7 @@ static void kick_tcg_thread(void *opaque)
static void start_tcg_kick_timer(void)
{
assert(!mttcg_en
[Qemu-devel] [PATCH v7 32/73] cpu: define cpu_interrupt_request helpers
Add a comment about how atomic_read works here. The comment refers to
a "BQL-less CPU loop", which will materialize toward the end
of this series.
Note that the modifications to cpu_reset_interrupt are there to
avoid deadlock during the CPU lock transition; once that is complete,
cpu_interrupt_request will be simple again.
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
include/qom/cpu.h | 37 +
qom/cpu.c | 27 +--
2 files changed, 58 insertions(+), 6 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 9fe74e4392..c92a8c544a 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -526,6 +526,43 @@ static inline void cpu_halted_set(CPUState *cpu, uint32_t
val)
cpu_mutex_unlock(cpu);
}
+/*
+ * When sending an interrupt, setters OR the appropriate bit and kick the
+ * destination vCPU. The latter can then read interrupt_request without
+ * acquiring the CPU lock, because once the kick-induced completes, they'll
read
+ * an up-to-date interrupt_request.
+ * Setters always acquire the lock, which guarantees that (1) concurrent
+ * updates from different threads won't result in data races, and (2) the
+ * BQL-less CPU loop will always see an up-to-date interrupt_request, since the
+ * loop holds the CPU lock.
+ */
+static inline uint32_t cpu_interrupt_request(CPUState *cpu)
+{
+return atomic_read(>interrupt_request);
+}
+
+static inline void cpu_interrupt_request_or(CPUState *cpu, uint32_t mask)
+{
+if (cpu_mutex_locked(cpu)) {
+atomic_set(>interrupt_request, cpu->interrupt_request | mask);
+return;
+}
+cpu_mutex_lock(cpu);
+atomic_set(>interrupt_request, cpu->interrupt_request | mask);
+cpu_mutex_unlock(cpu);
+}
+
+static inline void cpu_interrupt_request_set(CPUState *cpu, uint32_t val)
+{
+if (cpu_mutex_locked(cpu)) {
+atomic_set(>interrupt_request, val);
+return;
+}
+cpu_mutex_lock(cpu);
+atomic_set(>interrupt_request, val);
+cpu_mutex_unlock(cpu);
+}
+
static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
{
unsigned int i;
diff --git a/qom/cpu.c b/qom/cpu.c
index c5106d5af8..00add81a7f 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -98,14 +98,29 @@ static void cpu_common_get_memory_mapping(CPUState *cpu,
* BQL here if we need to. cpu_interrupt assumes it is held.*/
void cpu_reset_interrupt(CPUState *cpu, int mask)
{
-bool need_lock = !qemu_mutex_iothread_locked();
+bool has_bql = qemu_mutex_iothread_locked();
+bool has_cpu_lock = cpu_mutex_locked(cpu);
-if (need_lock) {
-qemu_mutex_lock_iothread();
+if (has_bql) {
+if (has_cpu_lock) {
+atomic_set(>interrupt_request, cpu->interrupt_request &
~mask);
+} else {
+cpu_mutex_lock(cpu);
+atomic_set(>interrupt_request, cpu->interrupt_request &
~mask);
+cpu_mutex_unlock(cpu);
+}
+return;
+}
+
+if (has_cpu_lock) {
+cpu_mutex_unlock(cpu);
}
-cpu->interrupt_request &= ~mask;
-if (need_lock) {
-qemu_mutex_unlock_iothread();
+qemu_mutex_lock_iothread();
+cpu_mutex_lock(cpu);
+atomic_set(>interrupt_request, cpu->interrupt_request & ~mask);
+qemu_mutex_unlock_iothread();
+if (!has_cpu_lock) {
+cpu_mutex_unlock(cpu);
}
}
--
2.17.1
[Qemu-devel] [PATCH v7 43/73] i386/hvf: convert to cpu_request_interrupt
Reviewed-by: Richard Henderson
Signed-off-by: Emilio G. Cota
---
target/i386/hvf/hvf.c| 8 +---
target/i386/hvf/x86hvf.c | 26 +++---
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 7dc1d721ff..3ecf91f682 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -249,7 +249,7 @@ void update_apic_tpr(CPUState *cpu)
static void hvf_handle_interrupt(CPUState * cpu, int mask)
{
-cpu->interrupt_request |= mask;
+cpu_interrupt_request_or(cpu, mask);
if (!qemu_cpu_is_self(cpu)) {
qemu_cpu_kick(cpu);
}
@@ -701,10 +701,12 @@ int hvf_vcpu_exec(CPUState *cpu)
ret = 0;
switch (exit_reason) {
case EXIT_REASON_HLT: {
+uint32_t interrupt_request = cpu_interrupt_request(cpu);
+
macvm_set_rip(cpu, rip + ins_len);
-if (!((cpu->interrupt_request & CPU_INTERRUPT_HARD) &&
+if (!((interrupt_request & CPU_INTERRUPT_HARD) &&
(EFLAGS(env) & IF_MASK))
-&& !(cpu->interrupt_request & CPU_INTERRUPT_NMI) &&
+&& !(interrupt_request & CPU_INTERRUPT_NMI) &&
!(idtvec_info & VMCS_IDT_VEC_VALID)) {
cpu_halted_set(cpu, 1);
ret = EXCP_HLT;
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index e8b13ed534..91feafeedc 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -358,6 +358,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
uint8_t vector;
uint64_t intr_type;
+uint32_t interrupt_request;
bool have_event = true;
if (env->interrupt_injected != -1) {
vector = env->interrupt_injected;
@@ -400,7 +401,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
};
}
-if (cpu_state->interrupt_request & CPU_INTERRUPT_NMI) {
+if (cpu_interrupt_request(cpu_state) & CPU_INTERRUPT_NMI) {
if (!(env->hflags2 & HF2_NMI_MASK) && !(info & VMCS_INTR_VALID)) {
cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_NMI);
info = VMCS_INTR_VALID | VMCS_INTR_T_NMI | NMI_VEC;
@@ -411,7 +412,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
}
if (!(env->hflags & HF_INHIBIT_IRQ_MASK) &&
-(cpu_state->interrupt_request & CPU_INTERRUPT_HARD) &&
+(cpu_interrupt_request(cpu_state) & CPU_INTERRUPT_HARD) &&
(EFLAGS(env) & IF_MASK) && !(info & VMCS_INTR_VALID)) {
int line = cpu_get_pic_interrupt(>env);
cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_HARD);
@@ -420,39 +421,42 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
VMCS_INTR_VALID | VMCS_INTR_T_HWINTR);
}
}
-if (cpu_state->interrupt_request & CPU_INTERRUPT_HARD) {
+if (cpu_interrupt_request(cpu_state) & CPU_INTERRUPT_HARD) {
vmx_set_int_window_exiting(cpu_state);
}
-return (cpu_state->interrupt_request
-& (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR));
+return cpu_interrupt_request(cpu_state) & (CPU_INTERRUPT_INIT |
+ CPU_INTERRUPT_TPR);
}
int hvf_process_events(CPUState *cpu_state)
{
X86CPU *cpu = X86_CPU(cpu_state);
CPUX86State *env = >env;
+uint32_t interrupt_request;
EFLAGS(env) = rreg(cpu_state->hvf_fd, HV_X86_RFLAGS);
-if (cpu_state->interrupt_request & CPU_INTERRUPT_INIT) {
+if (cpu_interrupt_request(cpu_state) & CPU_INTERRUPT_INIT) {
hvf_cpu_synchronize_state(cpu_state);
do_cpu_init(cpu);
}
-if (cpu_state->interrupt_request & CPU_INTERRUPT_POLL) {
+if (cpu_interrupt_request(cpu_state) & CPU_INTERRUPT_POLL) {
cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_POLL);
apic_poll_irq(cpu->apic_state);
}
-if (((cpu_state->interrupt_request & CPU_INTERRUPT_HARD) &&
+
+interrupt_request = cpu_interrupt_request(cpu_state);
+if (((interrupt_request & CPU_INTERRUPT_HARD) &&
(EFLAGS(env) & IF_MASK)) ||
-(cpu_state->interrupt_request & CPU_INTERRUPT_NMI)) {
+(interrupt_request & CPU_INTERRUPT_NMI)) {
cpu_halted_set(cpu_state, 0);
}
-if (cpu_state->interrupt_request & CPU_INTERRUPT_SIPI) {
+if (interrupt_request & CPU_INTERRUPT_SIPI) {
hvf_cpu_synchronize_state(cpu_state);
do_cpu_sipi(cpu);
}
-if (cpu_state->interrupt_request & CPU_INTERRUPT_TPR) {
+if (cpu_interrupt_request(cpu_state) & CPU_INTERRUPT_TPR) {
cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_TPR);
hvf_cpu_synchronize_state(cpu_state);
apic_handle_tpr_access_report(cpu->apic_state, env->eip,
--
2.17.1
[Qemu-devel] [PATCH v7 72/73] cpu: add async_run_on_cpu_no_bql
Some async jobs do not need the BQL.
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
include/qom/cpu.h | 14 ++
cpus-common.c | 39 ++-
2 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 44e6e83ecd..f75f5e74ca 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -898,9 +898,23 @@ void run_on_cpu(CPUState *cpu, run_on_cpu_func func,
run_on_cpu_data data);
* @data: Data to pass to the function.
*
* Schedules the function @func for execution on the vCPU @cpu asynchronously.
+ * See also: async_run_on_cpu_no_bql()
*/
void async_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data
data);
+/**
+ * async_run_on_cpu_no_bql:
+ * @cpu: The vCPU to run on.
+ * @func: The function to be executed.
+ * @data: Data to pass to the function.
+ *
+ * Schedules the function @func for execution on the vCPU @cpu asynchronously.
+ * This function is run outside the BQL.
+ * See also: async_run_on_cpu()
+ */
+void async_run_on_cpu_no_bql(CPUState *cpu, run_on_cpu_func func,
+ run_on_cpu_data data);
+
/**
* async_safe_run_on_cpu:
* @cpu: The vCPU to run on.
diff --git a/cpus-common.c b/cpus-common.c
index 1241024b2c..5832a8bf37 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -109,6 +109,7 @@ struct qemu_work_item {
run_on_cpu_func func;
run_on_cpu_data data;
bool free, exclusive, done;
+bool bql;
};
/* Called with the CPU's lock held */
@@ -155,6 +156,7 @@ void run_on_cpu(CPUState *cpu, run_on_cpu_func func,
run_on_cpu_data data)
wi.done = false;
wi.free = false;
wi.exclusive = false;
+wi.bql = true;
cpu_mutex_lock(cpu);
queue_work_on_cpu_locked(cpu, );
@@ -179,6 +181,21 @@ void async_run_on_cpu(CPUState *cpu, run_on_cpu_func func,
run_on_cpu_data data)
wi->func = func;
wi->data = data;
wi->free = true;
+wi->bql = true;
+
+queue_work_on_cpu(cpu, wi);
+}
+
+void async_run_on_cpu_no_bql(CPUState *cpu, run_on_cpu_func func,
+ run_on_cpu_data data)
+{
+struct qemu_work_item *wi;
+
+wi = g_malloc0(sizeof(struct qemu_work_item));
+wi->func = func;
+wi->data = data;
+wi->free = true;
+/* wi->bql initialized to false */
queue_work_on_cpu(cpu, wi);
}
@@ -323,6 +340,7 @@ void async_safe_run_on_cpu(CPUState *cpu, run_on_cpu_func
func,
wi->data = data;
wi->free = true;
wi->exclusive = true;
+/* wi->bql initialized to false */
queue_work_on_cpu(cpu, wi);
}
@@ -347,6 +365,7 @@ static void process_queued_cpu_work_locked(CPUState *cpu)
* BQL, so it goes to sleep; start_exclusive() is sleeping too, so
* neither CPU can proceed.
*/
+g_assert(!wi->bql);
if (has_bql) {
qemu_mutex_unlock_iothread();
}
@@ -357,12 +376,22 @@ static void process_queued_cpu_work_locked(CPUState *cpu)
qemu_mutex_lock_iothread();
}
} else {
-if (has_bql) {
-wi->func(cpu, wi->data);
+if (wi->bql) {
+if (has_bql) {
+wi->func(cpu, wi->data);
+} else {
+qemu_mutex_lock_iothread();
+wi->func(cpu, wi->data);
+qemu_mutex_unlock_iothread();
+}
} else {
-qemu_mutex_lock_iothread();
-wi->func(cpu, wi->data);
-qemu_mutex_unlock_iothread();
+if (has_bql) {
+qemu_mutex_unlock_iothread();
+wi->func(cpu, wi->data);
+qemu_mutex_lock_iothread();
+} else {
+wi->func(cpu, wi->data);
+}
}
}
cpu_mutex_lock(cpu);
--
2.17.1
[Qemu-devel] [PATCH v7 56/73] openrisc: convert to cpu_interrupt_request
Cc: Stafford Horne
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
hw/openrisc/cputimer.c | 2 +-
target/openrisc/cpu.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/openrisc/cputimer.c b/hw/openrisc/cputimer.c
index 850f88761c..739404e4f5 100644
--- a/hw/openrisc/cputimer.c
+++ b/hw/openrisc/cputimer.c
@@ -102,7 +102,7 @@ static void openrisc_timer_cb(void *opaque)
CPUState *cs = CPU(cpu);
cpu->env.ttmr |= TTMR_IP;
-cs->interrupt_request |= CPU_INTERRUPT_TIMER;
+cpu_interrupt_request_or(cs, CPU_INTERRUPT_TIMER);
}
switch (cpu->env.ttmr & TTMR_M) {
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 541b2a66c7..a01737dfda 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -32,8 +32,8 @@ static void openrisc_cpu_set_pc(CPUState *cs, vaddr value)
static bool openrisc_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & (CPU_INTERRUPT_HARD |
-CPU_INTERRUPT_TIMER);
+return cpu_interrupt_request(cs) & (CPU_INTERRUPT_HARD |
+CPU_INTERRUPT_TIMER);
}
static void openrisc_disas_set_info(CPUState *cpu, disassemble_info *info)
--
2.17.1
[Qemu-devel] [PATCH v7 18/73] ppc: convert to cpu_halted
In ppce500_spin.c, acquire the lock just once to update
both cpu->halted and cpu->stopped.
In hw/ppc/spapr_hcall.c, acquire the lock just once to
update cpu->halted and call cpu_has_work, since later
in the series we'll acquire the BQL (if not already held)
from cpu_has_work.
Cc: David Gibson
Cc: qemu-...@nongnu.org
Reviewed-by: Richard Henderson
Acked-by: David Gibson
Signed-off-by: Emilio G. Cota
---
target/ppc/helper_regs.h| 2 +-
hw/ppc/e500.c | 4 ++--
hw/ppc/ppc.c| 10 +-
hw/ppc/ppce500_spin.c | 6 --
hw/ppc/spapr_cpu_core.c | 4 ++--
hw/ppc/spapr_hcall.c| 4 +++-
hw/ppc/spapr_rtas.c | 6 +++---
target/ppc/excp_helper.c| 4 ++--
target/ppc/kvm.c| 4 ++--
target/ppc/translate_init.inc.c | 6 +++---
10 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
index a2205e1044..5afce54318 100644
--- a/target/ppc/helper_regs.h
+++ b/target/ppc/helper_regs.h
@@ -161,7 +161,7 @@ static inline int hreg_store_msr(CPUPPCState *env,
target_ulong value,
#if !defined(CONFIG_USER_ONLY)
if (unlikely(msr_pow == 1)) {
if (!env->pending_interrupts && (*env->check_pow)(env)) {
-cs->halted = 1;
+cpu_halted_set(cs, 1);
excp = EXCP_HALTED;
}
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 7553f674c9..4e82906ef2 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -657,7 +657,7 @@ static void ppce500_cpu_reset_sec(void *opaque)
/* Secondary CPU starts in halted state for now. Needs to change when
implementing non-kernel boot. */
-cs->halted = 1;
+cpu_halted_set(cs, 1);
cs->exception_index = EXCP_HLT;
}
@@ -671,7 +671,7 @@ static void ppce500_cpu_reset(void *opaque)
cpu_reset(cs);
/* Set initial guest state. */
-cs->halted = 0;
+cpu_halted_set(cs, 0);
env->gpr[1] = (16 * MiB) - 8;
env->gpr[3] = bi->dt_base;
env->gpr[4] = 0;
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index d1e3d4cd20..45e212390a 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -149,7 +149,7 @@ static void ppc6xx_set_irq(void *opaque, int pin, int level)
/* XXX: Note that the only way to restart the CPU is to reset it */
if (level) {
LOG_IRQ("%s: stop the CPU\n", __func__);
-cs->halted = 1;
+cpu_halted_set(cs, 1);
}
break;
case PPC6xx_INPUT_HRESET:
@@ -228,10 +228,10 @@ static void ppc970_set_irq(void *opaque, int pin, int
level)
/* XXX: TODO: relay the signal to CKSTP_OUT pin */
if (level) {
LOG_IRQ("%s: stop the CPU\n", __func__);
-cs->halted = 1;
+cpu_halted_set(cs, 1);
} else {
LOG_IRQ("%s: restart the CPU\n", __func__);
-cs->halted = 0;
+cpu_halted_set(cs, 0);
qemu_cpu_kick(cs);
}
break;
@@ -457,10 +457,10 @@ static void ppc40x_set_irq(void *opaque, int pin, int
level)
/* Level sensitive - active low */
if (level) {
LOG_IRQ("%s: stop the CPU\n", __func__);
-cs->halted = 1;
+cpu_halted_set(cs, 1);
} else {
LOG_IRQ("%s: restart the CPU\n", __func__);
-cs->halted = 0;
+cpu_halted_set(cs, 0);
qemu_cpu_kick(cs);
}
break;
diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
index c45fc858de..4b3532730f 100644
--- a/hw/ppc/ppce500_spin.c
+++ b/hw/ppc/ppce500_spin.c
@@ -107,9 +107,11 @@ static void spin_kick(CPUState *cs, run_on_cpu_data data)
map_start = ldq_p(>addr) & ~(map_size - 1);
mmubooke_create_initial_mapping(env, 0, map_start, map_size);
-cs->halted = 0;
-cs->exception_index = -1;
+cpu_mutex_lock(cs);
+cpu_halted_set(cs, 0);
cs->stopped = false;
+cpu_mutex_unlock(cs);
+cs->exception_index = -1;
qemu_cpu_kick(cs);
}
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ef6cbb9c29..cbb8474cad 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -36,7 +36,7 @@ static void spapr_cpu_reset(void *opaque)
/* All CPUs start halted. CPU0 is unhalted from the machine level
* reset code and the rest are explicitly started up by the guest
* using an RTAS call */
-cs->halted = 1;
+cpu_halted_set(cs, 1);
/* Set compatibility mode to match the boot CPU, which was either set
* by the machine reset code or by CAS. This should never fail.
@@ -90,7 +90,7 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ul
[Qemu-devel] [PATCH v7 38/73] arm: convert to cpu_interrupt_request
Cc: Peter Maydell
Cc: qemu-...@nongnu.org
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/arm/cpu.c | 6 +++---
target/arm/helper.c | 16 +++-
target/arm/machine.c | 2 +-
3 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 845730c00c..7037e22580 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -73,7 +73,7 @@ static bool arm_cpu_has_work(CPUState *cs)
ARMCPU *cpu = ARM_CPU(cs);
return (cpu->power_state != PSCI_OFF)
-&& cs->interrupt_request &
+&& cpu_interrupt_request(cs) &
(CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
| CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ
| CPU_INTERRUPT_EXITTB);
@@ -484,7 +484,7 @@ void arm_cpu_update_virq(ARMCPU *cpu)
bool new_state = (env->cp15.hcr_el2 & HCR_VI) ||
(env->irq_line_state & CPU_INTERRUPT_VIRQ);
-if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VIRQ) != 0)) {
+if (new_state != ((cpu_interrupt_request(cs) & CPU_INTERRUPT_VIRQ) != 0)) {
if (new_state) {
cpu_interrupt(cs, CPU_INTERRUPT_VIRQ);
} else {
@@ -505,7 +505,7 @@ void arm_cpu_update_vfiq(ARMCPU *cpu)
bool new_state = (env->cp15.hcr_el2 & HCR_VF) ||
(env->irq_line_state & CPU_INTERRUPT_VFIQ);
-if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VFIQ) != 0)) {
+if (new_state != ((cpu_interrupt_request(cs) & CPU_INTERRUPT_VFIQ) != 0)) {
if (new_state) {
cpu_interrupt(cs, CPU_INTERRUPT_VFIQ);
} else {
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1fa282a7fc..2affe7965c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1893,23 +1893,24 @@ static uint64_t isr_read(CPUARMState *env, const
ARMCPRegInfo *ri)
CPUState *cs = ENV_GET_CPU(env);
uint64_t hcr_el2 = arm_hcr_el2_eff(env);
uint64_t ret = 0;
+uint32_t interrupt_request = cpu_interrupt_request(cs);
if (hcr_el2 & HCR_IMO) {
-if (cs->interrupt_request & CPU_INTERRUPT_VIRQ) {
+if (interrupt_request & CPU_INTERRUPT_VIRQ) {
ret |= CPSR_I;
}
} else {
-if (cs->interrupt_request & CPU_INTERRUPT_HARD) {
+if (interrupt_request & CPU_INTERRUPT_HARD) {
ret |= CPSR_I;
}
}
if (hcr_el2 & HCR_FMO) {
-if (cs->interrupt_request & CPU_INTERRUPT_VFIQ) {
+if (interrupt_request & CPU_INTERRUPT_VFIQ) {
ret |= CPSR_F;
}
} else {
-if (cs->interrupt_request & CPU_INTERRUPT_FIQ) {
+if (interrupt_request & CPU_INTERRUPT_FIQ) {
ret |= CPSR_F;
}
}
@@ -9647,10 +9648,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
return;
}
-/* Hooks may change global state so BQL should be held, also the
- * BQL needs to be held for any modification of
- * cs->interrupt_request.
- */
+/* Hooks may change global state so BQL should be held */
g_assert(qemu_mutex_iothread_locked());
arm_call_pre_el_change_hook(cpu);
@@ -9665,7 +9663,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
arm_call_el_change_hook(cpu);
if (!kvm_enabled()) {
-cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+cpu_interrupt_request_or(cs, CPU_INTERRUPT_EXITTB);
}
}
#endif /* !CONFIG_USER_ONLY */
diff --git a/target/arm/machine.c b/target/arm/machine.c
index b292549614..4f2099ecde 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -693,7 +693,7 @@ static int cpu_post_load(void *opaque, int version_id)
if (env->irq_line_state == UINT32_MAX) {
CPUState *cs = CPU(cpu);
-env->irq_line_state = cs->interrupt_request &
+env->irq_line_state = cpu_interrupt_request(cs) &
(CPU_INTERRUPT_HARD | CPU_INTERRUPT_FIQ |
CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VFIQ);
}
--
2.17.1
[Qemu-devel] [PATCH v7 47/73] hppa: convert to cpu_interrupt_request
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/hppa/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 00bf444620..1ab4e62850 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -60,7 +60,7 @@ static void hppa_cpu_synchronize_from_tb(CPUState *cs,
TranslationBlock *tb)
static bool hppa_cpu_has_work(CPUState *cs)
{
-return cs->interrupt_request & CPU_INTERRUPT_HARD;
+return cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
}
static void hppa_cpu_disas_set_info(CPUState *cs, disassemble_info *info)
--
2.17.1
[Qemu-devel] [PATCH v7 73/73] cputlb: queue async flush jobs without the BQL
This yields sizable scalability improvements, as the below results show. Host: Two Intel E5-2683 v3 14-core CPUs at 2.00 GHz (Haswell) Workload: Ubuntu 18.04 ppc64 compiling the linux kernel with "make -j N", where N is the number of cores in the guest. Speedup vs a single thread (higher is better): 14 +---+ | ++ + + + + $$ + | |$ | | $$ | 12 |-+$A$$ +-| |$$ | | $$$ | 10 |-+ $$##D#D +-| |$$$ #**B | | $$* * | |A$#* B | 8 |-+$$B** +-| |$$** | | $** | 6 |-+ $$* +-| |A**| | $B | | $ | 4 |-+$* +-| | $| | $ | 2 |-+ $+-| |$ +cputlb-no-bql $$A$$ | | A +per-cpu-lock ##D## | | ++ + + + + baseline **B** | 0 +---+ 14 8 12 16 20 24 28 Guest vCPUs png: https://imgur.com/zZRvS7q Some notes: - baseline corresponds to the commit before this series - per-cpu-lock is the commit that converts the CPU loop to per-cpu locks. - cputlb-no-bql is this commit. - I'm using taskset to assign cores to threads, favouring locality whenever possible but not using SMT. When N=1, I'm using a single host core, which leads to superlinear speedups (since with more cores the I/O thread can execute while vCPU threads sleep). In the future I might use N+1 host cores for N guest cores to avoid this, or perhaps pin guest threads to cores one-by-one. Single-threaded performance is affected very lightly. Results below for debian aarch64 bootup+test for the entire series on an Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz host: - Before: Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs): 7269.033478 task-clock (msec) #0.998 CPUs utilized ( +- 0.06% ) 30,659,870,302 cycles#4.218 GHz ( +- 0.06% ) 54,790,540,051 instructions #1.79 insns per cycle ( +- 0.05% ) 9,796,441,380 branches # 1347.695 M/sec ( +- 0.05% ) 165,132,201 branch-misses #1.69% of all branches ( +- 0.12% ) 7.287011656 seconds time elapsed ( +- 0.10% ) - After: 7375.924053 task-clock (msec) #0.998 CPUs utilized ( +- 0.13% ) 31,107,548,846 cycles#4.217 GHz ( +- 0.12% ) 55,355,668,947 instructions #1.78 insns per cycle ( +- 0.05% ) 9,929,917,664 branches # 1346.261 M/sec ( +- 0.04% ) 166,547,442 branch-misses #1.68% of all branches ( +- 0.09% ) 7.389068145 seconds time elapsed ( +- 0.13% ) That is, a 1.37% slowdown. Reviewed-by: Alex Bennée Reviewed-by: Richard Henderson Tested-by: Alex Bennée Signed-off-by: Emilio G. Cota --- accel/tcg/cputlb.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 88cc8389e9..d9e0814b5c 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -260,7 +260,7 @@ static void flush_all_helper(CPU
[Qemu-devel] [PATCH v7 26/73] sparc: convert to cpu_halted
Cc: Fabien Chouteau
Cc: Mark Cave-Ayland
Cc: Artyom Tarasenko
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Reviewed-by: Mark Cave-Ayland
Signed-off-by: Emilio G. Cota
---
hw/sparc/leon3.c | 2 +-
hw/sparc/sun4m.c | 8
hw/sparc64/sparc64.c | 4 ++--
target/sparc/helper.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 774639af33..7b9239af87 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -61,7 +61,7 @@ static void main_cpu_reset(void *opaque)
cpu_reset(cpu);
-cpu->halted = 0;
+cpu_halted_set(cpu, 0);
env->pc = s->entry;
env->npc= s->entry + 4;
env->regbase[6] = s->sp;
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index ca1e3825d5..4aee7eef48 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -167,7 +167,7 @@ static void cpu_kick_irq(SPARCCPU *cpu)
CPUSPARCState *env = >env;
CPUState *cs = CPU(cpu);
-cs->halted = 0;
+cpu_halted_set(cs, 0);
cpu_check_irqs(env);
qemu_cpu_kick(cs);
}
@@ -198,7 +198,7 @@ static void main_cpu_reset(void *opaque)
CPUState *cs = CPU(cpu);
cpu_reset(cs);
-cs->halted = 0;
+cpu_halted_set(cs, 0);
}
static void secondary_cpu_reset(void *opaque)
@@ -207,7 +207,7 @@ static void secondary_cpu_reset(void *opaque)
CPUState *cs = CPU(cpu);
cpu_reset(cs);
-cs->halted = 1;
+cpu_halted_set(cs, 1);
}
static void cpu_halt_signal(void *opaque, int irq, int level)
@@ -828,7 +828,7 @@ static void cpu_devinit(const char *cpu_type, unsigned int
id,
} else {
qemu_register_reset(secondary_cpu_reset, cpu);
cs = CPU(cpu);
-cs->halted = 1;
+cpu_halted_set(cs, 1);
}
*cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS);
env->prom_addr = prom_addr;
diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index 408388945e..372bbd4f5b 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -100,7 +100,7 @@ static void cpu_kick_irq(SPARCCPU *cpu)
CPUState *cs = CPU(cpu);
CPUSPARCState *env = >env;
-cs->halted = 0;
+cpu_halted_set(cs, 0);
cpu_check_irqs(env);
qemu_cpu_kick(cs);
}
@@ -115,7 +115,7 @@ void sparc64_cpu_set_ivec_irq(void *opaque, int irq, int
level)
if (!(env->ivec_status & 0x20)) {
trace_sparc64_cpu_ivec_raise_irq(irq);
cs = CPU(cpu);
-cs->halted = 0;
+cpu_halted_set(cs, 0);
env->interrupt_index = TT_IVEC;
env->ivec_status |= 0x20;
env->ivec_data[0] = (0x1f << 6) | irq;
diff --git a/target/sparc/helper.c b/target/sparc/helper.c
index 46232788c8..dd00cf7cac 100644
--- a/target/sparc/helper.c
+++ b/target/sparc/helper.c
@@ -245,7 +245,7 @@ void helper_power_down(CPUSPARCState *env)
{
CPUState *cs = CPU(sparc_env_get_cpu(env));
-cs->halted = 1;
+cpu_halted_set(cs, 1);
cs->exception_index = EXCP_HLT;
env->pc = env->npc;
env->npc = env->pc + 4;
--
2.17.1
[Qemu-devel] [PATCH v7 68/73] xtensa: convert to cpu_has_work_with_iothread_lock
Soon we will call cpu_has_work without the BQL. Cc: Max Filippov Reviewed-by: Richard Henderson Signed-off-by: Emilio G. Cota --- target/xtensa/cpu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c index d4ca35e6cc..5f3b4a70b0 100644 --- a/target/xtensa/cpu.c +++ b/target/xtensa/cpu.c @@ -47,6 +47,8 @@ static bool xtensa_cpu_has_work(CPUState *cs) #ifndef CONFIG_USER_ONLY XtensaCPU *cpu = XTENSA_CPU(cs); +g_assert(qemu_mutex_iothread_locked()); + return !cpu->env.runstall && cpu->env.pending_irq_level; #else return true; @@ -173,7 +175,7 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data) cc->reset = xtensa_cpu_reset; cc->class_by_name = xtensa_cpu_class_by_name; -cc->has_work = xtensa_cpu_has_work; +cc->has_work_with_iothread_lock = xtensa_cpu_has_work; cc->do_interrupt = xtensa_cpu_do_interrupt; cc->cpu_exec_interrupt = xtensa_cpu_exec_interrupt; cc->dump_state = xtensa_cpu_dump_state; -- 2.17.1
[Qemu-devel] [PATCH v7 53/73] alpha: convert to cpu_interrupt_request
Reviewed-by: Richard Henderson Reviewed-by: Alex Bennée Signed-off-by: Emilio G. Cota --- target/alpha/cpu.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c index 1fd95d6c0f..cebd459251 100644 --- a/target/alpha/cpu.c +++ b/target/alpha/cpu.c @@ -42,10 +42,10 @@ static bool alpha_cpu_has_work(CPUState *cs) assume that if a CPU really wants to stay asleep, it will mask interrupts at the chipset level, which will prevent these bits from being set in the first place. */ -return cs->interrupt_request & (CPU_INTERRUPT_HARD -| CPU_INTERRUPT_TIMER -| CPU_INTERRUPT_SMP -| CPU_INTERRUPT_MCHK); +return cpu_interrupt_request(cs) & (CPU_INTERRUPT_HARD +| CPU_INTERRUPT_TIMER +| CPU_INTERRUPT_SMP +| CPU_INTERRUPT_MCHK); } static void alpha_cpu_disas_set_info(CPUState *cpu, disassemble_info *info) -- 2.17.1
[Qemu-devel] [PATCH v7 42/73] i386/whpx-all: convert to cpu_interrupt_request
Reviewed-by: Richard Henderson
Signed-off-by: Emilio G. Cota
---
target/i386/whpx-all.c | 41 -
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index 9673bdc219..0d8cfa3a19 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -690,12 +690,14 @@ static int whpx_handle_portio(CPUState *cpu,
static int whpx_handle_halt(CPUState *cpu)
{
struct CPUX86State *env = (CPUArchState *)(cpu->env_ptr);
+uint32_t interrupt_request;
int ret = 0;
qemu_mutex_lock_iothread();
-if (!((cpu->interrupt_request & CPU_INTERRUPT_HARD) &&
+interrupt_request = cpu_interrupt_request(cpu);
+if (!((interrupt_request & CPU_INTERRUPT_HARD) &&
(env->eflags & IF_MASK)) &&
-!(cpu->interrupt_request & CPU_INTERRUPT_NMI)) {
+!(interrupt_request & CPU_INTERRUPT_NMI)) {
cpu->exception_index = EXCP_HLT;
cpu_halted_set(cpu, true);
ret = 1;
@@ -713,6 +715,7 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
struct CPUX86State *env = (CPUArchState *)(cpu->env_ptr);
X86CPU *x86_cpu = X86_CPU(cpu);
int irq;
+uint32_t interrupt_request;
uint8_t tpr;
WHV_X64_PENDING_INTERRUPTION_REGISTER new_int;
UINT32 reg_count = 0;
@@ -724,17 +727,19 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
qemu_mutex_lock_iothread();
+interrupt_request = cpu_interrupt_request(cpu);
+
/* Inject NMI */
if (!vcpu->interruption_pending &&
-cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
-if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
+interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
+if (interrupt_request & CPU_INTERRUPT_NMI) {
cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
vcpu->interruptable = false;
new_int.InterruptionType = WHvX64PendingNmi;
new_int.InterruptionPending = 1;
new_int.InterruptionVector = 2;
}
-if (cpu->interrupt_request & CPU_INTERRUPT_SMI) {
+if (interrupt_request & CPU_INTERRUPT_SMI) {
cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
}
}
@@ -743,12 +748,12 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
* Force the VCPU out of its inner loop to process any INIT requests or
* commit pending TPR access.
*/
-if (cpu->interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
-if ((cpu->interrupt_request & CPU_INTERRUPT_INIT) &&
+if (interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
+if ((interrupt_request & CPU_INTERRUPT_INIT) &&
!(env->hflags & HF_SMM_MASK)) {
cpu->exit_request = 1;
}
-if (cpu->interrupt_request & CPU_INTERRUPT_TPR) {
+if (interrupt_request & CPU_INTERRUPT_TPR) {
cpu->exit_request = 1;
}
}
@@ -757,7 +762,7 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
if (!vcpu->interruption_pending &&
vcpu->interruptable && (env->eflags & IF_MASK)) {
assert(!new_int.InterruptionPending);
-if (cpu->interrupt_request & CPU_INTERRUPT_HARD) {
+if (interrupt_request & CPU_INTERRUPT_HARD) {
cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
irq = cpu_get_pic_interrupt(env);
if (irq >= 0) {
@@ -787,7 +792,7 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
/* Update the state of the interrupt delivery notification */
if (!vcpu->window_registered &&
-cpu->interrupt_request & CPU_INTERRUPT_HARD) {
+cpu_interrupt_request(cpu) & CPU_INTERRUPT_HARD) {
reg_values[reg_count].DeliverabilityNotifications.InterruptNotification
= 1;
vcpu->window_registered = 1;
@@ -840,8 +845,9 @@ static void whpx_vcpu_process_async_events(CPUState *cpu)
struct CPUX86State *env = (CPUArchState *)(cpu->env_ptr);
X86CPU *x86_cpu = X86_CPU(cpu);
struct whpx_vcpu *vcpu = get_whpx_vcpu(cpu);
+uint32_t interrupt_request;
-if ((cpu->interrupt_request & CPU_INTERRUPT_INIT) &&
+if ((cpu_interrupt_request(cpu) & CPU_INTERRUPT_INIT) &&
!(env->hflags & HF_SMM_MASK)) {
do_cpu_init(x86_cpu);
@@ -849,25 +855,26 @@ static void whpx_vcpu_process_async_events(CPUState *cpu)
vcpu->interruptable = true;
}
-if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
+if (cpu_interrupt_request(cpu) & CPU_INTERRUPT_POLL) {
cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
apic_poll_irq(x86_cpu->apic_state);
}
-if (((cpu-
[Qemu-devel] [PATCH v7 28/73] gdbstub: convert to cpu_halted
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
gdbstub.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index bc774ae992..569b504552 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1664,13 +1664,13 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
object_get_canonical_path_component(OBJECT(cpu));
len = snprintf((char *)mem_buf, sizeof(buf) / 2,
"%s %s [%s]", cpu_model, cpu_name,
- cpu->halted ? "halted " : "running");
+ cpu_halted(cpu) ? "halted " : "running");
g_free(cpu_name);
} else {
/* memtohex() doubles the required space */
len = snprintf((char *)mem_buf, sizeof(buf) / 2,
"CPU#%d [%s]", cpu->cpu_index,
- cpu->halted ? "halted " : "running");
+ cpu_halted(cpu) ? "halted " : "running");
}
trace_gdbstub_op_extra_info((char *)mem_buf);
memtohex(buf, mem_buf, len);
--
2.17.1
[Qemu-devel] [PATCH v7 55/73] sparc: convert to cpu_interrupt_request
Cc: Mark Cave-Ayland
Cc: Artyom Tarasenko
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Reviewed-by: Mark Cave-Ayland
Signed-off-by: Emilio G. Cota
---
hw/sparc64/sparc64.c | 4 ++--
target/sparc/cpu.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index 372bbd4f5b..58faeb111a 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -56,7 +56,7 @@ void cpu_check_irqs(CPUSPARCState *env)
/* The bit corresponding to psrpil is (1<< psrpil), the next bit
is (2 << psrpil). */
if (pil < (2 << env->psrpil)) {
-if (cs->interrupt_request & CPU_INTERRUPT_HARD) {
+if (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD) {
trace_sparc64_cpu_check_irqs_reset_irq(env->interrupt_index);
env->interrupt_index = 0;
cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
@@ -87,7 +87,7 @@ void cpu_check_irqs(CPUSPARCState *env)
break;
}
}
-} else if (cs->interrupt_request & CPU_INTERRUPT_HARD) {
+} else if (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD) {
trace_sparc64_cpu_check_irqs_disabled(pil, env->pil_in, env->softint,
env->interrupt_index);
env->interrupt_index = 0;
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 4a4445bdf5..933fd8e954 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -708,7 +708,7 @@ static bool sparc_cpu_has_work(CPUState *cs)
SPARCCPU *cpu = SPARC_CPU(cs);
CPUSPARCState *env = >env;
-return (cs->interrupt_request & CPU_INTERRUPT_HARD) &&
+return (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD) &&
cpu_interrupts_enabled(env);
}
--
2.17.1
[Qemu-devel] [PATCH v7 36/73] s390x: use cpu_reset_interrupt
From: Paolo Bonzini
Cc: Cornelia Huck
Cc: David Hildenbrand
Cc: qemu-s3...@nongnu.org
Reviewed-by: David Hildenbrand
Reviewed-by: Richard Henderson
Reviewed-by: Cornelia Huck
Reviewed-by: Alex Bennée
Signed-off-by: Paolo Bonzini
Signed-off-by: Emilio G. Cota
---
target/s390x/excp_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 633d59346b..56bb107b48 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -454,7 +454,7 @@ try_deliver:
/* we might still have pending interrupts, but not deliverable */
if (!env->pending_int && !qemu_s390_flic_has_any(flic)) {
-cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
+cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
}
/* WAIT PSW during interrupt injection or STOP interrupt */
--
2.17.1
[Qemu-devel] [PATCH v7 41/73] i386/hax-all: convert to cpu_interrupt_request
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/i386/hax-all.c | 30 +-
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index 518c6ff103..18da1808c6 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -284,7 +284,7 @@ int hax_vm_destroy(struct hax_vm *vm)
static void hax_handle_interrupt(CPUState *cpu, int mask)
{
-cpu->interrupt_request |= mask;
+cpu_interrupt_request_or(cpu, mask);
if (!qemu_cpu_is_self(cpu)) {
qemu_cpu_kick(cpu);
@@ -418,7 +418,7 @@ static int hax_vcpu_interrupt(CPUArchState *env)
* Unlike KVM, HAX kernel check for the eflags, instead of qemu
*/
if (ht->ready_for_interrupt_injection &&
-(cpu->interrupt_request & CPU_INTERRUPT_HARD)) {
+(cpu_interrupt_request(cpu) & CPU_INTERRUPT_HARD)) {
int irq;
irq = cpu_get_pic_interrupt(env);
@@ -432,7 +432,7 @@ static int hax_vcpu_interrupt(CPUArchState *env)
* interrupt, request an interrupt window exit. This will
* cause a return to userspace as soon as the guest is ready to
* receive interrupts. */
-if ((cpu->interrupt_request & CPU_INTERRUPT_HARD)) {
+if ((cpu_interrupt_request(cpu) & CPU_INTERRUPT_HARD)) {
ht->request_interrupt_window = 1;
} else {
ht->request_interrupt_window = 0;
@@ -473,19 +473,19 @@ static int hax_vcpu_hax_exec(CPUArchState *env)
cpu_halted_set(cpu, 0);
-if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
+if (cpu_interrupt_request(cpu) & CPU_INTERRUPT_POLL) {
cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
apic_poll_irq(x86_cpu->apic_state);
}
-if (cpu->interrupt_request & CPU_INTERRUPT_INIT) {
+if (cpu_interrupt_request(cpu) & CPU_INTERRUPT_INIT) {
DPRINTF("\nhax_vcpu_hax_exec: handling INIT for %d\n",
cpu->cpu_index);
do_cpu_init(x86_cpu);
hax_vcpu_sync_state(env, 1);
}
-if (cpu->interrupt_request & CPU_INTERRUPT_SIPI) {
+if (cpu_interrupt_request(cpu) & CPU_INTERRUPT_SIPI) {
DPRINTF("hax_vcpu_hax_exec: handling SIPI for %d\n",
cpu->cpu_index);
hax_vcpu_sync_state(env, 0);
@@ -544,13 +544,17 @@ static int hax_vcpu_hax_exec(CPUArchState *env)
ret = -1;
break;
case HAX_EXIT_HLT:
-if (!(cpu->interrupt_request & CPU_INTERRUPT_HARD) &&
-!(cpu->interrupt_request & CPU_INTERRUPT_NMI)) {
-/* hlt instruction with interrupt disabled is shutdown */
-env->eflags |= IF_MASK;
-cpu_halted_set(cpu, 1);
-cpu->exception_index = EXCP_HLT;
-ret = 1;
+{
+uint32_t interrupt_request = cpu_interrupt_request(cpu);
+
+if (!(interrupt_request & CPU_INTERRUPT_HARD) &&
+!(interrupt_request & CPU_INTERRUPT_NMI)) {
+/* hlt instruction with interrupt disabled is shutdown */
+env->eflags |= IF_MASK;
+cpu_halted_set(cpu, 1);
+cpu->exception_index = EXCP_HLT;
+ret = 1;
+}
}
break;
/* these situations will continue to hax module */
--
2.17.1
[Qemu-devel] [PATCH v7 37/73] openrisc: use cpu_reset_interrupt
From: Paolo Bonzini
Cc: Stafford Horne
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Paolo Bonzini
Signed-off-by: Emilio G. Cota
---
target/openrisc/sys_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index 0915e622e6..1f367de6f2 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -170,7 +170,7 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr,
target_ulong rb)
env->ttmr = (rb & ~TTMR_IP) | ip;
} else {/* Clear IP bit. */
env->ttmr = rb & ~TTMR_IP;
-cs->interrupt_request &= ~CPU_INTERRUPT_TIMER;
+cpu_reset_interrupt(cs, CPU_INTERRUPT_TIMER);
}
cpu_openrisc_timer_update(cpu);
--
2.17.1
[Qemu-devel] [PATCH v7 20/73] i386: convert to cpu_halted
Cc: Eduardo Habkost
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
target/i386/cpu.h | 2 +-
target/i386/cpu.c | 2 +-
target/i386/hax-all.c | 4 ++--
target/i386/helper.c | 4 ++--
target/i386/hvf/hvf.c | 4 ++--
target/i386/hvf/x86hvf.c | 4 ++--
target/i386/kvm.c | 10 +-
target/i386/misc_helper.c | 2 +-
target/i386/whpx-all.c| 6 +++---
9 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 95112b9118..65b4e5341b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1617,7 +1617,7 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU
*cpu,
sipi_vector << 12,
env->segs[R_CS].limit,
env->segs[R_CS].flags);
-cs->halted = 0;
+cpu_halted_set(cs, 0);
}
int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d3aa6a815b..fe23d684db 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4731,7 +4731,7 @@ static void x86_cpu_reset(CPUState *s)
/* We hard-wire the BSP to the first CPU. */
apic_designate_bsp(cpu->apic_state, s->cpu_index == 0);
-s->halted = !cpu_is_bsp(cpu);
+cpu_halted_set(s, !cpu_is_bsp(cpu));
if (kvm_enabled()) {
kvm_arch_reset_vcpu(cpu);
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index b978a9b821..22951017cf 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -471,7 +471,7 @@ static int hax_vcpu_hax_exec(CPUArchState *env)
return 0;
}
-cpu->halted = 0;
+cpu_halted_set(cpu, 0);
if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
cpu->interrupt_request &= ~CPU_INTERRUPT_POLL;
@@ -548,7 +548,7 @@ static int hax_vcpu_hax_exec(CPUArchState *env)
!(cpu->interrupt_request & CPU_INTERRUPT_NMI)) {
/* hlt instruction with interrupt disabled is shutdown */
env->eflags |= IF_MASK;
-cpu->halted = 1;
+cpu_halted_set(cpu, 1);
cpu->exception_index = EXCP_HLT;
ret = 1;
}
diff --git a/target/i386/helper.c b/target/i386/helper.c
index e695f8ba7a..a75278f954 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -454,7 +454,7 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
fprintf_function cpu_fprintf,
(env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1,
(env->a20_mask >> 20) & 1,
(env->hflags >> HF_SMM_SHIFT) & 1,
-cs->halted);
+cpu_halted(cs));
} else
#endif
{
@@ -481,7 +481,7 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
fprintf_function cpu_fprintf,
(env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1,
(env->a20_mask >> 20) & 1,
(env->hflags >> HF_SMM_SHIFT) & 1,
-cs->halted);
+cpu_halted(cs));
}
for(i = 0; i < 6; i++) {
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 42f9447303..7dc1d721ff 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -672,7 +672,7 @@ int hvf_vcpu_exec(CPUState *cpu)
vmx_update_tpr(cpu);
qemu_mutex_unlock_iothread();
-if (!cpu_is_bsp(X86_CPU(cpu)) && cpu->halted) {
+if (!cpu_is_bsp(X86_CPU(cpu)) && cpu_halted(cpu)) {
qemu_mutex_lock_iothread();
return EXCP_HLT;
}
@@ -706,7 +706,7 @@ int hvf_vcpu_exec(CPUState *cpu)
(EFLAGS(env) & IF_MASK))
&& !(cpu->interrupt_request & CPU_INTERRUPT_NMI) &&
!(idtvec_info & VMCS_IDT_VEC_VALID)) {
-cpu->halted = 1;
+cpu_halted_set(cpu, 1);
ret = EXCP_HLT;
}
ret = EXCP_INTERRUPT;
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index df8e946fbc..163bbed23f 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -446,7 +446,7 @@ int hvf_process_events(CPUState *cpu_state)
if (((cpu_state->interrupt_request & CPU_INTERRUPT_HARD) &&
(EFLAGS(env) & IF_MASK)) ||
(cpu_state->interrupt_request & CPU_INTERRUPT_NMI)) {
-cpu_state->halted = 0;
+cpu_halted_set(cpu_state, 0);
}
if (cpu_state->interrupt_request & CPU_INTERRUPT_SIPI) {
hvf_cpu_synchronize_state(cpu_state);
@@ -458,5 +458,5 @@ int hvf_process_events(CPUState *cpu_state)
apic_handle_tpr_access_report(cpu->apic_state, env->eip,
[Qemu-devel] [PATCH v7 25/73] s390x: convert to cpu_halted
Cc: Cornelia Huck
Cc: Christian Borntraeger
Cc: David Hildenbrand
Cc: qemu-s3...@nongnu.org
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Reviewed-by: Cornelia Huck
Signed-off-by: Emilio G. Cota
---
hw/intc/s390_flic.c| 2 +-
target/s390x/cpu.c | 22 +++---
target/s390x/excp_helper.c | 2 +-
target/s390x/kvm.c | 2 +-
target/s390x/sigp.c| 8
5 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index 5f8168f0f0..bfb5cf1d07 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -198,7 +198,7 @@ static void qemu_s390_flic_notify(uint32_t type)
}
/* we always kick running CPUs for now, this is tricky */
-if (cs->halted) {
+if (cpu_halted(cs)) {
/* don't check for subclasses, CPUs double check when waking up */
if (type & FLIC_PENDING_SERVICE) {
if (!(cpu->env.psw.mask & PSW_MASK_EXT)) {
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 698dd9cb82..4e17df5c39 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -285,7 +285,7 @@ static void s390_cpu_initfn(Object *obj)
CPUS390XState *env = >env;
cs->env_ptr = env;
-cs->halted = 1;
+cpu_halted_set(cs, 1);
cs->exception_index = EXCP_HLT;
object_property_add(obj, "crash-information", "GuestPanicInformation",
s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
@@ -310,8 +310,8 @@ static void s390_cpu_finalize(Object *obj)
#if !defined(CONFIG_USER_ONLY)
static bool disabled_wait(CPUState *cpu)
{
-return cpu->halted && !(S390_CPU(cpu)->env.psw.mask &
-(PSW_MASK_IO | PSW_MASK_EXT | PSW_MASK_MCHECK));
+return cpu_halted(cpu) && !(S390_CPU(cpu)->env.psw.mask &
+(PSW_MASK_IO | PSW_MASK_EXT |
PSW_MASK_MCHECK));
}
static unsigned s390_count_running_cpus(void)
@@ -337,10 +337,16 @@ unsigned int s390_cpu_halt(S390CPU *cpu)
CPUState *cs = CPU(cpu);
trace_cpu_halt(cs->cpu_index);
-if (!cs->halted) {
-cs->halted = 1;
+/*
+ * cpu_halted and cpu_halted_set acquire the cpu lock if it
+ * isn't already held, so acquire it first.
+ */
+cpu_mutex_lock(cs);
+if (!cpu_halted(cs)) {
+cpu_halted_set(cs, 1);
cs->exception_index = EXCP_HLT;
}
+cpu_mutex_unlock(cs);
return s390_count_running_cpus();
}
@@ -350,10 +356,12 @@ void s390_cpu_unhalt(S390CPU *cpu)
CPUState *cs = CPU(cpu);
trace_cpu_unhalt(cs->cpu_index);
-if (cs->halted) {
-cs->halted = 0;
+cpu_mutex_lock(cs);
+if (cpu_halted(cs)) {
+cpu_halted_set(cs, 0);
cs->exception_index = -1;
}
+cpu_mutex_unlock(cs);
}
unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index a758649f47..633d59346b 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -461,7 +461,7 @@ try_deliver:
if ((env->psw.mask & PSW_MASK_WAIT) || stopped) {
/* don't trigger a cpu_loop_exit(), use an interrupt instead */
cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HALT);
-} else if (cs->halted) {
+} else if (cpu_halted(cs)) {
/* unhalt if we had a WAIT PSW somehwere in our injection chain */
s390_cpu_unhalt(cpu);
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 19530fb94e..433a7e4573 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1003,7 +1003,7 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run
*run)
int kvm_arch_process_async_events(CPUState *cs)
{
-return cs->halted;
+return cpu_halted(cs);
}
static int s390_kvm_irq_to_interrupt(struct kvm_s390_irq *irq,
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index c1f9245797..d410da797a 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -115,7 +115,7 @@ static void sigp_stop(CPUState *cs, run_on_cpu_data arg)
}
/* disabled wait - sleeping in user space */
-if (cs->halted) {
+if (cpu_halted(cs)) {
s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
} else {
/* execute the stop function */
@@ -131,7 +131,7 @@ static void sigp_stop_and_store_status(CPUState *cs,
run_on_cpu_data arg)
SigpInfo *si = arg.host_ptr;
/* disabled wait - sleeping in user space */
-if (s390_cpu_get_state(cpu) == S390_CPU_STATE_OPERATING && cs->halted) {
+if (s390_cpu_get_state(cpu) == S390_CPU_STATE_OPERATING && cpu_halted(cs))
{
s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
}
@@ -313,7 +313,7 @@ static void sigp_cond_emergency(S390CPU *src_cpu, S390CPU
*dst_cpu,
}
/* this looks racy, but these values a
[Qemu-devel] [PATCH v7 61/73] cpu: call .cpu_has_work with the CPU lock held
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
include/qom/cpu.h | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index c92a8c544a..651e1ab4d8 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -85,7 +85,8 @@ struct TranslationBlock;
* @parse_features: Callback to parse command line arguments.
* @reset: Callback to reset the #CPUState to its initial state.
* @reset_dump_flags: #CPUDumpFlags to use for reset logging.
- * @has_work: Callback for checking if there is work to do.
+ * @has_work: Callback for checking if there is work to do. Called with the
+ * CPU lock held.
* @do_interrupt: Callback for interrupt handling.
* @do_unassigned_access: Callback for unassigned access handling.
* (this is deprecated: new targets should use do_transaction_failed instead)
@@ -808,9 +809,16 @@ const char *parse_cpu_model(const char *cpu_model);
static inline bool cpu_has_work(CPUState *cpu)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
+bool ret;
g_assert(cc->has_work);
-return cc->has_work(cpu);
+if (cpu_mutex_locked(cpu)) {
+return cc->has_work(cpu);
+}
+cpu_mutex_lock(cpu);
+ret = cc->has_work(cpu);
+cpu_mutex_unlock(cpu);
+return ret;
}
/**
--
2.17.1
[Qemu-devel] [PATCH v7 30/73] cpu-exec: convert to cpu_halted
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
accel/tcg/cpu-exec.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 45ef41ebb2..78dca325ba 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -425,14 +425,21 @@ static inline TranslationBlock *tb_find(CPUState *cpu,
return tb;
}
-static inline bool cpu_handle_halt(CPUState *cpu)
+static inline bool cpu_handle_halt_locked(CPUState *cpu)
{
-if (cpu->halted) {
+g_assert(cpu_mutex_locked(cpu));
+
+if (cpu_halted(cpu)) {
#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
if ((cpu->interrupt_request & CPU_INTERRUPT_POLL)
&& replay_interrupt()) {
X86CPU *x86_cpu = X86_CPU(cpu);
+
+/* prevent deadlock; cpu_mutex must be acquired _after_ the BQL */
+cpu_mutex_unlock(cpu);
qemu_mutex_lock_iothread();
+cpu_mutex_lock(cpu);
+
apic_poll_irq(x86_cpu->apic_state);
cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
qemu_mutex_unlock_iothread();
@@ -442,12 +449,22 @@ static inline bool cpu_handle_halt(CPUState *cpu)
return true;
}
-cpu->halted = 0;
+cpu_halted_set(cpu, 0);
}
return false;
}
+static inline bool cpu_handle_halt(CPUState *cpu)
+{
+bool ret;
+
+cpu_mutex_lock(cpu);
+ret = cpu_handle_halt_locked(cpu);
+cpu_mutex_unlock(cpu);
+return ret;
+}
+
static inline void cpu_handle_debug_exception(CPUState *cpu)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
@@ -546,7 +563,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
} else if (interrupt_request & CPU_INTERRUPT_HALT) {
replay_interrupt();
cpu->interrupt_request &= ~CPU_INTERRUPT_HALT;
-cpu->halted = 1;
+cpu_halted_set(cpu, 1);
cpu->exception_index = EXCP_HLT;
qemu_mutex_unlock_iothread();
return true;
--
2.17.1
[Qemu-devel] [PATCH v7 17/73] arm: convert to cpu_halted
Cc: Andrzej Zaborowski
Cc: Peter Maydell
Cc: qemu-...@nongnu.org
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
hw/arm/omap1.c| 4 ++--
hw/arm/pxa2xx_gpio.c | 2 +-
hw/arm/pxa2xx_pic.c | 2 +-
target/arm/arm-powerctl.c | 6 +++---
target/arm/cpu.c | 2 +-
target/arm/op_helper.c| 2 +-
6 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index 539d29ef9c..55a7672976 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -1769,7 +1769,7 @@ static uint64_t omap_clkdsp_read(void *opaque, hwaddr
addr,
case 0x18: /* DSP_SYSST */
cpu = CPU(s->cpu);
return (s->clkm.clocking_scheme << 11) | s->clkm.cold_start |
-(cpu->halted << 6); /* Quite useless... */
+(cpu_halted(cpu) << 6); /* Quite useless... */
}
OMAP_BAD_REG(addr);
@@ -3790,7 +3790,7 @@ void omap_mpu_wakeup(void *opaque, int irq, int req)
struct omap_mpu_state_s *mpu = (struct omap_mpu_state_s *) opaque;
CPUState *cpu = CPU(mpu->cpu);
-if (cpu->halted) {
+if (cpu_halted(cpu)) {
cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);
}
}
diff --git a/hw/arm/pxa2xx_gpio.c b/hw/arm/pxa2xx_gpio.c
index e15070188e..5c3fea42e9 100644
--- a/hw/arm/pxa2xx_gpio.c
+++ b/hw/arm/pxa2xx_gpio.c
@@ -128,7 +128,7 @@ static void pxa2xx_gpio_set(void *opaque, int line, int
level)
pxa2xx_gpio_irq_update(s);
/* Wake-up GPIOs */
-if (cpu->halted && (mask & ~s->dir[bank] & pxa2xx_gpio_wake[bank])) {
+if (cpu_halted(cpu) && (mask & ~s->dir[bank] & pxa2xx_gpio_wake[bank])) {
cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);
}
}
diff --git a/hw/arm/pxa2xx_pic.c b/hw/arm/pxa2xx_pic.c
index 61275fa040..46ab4c3fc2 100644
--- a/hw/arm/pxa2xx_pic.c
+++ b/hw/arm/pxa2xx_pic.c
@@ -58,7 +58,7 @@ static void pxa2xx_pic_update(void *opaque)
PXA2xxPICState *s = (PXA2xxPICState *) opaque;
CPUState *cpu = CPU(s->cpu);
-if (cpu->halted) {
+if (cpu_halted(cpu)) {
mask[0] = s->int_pending[0] & (s->int_enabled[0] | s->int_idle);
mask[1] = s->int_pending[1] & (s->int_enabled[1] | s->int_idle);
if (mask[0] || mask[1]) {
diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
index f77a950db6..bd94ac2235 100644
--- a/target/arm/arm-powerctl.c
+++ b/target/arm/arm-powerctl.c
@@ -64,7 +64,7 @@ static void arm_set_cpu_on_async_work(CPUState
*target_cpu_state,
/* Initialize the cpu we are turning on */
cpu_reset(target_cpu_state);
-target_cpu_state->halted = 0;
+cpu_halted_set(target_cpu_state, 0);
if (info->target_aa64) {
if ((info->target_el < 3) && arm_feature(_cpu->env,
@@ -235,7 +235,7 @@ static void arm_set_cpu_on_and_reset_async_work(CPUState
*target_cpu_state,
/* Initialize the cpu we are turning on */
cpu_reset(target_cpu_state);
-target_cpu_state->halted = 0;
+cpu_halted_set(target_cpu_state, 0);
/* Finally set the power status */
assert(qemu_mutex_iothread_locked());
@@ -291,7 +291,7 @@ static void arm_set_cpu_off_async_work(CPUState
*target_cpu_state,
assert(qemu_mutex_iothread_locked());
target_cpu->power_state = PSCI_OFF;
-target_cpu_state->halted = 1;
+cpu_halted_set(target_cpu_state, 1);
target_cpu_state->exception_index = EXCP_HLT;
}
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 54b61f917b..845730c00c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -173,7 +173,7 @@ static void arm_cpu_reset(CPUState *s)
env->vfp.xregs[ARM_VFP_MVFR2] = cpu->isar.mvfr2;
cpu->power_state = cpu->start_powered_off ? PSCI_OFF : PSCI_ON;
-s->halted = cpu->start_powered_off;
+cpu_halted_set(s, cpu->start_powered_off);
if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index c998eadfaa..f9ccdc9abf 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -479,7 +479,7 @@ void HELPER(wfi)(CPUARMState *env, uint32_t insn_len)
}
cs->exception_index = EXCP_HLT;
-cs->halted = 1;
+cpu_halted_set(cs, 1);
cpu_loop_exit(cs);
}
--
2.17.1
[Qemu-devel] [PATCH v7 66/73] riscv: convert to cpu_has_work_with_iothread_lock
Soon we will call cpu_has_work without the BQL. Cc: Palmer Dabbelt Cc: Sagar Karandikar Cc: Bastian Koppelmann Reviewed-by: Palmer Dabbelt Reviewed-by: Richard Henderson Reviewed-by: Alistair Francis Signed-off-by: Emilio G. Cota --- target/riscv/cpu.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index cc3ddc0ae4..a9a0a6728d 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -257,6 +257,9 @@ static bool riscv_cpu_has_work(CPUState *cs) #ifndef CONFIG_USER_ONLY RISCVCPU *cpu = RISCV_CPU(cs); CPURISCVState *env = >env; + +g_assert(qemu_mutex_iothread_locked()); + /* * Definition of the WFI instruction requires it to ignore the privilege * mode and delegation registers, but respect individual enables @@ -343,7 +346,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data) cc->reset = riscv_cpu_reset; cc->class_by_name = riscv_cpu_class_by_name; -cc->has_work = riscv_cpu_has_work; +cc->has_work_with_iothread_lock = riscv_cpu_has_work; cc->do_interrupt = riscv_cpu_do_interrupt; cc->cpu_exec_interrupt = riscv_cpu_exec_interrupt; cc->dump_state = riscv_cpu_dump_state; -- 2.17.1
[Qemu-devel] [PATCH v7 15/73] cpu: define cpu_halted helpers
cpu->halted will soon be protected by cpu->lock.
We will use these helpers to ease the transition,
since right now cpu->halted has many direct callers.
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
include/qom/cpu.h | 24
1 file changed, 24 insertions(+)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index d826a2852e..9fe74e4392 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -502,6 +502,30 @@ bool cpu_mutex_locked(const CPUState *cpu);
*/
bool no_cpu_mutex_locked(void);
+static inline uint32_t cpu_halted(CPUState *cpu)
+{
+uint32_t ret;
+
+if (cpu_mutex_locked(cpu)) {
+return cpu->halted;
+}
+cpu_mutex_lock(cpu);
+ret = cpu->halted;
+cpu_mutex_unlock(cpu);
+return ret;
+}
+
+static inline void cpu_halted_set(CPUState *cpu, uint32_t val)
+{
+if (cpu_mutex_locked(cpu)) {
+cpu->halted = val;
+return;
+}
+cpu_mutex_lock(cpu);
+cpu->halted = val;
+cpu_mutex_unlock(cpu);
+}
+
static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
{
unsigned int i;
--
2.17.1
[Qemu-devel] [PATCH v7 22/73] m68k: convert to cpu_halted
Cc: Laurent Vivier Reviewed-by: Richard Henderson Reviewed-by: Laurent Vivier Reviewed-by: Alex Bennée Signed-off-by: Emilio G. Cota --- target/m68k/op_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c index 76f439985a..9f717dfec1 100644 --- a/target/m68k/op_helper.c +++ b/target/m68k/op_helper.c @@ -237,7 +237,7 @@ static void cf_interrupt_all(CPUM68KState *env, int is_hw) do_m68k_semihosting(env, env->dregs[0]); return; } -cs->halted = 1; +cpu_halted_set(cs, 1); cs->exception_index = EXCP_HLT; cpu_loop_exit(cs); return; -- 2.17.1
