Re: [PATCH 2/2] accel/tcg: Always lock pages before translation
On 7/6/23 18:05, Richard Henderson wrote:
+++ b/accel/tcg/cpu-exec.c
@@ -531,6 +531,10 @@ static void cpu_exec_longjmp_cleanup(CPUState *cpu)
/* Non-buggy compilers preserve this; assert the correct value. */
g_assert(cpu == current_cpu);
+if (tcg_ctx->gen_tb) {
+tb_unlock_pages(tcg_ctx->gen_tb);
+tcg_ctx->gen_tb = NULL;
+}
Ho hum, fails for user-only, since there is one tcg_ctx and this cpu didn't necessarily
own it. Just a slight tweak needed, I'm sure.
r~
Re: [PATCH 2/2] accel/tcg: Always lock pages before translation
On Thu, Jul 06, 2023 at 06:05:37PM +0100, Richard Henderson wrote:
> We had done this for user-mode by invoking page_protect
> within the translator loop. Extend this to handle system
> mode as well. Move page locking out of tb_link_page.
>
> Reported-by: Liren Wei
> Reported-by: Richard W.M. Jones
> Signed-off-by: Richard Henderson
Tested-by: Richard W.M. Jones
I tested it across two machines, total of 10,000 iterations successfully.
Great fix, thanks.
Rich.
> accel/tcg/internal.h | 30 -
> accel/tcg/cpu-exec.c | 4 +
> accel/tcg/tb-maint.c | 242 --
> accel/tcg/translate-all.c | 43 ++-
> accel/tcg/translator.c| 34 --
> 5 files changed, 220 insertions(+), 133 deletions(-)
>
> diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
> index 650c3ac53f..e8cbbde581 100644
> --- a/accel/tcg/internal.h
> +++ b/accel/tcg/internal.h
> @@ -10,6 +10,7 @@
> #define ACCEL_TCG_INTERNAL_H
>
> #include "exec/exec-all.h"
> +#include "exec/translate-all.h"
>
> /*
> * Access to the various translations structures need to be serialised
> @@ -35,6 +36,32 @@ static inline void page_table_config_init(void) { }
> void page_table_config_init(void);
> #endif
>
> +#ifdef CONFIG_USER_ONLY
> +/*
> + * For user-only, page_protect sets the page read-only.
> + * Since most execution is already on read-only pages, and we'd need to
> + * account for other TBs on the same page, defer undoing any page protection
> + * until we receive the write fault.
> + */
> +static inline void tb_lock_page0(tb_page_addr_t p0)
> +{
> +page_protect(p0);
> +}
> +
> +static inline void tb_lock_page1(tb_page_addr_t p0, tb_page_addr_t p1)
> +{
> +page_protect(p1);
> +}
> +
> +static inline void tb_unlock_page1(tb_page_addr_t p0, tb_page_addr_t p1) { }
> +static inline void tb_unlock_pages(TranslationBlock *tb) { }
> +#else
> +void tb_lock_page0(tb_page_addr_t);
> +void tb_lock_page1(tb_page_addr_t, tb_page_addr_t);
> +void tb_unlock_page1(tb_page_addr_t, tb_page_addr_t);
> +void tb_unlock_pages(TranslationBlock *);
> +#endif
> +
> #ifdef CONFIG_SOFTMMU
> void tb_invalidate_phys_range_fast(ram_addr_t ram_addr,
> unsigned size,
> @@ -48,8 +75,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, vaddr pc,
> void page_init(void);
> void tb_htable_init(void);
> void tb_reset_jump(TranslationBlock *tb, int n);
> -TranslationBlock *tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> - tb_page_addr_t phys_page2);
> +TranslationBlock *tb_link_page(TranslationBlock *tb);
> bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
> void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
> uintptr_t host_pc);
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 31aa320513..4e9dc0b3ba 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -531,6 +531,10 @@ static void cpu_exec_longjmp_cleanup(CPUState *cpu)
> /* Non-buggy compilers preserve this; assert the correct value. */
> g_assert(cpu == current_cpu);
>
> +if (tcg_ctx->gen_tb) {
> +tb_unlock_pages(tcg_ctx->gen_tb);
> +tcg_ctx->gen_tb = NULL;
> +}
> #ifdef CONFIG_USER_ONLY
> clear_helper_retaddr();
> if (have_mmap_lock()) {
> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
> index 9566224d18..c406b2f7b7 100644
> --- a/accel/tcg/tb-maint.c
> +++ b/accel/tcg/tb-maint.c
> @@ -70,17 +70,7 @@ typedef struct PageDesc PageDesc;
> */
> #define assert_page_locked(pd) tcg_debug_assert(have_mmap_lock())
>
> -static inline void page_lock_pair(PageDesc **ret_p1, tb_page_addr_t phys1,
> - PageDesc **ret_p2, tb_page_addr_t phys2,
> - bool alloc)
> -{
> -*ret_p1 = NULL;
> -*ret_p2 = NULL;
> -}
> -
> -static inline void page_unlock(PageDesc *pd) { }
> -static inline void page_lock_tb(const TranslationBlock *tb) { }
> -static inline void page_unlock_tb(const TranslationBlock *tb) { }
> +static inline void tb_lock_pages(const TranslationBlock *tb) { }
>
> /*
> * For user-only, since we are protecting all of memory with a single lock,
> @@ -96,7 +86,7 @@ static void tb_remove_all(void)
> }
>
> /* Call with mmap_lock held. */
> -static void tb_record(TranslationBlock *tb, PageDesc *p1, PageDesc *p2)
> +static void tb_record(TranslationBlock *tb)
> {
> vaddr addr;
> int flags;
> @@ -391,12 +381,108 @@ static void page_lock(PageDesc *pd)
> qemu_spin_lock(>lock);
> }
>
> +/* Like qemu_spin_trylock, returns false on success */
> +static bool page_trylock(PageDesc *pd)
> +{
> +bool busy = qemu_spin_trylock(>lock);
> +if (!busy) {
> +page_lock__debug(pd);
> +}
> +return busy;
> +}
> +
> static void page_unlock(PageDesc *pd)
> {
> qemu_spin_unlock(>lock);
>
[PATCH 2/2] accel/tcg: Always lock pages before translation
We had done this for user-mode by invoking page_protect
within the translator loop. Extend this to handle system
mode as well. Move page locking out of tb_link_page.
Reported-by: Liren Wei
Reported-by: Richard W.M. Jones
Signed-off-by: Richard Henderson
---
accel/tcg/internal.h | 30 -
accel/tcg/cpu-exec.c | 4 +
accel/tcg/tb-maint.c | 242 --
accel/tcg/translate-all.c | 43 ++-
accel/tcg/translator.c| 34 --
5 files changed, 220 insertions(+), 133 deletions(-)
diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
index 650c3ac53f..e8cbbde581 100644
--- a/accel/tcg/internal.h
+++ b/accel/tcg/internal.h
@@ -10,6 +10,7 @@
#define ACCEL_TCG_INTERNAL_H
#include "exec/exec-all.h"
+#include "exec/translate-all.h"
/*
* Access to the various translations structures need to be serialised
@@ -35,6 +36,32 @@ static inline void page_table_config_init(void) { }
void page_table_config_init(void);
#endif
+#ifdef CONFIG_USER_ONLY
+/*
+ * For user-only, page_protect sets the page read-only.
+ * Since most execution is already on read-only pages, and we'd need to
+ * account for other TBs on the same page, defer undoing any page protection
+ * until we receive the write fault.
+ */
+static inline void tb_lock_page0(tb_page_addr_t p0)
+{
+page_protect(p0);
+}
+
+static inline void tb_lock_page1(tb_page_addr_t p0, tb_page_addr_t p1)
+{
+page_protect(p1);
+}
+
+static inline void tb_unlock_page1(tb_page_addr_t p0, tb_page_addr_t p1) { }
+static inline void tb_unlock_pages(TranslationBlock *tb) { }
+#else
+void tb_lock_page0(tb_page_addr_t);
+void tb_lock_page1(tb_page_addr_t, tb_page_addr_t);
+void tb_unlock_page1(tb_page_addr_t, tb_page_addr_t);
+void tb_unlock_pages(TranslationBlock *);
+#endif
+
#ifdef CONFIG_SOFTMMU
void tb_invalidate_phys_range_fast(ram_addr_t ram_addr,
unsigned size,
@@ -48,8 +75,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, vaddr pc,
void page_init(void);
void tb_htable_init(void);
void tb_reset_jump(TranslationBlock *tb, int n);
-TranslationBlock *tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
- tb_page_addr_t phys_page2);
+TranslationBlock *tb_link_page(TranslationBlock *tb);
bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
uintptr_t host_pc);
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 31aa320513..4e9dc0b3ba 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -531,6 +531,10 @@ static void cpu_exec_longjmp_cleanup(CPUState *cpu)
/* Non-buggy compilers preserve this; assert the correct value. */
g_assert(cpu == current_cpu);
+if (tcg_ctx->gen_tb) {
+tb_unlock_pages(tcg_ctx->gen_tb);
+tcg_ctx->gen_tb = NULL;
+}
#ifdef CONFIG_USER_ONLY
clear_helper_retaddr();
if (have_mmap_lock()) {
diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index 9566224d18..c406b2f7b7 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -70,17 +70,7 @@ typedef struct PageDesc PageDesc;
*/
#define assert_page_locked(pd) tcg_debug_assert(have_mmap_lock())
-static inline void page_lock_pair(PageDesc **ret_p1, tb_page_addr_t phys1,
- PageDesc **ret_p2, tb_page_addr_t phys2,
- bool alloc)
-{
-*ret_p1 = NULL;
-*ret_p2 = NULL;
-}
-
-static inline void page_unlock(PageDesc *pd) { }
-static inline void page_lock_tb(const TranslationBlock *tb) { }
-static inline void page_unlock_tb(const TranslationBlock *tb) { }
+static inline void tb_lock_pages(const TranslationBlock *tb) { }
/*
* For user-only, since we are protecting all of memory with a single lock,
@@ -96,7 +86,7 @@ static void tb_remove_all(void)
}
/* Call with mmap_lock held. */
-static void tb_record(TranslationBlock *tb, PageDesc *p1, PageDesc *p2)
+static void tb_record(TranslationBlock *tb)
{
vaddr addr;
int flags;
@@ -391,12 +381,108 @@ static void page_lock(PageDesc *pd)
qemu_spin_lock(>lock);
}
+/* Like qemu_spin_trylock, returns false on success */
+static bool page_trylock(PageDesc *pd)
+{
+bool busy = qemu_spin_trylock(>lock);
+if (!busy) {
+page_lock__debug(pd);
+}
+return busy;
+}
+
static void page_unlock(PageDesc *pd)
{
qemu_spin_unlock(>lock);
page_unlock__debug(pd);
}
+void tb_lock_page0(tb_page_addr_t paddr)
+{
+page_lock(page_find_alloc(paddr >> TARGET_PAGE_BITS, true));
+}
+
+void tb_lock_page1(tb_page_addr_t paddr0, tb_page_addr_t paddr1)
+{
+tb_page_addr_t pindex0 = paddr0 >> TARGET_PAGE_BITS;
+tb_page_addr_t pindex1 = paddr1 >> TARGET_PAGE_BITS;
+PageDesc *pd0, *pd1;
+
+if (pindex0 == pindex1) {
+/* Identical pages, and the first page is already locked. */
+return;
+}
+
+
