[PATCH qemu v2 0/2] target/arm: Improve user-mode compatibility with JITs

[PATCH qemu v2 0/2] target/arm: Improve user-mode compatibility with JITs

[~jhogberg]

The previous version of this got mangled, so I'm re-sending it through
sourcehut as mentioned in the documentation in the hopes that it's
foolproof. Sorry about the extra traffic :-(

----

When running in user-mode QEMU currently fails to emulate JITs that
use dual-mapped code to get around W^X restrictions, where one mapping
is writable and one is executable. As it has no way of knowing that a
write to the writable region is reflected in the executable one, it
fails to invalidate previously translated code which leads to a crash
at best.

(Note that system mode is unaffected as the softmmu is fully aware of
what is going on.)

This patch series catches changes to dual-mapped code by honoring the
cache management instructions required to make things work on actual
hardware.

See https://gitlab.com/qemu-project/qemu/-/issues/1034 for more
background information

John Högberg (2):
  target/arm: Handle IC IVAU to improve compatibility with JITs
  tests/tcg/aarch64: Add testcases for IC IVAU and dual-mapped code

 target/arm/helper.c               |  47 ++++++-
 tests/tcg/aarch64/Makefile.target |   3 +-
 tests/tcg/aarch64/icivau.c        | 204 ++++++++++++++++++++++++++++++
 3 files changed, 250 insertions(+), 4 deletions(-)
 create mode 100644 tests/tcg/aarch64/icivau.c

-- 
2.38.5

[PATCH qemu v2 1/2] target/arm: Handle IC IVAU to improve compatibility with JITs

[~jhogberg]

From: John Högberg <john.hogberg@ericsson.com>

Unlike architectures with precise self-modifying code semantics
(e.g. x86) ARM processors do not maintain coherency for instruction
execution and memory, and require the explicit use of cache
management instructions as well as an instruction barrier to make
code updates visible (the latter on every core that is going to
execute said code).

While this is required to make JITs work on actual hardware, QEMU
has gotten away with not handling this since it does not emulate
caches, and unconditionally invalidates code whenever the softmmu
or the user-mode page protection logic detects that code has been
modified.

Unfortunately the latter does not work in the face of dual-mapped
code (a common W^X workaround), where one page is executable and
the other is writable: user-mode has no way to connect one with the
other as that is only known to the kernel and the emulated
application.

This commit works around the issue by invalidating code in
IC IVAU instructions.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1034

Co-authored-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: John Högberg <john.hogberg@ericsson.com>
---
 target/arm/helper.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d4bee43bd0..235e3cd0b6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5228,6 +5228,36 @@ static void mdcr_el2_write(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 }
 
+#ifdef CONFIG_USER_ONLY
+/*
+ * `IC IVAU` is handled to improve compatibility with JITs that dual-map their
+ * code to get around W^X restrictions, where one region is writable and the
+ * other is executable.
+ *
+ * Since the executable region is never written to we cannot detect code
+ * changes when running in user mode, and rely on the emulated JIT telling us
+ * that the code has changed by executing this instruction.
+ */
+static void ic_ivau_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                          uint64_t value)
+{
+    uint64_t icache_line_mask, start_address, end_address;
+    const ARMCPU *cpu;
+
+    cpu = env_archcpu(env);
+
+    icache_line_mask = (4 << extract32(cpu->ctr, 0, 4)) - 1;
+    start_address = value & ~icache_line_mask;
+    end_address = value | icache_line_mask;
+
+    mmap_lock();
+
+    tb_invalidate_phys_range(start_address, end_address);
+
+    mmap_unlock();
+}
+#endif
+
 static const ARMCPRegInfo v8_cp_reginfo[] = {
     /*
      * Minimal set of EL0-visible registers. This will need to be expanded
@@ -5267,7 +5297,10 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
     { .name = "CURRENTEL", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 0, .opc2 = 2, .crn = 4, .crm = 2,
       .access = PL1_R, .type = ARM_CP_CURRENTEL },
-    /* Cache ops: all NOPs since we don't emulate caches */
+    /*
+     * Instruction cache ops. All of these except `IC IVAU` NOP because we
+     * don't emulate caches.
+     */
     { .name = "IC_IALLUIS", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 1, .opc2 = 0,
       .access = PL1_W, .type = ARM_CP_NOP,
@@ -5280,9 +5313,17 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
       .accessfn = access_tocu },
     { .name = "IC_IVAU", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 3, .crn = 7, .crm = 5, .opc2 = 1,
-      .access = PL0_W, .type = ARM_CP_NOP,
+      .access = PL0_W,
       .fgt = FGT_ICIVAU,
-      .accessfn = access_tocu },
+      .accessfn = access_tocu,
+#ifdef CONFIG_USER_ONLY
+      .type = ARM_CP_NO_RAW,
+      .writefn = ic_ivau_write
+#else
+      .type = ARM_CP_NOP
+#endif
+    },
+    /* Cache ops: all NOPs since we don't emulate caches */
     { .name = "DC_IVAC", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 6, .opc2 = 1,
       .access = PL1_W, .accessfn = aa64_cacheop_poc_access,
-- 
2.38.5

Re: [PATCH qemu v2 1/2] target/arm: Handle IC IVAU to improve compatibility with JITs

[Richard Henderson]

On 6/8/23 19:49, ~jhogberg wrote:
> From: John Högberg<john.hogberg@ericsson.com>
> 
> Unlike architectures with precise self-modifying code semantics
> (e.g. x86) ARM processors do not maintain coherency for instruction
> execution and memory, and require the explicit use of cache
> management instructions as well as an instruction barrier to make
> code updates visible (the latter on every core that is going to
> execute said code).
> 
> While this is required to make JITs work on actual hardware, QEMU
> has gotten away with not handling this since it does not emulate
> caches, and unconditionally invalidates code whenever the softmmu
> or the user-mode page protection logic detects that code has been
> modified.
> 
> Unfortunately the latter does not work in the face of dual-mapped
> code (a common W^X workaround), where one page is executable and
> the other is writable: user-mode has no way to connect one with the
> other as that is only known to the kernel and the emulated
> application.
> 
> This commit works around the issue by invalidating code in
> IC IVAU instructions.
> 
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1034
> 
> Co-authored-by: Richard Henderson<richard.henderson@linaro.org>
> Signed-off-by: John Högberg<john.hogberg@ericsson.com>
> ---
>   target/arm/helper.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 44 insertions(+), 3 deletions(-)

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

r~

[PATCH qemu v2 2/2] tests/tcg/aarch64: Add testcases for IC IVAU and dual-mapped code

[~jhogberg]

From: John Högberg <john.hogberg@ericsson.com>

https://gitlab.com/qemu-project/qemu/-/issues/1034

Signed-off-by: John Högberg <john.hogberg@ericsson.com>
---
 tests/tcg/aarch64/Makefile.target |   3 +-
 tests/tcg/aarch64/icivau.c        | 204 ++++++++++++++++++++++++++++++
 2 files changed, 206 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/aarch64/icivau.c

diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 3430fd3cd8..de6566d0d4 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -9,9 +9,10 @@ AARCH64_SRC=$(SRC_PATH)/tests/tcg/aarch64
 VPATH 		+= $(AARCH64_SRC)
 
 # Base architecture tests
-AARCH64_TESTS=fcvt pcalign-a64
+AARCH64_TESTS=fcvt pcalign-a64 icivau
 
 fcvt: LDFLAGS+=-lm
+icivau: LDFLAGS+=-lrt
 
 run-fcvt: fcvt
 	$(call run-test,$<,$(QEMU) $<, "$< on $(TARGET_NAME)")
diff --git a/tests/tcg/aarch64/icivau.c b/tests/tcg/aarch64/icivau.c
new file mode 100644
index 0000000000..ff80d3d868
--- /dev/null
+++ b/tests/tcg/aarch64/icivau.c
@@ -0,0 +1,204 @@
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+#define PAYLOAD_SIZE (256)
+
+typedef int (*SelfModTestPtr)(char *, const char*, int);
+typedef int (*CompareTestPtr)(int, int);
+
+void flush_icache(const char *exec_data, size_t length)
+{
+    size_t dcache_stride, icache_stride, i;
+    unsigned long ctr_el0;
+
+    /*
+     * Step according to minimum cache sizes, as the cache maintenance
+     * instructions operate on the cache line of the given address.
+     *
+     * We assume that exec_data is properly aligned.
+     */
+    __asm__("mrs %0, ctr_el0\n" : "=r"(ctr_el0));
+    dcache_stride = (4 << ((ctr_el0 >> 16) & 0xF));
+    icache_stride = (4 << (ctr_el0 & 0xF));
+
+    for (i = 0; i < length; i += dcache_stride) {
+        const char *dc_addr = &exec_data[i];
+        __asm__ ("dc cvau, %x[dc_addr]\n"
+                 : /* no outputs */
+                 : [dc_addr] "r"(dc_addr)
+                 : "memory");
+    }
+
+    __asm__ ("dmb ish\n");
+
+    for (i = 0; i < length; i += icache_stride) {
+        const char *ic_addr = &exec_data[i];
+        __asm__ ("ic ivau, %x[ic_addr]\n"
+                 : /* no outputs */
+                 : [ic_addr] "r"(ic_addr)
+                 : "memory");
+    }
+
+    __asm__ ("dmb ish\n"
+             "isb sy\n");
+}
+
+/*
+ * The unmodified assembly of this function returns 0, it self-modifies to
+ * return the value indicated by new_move.
+ */
+int self_modification_payload(char *rw_data, const char *exec_data,
+                              int new_move)
+{
+    register int result __asm__ ("w0") = new_move;
+
+    __asm__ (/* Get the writable address of __modify_me. */
+             "sub %x[rw_data], %x[rw_data], %x[exec_data]\n"
+             "adr %x[exec_data], __modify_me\n"
+             "add %x[rw_data], %x[rw_data], %x[exec_data]\n"
+             /* Overwrite the `MOV W0, #0` with the new move. */
+             "str %w[result], [%x[rw_data]]\n"
+             /*
+              * Mark the code as modified.
+              *
+              * Note that we align to the nearest 64 bytes in an attempt to put
+              * the flush sequence in the same cache line as the modified move.
+              */
+             ".align 6\n"
+             "dc cvau, %x[exec_data]\n"
+             ".align 2\n"
+             "dmb ish\n"
+             "ic ivau, %x[exec_data]\n"
+             "dmb ish\n"
+             "isb sy\n"
+             "__modify_me: mov w0, #0x0\n"
+             : [result] "+r"(result),
+               [rw_data] "+r"(rw_data),
+               [exec_data] "+r"(exec_data)
+             : /* No untouched inputs */
+             : "memory");
+
+    return result;
+}
+
+int self_modification_test(char *rw_data, const char *exec_data)
+{
+    SelfModTestPtr copied_ptr = (SelfModTestPtr)exec_data;
+    int i;
+
+    /*
+     * Bluntly assumes that the payload is position-independent and not larger
+     * than PAYLOAD_SIZE.
+     */
+    memcpy(rw_data, self_modification_payload, PAYLOAD_SIZE);
+
+    /*
+     * Notify all PEs that the code at exec_data has been altered.
+     *
+     * For completeness we could assert that we should fail when this is
+     * omitted, which works in user mode and on actual hardware as the
+     * modification won't "take," but doesn't work in system mode as the
+     * softmmu handles everything for us.
+     */
+    flush_icache(exec_data, PAYLOAD_SIZE);
+
+    for (i = 1; i < 10; i++) {
+        const int mov_w0_template = 0x52800000;
+
+        /* MOV W0, i */
+        if (copied_ptr(rw_data, exec_data, mov_w0_template | (i << 5)) != i) {
+            return 0;
+        }
+    }
+
+    return 1;
+}
+
+int compare_copied(char *rw_data, const char *exec_data,
+                   int (*reference_ptr)(int, int))
+{
+    CompareTestPtr copied_ptr = (CompareTestPtr)exec_data;
+    int a, b;
+
+    memcpy(rw_data, reference_ptr, PAYLOAD_SIZE);
+    flush_icache(exec_data, PAYLOAD_SIZE);
+
+    for (a = 1; a < 10; a++) {
+        for (b = 1; b < 10; b++) {
+            if (copied_ptr(a, b) != reference_ptr(a, b)) {
+                return 0;
+            }
+        }
+    }
+
+    return 1;
+}
+
+int compare_alpha(int a, int b)
+{
+    return a + b;
+}
+
+int compare_beta(int a, int b)
+{
+    return a - b;
+}
+
+int compare_gamma(int a, int b)
+{
+    return a * b;
+}
+
+int compare_delta(int a, int b)
+{
+    return a / b;
+}
+
+int main(int argc, char **argv)
+{
+    const char *shm_name = "qemu-test-tcg-aarch64-icivau";
+    int fd;
+
+    fd = shm_open(shm_name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
+
+    if (fd < 0) {
+        return EXIT_FAILURE;
+    }
+
+    /* Unlink early to avoid leaving garbage in case the test crashes. */
+    shm_unlink(shm_name);
+
+    if (ftruncate(fd, PAYLOAD_SIZE) == 0) {
+        const char *exec_data;
+        char *rw_data;
+
+        rw_data = mmap(0, PAYLOAD_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+                       fd, 0);
+        exec_data = mmap(0, PAYLOAD_SIZE, PROT_READ | PROT_EXEC, MAP_SHARED,
+                         fd, 0);
+
+        if (rw_data && exec_data) {
+            CompareTestPtr compare_tests[4] = {compare_alpha,
+                                               compare_beta,
+                                               compare_gamma,
+                                               compare_delta};
+            int success, i;
+
+            success = self_modification_test(rw_data, exec_data);
+
+            for (i = 0; i < 4; i++) {
+                success &= compare_copied(rw_data, exec_data, compare_tests[i]);
+            }
+
+            if (success) {
+                return EXIT_SUCCESS;
+            }
+        }
+    }
+
+    return EXIT_FAILURE;
+}
-- 
2.38.5

Re: [PATCH qemu v2 2/2] tests/tcg/aarch64: Add testcases for IC IVAU and dual-mapped code

[Peter Maydell]

On Mon, 12 Jun 2023 at 10:40, ~jhogberg <jhogberg@git.sr.ht> wrote:
>
> From: John Högberg <john.hogberg@ericsson.com>
>
> https://gitlab.com/qemu-project/qemu/-/issues/1034
>
> Signed-off-by: John Högberg <john.hogberg@ericsson.com>

> diff --git a/tests/tcg/aarch64/icivau.c b/tests/tcg/aarch64/icivau.c
> new file mode 100644
> index 0000000000..ff80d3d868
> --- /dev/null
> +++ b/tests/tcg/aarch64/icivau.c
> @@ -0,0 +1,204 @@
> +#include <sys/mman.h>

All new source files must start with a license-and-copyright
comment.

> +#include <sys/stat.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +
> +#define PAYLOAD_SIZE (256)
> +
> +typedef int (*SelfModTestPtr)(char *, const char*, int);
> +typedef int (*CompareTestPtr)(int, int);

Generally in QEMU we prefer to typedef the function type,
not the pointer-to-function type.

> +
> +void flush_icache(const char *exec_data, size_t length)
> +{
> +    size_t dcache_stride, icache_stride, i;
> +    unsigned long ctr_el0;
> +
> +    /*
> +     * Step according to minimum cache sizes, as the cache maintenance
> +     * instructions operate on the cache line of the given address.
> +     *
> +     * We assume that exec_data is properly aligned.
> +     */
> +    __asm__("mrs %0, ctr_el0\n" : "=r"(ctr_el0));
> +    dcache_stride = (4 << ((ctr_el0 >> 16) & 0xF));
> +    icache_stride = (4 << (ctr_el0 & 0xF));
> +
> +    for (i = 0; i < length; i += dcache_stride) {
> +        const char *dc_addr = &exec_data[i];
> +        __asm__ ("dc cvau, %x[dc_addr]\n"
> +                 : /* no outputs */
> +                 : [dc_addr] "r"(dc_addr)
> +                 : "memory");
> +    }
> +
> +    __asm__ ("dmb ish\n");

You should probably mark all these asm statements as 'volatile'
to ensure that the compiler doesn't decide it can helpfully
reorder or remove them.

> +
> +    for (i = 0; i < length; i += icache_stride) {
> +        const char *ic_addr = &exec_data[i];
> +        __asm__ ("ic ivau, %x[ic_addr]\n"
> +                 : /* no outputs */
> +                 : [ic_addr] "r"(ic_addr)
> +                 : "memory");
> +    }
> +
> +    __asm__ ("dmb ish\n"
> +             "isb sy\n");
> +}
> +
> +/*
> + * The unmodified assembly of this function returns 0, it self-modifies to
> + * return the value indicated by new_move.
> + */
> +int self_modification_payload(char *rw_data, const char *exec_data,
> +                              int new_move)
> +{
> +    register int result __asm__ ("w0") = new_move;

Why use an explicit register variable for this rather than
having the __asm__ return its result via an output ?

> +
> +    __asm__ (/* Get the writable address of __modify_me. */
> +             "sub %x[rw_data], %x[rw_data], %x[exec_data]\n"
> +             "adr %x[exec_data], __modify_me\n"
> +             "add %x[rw_data], %x[rw_data], %x[exec_data]\n"
> +             /* Overwrite the `MOV W0, #0` with the new move. */
> +             "str %w[result], [%x[rw_data]]\n"
> +             /*
> +              * Mark the code as modified.
> +              *
> +              * Note that we align to the nearest 64 bytes in an attempt to put
> +              * the flush sequence in the same cache line as the modified move.
> +              */

This is a QEMU test case, though, and QEMU doesn't care about
cache lines because we don't model the cache. So why does it
matter ?

> +             ".align 6\n"
> +             "dc cvau, %x[exec_data]\n"
> +             ".align 2\n"
> +             "dmb ish\n"
> +             "ic ivau, %x[exec_data]\n"
> +             "dmb ish\n"
> +             "isb sy\n"
> +             "__modify_me: mov w0, #0x0\n"
> +             : [result] "+r"(result),
> +               [rw_data] "+r"(rw_data),
> +               [exec_data] "+r"(exec_data)
> +             : /* No untouched inputs */
> +             : "memory");
> +
> +    return result;
> +}
> +
> +int self_modification_test(char *rw_data, const char *exec_data)
> +{
> +    SelfModTestPtr copied_ptr = (SelfModTestPtr)exec_data;
> +    int i;
> +
> +    /*
> +     * Bluntly assumes that the payload is position-independent and not larger
> +     * than PAYLOAD_SIZE.
> +     */
> +    memcpy(rw_data, self_modification_payload, PAYLOAD_SIZE);

Your asm code may be position-independent, but you're copying the
entire function here including the preamble and postamble that the
compiler emits for it, and you don't do anything in the makefile
to ensure that it's going to be position-independent.

> +
> +    /*
> +     * Notify all PEs that the code at exec_data has been altered.
> +     *
> +     * For completeness we could assert that we should fail when this is
> +     * omitted, which works in user mode and on actual hardware as the
> +     * modification won't "take," but doesn't work in system mode as the
> +     * softmmu handles everything for us.
> +     */

There's no guarantee on actual hardware that omitting this
flush will cause the test to fail. The hardware implementation
is permitted to drop stuff from the cache any time it feels
like it, and it might choose to do that right at this point.
So any attempt to test "fails if we don't flush the icache"
would be a flaky test. It would also fail on a hardware
implementation where the icache and dcache are transparently
coherent (which is a permitted implementation; the CTR_EL0
DIC and IDC bits exist to tell software it can happily
skip the cache ops).

System mode QEMU works because we model an implementation
with no caches (which is also permitted architecturally).

> +    flush_icache(exec_data, PAYLOAD_SIZE);
> +
> +    for (i = 1; i < 10; i++) {
> +        const int mov_w0_template = 0x52800000;
> +
> +        /* MOV W0, i */
> +        if (copied_ptr(rw_data, exec_data, mov_w0_template | (i << 5)) != i) {
> +            return 0;
> +        }
> +    }
> +
> +    return 1;
> +}
> +
> +int compare_copied(char *rw_data, const char *exec_data,
> +                   int (*reference_ptr)(int, int))
> +{
> +    CompareTestPtr copied_ptr = (CompareTestPtr)exec_data;
> +    int a, b;
> +
> +    memcpy(rw_data, reference_ptr, PAYLOAD_SIZE);
> +    flush_icache(exec_data, PAYLOAD_SIZE);
> +
> +    for (a = 1; a < 10; a++) {
> +        for (b = 1; b < 10; b++) {
> +            if (copied_ptr(a, b) != reference_ptr(a, b)) {
> +                return 0;
> +            }
> +        }
> +    }
> +
> +    return 1;
> +}
> +
> +int compare_alpha(int a, int b)
> +{
> +    return a + b;
> +}
> +
> +int compare_beta(int a, int b)
> +{
> +    return a - b;
> +}
> +
> +int compare_gamma(int a, int b)
> +{
> +    return a * b;
> +}
> +
> +int compare_delta(int a, int b)
> +{
> +    return a / b;
> +}

I don't understand the purpose of all these functions.
I was expecting the test to be something like
 * write initial code
 * execute it
 * modify it
 * execute again and check we got the changed version

I don't see why we need four "compare" functions to do that.

> +int main(int argc, char **argv)
> +{
> +    const char *shm_name = "qemu-test-tcg-aarch64-icivau";
> +    int fd;
> +
> +    fd = shm_open(shm_name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
> +
> +    if (fd < 0) {
> +        return EXIT_FAILURE;
> +    }
> +
> +    /* Unlink early to avoid leaving garbage in case the test crashes. */
> +    shm_unlink(shm_name);
> +
> +    if (ftruncate(fd, PAYLOAD_SIZE) == 0) {
> +        const char *exec_data;
> +        char *rw_data;
> +
> +        rw_data = mmap(0, PAYLOAD_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
> +                       fd, 0);
> +        exec_data = mmap(0, PAYLOAD_SIZE, PROT_READ | PROT_EXEC, MAP_SHARED,
> +                         fd, 0);
> +
> +        if (rw_data && exec_data) {
> +            CompareTestPtr compare_tests[4] = {compare_alpha,
> +                                               compare_beta,
> +                                               compare_gamma,
> +                                               compare_delta};
> +            int success, i;
> +
> +            success = self_modification_test(rw_data, exec_data);
> +
> +            for (i = 0; i < 4; i++) {
> +                success &= compare_copied(rw_data, exec_data, compare_tests[i]);
> +            }
> +
> +            if (success) {
> +                return EXIT_SUCCESS;
> +            }
> +        }
> +    }
> +
> +    return EXIT_FAILURE;
> +}
> --
> 2.38.5

thanks
-- PMM

Re: [PATCH qemu v2 2/2] tests/tcg/aarch64: Add testcases for IC IVAU and dual-mapped code

[John Högberg]

Thanks for the review! :)

> All new source files must start with a license-and-copyright
comment.
> ... snip ...
> Generally in QEMU we prefer to typedef the function type,
not the pointer-to-function type.
> ... snip ...
> You should probably mark all these asm statements as 'volatile'
> to ensure that the compiler doesn't decide it can helpfully
> reorder or remove them.

Got it, I'll fix those.

> Your asm code may be position-independent, but you're copying the
> entire function here including the preamble and postamble that the
> compiler emits for it, and you don't do anything in the makefile
> to ensure that it's going to be position-independent.
> ... snip ...
> Why use an explicit register variable for this rather than
> having the __asm__ return its result via an output ?

Good point, I'll rewrite this part to avoid these issues.

> This is a QEMU test case, though, and QEMU doesn't care about
> cache lines because we don't model the cache. So why does it
> matter ?

We're trying to catch code modification through the use of instructions
that invalidate cache lines. I wanted the test to cover invalidation of
the code that does the invalidation itself too as "what happens if we
invalidate the current TB and return" came up in the discussion on the
bug tracker, but I can certainly cut this or expand on it in a comment
if you wish.

> There's no guarantee on actual hardware that omitting this
> flush will cause the test to fail. The hardware implementation
> is permitted to drop stuff from the cache any time it feels
> like it, and it might choose to do that right at this point.
> So any attempt to test "fails if we don't flush the icache"
> would be a flaky test. It would also fail on a hardware
> implementation where the icache and dcache are transparently
> coherent (which is a permitted implementation; the CTR_EL0
> DIC and IDC bits exist to tell software it can happily
> skip the cache ops).
>
> System mode QEMU works because we model an implementation
> with no caches (which is also permitted architecturally).

True, do you want me to change the comment to this effect or cut the
comment altogether?

> I don't understand the purpose of all these functions.
> I was expecting the test to be something like
>  * write initial code
>  * execute it
>  * modify it
>  * execute again and check we got the changed version
>
> I don't see why we need four "compare" functions to do that.

Sure, the self-modification test alone should suffice.

Thanks again :)

Regards,
John Högberg

Re: [PATCH qemu v2 2/2] tests/tcg/aarch64: Add testcases for IC IVAU and dual-mapped code

[Peter Maydell]

On Mon, 19 Jun 2023 at 15:31, John Högberg <john.hogberg@ericsson.com> wrote:
>
> Thanks for the review! :)
>
> > All new source files must start with a license-and-copyright
> comment.
> > ... snip ...
> > Generally in QEMU we prefer to typedef the function type,
> not the pointer-to-function type.
> > ... snip ...
> > You should probably mark all these asm statements as 'volatile'
> > to ensure that the compiler doesn't decide it can helpfully
> > reorder or remove them.
>
> Got it, I'll fix those.
>
> > Your asm code may be position-independent, but you're copying the
> > entire function here including the preamble and postamble that the
> > compiler emits for it, and you don't do anything in the makefile
> > to ensure that it's going to be position-independent.
> > ... snip ...
> > Why use an explicit register variable for this rather than
> > having the __asm__ return its result via an output ?
>
> Good point, I'll rewrite this part to avoid these issues.
>
> > This is a QEMU test case, though, and QEMU doesn't care about
> > cache lines because we don't model the cache. So why does it
> > matter ?
>
> We're trying to catch code modification through the use of instructions
> that invalidate cache lines. I wanted the test to cover invalidation of
> the code that does the invalidation itself too as "what happens if we
> invalidate the current TB and return" came up in the discussion on the
> bug tracker, but I can certainly cut this or expand on it in a comment
> if you wish.

I think expanding it a little would help.

> > There's no guarantee on actual hardware that omitting this
> > flush will cause the test to fail. The hardware implementation
> > is permitted to drop stuff from the cache any time it feels
> > like it, and it might choose to do that right at this point.
> > So any attempt to test "fails if we don't flush the icache"
> > would be a flaky test. It would also fail on a hardware
> > implementation where the icache and dcache are transparently
> > coherent (which is a permitted implementation; the CTR_EL0
> > DIC and IDC bits exist to tell software it can happily
> > skip the cache ops).
> >
> > System mode QEMU works because we model an implementation
> > with no caches (which is also permitted architecturally).
>
> True, do you want me to change the comment to this effect or cut the
> comment altogether?

Similarly here tweaking the comment would be enough.

thanks
-- PMM