[PATCH v2 1/7] target/hexagon: initialize deferred sreg-write temps to current value

Brian Cain posted 7 patches 2 months ago
Maintainers: Brian Cain <brian.cain@oss.qualcomm.com>, Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>, Paolo Bonzini <pbonzini@redhat.com>, Thomas Huth <th.huth+qemu@posteo.eu>, "Philippe Mathieu-Daudé" <philmd@mailo.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>
[PATCH v2 1/7] target/hexagon: initialize deferred sreg-write temps to current value
Posted by Brian Cain 2 months ago
Unconditionally allocate and initialize the temp to the sreg's current
value for every logged sreg write, rather than only allocating
when need_commit is set or the register is SSR.

Without this, reading a deferred sreg write within the same packet
before it is committed can observe an uninitialized temp instead
of the register's pre-packet value.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/translate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index 77235916f4b..28a171bf623 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -615,9 +615,10 @@ static void gen_start_packet(DisasContext *ctx)
     }
     for (i = 0; i < ctx->sreg_log_idx; i++) {
         int reg_num = ctx->sreg_log[i];
-        if (reg_num < HEX_SREG_GLB_START &&
-            (ctx->need_commit || reg_num == HEX_SREG_SSR)) {
+        if (reg_num < HEX_SREG_GLB_START) {
             ctx->t_sreg_new_value[reg_num] = tcg_temp_new();
+            tcg_gen_mov_tl(ctx->t_sreg_new_value[reg_num],
+                          hex_t_sreg[reg_num]);
         }
     }
     for (i = 0; i < NUM_GREGS; i++) {
-- 
2.34.1

Re: [PATCH v2 1/7] target/hexagon: initialize deferred sreg-write temps to current value
Posted by Taylor Simpson 1 month, 4 weeks ago
On Wed, Jul 15, 2026 at 12:57 PM Brian Cain <brian.cain@oss.qualcomm.com>
wrote:

> Unconditionally allocate and initialize the temp to the sreg's current
> value for every logged sreg write, rather than only allocating
> when need_commit is set or the register is SSR.
>
> Without this, reading a deferred sreg write within the same packet
> before it is committed can observe an uninitialized temp instead
> of the register's pre-packet value.
>

Do you have an example where this happens?  I searched the code and only
found references to t_sreg_new_value guarded by ctx->need_commit except
gen_sreg_writes which should have an early exit if ctx->need_commit is set
(similar to gen_reg_writes).

The special handling of SSR could be cleaned up.  Consider making writes to
SSR set ctx->need_commit.

Also, consider creating a function similar to get_result_gpr to centralize
all the checking (e.g., in gen_log_sreg_write).


>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/translate.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
> index 77235916f4b..28a171bf623 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -615,9 +615,10 @@ static void gen_start_packet(DisasContext *ctx)
>      }
>      for (i = 0; i < ctx->sreg_log_idx; i++) {
>          int reg_num = ctx->sreg_log[i];
> -        if (reg_num < HEX_SREG_GLB_START &&
> -            (ctx->need_commit || reg_num == HEX_SREG_SSR)) {
> +        if (reg_num < HEX_SREG_GLB_START) {
>              ctx->t_sreg_new_value[reg_num] = tcg_temp_new();
> +            tcg_gen_mov_tl(ctx->t_sreg_new_value[reg_num],
> +                          hex_t_sreg[reg_num]);
>          }
>      }
>      for (i = 0; i < NUM_GREGS; i++) {
> --
> 2.34.1
>
>