[PATCH 0/1] util/cacheflush: Make first DSB unconditional on aarch64

[PATCH 0/1] util/cacheflush: Make first DSB unconditional on aarch64

[Joe Komlodi]

Hi all,

This fixes some TCG TB corruption we would occasionally see on aarch64
hosts in certain situations. Specifically, if the host had CTR_EL0.DIC
and CTR_EL0.IDC set, and if the TBs generated were very small, the
instructions in the TB would sometimes be garbage. This would mostly
result in a SIGILL when executing the TB, or sometimes a SIGSEGV if the
garbage instruction was to branch to a garbage address.

If a host has CTR_EL0.DIC and CTR_EL0.IDC set, the aarch64 cache
maintenance function doesn't execute a DSB, which seems to be the cause
of the corruption. I think it's because the ISB guarantees that the
instructions will be executed, but doesn't guarantee that any
outstanding writes will be fully committed.
This only seemed to happen on very small TBs, which I'm guessing is
because there's much fewer instructions between the TB being generated and
executed, which could lead to writes not being committed before execution.

This function is intended to be a copy of the upstream gcc one, which
does an unconditional DSB, so we can fix this by just doing that as well.

Thanks!
Joe

Joe Komlodi (1):
  util/cacheflush: Make first DSB unconditional on aarch64

 util/cacheflush.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.49.0.rc0.332.g42c0ae87b1-goog

[PATCH 1/1] util/cacheflush: Make first DSB unconditional on aarch64

[Joe Komlodi]

On ARM hosts with CTR_EL0.DIC and CTR_EL0.IDC set, this would only cause
an ISB to be executed during cache maintenance, which could lead to QEMU
executing TBs containing garbage instructions.

This seems to be because the ISB finishes executing instructions and
flushes the pipeline, but the ISB doesn't guarantee that writes from the
executed instructions are committed. If a small enough TB is created, it's
possible that the writes setting up the TB aren't committed by the time the
TB is executed.

This function is intended to be a port of the gcc implementation
(https://github.com/gcc-mirror/gcc/blob/85b46d0795ac76bc192cb8f88b646a647acf98c1/libgcc/config/aarch64/sync-cache.c#L67)
which makes the first DSB unconditional, so we can fix the synchronization
issue by doing that as well.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 util/cacheflush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/cacheflush.c b/util/cacheflush.c
index a08906155a..1d12899a39 100644
--- a/util/cacheflush.c
+++ b/util/cacheflush.c
@@ -279,9 +279,11 @@ void flush_idcache_range(uintptr_t rx, uintptr_t rw, size_t len)
         for (p = rw & -dcache_lsize; p < rw + len; p += dcache_lsize) {
             asm volatile("dc\tcvau, %0" : : "r" (p) : "memory");
         }
-        asm volatile("dsb\tish" : : : "memory");
     }
 
+    /* DSB unconditionally to ensure any outstanding writes are committed. */
+    asm volatile("dsb\tish" : : : "memory");
+
     /*
      * If CTR_EL0.DIC is enabled, Instruction cache cleaning to the Point
      * of Unification is not required for instruction to data coherence.
-- 
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH 1/1] util/cacheflush: Make first DSB unconditional on aarch64

[Peter Maydell]

On Mon, 10 Mar 2025 at 20:36, Joe Komlodi <komlodi@google.com> wrote:
>
> On ARM hosts with CTR_EL0.DIC and CTR_EL0.IDC set, this would only cause
> an ISB to be executed during cache maintenance, which could lead to QEMU
> executing TBs containing garbage instructions.
>
> This seems to be because the ISB finishes executing instructions and
> flushes the pipeline, but the ISB doesn't guarantee that writes from the
> executed instructions are committed. If a small enough TB is created, it's
> possible that the writes setting up the TB aren't committed by the time the
> TB is executed.

Yes; we need the DSB to ensure that the stores have completed
and are visible to subsequent icache fills; and then we need
the ISB to ensure that any instructions that we execute after
this are done with an instruction fetch that happens after the
ISB (i.e. the CPU hasn't already speculatively fetched the insn
before we forced the store to complete).

> This function is intended to be a port of the gcc implementation
> (https://github.com/gcc-mirror/gcc/blob/85b46d0795ac76bc192cb8f88b646a647acf98c1/libgcc/config/aarch64/sync-cache.c#L67)
> which makes the first DSB unconditional, so we can fix the synchronization
> issue by doing that as well.
>
> Signed-off-by: Joe Komlodi <komlodi@google.com>

We should add:

Cc: qemu-stable@nongnu.org
Fixes: 664a79735e4deb1 ("util: Specialize flush_idcache_range for aarch64")

> ---
>  util/cacheflush.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/util/cacheflush.c b/util/cacheflush.c
> index a08906155a..1d12899a39 100644
> --- a/util/cacheflush.c
> +++ b/util/cacheflush.c
> @@ -279,9 +279,11 @@ void flush_idcache_range(uintptr_t rx, uintptr_t rw, size_t len)
>          for (p = rw & -dcache_lsize; p < rw + len; p += dcache_lsize) {
>              asm volatile("dc\tcvau, %0" : : "r" (p) : "memory");
>          }
> -        asm volatile("dsb\tish" : : : "memory");
>      }
>
> +    /* DSB unconditionally to ensure any outstanding writes are committed. */
> +    asm volatile("dsb\tish" : : : "memory");
> +
>      /*
>       * If CTR_EL0.DIC is enabled, Instruction cache cleaning to the Point
>       * of Unification is not required for instruction to data coherence.

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

Richard, are you doing a TCG pullreq for rc0? If not, I can
put this into target-arm.next.

thanks
-- PMM

Re: [PATCH 1/1] util/cacheflush: Make first DSB unconditional on aarch64

[Richard Henderson]

On 3/12/25 07:18, Peter Maydell wrote:
> On Mon, 10 Mar 2025 at 20:36, Joe Komlodi <komlodi@google.com> wrote:
>>
>> On ARM hosts with CTR_EL0.DIC and CTR_EL0.IDC set, this would only cause
>> an ISB to be executed during cache maintenance, which could lead to QEMU
>> executing TBs containing garbage instructions.
>>
>> This seems to be because the ISB finishes executing instructions and
>> flushes the pipeline, but the ISB doesn't guarantee that writes from the
>> executed instructions are committed. If a small enough TB is created, it's
>> possible that the writes setting up the TB aren't committed by the time the
>> TB is executed.
> 
> Yes; we need the DSB to ensure that the stores have completed
> and are visible to subsequent icache fills; and then we need
> the ISB to ensure that any instructions that we execute after
> this are done with an instruction fetch that happens after the
> ISB (i.e. the CPU hasn't already speculatively fetched the insn
> before we forced the store to complete).
> 
>> This function is intended to be a port of the gcc implementation
>> (https://github.com/gcc-mirror/gcc/blob/85b46d0795ac76bc192cb8f88b646a647acf98c1/libgcc/config/aarch64/sync-cache.c#L67)
>> which makes the first DSB unconditional, so we can fix the synchronization
>> issue by doing that as well.
>>
>> Signed-off-by: Joe Komlodi <komlodi@google.com>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


> Richard, are you doing a TCG pullreq for rc0? If not, I can
> put this into target-arm.next.

So far I have nothing queued for tcg.  Please go ahead.


r~