1 | target-arm queue for 3.1: mostly bug fixes, but the "turn on | 1 | target-arm queue for rc1 -- these are all bug fixes. |
---|---|---|---|
2 | EL2 support for Cortex-A7 and -A15" is technically enabling | ||
3 | of a new feature... I think this is OK since we're only at rc1, | ||
4 | and it's easy to revert that feature bit flip if necessary. | ||
5 | 2 | ||
6 | thanks | 3 | thanks |
7 | -- PMM | 4 | -- PMM |
8 | 5 | ||
6 | The following changes since commit b9404bf592e7ba74180e1a54ed7a266ec6ee67f2: | ||
9 | 7 | ||
10 | The following changes since commit 5704c36d25ee84e7129722cb0db53df9faefe943: | 8 | Merge remote-tracking branch 'remotes/dgilbert/tags/pull-hmp-20190715' into staging (2019-07-15 12:22:07 +0100) |
11 | |||
12 | Merge remote-tracking branch 'remotes/kraxel/tags/fixes-31-20181112-pull-request' into staging (2018-11-12 15:55:40 +0000) | ||
13 | 9 | ||
14 | are available in the Git repository at: | 10 | are available in the Git repository at: |
15 | 11 | ||
16 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20181112 | 12 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20190715 |
17 | 13 | ||
18 | for you to fetch changes up to 1a4c1a6dbf60aebddd07753f1013ea896c06ad29: | 14 | for you to fetch changes up to 51c9122e92b776a3f16af0b9282f1dc5012e2a19: |
19 | 15 | ||
20 | target/arm/cpu: Give Cortex-A15 and -A7 the EL2 feature (2018-11-12 16:52:29 +0000) | 16 | target/arm: NS BusFault on vector table fetch escalates to NS HardFault (2019-07-15 14:17:04 +0100) |
21 | 17 | ||
22 | ---------------------------------------------------------------- | 18 | ---------------------------------------------------------------- |
23 | target/arm queue: | 19 | target-arm queue: |
24 | * Remove no-longer-needed workaround for small SAU regions for v8M | 20 | * report ARMv8-A FP support for AArch32 -cpu max |
25 | * Remove antique TODO comment | 21 | * hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory |
26 | * MAINTAINERS: Add an entry for the 'collie' machine | 22 | * hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[] |
27 | * hw/arm/sysbus-fdt: Only call match_fn callback if the type matches | 23 | * hw/ssi/mss-spi: Avoid crash when reading empty RX FIFO |
28 | * Fix infinite recursion in tlbi_aa64_vmalle1_write() | 24 | * hw/display/xlnx_dp: Avoid crash when reading empty RX FIFO |
29 | * ARM KVM: fix various bugs in handling of guest debugging | 25 | * hw/arm/virt: Fix non-secure flash mode |
30 | * Correctly implement handling of HCR_EL2.{VI, VF} | 26 | * pl031: Correctly migrate state when using -rtc clock=host |
31 | * Hyp mode R14 is shared with User and System | 27 | * fix regression that meant arm926 and arm1026 lost VFP |
32 | * Give Cortex-A15 and -A7 the EL2 feature | 28 | double-precision support |
29 | * v8M: NS BusFault on vector table fetch escalates to NS HardFault | ||
33 | 30 | ||
34 | ---------------------------------------------------------------- | 31 | ---------------------------------------------------------------- |
35 | Alex Bennée (6): | 32 | Alex Bennée (1): |
36 | target/arm64: properly handle DBGVR RESS bits | 33 | target/arm: report ARMv8-A FP support for AArch32 -cpu max |
37 | target/arm64: hold BQL when calling do_interrupt() | ||
38 | target/arm64: kvm debug set target_el when passing exception to guest | ||
39 | tests/guest-debug: fix scoping of failcount | ||
40 | arm: use symbolic MDCR_TDE in arm_debug_target_el | ||
41 | arm: fix aa64_generate_debug_exceptions to work with EL2 | ||
42 | 34 | ||
43 | Eric Auger (1): | 35 | David Engraf (1): |
44 | hw/arm/sysbus-fdt: Only call match_fn callback if the type matches | 36 | hw/arm/virt: Fix non-secure flash mode |
45 | 37 | ||
46 | Peter Maydell (7): | 38 | Peter Maydell (3): |
47 | target/arm: Remove workaround for small SAU regions | 39 | pl031: Correctly migrate state when using -rtc clock=host |
48 | target/arm: Remove antique TODO comment | 40 | target/arm: Set VFP-related MVFR0 fields for arm926 and arm1026 |
49 | Revert "target/arm: Implement HCR.VI and VF" | 41 | target/arm: NS BusFault on vector table fetch escalates to NS HardFault |
50 | target/arm: Track the state of our irq lines from the GIC explicitly | ||
51 | target/arm: Correctly implement handling of HCR_EL2.{VI, VF} | ||
52 | target/arm: Hyp mode R14 is shared with User and System | ||
53 | target/arm/cpu: Give Cortex-A15 and -A7 the EL2 feature | ||
54 | 42 | ||
55 | Richard Henderson (1): | 43 | Philippe Mathieu-Daudé (5): |
56 | target/arm: Fix typo in tlbi_aa64_vmalle1_write | 44 | hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs |
45 | hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory | ||
46 | hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[] | ||
47 | hw/ssi/mss-spi: Avoid crash when reading empty RX FIFO | ||
48 | hw/display/xlnx_dp: Avoid crash when reading empty RX FIFO | ||
57 | 49 | ||
58 | Thomas Huth (1): | 50 | include/hw/timer/pl031.h | 2 ++ |
59 | MAINTAINERS: Add an entry for the 'collie' machine | 51 | hw/arm/virt.c | 2 +- |
52 | hw/core/machine.c | 1 + | ||
53 | hw/display/xlnx_dp.c | 15 +++++--- | ||
54 | hw/ssi/mss-spi.c | 8 ++++- | ||
55 | hw/ssi/xilinx_spips.c | 43 +++++++++++++++------- | ||
56 | hw/timer/pl031.c | 92 +++++++++++++++++++++++++++++++++++++++++++++--- | ||
57 | target/arm/cpu.c | 16 +++++++++ | ||
58 | target/arm/m_helper.c | 21 ++++++++--- | ||
59 | 9 files changed, 174 insertions(+), 26 deletions(-) | ||
60 | 60 | ||
61 | target/arm/cpu.h | 44 +++++++++++------ | ||
62 | target/arm/internals.h | 34 +++++++++++++ | ||
63 | hw/arm/sysbus-fdt.c | 12 +++-- | ||
64 | target/arm/cpu.c | 66 ++++++++++++++++++++++++- | ||
65 | target/arm/helper.c | 101 +++++++++++++------------------------- | ||
66 | target/arm/kvm32.c | 4 +- | ||
67 | target/arm/kvm64.c | 20 +++++++- | ||
68 | target/arm/machine.c | 51 +++++++++++++++++++ | ||
69 | target/arm/op_helper.c | 4 +- | ||
70 | MAINTAINERS | 7 +++ | ||
71 | tests/guest-debug/test-gdbstub.py | 1 + | ||
72 | 11 files changed, 248 insertions(+), 96 deletions(-) | ||
73 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | Before we supported direct execution from MMIO regions, we | ||
2 | implemented workarounds in commit 720424359917887c926a33d2 | ||
3 | which let us avoid doing so, even if the SAU or MPU region | ||
4 | was less than page-sized. | ||
5 | 1 | ||
6 | Once we implemented execute-from-MMIO, we removed part | ||
7 | of those workarounds in commit d4b6275df320cee76; but | ||
8 | we forgot the one in get_phys_addr_pmsav8() which | ||
9 | suppressed use of small SAU regions in executable regions. | ||
10 | Remove that workaround now. | ||
11 | |||
12 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
13 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
14 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
15 | Message-id: 20181106163801.14474-1-peter.maydell@linaro.org | ||
16 | --- | ||
17 | target/arm/helper.c | 12 ------------ | ||
18 | 1 file changed, 12 deletions(-) | ||
19 | |||
20 | diff --git a/target/arm/helper.c b/target/arm/helper.c | ||
21 | index XXXXXXX..XXXXXXX 100644 | ||
22 | --- a/target/arm/helper.c | ||
23 | +++ b/target/arm/helper.c | ||
24 | @@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address, | ||
25 | |||
26 | ret = pmsav8_mpu_lookup(env, address, access_type, mmu_idx, phys_ptr, | ||
27 | txattrs, prot, &mpu_is_subpage, fi, NULL); | ||
28 | - /* | ||
29 | - * TODO: this is a temporary hack to ignore the fact that the SAU region | ||
30 | - * is smaller than a page if this is an executable region. We never | ||
31 | - * supported small MPU regions, but we did (accidentally) allow small | ||
32 | - * SAU regions, and if we now made small SAU regions not be executable | ||
33 | - * then this would break previously working guest code. We can't | ||
34 | - * remove this until/unless we implement support for execution from | ||
35 | - * small regions. | ||
36 | - */ | ||
37 | - if (*prot & PAGE_EXEC) { | ||
38 | - sattrs.subpage = false; | ||
39 | - } | ||
40 | *page_size = sattrs.subpage || mpu_is_subpage ? 1 : TARGET_PAGE_SIZE; | ||
41 | return ret; | ||
42 | } | ||
43 | -- | ||
44 | 2.19.1 | ||
45 | |||
46 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | Remove a TODO comment about implementing the vectored interrupt | ||
2 | controller. We have had an implementation of that for a decade; | ||
3 | it's in hw/intc/pl190.c. | ||
4 | 1 | ||
5 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
6 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
7 | Message-id: 20181106164118.16184-1-peter.maydell@linaro.org | ||
8 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | ||
9 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
10 | --- | ||
11 | target/arm/helper.c | 1 - | ||
12 | 1 file changed, 1 deletion(-) | ||
13 | |||
14 | diff --git a/target/arm/helper.c b/target/arm/helper.c | ||
15 | index XXXXXXX..XXXXXXX 100644 | ||
16 | --- a/target/arm/helper.c | ||
17 | +++ b/target/arm/helper.c | ||
18 | @@ -XXX,XX +XXX,XX @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs) | ||
19 | return; | ||
20 | } | ||
21 | |||
22 | - /* TODO: Vectored interrupt controller. */ | ||
23 | switch (cs->exception_index) { | ||
24 | case EXCP_UDEF: | ||
25 | new_mode = ARM_CPU_MODE_UND; | ||
26 | -- | ||
27 | 2.19.1 | ||
28 | |||
29 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | From: Thomas Huth <thuth@redhat.com> | ||
2 | 1 | ||
3 | There is no active maintainer, but since Peter is picking up | ||
4 | patches via qemu-arm@nongnu.org, I think we could at least use | ||
5 | "Odd Fixes" as status here. | ||
6 | |||
7 | Signed-off-by: Thomas Huth <thuth@redhat.com> | ||
8 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
9 | Message-id: 1541528230-31817-1-git-send-email-thuth@redhat.com | ||
10 | [PMM: Also add myself as an M: contact] | ||
11 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
12 | --- | ||
13 | MAINTAINERS | 7 +++++++ | ||
14 | 1 file changed, 7 insertions(+) | ||
15 | |||
16 | diff --git a/MAINTAINERS b/MAINTAINERS | ||
17 | index XXXXXXX..XXXXXXX 100644 | ||
18 | --- a/MAINTAINERS | ||
19 | +++ b/MAINTAINERS | ||
20 | @@ -XXX,XX +XXX,XX @@ F: hw/*/pxa2xx* | ||
21 | F: hw/misc/mst_fpga.c | ||
22 | F: include/hw/arm/pxa.h | ||
23 | |||
24 | +Sharp SL-5500 (Collie) PDA | ||
25 | +M: Peter Maydell <peter.maydell@linaro.org> | ||
26 | +L: qemu-arm@nongnu.org | ||
27 | +S: Odd Fixes | ||
28 | +F: hw/arm/collie.c | ||
29 | +F: hw/arm/strongarm* | ||
30 | + | ||
31 | Stellaris | ||
32 | M: Peter Maydell <peter.maydell@linaro.org> | ||
33 | L: qemu-arm@nongnu.org | ||
34 | -- | ||
35 | 2.19.1 | ||
36 | |||
37 | diff view generated by jsdifflib |
1 | From: Alex Bennée <alex.bennee@linaro.org> | 1 | From: Alex Bennée <alex.bennee@linaro.org> |
---|---|---|---|
2 | 2 | ||
3 | We already have this symbol defined so lets use it. | 3 | When we converted to using feature bits in 602f6e42cfbf we missed out |
4 | the fact (dp && arm_dc_feature(s, ARM_FEATURE_V8)) was supported for | ||
5 | -cpu max configurations. This caused a regression in the GCC test | ||
6 | suite. Fix this by setting the appropriate bits in mvfr1.FPHP to | ||
7 | report ARMv8-A with FP support (but not ARMv8.2-FP16). | ||
4 | 8 | ||
9 | Fixes: https://bugs.launchpad.net/qemu/+bug/1836078 | ||
5 | Signed-off-by: Alex Bennée <alex.bennee@linaro.org> | 10 | Signed-off-by: Alex Bennée <alex.bennee@linaro.org> |
6 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | 11 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> |
7 | Message-id: 20181109152119.9242-7-alex.bennee@linaro.org | 12 | Message-id: 20190711103737.10017-1-alex.bennee@linaro.org |
8 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 13 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
9 | --- | 14 | --- |
10 | target/arm/cpu.h | 2 +- | 15 | target/arm/cpu.c | 4 ++++ |
11 | 1 file changed, 1 insertion(+), 1 deletion(-) | 16 | 1 file changed, 4 insertions(+) |
12 | 17 | ||
13 | diff --git a/target/arm/cpu.h b/target/arm/cpu.h | 18 | diff --git a/target/arm/cpu.c b/target/arm/cpu.c |
14 | index XXXXXXX..XXXXXXX 100644 | 19 | index XXXXXXX..XXXXXXX 100644 |
15 | --- a/target/arm/cpu.h | 20 | --- a/target/arm/cpu.c |
16 | +++ b/target/arm/cpu.h | 21 | +++ b/target/arm/cpu.c |
17 | @@ -XXX,XX +XXX,XX @@ static inline int arm_debug_target_el(CPUARMState *env) | 22 | @@ -XXX,XX +XXX,XX @@ static void arm_max_initfn(Object *obj) |
18 | 23 | t = FIELD_DP32(t, ID_ISAR6, SPECRES, 1); | |
19 | if (arm_feature(env, ARM_FEATURE_EL2) && !secure) { | 24 | cpu->isar.id_isar6 = t; |
20 | route_to_el2 = env->cp15.hcr_el2 & HCR_TGE || | 25 | |
21 | - env->cp15.mdcr_el2 & (1 << 8); | 26 | + t = cpu->isar.mvfr1; |
22 | + env->cp15.mdcr_el2 & MDCR_TDE; | 27 | + t = FIELD_DP32(t, MVFR1, FPHP, 2); /* v8.0 FP support */ |
23 | } | 28 | + cpu->isar.mvfr1 = t; |
24 | 29 | + | |
25 | if (route_to_el2) { | 30 | t = cpu->isar.mvfr2; |
31 | t = FIELD_DP32(t, MVFR2, SIMDMISC, 3); /* SIMD MaxNum */ | ||
32 | t = FIELD_DP32(t, MVFR2, FPMISC, 4); /* FP MaxNum */ | ||
26 | -- | 33 | -- |
27 | 2.19.1 | 34 | 2.20.1 |
28 | 35 | ||
29 | 36 | diff view generated by jsdifflib |
1 | From: Alex Bennée <alex.bennee@linaro.org> | 1 | From: Philippe Mathieu-Daudé <philmd@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | The test was incomplete and incorrectly caused debug exceptions to be | 3 | In the next commit we will implement the write_with_attrs() |
4 | generated when returning to EL2 after a failed attempt to single-step | 4 | handler. To avoid using different APIs, convert the read() |
5 | an EL1 instruction. Fix this while cleaning up the function a little. | 5 | handler first. |
6 | 6 | ||
7 | Signed-off-by: Alex Bennée <alex.bennee@linaro.org> | 7 | Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> |
8 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | 8 | Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com> |
9 | Message-id: 20181109152119.9242-8-alex.bennee@linaro.org | 9 | Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> |
10 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 10 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
11 | --- | 11 | --- |
12 | target/arm/cpu.h | 39 ++++++++++++++++++++++++--------------- | 12 | hw/ssi/xilinx_spips.c | 23 +++++++++++------------ |
13 | 1 file changed, 24 insertions(+), 15 deletions(-) | 13 | 1 file changed, 11 insertions(+), 12 deletions(-) |
14 | 14 | ||
15 | diff --git a/target/arm/cpu.h b/target/arm/cpu.h | 15 | diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c |
16 | index XXXXXXX..XXXXXXX 100644 | 16 | index XXXXXXX..XXXXXXX 100644 |
17 | --- a/target/arm/cpu.h | 17 | --- a/hw/ssi/xilinx_spips.c |
18 | +++ b/target/arm/cpu.h | 18 | +++ b/hw/ssi/xilinx_spips.c |
19 | @@ -XXX,XX +XXX,XX @@ static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu) | 19 | @@ -XXX,XX +XXX,XX @@ static void lqspi_load_cache(void *opaque, hwaddr addr) |
20 | return (cpu->clidr & R_V7M_CLIDR_CTYPE_ALL_MASK) != 0; | 20 | } |
21 | } | 21 | } |
22 | 22 | ||
23 | +/* See AArch64.GenerateDebugExceptionsFrom() in ARM ARM pseudocode */ | 23 | -static uint64_t |
24 | static inline bool aa64_generate_debug_exceptions(CPUARMState *env) | 24 | -lqspi_read(void *opaque, hwaddr addr, unsigned int size) |
25 | +static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value, | ||
26 | + unsigned size, MemTxAttrs attrs) | ||
25 | { | 27 | { |
26 | - if (arm_is_secure(env)) { | 28 | - XilinxQSPIPS *q = opaque; |
27 | - /* MDCR_EL3.SDD disables debug events from Secure state */ | 29 | - uint32_t ret; |
28 | - if (extract32(env->cp15.mdcr_el3, 16, 1) != 0 | 30 | + XilinxQSPIPS *q = XILINX_QSPIPS(opaque); |
29 | - || arm_current_el(env) == 3) { | 31 | |
30 | - return false; | 32 | if (addr >= q->lqspi_cached_addr && |
31 | - } | 33 | addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) { |
32 | + int cur_el = arm_current_el(env); | 34 | uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr]; |
33 | + int debug_el; | 35 | - ret = cpu_to_le32(*(uint32_t *)retp); |
36 | - DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr, | ||
37 | - (unsigned)ret); | ||
38 | - return ret; | ||
39 | - } else { | ||
40 | - lqspi_load_cache(opaque, addr); | ||
41 | - return lqspi_read(opaque, addr, size); | ||
42 | + *value = cpu_to_le32(*(uint32_t *)retp); | ||
43 | + DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n", | ||
44 | + addr, *value); | ||
45 | + return MEMTX_OK; | ||
46 | } | ||
34 | + | 47 | + |
35 | + if (cur_el == 3) { | 48 | + lqspi_load_cache(opaque, addr); |
36 | + return false; | 49 | + return lqspi_read(opaque, addr, value, size, attrs); |
37 | } | ||
38 | |||
39 | - if (arm_current_el(env) == arm_debug_target_el(env)) { | ||
40 | - if ((extract32(env->cp15.mdscr_el1, 13, 1) == 0) | ||
41 | - || (env->daif & PSTATE_D)) { | ||
42 | - return false; | ||
43 | - } | ||
44 | + /* MDCR_EL3.SDD disables debug events from Secure state */ | ||
45 | + if (arm_is_secure_below_el3(env) | ||
46 | + && extract32(env->cp15.mdcr_el3, 16, 1)) { | ||
47 | + return false; | ||
48 | } | ||
49 | - return true; | ||
50 | + | ||
51 | + /* | ||
52 | + * Same EL to same EL debug exceptions need MDSCR_KDE enabled | ||
53 | + * while not masking the (D)ebug bit in DAIF. | ||
54 | + */ | ||
55 | + debug_el = arm_debug_target_el(env); | ||
56 | + | ||
57 | + if (cur_el == debug_el) { | ||
58 | + return extract32(env->cp15.mdscr_el1, 13, 1) | ||
59 | + && !(env->daif & PSTATE_D); | ||
60 | + } | ||
61 | + | ||
62 | + /* Otherwise the debug target needs to be a higher EL */ | ||
63 | + return debug_el > cur_el; | ||
64 | } | 50 | } |
65 | 51 | ||
66 | static inline bool aa32_generate_debug_exceptions(CPUARMState *env) | 52 | static const MemoryRegionOps lqspi_ops = { |
67 | @@ -XXX,XX +XXX,XX @@ static inline bool aa32_generate_debug_exceptions(CPUARMState *env) | 53 | - .read = lqspi_read, |
68 | * since the pseudocode has it at all callsites except for the one in | 54 | + .read_with_attrs = lqspi_read, |
69 | * CheckSoftwareStep(), where it is elided because both branches would | 55 | .endianness = DEVICE_NATIVE_ENDIAN, |
70 | * always return the same value. | 56 | .valid = { |
71 | - * | 57 | .min_access_size = 1, |
72 | - * Parts of the pseudocode relating to EL2 and EL3 are omitted because we | ||
73 | - * don't yet implement those exception levels or their associated trap bits. | ||
74 | */ | ||
75 | static inline bool arm_generate_debug_exceptions(CPUARMState *env) | ||
76 | { | ||
77 | -- | 58 | -- |
78 | 2.19.1 | 59 | 2.20.1 |
79 | 60 | ||
80 | 61 | diff view generated by jsdifflib |
1 | In commit 8a0fc3a29fc2315325400 we tried to implement HCR_EL2.{VI,VF}, | 1 | From: Philippe Mathieu-Daudé <philmd@redhat.com> |
---|---|---|---|
2 | but we got it wrong and had to revert it. | ||
3 | 2 | ||
4 | In that commit we implemented them as simply tracking whether there | 3 | Lei Sun found while auditing the code that a CPU write would |
5 | is a pending virtual IRQ or virtual FIQ. This is not correct -- these | 4 | trigger a NULL pointer dereference. |
6 | bits cause a software-generated VIRQ/VFIQ, which is distinct from | ||
7 | whether there is a hardware-generated VIRQ/VFIQ caused by the | ||
8 | external interrupt controller. So we need to track separately | ||
9 | the HCR_EL2 bit state and the external virq/vfiq line state, and | ||
10 | OR the two together to get the actual pending VIRQ/VFIQ state. | ||
11 | 5 | ||
12 | Fixes: 8a0fc3a29fc2315325400c738f807d0d4ae0ab7f | 6 | >From UG1085 datasheet [*] AXI writes in this region are ignored |
7 | and generates an AXI Slave Error (SLVERR). | ||
8 | |||
9 | Fix by implementing the write_with_attrs() handler. | ||
10 | Return MEMTX_ERROR when the region is accessed (this error maps | ||
11 | to an AXI slave error). | ||
12 | |||
13 | [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf | ||
14 | |||
15 | Reported-by: Lei Sun <slei.casper@gmail.com> | ||
16 | Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> | ||
17 | Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com> | ||
18 | Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
13 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 19 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
14 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | ||
15 | Message-id: 20181109134731.11605-4-peter.maydell@linaro.org | ||
16 | --- | 20 | --- |
17 | target/arm/internals.h | 18 ++++++++++++++++ | 21 | hw/ssi/xilinx_spips.c | 16 ++++++++++++++++ |
18 | target/arm/cpu.c | 48 +++++++++++++++++++++++++++++++++++++++++- | 22 | 1 file changed, 16 insertions(+) |
19 | target/arm/helper.c | 20 ++++++++++++++++-- | ||
20 | 3 files changed, 83 insertions(+), 3 deletions(-) | ||
21 | 23 | ||
22 | diff --git a/target/arm/internals.h b/target/arm/internals.h | 24 | diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c |
23 | index XXXXXXX..XXXXXXX 100644 | 25 | index XXXXXXX..XXXXXXX 100644 |
24 | --- a/target/arm/internals.h | 26 | --- a/hw/ssi/xilinx_spips.c |
25 | +++ b/target/arm/internals.h | 27 | +++ b/hw/ssi/xilinx_spips.c |
26 | @@ -XXX,XX +XXX,XX @@ static inline const char *aarch32_mode_name(uint32_t psr) | 28 | @@ -XXX,XX +XXX,XX @@ static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value, |
27 | return cpu_mode_names[psr & 0xf]; | 29 | return lqspi_read(opaque, addr, value, size, attrs); |
28 | } | 30 | } |
29 | 31 | ||
30 | +/** | 32 | +static MemTxResult lqspi_write(void *opaque, hwaddr offset, uint64_t value, |
31 | + * arm_cpu_update_virq: Update CPU_INTERRUPT_VIRQ bit in cs->interrupt_request | 33 | + unsigned size, MemTxAttrs attrs) |
32 | + * | ||
33 | + * Update the CPU_INTERRUPT_VIRQ bit in cs->interrupt_request, following | ||
34 | + * a change to either the input VIRQ line from the GIC or the HCR_EL2.VI bit. | ||
35 | + * Must be called with the iothread lock held. | ||
36 | + */ | ||
37 | +void arm_cpu_update_virq(ARMCPU *cpu); | ||
38 | + | ||
39 | +/** | ||
40 | + * arm_cpu_update_vfiq: Update CPU_INTERRUPT_VFIQ bit in cs->interrupt_request | ||
41 | + * | ||
42 | + * Update the CPU_INTERRUPT_VFIQ bit in cs->interrupt_request, following | ||
43 | + * a change to either the input VFIQ line from the GIC or the HCR_EL2.VF bit. | ||
44 | + * Must be called with the iothread lock held. | ||
45 | + */ | ||
46 | +void arm_cpu_update_vfiq(ARMCPU *cpu); | ||
47 | + | ||
48 | #endif | ||
49 | diff --git a/target/arm/cpu.c b/target/arm/cpu.c | ||
50 | index XXXXXXX..XXXXXXX 100644 | ||
51 | --- a/target/arm/cpu.c | ||
52 | +++ b/target/arm/cpu.c | ||
53 | @@ -XXX,XX +XXX,XX @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request) | ||
54 | } | ||
55 | #endif | ||
56 | |||
57 | +void arm_cpu_update_virq(ARMCPU *cpu) | ||
58 | +{ | 34 | +{ |
59 | + /* | 35 | + /* |
60 | + * Update the interrupt level for VIRQ, which is the logical OR of | 36 | + * From UG1085, Chapter 24 (Quad-SPI controllers): |
61 | + * the HCR_EL2.VI bit and the input line level from the GIC. | 37 | + * - Writes are ignored |
38 | + * - AXI writes generate an external AXI slave error (SLVERR) | ||
62 | + */ | 39 | + */ |
63 | + CPUARMState *env = &cpu->env; | 40 | + qemu_log_mask(LOG_GUEST_ERROR, "%s Unexpected %u-bit access to 0x%" PRIx64 |
64 | + CPUState *cs = CPU(cpu); | 41 | + " (value: 0x%" PRIx64 "\n", |
42 | + __func__, size << 3, offset, value); | ||
65 | + | 43 | + |
66 | + bool new_state = (env->cp15.hcr_el2 & HCR_VI) || | 44 | + return MEMTX_ERROR; |
67 | + (env->irq_line_state & CPU_INTERRUPT_VIRQ); | ||
68 | + | ||
69 | + if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VIRQ) != 0)) { | ||
70 | + if (new_state) { | ||
71 | + cpu_interrupt(cs, CPU_INTERRUPT_VIRQ); | ||
72 | + } else { | ||
73 | + cpu_reset_interrupt(cs, CPU_INTERRUPT_VIRQ); | ||
74 | + } | ||
75 | + } | ||
76 | +} | 45 | +} |
77 | + | 46 | + |
78 | +void arm_cpu_update_vfiq(ARMCPU *cpu) | 47 | static const MemoryRegionOps lqspi_ops = { |
79 | +{ | 48 | .read_with_attrs = lqspi_read, |
80 | + /* | 49 | + .write_with_attrs = lqspi_write, |
81 | + * Update the interrupt level for VFIQ, which is the logical OR of | 50 | .endianness = DEVICE_NATIVE_ENDIAN, |
82 | + * the HCR_EL2.VF bit and the input line level from the GIC. | 51 | .valid = { |
83 | + */ | 52 | .min_access_size = 1, |
84 | + CPUARMState *env = &cpu->env; | ||
85 | + CPUState *cs = CPU(cpu); | ||
86 | + | ||
87 | + bool new_state = (env->cp15.hcr_el2 & HCR_VF) || | ||
88 | + (env->irq_line_state & CPU_INTERRUPT_VFIQ); | ||
89 | + | ||
90 | + if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VFIQ) != 0)) { | ||
91 | + if (new_state) { | ||
92 | + cpu_interrupt(cs, CPU_INTERRUPT_VFIQ); | ||
93 | + } else { | ||
94 | + cpu_reset_interrupt(cs, CPU_INTERRUPT_VFIQ); | ||
95 | + } | ||
96 | + } | ||
97 | +} | ||
98 | + | ||
99 | #ifndef CONFIG_USER_ONLY | ||
100 | static void arm_cpu_set_irq(void *opaque, int irq, int level) | ||
101 | { | ||
102 | @@ -XXX,XX +XXX,XX @@ static void arm_cpu_set_irq(void *opaque, int irq, int level) | ||
103 | |||
104 | switch (irq) { | ||
105 | case ARM_CPU_VIRQ: | ||
106 | + assert(arm_feature(env, ARM_FEATURE_EL2)); | ||
107 | + arm_cpu_update_virq(cpu); | ||
108 | + break; | ||
109 | case ARM_CPU_VFIQ: | ||
110 | assert(arm_feature(env, ARM_FEATURE_EL2)); | ||
111 | - /* fall through */ | ||
112 | + arm_cpu_update_vfiq(cpu); | ||
113 | + break; | ||
114 | case ARM_CPU_IRQ: | ||
115 | case ARM_CPU_FIQ: | ||
116 | if (level) { | ||
117 | diff --git a/target/arm/helper.c b/target/arm/helper.c | ||
118 | index XXXXXXX..XXXXXXX 100644 | ||
119 | --- a/target/arm/helper.c | ||
120 | +++ b/target/arm/helper.c | ||
121 | @@ -XXX,XX +XXX,XX @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) | ||
122 | tlb_flush(CPU(cpu)); | ||
123 | } | ||
124 | env->cp15.hcr_el2 = value; | ||
125 | + | ||
126 | + /* | ||
127 | + * Updates to VI and VF require us to update the status of | ||
128 | + * virtual interrupts, which are the logical OR of these bits | ||
129 | + * and the state of the input lines from the GIC. (This requires | ||
130 | + * that we have the iothread lock, which is done by marking the | ||
131 | + * reginfo structs as ARM_CP_IO.) | ||
132 | + * Note that if a write to HCR pends a VIRQ or VFIQ it is never | ||
133 | + * possible for it to be taken immediately, because VIRQ and | ||
134 | + * VFIQ are masked unless running at EL0 or EL1, and HCR | ||
135 | + * can only be written at EL2. | ||
136 | + */ | ||
137 | + g_assert(qemu_mutex_iothread_locked()); | ||
138 | + arm_cpu_update_virq(cpu); | ||
139 | + arm_cpu_update_vfiq(cpu); | ||
140 | } | ||
141 | |||
142 | static void hcr_writehigh(CPUARMState *env, const ARMCPRegInfo *ri, | ||
143 | @@ -XXX,XX +XXX,XX @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri, | ||
144 | |||
145 | static const ARMCPRegInfo el2_cp_reginfo[] = { | ||
146 | { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64, | ||
147 | + .type = ARM_CP_IO, | ||
148 | .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0, | ||
149 | .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2), | ||
150 | .writefn = hcr_write }, | ||
151 | { .name = "HCR", .state = ARM_CP_STATE_AA32, | ||
152 | - .type = ARM_CP_ALIAS, | ||
153 | + .type = ARM_CP_ALIAS | ARM_CP_IO, | ||
154 | .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0, | ||
155 | .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2), | ||
156 | .writefn = hcr_writelow }, | ||
157 | @@ -XXX,XX +XXX,XX @@ static const ARMCPRegInfo el2_cp_reginfo[] = { | ||
158 | |||
159 | static const ARMCPRegInfo el2_v8_cp_reginfo[] = { | ||
160 | { .name = "HCR2", .state = ARM_CP_STATE_AA32, | ||
161 | - .type = ARM_CP_ALIAS, | ||
162 | + .type = ARM_CP_ALIAS | ARM_CP_IO, | ||
163 | .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4, | ||
164 | .access = PL2_RW, | ||
165 | .fieldoffset = offsetofhigh32(CPUARMState, cp15.hcr_el2), | ||
166 | -- | 53 | -- |
167 | 2.19.1 | 54 | 2.20.1 |
168 | 55 | ||
169 | 56 | diff view generated by jsdifflib |
1 | From: Alex Bennée <alex.bennee@linaro.org> | 1 | From: Philippe Mathieu-Daudé <philmd@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | You should declare you are using a global version of a variable before | 3 | Both lqspi_read() and lqspi_load_cache() expect a 32-bit |
4 | you attempt to modify it in a function. | 4 | aligned address. |
5 | 5 | ||
6 | Signed-off-by: Alex Bennée <alex.bennee@linaro.org> | 6 | >From UG1085 datasheet [*] chapter on 'Quad-SPI Controller': |
7 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | 7 | |
8 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | 8 | Transfer Size Limitations |
9 | Message-id: 20181109152119.9242-5-alex.bennee@linaro.org | 9 | |
10 | Because of the 32-bit wide TX, RX, and generic FIFO, all | ||
11 | APB/AXI transfers must be an integer multiple of 4-bytes. | ||
12 | Shorter transfers are not possible. | ||
13 | |||
14 | Set MemoryRegionOps.impl values to force 32-bit accesses, | ||
15 | this way we are sure we do not access the lqspi_buf[] array | ||
16 | out of bound. | ||
17 | |||
18 | [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf | ||
19 | |||
20 | Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> | ||
21 | Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com> | ||
22 | Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
10 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 23 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
11 | --- | 24 | --- |
12 | tests/guest-debug/test-gdbstub.py | 1 + | 25 | hw/ssi/xilinx_spips.c | 4 ++++ |
13 | 1 file changed, 1 insertion(+) | 26 | 1 file changed, 4 insertions(+) |
14 | 27 | ||
15 | diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py | 28 | diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c |
16 | index XXXXXXX..XXXXXXX 100644 | 29 | index XXXXXXX..XXXXXXX 100644 |
17 | --- a/tests/guest-debug/test-gdbstub.py | 30 | --- a/hw/ssi/xilinx_spips.c |
18 | +++ b/tests/guest-debug/test-gdbstub.py | 31 | +++ b/hw/ssi/xilinx_spips.c |
19 | @@ -XXX,XX +XXX,XX @@ def report(cond, msg): | 32 | @@ -XXX,XX +XXX,XX @@ static const MemoryRegionOps lqspi_ops = { |
20 | print ("PASS: %s" % (msg)) | 33 | .read_with_attrs = lqspi_read, |
21 | else: | 34 | .write_with_attrs = lqspi_write, |
22 | print ("FAIL: %s" % (msg)) | 35 | .endianness = DEVICE_NATIVE_ENDIAN, |
23 | + global failcount | 36 | + .impl = { |
24 | failcount += 1 | 37 | + .min_access_size = 4, |
25 | 38 | + .max_access_size = 4, | |
26 | 39 | + }, | |
40 | .valid = { | ||
41 | .min_access_size = 1, | ||
42 | .max_access_size = 4 | ||
27 | -- | 43 | -- |
28 | 2.19.1 | 44 | 2.20.1 |
29 | 45 | ||
30 | 46 | diff view generated by jsdifflib |
1 | From: Alex Bennée <alex.bennee@linaro.org> | 1 | From: Philippe Mathieu-Daudé <philmd@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | When we are debugging the guest all exceptions come our way but might | 3 | Reading the RX_DATA register when the RX_FIFO is empty triggers |
4 | be for the guest's own debug exceptions. We use the ->do_interrupt() | 4 | an abort. This can be easily reproduced: |
5 | infrastructure to inject the exception into the guest. However, we are | ||
6 | missing a full setup of the exception structure, causing an assert | ||
7 | later down the line. | ||
8 | 5 | ||
9 | Signed-off-by: Alex Bennée <alex.bennee@linaro.org> | 6 | $ qemu-system-arm -M emcraft-sf2 -monitor stdio -S |
10 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | 7 | QEMU 4.0.50 monitor - type 'help' for more information |
11 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | 8 | (qemu) x 0x40001010 |
12 | Message-id: 20181109152119.9242-4-alex.bennee@linaro.org | 9 | Aborted (core dumped) |
10 | |||
11 | (gdb) bt | ||
12 | #1 0x00007f035874f895 in abort () at /lib64/libc.so.6 | ||
13 | #2 0x00005628686591ff in fifo8_pop (fifo=0x56286a9a4c68) at util/fifo8.c:66 | ||
14 | #3 0x00005628683e0b8e in fifo32_pop (fifo=0x56286a9a4c68) at include/qemu/fifo32.h:137 | ||
15 | #4 0x00005628683e0efb in spi_read (opaque=0x56286a9a4850, addr=4, size=4) at hw/ssi/mss-spi.c:168 | ||
16 | #5 0x0000562867f96801 in memory_region_read_accessor (mr=0x56286a9a4b60, addr=16, value=0x7ffeecb0c5c8, size=4, shift=0, mask=4294967295, attrs=...) at memory.c:439 | ||
17 | #6 0x0000562867f96cdb in access_with_adjusted_size (addr=16, value=0x7ffeecb0c5c8, size=4, access_size_min=1, access_size_max=4, access_fn=0x562867f967c3 <memory_region_read_accessor>, mr=0x56286a9a4b60, attrs=...) at memory.c:569 | ||
18 | #7 0x0000562867f99940 in memory_region_dispatch_read1 (mr=0x56286a9a4b60, addr=16, pval=0x7ffeecb0c5c8, size=4, attrs=...) at memory.c:1420 | ||
19 | #8 0x0000562867f99a08 in memory_region_dispatch_read (mr=0x56286a9a4b60, addr=16, pval=0x7ffeecb0c5c8, size=4, attrs=...) at memory.c:1447 | ||
20 | #9 0x0000562867f38721 in flatview_read_continue (fv=0x56286aec6360, addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, addr1=16, l=4, mr=0x56286a9a4b60) at exec.c:3385 | ||
21 | #10 0x0000562867f38874 in flatview_read (fv=0x56286aec6360, addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4) at exec.c:3423 | ||
22 | #11 0x0000562867f388ea in address_space_read_full (as=0x56286aa3e890, addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4) at exec.c:3436 | ||
23 | #12 0x0000562867f389c5 in address_space_rw (as=0x56286aa3e890, addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, is_write=false) at exec.c:3466 | ||
24 | #13 0x0000562867f3bdd7 in cpu_memory_rw_debug (cpu=0x56286aa19d00, addr=1073745936, buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, is_write=0) at exec.c:3976 | ||
25 | #14 0x000056286811ed51 in memory_dump (mon=0x56286a8c32d0, count=1, format=120, wsize=4, addr=1073745936, is_physical=0) at monitor/misc.c:730 | ||
26 | #15 0x000056286811eff1 in hmp_memory_dump (mon=0x56286a8c32d0, qdict=0x56286b15c400) at monitor/misc.c:785 | ||
27 | #16 0x00005628684740ee in handle_hmp_command (mon=0x56286a8c32d0, cmdline=0x56286a8caeb2 "0x40001010") at monitor/hmp.c:1082 | ||
28 | |||
29 | From the datasheet "Actel SmartFusion Microcontroller Subsystem | ||
30 | User's Guide" Rev.1, Table 13-3 "SPI Register Summary", this | ||
31 | register has a reset value of 0. | ||
32 | |||
33 | Check the FIFO is not empty before accessing it, else log an | ||
34 | error message. | ||
35 | |||
36 | Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
37 | Reviewed-by: Alistair Francis <alistair.francis@wdc.com> | ||
38 | Message-id: 20190709113715.7761-3-philmd@redhat.com | ||
13 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 39 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
14 | --- | 40 | --- |
15 | target/arm/kvm64.c | 1 + | 41 | hw/ssi/mss-spi.c | 8 +++++++- |
16 | 1 file changed, 1 insertion(+) | 42 | 1 file changed, 7 insertions(+), 1 deletion(-) |
17 | 43 | ||
18 | diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c | 44 | diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c |
19 | index XXXXXXX..XXXXXXX 100644 | 45 | index XXXXXXX..XXXXXXX 100644 |
20 | --- a/target/arm/kvm64.c | 46 | --- a/hw/ssi/mss-spi.c |
21 | +++ b/target/arm/kvm64.c | 47 | +++ b/hw/ssi/mss-spi.c |
22 | @@ -XXX,XX +XXX,XX @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit) | 48 | @@ -XXX,XX +XXX,XX @@ spi_read(void *opaque, hwaddr addr, unsigned int size) |
23 | cs->exception_index = EXCP_BKPT; | 49 | case R_SPI_RX: |
24 | env->exception.syndrome = debug_exit->hsr; | 50 | s->regs[R_SPI_STATUS] &= ~S_RXFIFOFUL; |
25 | env->exception.vaddress = debug_exit->far; | 51 | s->regs[R_SPI_STATUS] &= ~S_RXCHOVRF; |
26 | + env->exception.target_el = 1; | 52 | - ret = fifo32_pop(&s->rx_fifo); |
27 | qemu_mutex_lock_iothread(); | 53 | + if (fifo32_is_empty(&s->rx_fifo)) { |
28 | cc->do_interrupt(cs); | 54 | + qemu_log_mask(LOG_GUEST_ERROR, |
29 | qemu_mutex_unlock_iothread(); | 55 | + "%s: Reading empty RX_FIFO\n", |
56 | + __func__); | ||
57 | + } else { | ||
58 | + ret = fifo32_pop(&s->rx_fifo); | ||
59 | + } | ||
60 | if (fifo32_is_empty(&s->rx_fifo)) { | ||
61 | s->regs[R_SPI_STATUS] |= S_RXFIFOEMP; | ||
62 | } | ||
30 | -- | 63 | -- |
31 | 2.19.1 | 64 | 2.20.1 |
32 | 65 | ||
33 | 66 | diff view generated by jsdifflib |
1 | From: Eric Auger <eric.auger@redhat.com> | 1 | From: Philippe Mathieu-Daudé <philmd@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | Commit af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with DT | 3 | In the previous commit we fixed a crash when the guest read a |
4 | compatible value) introduced a match_fn callback which gets called | 4 | register that pop from an empty FIFO. |
5 | for each registered combo to check whether a sysbus device can be | 5 | By auditing the repository, we found another similar use with |
6 | dynamically instantiated. However the callback gets called even if | 6 | an easy way to reproduce: |
7 | the device type does not match the binding combo typename field. | ||
8 | This causes an assert when passing "-device ramfb" to the qemu | ||
9 | command line as vfio_platform_match() gets called on a non | ||
10 | vfio-platform device. | ||
11 | 7 | ||
12 | To fix this regression, let's change the add_fdt_node() logic so | 8 | $ qemu-system-aarch64 -M xlnx-zcu102 -monitor stdio -S |
13 | that we first check the type and if the match_fn callback is defined, | 9 | QEMU 4.0.50 monitor - type 'help' for more information |
14 | then we also call it. | 10 | (qemu) xp/b 0xfd4a0134 |
11 | Aborted (core dumped) | ||
15 | 12 | ||
16 | Binding combos only requesting a type check do not define the | 13 | (gdb) bt |
17 | match_fn callback. | 14 | #0 0x00007f6936dea57f in raise () at /lib64/libc.so.6 |
15 | #1 0x00007f6936dd4895 in abort () at /lib64/libc.so.6 | ||
16 | #2 0x0000561ad32975ec in xlnx_dp_aux_pop_rx_fifo (s=0x7f692babee70) at hw/display/xlnx_dp.c:431 | ||
17 | #3 0x0000561ad3297dc0 in xlnx_dp_read (opaque=0x7f692babee70, offset=77, size=4) at hw/display/xlnx_dp.c:667 | ||
18 | #4 0x0000561ad321b896 in memory_region_read_accessor (mr=0x7f692babf620, addr=308, value=0x7ffe05c1db88, size=4, shift=0, mask=4294967295, attrs=...) at memory.c:439 | ||
19 | #5 0x0000561ad321bd70 in access_with_adjusted_size (addr=308, value=0x7ffe05c1db88, size=1, access_size_min=4, access_size_max=4, access_fn=0x561ad321b858 <memory_region_read_accessor>, mr=0x7f692babf620, attrs=...) at memory.c:569 | ||
20 | #6 0x0000561ad321e9d5 in memory_region_dispatch_read1 (mr=0x7f692babf620, addr=308, pval=0x7ffe05c1db88, size=1, attrs=...) at memory.c:1420 | ||
21 | #7 0x0000561ad321ea9d in memory_region_dispatch_read (mr=0x7f692babf620, addr=308, pval=0x7ffe05c1db88, size=1, attrs=...) at memory.c:1447 | ||
22 | #8 0x0000561ad31bd742 in flatview_read_continue (fv=0x561ad69c04f0, addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", len=1, addr1=308, l=1, mr=0x7f692babf620) at exec.c:3385 | ||
23 | #9 0x0000561ad31bd895 in flatview_read (fv=0x561ad69c04f0, addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", len=1) at exec.c:3423 | ||
24 | #10 0x0000561ad31bd90b in address_space_read_full (as=0x561ad5bb3020, addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", len=1) at exec.c:3436 | ||
25 | #11 0x0000561ad33b1c42 in address_space_read (len=1, buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", attrs=..., addr=4249485620, as=0x561ad5bb3020) at include/exec/memory.h:2131 | ||
26 | #12 0x0000561ad33b1c42 in memory_dump (mon=0x561ad59c4530, count=1, format=120, wsize=1, addr=4249485620, is_physical=1) at monitor/misc.c:723 | ||
27 | #13 0x0000561ad33b1fc1 in hmp_physical_memory_dump (mon=0x561ad59c4530, qdict=0x561ad6c6fd00) at monitor/misc.c:795 | ||
28 | #14 0x0000561ad37b4a9f in handle_hmp_command (mon=0x561ad59c4530, cmdline=0x561ad59d0f22 "/b 0x00000000fd4a0134") at monitor/hmp.c:1082 | ||
18 | 29 | ||
19 | Fixes: af7d64ede0b9 (hw/arm/sysbus-fdt: Allow device matching with | 30 | Fix by checking the FIFO is not empty before popping from it. |
20 | DT compatible value) | ||
21 | 31 | ||
22 | Signed-off-by: Eric Auger <eric.auger@redhat.com> | 32 | The datasheet is not clear about the reset value of this register, |
23 | Reported-by: Thomas Huth <thuth@redhat.com> | 33 | we choose to return '0'. |
24 | Reviewed-by: Alex Williamson <alex.williamson@redhat.com> | 34 | |
25 | Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> | 35 | Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> |
26 | Message-id: 20181106184212.29377-1-eric.auger@redhat.com | 36 | Reviewed-by: Alistair Francis <alistair.francis@wdc.com> |
37 | Message-id: 20190709113715.7761-4-philmd@redhat.com | ||
27 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 38 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
28 | --- | 39 | --- |
29 | hw/arm/sysbus-fdt.c | 12 +++++++----- | 40 | hw/display/xlnx_dp.c | 15 +++++++++++---- |
30 | 1 file changed, 7 insertions(+), 5 deletions(-) | 41 | 1 file changed, 11 insertions(+), 4 deletions(-) |
31 | 42 | ||
32 | diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c | 43 | diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c |
33 | index XXXXXXX..XXXXXXX 100644 | 44 | index XXXXXXX..XXXXXXX 100644 |
34 | --- a/hw/arm/sysbus-fdt.c | 45 | --- a/hw/display/xlnx_dp.c |
35 | +++ b/hw/arm/sysbus-fdt.c | 46 | +++ b/hw/display/xlnx_dp.c |
36 | @@ -XXX,XX +XXX,XX @@ static bool type_match(SysBusDevice *sbdev, const BindingEntry *entry) | 47 | @@ -XXX,XX +XXX,XX @@ static uint8_t xlnx_dp_aux_pop_rx_fifo(XlnxDPState *s) |
37 | return !strcmp(object_get_typename(OBJECT(sbdev)), entry->typename); | 48 | uint8_t ret; |
49 | |||
50 | if (fifo8_is_empty(&s->rx_fifo)) { | ||
51 | - DPRINTF("rx_fifo underflow..\n"); | ||
52 | - abort(); | ||
53 | + qemu_log_mask(LOG_GUEST_ERROR, | ||
54 | + "%s: Reading empty RX_FIFO\n", | ||
55 | + __func__); | ||
56 | + /* | ||
57 | + * The datasheet is not clear about the reset value, it seems | ||
58 | + * to be unspecified. We choose to return '0'. | ||
59 | + */ | ||
60 | + ret = 0; | ||
61 | + } else { | ||
62 | + ret = fifo8_pop(&s->rx_fifo); | ||
63 | + DPRINTF("pop 0x%" PRIX8 " from rx_fifo.\n", ret); | ||
64 | } | ||
65 | - ret = fifo8_pop(&s->rx_fifo); | ||
66 | - DPRINTF("pop 0x%" PRIX8 " from rx_fifo.\n", ret); | ||
67 | return ret; | ||
38 | } | 68 | } |
39 | 69 | ||
40 | -#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), type_match} | ||
41 | +#define TYPE_BINDING(type, add_fn) {(type), NULL, (add_fn), NULL} | ||
42 | |||
43 | /* list of supported dynamic sysbus bindings */ | ||
44 | static const BindingEntry bindings[] = { | ||
45 | @@ -XXX,XX +XXX,XX @@ static void add_fdt_node(SysBusDevice *sbdev, void *opaque) | ||
46 | for (i = 0; i < ARRAY_SIZE(bindings); i++) { | ||
47 | const BindingEntry *iter = &bindings[i]; | ||
48 | |||
49 | - if (iter->match_fn(sbdev, iter)) { | ||
50 | - ret = iter->add_fn(sbdev, opaque); | ||
51 | - assert(!ret); | ||
52 | - return; | ||
53 | + if (type_match(sbdev, iter)) { | ||
54 | + if (!iter->match_fn || iter->match_fn(sbdev, iter)) { | ||
55 | + ret = iter->add_fn(sbdev, opaque); | ||
56 | + assert(!ret); | ||
57 | + return; | ||
58 | + } | ||
59 | } | ||
60 | } | ||
61 | error_report("Device %s can not be dynamically instantiated", | ||
62 | -- | 70 | -- |
63 | 2.19.1 | 71 | 2.20.1 |
64 | 72 | ||
65 | 73 | diff view generated by jsdifflib |
1 | From: Richard Henderson <richard.henderson@linaro.org> | 1 | From: David Engraf <david.engraf@sysgo.com> |
---|---|---|---|
2 | 2 | ||
3 | This would cause an infinite recursion or loop. | 3 | Using the whole 128 MiB flash in non-secure mode is not working because |
4 | virt_flash_fdt() expects the same address for secure_sysmem and sysmem. | ||
5 | This is not correctly handled by caller because it forwards NULL for | ||
6 | secure_sysmem in non-secure flash mode. | ||
4 | 7 | ||
5 | Signed-off-by: Richard Henderson <richard.henderson@linaro.org> | 8 | Fixed by using sysmem when secure_sysmem is NULL. |
6 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | 9 | |
7 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | 10 | Signed-off-by: David Engraf <david.engraf@sysgo.com> |
8 | Message-id: 20181110121711.15257-1-richard.henderson@linaro.org | 11 | Message-id: 20190712075002.14326-1-david.engraf@sysgo.com |
9 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | 12 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> |
10 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 13 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
11 | --- | 14 | --- |
12 | target/arm/helper.c | 2 +- | 15 | hw/arm/virt.c | 2 +- |
13 | 1 file changed, 1 insertion(+), 1 deletion(-) | 16 | 1 file changed, 1 insertion(+), 1 deletion(-) |
14 | 17 | ||
15 | diff --git a/target/arm/helper.c b/target/arm/helper.c | 18 | diff --git a/hw/arm/virt.c b/hw/arm/virt.c |
16 | index XXXXXXX..XXXXXXX 100644 | 19 | index XXXXXXX..XXXXXXX 100644 |
17 | --- a/target/arm/helper.c | 20 | --- a/hw/arm/virt.c |
18 | +++ b/target/arm/helper.c | 21 | +++ b/hw/arm/virt.c |
19 | @@ -XXX,XX +XXX,XX @@ static void tlbi_aa64_vmalle1_write(CPUARMState *env, const ARMCPRegInfo *ri, | 22 | @@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine) |
20 | CPUState *cs = ENV_GET_CPU(env); | 23 | &machine->device_memory->mr); |
21 | |||
22 | if (tlb_force_broadcast(env)) { | ||
23 | - tlbi_aa64_vmalle1_write(env, NULL, value); | ||
24 | + tlbi_aa64_vmalle1is_write(env, NULL, value); | ||
25 | return; | ||
26 | } | 24 | } |
27 | 25 | ||
26 | - virt_flash_fdt(vms, sysmem, secure_sysmem); | ||
27 | + virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem); | ||
28 | |||
29 | create_gic(vms, pic); | ||
30 | |||
28 | -- | 31 | -- |
29 | 2.19.1 | 32 | 2.20.1 |
30 | 33 | ||
31 | 34 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | From: Alex Bennée <alex.bennee@linaro.org> | ||
2 | 1 | ||
3 | This only fails with some (broken) versions of gdb but we should | ||
4 | treat the top bits of DBGBVR as RESS. Properly sign extend QEMU's | ||
5 | reference copy of dbgbvr and also update the register descriptions in | ||
6 | the comment. | ||
7 | |||
8 | Signed-off-by: Alex Bennée <alex.bennee@linaro.org> | ||
9 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
10 | Message-id: 20181109152119.9242-2-alex.bennee@linaro.org | ||
11 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
12 | --- | ||
13 | target/arm/kvm64.c | 17 +++++++++++++++-- | ||
14 | 1 file changed, 15 insertions(+), 2 deletions(-) | ||
15 | |||
16 | diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c | ||
17 | index XXXXXXX..XXXXXXX 100644 | ||
18 | --- a/target/arm/kvm64.c | ||
19 | +++ b/target/arm/kvm64.c | ||
20 | @@ -XXX,XX +XXX,XX @@ static void kvm_arm_init_debug(CPUState *cs) | ||
21 | * capable of fancier matching but that will require exposing that | ||
22 | * fanciness to GDB's interface | ||
23 | * | ||
24 | - * D7.3.2 DBGBCR<n>_EL1, Debug Breakpoint Control Registers | ||
25 | + * DBGBCR<n>_EL1, Debug Breakpoint Control Registers | ||
26 | * | ||
27 | * 31 24 23 20 19 16 15 14 13 12 9 8 5 4 3 2 1 0 | ||
28 | * +------+------+-------+-----+----+------+-----+------+-----+---+ | ||
29 | @@ -XXX,XX +XXX,XX @@ static void kvm_arm_init_debug(CPUState *cs) | ||
30 | * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12) | ||
31 | * BAS: Byte Address Select (RES1 for AArch64) | ||
32 | * E: Enable bit | ||
33 | + * | ||
34 | + * DBGBVR<n>_EL1, Debug Breakpoint Value Registers | ||
35 | + * | ||
36 | + * 63 53 52 49 48 2 1 0 | ||
37 | + * +------+-----------+----------+-----+ | ||
38 | + * | RESS | VA[52:49] | VA[48:2] | 0 0 | | ||
39 | + * +------+-----------+----------+-----+ | ||
40 | + * | ||
41 | + * Depending on the addressing mode bits the top bits of the register | ||
42 | + * are a sign extension of the highest applicable VA bit. Some | ||
43 | + * versions of GDB don't do it correctly so we ensure they are correct | ||
44 | + * here so future PC comparisons will work properly. | ||
45 | */ | ||
46 | + | ||
47 | static int insert_hw_breakpoint(target_ulong addr) | ||
48 | { | ||
49 | HWBreakpoint brk = { | ||
50 | .bcr = 0x1, /* BCR E=1, enable */ | ||
51 | - .bvr = addr | ||
52 | + .bvr = sextract64(addr, 0, 53) | ||
53 | }; | ||
54 | |||
55 | if (cur_hw_bps >= max_hw_bps) { | ||
56 | -- | ||
57 | 2.19.1 | ||
58 | |||
59 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | From: Alex Bennée <alex.bennee@linaro.org> | ||
2 | 1 | ||
3 | Fix the assertion failure when running interrupts. | ||
4 | |||
5 | Signed-off-by: Alex Bennée <alex.bennee@linaro.org> | ||
6 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | ||
7 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
8 | Message-id: 20181109152119.9242-3-alex.bennee@linaro.org | ||
9 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
10 | --- | ||
11 | target/arm/kvm64.c | 2 ++ | ||
12 | 1 file changed, 2 insertions(+) | ||
13 | |||
14 | diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c | ||
15 | index XXXXXXX..XXXXXXX 100644 | ||
16 | --- a/target/arm/kvm64.c | ||
17 | +++ b/target/arm/kvm64.c | ||
18 | @@ -XXX,XX +XXX,XX @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit) | ||
19 | cs->exception_index = EXCP_BKPT; | ||
20 | env->exception.syndrome = debug_exit->hsr; | ||
21 | env->exception.vaddress = debug_exit->far; | ||
22 | + qemu_mutex_lock_iothread(); | ||
23 | cc->do_interrupt(cs); | ||
24 | + qemu_mutex_unlock_iothread(); | ||
25 | |||
26 | return false; | ||
27 | } | ||
28 | -- | ||
29 | 2.19.1 | ||
30 | |||
31 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | This reverts commit 8a0fc3a29fc2315325400c738f807d0d4ae0ab7f. | ||
2 | 1 | ||
3 | The implementation of HCR.VI and VF in that commit is not | ||
4 | correct -- they do not track the overall "is there a pending | ||
5 | VIRQ or VFIQ" status, but whether there is a pending interrupt | ||
6 | due to "this mechanism", ie the hypervisor having set the VI/VF | ||
7 | bits. The overall pending state for VIRQ and VFIQ is effectively | ||
8 | the logical OR of the inbound lines from the GIC with the | ||
9 | VI and VF bits. Commit 8a0fc3a29fc231 would result in pending | ||
10 | VIRQ/VFIQ possibly being lost when the hypervisor wrote to HCR. | ||
11 | |||
12 | As a preliminary to implementing the HCR.VI/VF feature properly, | ||
13 | revert the broken one entirely. | ||
14 | |||
15 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
16 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | ||
17 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
18 | Message-id: 20181109134731.11605-2-peter.maydell@linaro.org | ||
19 | --- | ||
20 | target/arm/helper.c | 47 ++++----------------------------------------- | ||
21 | 1 file changed, 4 insertions(+), 43 deletions(-) | ||
22 | |||
23 | diff --git a/target/arm/helper.c b/target/arm/helper.c | ||
24 | index XXXXXXX..XXXXXXX 100644 | ||
25 | --- a/target/arm/helper.c | ||
26 | +++ b/target/arm/helper.c | ||
27 | @@ -XXX,XX +XXX,XX @@ static const ARMCPRegInfo el3_no_el2_v8_cp_reginfo[] = { | ||
28 | static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) | ||
29 | { | ||
30 | ARMCPU *cpu = arm_env_get_cpu(env); | ||
31 | - CPUState *cs = ENV_GET_CPU(env); | ||
32 | uint64_t valid_mask = HCR_MASK; | ||
33 | |||
34 | if (arm_feature(env, ARM_FEATURE_EL3)) { | ||
35 | @@ -XXX,XX +XXX,XX @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) | ||
36 | /* Clear RES0 bits. */ | ||
37 | value &= valid_mask; | ||
38 | |||
39 | - /* | ||
40 | - * VI and VF are kept in cs->interrupt_request. Modifying that | ||
41 | - * requires that we have the iothread lock, which is done by | ||
42 | - * marking the reginfo structs as ARM_CP_IO. | ||
43 | - * Note that if a write to HCR pends a VIRQ or VFIQ it is never | ||
44 | - * possible for it to be taken immediately, because VIRQ and | ||
45 | - * VFIQ are masked unless running at EL0 or EL1, and HCR | ||
46 | - * can only be written at EL2. | ||
47 | - */ | ||
48 | - g_assert(qemu_mutex_iothread_locked()); | ||
49 | - if (value & HCR_VI) { | ||
50 | - cs->interrupt_request |= CPU_INTERRUPT_VIRQ; | ||
51 | - } else { | ||
52 | - cs->interrupt_request &= ~CPU_INTERRUPT_VIRQ; | ||
53 | - } | ||
54 | - if (value & HCR_VF) { | ||
55 | - cs->interrupt_request |= CPU_INTERRUPT_VFIQ; | ||
56 | - } else { | ||
57 | - cs->interrupt_request &= ~CPU_INTERRUPT_VFIQ; | ||
58 | - } | ||
59 | - value &= ~(HCR_VI | HCR_VF); | ||
60 | - | ||
61 | /* These bits change the MMU setup: | ||
62 | * HCR_VM enables stage 2 translation | ||
63 | * HCR_PTW forbids certain page-table setups | ||
64 | @@ -XXX,XX +XXX,XX @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri, | ||
65 | hcr_write(env, NULL, value); | ||
66 | } | ||
67 | |||
68 | -static uint64_t hcr_read(CPUARMState *env, const ARMCPRegInfo *ri) | ||
69 | -{ | ||
70 | - /* The VI and VF bits live in cs->interrupt_request */ | ||
71 | - uint64_t ret = env->cp15.hcr_el2 & ~(HCR_VI | HCR_VF); | ||
72 | - CPUState *cs = ENV_GET_CPU(env); | ||
73 | - | ||
74 | - if (cs->interrupt_request & CPU_INTERRUPT_VIRQ) { | ||
75 | - ret |= HCR_VI; | ||
76 | - } | ||
77 | - if (cs->interrupt_request & CPU_INTERRUPT_VFIQ) { | ||
78 | - ret |= HCR_VF; | ||
79 | - } | ||
80 | - return ret; | ||
81 | -} | ||
82 | - | ||
83 | static const ARMCPRegInfo el2_cp_reginfo[] = { | ||
84 | { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64, | ||
85 | - .type = ARM_CP_IO, | ||
86 | .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0, | ||
87 | .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2), | ||
88 | - .writefn = hcr_write, .readfn = hcr_read }, | ||
89 | + .writefn = hcr_write }, | ||
90 | { .name = "HCR", .state = ARM_CP_STATE_AA32, | ||
91 | - .type = ARM_CP_ALIAS | ARM_CP_IO, | ||
92 | + .type = ARM_CP_ALIAS, | ||
93 | .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0, | ||
94 | .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2), | ||
95 | - .writefn = hcr_writelow, .readfn = hcr_read }, | ||
96 | + .writefn = hcr_writelow }, | ||
97 | { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64, | ||
98 | .type = ARM_CP_ALIAS, | ||
99 | .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1, | ||
100 | @@ -XXX,XX +XXX,XX @@ static const ARMCPRegInfo el2_cp_reginfo[] = { | ||
101 | |||
102 | static const ARMCPRegInfo el2_v8_cp_reginfo[] = { | ||
103 | { .name = "HCR2", .state = ARM_CP_STATE_AA32, | ||
104 | - .type = ARM_CP_ALIAS | ARM_CP_IO, | ||
105 | + .type = ARM_CP_ALIAS, | ||
106 | .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4, | ||
107 | .access = PL2_RW, | ||
108 | .fieldoffset = offsetofhigh32(CPUARMState, cp15.hcr_el2), | ||
109 | -- | ||
110 | 2.19.1 | ||
111 | |||
112 | diff view generated by jsdifflib |
1 | Currently we track the state of the four irq lines from the GIC | 1 | The PL031 RTC tracks the difference between the guest RTC |
---|---|---|---|
2 | only via the cs->interrupt_request or KVM irq state. That means | 2 | and the host RTC using a tick_offset field. For migration, |
3 | that we assume that an interrupt is asserted if and only if the | 3 | however, we currently always migrate the offset between |
4 | external line is set. This assumption is incorrect for VIRQ | 4 | the guest and the vm_clock, even if the RTC clock is not |
5 | and VFIQ, because the HCR_EL2.{VI,VF} bits allow assertion | 5 | the same as the vm_clock; this was an attempt to retain |
6 | of VIRQ and VFIQ separately from the state of the external line. | 6 | migration backwards compatibility. |
7 | 7 | ||
8 | To handle this, start tracking the state of the external lines | 8 | Unfortunately this results in the RTC behaving oddly across |
9 | explicitly in a CPU state struct field, as is common practice | 9 | a VM state save and restore -- since the VM clock stands still |
10 | for devices. | 10 | across save-then-restore, regardless of how much real world |
11 | 11 | time has elapsed, the guest RTC ends up out of sync with the | |
12 | The complicated part of this is dealing with inbound migration | 12 | host RTC in the restored VM. |
13 | from an older QEMU which didn't have this state. We assume in | 13 | |
14 | that case that the older QEMU did not implement the HCR_EL2.{VI,VF} | 14 | Fix this by migrating the raw tick_offset. To retain migration |
15 | bits as generating interrupts, and so the line state matches | 15 | compatibility as far as possible, we have a new property |
16 | the current state in cs->interrupt_request. (This is not quite | 16 | migrate-tick-offset; by default this is 'true' and we will |
17 | true between commit 8a0fc3a29fc2315325400c7 and its revert, but | 17 | migrate the true tick offset in a new subsection; if the |
18 | that commit is broken and never made it into any released QEMU | 18 | incoming data has no subsection we fall back to the old |
19 | version.) | 19 | vm_clock-based offset information, so old->new migration |
20 | 20 | compatibility is preserved. For complete new->old migration | |
21 | compatibility, the property is set to 'false' for 4.0 and | ||
22 | earlier machine types (this will only affect 'virt-4.0' | ||
23 | and below, as none of the other pl031-using machines are | ||
24 | versioned). | ||
25 | |||
26 | Reported-by: Russell King <rmk@armlinux.org.uk> | ||
21 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 27 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
22 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | 28 | Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> |
23 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | 29 | Message-id: 20190709143912.28905-1-peter.maydell@linaro.org |
24 | Message-id: 20181109134731.11605-3-peter.maydell@linaro.org | ||
25 | --- | 30 | --- |
26 | target/arm/cpu.h | 3 +++ | 31 | include/hw/timer/pl031.h | 2 + |
27 | target/arm/cpu.c | 16 ++++++++++++++ | 32 | hw/core/machine.c | 1 + |
28 | target/arm/machine.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ | 33 | hw/timer/pl031.c | 92 ++++++++++++++++++++++++++++++++++++++-- |
29 | 3 files changed, 70 insertions(+) | 34 | 3 files changed, 91 insertions(+), 4 deletions(-) |
30 | 35 | ||
31 | diff --git a/target/arm/cpu.h b/target/arm/cpu.h | 36 | diff --git a/include/hw/timer/pl031.h b/include/hw/timer/pl031.h |
32 | index XXXXXXX..XXXXXXX 100644 | 37 | index XXXXXXX..XXXXXXX 100644 |
33 | --- a/target/arm/cpu.h | 38 | --- a/include/hw/timer/pl031.h |
34 | +++ b/target/arm/cpu.h | 39 | +++ b/include/hw/timer/pl031.h |
35 | @@ -XXX,XX +XXX,XX @@ typedef struct CPUARMState { | 40 | @@ -XXX,XX +XXX,XX @@ typedef struct PL031State { |
36 | uint64_t esr; | 41 | */ |
37 | } serror; | 42 | uint32_t tick_offset_vmstate; |
38 | 43 | uint32_t tick_offset; | |
39 | + /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ | 44 | + bool tick_offset_migrated; |
40 | + uint32_t irq_line_state; | 45 | + bool migrate_tick_offset; |
41 | + | 46 | |
42 | /* Thumb-2 EE state. */ | 47 | uint32_t mr; |
43 | uint32_t teecr; | 48 | uint32_t lr; |
44 | uint32_t teehbr; | 49 | diff --git a/hw/core/machine.c b/hw/core/machine.c |
45 | diff --git a/target/arm/cpu.c b/target/arm/cpu.c | ||
46 | index XXXXXXX..XXXXXXX 100644 | 50 | index XXXXXXX..XXXXXXX 100644 |
47 | --- a/target/arm/cpu.c | 51 | --- a/hw/core/machine.c |
48 | +++ b/target/arm/cpu.c | 52 | +++ b/hw/core/machine.c |
49 | @@ -XXX,XX +XXX,XX @@ static void arm_cpu_set_irq(void *opaque, int irq, int level) | 53 | @@ -XXX,XX +XXX,XX @@ GlobalProperty hw_compat_4_0[] = { |
50 | [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ | 54 | { "virtio-gpu-pci", "edid", "false" }, |
51 | }; | 55 | { "virtio-device", "use-started", "false" }, |
52 | 56 | { "virtio-balloon-device", "qemu-4-0-config-size", "true" }, | |
53 | + if (level) { | 57 | + { "pl031", "migrate-tick-offset", "false" }, |
54 | + env->irq_line_state |= mask[irq]; | 58 | }; |
55 | + } else { | 59 | const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0); |
56 | + env->irq_line_state &= ~mask[irq]; | 60 | |
61 | diff --git a/hw/timer/pl031.c b/hw/timer/pl031.c | ||
62 | index XXXXXXX..XXXXXXX 100644 | ||
63 | --- a/hw/timer/pl031.c | ||
64 | +++ b/hw/timer/pl031.c | ||
65 | @@ -XXX,XX +XXX,XX @@ static int pl031_pre_save(void *opaque) | ||
66 | { | ||
67 | PL031State *s = opaque; | ||
68 | |||
69 | - /* tick_offset is base_time - rtc_clock base time. Instead, we want to | ||
70 | - * store the base time relative to the QEMU_CLOCK_VIRTUAL for backwards-compatibility. */ | ||
71 | + /* | ||
72 | + * The PL031 device model code uses the tick_offset field, which is | ||
73 | + * the offset between what the guest RTC should read and what the | ||
74 | + * QEMU rtc_clock reads: | ||
75 | + * guest_rtc = rtc_clock + tick_offset | ||
76 | + * and so | ||
77 | + * tick_offset = guest_rtc - rtc_clock | ||
78 | + * | ||
79 | + * We want to migrate this offset, which sounds straightforward. | ||
80 | + * Unfortunately older versions of QEMU migrated a conversion of this | ||
81 | + * offset into an offset from the vm_clock. (This was in turn an | ||
82 | + * attempt to be compatible with even older QEMU versions, but it | ||
83 | + * has incorrect behaviour if the rtc_clock is not the same as the | ||
84 | + * vm_clock.) So we put the actual tick_offset into a migration | ||
85 | + * subsection, and the backwards-compatible time-relative-to-vm_clock | ||
86 | + * in the main migration state. | ||
87 | + * | ||
88 | + * Calculate base time relative to QEMU_CLOCK_VIRTUAL: | ||
89 | + */ | ||
90 | int64_t delta = qemu_clock_get_ns(rtc_clock) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); | ||
91 | s->tick_offset_vmstate = s->tick_offset + delta / NANOSECONDS_PER_SECOND; | ||
92 | |||
93 | return 0; | ||
94 | } | ||
95 | |||
96 | +static int pl031_pre_load(void *opaque) | ||
97 | +{ | ||
98 | + PL031State *s = opaque; | ||
99 | + | ||
100 | + s->tick_offset_migrated = false; | ||
101 | + return 0; | ||
102 | +} | ||
103 | + | ||
104 | static int pl031_post_load(void *opaque, int version_id) | ||
105 | { | ||
106 | PL031State *s = opaque; | ||
107 | |||
108 | - int64_t delta = qemu_clock_get_ns(rtc_clock) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); | ||
109 | - s->tick_offset = s->tick_offset_vmstate - delta / NANOSECONDS_PER_SECOND; | ||
110 | + /* | ||
111 | + * If we got the tick_offset subsection, then we can just use | ||
112 | + * the value in that. Otherwise the source is an older QEMU and | ||
113 | + * has given us the offset from the vm_clock; convert it back to | ||
114 | + * an offset from the rtc_clock. This will cause time to incorrectly | ||
115 | + * go backwards compared to the host RTC, but this is unavoidable. | ||
116 | + */ | ||
117 | + | ||
118 | + if (!s->tick_offset_migrated) { | ||
119 | + int64_t delta = qemu_clock_get_ns(rtc_clock) - | ||
120 | + qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); | ||
121 | + s->tick_offset = s->tick_offset_vmstate - | ||
122 | + delta / NANOSECONDS_PER_SECOND; | ||
57 | + } | 123 | + } |
58 | + | 124 | pl031_set_alarm(s); |
59 | switch (irq) { | 125 | return 0; |
60 | case ARM_CPU_VIRQ: | 126 | } |
61 | case ARM_CPU_VFIQ: | 127 | |
62 | @@ -XXX,XX +XXX,XX @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level) | 128 | +static int pl031_tick_offset_post_load(void *opaque, int version_id) |
63 | ARMCPU *cpu = opaque; | ||
64 | CPUState *cs = CPU(cpu); | ||
65 | int kvm_irq = KVM_ARM_IRQ_TYPE_CPU << KVM_ARM_IRQ_TYPE_SHIFT; | ||
66 | + uint32_t linestate_bit; | ||
67 | |||
68 | switch (irq) { | ||
69 | case ARM_CPU_IRQ: | ||
70 | kvm_irq |= KVM_ARM_IRQ_CPU_IRQ; | ||
71 | + linestate_bit = CPU_INTERRUPT_HARD; | ||
72 | break; | ||
73 | case ARM_CPU_FIQ: | ||
74 | kvm_irq |= KVM_ARM_IRQ_CPU_FIQ; | ||
75 | + linestate_bit = CPU_INTERRUPT_FIQ; | ||
76 | break; | ||
77 | default: | ||
78 | g_assert_not_reached(); | ||
79 | } | ||
80 | + | ||
81 | + if (level) { | ||
82 | + env->irq_line_state |= linestate_bit; | ||
83 | + } else { | ||
84 | + env->irq_line_state &= ~linestate_bit; | ||
85 | + } | ||
86 | + | ||
87 | kvm_irq |= cs->cpu_index << KVM_ARM_IRQ_VCPU_SHIFT; | ||
88 | kvm_set_irq(kvm_state, kvm_irq, level ? 1 : 0); | ||
89 | #endif | ||
90 | diff --git a/target/arm/machine.c b/target/arm/machine.c | ||
91 | index XXXXXXX..XXXXXXX 100644 | ||
92 | --- a/target/arm/machine.c | ||
93 | +++ b/target/arm/machine.c | ||
94 | @@ -XXX,XX +XXX,XX @@ static const VMStateDescription vmstate_serror = { | ||
95 | } | ||
96 | }; | ||
97 | |||
98 | +static bool irq_line_state_needed(void *opaque) | ||
99 | +{ | 129 | +{ |
100 | + return true; | 130 | + PL031State *s = opaque; |
131 | + | ||
132 | + s->tick_offset_migrated = true; | ||
133 | + return 0; | ||
101 | +} | 134 | +} |
102 | + | 135 | + |
103 | +static const VMStateDescription vmstate_irq_line_state = { | 136 | +static bool pl031_tick_offset_needed(void *opaque) |
104 | + .name = "cpu/irq-line-state", | 137 | +{ |
138 | + PL031State *s = opaque; | ||
139 | + | ||
140 | + return s->migrate_tick_offset; | ||
141 | +} | ||
142 | + | ||
143 | +static const VMStateDescription vmstate_pl031_tick_offset = { | ||
144 | + .name = "pl031/tick-offset", | ||
105 | + .version_id = 1, | 145 | + .version_id = 1, |
106 | + .minimum_version_id = 1, | 146 | + .minimum_version_id = 1, |
107 | + .needed = irq_line_state_needed, | 147 | + .needed = pl031_tick_offset_needed, |
148 | + .post_load = pl031_tick_offset_post_load, | ||
108 | + .fields = (VMStateField[]) { | 149 | + .fields = (VMStateField[]) { |
109 | + VMSTATE_UINT32(env.irq_line_state, ARMCPU), | 150 | + VMSTATE_UINT32(tick_offset, PL031State), |
110 | + VMSTATE_END_OF_LIST() | 151 | + VMSTATE_END_OF_LIST() |
111 | + } | 152 | + } |
112 | +}; | 153 | +}; |
113 | + | 154 | + |
114 | static bool m_needed(void *opaque) | 155 | static const VMStateDescription vmstate_pl031 = { |
115 | { | 156 | .name = "pl031", |
116 | ARMCPU *cpu = opaque; | 157 | .version_id = 1, |
117 | @@ -XXX,XX +XXX,XX @@ static int cpu_pre_save(void *opaque) | 158 | .minimum_version_id = 1, |
118 | return 0; | 159 | .pre_save = pl031_pre_save, |
119 | } | 160 | + .pre_load = pl031_pre_load, |
120 | 161 | .post_load = pl031_post_load, | |
121 | +static int cpu_pre_load(void *opaque) | ||
122 | +{ | ||
123 | + ARMCPU *cpu = opaque; | ||
124 | + CPUARMState *env = &cpu->env; | ||
125 | + | ||
126 | + /* | ||
127 | + * Pre-initialize irq_line_state to a value that's never valid as | ||
128 | + * real data, so cpu_post_load() can tell whether we've seen the | ||
129 | + * irq-line-state subsection in the incoming migration state. | ||
130 | + */ | ||
131 | + env->irq_line_state = UINT32_MAX; | ||
132 | + | ||
133 | + return 0; | ||
134 | +} | ||
135 | + | ||
136 | static int cpu_post_load(void *opaque, int version_id) | ||
137 | { | ||
138 | ARMCPU *cpu = opaque; | ||
139 | + CPUARMState *env = &cpu->env; | ||
140 | int i, v; | ||
141 | |||
142 | + /* | ||
143 | + * Handle migration compatibility from old QEMU which didn't | ||
144 | + * send the irq-line-state subsection. A QEMU without it did not | ||
145 | + * implement the HCR_EL2.{VI,VF} bits as generating interrupts, | ||
146 | + * so for TCG the line state matches the bits set in cs->interrupt_request. | ||
147 | + * For KVM the line state is not stored in cs->interrupt_request | ||
148 | + * and so this will leave irq_line_state as 0, but this is OK because | ||
149 | + * we only need to care about it for TCG. | ||
150 | + */ | ||
151 | + if (env->irq_line_state == UINT32_MAX) { | ||
152 | + CPUState *cs = CPU(cpu); | ||
153 | + | ||
154 | + env->irq_line_state = cs->interrupt_request & | ||
155 | + (CPU_INTERRUPT_HARD | CPU_INTERRUPT_FIQ | | ||
156 | + CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VFIQ); | ||
157 | + } | ||
158 | + | ||
159 | /* Update the values list from the incoming migration data. | ||
160 | * Anything in the incoming data which we don't know about is | ||
161 | * a migration failure; anything we know about but the incoming | ||
162 | @@ -XXX,XX +XXX,XX @@ const VMStateDescription vmstate_arm_cpu = { | ||
163 | .version_id = 22, | ||
164 | .minimum_version_id = 22, | ||
165 | .pre_save = cpu_pre_save, | ||
166 | + .pre_load = cpu_pre_load, | ||
167 | .post_load = cpu_post_load, | ||
168 | .fields = (VMStateField[]) { | 162 | .fields = (VMStateField[]) { |
169 | VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16), | 163 | VMSTATE_UINT32(tick_offset_vmstate, PL031State), |
170 | @@ -XXX,XX +XXX,XX @@ const VMStateDescription vmstate_arm_cpu = { | 164 | @@ -XXX,XX +XXX,XX @@ static const VMStateDescription vmstate_pl031 = { |
171 | &vmstate_sve, | 165 | VMSTATE_UINT32(im, PL031State), |
172 | #endif | 166 | VMSTATE_UINT32(is, PL031State), |
173 | &vmstate_serror, | 167 | VMSTATE_END_OF_LIST() |
174 | + &vmstate_irq_line_state, | 168 | + }, |
175 | NULL | 169 | + .subsections = (const VMStateDescription*[]) { |
170 | + &vmstate_pl031_tick_offset, | ||
171 | + NULL | ||
176 | } | 172 | } |
177 | }; | 173 | }; |
174 | |||
175 | +static Property pl031_properties[] = { | ||
176 | + /* | ||
177 | + * True to correctly migrate the tick offset of the RTC. False to | ||
178 | + * obtain backward migration compatibility with older QEMU versions, | ||
179 | + * at the expense of the guest RTC going backwards compared with the | ||
180 | + * host RTC when the VM is saved/restored if using -rtc host. | ||
181 | + * (Even if set to 'true' older QEMU can migrate forward to newer QEMU; | ||
182 | + * 'false' also permits newer QEMU to migrate to older QEMU.) | ||
183 | + */ | ||
184 | + DEFINE_PROP_BOOL("migrate-tick-offset", | ||
185 | + PL031State, migrate_tick_offset, true), | ||
186 | + DEFINE_PROP_END_OF_LIST() | ||
187 | +}; | ||
188 | + | ||
189 | static void pl031_class_init(ObjectClass *klass, void *data) | ||
190 | { | ||
191 | DeviceClass *dc = DEVICE_CLASS(klass); | ||
192 | |||
193 | dc->vmsd = &vmstate_pl031; | ||
194 | + dc->props = pl031_properties; | ||
195 | } | ||
196 | |||
197 | static const TypeInfo pl031_info = { | ||
178 | -- | 198 | -- |
179 | 2.19.1 | 199 | 2.20.1 |
180 | 200 | ||
181 | 201 | diff view generated by jsdifflib |
1 | The Cortex-A15 and Cortex-A7 both have EL2; now we've implemented | 1 | The ARMv5 architecture didn't specify detailed per-feature ID |
---|---|---|---|
2 | it properly we can enable the feature bit. | 2 | registers. Now that we're using the MVFR0 register fields to |
3 | gate the existence of VFP instructions, we need to set up | ||
4 | the correct values in the cpu->isar structure so that we still | ||
5 | provide an FPU to the guest. | ||
3 | 6 | ||
7 | This fixes a regression in the arm926 and arm1026 CPUs, which | ||
8 | are the only ones that both have VFP and are ARMv5 or earlier. | ||
9 | This regression was introduced by the VFP refactoring, and more | ||
10 | specifically by commits 1120827fa182f0e76 and 266bd25c485597c, | ||
11 | which accidentally disabled VFP short-vector support and | ||
12 | double-precision support on these CPUs. | ||
13 | |||
14 | Fixes: 1120827fa182f0e | ||
15 | Fixes: 266bd25c485597c | ||
16 | Fixes: https://bugs.launchpad.net/qemu/+bug/1836192 | ||
17 | Reported-by: Christophe Lyon <christophe.lyon@linaro.org> | ||
4 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 18 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
5 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | 19 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> |
6 | Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> | 20 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> |
7 | Message-id: 20181109173553.22341-3-peter.maydell@linaro.org | 21 | Tested-by: Christophe Lyon <christophe.lyon@linaro.org> |
22 | Message-id: 20190711131241.22231-1-peter.maydell@linaro.org | ||
8 | --- | 23 | --- |
9 | target/arm/cpu.c | 2 ++ | 24 | target/arm/cpu.c | 12 ++++++++++++ |
10 | 1 file changed, 2 insertions(+) | 25 | 1 file changed, 12 insertions(+) |
11 | 26 | ||
12 | diff --git a/target/arm/cpu.c b/target/arm/cpu.c | 27 | diff --git a/target/arm/cpu.c b/target/arm/cpu.c |
13 | index XXXXXXX..XXXXXXX 100644 | 28 | index XXXXXXX..XXXXXXX 100644 |
14 | --- a/target/arm/cpu.c | 29 | --- a/target/arm/cpu.c |
15 | +++ b/target/arm/cpu.c | 30 | +++ b/target/arm/cpu.c |
16 | @@ -XXX,XX +XXX,XX @@ static void cortex_a7_initfn(Object *obj) | 31 | @@ -XXX,XX +XXX,XX @@ static void arm926_initfn(Object *obj) |
17 | set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER); | 32 | * set the field to indicate Jazelle support within QEMU. |
18 | set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS); | 33 | */ |
19 | set_feature(&cpu->env, ARM_FEATURE_CBAR_RO); | 34 | cpu->isar.id_isar1 = FIELD_DP32(cpu->isar.id_isar1, ID_ISAR1, JAZELLE, 1); |
20 | + set_feature(&cpu->env, ARM_FEATURE_EL2); | 35 | + /* |
21 | set_feature(&cpu->env, ARM_FEATURE_EL3); | 36 | + * Similarly, we need to set MVFR0 fields to enable double precision |
22 | cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A7; | 37 | + * and short vector support even though ARMv5 doesn't have this register. |
23 | cpu->midr = 0x410fc075; | 38 | + */ |
24 | @@ -XXX,XX +XXX,XX @@ static void cortex_a15_initfn(Object *obj) | 39 | + cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1); |
25 | set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER); | 40 | + cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1); |
26 | set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS); | 41 | } |
27 | set_feature(&cpu->env, ARM_FEATURE_CBAR_RO); | 42 | |
28 | + set_feature(&cpu->env, ARM_FEATURE_EL2); | 43 | static void arm946_initfn(Object *obj) |
29 | set_feature(&cpu->env, ARM_FEATURE_EL3); | 44 | @@ -XXX,XX +XXX,XX @@ static void arm1026_initfn(Object *obj) |
30 | cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A15; | 45 | * set the field to indicate Jazelle support within QEMU. |
31 | cpu->midr = 0x412fc0f1; | 46 | */ |
47 | cpu->isar.id_isar1 = FIELD_DP32(cpu->isar.id_isar1, ID_ISAR1, JAZELLE, 1); | ||
48 | + /* | ||
49 | + * Similarly, we need to set MVFR0 fields to enable double precision | ||
50 | + * and short vector support even though ARMv5 doesn't have this register. | ||
51 | + */ | ||
52 | + cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPSHVEC, 1); | ||
53 | + cpu->isar.mvfr0 = FIELD_DP32(cpu->isar.mvfr0, MVFR0, FPDP, 1); | ||
54 | |||
55 | { | ||
56 | /* The 1026 had an IFAR at c6,c0,0,1 rather than the ARMv6 c6,c0,0,2 */ | ||
32 | -- | 57 | -- |
33 | 2.19.1 | 58 | 2.20.1 |
34 | 59 | ||
35 | 60 | diff view generated by jsdifflib |
1 | Hyp mode is an exception to the general rule that each AArch32 | 1 | In the M-profile architecture, when we do a vector table fetch and it |
---|---|---|---|
2 | mode has its own r13, r14 and SPSR -- it has a banked r13 and | 2 | fails, we need to report a HardFault. Whether this is a Secure HF or |
3 | SPSR but shares its r14 with User and System mode. We were | 3 | a NonSecure HF depends on several things. If AIRCR.BFHFNMINS is 0 |
4 | incorrectly implementing it as banked, which meant that on | 4 | then HF is always Secure, because there is no NonSecure HardFault. |
5 | entry to Hyp mode r14 was 0 rather than the USR/SYS r14. | 5 | Otherwise, the answer depends on whether the 'underlying exception' |
6 | (MemManage, BusFault, SecureFault) targets Secure or NonSecure. (In | ||
7 | the pseudocode, this is handled in the Vector() function: the final | ||
8 | exc.isSecure is calculated by looking at the exc.isSecure from the | ||
9 | exception returned from the memory access, not the isSecure input | ||
10 | argument.) | ||
6 | 11 | ||
7 | We provide a new function r14_bank_number() which is like | 12 | We weren't doing this correctly, because we were looking at |
8 | the existing bank_number() but provides the index into | 13 | the target security domain of the exception we were trying to |
9 | env->banked_r14[]; bank_number() provides the index to use | 14 | load the vector table entry for. This produces errors of two kinds: |
10 | for env->banked_r13[] and env->banked_cpsr[]. | 15 | * a load from the NS vector table which hits the "NS access |
16 | to S memory" SecureFault should end up as a Secure HardFault, | ||
17 | but we were raising an NS HardFault | ||
18 | * a load from the S vector table which causes a BusFault | ||
19 | should raise an NS HardFault if BFHFNMINS == 1 (because | ||
20 | in that case all BusFaults are NonSecure), but we were raising | ||
21 | a Secure HardFault | ||
11 | 22 | ||
12 | All the points in the code that were using bank_number() | 23 | Correct the logic. |
13 | to index into env->banked_r14[] are updated for consintency: | 24 | |
14 | * switch_mode() -- this is the only place where we fix | 25 | We also fix a comment error where we claimed that we might |
15 | an actual bug | 26 | be escalating MemManage to HardFault, and forgot about SecureFault. |
16 | * aarch64_sync_32_to_64() and aarch64_sync_64_to_32(): | 27 | (Vector loads can never hit MPU access faults, because they're |
17 | no behavioural change as we already special-cased Hyp R14 | 28 | always aligned and always use the default address map.) |
18 | * kvm32.c: no behavioural change since the guest can't ever | ||
19 | be in Hyp mode, but conceptually the right thing to do | ||
20 | * msr_banked()/mrs_banked(): we can never get to the case | ||
21 | that accesses banked_r14[] with tgtmode == ARM_CPU_MODE_HYP, | ||
22 | so no behavioural change | ||
23 | 29 | ||
24 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 30 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
25 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | 31 | Message-id: 20190705094823.28905-1-peter.maydell@linaro.org |
26 | Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> | ||
27 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
28 | Message-id: 20181109173553.22341-2-peter.maydell@linaro.org | ||
29 | --- | 32 | --- |
30 | target/arm/internals.h | 16 ++++++++++++++++ | 33 | target/arm/m_helper.c | 21 +++++++++++++++++---- |
31 | target/arm/helper.c | 29 +++++++++++++++-------------- | 34 | 1 file changed, 17 insertions(+), 4 deletions(-) |
32 | target/arm/kvm32.c | 4 ++-- | ||
33 | target/arm/op_helper.c | 4 ++-- | ||
34 | 4 files changed, 35 insertions(+), 18 deletions(-) | ||
35 | 35 | ||
36 | diff --git a/target/arm/internals.h b/target/arm/internals.h | 36 | diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c |
37 | index XXXXXXX..XXXXXXX 100644 | 37 | index XXXXXXX..XXXXXXX 100644 |
38 | --- a/target/arm/internals.h | 38 | --- a/target/arm/m_helper.c |
39 | +++ b/target/arm/internals.h | 39 | +++ b/target/arm/m_helper.c |
40 | @@ -XXX,XX +XXX,XX @@ static inline int bank_number(int mode) | 40 | @@ -XXX,XX +XXX,XX @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure, |
41 | g_assert_not_reached(); | 41 | if (sattrs.ns) { |
42 | } | 42 | attrs.secure = false; |
43 | 43 | } else if (!targets_secure) { | |
44 | +/** | 44 | - /* NS access to S memory */ |
45 | + * r14_bank_number: Map CPU mode onto register bank for r14 | 45 | + /* |
46 | + * | 46 | + * NS access to S memory: the underlying exception which we escalate |
47 | + * Given an AArch32 CPU mode, return the index into the saved register | 47 | + * to HardFault is SecureFault, which always targets Secure. |
48 | + * banks to use for the R14 (LR) in that mode. This is the same as | 48 | + */ |
49 | + * bank_number(), except for the special case of Hyp mode, where | 49 | + exc_secure = true; |
50 | + * R14 is shared with USR and SYS, unlike its R13 and SPSR. | 50 | goto load_fail; |
51 | + * This should be used as the index into env->banked_r14[], and | ||
52 | + * bank_number() used for the index into env->banked_r13[] and | ||
53 | + * env->banked_spsr[]. | ||
54 | + */ | ||
55 | +static inline int r14_bank_number(int mode) | ||
56 | +{ | ||
57 | + return (mode == ARM_CPU_MODE_HYP) ? BANK_USRSYS : bank_number(mode); | ||
58 | +} | ||
59 | + | ||
60 | void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu); | ||
61 | void arm_translate_init(void); | ||
62 | |||
63 | diff --git a/target/arm/helper.c b/target/arm/helper.c | ||
64 | index XXXXXXX..XXXXXXX 100644 | ||
65 | --- a/target/arm/helper.c | ||
66 | +++ b/target/arm/helper.c | ||
67 | @@ -XXX,XX +XXX,XX @@ static void switch_mode(CPUARMState *env, int mode) | ||
68 | |||
69 | i = bank_number(old_mode); | ||
70 | env->banked_r13[i] = env->regs[13]; | ||
71 | - env->banked_r14[i] = env->regs[14]; | ||
72 | env->banked_spsr[i] = env->spsr; | ||
73 | |||
74 | i = bank_number(mode); | ||
75 | env->regs[13] = env->banked_r13[i]; | ||
76 | - env->regs[14] = env->banked_r14[i]; | ||
77 | env->spsr = env->banked_spsr[i]; | ||
78 | + | ||
79 | + env->banked_r14[r14_bank_number(old_mode)] = env->regs[14]; | ||
80 | + env->regs[14] = env->banked_r14[r14_bank_number(mode)]; | ||
81 | } | ||
82 | |||
83 | /* Physical Interrupt Target EL Lookup Table | ||
84 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_32_to_64(CPUARMState *env) | ||
85 | if (mode == ARM_CPU_MODE_HYP) { | ||
86 | env->xregs[14] = env->regs[14]; | ||
87 | } else { | ||
88 | - env->xregs[14] = env->banked_r14[bank_number(ARM_CPU_MODE_USR)]; | ||
89 | + env->xregs[14] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_USR)]; | ||
90 | } | 51 | } |
91 | } | 52 | } |
92 | 53 | @@ -XXX,XX +XXX,XX @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure, | |
93 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_32_to_64(CPUARMState *env) | 54 | vector_entry = address_space_ldl(arm_addressspace(cs, attrs), addr, |
94 | env->xregs[16] = env->regs[14]; | 55 | attrs, &result); |
95 | env->xregs[17] = env->regs[13]; | 56 | if (result != MEMTX_OK) { |
96 | } else { | 57 | + /* |
97 | - env->xregs[16] = env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)]; | 58 | + * Underlying exception is BusFault: its target security state |
98 | + env->xregs[16] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_IRQ)]; | 59 | + * depends on BFHFNMINS. |
99 | env->xregs[17] = env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)]; | 60 | + */ |
61 | + exc_secure = !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK); | ||
62 | goto load_fail; | ||
100 | } | 63 | } |
101 | 64 | *pvec = vector_entry; | |
102 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_32_to_64(CPUARMState *env) | 65 | @@ -XXX,XX +XXX,XX @@ load_fail: |
103 | env->xregs[18] = env->regs[14]; | 66 | /* |
104 | env->xregs[19] = env->regs[13]; | 67 | * All vector table fetch fails are reported as HardFault, with |
105 | } else { | 68 | * HFSR.VECTTBL and .FORCED set. (FORCED is set because |
106 | - env->xregs[18] = env->banked_r14[bank_number(ARM_CPU_MODE_SVC)]; | 69 | - * technically the underlying exception is a MemManage or BusFault |
107 | + env->xregs[18] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_SVC)]; | 70 | + * technically the underlying exception is a SecureFault or BusFault |
108 | env->xregs[19] = env->banked_r13[bank_number(ARM_CPU_MODE_SVC)]; | 71 | * that is escalated to HardFault.) This is a terminal exception, |
109 | } | 72 | * so we will either take the HardFault immediately or else enter |
110 | 73 | * lockup (the latter case is handled in armv7m_nvic_set_pending_derived()). | |
111 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_32_to_64(CPUARMState *env) | 74 | + * The HardFault is Secure if BFHFNMINS is 0 (meaning that all HFs are |
112 | env->xregs[20] = env->regs[14]; | 75 | + * secure); otherwise it targets the same security state as the |
113 | env->xregs[21] = env->regs[13]; | 76 | + * underlying exception. |
114 | } else { | 77 | */ |
115 | - env->xregs[20] = env->banked_r14[bank_number(ARM_CPU_MODE_ABT)]; | 78 | - exc_secure = targets_secure || |
116 | + env->xregs[20] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_ABT)]; | 79 | - !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK); |
117 | env->xregs[21] = env->banked_r13[bank_number(ARM_CPU_MODE_ABT)]; | 80 | + if (!(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK)) { |
118 | } | 81 | + exc_secure = true; |
119 | 82 | + } | |
120 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_32_to_64(CPUARMState *env) | 83 | env->v7m.hfsr |= R_V7M_HFSR_VECTTBL_MASK | R_V7M_HFSR_FORCED_MASK; |
121 | env->xregs[22] = env->regs[14]; | 84 | armv7m_nvic_set_pending_derived(env->nvic, ARMV7M_EXCP_HARD, exc_secure); |
122 | env->xregs[23] = env->regs[13]; | 85 | return false; |
123 | } else { | ||
124 | - env->xregs[22] = env->banked_r14[bank_number(ARM_CPU_MODE_UND)]; | ||
125 | + env->xregs[22] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_UND)]; | ||
126 | env->xregs[23] = env->banked_r13[bank_number(ARM_CPU_MODE_UND)]; | ||
127 | } | ||
128 | |||
129 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_32_to_64(CPUARMState *env) | ||
130 | env->xregs[i] = env->fiq_regs[i - 24]; | ||
131 | } | ||
132 | env->xregs[29] = env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)]; | ||
133 | - env->xregs[30] = env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)]; | ||
134 | + env->xregs[30] = env->banked_r14[r14_bank_number(ARM_CPU_MODE_FIQ)]; | ||
135 | } | ||
136 | |||
137 | env->pc = env->regs[15]; | ||
138 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_64_to_32(CPUARMState *env) | ||
139 | if (mode == ARM_CPU_MODE_HYP) { | ||
140 | env->regs[14] = env->xregs[14]; | ||
141 | } else { | ||
142 | - env->banked_r14[bank_number(ARM_CPU_MODE_USR)] = env->xregs[14]; | ||
143 | + env->banked_r14[r14_bank_number(ARM_CPU_MODE_USR)] = env->xregs[14]; | ||
144 | } | ||
145 | } | ||
146 | |||
147 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_64_to_32(CPUARMState *env) | ||
148 | env->regs[14] = env->xregs[16]; | ||
149 | env->regs[13] = env->xregs[17]; | ||
150 | } else { | ||
151 | - env->banked_r14[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16]; | ||
152 | + env->banked_r14[r14_bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[16]; | ||
153 | env->banked_r13[bank_number(ARM_CPU_MODE_IRQ)] = env->xregs[17]; | ||
154 | } | ||
155 | |||
156 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_64_to_32(CPUARMState *env) | ||
157 | env->regs[14] = env->xregs[18]; | ||
158 | env->regs[13] = env->xregs[19]; | ||
159 | } else { | ||
160 | - env->banked_r14[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18]; | ||
161 | + env->banked_r14[r14_bank_number(ARM_CPU_MODE_SVC)] = env->xregs[18]; | ||
162 | env->banked_r13[bank_number(ARM_CPU_MODE_SVC)] = env->xregs[19]; | ||
163 | } | ||
164 | |||
165 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_64_to_32(CPUARMState *env) | ||
166 | env->regs[14] = env->xregs[20]; | ||
167 | env->regs[13] = env->xregs[21]; | ||
168 | } else { | ||
169 | - env->banked_r14[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20]; | ||
170 | + env->banked_r14[r14_bank_number(ARM_CPU_MODE_ABT)] = env->xregs[20]; | ||
171 | env->banked_r13[bank_number(ARM_CPU_MODE_ABT)] = env->xregs[21]; | ||
172 | } | ||
173 | |||
174 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_64_to_32(CPUARMState *env) | ||
175 | env->regs[14] = env->xregs[22]; | ||
176 | env->regs[13] = env->xregs[23]; | ||
177 | } else { | ||
178 | - env->banked_r14[bank_number(ARM_CPU_MODE_UND)] = env->xregs[22]; | ||
179 | + env->banked_r14[r14_bank_number(ARM_CPU_MODE_UND)] = env->xregs[22]; | ||
180 | env->banked_r13[bank_number(ARM_CPU_MODE_UND)] = env->xregs[23]; | ||
181 | } | ||
182 | |||
183 | @@ -XXX,XX +XXX,XX @@ void aarch64_sync_64_to_32(CPUARMState *env) | ||
184 | env->fiq_regs[i - 24] = env->xregs[i]; | ||
185 | } | ||
186 | env->banked_r13[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[29]; | ||
187 | - env->banked_r14[bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30]; | ||
188 | + env->banked_r14[r14_bank_number(ARM_CPU_MODE_FIQ)] = env->xregs[30]; | ||
189 | } | ||
190 | |||
191 | env->regs[15] = env->pc; | ||
192 | diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c | ||
193 | index XXXXXXX..XXXXXXX 100644 | ||
194 | --- a/target/arm/kvm32.c | ||
195 | +++ b/target/arm/kvm32.c | ||
196 | @@ -XXX,XX +XXX,XX @@ int kvm_arch_put_registers(CPUState *cs, int level) | ||
197 | memcpy(env->usr_regs, env->regs + 8, 5 * sizeof(uint32_t)); | ||
198 | } | ||
199 | env->banked_r13[bn] = env->regs[13]; | ||
200 | - env->banked_r14[bn] = env->regs[14]; | ||
201 | env->banked_spsr[bn] = env->spsr; | ||
202 | + env->banked_r14[r14_bank_number(mode)] = env->regs[14]; | ||
203 | |||
204 | /* Now we can safely copy stuff down to the kernel */ | ||
205 | for (i = 0; i < ARRAY_SIZE(regs); i++) { | ||
206 | @@ -XXX,XX +XXX,XX @@ int kvm_arch_get_registers(CPUState *cs) | ||
207 | memcpy(env->regs + 8, env->usr_regs, 5 * sizeof(uint32_t)); | ||
208 | } | ||
209 | env->regs[13] = env->banked_r13[bn]; | ||
210 | - env->regs[14] = env->banked_r14[bn]; | ||
211 | env->spsr = env->banked_spsr[bn]; | ||
212 | + env->regs[14] = env->banked_r14[r14_bank_number(mode)]; | ||
213 | |||
214 | /* VFP registers */ | ||
215 | r.id = KVM_REG_ARM | KVM_REG_SIZE_U64 | KVM_REG_ARM_VFP; | ||
216 | diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c | ||
217 | index XXXXXXX..XXXXXXX 100644 | ||
218 | --- a/target/arm/op_helper.c | ||
219 | +++ b/target/arm/op_helper.c | ||
220 | @@ -XXX,XX +XXX,XX @@ void HELPER(msr_banked)(CPUARMState *env, uint32_t value, uint32_t tgtmode, | ||
221 | env->banked_r13[bank_number(tgtmode)] = value; | ||
222 | break; | ||
223 | case 14: | ||
224 | - env->banked_r14[bank_number(tgtmode)] = value; | ||
225 | + env->banked_r14[r14_bank_number(tgtmode)] = value; | ||
226 | break; | ||
227 | case 8 ... 12: | ||
228 | switch (tgtmode) { | ||
229 | @@ -XXX,XX +XXX,XX @@ uint32_t HELPER(mrs_banked)(CPUARMState *env, uint32_t tgtmode, uint32_t regno) | ||
230 | case 13: | ||
231 | return env->banked_r13[bank_number(tgtmode)]; | ||
232 | case 14: | ||
233 | - return env->banked_r14[bank_number(tgtmode)]; | ||
234 | + return env->banked_r14[r14_bank_number(tgtmode)]; | ||
235 | case 8 ... 12: | ||
236 | switch (tgtmode) { | ||
237 | case ARM_CPU_MODE_USR: | ||
238 | -- | 86 | -- |
239 | 2.19.1 | 87 | 2.20.1 |
240 | 88 | ||
241 | 89 | diff view generated by jsdifflib |