Hi Michael,
On 11/16/24 19:49, Michael Tokarev wrote:
25.10.2024 20:58, Pierrick Bouvier wrote:
When instrumenting memory accesses for plugin, we force memory accesses
to use the slow path for mmu [1]. This create a situation where we end
up calling ptw_setl_slow. This was fixed recently in [2] but the issue
still could appear out of plugins use case.
Since this function gets called during a cpu_exec, start_exclusive then
hangs. This exclusive section was introduced initially for security
reasons [3].
I suspect this code path was never triggered, because ptw_setl_slow
would always be called transitively from cpu_exec, resulting in a hang.
[1]
https://gitlab.com/qemu-project/qemu/-/commit/6d03226b42247b68ab2f0b3663e0f624335a4055
[2]
https://gitlab.com/qemu-project/qemu/-/commit/115ade42d50144c15b74368d32dc734ea277d853
[3] https://gitlab.com/qemu-project/qemu/-/issues/279
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2566
Signed-off-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>
[1] is in 8.2.x. [2] is in 9.2.tobe, and marked as should be picked up
for stable (I picked it up for 8.2.x, 9.0.x and 9.1.x).
Shouldn't this one be picked up for stable too, as an addition fix
ontop of [2]? Or is it not important? (I guess since it's reported
in our issue tracker, it is problematic for someone already).
I picked this one up for 8.2, 9.0 and 9.1 stable series -- please
let me know if I should not.
You can safely integrate this on all stable branches. It will fix the
issue with plugins, and might possibly fix an issue with the memory
access if someone hit that path. The original problematic change was
missing this fix from the start.
Also, what about the 2/2 in this series, "cpu: ensure we don't call
start_exclusive from cpu_exec", which is
779f30a01af8566780cefc8639505b758950afb3
in master now?
It's an assert to ensure we don't hit the same blocking situation in the
future (with new changes), so I don't think there is an added value to
integrate that on stable branches.
Thanks,
Pierrick
Thanks,
/mjt
target/i386/tcg/sysemu/excp_helper.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/target/i386/tcg/sysemu/excp_helper.c
b/target/i386/tcg/sysemu/excp_helper.c
index da187c8792a..ddc51e3e0b8 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -107,6 +107,10 @@ static bool ptw_setl_slow(const PTETranslate *in, uint32_t
old, uint32_t new)
{
uint32_t cmp;
+ CPUState *cpu = env_cpu(in->env);
+ /* We are in cpu_exec, and start_exclusive can't be called directly.*/
+ g_assert(cpu->running);
+ cpu_exec_end(cpu);
/* Does x86 really perform a rmw cycle on mmio for ptw? */
start_exclusive();
cmp = cpu_ldl_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
@@ -114,6 +118,7 @@ static bool ptw_setl_slow(const PTETranslate *in, uint32_t
old, uint32_t new)
cpu_stl_mmuidx_ra(in->env, in->gaddr, new, in->ptw_idx, 0);
}
end_exclusive();
+ cpu_exec_start(cpu);
return cmp == old;
}
