From: Richard Henderson <richard.henderson@linaro.org>
To: qemu-devel@nongnu.org
Subject: [PATCH] accel/tcg: Probe the proper permissions for atomic ops
Date: Sat, 12 Jun 2021 17:25:05 -0700 [thread overview]
Message-ID: <20210613002505.898859-1-richard.henderson@linaro.org> (raw)
We had a single ATOMIC_MMU_LOOKUP macro that probed for
read+write on all atomic ops. This is incorrect for
plain atomic load and atomic store.
For user-only, we rely on the host page permissions.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/390
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/atomic_template.h | 24 +++++-----
accel/tcg/cputlb.c | 89 ++++++++++++++++++++++++++-----------
accel/tcg/user-exec.c | 8 ++--
3 files changed, 80 insertions(+), 41 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index 0ff7f913e1..afa8a9daf3 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -74,7 +74,7 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
{
ATOMIC_MMU_DECLS;
- DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
+ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
ATOMIC_MMU_IDX);
@@ -95,7 +95,7 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
{
ATOMIC_MMU_DECLS;
- DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP;
+ DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP_R;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
ATOMIC_MMU_IDX);
@@ -110,7 +110,7 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
ABI_TYPE val EXTRA_ARGS)
{
ATOMIC_MMU_DECLS;
- DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
+ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_W;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, true,
ATOMIC_MMU_IDX);
@@ -125,7 +125,7 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE val EXTRA_ARGS)
{
ATOMIC_MMU_DECLS;
- DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
+ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false,
ATOMIC_MMU_IDX);
@@ -142,7 +142,7 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE val EXTRA_ARGS) \
{ \
ATOMIC_MMU_DECLS; \
- DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \
+ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW; \
DATA_TYPE ret; \
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false, \
ATOMIC_MMU_IDX); \
@@ -176,7 +176,7 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE xval EXTRA_ARGS) \
{ \
ATOMIC_MMU_DECLS; \
- XDATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \
+ XDATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW; \
XDATA_TYPE cmp, old, new, val = xval; \
uint16_t info = trace_mem_build_info(SHIFT, false, 0, false, \
ATOMIC_MMU_IDX); \
@@ -221,7 +221,7 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
{
ATOMIC_MMU_DECLS;
- DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
+ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
DATA_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, false,
ATOMIC_MMU_IDX);
@@ -242,7 +242,7 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
{
ATOMIC_MMU_DECLS;
- DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP;
+ DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP_R;
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, false,
ATOMIC_MMU_IDX);
@@ -257,7 +257,7 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
ABI_TYPE val EXTRA_ARGS)
{
ATOMIC_MMU_DECLS;
- DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
+ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_W;
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, true,
ATOMIC_MMU_IDX);
@@ -274,7 +274,7 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE val EXTRA_ARGS)
{
ATOMIC_MMU_DECLS;
- DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
+ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW;
ABI_TYPE ret;
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, false,
ATOMIC_MMU_IDX);
@@ -291,7 +291,7 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE val EXTRA_ARGS) \
{ \
ATOMIC_MMU_DECLS; \
- DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \
+ DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW; \
DATA_TYPE ret; \
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, \
false, ATOMIC_MMU_IDX); \
@@ -323,7 +323,7 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE xval EXTRA_ARGS) \
{ \
ATOMIC_MMU_DECLS; \
- XDATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \
+ XDATA_TYPE *haddr = ATOMIC_MMU_LOOKUP_RW; \
XDATA_TYPE ldo, ldn, old, new, val = xval; \
uint16_t info = trace_mem_build_info(SHIFT, false, MO_BSWAP, \
false, ATOMIC_MMU_IDX); \
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f24348e979..a6d893fb25 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1742,18 +1742,22 @@ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
#endif
-/* Probe for a read-modify-write atomic operation. Do not allow unaligned
- * operations, or io operations to proceed. Return the host address. */
+/*
+ * Probe for an atomic operation. Do not allow unaligned operations,
+ * or io operations to proceed. Return the host address.
+ *
+ * @prot may be PAGE_READ, PAGE_WRITE, or PAGE_READ|PAGE_WRITE.
+ */
static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
- TCGMemOpIdx oi, uintptr_t retaddr)
+ TCGMemOpIdx oi, int prot, uintptr_t retaddr)
{
size_t mmu_idx = get_mmuidx(oi);
- uintptr_t index = tlb_index(env, mmu_idx, addr);
- CPUTLBEntry *tlbe = tlb_entry(env, mmu_idx, addr);
- target_ulong tlb_addr = tlb_addr_write(tlbe);
MemOp mop = get_memop(oi);
int a_bits = get_alignment_bits(mop);
int s_bits = mop & MO_SIZE;
+ uintptr_t index;
+ CPUTLBEntry *tlbe;
+ target_ulong tlb_addr;
void *hostaddr;
/* Adjust the given return address. */
@@ -1775,15 +1779,45 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
goto stop_the_world;
}
+ index = tlb_index(env, mmu_idx, addr);
+ tlbe = tlb_entry(env, mmu_idx, addr);
+
/* Check TLB entry and enforce page permissions. */
- if (!tlb_hit(tlb_addr, addr)) {
- if (!VICTIM_TLB_HIT(addr_write, addr)) {
- tlb_fill(env_cpu(env), addr, 1 << s_bits, MMU_DATA_STORE,
- mmu_idx, retaddr);
- index = tlb_index(env, mmu_idx, addr);
- tlbe = tlb_entry(env, mmu_idx, addr);
+ if (prot & PAGE_WRITE) {
+ tlb_addr = tlb_addr_write(tlbe);
+ if (!tlb_hit(tlb_addr, addr)) {
+ if (!VICTIM_TLB_HIT(addr_write, addr)) {
+ tlb_fill(env_cpu(env), addr, 1 << s_bits,
+ MMU_DATA_STORE, mmu_idx, retaddr);
+ index = tlb_index(env, mmu_idx, addr);
+ tlbe = tlb_entry(env, mmu_idx, addr);
+ }
+ tlb_addr = tlb_addr_write(tlbe) & ~TLB_INVALID_MASK;
+ }
+
+ /* Let the guest notice RMW on a write-only page. */
+ if ((prot & PAGE_READ) &&
+ unlikely(tlbe->addr_read != (tlb_addr & ~TLB_NOTDIRTY))) {
+ tlb_fill(env_cpu(env), addr, 1 << s_bits,
+ MMU_DATA_LOAD, mmu_idx, retaddr);
+ /*
+ * Since we don't support reads and writes to different addresses,
+ * and we do have the proper page loaded for write, this shouldn't
+ * ever return. But just in case, handle via stop-the-world.
+ */
+ goto stop_the_world;
+ }
+ } else /* if (prot & PAGE_READ) */ {
+ tlb_addr = tlbe->addr_read;
+ if (!tlb_hit(tlb_addr, addr)) {
+ if (!VICTIM_TLB_HIT(addr_write, addr)) {
+ tlb_fill(env_cpu(env), addr, 1 << s_bits,
+ MMU_DATA_LOAD, mmu_idx, retaddr);
+ index = tlb_index(env, mmu_idx, addr);
+ tlbe = tlb_entry(env, mmu_idx, addr);
+ }
+ tlb_addr = tlbe->addr_read & ~TLB_INVALID_MASK;
}
- tlb_addr = tlb_addr_write(tlbe) & ~TLB_INVALID_MASK;
}
/* Notice an IO access or a needs-MMU-lookup access */
@@ -1793,16 +1827,6 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
goto stop_the_world;
}
- /* Let the guest notice RMW on a write-only page. */
- if (unlikely(tlbe->addr_read != (tlb_addr & ~TLB_NOTDIRTY))) {
- tlb_fill(env_cpu(env), addr, 1 << s_bits, MMU_DATA_LOAD,
- mmu_idx, retaddr);
- /* Since we don't support reads and writes to different addresses,
- and we do have the proper page loaded for write, this shouldn't
- ever return. But just in case, handle via stop-the-world. */
- goto stop_the_world;
- }
-
hostaddr = (void *)((uintptr_t)addr + tlbe->addend);
if (unlikely(tlb_addr & TLB_NOTDIRTY)) {
@@ -2669,7 +2693,12 @@ void cpu_stq_le_data(CPUArchState *env, target_ulong ptr, uint64_t val)
#define ATOMIC_NAME(X) \
HELPER(glue(glue(glue(atomic_ ## X, SUFFIX), END), _mmu))
#define ATOMIC_MMU_DECLS
-#define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, oi, retaddr)
+#define ATOMIC_MMU_LOOKUP_RW \
+ atomic_mmu_lookup(env, addr, oi, PAGE_READ | PAGE_WRITE, retaddr)
+#define ATOMIC_MMU_LOOKUP_R \
+ atomic_mmu_lookup(env, addr, oi, PAGE_READ, retaddr)
+#define ATOMIC_MMU_LOOKUP_W \
+ atomic_mmu_lookup(env, addr, oi, PAGE_WRITE, retaddr)
#define ATOMIC_MMU_CLEANUP
#define ATOMIC_MMU_IDX get_mmuidx(oi)
@@ -2698,10 +2727,18 @@ void cpu_stq_le_data(CPUArchState *env, target_ulong ptr, uint64_t val)
#undef EXTRA_ARGS
#undef ATOMIC_NAME
-#undef ATOMIC_MMU_LOOKUP
+#undef ATOMIC_MMU_LOOKUP_RW
+#undef ATOMIC_MMU_LOOKUP_R
+#undef ATOMIC_MMU_LOOKUP_W
+
#define EXTRA_ARGS , TCGMemOpIdx oi
#define ATOMIC_NAME(X) HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
-#define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, oi, GETPC())
+#define ATOMIC_MMU_LOOKUP_RW \
+ atomic_mmu_lookup(env, addr, oi, PAGE_READ | PAGE_WRITE, GETPC())
+#define ATOMIC_MMU_LOOKUP_R \
+ atomic_mmu_lookup(env, addr, oi, PAGE_READ, GETPC())
+#define ATOMIC_MMU_LOOKUP_W \
+ atomic_mmu_lookup(env, addr, oi, PAGE_WRITE, GETPC())
#define DATA_SIZE 1
#include "atomic_template.h"
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index fb2d43e6a9..e67b1617b5 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -1220,7 +1220,9 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
/* Macro to call the above, with local variables from the use context. */
#define ATOMIC_MMU_DECLS do {} while (0)
-#define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
+#define ATOMIC_MMU_LOOKUP_RW atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
+#define ATOMIC_MMU_LOOKUP_R ATOMIC_MMU_LOOKUP_RW
+#define ATOMIC_MMU_LOOKUP_W ATOMIC_MMU_LOOKUP_RW
#define ATOMIC_MMU_CLEANUP do { clear_helper_retaddr(); } while (0)
#define ATOMIC_MMU_IDX MMU_USER_IDX
@@ -1250,12 +1252,12 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
#undef EXTRA_ARGS
#undef ATOMIC_NAME
-#undef ATOMIC_MMU_LOOKUP
+#undef ATOMIC_MMU_LOOKUP_RW
#define EXTRA_ARGS , TCGMemOpIdx oi, uintptr_t retaddr
#define ATOMIC_NAME(X) \
HELPER(glue(glue(glue(atomic_ ## X, SUFFIX), END), _mmu))
-#define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, DATA_SIZE, retaddr)
+#define ATOMIC_MMU_LOOKUP_RW atomic_mmu_lookup(env, addr, DATA_SIZE, retaddr)
#define DATA_SIZE 16
#include "atomic_template.h"
--
2.25.1
next reply other threads:[~2021-06-13 0:26 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-13 0:25 Richard Henderson [this message]
2021-06-14 10:18 ` [PATCH] accel/tcg: Probe the proper permissions for atomic ops Alex Bennée
2021-06-14 14:49 ` Richard Henderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210613002505.898859-1-richard.henderson@linaro.org \
--to=richard.henderson@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.