[PATCH for-8.0 0/3] last minute tcg fixes

[PATCH for-8.0 0/3] last minute tcg fixes

[Richard Henderson]

Fix a bug just exposed concerning qemu-arm commpage, leading to an
immediate crash on any 64k page host.  Fix two bugs regressing
pc-relative tb generation, found by Weiwei Li.


r~


Richard Henderson (2):
  Revert "linux-user/arm: Take more care allocating commpage"
  accel/tcg: Fix jump cache set in cpu_exec_loop

Weiwei Li (1):
  accel/tcg: Fix overwrite problems of tcg_cflags

 accel/tcg/cpu-exec.c      | 17 +++++++++++++----
 accel/tcg/tcg-accel-ops.c |  2 +-
 linux-user/elfload.c      | 37 ++++++++++---------------------------
 3 files changed, 24 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH 1/3] Revert "linux-user/arm: Take more care allocating commpage"

[Richard Henderson]

This reverts commit 4f5c67f8df7f26e559509c68c45e652709edd23f.

This exposes bugs in target_mmap et al with respect to overflow
with the final page of the guest address space.  To be fixed in
the next development cycle.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/elfload.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index b96b3e566b..f1370a7a8b 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -423,32 +423,12 @@ enum {
 
 static bool init_guest_commpage(void)
 {
-    ARMCPU *cpu = ARM_CPU(thread_cpu);
-    abi_ptr want = HI_COMMPAGE & TARGET_PAGE_MASK;
-    abi_ptr addr;
+    abi_ptr commpage = HI_COMMPAGE & -qemu_host_page_size;
+    void *want = g2h_untagged(commpage);
+    void *addr = mmap(want, qemu_host_page_size, PROT_READ | PROT_WRITE,
+                      MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
 
-    /*
-     * M-profile allocates maximum of 2GB address space, so can never
-     * allocate the commpage.  Skip it.
-     */
-    if (arm_feature(&cpu->env, ARM_FEATURE_M)) {
-        return true;
-    }
-
-    /*
-     * If reserved_va does not cover the commpage, we get an assert
-     * in page_set_flags.  Produce an intelligent error instead.
-     */
-    if (reserved_va != 0 && want + TARGET_PAGE_SIZE - 1 > reserved_va) {
-        error_report("Allocating guest commpage: -R 0x%" PRIx64 " too small",
-                     (uint64_t)reserved_va + 1);
-        exit(EXIT_FAILURE);
-    }
-
-    addr = target_mmap(want, TARGET_PAGE_SIZE, PROT_READ | PROT_WRITE,
-                       MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
-
-    if (addr == -1) {
+    if (addr == MAP_FAILED) {
         perror("Allocating guest commpage");
         exit(EXIT_FAILURE);
     }
@@ -457,12 +437,15 @@ static bool init_guest_commpage(void)
     }
 
     /* Set kernel helper versions; rest of page is 0.  */
-    put_user_u32(5, 0xffff0ffcu);
+    __put_user(5, (uint32_t *)g2h_untagged(0xffff0ffcu));
 
-    if (target_mprotect(addr, qemu_host_page_size, PROT_READ | PROT_EXEC)) {
+    if (mprotect(addr, qemu_host_page_size, PROT_READ)) {
         perror("Protecting guest commpage");
         exit(EXIT_FAILURE);
     }
+
+    page_set_flags(commpage, commpage | ~qemu_host_page_mask,
+                   PAGE_READ | PAGE_EXEC | PAGE_VALID);
     return true;
 }
 
-- 
2.34.1

Re: [PATCH 1/3] Revert "linux-user/arm: Take more care allocating commpage"

[Philippe Mathieu-Daudé]

On 1/4/23 06:51, Richard Henderson wrote:
> This reverts commit 4f5c67f8df7f26e559509c68c45e652709edd23f.
> 
> This exposes bugs in target_mmap et al with respect to overflow
> with the final page of the guest address space.  To be fixed in
> the next development cycle.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/elfload.c | 37 ++++++++++---------------------------
>   1 file changed, 10 insertions(+), 27 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/3] accel/tcg: Fix overwrite problems of tcg_cflags

[Richard Henderson]

From: Weiwei Li <liweiwei@iscas.ac.cn>

CPUs often set CF_PCREL in tcg_cflags before qemu_init_vcpu(), in which
tcg_cflags will be overwrited by tcg_cpu_init_cflags().

Fixes: 4be790263ffc ("accel/tcg: Replace `TARGET_TB_PCREL` with `CF_PCREL`")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Message-Id: <20230331150609.114401-6-liweiwei@iscas.ac.cn>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/tcg-accel-ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index af35e0d092..58c8e64096 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel)
 
     cflags |= parallel ? CF_PARALLEL : 0;
     cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
-    cpu->tcg_cflags = cflags;
+    cpu->tcg_cflags |= cflags;
 }
 
 void tcg_cpus_destroy(CPUState *cpu)
-- 
2.34.1

Re: [PATCH 2/3] accel/tcg: Fix overwrite problems of tcg_cflags

[Philippe Mathieu-Daudé]

On 1/4/23 06:51, Richard Henderson wrote:
> From: Weiwei Li <liweiwei@iscas.ac.cn>
> 
> CPUs often set CF_PCREL in tcg_cflags before qemu_init_vcpu(), in which
> tcg_cflags will be overwrited by tcg_cpu_init_cflags().

The description makes sense, but I couldn't reproduce using:

-- >8 --
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index af35e0d092..23ebf7de21 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -59,6 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel)

      cflags |= parallel ? CF_PARALLEL : 0;
      cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
+    tcg_debug_assert(!cpu->tcg_cflags);
      cpu->tcg_cflags = cflags;
  }
---

Li and Junqiang, what is your use case?

> Fixes: 4be790263ffc ("accel/tcg: Replace `TARGET_TB_PCREL` with `CF_PCREL`")
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> Message-Id: <20230331150609.114401-6-liweiwei@iscas.ac.cn>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/tcg-accel-ops.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index af35e0d092..58c8e64096 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel)
>   
>       cflags |= parallel ? CF_PARALLEL : 0;
>       cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
> -    cpu->tcg_cflags = cflags;
> +    cpu->tcg_cflags |= cflags;

This could be acceptable as a release kludge, but semantically
tcg_cpu_init_cflags() is meant to *initialize* all flags, not
set few of them. Either we aren't calling it from the proper place,
or we need to rename it.

>   }
>   
>   void tcg_cpus_destroy(CPUState *cpu)

Re: [PATCH 2/3] accel/tcg: Fix overwrite problems of tcg_cflags

[liweiwei]

On 2023/4/3 16:09, Philippe Mathieu-Daudé wrote:
> On 1/4/23 06:51, Richard Henderson wrote:
>> From: Weiwei Li <liweiwei@iscas.ac.cn>
>>
>> CPUs often set CF_PCREL in tcg_cflags before qemu_init_vcpu(), in which
>> tcg_cflags will be overwrited by tcg_cpu_init_cflags().
>
> The description makes sense, but I couldn't reproduce using:
>
> -- >8 --
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index af35e0d092..23ebf7de21 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -59,6 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel)
>
>      cflags |= parallel ? CF_PARALLEL : 0;
>      cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
> +    tcg_debug_assert(!cpu->tcg_cflags);
>      cpu->tcg_cflags = cflags;
>  }
> ---
>
> Li and Junqiang, what is your use case?

Only few CPUs support CF_PCREL currently. I found this problem when I 
tried to introduce PC-relative translation into RISC-V.

Maybe It also can be reproduced in system mode for ARM and X86.

Regards,

Weiwei Li


>
>> Fixes: 4be790263ffc ("accel/tcg: Replace `TARGET_TB_PCREL` with 
>> `CF_PCREL`")
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> Message-Id: <20230331150609.114401-6-liweiwei@iscas.ac.cn>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   accel/tcg/tcg-accel-ops.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
>> index af35e0d092..58c8e64096 100644
>> --- a/accel/tcg/tcg-accel-ops.c
>> +++ b/accel/tcg/tcg-accel-ops.c
>> @@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel)
>>         cflags |= parallel ? CF_PARALLEL : 0;
>>       cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
>> -    cpu->tcg_cflags = cflags;
>> +    cpu->tcg_cflags |= cflags;
>
> This could be acceptable as a release kludge, but semantically
> tcg_cpu_init_cflags() is meant to *initialize* all flags, not
> set few of them. Either we aren't calling it from the proper place,
> or we need to rename it.
>
>>   }
>>     void tcg_cpus_destroy(CPUState *cpu)

Re: [PATCH 2/3] accel/tcg: Fix overwrite problems of tcg_cflags

[Anton Johansson via]

On 4/3/23 11:09, liweiwei wrote:
>
> On 2023/4/3 16:09, Philippe Mathieu-Daudé wrote:
>>      cflags |= parallel ? CF_PARALLEL : 0;
>>      cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
>> +    tcg_debug_assert(!cpu->tcg_cflags);
>>      cpu->tcg_cflags = cflags;
>>  }
>> ---
>>
>> Li and Junqiang, what is your use case?
>
> Only few CPUs support CF_PCREL currently. I found this problem when I 
> tried to introduce PC-relative translation into RISC-V.
>
> Maybe It also can be reproduced in system mode for ARM and X86.

Yes, this can be reproduced on arm-softmmu with --enable-debug-tcg and 
the above assertion.


On 4/3/23 10:09, Philippe Mathieu-Daudé wrote:
>>
>> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
>> index af35e0d092..58c8e64096 100644
>> --- a/accel/tcg/tcg-accel-ops.c
>> +++ b/accel/tcg/tcg-accel-ops.c
>> @@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel)
>>         cflags |= parallel ? CF_PARALLEL : 0;
>>       cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
>> -    cpu->tcg_cflags = cflags;
>> +    cpu->tcg_cflags |= cflags;
>
> This could be acceptable as a release kludge, but semantically
> tcg_cpu_init_cflags() is meant to *initialize* all flags, not
> set few of them. Either we aren't calling it from the proper place,
> or we need to rename it.
Agree, this sounds reasonable.  Also, maybe setting
cpu->tcg_cflags from *_cpu_realizefn was not the right call and
we should have some other way of providing target specific cflags.

-- 
Anton Johansson,
rev.ng Labs Srl.

Re: [PATCH 2/3] accel/tcg: Fix overwrite problems of tcg_cflags

[Philippe Mathieu-Daudé]

On 3/4/23 12:44, Anton Johansson wrote:
> 
> On 4/3/23 11:09, liweiwei wrote:
>>
>> On 2023/4/3 16:09, Philippe Mathieu-Daudé wrote:
>>>      cflags |= parallel ? CF_PARALLEL : 0;
>>>      cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
>>> +    tcg_debug_assert(!cpu->tcg_cflags);
>>>      cpu->tcg_cflags = cflags;
>>>  }
>>> ---
>>>
>>> Li and Junqiang, what is your use case?
>>
>> Only few CPUs support CF_PCREL currently. I found this problem when I 
>> tried to introduce PC-relative translation into RISC-V.
>>
>> Maybe It also can be reproduced in system mode for ARM and X86.
> 
> Yes, this can be reproduced on arm-softmmu with --enable-debug-tcg and 
> the above assertion.

Ah OK. Then...

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/3] accel/tcg: Fix jump cache set in cpu_exec_loop

[Richard Henderson]

Assign pc and use store_release to assign tb.

Fixes: 2dd5b7a1b91 ("accel/tcg: Move jmp-cache `CF_PCREL` checks to caller")
Reported-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cpu-exec.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index c815f2dbfd..8370c92c05 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -257,7 +257,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
 
     if (cflags & CF_PCREL) {
         /* Use acquire to ensure current load of pc from jc. */
-        tb =  qatomic_load_acquire(&jc->array[hash].tb);
+        tb = qatomic_load_acquire(&jc->array[hash].tb);
 
         if (likely(tb &&
                    jc->array[hash].pc == pc &&
@@ -272,7 +272,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
             return NULL;
         }
         jc->array[hash].pc = pc;
-        /* Use store_release on tb to ensure pc is written first. */
+        /* Ensure pc is written first. */
         qatomic_store_release(&jc->array[hash].tb, tb);
     } else {
         /* Use rcu_read to ensure current load of pc from *tb. */
@@ -971,18 +971,27 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
 
             tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
             if (tb == NULL) {
+                CPUJumpCache *jc;
                 uint32_t h;
 
                 mmap_lock();
                 tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
                 mmap_unlock();
+
                 /*
                  * We add the TB in the virtual pc hash table
                  * for the fast lookup
                  */
                 h = tb_jmp_cache_hash_func(pc);
-                /* Use the pc value already stored in tb->pc. */
-                qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb);
+                jc = cpu->tb_jmp_cache;
+                if (cflags & CF_PCREL) {
+                    jc->array[h].pc = pc;
+                    /* Ensure pc is written first. */
+                    qatomic_store_release(&jc->array[h].tb, tb);
+                } else {
+                    /* Use the pc value already stored in tb->pc. */
+                    qatomic_set(&jc->array[h].tb, tb);
+                }
             }
 
 #ifndef CONFIG_USER_ONLY
-- 
2.34.1

Re: [PATCH 3/3] accel/tcg: Fix jump cache set in cpu_exec_loop

[liweiwei]

On 2023/4/1 12:51, Richard Henderson wrote:
> Assign pc and use store_release to assign tb.
>
> Fixes: 2dd5b7a1b91 ("accel/tcg: Move jmp-cache `CF_PCREL` checks to caller")
> Reported-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/cpu-exec.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index c815f2dbfd..8370c92c05 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -257,7 +257,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
>   
>       if (cflags & CF_PCREL) {
>           /* Use acquire to ensure current load of pc from jc. */
> -        tb =  qatomic_load_acquire(&jc->array[hash].tb);
> +        tb = qatomic_load_acquire(&jc->array[hash].tb);
>   
>           if (likely(tb &&
>                      jc->array[hash].pc == pc &&
> @@ -272,7 +272,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
>               return NULL;
>           }
>           jc->array[hash].pc = pc;
> -        /* Use store_release on tb to ensure pc is written first. */
> +        /* Ensure pc is written first. */
>           qatomic_store_release(&jc->array[hash].tb, tb);
>       } else {
>           /* Use rcu_read to ensure current load of pc from *tb. */
> @@ -971,18 +971,27 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
>   
>               tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
>               if (tb == NULL) {
> +                CPUJumpCache *jc;
>                   uint32_t h;
>   
>                   mmap_lock();
>                   tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
>                   mmap_unlock();
> +
Blank line.
>                   /*
>                    * We add the TB in the virtual pc hash table
>                    * for the fast lookup
>                    */
>                   h = tb_jmp_cache_hash_func(pc);
> -                /* Use the pc value already stored in tb->pc. */
> -                qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb);
> +                jc = cpu->tb_jmp_cache;
> +                if (cflags & CF_PCREL) {
> +                    jc->array[h].pc = pc;
> +                    /* Ensure pc is written first. */
> +                    qatomic_store_release(&jc->array[h].tb, tb);

Whether we should add a qatomic_load_require() before this?

Regards,

Weiwei Li

> +                } else {
> +                    /* Use the pc value already stored in tb->pc. */
> +                    qatomic_set(&jc->array[h].tb, tb);
> +                }
>               }
>   
>   #ifndef CONFIG_USER_ONLY

Re: [PATCH 3/3] accel/tcg: Fix jump cache set in cpu_exec_loop

[Richard Henderson]

On 4/1/23 04:03, liweiwei wrote:
>>                   mmap_unlock();
>> +
> Blank line.

Yes, adding separation.

>>                   /*
>>                    * We add the TB in the virtual pc hash table
>>                    * for the fast lookup
>>                    */
>>                   h = tb_jmp_cache_hash_func(pc);
>> -                /* Use the pc value already stored in tb->pc. */
>> -                qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb);
>> +                jc = cpu->tb_jmp_cache;
>> +                if (cflags & CF_PCREL) {
>> +                    jc->array[h].pc = pc;
>> +                    /* Ensure pc is written first. */
>> +                    qatomic_store_release(&jc->array[h].tb, tb);
> 
> Whether we should add a qatomic_load_require() before this?

The load_acquire is already present in tb_lookup.


r~

Re: [PATCH 3/3] accel/tcg: Fix jump cache set in cpu_exec_loop

[Peter Maydell]

On Sat, 1 Apr 2023 at 05:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Assign pc and use store_release to assign tb.
>
> Fixes: 2dd5b7a1b91 ("accel/tcg: Move jmp-cache `CF_PCREL` checks to caller")
> Reported-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  accel/tcg/cpu-exec.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

I don't fully understand the code here, but before 2dd5b7a1b91
this was a callsite for tb_jmp_cache_set(), 2dd5b7a1b91
expanded out the call but forgot the CF_PCREL half of the if,
and this patch restores it. Which all seems logical, so

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH for-8.0 0/3] last minute tcg fixes

[Richard Henderson]

Ping patch 2 & 3.

r~

On 3/31/23 21:51, Richard Henderson wrote:
> Fix a bug just exposed concerning qemu-arm commpage, leading to an
> immediate crash on any 64k page host.  Fix two bugs regressing
> pc-relative tb generation, found by Weiwei Li.
> 
> 
> r~
> 
> 
> Richard Henderson (2):
>    Revert "linux-user/arm: Take more care allocating commpage"
>    accel/tcg: Fix jump cache set in cpu_exec_loop
> 
> Weiwei Li (1):
>    accel/tcg: Fix overwrite problems of tcg_cflags
> 
>   accel/tcg/cpu-exec.c      | 17 +++++++++++++----
>   accel/tcg/tcg-accel-ops.c |  2 +-
>   linux-user/elfload.c      | 37 ++++++++++---------------------------
>   3 files changed, 24 insertions(+), 32 deletions(-)
>