[PATCH v3 0/5] hexagon: fix HVX scatter gather, BE host

[PATCH v3 0/5] hexagon: fix HVX scatter gather, BE host

[Brian Cain]

Anton pointed out this endianness issue with HVX,
and found this region length issue along the way.

v3:
* Changed the region-length test to be more consistent between scatter
and gather and focusing on just inside, outside, beyond the boundary.

Fixed in v2:
* byte access helpers now all use hexagon_ prefix.
* Predicate bytes are extracted from 32-bit words rather than 64-bit words,
  matching the host-native predicate layout.
* Gather only reads memory for lanes that are both in range and predicate
  enabled.  So dropped lanes cannot fault or otherwise access memory.
* The gather region-length test now places an out-of-range offset on an
  inaccessible page, verifying that a dropped lane performs no read.

Added v2 commits:
* fix for histogram predicates - were using the wrong size
* test to cover predicate byte permutation

Brian Cain (5):
  target/hexagon: fix HVX big-endian byte access
  target/hexagon: fix HVX scatter/gather region-length check
  tests/tcg/hexagon: add vgather/vscatter region-length tests
  target/hexagon: fix HVX predicate save size for histogram ops
  tests/tcg/hexagon: check the two HVX predicate build paths agree

 target/hexagon/gen_tcg_hvx.h            |  10 +-
 target/hexagon/mmvec/macros.h           |  50 +++++----
 target/hexagon/mmvec/mmvec.h            |  18 ++++
 target/hexagon/cpu.c                    |  17 +--
 target/hexagon/genptr.c                 |   3 +-
 target/hexagon/mmvec/system_ext_mmvec.c |   2 +-
 target/hexagon/op_helper.c              |  17 ++-
 tests/tcg/hexagon/hvx_misc.c            |  45 ++++++++
 tests/tcg/hexagon/scatter_gather.c      | 137 +++++++++++++++++++++++-
 9 files changed, 260 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] target/hexagon: fix HVX big-endian byte access

[Brian Cain]

Keep HVX lanes host-native while converting byte indexes at memory
and helper boundaries.

Also place vector comparison predicate bytes according to host word order.

Suggested-by: Anton Johansson <anjo@rev.ng>
Reviewed-by: Anton Johansson <anjo@rev.ng>
Reviewed-by: Matheus Tavares Bernardino <matheus.bernardino@oss.qualcomm.com>
Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/mmvec/macros.h           | 23 +++++++++++++++--------
 target/hexagon/mmvec/mmvec.h            | 18 ++++++++++++++++++
 target/hexagon/cpu.c                    | 17 +++++++++++------
 target/hexagon/genptr.c                 |  3 ++-
 target/hexagon/mmvec/system_ext_mmvec.c |  2 +-
 target/hexagon/op_helper.c              | 17 ++++++++++++-----
 6 files changed, 59 insertions(+), 21 deletions(-)

diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
index b36f833b1de..e85beed3387 100644
--- a/target/hexagon/mmvec/macros.h
+++ b/target/hexagon/mmvec/macros.h
@@ -53,7 +53,7 @@
 
 #define LOG_VTCM_BYTE(VA, MASK, VAL, IDX) \
     do { \
-        env->vtcm_log.data.ub[IDX] = (VAL); \
+        hexagon_mmvec_set_byte(&env->vtcm_log.data, IDX, VAL); \
         if (MASK) { \
             set_bit((IDX), env->vtcm_log.mask); \
         } else { \
@@ -133,7 +133,8 @@
         target_ulong va_high = EA + LEN; \
         for (int i0 = 0; i0 < 4; i0++) { \
             log_byte = (va + i0) <= va_high; \
-            LOG_VTCM_BYTE(va + i0, log_byte, INC. ub[4 * IDX + i0], \
+            LOG_VTCM_BYTE(va + i0, log_byte, \
+                           hexagon_mmvec_get_byte(&(INC), 4 * IDX + i0), \
                           4 * IDX + i0); \
         } \
     } while (0)
@@ -144,7 +145,8 @@
         target_ulong va_high = EA + LEN; \
         for (int i0 = 0; i0 < 2; i0++) { \
             log_byte = (va + i0) <= va_high; \
-            LOG_VTCM_BYTE(va + i0, log_byte, INC.ub[2 * IDX + i0], \
+            LOG_VTCM_BYTE(va + i0, log_byte, \
+                           hexagon_mmvec_get_byte(&(INC), 2 * IDX + i0), \
                           2 * IDX + i0); \
         } \
     } while (0)
@@ -157,7 +159,8 @@
         target_ulong va_high = EA + LEN; \
         for (int i0 = 0; i0 < 2; i0++) { \
             log_byte = (va + i0) <= va_high; \
-            LOG_VTCM_BYTE(va + i0, log_byte, INC.ub[2 * IDX + i0], \
+            LOG_VTCM_BYTE(va + i0, log_byte, \
+                           hexagon_mmvec_get_byte(&(INC), 2 * IDX + i0), \
                           2 * IDX + i0); \
         } \
     } while (0)
@@ -174,7 +177,8 @@
             log_byte = ((va + i0) <= va_high) && QVAL; \
             uint8_t B; \
             B = cpu_ldub_data_ra(env, EA + i0, ra); \
-            env->tmp_VRegs[0].ub[ELEMENT_SIZE * IDX + i0] = B; \
+            hexagon_mmvec_set_byte(&env->tmp_VRegs[0], \
+                                   ELEMENT_SIZE * IDX + i0, B); \
             LOG_VTCM_BYTE(va + i0, log_byte, B, ELEMENT_SIZE * IDX + i0); \
         } \
     } while (0)
@@ -216,9 +220,10 @@
                     uint8_t val; \
                     val = cpu_ldub_data_ra(env, env->vtcm_log.va[i + j], ra); \
                     dst |= val << (8 * j); \
-                    inc |= env->vtcm_log.data.ub[j + i] << (8 * j); \
+                    inc |= hexagon_mmvec_get_byte(&env->vtcm_log.data, j + i) \
+                           << (8 * j); \
                     clear_bit(j + i, env->vtcm_log.mask); \
-                    env->vtcm_log.data.ub[j + i] = 0; \
+                    hexagon_mmvec_set_byte(&env->vtcm_log.data, j + i, 0); \
                 } \
                 dst += inc; \
                 for (int j = 0; j < sizeof(TYPE); j++) { \
@@ -249,7 +254,9 @@
         int log_byte = 0; \
         for (i0 = 0; i0 < ELEM_SIZE; i0++) { \
             log_byte = ((va + i0) <= va_high) && QVAL; \
-            LOG_VTCM_BYTE(va + i0, log_byte, IN.ub[ELEM_SIZE * IDX + i0], \
+            LOG_VTCM_BYTE(va + i0, log_byte, \
+                           hexagon_mmvec_get_byte(&(IN), \
+                                                    ELEM_SIZE * IDX + i0), \
                           ELEM_SIZE * IDX + i0); \
         } \
     } while (0)
diff --git a/target/hexagon/mmvec/mmvec.h b/target/hexagon/mmvec/mmvec.h
index 8e72f2f6ae7..662f9d1597e 100644
--- a/target/hexagon/mmvec/mmvec.h
+++ b/target/hexagon/mmvec/mmvec.h
@@ -20,6 +20,7 @@
 
 #include "exec/target_long.h"
 #include "qemu/bitmap.h"
+#include "qemu/bitops.h"
 
 #define MAX_VEC_SIZE_LOGBYTES 7
 #define MAX_VEC_SIZE_BYTES  (1 << MAX_VEC_SIZE_LOGBYTES)
@@ -69,6 +70,23 @@ typedef union {
     int8_t    b[MAX_VEC_SIZE_BYTES / 1 / 8];
 } MMQReg;
 
+static inline uint8_t hexagon_mmvec_get_byte(const MMVector *v, size_t index)
+{
+    return extract64(v->ud[index / 8], (index % 8) * 8, 8);
+}
+
+static inline void hexagon_mmvec_set_byte(MMVector *v, size_t index,
+                                          uint8_t value)
+{
+    v->ud[index / 8] = deposit64(v->ud[index / 8], (index % 8) * 8, 8,
+                                 value);
+}
+
+static inline uint8_t hexagon_mmqreg_get_byte(const MMQReg *q, size_t index)
+{
+    return extract32(q->uw[index / 4], (index % 4) * 8, 8);
+}
+
 typedef struct {
     MMVector data;
     DECLARE_BITMAP(mask, MAX_VEC_SIZE_BYTES);
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 4a7b76e3bcc..ac5821a1e44 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -182,7 +182,7 @@ static void print_vreg(FILE *f, CPUHexagonState *env, int regnum,
     if (skip_if_zero) {
         bool nonzero_found = false;
         for (int i = 0; i < MAX_VEC_SIZE_BYTES; i++) {
-            if (env->VRegs[regnum].ub[i] != 0) {
+            if (hexagon_mmvec_get_byte(&env->VRegs[regnum], i) != 0) {
                 nonzero_found = true;
                 break;
             }
@@ -193,9 +193,12 @@ static void print_vreg(FILE *f, CPUHexagonState *env, int regnum,
     }
 
     qemu_fprintf(f, "  v%d = ( ", regnum);
-    qemu_fprintf(f, "0x%02x", env->VRegs[regnum].ub[MAX_VEC_SIZE_BYTES - 1]);
+    qemu_fprintf(f, "0x%02x",
+                 hexagon_mmvec_get_byte(&env->VRegs[regnum],
+                                        MAX_VEC_SIZE_BYTES - 1));
     for (int i = MAX_VEC_SIZE_BYTES - 2; i >= 0; i--) {
-        qemu_fprintf(f, ", 0x%02x", env->VRegs[regnum].ub[i]);
+        qemu_fprintf(f, ", 0x%02x",
+                     hexagon_mmvec_get_byte(&env->VRegs[regnum], i));
     }
     qemu_fprintf(f, " )\n");
 }
@@ -211,7 +214,7 @@ static void print_qreg(FILE *f, CPUHexagonState *env, int regnum,
     if (skip_if_zero) {
         bool nonzero_found = false;
         for (int i = 0; i < MAX_VEC_SIZE_BYTES / 8; i++) {
-            if (env->QRegs[regnum].ub[i] != 0) {
+            if (hexagon_mmqreg_get_byte(&env->QRegs[regnum], i) != 0) {
                 nonzero_found = true;
                 break;
             }
@@ -223,9 +226,11 @@ static void print_qreg(FILE *f, CPUHexagonState *env, int regnum,
 
     qemu_fprintf(f, "  q%d = ( ", regnum);
     qemu_fprintf(f, "0x%02x",
-                 env->QRegs[regnum].ub[MAX_VEC_SIZE_BYTES / 8 - 1]);
+                 hexagon_mmqreg_get_byte(&env->QRegs[regnum],
+                                         MAX_VEC_SIZE_BYTES / 8 - 1));
     for (int i = MAX_VEC_SIZE_BYTES / 8 - 2; i >= 0; i--) {
-        qemu_fprintf(f, ", 0x%02x", env->QRegs[regnum].ub[i]);
+        qemu_fprintf(f, ", 0x%02x",
+                     hexagon_mmqreg_get_byte(&env->QRegs[regnum], i));
     }
     qemu_fprintf(f, " )\n");
 }
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index 1f109d44de8..cfcbd77d47a 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -1557,7 +1557,8 @@ static void vec_to_qvec(size_t size, intptr_t dstoff, intptr_t srcoff)
             tcg_gen_deposit_i64(mask, mask, bits, j, size);
         }
 
-        tcg_gen_st8_i64(mask, tcg_env, dstoff + i);
+        tcg_gen_st8_i64(mask, tcg_env,
+                        dstoff + (i ^ (HOST_BIG_ENDIAN ? 3 : 0)));
     }
 }
 
diff --git a/target/hexagon/mmvec/system_ext_mmvec.c b/target/hexagon/mmvec/system_ext_mmvec.c
index 8351f2cc01b..081cadd814a 100644
--- a/target/hexagon/mmvec/system_ext_mmvec.c
+++ b/target/hexagon/mmvec/system_ext_mmvec.c
@@ -26,7 +26,7 @@ void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int slot)
     env->vstore_pending[slot] = 1;
     env->vstore[slot].va   = vaddr;
     env->vstore[slot].size = size;
-    memcpy(&env->vstore[slot].data.ub[0], &env->tmp_VRegs[0], size);
+    memcpy(&env->vstore[slot].data, &env->tmp_VRegs[0], size);
 
     /* On a gather store, overwrite the store mask to emulate dropped gathers */
     bitmap_copy(env->vstore[slot].mask, env->vtcm_log.mask, size);
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 71555a7ba36..d74faebc582 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -168,7 +168,10 @@ void HELPER(commit_hvx_stores)(CPUHexagonState *env)
             int size = env->vstore[i].size;
             for (int j = 0; j < size; j++) {
                 if (test_bit(j, env->vstore[i].mask)) {
-                    cpu_stb_data_ra(env, va + j, env->vstore[i].data.ub[j], ra);
+                    cpu_stb_data_ra(env, va + j,
+                                    hexagon_mmvec_get_byte(&env->vstore[i].data,
+                                                           j),
+                                    ra);
                 }
             }
         }
@@ -191,9 +194,11 @@ void HELPER(commit_hvx_stores)(CPUHexagonState *env)
             for (int i = 0; i < sizeof(MMVector); i++) {
                 if (test_bit(i, env->vtcm_log.mask)) {
                     cpu_stb_data_ra(env, env->vtcm_log.va[i],
-                                    env->vtcm_log.data.ub[i], ra);
+                                     hexagon_mmvec_get_byte(&env->vtcm_log.data,
+                                                            i),
+                                     ra);
                     clear_bit(i, env->vtcm_log.mask);
-                    env->vtcm_log.data.ub[i] = 0;
+                    hexagon_mmvec_set_byte(&env->vtcm_log.data, i, 0);
                 }
 
             }
@@ -1404,7 +1409,8 @@ void HELPER(vhist)(CPUHexagonState *env)
 
     for (int lane = 0; lane < 8; lane++) {
         for (int i = 0; i < sizeof(MMVector) / 8; ++i) {
-            unsigned char value = input->ub[(sizeof(MMVector) / 8) * lane + i];
+            unsigned char value = hexagon_mmvec_get_byte(input,
+                (sizeof(MMVector) / 8) * lane + i);
             unsigned char regno = value >> 3;
             unsigned char element = value & 7;
 
@@ -1419,7 +1425,8 @@ void HELPER(vhistq)(CPUHexagonState *env)
 
     for (int lane = 0; lane < 8; lane++) {
         for (int i = 0; i < sizeof(MMVector) / 8; ++i) {
-            unsigned char value = input->ub[(sizeof(MMVector) / 8) * lane + i];
+            unsigned char value = hexagon_mmvec_get_byte(input,
+                (sizeof(MMVector) / 8) * lane + i);
             unsigned char regno = value >> 3;
             unsigned char element = value & 7;
 
-- 
2.34.1

[PATCH v3 2/5] target/hexagon: fix HVX scatter/gather region-length check

[Brian Cain]

The region-length bounds check compared (EA + i0) <= (EA + LEN),
which reduces to i0 <= LEN and never actually depends on the region
base. This let vgather/vscatter keep elements whose offset was
beyond the declared region instead of dropping them. Compare the
offset to the region length directly instead.

Only load bytes for gather lanes that are kept. A dropped or
predicate-false lane must not access memory and raise an exception.

Reviewed-by: Matheus Tavares Bernardino <matheus.bernardino@oss.qualcomm.com>
Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/mmvec/macros.h | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
index e85beed3387..858afd995d9 100644
--- a/target/hexagon/mmvec/macros.h
+++ b/target/hexagon/mmvec/macros.h
@@ -130,9 +130,9 @@
     do { \
         int log_byte = 0; \
         target_ulong va = EA; \
-        target_ulong va_high = EA + LEN; \
+        int in_region = (OFFSET) <= (LEN); \
         for (int i0 = 0; i0 < 4; i0++) { \
-            log_byte = (va + i0) <= va_high; \
+            log_byte = in_region; \
             LOG_VTCM_BYTE(va + i0, log_byte, \
                            hexagon_mmvec_get_byte(&(INC), 4 * IDX + i0), \
                           4 * IDX + i0); \
@@ -142,9 +142,9 @@
     do { \
         int log_byte = 0; \
         target_ulong va = EA; \
-        target_ulong va_high = EA + LEN; \
+        int in_region = (OFFSET) <= (LEN); \
         for (int i0 = 0; i0 < 2; i0++) { \
-            log_byte = (va + i0) <= va_high; \
+            log_byte = in_region; \
             LOG_VTCM_BYTE(va + i0, log_byte, \
                            hexagon_mmvec_get_byte(&(INC), 2 * IDX + i0), \
                           2 * IDX + i0); \
@@ -156,9 +156,9 @@
     do { \
         int log_byte = 0; \
         target_ulong va = EA; \
-        target_ulong va_high = EA + LEN; \
+        int in_region = (OFFSET) <= (LEN); \
         for (int i0 = 0; i0 < 2; i0++) { \
-            log_byte = (va + i0) <= va_high; \
+            log_byte = in_region; \
             LOG_VTCM_BYTE(va + i0, log_byte, \
                            hexagon_mmvec_get_byte(&(INC), 2 * IDX + i0), \
                           2 * IDX + i0); \
@@ -170,15 +170,18 @@
     do { \
         int i0; \
         target_ulong va = EA; \
-        target_ulong va_high = EA + LEN; \
         uintptr_t ra = GETPC(); \
         int log_byte = 0; \
+        int in_region = (OFFSET) <= (LEN); \
         for (i0 = 0; i0 < ELEMENT_SIZE; i0++) { \
-            log_byte = ((va + i0) <= va_high) && QVAL; \
-            uint8_t B; \
-            B = cpu_ldub_data_ra(env, EA + i0, ra); \
-            hexagon_mmvec_set_byte(&env->tmp_VRegs[0], \
-                                   ELEMENT_SIZE * IDX + i0, B); \
+            uint8_t B = 0; \
+            \
+            log_byte = in_region && QVAL; \
+            if (log_byte) { \
+                B = cpu_ldub_data_ra(env, va + i0, ra); \
+                hexagon_mmvec_set_byte(&env->tmp_VRegs[0], \
+                                       ELEMENT_SIZE * IDX + i0, B); \
+            } \
             LOG_VTCM_BYTE(va + i0, log_byte, B, ELEMENT_SIZE * IDX + i0); \
         } \
     } while (0)
@@ -250,10 +253,10 @@
     do { \
         int i0; \
         target_ulong va = EA; \
-        target_ulong va_high = EA + LEN; \
         int log_byte = 0; \
+        int in_region = (OFFSET) <= (LEN); \
         for (i0 = 0; i0 < ELEM_SIZE; i0++) { \
-            log_byte = ((va + i0) <= va_high) && QVAL; \
+            log_byte = in_region && QVAL; \
             LOG_VTCM_BYTE(va + i0, log_byte, \
                            hexagon_mmvec_get_byte(&(IN), \
                                                     ELEM_SIZE * IDX + i0), \
-- 
2.34.1

[PATCH v3 3/5] tests/tcg/hexagon: add vgather/vscatter region-length tests

[Brian Cain]

Cover the case where an offset lies beyond the region length: it
must be dropped rather than read/written.

Place one dropped gather offset on a PROT_NONE page to verify that
the instruction does not access memory for that lane.

Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 tests/tcg/hexagon/scatter_gather.c | 137 ++++++++++++++++++++++++++++-
 1 file changed, 136 insertions(+), 1 deletion(-)

diff --git a/tests/tcg/hexagon/scatter_gather.c b/tests/tcg/hexagon/scatter_gather.c
index bf8b5e03172..40f00cc6cf5 100644
--- a/tests/tcg/hexagon/scatter_gather.c
+++ b/tests/tcg/hexagon/scatter_gather.c
@@ -31,7 +31,11 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
+#include <assert.h>
 #include <inttypes.h>
+#include <sys/mman.h>
+#include <hexagon_types.h>
+#include <hvx_hexagon_protos.h>
 
 typedef long HVX_Vector       __attribute__((__vector_size__(128)))
                               __attribute__((aligned(128)));
@@ -85,6 +89,23 @@ unsigned int   word_predicates[MATRIX_SIZE] __attribute__((aligned(128)));
 /* make this big enough for all the operations */
 const size_t region_len = sizeof(vtcm);
 
+/* Mu (region length - 1); offset is kept iff offset <= Mu */
+#define REGION_LEN_TEST_MU 127
+#define GATHER_LEN_TEST_PAGE_SIZE 4096
+static unsigned char *gather_len_test_src;
+static unsigned char gather_len_test_dst[128] __attribute__((aligned(128)));
+static unsigned char gather_len_test_dst_ref[128];
+
+/* The dropped offset must remain in the backing destination buffer. */
+#define SCATTER_LEN_TEST_DST_SIZE 512
+static unsigned char scatter_len_test_dst[SCATTER_LEN_TEST_DST_SIZE]
+    __attribute__((aligned(128)));
+static unsigned char scatter_len_test_dst_ref[SCATTER_LEN_TEST_DST_SIZE];
+static unsigned short region_len_test_offsets[MATRIX_SIZE]
+    __attribute__((aligned(128)));
+static unsigned short scatter_len_test_values[MATRIX_SIZE]
+    __attribute__((aligned(128)));
+
 /* optionally add sync instructions */
 #define SYNC_VECTOR 1
 
@@ -96,7 +117,7 @@ static void sync_scatter(void *addr)
      * synchronization.  Normally the dummy load would be deferred as
      * long as possible to minimize stalls.
      */
-    asm volatile("vmem(%0 + #0):scatter_release\n" : : "r"(addr));
+    asm volatile("vmem(%[addr] + #0):scatter_release\n" : : [addr] "r"(addr));
     /* use volatile to force the load */
     volatile HVX_Vector vDummy = *(HVX_Vector *)addr; vDummy = vDummy;
 #endif
@@ -855,6 +876,112 @@ void check_gather_16_32_masked(void)
                  MATRIX_SIZE * sizeof(unsigned short));
 }
 
+static void init_region_len_test_offsets(void)
+{
+    memset(region_len_test_offsets, 0, sizeof(region_len_test_offsets));
+    region_len_test_offsets[0] = 0;                      /* in region */
+    region_len_test_offsets[1] = REGION_LEN_TEST_MU - 1;  /* in region */
+    region_len_test_offsets[2] = REGION_LEN_TEST_MU;      /* in region */
+    region_len_test_offsets[3] = REGION_LEN_TEST_MU + 1;  /* dropped */
+    region_len_test_offsets[4] = 256;                     /* dropped */
+}
+
+/* vgather must drop elements whose offset is beyond the region */
+void create_gather_region_len_test(void)
+{
+    unsigned char *mapping;
+
+    mapping = mmap(NULL, 2 * GATHER_LEN_TEST_PAGE_SIZE,
+                   PROT_READ | PROT_WRITE,
+                   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    assert(mapping != MAP_FAILED);
+    assert(mprotect(mapping + GATHER_LEN_TEST_PAGE_SIZE,
+                    GATHER_LEN_TEST_PAGE_SIZE, PROT_NONE) == 0);
+    gather_len_test_src = mapping + GATHER_LEN_TEST_PAGE_SIZE - 256;
+
+    for (int i = 0; i < 256; i++) {
+        gather_len_test_src[i] = (unsigned char)(13 + 7 * i);
+    }
+    init_region_len_test_offsets();
+    memset(gather_len_test_dst, FILL_CHAR, sizeof(gather_len_test_dst));
+}
+
+/* gather with a region shorter than the source buffer, using HVX */
+void vector_gather_region_len(void)
+{
+    HVX_Vector voff = *(HVX_Vector *)region_len_test_offsets;
+
+    Q6_vgather_ARMVh((HVX_Vector *)gather_len_test_dst,
+                      (int)(uintptr_t)gather_len_test_src,
+                      REGION_LEN_TEST_MU, voff);
+
+    sync_gather(gather_len_test_dst);
+}
+
+/* gather with a region shorter than the source buffer, using C */
+void scalar_gather_region_len(unsigned char *dst)
+{
+    for (int i = 0; i < MATRIX_SIZE; i++) {
+        unsigned short off = region_len_test_offsets[i];
+        if (off <= REGION_LEN_TEST_MU) {
+            dst[2 * i] = gather_len_test_src[off];
+            dst[2 * i + 1] = gather_len_test_src[off + 1];
+        }
+    }
+}
+
+void check_gather_region_len(void)
+{
+    memset(gather_len_test_dst_ref, FILL_CHAR,
+           sizeof(gather_len_test_dst_ref));
+    scalar_gather_region_len(gather_len_test_dst_ref);
+    check_buffer(__func__, gather_len_test_dst, gather_len_test_dst_ref,
+                 sizeof(gather_len_test_dst_ref));
+}
+
+/* vscatter must drop elements whose offset is beyond the region */
+void create_scatter_region_len_test(void)
+{
+    init_region_len_test_offsets();
+    for (int i = 0; i < MATRIX_SIZE; i++) {
+        scatter_len_test_values[i] = 0x4100 + i;
+    }
+    memset(scatter_len_test_dst, FILL_CHAR, sizeof(scatter_len_test_dst));
+}
+
+/* scatter with a region shorter than the destination buffer, using HVX */
+void vector_scatter_region_len(void)
+{
+    HVX_Vector voff = *(HVX_Vector *)region_len_test_offsets;
+    HVX_Vector vval = *(HVX_Vector *)scatter_len_test_values;
+
+    Q6_vscatter_RMVhV((int)(uintptr_t)scatter_len_test_dst,
+                       REGION_LEN_TEST_MU, voff, vval);
+
+    sync_scatter(scatter_len_test_dst);
+}
+
+/* scatter with a region shorter than the destination buffer, using C */
+void scalar_scatter_region_len(unsigned char *dst)
+{
+    for (int i = 0; i < MATRIX_SIZE; i++) {
+        unsigned short off = region_len_test_offsets[i];
+        if (off <= REGION_LEN_TEST_MU) {
+            memcpy(dst + off, &scatter_len_test_values[i],
+                   sizeof(scatter_len_test_values[i]));
+        }
+    }
+}
+
+void check_scatter_region_len(void)
+{
+    memset(scatter_len_test_dst_ref, FILL_CHAR,
+           sizeof(scatter_len_test_dst_ref));
+    scalar_scatter_region_len(scatter_len_test_dst_ref);
+    check_buffer(__func__, scatter_len_test_dst, scatter_len_test_dst_ref,
+                 sizeof(scatter_len_test_dst_ref));
+}
+
 /* print scatter16 buffer */
 void print_scatter16_buffer(void)
 {
@@ -1035,6 +1162,14 @@ int main()
     print_scatter16_32_buffer();
     check_scatter_16_32_masked();
 
+    create_gather_region_len_test();
+    vector_gather_region_len();
+    check_gather_region_len();
+
+    create_scatter_region_len_test();
+    vector_scatter_region_len();
+    check_scatter_region_len();
+
     puts(err ? "FAIL" : "PASS");
     return err;
 }
-- 
2.34.1

[PATCH v3 4/5] target/hexagon: fix HVX predicate save size for histogram ops

[Brian Cain]

The masked histogram instructions save the predicate operand to the qtmp
temporary with a gvec move sized sizeof(MMVector). Both source and
destination are MMQReg, which is eight times smaller, so the move read
112 bytes past the predicate register and wrote them over the fields
following qtmp in CPUHexagonState, that is vstore[0]. Use the size of
the registers actually being copied.

Fixes: 7ba7657bc93 ("Hexagon HVX helper overrides for histogram  instructions")
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Reviewed-by: Matheus Tavares Bernardino <matheus.bernardino@oss.qualcomm.com>
Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/gen_tcg_hvx.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/hexagon/gen_tcg_hvx.h b/target/hexagon/gen_tcg_hvx.h
index 2a342cdee69..d3f2cc7bc62 100644
--- a/target/hexagon/gen_tcg_hvx.h
+++ b/target/hexagon/gen_tcg_hvx.h
@@ -50,7 +50,7 @@ static inline void assert_vhist_tmp(DisasContext *ctx)
         if (ctx->pre_commit) { \
             intptr_t dstoff = offsetof(CPUHexagonState, qtmp); \
             tcg_gen_gvec_mov(MO_64, dstoff, QvV_off, \
-                             sizeof(MMVector), sizeof(MMVector)); \
+                             sizeof(MMQReg), sizeof(MMQReg)); \
         } else { \
             assert_vhist_tmp(ctx); \
             gen_helper_vhistq(tcg_env); \
@@ -66,7 +66,7 @@ static inline void assert_vhist_tmp(DisasContext *ctx)
         if (ctx->pre_commit) { \
             intptr_t dstoff = offsetof(CPUHexagonState, qtmp); \
             tcg_gen_gvec_mov(MO_64, dstoff, QvV_off, \
-                             sizeof(MMVector), sizeof(MMVector)); \
+                             sizeof(MMQReg), sizeof(MMQReg)); \
         } else { \
             assert_vhist_tmp(ctx); \
             gen_helper_vwhist256q(tcg_env); \
@@ -82,7 +82,7 @@ static inline void assert_vhist_tmp(DisasContext *ctx)
         if (ctx->pre_commit) { \
             intptr_t dstoff = offsetof(CPUHexagonState, qtmp); \
             tcg_gen_gvec_mov(MO_64, dstoff, QvV_off, \
-                             sizeof(MMVector), sizeof(MMVector)); \
+                             sizeof(MMQReg), sizeof(MMQReg)); \
         } else { \
             assert_vhist_tmp(ctx); \
             gen_helper_vwhist256q_sat(tcg_env); \
@@ -98,7 +98,7 @@ static inline void assert_vhist_tmp(DisasContext *ctx)
         if (ctx->pre_commit) { \
             intptr_t dstoff = offsetof(CPUHexagonState, qtmp); \
             tcg_gen_gvec_mov(MO_64, dstoff, QvV_off, \
-                             sizeof(MMVector), sizeof(MMVector)); \
+                             sizeof(MMQReg), sizeof(MMQReg)); \
         } else { \
             assert_vhist_tmp(ctx); \
             gen_helper_vwhist128q(tcg_env); \
@@ -115,7 +115,7 @@ static inline void assert_vhist_tmp(DisasContext *ctx)
         if (ctx->pre_commit) { \
             intptr_t dstoff = offsetof(CPUHexagonState, qtmp); \
             tcg_gen_gvec_mov(MO_64, dstoff, QvV_off, \
-                             sizeof(MMVector), sizeof(MMVector)); \
+                             sizeof(MMQReg), sizeof(MMQReg)); \
         } else { \
             TCGv tcgv_uiV = tcg_constant_tl(uiV); \
             assert_vhist_tmp(ctx); \
-- 
2.34.1

[PATCH v3 5/5] tests/tcg/hexagon: check the two HVX predicate build paths agree

[Brian Cain]

A Q reg can be built by a vector compare or by vand(Vu, Rt), and
those reach the predicate register through independent code paths. Check
that both select the same byte lanes, so a byte-order mistake in either
one is caught.

The input alternates every eight lanes: a flatter pattern
stays unchanged when predicate bytes are permuted and hides the bug.

Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 tests/tcg/hexagon/hvx_misc.c | 45 ++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tests/tcg/hexagon/hvx_misc.c b/tests/tcg/hexagon/hvx_misc.c
index 32a3661a86f..225666137c9 100644
--- a/tests/tcg/hexagon/hvx_misc.c
+++ b/tests/tcg/hexagon/hvx_misc.c
@@ -181,6 +181,50 @@ static void test_store_unaligned(void)
     check_output_w(__LINE__, 2);
 }
 
+/*
+ * A Q register can be built either by a vector compare or by vand(Vu, Rt).
+ * Those are two independent code paths onto the same predicate-register
+ * layout, so cross-check that they select exactly the same byte lanes.
+ * A byte-order mistake in either one shows up here as a mismatch.
+ *
+ * The input pattern has to vary across 8-lane blocks, otherwise a predicate
+ * whose bytes are permuted still selects the same lanes and the mismatch is
+ * invisible.
+ */
+static void test_qreg_alias(void)
+{
+    HVX_Vector *pcmp = (HVX_Vector *)&output[0];
+    HVX_Vector *pand = (HVX_Vector *)&output[1];
+    HVX_Vector input;
+    HVX_Vector ones;
+    HVX_VectorPred qcmp;
+    HVX_VectorPred qand;
+
+    for (int i = 0; i < BUFSIZE; i++) {
+        /*
+         * Build 0/1 per byte, alternating every 8 lanes, so that "!= 0" and
+         * "low bit set" describe the same lanes.  Then form one predicate
+         * with a compare and the other with vand(Vu, Rt), store 0xff through
+         * each, and require the two result vectors to be identical.
+         */
+        for (int j = 0; j < MAX_VEC_SIZE_BYTES; j++) {
+            expect[0].ub[j] = ((j >> 3) + i) & 1;
+        }
+        memcpy(&input, &expect[0], sizeof(MMVector));
+        memset(output, 0, 2 * sizeof(MMVector));
+
+        ones = Q6_V_vsplat_R(0xffffffff);
+        qcmp = Q6_Q_vcmp_gt_VubVub(input, Q6_V_vzero());
+        qand = Q6_Q_vand_VR(input, 0x01010101);
+        Q6_vmem_QRIV(qcmp, pcmp, ones);
+        Q6_vmem_QRIV(qand, pand, ones);
+
+        for (int j = 0; j < MAX_VEC_SIZE_BYTES; j++) {
+            check(__LINE__, i, j, output[0].ub[j], output[1].ub[j]);
+        }
+    }
+}
+
 static void test_masked_store(bool invert)
 {
     void *p0 = buffer0;
@@ -579,6 +623,7 @@ int main()
     test_store_unaligned();
     test_masked_store(false);
     test_masked_store(true);
+    test_qreg_alias();
     test_new_value_store();
     test_max_temps();
 
-- 
2.34.1

Re: [PATCH v3 3/5] tests/tcg/hexagon: add vgather/vscatter region-length tests

[Pierrick Bouvier]

On 9/3/26 6:22 PM, Brian Cain wrote:
> Cover the case where an offset lies beyond the region length: it
> must be dropped rather than read/written.
> 
> Place one dropped gather offset on a PROT_NONE page to verify that
> the instruction does not access memory for that lane.
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>   tests/tcg/hexagon/scatter_gather.c | 137 ++++++++++++++++++++++++++++-
>   1 file changed, 136 insertions(+), 1 deletion(-)
> 

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>

Re: [PATCH v3 5/5] tests/tcg/hexagon: check the two HVX predicate build paths agree

[Pierrick Bouvier]

On 9/3/26 6:22 PM, Brian Cain wrote:
> A Q reg can be built by a vector compare or by vand(Vu, Rt), and
> those reach the predicate register through independent code paths. Check
> that both select the same byte lanes, so a byte-order mistake in either
> one is caught.
> 
> The input alternates every eight lanes: a flatter pattern
> stays unchanged when predicate bytes are permuted and hides the bug.
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>   tests/tcg/hexagon/hvx_misc.c | 45 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
> 

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>

Re: [PATCH v3 2/5] target/hexagon: fix HVX scatter/gather region-length check

[Philippe Mathieu-Daudé]

On 4/9/26 03:22, Brian Cain wrote:
> The region-length bounds check compared (EA + i0) <= (EA + LEN),
> which reduces to i0 <= LEN and never actually depends on the region
> base. This let vgather/vscatter keep elements whose offset was
> beyond the declared region instead of dropping them. Compare the
> offset to the region length directly instead.
> 
> Only load bytes for gather lanes that are kept. A dropped or
> predicate-false lane must not access memory and raise an exception.
> 
> Reviewed-by: Matheus Tavares Bernardino <matheus.bernardino@oss.qualcomm.com>
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>   target/hexagon/mmvec/macros.h | 31 +++++++++++++++++--------------
>   1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
> index e85beed3387..858afd995d9 100644
> --- a/target/hexagon/mmvec/macros.h
> +++ b/target/hexagon/mmvec/macros.h
> @@ -130,9 +130,9 @@
>       do { \
>           int log_byte = 0; \
>           target_ulong va = EA; \
> -        target_ulong va_high = EA + LEN; \
> +        int in_region = (OFFSET) <= (LEN); \

I'm a bit confused here (and hopefully wrong), shouldn't it be
(OFFSET) < (LEN), otherwise the byte accessed is out of the limit?

(pattern used multiple times in this patch)

Re: [PATCH v3 2/5] target/hexagon: fix HVX scatter/gather region-length check

[Brian Cain]

On 9/4/2026 5:33 AM, Philippe Mathieu-Daudé wrote:
> On 4/9/26 03:22, Brian Cain wrote:
>> The region-length bounds check compared (EA + i0) <= (EA + LEN),
>> which reduces to i0 <= LEN and never actually depends on the region
>> base. This let vgather/vscatter keep elements whose offset was
>> beyond the declared region instead of dropping them. Compare the
>> offset to the region length directly instead.
>>
>> Only load bytes for gather lanes that are kept. A dropped or
>> predicate-false lane must not access memory and raise an exception.
>>
>> Reviewed-by: Matheus Tavares Bernardino 
>> <matheus.bernardino@oss.qualcomm.com>
>> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
>> ---
>>   target/hexagon/mmvec/macros.h | 31 +++++++++++++++++--------------
>>   1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/hexagon/mmvec/macros.h 
>> b/target/hexagon/mmvec/macros.h
>> index e85beed3387..858afd995d9 100644
>> --- a/target/hexagon/mmvec/macros.h
>> +++ b/target/hexagon/mmvec/macros.h
>> @@ -130,9 +130,9 @@
>>       do { \
>>           int log_byte = 0; \
>>           target_ulong va = EA; \
>> -        target_ulong va_high = EA + LEN; \
>> +        int in_region = (OFFSET) <= (LEN); \
>
> I'm a bit confused here (and hopefully wrong), shouldn't it be
> (OFFSET) < (LEN), otherwise the byte accessed is out of the limit?


Yeah, it's confusing as-is.  I'll change the code to make it clearer.  
s/LEN/OFFS_REG_END/ should make the code look more conventional, I think.


HVX PRM 
https://docs.qualcomm.com/doc/80-N2040-54/80-N2040-54_REV_AB_Qualcomm_Hexagon_V73_HVX_Programmers_Reference_Manual.pdf

Refer to section 5.3, "Scatter and Gather" Table 5-3 - "Mu: Byte offset 
of last valid byte of the region (for example, region size - 1)" - the 
Mu value is what's been interpreted here as "LEN."


>
> (pattern used multiple times in this patch)