1 | Small target-arm queue for 2.9: just the patches | 1 | The following changes since commit e3debd5e7d0ce031356024878a0a18b9d109354a: |
---|---|---|---|
2 | which fix bugs in our MRS/MSR decoding for M profile, | ||
3 | including a fix for a regression introduced in commit | ||
4 | 58117c9bb429cd. | ||
5 | 2 | ||
6 | thanks | 3 | Merge tag 'pull-request-2023-03-24' of https://gitlab.com/thuth/qemu into staging (2023-03-24 16:08:46 +0000) |
7 | -- PMM | ||
8 | 4 | ||
9 | The following changes since commit 00e7c07b06d004cf54b19724f82afde8a7a37f37: | 5 | are available in the Git repository at: |
10 | 6 | ||
11 | Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20170320' into staging (2017-03-20 10:51:30 +0000) | 7 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20230328 |
12 | 8 | ||
13 | are available in the git repository at: | 9 | for you to fetch changes up to 46e3b237c52e0c48bfd81bce020b51fbe300b23a: |
14 | 10 | ||
15 | git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20170320 | 11 | target/arm/gdbstub: Only advertise M-profile features if TCG available (2023-03-28 10:53:40 +0100) |
16 | |||
17 | for you to fetch changes up to b28b3377d7e9ba35611d454d5a63ef50cab1f8c5: | ||
18 | |||
19 | arm: Fix APSR writes via M profile MSR (2017-03-20 12:41:44 +0000) | ||
20 | 12 | ||
21 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
22 | target-arm queue: | 14 | target-arm queue: |
23 | * fix MSR/MRS decoding for M profile CPUs | 15 | * fix part of the "TCG-disabled builds are broken" issue |
24 | 16 | ||
25 | ---------------------------------------------------------------- | 17 | ---------------------------------------------------------------- |
26 | Peter Maydell (4): | 18 | Philippe Mathieu-Daudé (1): |
27 | arm: HVC and SMC encodings don't exist for M profile | 19 | target/arm/gdbstub: Only advertise M-profile features if TCG available |
28 | arm: Don't decode MRS(banked) or MSR(banked) for M profile | ||
29 | arm: Enforce should-be-1 bits in MRS decoding | ||
30 | arm: Fix APSR writes via M profile MSR | ||
31 | 20 | ||
32 | target/arm/helper.c | 26 ++++++++++++++++++++++---- | 21 | target/arm/gdbstub.c | 5 +++-- |
33 | target/arm/translate.c | 26 +++++++++++++++++++++++--- | 22 | 1 file changed, 3 insertions(+), 2 deletions(-) |
34 | 2 files changed, 45 insertions(+), 7 deletions(-) | ||
35 | 23 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | M profile doesn't have the HVC or SMC encodings, so make them always | ||
2 | UNDEF rather than generating calls to helper functions that assume | ||
3 | A/R profile. | ||
4 | 1 | ||
5 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
6 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
7 | Message-id: 1487616072-9226-2-git-send-email-peter.maydell@linaro.org | ||
8 | --- | ||
9 | target/arm/translate.c | 3 +++ | ||
10 | 1 file changed, 3 insertions(+) | ||
11 | |||
12 | diff --git a/target/arm/translate.c b/target/arm/translate.c | ||
13 | index XXXXXXX..XXXXXXX 100644 | ||
14 | --- a/target/arm/translate.c | ||
15 | +++ b/target/arm/translate.c | ||
16 | @@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw | ||
17 | goto illegal_op; | ||
18 | |||
19 | if (insn & (1 << 26)) { | ||
20 | + if (arm_dc_feature(s, ARM_FEATURE_M)) { | ||
21 | + goto illegal_op; | ||
22 | + } | ||
23 | if (!(insn & (1 << 20))) { | ||
24 | /* Hypervisor call (v7) */ | ||
25 | int imm16 = extract32(insn, 16, 4) << 12 | ||
26 | -- | ||
27 | 2.7.4 | ||
28 | |||
29 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | M profile doesn't have the MSR(banked) and MRS(banked) instructions | ||
2 | and uses the encodings for different kinds of M-profile MRS/MSR. | ||
3 | Guard the relevant bits of the decode logic to make sure we don't | ||
4 | accidentally fall into them by accident on M-profile. | ||
5 | 1 | ||
6 | (The bit being checked for this (bit 5) is part of the SYSm field on | ||
7 | M-profile, but since no currently allocated system registers have | ||
8 | encodings with bit 5 of SYSm set, this hasn't been a problem in | ||
9 | practice.) | ||
10 | |||
11 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
12 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
13 | Message-id: 1487616072-9226-3-git-send-email-peter.maydell@linaro.org | ||
14 | --- | ||
15 | target/arm/translate.c | 6 ++++-- | ||
16 | 1 file changed, 4 insertions(+), 2 deletions(-) | ||
17 | |||
18 | diff --git a/target/arm/translate.c b/target/arm/translate.c | ||
19 | index XXXXXXX..XXXXXXX 100644 | ||
20 | --- a/target/arm/translate.c | ||
21 | +++ b/target/arm/translate.c | ||
22 | @@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw | ||
23 | gen_exception_return(s, tmp); | ||
24 | break; | ||
25 | case 6: /* MRS */ | ||
26 | - if (extract32(insn, 5, 1)) { | ||
27 | + if (extract32(insn, 5, 1) && | ||
28 | + !arm_dc_feature(s, ARM_FEATURE_M)) { | ||
29 | /* MRS (banked) */ | ||
30 | int sysm = extract32(insn, 16, 4) | | ||
31 | (extract32(insn, 4, 1) << 4); | ||
32 | @@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw | ||
33 | store_reg(s, rd, tmp); | ||
34 | break; | ||
35 | case 7: /* MRS */ | ||
36 | - if (extract32(insn, 5, 1)) { | ||
37 | + if (extract32(insn, 5, 1) && | ||
38 | + !arm_dc_feature(s, ARM_FEATURE_M)) { | ||
39 | /* MRS (banked) */ | ||
40 | int sysm = extract32(insn, 16, 4) | | ||
41 | (extract32(insn, 4, 1) << 4); | ||
42 | -- | ||
43 | 2.7.4 | ||
44 | |||
45 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | The MRS instruction requires that bits [19..16] are all 1s, and for | ||
2 | A/R profile also that bits [7..0] are all 0s. At this point in the | ||
3 | decode tree we have checked all of the rest of the instruction but | ||
4 | were allowing these to be any value. If these bits are not set then | ||
5 | the result is architecturally UNPREDICTABLE, but choosing to UNDEF is | ||
6 | more helpful to the user and avoids unexpected odd behaviour if the | ||
7 | encodings are used for some purpose in future architecture versions. | ||
8 | 1 | ||
9 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
10 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
11 | Message-id: 1487616072-9226-4-git-send-email-peter.maydell@linaro.org | ||
12 | --- | ||
13 | target/arm/translate.c | 14 ++++++++++++++ | ||
14 | 1 file changed, 14 insertions(+) | ||
15 | |||
16 | diff --git a/target/arm/translate.c b/target/arm/translate.c | ||
17 | index XXXXXXX..XXXXXXX 100644 | ||
18 | --- a/target/arm/translate.c | ||
19 | +++ b/target/arm/translate.c | ||
20 | @@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw | ||
21 | break; | ||
22 | } | ||
23 | |||
24 | + if (extract32(insn, 16, 4) != 0xf) { | ||
25 | + goto illegal_op; | ||
26 | + } | ||
27 | + if (!arm_dc_feature(s, ARM_FEATURE_M) && | ||
28 | + extract32(insn, 0, 8) != 0) { | ||
29 | + goto illegal_op; | ||
30 | + } | ||
31 | + | ||
32 | /* mrs cpsr */ | ||
33 | tmp = tcg_temp_new_i32(); | ||
34 | if (arm_dc_feature(s, ARM_FEATURE_M)) { | ||
35 | @@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw | ||
36 | if (IS_USER(s) || arm_dc_feature(s, ARM_FEATURE_M)) { | ||
37 | goto illegal_op; | ||
38 | } | ||
39 | + | ||
40 | + if (extract32(insn, 16, 4) != 0xf || | ||
41 | + extract32(insn, 0, 8) != 0) { | ||
42 | + goto illegal_op; | ||
43 | + } | ||
44 | + | ||
45 | tmp = load_cpu_field(spsr); | ||
46 | store_reg(s, rd, tmp); | ||
47 | break; | ||
48 | -- | ||
49 | 2.7.4 | ||
50 | |||
51 | diff view generated by jsdifflib |
1 | Our implementation of writes to the APSR for M-profile via the MSR | 1 | From: Philippe Mathieu-Daudé <philmd@linaro.org> |
---|---|---|---|
2 | instruction was badly broken. | ||
3 | 2 | ||
4 | First and worst, we had the sense wrong on the test of bit 2 of the | 3 | Cortex-M profile is only emulable from TCG accelerator. Restrict |
5 | SYSm field -- this is supposed to request an APSR write if bit 2 is 0 | 4 | the GDBstub features to its availability in order to avoid a link |
6 | but we were doing it if bit 2 was 1. This bug was introduced in | 5 | error when TCG is not enabled: |
7 | commit 58117c9bb429cd, so hasn't been in a QEMU release. | ||
8 | 6 | ||
9 | Secondly, the choice of exactly which parts of APSR should be written | 7 | Undefined symbols for architecture arm64: |
10 | is defined by bits in the 'mask' field. We were not passing these | 8 | "_arm_v7m_get_sp_ptr", referenced from: |
11 | through from instruction decode, making it impossible to check them | 9 | _m_sysreg_get in target_arm_gdbstub.c.o |
12 | in the helper. | 10 | "_arm_v7m_mrs_control", referenced from: |
11 | _arm_gdb_get_m_systemreg in target_arm_gdbstub.c.o | ||
12 | ld: symbol(s) not found for architecture arm64 | ||
13 | clang: error: linker command failed with exit code 1 (use -v to see invocation) | ||
13 | 14 | ||
14 | Pass the mask bits through from the instruction decode to the helper | 15 | Fixes: 7d8b28b8b5 ("target/arm: Implement gdbstub m-profile systemreg and secext") |
15 | function and process them appropriately; fix the wrong sense of the | 16 | Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> |
16 | SYSm bit 2 check. | 17 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> |
18 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
19 | Message-id: 20230322142902.69511-3-philmd@linaro.org | ||
20 | [PMM: add #include since I cherry-picked this patch from the series] | ||
21 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
22 | --- | ||
23 | target/arm/gdbstub.c | 5 +++-- | ||
24 | 1 file changed, 3 insertions(+), 2 deletions(-) | ||
17 | 25 | ||
18 | Invalid mask values and invalid combinations of mask and register | 26 | diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c |
19 | number are UNPREDICTABLE; we choose to treat them as if the mask | ||
20 | values were valid. | ||
21 | |||
22 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
23 | Message-id: 1487616072-9226-5-git-send-email-peter.maydell@linaro.org | ||
24 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
25 | --- | ||
26 | target/arm/helper.c | 26 ++++++++++++++++++++++---- | ||
27 | target/arm/translate.c | 3 ++- | ||
28 | 2 files changed, 24 insertions(+), 5 deletions(-) | ||
29 | |||
30 | diff --git a/target/arm/helper.c b/target/arm/helper.c | ||
31 | index XXXXXXX..XXXXXXX 100644 | 27 | index XXXXXXX..XXXXXXX 100644 |
32 | --- a/target/arm/helper.c | 28 | --- a/target/arm/gdbstub.c |
33 | +++ b/target/arm/helper.c | 29 | +++ b/target/arm/gdbstub.c |
34 | @@ -XXX,XX +XXX,XX @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg) | 30 | @@ -XXX,XX +XXX,XX @@ |
31 | #include "cpu.h" | ||
32 | #include "exec/gdbstub.h" | ||
33 | #include "gdbstub/helpers.h" | ||
34 | +#include "sysemu/tcg.h" | ||
35 | #include "internals.h" | ||
36 | #include "cpregs.h" | ||
37 | |||
38 | @@ -XXX,XX +XXX,XX @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) | ||
39 | 2, "arm-vfp-sysregs.xml", 0); | ||
40 | } | ||
35 | } | 41 | } |
36 | } | 42 | - if (cpu_isar_feature(aa32_mve, cpu)) { |
37 | 43 | + if (cpu_isar_feature(aa32_mve, cpu) && tcg_enabled()) { | |
38 | -void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val) | 44 | gdb_register_coprocessor(cs, mve_gdb_get_reg, mve_gdb_set_reg, |
39 | -{ | 45 | 1, "arm-m-profile-mve.xml", 0); |
40 | +void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val) | 46 | } |
41 | +{ | 47 | @@ -XXX,XX +XXX,XX @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) |
42 | + /* We're passed bits [11..0] of the instruction; extract | 48 | arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs), |
43 | + * SYSm and the mask bits. | 49 | "system-registers.xml", 0); |
44 | + * Invalid combinations of SYSm and mask are UNPREDICTABLE; | 50 | |
45 | + * we choose to treat them as if the mask bits were valid. | 51 | - if (arm_feature(env, ARM_FEATURE_M)) { |
46 | + * NB that the pseudocode 'mask' variable is bits [11..10], | 52 | + if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) { |
47 | + * whereas ours is [11..8]. | 53 | gdb_register_coprocessor(cs, |
48 | + */ | 54 | arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg, |
49 | + uint32_t mask = extract32(maskreg, 8, 4); | 55 | arm_gen_dynamic_m_systemreg_xml(cs, cs->gdb_num_regs), |
50 | + uint32_t reg = extract32(maskreg, 0, 8); | ||
51 | + | ||
52 | if (arm_current_el(env) == 0 && reg > 7) { | ||
53 | /* only xPSR sub-fields may be written by unprivileged */ | ||
54 | return; | ||
55 | @@ -XXX,XX +XXX,XX @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val) | ||
56 | switch (reg) { | ||
57 | case 0 ... 7: /* xPSR sub-fields */ | ||
58 | /* only APSR is actually writable */ | ||
59 | - if (reg & 4) { | ||
60 | - xpsr_write(env, val, 0xf8000000); /* APSR */ | ||
61 | + if (!(reg & 4)) { | ||
62 | + uint32_t apsrmask = 0; | ||
63 | + | ||
64 | + if (mask & 8) { | ||
65 | + apsrmask |= 0xf8000000; /* APSR NZCVQ */ | ||
66 | + } | ||
67 | + if ((mask & 4) && arm_feature(env, ARM_FEATURE_THUMB_DSP)) { | ||
68 | + apsrmask |= 0x000f0000; /* APSR GE[3:0] */ | ||
69 | + } | ||
70 | + xpsr_write(env, val, apsrmask); | ||
71 | } | ||
72 | break; | ||
73 | case 8: /* MSP */ | ||
74 | diff --git a/target/arm/translate.c b/target/arm/translate.c | ||
75 | index XXXXXXX..XXXXXXX 100644 | ||
76 | --- a/target/arm/translate.c | ||
77 | +++ b/target/arm/translate.c | ||
78 | @@ -XXX,XX +XXX,XX @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw | ||
79 | case 0: /* msr cpsr. */ | ||
80 | if (arm_dc_feature(s, ARM_FEATURE_M)) { | ||
81 | tmp = load_reg(s, rn); | ||
82 | - addr = tcg_const_i32(insn & 0xff); | ||
83 | + /* the constant is the mask and SYSm fields */ | ||
84 | + addr = tcg_const_i32(insn & 0xfff); | ||
85 | gen_helper_v7m_msr(cpu_env, addr, tmp); | ||
86 | tcg_temp_free_i32(addr); | ||
87 | tcg_temp_free_i32(tmp); | ||
88 | -- | 56 | -- |
89 | 2.7.4 | 57 | 2.34.1 |
90 | 58 | ||
91 | 59 | diff view generated by jsdifflib |