* [Qemu-devel] [PATCH] softmmu: fixing usage of cpu_st/ld* from helpers
@ 2014-09-15 10:50 Pavel Dovgalyuk
2014-09-15 13:55 ` Peter Maydell
2014-09-15 16:28 ` Richard Henderson
0 siblings, 2 replies; 3+ messages in thread
From: Pavel Dovgalyuk @ 2014-09-15 10:50 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, zealot351, maria.klimushenkova, pavel.dovgaluk, batuzovk
MMU helper functions are called from generated code and other helper
functions. In both cases they try to get function's return address for
using it while restoring virtual CPU state.
When MMU helper is called from some other helper function
(like helper_maskmov_xmm) through cpu_st* function, the return address
will point to that helper. That is why CPU state cannot be restored in
the case of MMU fault.
This bug can occur when maskmov instruction is located in the middle of the
translation block.
Execution sequence for this example:
TB start:
PC1: instr1
instr2
PC2: maskmov <page fault>
<page fault processing>
PC1: instr1
instr2
maskmov
At the start of TB execution guest PC points to instr1. When page fault occurs
QEMU tries to restore guest PC (which should be equal to PC2). It reads host PC
from the call stack and checks whether it points to TB or not. Bug in ldst
helpers implementation provides incorrect host PC, which is not located within
the TB. That's why QEMU cannot recover guest PC and it remains the same (PC1).
After page fault processing QEMU restarts TB and executes instr1 and instr2
for the second time, because guest PC was not recovered.
This patch introduces several inline helpers to load return address
which points to the right place. Correct return address allows correct
restoring of the guest PC.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
include/exec/cpu_ldst_template.h | 31 +++++++++++++++++++++++++++----
include/exec/exec-all.h | 27 +++++++++++++++++++++++++++
softmmu_template.h | 18 ++++++++++++++++++
3 files changed, 72 insertions(+), 4 deletions(-)
diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 006093a..2658955 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -61,6 +61,17 @@
#define MMUSUFFIX _mmu
#endif
+/* inline helper ld function */
+
+static inline DATA_TYPE
+glue(glue(helper_inline_ld, SUFFIX), MEMSUFFIX)(CPUArchState *env,
+ target_ulong addr,
+ int mmu_idx)
+{
+ return glue(glue(helper_call_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx,
+ GETRA());
+}
+
/* generic load/store macros */
static inline RES_TYPE
@@ -76,7 +87,8 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
mmu_idx = CPU_MMU_INDEX;
if (unlikely(env->tlb_table[mmu_idx][page_index].ADDR_READ !=
(addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
- res = glue(glue(helper_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx);
+ res = glue(glue(helper_inline_ld, SUFFIX), MEMSUFFIX)(env, addr,
+ mmu_idx);
} else {
uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
res = glue(glue(ld, USUFFIX), _raw)(hostaddr);
@@ -97,8 +109,8 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
mmu_idx = CPU_MMU_INDEX;
if (unlikely(env->tlb_table[mmu_idx][page_index].ADDR_READ !=
(addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
- res = (DATA_STYPE)glue(glue(helper_ld, SUFFIX),
- MMUSUFFIX)(env, addr, mmu_idx);
+ res = (DATA_STYPE)glue(glue(helper_inline_ld, SUFFIX),
+ MEMSUFFIX)(env, addr, mmu_idx);
} else {
uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
res = glue(glue(lds, SUFFIX), _raw)(hostaddr);
@@ -109,6 +121,17 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
#ifndef SOFTMMU_CODE_ACCESS
+/* inline helper st function */
+
+static inline void
+glue(glue(helper_inline_st, SUFFIX), MEMSUFFIX)(CPUArchState *env,
+ target_ulong addr,
+ DATA_TYPE val, int mmu_idx)
+{
+ glue(glue(helper_call_st, SUFFIX), MMUSUFFIX)(env, addr, val, mmu_idx,
+ GETRA());
+}
+
/* generic store macro */
static inline void
@@ -124,7 +147,7 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
mmu_idx = CPU_MMU_INDEX;
if (unlikely(env->tlb_table[mmu_idx][page_index].addr_write !=
(addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
- glue(glue(helper_st, SUFFIX), MMUSUFFIX)(env, addr, v, mmu_idx);
+ glue(glue(helper_inline_st, SUFFIX), MEMSUFFIX)(env, addr, v, mmu_idx);
} else {
uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
glue(glue(st, SUFFIX), _raw)(hostaddr, v);
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 5e5d86e..528928f 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -344,6 +344,33 @@ bool io_mem_write(struct MemoryRegion *mr, hwaddr addr,
void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
uintptr_t retaddr);
+uint8_t helper_call_ldb_cmmu(CPUArchState *env, target_ulong addr,
+ int mmu_idx, uintptr_t retaddr);
+uint16_t helper_call_ldw_cmmu(CPUArchState *env, target_ulong addr,
+ int mmu_idx, uintptr_t retaddr);
+uint32_t helper_call_ldl_cmmu(CPUArchState *env, target_ulong addr,
+ int mmu_idx, uintptr_t retaddr);
+uint64_t helper_call_ldq_cmmu(CPUArchState *env, target_ulong addr,
+ int mmu_idx, uintptr_t retaddr);
+
+uint8_t helper_call_ldb_mmu(CPUArchState *env, target_ulong addr,
+ int mmu_idx, uintptr_t retaddr);
+uint16_t helper_call_ldw_mmu(CPUArchState *env, target_ulong addr,
+ int mmu_idx, uintptr_t retaddr);
+uint32_t helper_call_ldl_mmu(CPUArchState *env, target_ulong addr,
+ int mmu_idx, uintptr_t retaddr);
+uint64_t helper_call_ldq_mmu(CPUArchState *env, target_ulong addr,
+ int mmu_idx, uintptr_t retaddr);
+
+void helper_call_stb_mmu(CPUArchState *env, target_ulong addr,
+ uint8_t val, int mmu_idx, uintptr_t retaddr);
+void helper_call_stw_mmu(CPUArchState *env, target_ulong addr,
+ uint16_t val, int mmu_idx, uintptr_t retaddr);
+void helper_call_stl_mmu(CPUArchState *env, target_ulong addr,
+ uint32_t val, int mmu_idx, uintptr_t retaddr);
+void helper_call_stq_mmu(CPUArchState *env, target_ulong addr,
+ uint64_t val, int mmu_idx, uintptr_t retaddr);
+
#endif
#if defined(CONFIG_USER_ONLY)
diff --git a/softmmu_template.h b/softmmu_template.h
index 5a07f99..1053cf3 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -311,6 +311,15 @@ glue(glue(helper_ld, SUFFIX), MMUSUFFIX)(CPUArchState *env, target_ulong addr,
return helper_te_ld_name (env, addr, mmu_idx, GETRA());
}
+DATA_TYPE
+glue(glue(helper_call_ld, SUFFIX), MMUSUFFIX)(CPUArchState *env,
+ target_ulong addr,
+ int mmu_idx,
+ uintptr_t retaddr)
+{
+ return helper_te_ld_name(env, addr, mmu_idx, retaddr);
+}
+
#ifndef SOFTMMU_CODE_ACCESS
/* Provide signed versions of the load routines as well. We can of course
@@ -505,6 +514,15 @@ glue(glue(helper_st, SUFFIX), MMUSUFFIX)(CPUArchState *env, target_ulong addr,
helper_te_st_name(env, addr, val, mmu_idx, GETRA());
}
+void
+glue(glue(helper_call_st, SUFFIX), MMUSUFFIX)(CPUArchState *env,
+ target_ulong addr,
+ DATA_TYPE val, int mmu_idx,
+ uintptr_t retaddr)
+{
+ helper_te_st_name(env, addr, val, mmu_idx, retaddr);
+}
+
#endif /* !defined(SOFTMMU_CODE_ACCESS) */
#undef READ_ACCESS_TYPE
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] softmmu: fixing usage of cpu_st/ld* from helpers
2014-09-15 10:50 [Qemu-devel] [PATCH] softmmu: fixing usage of cpu_st/ld* from helpers Pavel Dovgalyuk
@ 2014-09-15 13:55 ` Peter Maydell
2014-09-15 16:28 ` Richard Henderson
1 sibling, 0 replies; 3+ messages in thread
From: Peter Maydell @ 2014-09-15 13:55 UTC (permalink / raw)
To: Pavel Dovgalyuk
Cc: QEMU Developers, Kirill Batuzov, maria.klimushenkova,
Paolo Bonzini,
Денис
Дмитриев,
Richard Henderson
CCing RTH who was the last person to do something with
this area of the code I think. I thought the correct answer to this
problem was "don't use the cpu_st* functions but use something
else you can pass GETRA() to" ?
On 15 September 2014 03:50, Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> wrote:
> MMU helper functions are called from generated code and other helper
> functions. In both cases they try to get function's return address for
> using it while restoring virtual CPU state.
>
> When MMU helper is called from some other helper function
> (like helper_maskmov_xmm) through cpu_st* function, the return address
> will point to that helper. That is why CPU state cannot be restored in
> the case of MMU fault.
>
> This bug can occur when maskmov instruction is located in the middle of the
> translation block.
>
> Execution sequence for this example:
>
> TB start:
> PC1: instr1
> instr2
> PC2: maskmov <page fault>
> <page fault processing>
> PC1: instr1
> instr2
> maskmov
>
> At the start of TB execution guest PC points to instr1. When page fault occurs
> QEMU tries to restore guest PC (which should be equal to PC2). It reads host PC
> from the call stack and checks whether it points to TB or not. Bug in ldst
> helpers implementation provides incorrect host PC, which is not located within
> the TB. That's why QEMU cannot recover guest PC and it remains the same (PC1).
> After page fault processing QEMU restarts TB and executes instr1 and instr2
> for the second time, because guest PC was not recovered.
>
> This patch introduces several inline helpers to load return address
> which points to the right place. Correct return address allows correct
> restoring of the guest PC.
thanks
-- PMM
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] softmmu: fixing usage of cpu_st/ld* from helpers
2014-09-15 10:50 [Qemu-devel] [PATCH] softmmu: fixing usage of cpu_st/ld* from helpers Pavel Dovgalyuk
2014-09-15 13:55 ` Peter Maydell
@ 2014-09-15 16:28 ` Richard Henderson
1 sibling, 0 replies; 3+ messages in thread
From: Richard Henderson @ 2014-09-15 16:28 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel
Cc: pbonzini, zealot351, maria.klimushenkova, batuzovk
On 09/15/2014 03:50 AM, Pavel Dovgalyuk wrote:
> +/* inline helper ld function */
> +
> +static inline DATA_TYPE
> +glue(glue(helper_inline_ld, SUFFIX), MEMSUFFIX)(CPUArchState *env,
> + target_ulong addr,
> + int mmu_idx)
> +{
> + return glue(glue(helper_call_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx,
> + GETRA());
> +}
You'd have to mark this always_inline to make absolutely sure that the caller's
GETRA value is used. That said...
> @@ -76,7 +87,8 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
> mmu_idx = CPU_MMU_INDEX;
> if (unlikely(env->tlb_table[mmu_idx][page_index].ADDR_READ !=
> (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
> - res = glue(glue(helper_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx);
> + res = glue(glue(helper_inline_ld, SUFFIX), MEMSUFFIX)(env, addr,
> + mmu_idx);
> } else {
> uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
> res = glue(glue(ld, USUFFIX), _raw)(hostaddr);
... this is also the wrong context.
The only GETRA value that helps you at all is the one from the *top level*
helper -- the one that's directly called from TCG code. So, in the case of
maskmov, helper_maskmov_xmm. Anything else and you aren't getting the call
site address from the TCG code, and so can't be used to detect the PC of the
MMU fault.
I guess there are only two real possibilities:
(1) Have the cpu_ldst_template helpers all be marked always_inline so that they
could use GETRA. I'm not too fond of this because we'd still get the wrong
results if these are not used from top-level helpers.
(2) Add helpers that accept the GETRA value from the top-level helper. And not
hidden within a macro or always_inline function. This helps us see what
portions of the code have been audited for the new interface. This will
involve quite a bit more code churn, but shouldn't been too difficult for any
single function.
r~
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-09-15 16:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-15 10:50 [Qemu-devel] [PATCH] softmmu: fixing usage of cpu_st/ld* from helpers Pavel Dovgalyuk
2014-09-15 13:55 ` Peter Maydell
2014-09-15 16:28 ` Richard Henderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.