1 | Handful of bugfixes for rc2. None of these are particularly critical | 1 | Hi; this is a collection of mostly GIC related patches for rc3. |
---|---|---|---|
2 | or exciting. | 2 | The "Update cached state after LPI state changes" fix is important |
3 | and fixes what would otherwise be a regression since we enable the | ||
4 | ITS by default in the virt board now. The others are not regressions | ||
5 | but I think are OK for rc3 as they're fairly self contained (and two | ||
6 | of them are fixes to new-in-6.2 functionality). | ||
3 | 7 | ||
8 | thanks | ||
4 | -- PMM | 9 | -- PMM |
5 | 10 | ||
6 | The following changes since commit 45a150aa2b3492acf6691c7bdbeb25a8545d8345: | 11 | The following changes since commit dd4b0de45965538f19bb40c7ddaaba384a8c613a: |
7 | 12 | ||
8 | Merge remote-tracking branch 'remotes/ericb/tags/pull-bitmaps-2020-08-03' into staging (2020-08-03 15:13:49 +0100) | 13 | Fix version for v6.2.0-rc2 release (2021-11-26 11:58:54 +0100) |
9 | 14 | ||
10 | are available in the Git repository at: | 15 | are available in the Git repository at: |
11 | 16 | ||
12 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20200803 | 17 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20211129 |
13 | 18 | ||
14 | for you to fetch changes up to 13557fd392890cbd985bceba7f717e01efd674b8: | 19 | for you to fetch changes up to 90feffad2aafe856ed2af75313b2c1669ba671e9: |
15 | 20 | ||
16 | hw/timer/imx_epit: Avoid assertion when CR.SWR is written (2020-08-03 17:56:11 +0100) | 21 | hw/intc/arm_gicv3: fix handling of LPIs in list registers (2021-11-29 10:10:21 +0000) |
17 | 22 | ||
18 | ---------------------------------------------------------------- | 23 | ---------------------------------------------------------------- |
19 | target-arm queue: | 24 | target-arm queue: |
20 | * hw/timer/imx_epit: Avoid assertion when CR.SWR is written | 25 | * virt: Diagnose attempts to enable MTE or virt when using HVF accelerator |
21 | * netduino2, netduinoplus2, microbit: set system_clock_scale so that | 26 | * GICv3 ITS: Allow clearing of ITS CTLR Enabled bit |
22 | SysTick running on the CPU clock works | 27 | * GICv3: Update cached state after LPI state changes |
23 | * target/arm: Avoid maybe-uninitialized warning with gcc 4.9 | 28 | * GICv3: Fix handling of LPIs in list registers |
24 | * target/arm: Fix AddPAC error indication | ||
25 | * Make AIRCR.SYSRESETREQ actually reset the system for the | ||
26 | microbit, mps2-*, musca-*, netduino* boards | ||
27 | 29 | ||
28 | ---------------------------------------------------------------- | 30 | ---------------------------------------------------------------- |
29 | Kaige Li (1): | 31 | Alexander Graf (1): |
30 | target/arm: Avoid maybe-uninitialized warning with gcc 4.9 | 32 | hw/arm/virt: Extend nested and mte checks to hvf |
31 | 33 | ||
32 | Peter Maydell (6): | 34 | Peter Maydell (3): |
33 | hw/arm/netduino2, netduinoplus2: Set system_clock_scale | 35 | hw/intc/arm_gicv3: Update cached state after LPI state changes |
34 | include/hw/irq.h: New function qemu_irq_is_connected() | 36 | hw/intc/arm_gicv3: Add new gicv3_intid_is_special() function |
35 | hw/intc/armv7m_nvic: Provide default "reset the system" behaviour for SYSRESETREQ | 37 | hw/intc/arm_gicv3: fix handling of LPIs in list registers |
36 | msf2-soc, stellaris: Don't wire up SYSRESETREQ | ||
37 | hw/arm/nrf51_soc: Set system_clock_scale | ||
38 | hw/timer/imx_epit: Avoid assertion when CR.SWR is written | ||
39 | 38 | ||
40 | Richard Henderson (1): | 39 | Shashi Mallela (1): |
41 | target/arm: Fix AddPAC error indication | 40 | hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit |
42 | 41 | ||
43 | include/hw/arm/armv7m.h | 4 +++- | 42 | hw/intc/gicv3_internal.h | 30 ++++++++++++++++++++++++++++++ |
44 | include/hw/irq.h | 18 ++++++++++++++++++ | 43 | hw/arm/virt.c | 15 +++++++++------ |
45 | hw/arm/msf2-soc.c | 11 ----------- | 44 | hw/intc/arm_gicv3.c | 6 ++++-- |
46 | hw/arm/netduino2.c | 10 ++++++++++ | 45 | hw/intc/arm_gicv3_cpuif.c | 9 ++++----- |
47 | hw/arm/netduinoplus2.c | 10 ++++++++++ | 46 | hw/intc/arm_gicv3_its.c | 7 ++++--- |
48 | hw/arm/nrf51_soc.c | 5 +++++ | 47 | hw/intc/arm_gicv3_redist.c | 14 ++++++++++---- |
49 | hw/arm/stellaris.c | 12 ------------ | 48 | 6 files changed, 61 insertions(+), 20 deletions(-) |
50 | hw/intc/armv7m_nvic.c | 17 ++++++++++++++++- | ||
51 | hw/timer/imx_epit.c | 13 ++++++++++--- | ||
52 | target/arm/pauth_helper.c | 6 +++++- | ||
53 | target/arm/translate-a64.c | 2 +- | ||
54 | tests/tcg/aarch64/pauth-5.c | 33 +++++++++++++++++++++++++++++++++ | ||
55 | tests/tcg/aarch64/Makefile.target | 2 +- | ||
56 | 13 files changed, 112 insertions(+), 31 deletions(-) | ||
57 | create mode 100644 tests/tcg/aarch64/pauth-5.c | ||
58 | 49 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | The netduino2 and netduinoplus2 boards forgot to set the system_clock_scale | ||
2 | global, which meant that if guest code used the systick timer in "use | ||
3 | the processor clock" mode it would hang because time never advances. | ||
4 | 1 | ||
5 | Set the global to match the documented CPU clock speed of these boards. | ||
6 | Judging by the data sheet this is slightly simplistic because the | ||
7 | SoC allows configuration of the SYSCLK source and frequency via the | ||
8 | RCC (reset and clock control) module, but we don't model that. | ||
9 | |||
10 | Fixes: https://bugs.launchpad.net/qemu/+bug/1876187 | ||
11 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
12 | Reviewed-by: Alistair Francis <alistair.francis@wdc.com> | ||
13 | Message-id: 20200727162617.26227-1-peter.maydell@linaro.org | ||
14 | --- | ||
15 | hw/arm/netduino2.c | 10 ++++++++++ | ||
16 | hw/arm/netduinoplus2.c | 10 ++++++++++ | ||
17 | 2 files changed, 20 insertions(+) | ||
18 | |||
19 | diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c | ||
20 | index XXXXXXX..XXXXXXX 100644 | ||
21 | --- a/hw/arm/netduino2.c | ||
22 | +++ b/hw/arm/netduino2.c | ||
23 | @@ -XXX,XX +XXX,XX @@ | ||
24 | #include "hw/arm/stm32f205_soc.h" | ||
25 | #include "hw/arm/boot.h" | ||
26 | |||
27 | +/* Main SYSCLK frequency in Hz (120MHz) */ | ||
28 | +#define SYSCLK_FRQ 120000000ULL | ||
29 | + | ||
30 | static void netduino2_init(MachineState *machine) | ||
31 | { | ||
32 | DeviceState *dev; | ||
33 | |||
34 | + /* | ||
35 | + * TODO: ideally we would model the SoC RCC and let it handle | ||
36 | + * system_clock_scale, including its ability to define different | ||
37 | + * possible SYSCLK sources. | ||
38 | + */ | ||
39 | + system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ; | ||
40 | + | ||
41 | dev = qdev_new(TYPE_STM32F205_SOC); | ||
42 | qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3")); | ||
43 | sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); | ||
44 | diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c | ||
45 | index XXXXXXX..XXXXXXX 100644 | ||
46 | --- a/hw/arm/netduinoplus2.c | ||
47 | +++ b/hw/arm/netduinoplus2.c | ||
48 | @@ -XXX,XX +XXX,XX @@ | ||
49 | #include "hw/arm/stm32f405_soc.h" | ||
50 | #include "hw/arm/boot.h" | ||
51 | |||
52 | +/* Main SYSCLK frequency in Hz (168MHz) */ | ||
53 | +#define SYSCLK_FRQ 168000000ULL | ||
54 | + | ||
55 | static void netduinoplus2_init(MachineState *machine) | ||
56 | { | ||
57 | DeviceState *dev; | ||
58 | |||
59 | + /* | ||
60 | + * TODO: ideally we would model the SoC RCC and let it handle | ||
61 | + * system_clock_scale, including its ability to define different | ||
62 | + * possible SYSCLK sources. | ||
63 | + */ | ||
64 | + system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ; | ||
65 | + | ||
66 | dev = qdev_new(TYPE_STM32F405_SOC); | ||
67 | qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4")); | ||
68 | sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); | ||
69 | -- | ||
70 | 2.20.1 | ||
71 | |||
72 | diff view generated by jsdifflib |
1 | From: Richard Henderson <richard.henderson@linaro.org> | 1 | From: Alexander Graf <agraf@csgraf.de> |
---|---|---|---|
2 | 2 | ||
3 | The definition of top_bit used in this function is one higher | 3 | The virt machine has properties to enable MTE and Nested Virtualization |
4 | than that used in the Arm ARM psuedo-code, which put the error | 4 | support. However, its check to ensure the backing accel implementation |
5 | indication at top_bit - 1 at the wrong place, which meant that | 5 | supports it today only looks for KVM and bails out if it finds it. |
6 | it wasn't visible to Auth. | ||
7 | 6 | ||
8 | Fixing the definition of top_bit requires more changes, because | 7 | Extend the checks to HVF as well as it does not support either today. |
9 | its most common use is for the count of bits in top_bit:bot_bit, | 8 | This will cause QEMU to print a useful error message rather than |
10 | which would then need to be computed as top_bit - bot_bit + 1. | 9 | silently ignoring the attempt by the user to enable either MTE or |
10 | the Virtualization extensions. | ||
11 | 11 | ||
12 | For now, prefer the minimal fix to the error indication alone. | 12 | Reported-by: saar amar <saaramar5@gmail.com> |
13 | 13 | Signed-off-by: Alexander Graf <agraf@csgraf.de> | |
14 | Fixes: 63ff0ca94cb | 14 | Message-id: 20211123122859.22452-1-agraf@csgraf.de |
15 | Reported-by: Derrick McKee <derrick.mckee@gmail.com> | ||
16 | Signed-off-by: Richard Henderson <richard.henderson@linaro.org> | ||
17 | Message-id: 20200728195706.11087-1-richard.henderson@linaro.org | ||
18 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | 15 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> |
19 | [PMM: added comment about the divergence from the pseudocode] | ||
20 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 16 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
21 | --- | 17 | --- |
22 | target/arm/pauth_helper.c | 6 +++++- | 18 | hw/arm/virt.c | 15 +++++++++------ |
23 | tests/tcg/aarch64/pauth-5.c | 33 +++++++++++++++++++++++++++++++ | 19 | 1 file changed, 9 insertions(+), 6 deletions(-) |
24 | tests/tcg/aarch64/Makefile.target | 2 +- | ||
25 | 3 files changed, 39 insertions(+), 2 deletions(-) | ||
26 | create mode 100644 tests/tcg/aarch64/pauth-5.c | ||
27 | 20 | ||
28 | diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c | 21 | diff --git a/hw/arm/virt.c b/hw/arm/virt.c |
29 | index XXXXXXX..XXXXXXX 100644 | 22 | index XXXXXXX..XXXXXXX 100644 |
30 | --- a/target/arm/pauth_helper.c | 23 | --- a/hw/arm/virt.c |
31 | +++ b/target/arm/pauth_helper.c | 24 | +++ b/hw/arm/virt.c |
32 | @@ -XXX,XX +XXX,XX @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier, | 25 | @@ -XXX,XX +XXX,XX @@ |
33 | */ | 26 | #include "sysemu/runstate.h" |
34 | test = sextract64(ptr, bot_bit, top_bit - bot_bit); | 27 | #include "sysemu/tpm.h" |
35 | if (test != 0 && test != -1) { | 28 | #include "sysemu/kvm.h" |
36 | - pac ^= MAKE_64BIT_MASK(top_bit - 1, 1); | 29 | +#include "sysemu/hvf.h" |
37 | + /* | 30 | #include "hw/loader.h" |
38 | + * Note that our top_bit is one greater than the pseudocode's | 31 | #include "qapi/error.h" |
39 | + * version, hence "- 2" here. | 32 | #include "qemu/bitops.h" |
40 | + */ | 33 | @@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine) |
41 | + pac ^= MAKE_64BIT_MASK(top_bit - 2, 1); | 34 | exit(1); |
42 | } | 35 | } |
43 | 36 | ||
44 | /* | 37 | - if (vms->virt && kvm_enabled()) { |
45 | diff --git a/tests/tcg/aarch64/pauth-5.c b/tests/tcg/aarch64/pauth-5.c | 38 | - error_report("mach-virt: KVM does not support providing " |
46 | new file mode 100644 | 39 | - "Virtualization extensions to the guest CPU"); |
47 | index XXXXXXX..XXXXXXX | 40 | + if (vms->virt && (kvm_enabled() || hvf_enabled())) { |
48 | --- /dev/null | 41 | + error_report("mach-virt: %s does not support providing " |
49 | +++ b/tests/tcg/aarch64/pauth-5.c | 42 | + "Virtualization extensions to the guest CPU", |
50 | @@ -XXX,XX +XXX,XX @@ | 43 | + kvm_enabled() ? "KVM" : "HVF"); |
51 | +#include <assert.h> | 44 | exit(1); |
52 | + | 45 | } |
53 | +static int x; | 46 | |
54 | + | 47 | - if (vms->mte && kvm_enabled()) { |
55 | +int main() | 48 | - error_report("mach-virt: KVM does not support providing " |
56 | +{ | 49 | - "MTE to the guest CPU"); |
57 | + int *p0 = &x, *p1, *p2, *p3; | 50 | + if (vms->mte && (kvm_enabled() || hvf_enabled())) { |
58 | + unsigned long salt = 0; | 51 | + error_report("mach-virt: %s does not support providing " |
59 | + | 52 | + "MTE to the guest CPU", |
60 | + /* | 53 | + kvm_enabled() ? "KVM" : "HVF"); |
61 | + * With TBI enabled and a 48-bit VA, there are 7 bits of auth, and so | 54 | exit(1); |
62 | + * a 1/128 chance of auth = pac(ptr,key,salt) producing zero. | 55 | } |
63 | + * Find a salt that creates auth != 0. | 56 | |
64 | + */ | ||
65 | + do { | ||
66 | + salt++; | ||
67 | + asm("pacda %0, %1" : "=r"(p1) : "r"(salt), "0"(p0)); | ||
68 | + } while (p0 == p1); | ||
69 | + | ||
70 | + /* | ||
71 | + * This pac must fail, because the input pointer bears an encryption, | ||
72 | + * and so is not properly extended within bits [55:47]. This will | ||
73 | + * toggle bit 54 in the output... | ||
74 | + */ | ||
75 | + asm("pacda %0, %1" : "=r"(p2) : "r"(salt), "0"(p1)); | ||
76 | + | ||
77 | + /* ... so that the aut must fail, setting bit 53 in the output ... */ | ||
78 | + asm("autda %0, %1" : "=r"(p3) : "r"(salt), "0"(p2)); | ||
79 | + | ||
80 | + /* ... which means this equality must not hold. */ | ||
81 | + assert(p3 != p0); | ||
82 | + return 0; | ||
83 | +} | ||
84 | diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target | ||
85 | index XXXXXXX..XXXXXXX 100644 | ||
86 | --- a/tests/tcg/aarch64/Makefile.target | ||
87 | +++ b/tests/tcg/aarch64/Makefile.target | ||
88 | @@ -XXX,XX +XXX,XX @@ run-fcvt: fcvt | ||
89 | |||
90 | # Pauth Tests | ||
91 | ifneq ($(DOCKER_IMAGE)$(CROSS_CC_HAS_ARMV8_3),) | ||
92 | -AARCH64_TESTS += pauth-1 pauth-2 pauth-4 | ||
93 | +AARCH64_TESTS += pauth-1 pauth-2 pauth-4 pauth-5 | ||
94 | pauth-%: CFLAGS += -march=armv8.3-a | ||
95 | run-pauth-%: QEMU_OPTS += -cpu max | ||
96 | run-plugin-pauth-%: QEMU_OPTS += -cpu max | ||
97 | -- | 57 | -- |
98 | 2.20.1 | 58 | 2.25.1 |
99 | 59 | ||
100 | 60 | diff view generated by jsdifflib |
1 | From: Kaige Li <likaige@loongson.cn> | 1 | From: Shashi Mallela <shashi.mallela@linaro.org> |
---|---|---|---|
2 | 2 | ||
3 | GCC version 4.9.4 isn't clever enough to figure out that all | 3 | When Enabled bit is cleared in GITS_CTLR,ITS feature continues |
4 | execution paths in disas_ldst() that use 'fn' will have initialized | 4 | to be enabled.This patch fixes the issue. |
5 | it first, and so it warns: | ||
6 | 5 | ||
7 | /home/LiKaige/qemu/target/arm/translate-a64.c: In function ‘disas_ldst’: | 6 | Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> |
8 | /home/LiKaige/qemu/target/arm/translate-a64.c:3392:5: error: ‘fn’ may be used uninitialized in this function [-Werror=maybe-uninitialized] | 7 | Tested-by: Alex Bennée <alex.bennee@linaro.org> |
9 | fn(cpu_reg(s, rt), clean_addr, tcg_rs, get_mem_index(s), | ||
10 | ^ | ||
11 | /home/LiKaige/qemu/target/arm/translate-a64.c:3318:22: note: ‘fn’ was declared here | ||
12 | AtomicThreeOpFn *fn; | ||
13 | ^ | ||
14 | |||
15 | Make it happy by initializing the variable to NULL. | ||
16 | |||
17 | Signed-off-by: Kaige Li <likaige@loongson.cn> | ||
18 | Message-id: 1596110248-7366-2-git-send-email-likaige@loongson.cn | ||
19 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | 8 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> |
20 | [PMM: Clean up commit message and note which gcc version this was] | 9 | Message-id: 20211124182246.67691-1-shashi.mallela@linaro.org |
21 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 10 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
22 | --- | 11 | --- |
23 | target/arm/translate-a64.c | 2 +- | 12 | hw/intc/arm_gicv3_its.c | 7 ++++--- |
24 | 1 file changed, 1 insertion(+), 1 deletion(-) | 13 | 1 file changed, 4 insertions(+), 3 deletions(-) |
25 | 14 | ||
26 | diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c | 15 | diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c |
27 | index XXXXXXX..XXXXXXX 100644 | 16 | index XXXXXXX..XXXXXXX 100644 |
28 | --- a/target/arm/translate-a64.c | 17 | --- a/hw/intc/arm_gicv3_its.c |
29 | +++ b/target/arm/translate-a64.c | 18 | +++ b/hw/intc/arm_gicv3_its.c |
30 | @@ -XXX,XX +XXX,XX @@ static void disas_ldst_atomic(DisasContext *s, uint32_t insn, | 19 | @@ -XXX,XX +XXX,XX @@ static bool its_writel(GICv3ITSState *s, hwaddr offset, |
31 | bool r = extract32(insn, 22, 1); | 20 | |
32 | bool a = extract32(insn, 23, 1); | 21 | switch (offset) { |
33 | TCGv_i64 tcg_rs, clean_addr; | 22 | case GITS_CTLR: |
34 | - AtomicThreeOpFn *fn; | 23 | - s->ctlr |= (value & ~(s->ctlr)); |
35 | + AtomicThreeOpFn *fn = NULL; | 24 | - |
36 | 25 | - if (s->ctlr & ITS_CTLR_ENABLED) { | |
37 | if (is_vector || !dc_isar_feature(aa64_atomics, s)) { | 26 | + if (value & R_GITS_CTLR_ENABLED_MASK) { |
38 | unallocated_encoding(s); | 27 | + s->ctlr |= ITS_CTLR_ENABLED; |
28 | extract_table_params(s); | ||
29 | extract_cmdq_params(s); | ||
30 | s->creadr = 0; | ||
31 | process_cmdq(s); | ||
32 | + } else { | ||
33 | + s->ctlr &= ~ITS_CTLR_ENABLED; | ||
34 | } | ||
35 | break; | ||
36 | case GITS_CBASER: | ||
39 | -- | 37 | -- |
40 | 2.20.1 | 38 | 2.25.1 |
41 | 39 | ||
42 | 40 | diff view generated by jsdifflib |
1 | The MSF2 SoC model and the Stellaris board code both wire | 1 | The logic of gicv3_redist_update() is as follows: |
---|---|---|---|
2 | SYSRESETREQ up to a function that just invokes | 2 | * it must be called in any code path that changes the state of |
3 | qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); | 3 | (only) redistributor interrupts |
4 | This is now the default action that the NVIC does if the line is | 4 | * if it finds a redistributor interrupt that is (now) higher |
5 | not connected, so we can delete the handling code. | 5 | priority than the previous highest-priority pending interrupt, |
6 | then this must be the new highest-priority pending interrupt | ||
7 | * if it does *not* find a better redistributor interrupt, then: | ||
8 | - if the previous state was "no interrupts pending" then | ||
9 | the new state is still "no interrupts pending" | ||
10 | - if the previous best interrupt was not a redistributor | ||
11 | interrupt then that remains the best interrupt | ||
12 | - if the previous best interrupt *was* a redistributor interrupt, | ||
13 | then the new best interrupt must be some non-redistributor | ||
14 | interrupt, but we don't know which so must do a full scan | ||
15 | |||
16 | In commit 17fb5e36aabd4b2c125 we effectively added the LPI interrupts | ||
17 | as a kind of "redistributor interrupt" for this purpose, by adding | ||
18 | cs->hpplpi to the set of things that gicv3_redist_update() considers | ||
19 | before it gives up and decides to do a full scan of distributor | ||
20 | interrupts. However we didn't quite get this right: | ||
21 | * the condition check for "was the previous best interrupt a | ||
22 | redistributor interrupt" must be updated to include LPIs | ||
23 | in what it considers to be redistributor interrupts | ||
24 | * every code path which updates the LPI state which | ||
25 | gicv3_redist_update() checks must also call gicv3_redist_update(): | ||
26 | this is cs->hpplpi and the GICR_CTLR ENABLE_LPIS bit | ||
27 | |||
28 | This commit fixes this by: | ||
29 | * correcting the test on cs->hppi.irq in gicv3_redist_update() | ||
30 | * making gicv3_redist_update_lpi() always call gicv3_redist_update() | ||
31 | * introducing a new gicv3_redist_update_lpi_only() for the one | ||
32 | callsite (the post-load hook) which must not call | ||
33 | gicv3_redist_update() | ||
34 | * making gicv3_redist_lpi_pending() always call gicv3_redist_update(), | ||
35 | either directly or via gicv3_redist_update_lpi() | ||
36 | * removing a couple of now-unnecessary calls to gicv3_redist_update() | ||
37 | from some callers of those two functions | ||
38 | * calling gicv3_redist_update() when the GICR_CTLR ENABLE_LPIS | ||
39 | bit is cleared | ||
40 | |||
41 | (This means that the not-file-local gicv3_redist_* LPI related | ||
42 | functions now all take care of the updates of internally cached | ||
43 | GICv3 information, in the same way the older functions | ||
44 | gicv3_redist_set_irq() and gicv3_redist_send_sgi() do.) | ||
45 | |||
46 | The visible effect of this bug was that when the guest acknowledged | ||
47 | an LPI by reading ICC_IAR1_EL1, we marked it as not pending in the | ||
48 | LPI data structure but still left it in cs->hppi so we would offer it | ||
49 | to the guest again. In particular for setups using an emulated GICv3 | ||
50 | and ITS and using devices which use LPIs (ie PCI devices) a Linux | ||
51 | guest would complain "irq 54: nobody cared" and then hang. (The hang | ||
52 | was intermittent, presumably depending on the timing between | ||
53 | different interrupts arriving and being completed.) | ||
6 | 54 | ||
7 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 55 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
8 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | 56 | Tested-by: Alex Bennée <alex.bennee@linaro.org> |
9 | Reviewed-by: Alistair Francis <alistair.francis@wdc.com> | 57 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> |
10 | Message-id: 20200728103744.6909-4-peter.maydell@linaro.org | 58 | Message-id: 20211124202005.989935-1-peter.maydell@linaro.org |
11 | --- | 59 | --- |
12 | hw/arm/msf2-soc.c | 11 ----------- | 60 | hw/intc/gicv3_internal.h | 17 +++++++++++++++++ |
13 | hw/arm/stellaris.c | 12 ------------ | 61 | hw/intc/arm_gicv3.c | 6 ++++-- |
14 | 2 files changed, 23 deletions(-) | 62 | hw/intc/arm_gicv3_redist.c | 14 ++++++++++---- |
63 | 3 files changed, 31 insertions(+), 6 deletions(-) | ||
15 | 64 | ||
16 | diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c | 65 | diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h |
17 | index XXXXXXX..XXXXXXX 100644 | 66 | index XXXXXXX..XXXXXXX 100644 |
18 | --- a/hw/arm/msf2-soc.c | 67 | --- a/hw/intc/gicv3_internal.h |
19 | +++ b/hw/arm/msf2-soc.c | 68 | +++ b/hw/intc/gicv3_internal.h |
20 | @@ -XXX,XX +XXX,XX @@ | 69 | @@ -XXX,XX +XXX,XX @@ void gicv3_dist_set_irq(GICv3State *s, int irq, int level); |
21 | #include "hw/irq.h" | 70 | void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level); |
22 | #include "hw/arm/msf2-soc.h" | 71 | void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level); |
23 | #include "hw/misc/unimp.h" | 72 | void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level); |
24 | -#include "sysemu/runstate.h" | 73 | +/** |
25 | #include "sysemu/sysemu.h" | 74 | + * gicv3_redist_update_lpi: |
26 | 75 | + * @cs: GICv3CPUState | |
27 | #define MSF2_TIMER_BASE 0x40004000 | 76 | + * |
28 | @@ -XXX,XX +XXX,XX @@ static const int spi_irq[MSF2_NUM_SPIS] = { 2, 3 }; | 77 | + * Scan the LPI pending table and recalculate the highest priority |
29 | static const int uart_irq[MSF2_NUM_UARTS] = { 10, 11 }; | 78 | + * pending LPI and also the overall highest priority pending interrupt. |
30 | static const int timer_irq[MSF2_NUM_TIMERS] = { 14, 15 }; | 79 | + */ |
31 | 80 | void gicv3_redist_update_lpi(GICv3CPUState *cs); | |
32 | -static void do_sys_reset(void *opaque, int n, int level) | 81 | +/** |
33 | -{ | 82 | + * gicv3_redist_update_lpi_only: |
34 | - if (level) { | 83 | + * @cs: GICv3CPUState |
35 | - qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); | 84 | + * |
36 | - } | 85 | + * Scan the LPI pending table and recalculate cs->hpplpi only, |
37 | -} | 86 | + * without calling gicv3_redist_update() to recalculate the overall |
87 | + * highest priority pending interrupt. This should be called after | ||
88 | + * an incoming migration has loaded new state. | ||
89 | + */ | ||
90 | +void gicv3_redist_update_lpi_only(GICv3CPUState *cs); | ||
91 | void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq, bool ns); | ||
92 | void gicv3_init_cpuif(GICv3State *s); | ||
93 | |||
94 | diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c | ||
95 | index XXXXXXX..XXXXXXX 100644 | ||
96 | --- a/hw/intc/arm_gicv3.c | ||
97 | +++ b/hw/intc/arm_gicv3.c | ||
98 | @@ -XXX,XX +XXX,XX @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs) | ||
99 | * interrupt has reduced in priority and any other interrupt could | ||
100 | * now be the new best one). | ||
101 | */ | ||
102 | - if (!seenbetter && cs->hppi.prio != 0xff && cs->hppi.irq < GIC_INTERNAL) { | ||
103 | + if (!seenbetter && cs->hppi.prio != 0xff && | ||
104 | + (cs->hppi.irq < GIC_INTERNAL || | ||
105 | + cs->hppi.irq >= GICV3_LPI_INTID_START)) { | ||
106 | gicv3_full_update_noirqset(cs->gic); | ||
107 | } | ||
108 | } | ||
109 | @@ -XXX,XX +XXX,XX @@ static void arm_gicv3_post_load(GICv3State *s) | ||
110 | * pending interrupt, but don't set IRQ or FIQ lines. | ||
111 | */ | ||
112 | for (i = 0; i < s->num_cpu; i++) { | ||
113 | - gicv3_redist_update_lpi(&s->cpu[i]); | ||
114 | + gicv3_redist_update_lpi_only(&s->cpu[i]); | ||
115 | } | ||
116 | gicv3_full_update_noirqset(s); | ||
117 | /* Repopulate the cache of GICv3CPUState pointers for target CPUs */ | ||
118 | diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c | ||
119 | index XXXXXXX..XXXXXXX 100644 | ||
120 | --- a/hw/intc/arm_gicv3_redist.c | ||
121 | +++ b/hw/intc/arm_gicv3_redist.c | ||
122 | @@ -XXX,XX +XXX,XX @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset, | ||
123 | cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS; | ||
124 | /* Check for any pending interr in pending table */ | ||
125 | gicv3_redist_update_lpi(cs); | ||
126 | - gicv3_redist_update(cs); | ||
127 | } else { | ||
128 | cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS; | ||
129 | + /* cs->hppi might have been an LPI; recalculate */ | ||
130 | + gicv3_redist_update(cs); | ||
131 | } | ||
132 | } | ||
133 | return MEMTX_OK; | ||
134 | @@ -XXX,XX +XXX,XX @@ static void gicv3_redist_check_lpi_priority(GICv3CPUState *cs, int irq) | ||
135 | } | ||
136 | } | ||
137 | |||
138 | -void gicv3_redist_update_lpi(GICv3CPUState *cs) | ||
139 | +void gicv3_redist_update_lpi_only(GICv3CPUState *cs) | ||
140 | { | ||
141 | /* | ||
142 | * This function scans the LPI pending table and for each pending | ||
143 | @@ -XXX,XX +XXX,XX @@ void gicv3_redist_update_lpi(GICv3CPUState *cs) | ||
144 | } | ||
145 | } | ||
146 | |||
147 | +void gicv3_redist_update_lpi(GICv3CPUState *cs) | ||
148 | +{ | ||
149 | + gicv3_redist_update_lpi_only(cs); | ||
150 | + gicv3_redist_update(cs); | ||
151 | +} | ||
152 | + | ||
153 | void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level) | ||
154 | { | ||
155 | /* | ||
156 | @@ -XXX,XX +XXX,XX @@ void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level) | ||
157 | */ | ||
158 | if (level) { | ||
159 | gicv3_redist_check_lpi_priority(cs, irq); | ||
160 | + gicv3_redist_update(cs); | ||
161 | } else { | ||
162 | if (irq == cs->hpplpi.irq) { | ||
163 | gicv3_redist_update_lpi(cs); | ||
164 | @@ -XXX,XX +XXX,XX @@ void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level) | ||
165 | |||
166 | /* set/clear the pending bit for this irq */ | ||
167 | gicv3_redist_lpi_pending(cs, irq, level); | ||
38 | - | 168 | - |
39 | static void m2sxxx_soc_initfn(Object *obj) | 169 | - gicv3_redist_update(cs); |
40 | { | ||
41 | MSF2State *s = MSF2_SOC(obj); | ||
42 | @@ -XXX,XX +XXX,XX @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) | ||
43 | return; | ||
44 | } | ||
45 | |||
46 | - qdev_connect_gpio_out_named(DEVICE(&s->armv7m.nvic), "SYSRESETREQ", 0, | ||
47 | - qemu_allocate_irq(&do_sys_reset, NULL, 0)); | ||
48 | - | ||
49 | system_clock_scale = NANOSECONDS_PER_SECOND / s->m3clk; | ||
50 | |||
51 | for (i = 0; i < MSF2_NUM_UARTS; i++) { | ||
52 | diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c | ||
53 | index XXXXXXX..XXXXXXX 100644 | ||
54 | --- a/hw/arm/stellaris.c | ||
55 | +++ b/hw/arm/stellaris.c | ||
56 | @@ -XXX,XX +XXX,XX @@ | ||
57 | #include "hw/boards.h" | ||
58 | #include "qemu/log.h" | ||
59 | #include "exec/address-spaces.h" | ||
60 | -#include "sysemu/runstate.h" | ||
61 | #include "sysemu/sysemu.h" | ||
62 | #include "hw/arm/armv7m.h" | ||
63 | #include "hw/char/pl011.h" | ||
64 | @@ -XXX,XX +XXX,XX @@ static void stellaris_adc_init(Object *obj) | ||
65 | qdev_init_gpio_in(dev, stellaris_adc_trigger, 1); | ||
66 | } | 170 | } |
67 | 171 | ||
68 | -static | 172 | void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level) |
69 | -void do_sys_reset(void *opaque, int n, int level) | ||
70 | -{ | ||
71 | - if (level) { | ||
72 | - qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); | ||
73 | - } | ||
74 | -} | ||
75 | - | ||
76 | /* Board init. */ | ||
77 | static stellaris_board_info stellaris_boards[] = { | ||
78 | { "LM3S811EVB", | ||
79 | @@ -XXX,XX +XXX,XX @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) | ||
80 | /* This will exit with an error if the user passed us a bad cpu_type */ | ||
81 | sysbus_realize_and_unref(SYS_BUS_DEVICE(nvic), &error_fatal); | ||
82 | |||
83 | - qdev_connect_gpio_out_named(nvic, "SYSRESETREQ", 0, | ||
84 | - qemu_allocate_irq(&do_sys_reset, NULL, 0)); | ||
85 | - | ||
86 | if (board->dc1 & (1 << 16)) { | ||
87 | dev = sysbus_create_varargs(TYPE_STELLARIS_ADC, 0x40038000, | ||
88 | qdev_get_gpio_in(nvic, 14), | ||
89 | -- | 173 | -- |
90 | 2.20.1 | 174 | 2.25.1 |
91 | 175 | ||
92 | 176 | diff view generated by jsdifflib |
1 | Mostly devices don't need to care whether one of their output | 1 | The GICv3/v4 pseudocode has a function IsSpecial() which returns true |
---|---|---|---|
2 | qemu_irq lines is connected, because functions like qemu_set_irq() | 2 | if passed a "special" interrupt ID number (anything between 1020 and |
3 | silently do nothing if there is nothing on the other end. However | 3 | 1023 inclusive). We open-code this condition in a couple of places, |
4 | sometimes a device might want to implement default behaviour for the | 4 | so abstract it out into a new function gicv3_intid_is_special(). |
5 | case where the machine hasn't wired the line up to anywhere. | ||
6 | |||
7 | Provide a function qemu_irq_is_connected() that devices can use for | ||
8 | this purpose. (The test is trivial but encapsulating it in a | ||
9 | function makes it easier to see where we're doing it in case we need | ||
10 | to change the implementation later.) | ||
11 | 5 | ||
12 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 6 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
13 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | 7 | Reviewed-by: Marc Zyngier <maz@kernel.org> |
14 | Reviewed-by: Alistair Francis <alistair.francis@wdc.com> | 8 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> |
15 | Message-id: 20200728103744.6909-2-peter.maydell@linaro.org | ||
16 | --- | 9 | --- |
17 | include/hw/irq.h | 18 ++++++++++++++++++ | 10 | hw/intc/gicv3_internal.h | 13 +++++++++++++ |
18 | 1 file changed, 18 insertions(+) | 11 | hw/intc/arm_gicv3_cpuif.c | 4 ++-- |
12 | 2 files changed, 15 insertions(+), 2 deletions(-) | ||
19 | 13 | ||
20 | diff --git a/include/hw/irq.h b/include/hw/irq.h | 14 | diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h |
21 | index XXXXXXX..XXXXXXX 100644 | 15 | index XXXXXXX..XXXXXXX 100644 |
22 | --- a/include/hw/irq.h | 16 | --- a/hw/intc/gicv3_internal.h |
23 | +++ b/include/hw/irq.h | 17 | +++ b/hw/intc/gicv3_internal.h |
24 | @@ -XXX,XX +XXX,XX @@ qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq irq2); | 18 | @@ -XXX,XX +XXX,XX @@ FIELD(MAPC, RDBASE, 16, 32) |
25 | on an existing vector of qemu_irq. */ | 19 | |
26 | void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n); | 20 | /* Functions internal to the emulated GICv3 */ |
27 | 21 | ||
28 | +/** | 22 | +/** |
29 | + * qemu_irq_is_connected: Return true if IRQ line is wired up | 23 | + * gicv3_intid_is_special: |
24 | + * @intid: interrupt ID | ||
30 | + * | 25 | + * |
31 | + * If a qemu_irq has a device on the other (receiving) end of it, | 26 | + * Return true if @intid is a special interrupt ID (1020 to |
32 | + * return true; otherwise return false. | 27 | + * 1023 inclusive). This corresponds to the GIC spec pseudocode |
33 | + * | 28 | + * IsSpecial() function. |
34 | + * Usually device models don't need to care whether the machine model | ||
35 | + * has wired up their outbound qemu_irq lines, because functions like | ||
36 | + * qemu_set_irq() silently do nothing if there is nothing on the other | ||
37 | + * end of the line. However occasionally a device model will want to | ||
38 | + * provide default behaviour if its output is left floating, and | ||
39 | + * it can use this function to identify when that is the case. | ||
40 | + */ | 29 | + */ |
41 | +static inline bool qemu_irq_is_connected(qemu_irq irq) | 30 | +static inline bool gicv3_intid_is_special(int intid) |
42 | +{ | 31 | +{ |
43 | + return irq != NULL; | 32 | + return intid >= INTID_SECURE && intid <= INTID_SPURIOUS; |
44 | +} | 33 | +} |
45 | + | 34 | + |
46 | #endif | 35 | /** |
36 | * gicv3_redist_update: | ||
37 | * @cs: GICv3CPUState for this redistributor | ||
38 | diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c | ||
39 | index XXXXXXX..XXXXXXX 100644 | ||
40 | --- a/hw/intc/arm_gicv3_cpuif.c | ||
41 | +++ b/hw/intc/arm_gicv3_cpuif.c | ||
42 | @@ -XXX,XX +XXX,XX @@ static uint64_t icc_iar0_read(CPUARMState *env, const ARMCPRegInfo *ri) | ||
43 | intid = icc_hppir0_value(cs, env); | ||
44 | } | ||
45 | |||
46 | - if (!(intid >= INTID_SECURE && intid <= INTID_SPURIOUS)) { | ||
47 | + if (!gicv3_intid_is_special(intid)) { | ||
48 | icc_activate_irq(cs, intid); | ||
49 | } | ||
50 | |||
51 | @@ -XXX,XX +XXX,XX @@ static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri) | ||
52 | intid = icc_hppir1_value(cs, env); | ||
53 | } | ||
54 | |||
55 | - if (!(intid >= INTID_SECURE && intid <= INTID_SPURIOUS)) { | ||
56 | + if (!gicv3_intid_is_special(intid)) { | ||
57 | icc_activate_irq(cs, intid); | ||
58 | } | ||
59 | |||
47 | -- | 60 | -- |
48 | 2.20.1 | 61 | 2.25.1 |
49 | 62 | ||
50 | 63 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | The NVIC provides an outbound qemu_irq "SYSRESETREQ" which it signals | ||
2 | when the guest sets the SYSRESETREQ bit in the AIRCR register. This | ||
3 | matches the hardware design (where the CPU has a signal of this name | ||
4 | and it is up to the SoC to connect that up to an actual reset | ||
5 | mechanism), but in QEMU it mostly results in duplicated code in SoC | ||
6 | objects and bugs where SoC model implementors forget to wire up the | ||
7 | SYSRESETREQ line. | ||
8 | 1 | ||
9 | Provide a default behaviour for the case where SYSRESETREQ is not | ||
10 | actually connected to anything: use qemu_system_reset_request() to | ||
11 | perform a system reset. This will allow us to remove the | ||
12 | implementations of SYSRESETREQ handling from the boards where that's | ||
13 | exactly what it does, and also fixes the bugs in the board models | ||
14 | which forgot to wire up the signal: | ||
15 | |||
16 | * microbit | ||
17 | * mps2-an385 | ||
18 | * mps2-an505 | ||
19 | * mps2-an511 | ||
20 | * mps2-an521 | ||
21 | * musca-a | ||
22 | * musca-b1 | ||
23 | * netduino | ||
24 | * netduinoplus2 | ||
25 | |||
26 | We still allow the board to wire up the signal if it needs to, in case | ||
27 | we need to model more complicated reset controller logic or to model | ||
28 | buggy SoC hardware which forgot to wire up the line itself. But | ||
29 | defaulting to "reset the system" is more often going to be correct | ||
30 | than defaulting to "do nothing". | ||
31 | |||
32 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
33 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | ||
34 | Reviewed-by: Alistair Francis <alistair.francis@wdc.com> | ||
35 | Message-id: 20200728103744.6909-3-peter.maydell@linaro.org | ||
36 | --- | ||
37 | include/hw/arm/armv7m.h | 4 +++- | ||
38 | hw/intc/armv7m_nvic.c | 17 ++++++++++++++++- | ||
39 | 2 files changed, 19 insertions(+), 2 deletions(-) | ||
40 | |||
41 | diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h | ||
42 | index XXXXXXX..XXXXXXX 100644 | ||
43 | --- a/include/hw/arm/armv7m.h | ||
44 | +++ b/include/hw/arm/armv7m.h | ||
45 | @@ -XXX,XX +XXX,XX @@ typedef struct { | ||
46 | |||
47 | /* ARMv7M container object. | ||
48 | * + Unnamed GPIO input lines: external IRQ lines for the NVIC | ||
49 | - * + Named GPIO output SYSRESETREQ: signalled for guest AIRCR.SYSRESETREQ | ||
50 | + * + Named GPIO output SYSRESETREQ: signalled for guest AIRCR.SYSRESETREQ. | ||
51 | + * If this GPIO is not wired up then the NVIC will default to performing | ||
52 | + * a qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET). | ||
53 | * + Property "cpu-type": CPU type to instantiate | ||
54 | * + Property "num-irq": number of external IRQ lines | ||
55 | * + Property "memory": MemoryRegion defining the physical address space | ||
56 | diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c | ||
57 | index XXXXXXX..XXXXXXX 100644 | ||
58 | --- a/hw/intc/armv7m_nvic.c | ||
59 | +++ b/hw/intc/armv7m_nvic.c | ||
60 | @@ -XXX,XX +XXX,XX @@ | ||
61 | #include "hw/intc/armv7m_nvic.h" | ||
62 | #include "hw/irq.h" | ||
63 | #include "hw/qdev-properties.h" | ||
64 | +#include "sysemu/runstate.h" | ||
65 | #include "target/arm/cpu.h" | ||
66 | #include "exec/exec-all.h" | ||
67 | #include "exec/memop.h" | ||
68 | @@ -XXX,XX +XXX,XX @@ static const uint8_t nvic_id[] = { | ||
69 | 0x00, 0xb0, 0x1b, 0x00, 0x0d, 0xe0, 0x05, 0xb1 | ||
70 | }; | ||
71 | |||
72 | +static void signal_sysresetreq(NVICState *s) | ||
73 | +{ | ||
74 | + if (qemu_irq_is_connected(s->sysresetreq)) { | ||
75 | + qemu_irq_pulse(s->sysresetreq); | ||
76 | + } else { | ||
77 | + /* | ||
78 | + * Default behaviour if the SoC doesn't need to wire up | ||
79 | + * SYSRESETREQ (eg to a system reset controller of some kind): | ||
80 | + * perform a system reset via the usual QEMU API. | ||
81 | + */ | ||
82 | + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); | ||
83 | + } | ||
84 | +} | ||
85 | + | ||
86 | static int nvic_pending_prio(NVICState *s) | ||
87 | { | ||
88 | /* return the group priority of the current pending interrupt, | ||
89 | @@ -XXX,XX +XXX,XX @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, | ||
90 | if (value & R_V7M_AIRCR_SYSRESETREQ_MASK) { | ||
91 | if (attrs.secure || | ||
92 | !(cpu->env.v7m.aircr & R_V7M_AIRCR_SYSRESETREQS_MASK)) { | ||
93 | - qemu_irq_pulse(s->sysresetreq); | ||
94 | + signal_sysresetreq(s); | ||
95 | } | ||
96 | } | ||
97 | if (value & R_V7M_AIRCR_VECTCLRACTIVE_MASK) { | ||
98 | -- | ||
99 | 2.20.1 | ||
100 | |||
101 | diff view generated by jsdifflib |
1 | The nrf51 SoC model wasn't setting the system_clock_scale | 1 | It is valid for an OS to put virtual interrupt ID values into the |
---|---|---|---|
2 | global.which meant that if guest code used the systick timer in "use | 2 | list registers ICH_LR<n> which are greater than 1023. This |
3 | the processor clock" mode it would hang because time never advances. | 3 | corresponds to (for example) KVM using the in-kernel emulated ITS to |
4 | give a (nested) guest an ITS. LPIs are delivered by the L1 kernel to | ||
5 | the L2 guest via the list registers in the same way as non-LPI | ||
6 | interrupts. | ||
4 | 7 | ||
5 | Set the global to match the documented CPU clock speed for this SoC. | 8 | QEMU's code for handling writes to ICV_IARn (which happen when the L2 |
9 | guest acknowledges an interrupt) and to ICV_EOIRn (which happen at | ||
10 | the end of the interrupt) did not consider LPIs, so it would | ||
11 | incorrectly treat interrupt IDs above 1023 as invalid. Fix this by | ||
12 | using the correct condition, which is gicv3_intid_is_special(). | ||
6 | 13 | ||
7 | This SoC in fact doesn't have a SysTick timer (which is the only thing | 14 | Note that the condition in icv_dir_write() is correct -- LPIs |
8 | currently that cares about the system_clock_scale), because it's | 15 | are not valid there and so we want to ignore both "special" ID |
9 | a configurable option in the Cortex-M0. However our Cortex-M0 and | 16 | values and LPIs. |
10 | thus our nrf51 and our micro:bit board do provide a SysTick, so | 17 | |
11 | we ought to provide a functional one rather than a broken one. | 18 | (In the pseudocode this logic is in: |
19 | - VirtualReadIAR0(), VirtualReadIAR1(), which call IsSpecial() | ||
20 | - VirtualWriteEOIR0(), VirtualWriteEOIR1(), which call | ||
21 | VirtualIdentifierValid(data, TRUE) meaning "LPIs OK" | ||
22 | - VirtualWriteDIR(), which calls VirtualIdentifierValid(data, FALSE) | ||
23 | meaning "LPIs not OK") | ||
24 | |||
25 | This bug doesn't seem to have any visible effect on Linux L2 guests | ||
26 | most of the time, because the two bugs cancel each other out: we | ||
27 | neither mark the interrupt active nor deactivate it. However it does | ||
28 | mean that the L2 vCPU priority while the LPI handler is running will | ||
29 | not be correct, so the interrupt handler could be unexpectedly | ||
30 | interrupted by a different interrupt. | ||
31 | |||
32 | (NB: this has nothing to do with using QEMU's emulated ITS.) | ||
12 | 33 | ||
13 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 34 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
14 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | 35 | Reviewed-by: Marc Zyngier <maz@kernel.org> |
15 | Message-id: 20200727193458.31250-1-peter.maydell@linaro.org | ||
16 | --- | 36 | --- |
17 | hw/arm/nrf51_soc.c | 5 +++++ | 37 | hw/intc/arm_gicv3_cpuif.c | 5 ++--- |
18 | 1 file changed, 5 insertions(+) | 38 | 1 file changed, 2 insertions(+), 3 deletions(-) |
19 | 39 | ||
20 | diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c | 40 | diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c |
21 | index XXXXXXX..XXXXXXX 100644 | 41 | index XXXXXXX..XXXXXXX 100644 |
22 | --- a/hw/arm/nrf51_soc.c | 42 | --- a/hw/intc/arm_gicv3_cpuif.c |
23 | +++ b/hw/arm/nrf51_soc.c | 43 | +++ b/hw/intc/arm_gicv3_cpuif.c |
24 | @@ -XXX,XX +XXX,XX @@ | 44 | @@ -XXX,XX +XXX,XX @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri) |
25 | 45 | ||
26 | #define BASE_TO_IRQ(base) ((base >> 12) & 0x1F) | 46 | if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) { |
27 | 47 | intid = ich_lr_vintid(lr); | |
28 | +/* HCLK (the main CPU clock) on this SoC is always 16MHz */ | 48 | - if (intid < INTID_SECURE) { |
29 | +#define HCLK_FRQ 16000000 | 49 | + if (!gicv3_intid_is_special(intid)) { |
30 | + | 50 | icv_activate_irq(cs, idx, grp); |
31 | static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size) | 51 | } else { |
32 | { | 52 | /* Interrupt goes from Pending to Invalid */ |
33 | qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n", | 53 | @@ -XXX,XX +XXX,XX @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, |
34 | @@ -XXX,XX +XXX,XX @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp) | 54 | trace_gicv3_icv_eoir_write(ri->crm == 8 ? 0 : 1, |
55 | gicv3_redist_affid(cs), value); | ||
56 | |||
57 | - if (irq >= GICV3_MAXIRQ) { | ||
58 | - /* Also catches special interrupt numbers and LPIs */ | ||
59 | + if (gicv3_intid_is_special(irq)) { | ||
35 | return; | 60 | return; |
36 | } | 61 | } |
37 | 62 | ||
38 | + system_clock_scale = NANOSECONDS_PER_SECOND / HCLK_FRQ; | ||
39 | + | ||
40 | object_property_set_link(OBJECT(&s->cpu), "memory", OBJECT(&s->container), | ||
41 | &error_abort); | ||
42 | if (!sysbus_realize(SYS_BUS_DEVICE(&s->cpu), errp)) { | ||
43 | -- | 63 | -- |
44 | 2.20.1 | 64 | 2.25.1 |
45 | 65 | ||
46 | 66 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | The imx_epit device has a software-controllable reset triggered by | ||
2 | setting the SWR bit in the CR register. An error in commit cc2722ec83ad9 | ||
3 | means that we will end up assert()ing if the guest does this, because | ||
4 | the code in imx_epit_write() starts ptimer transactions, and then | ||
5 | imx_epit_reset() also starts ptimer transactions, triggering | ||
6 | "ptimer_transaction_begin: Assertion `!s->in_transaction' failed". | ||
7 | 1 | ||
8 | The cleanest way to avoid this double-transaction is to move the | ||
9 | start-transaction for the CR write handling down below the check of | ||
10 | the SWR bit. | ||
11 | |||
12 | Fixes: https://bugs.launchpad.net/qemu/+bug/1880424 | ||
13 | Fixes: cc2722ec83ad944505fe | ||
14 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
15 | Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> | ||
16 | Message-id: 20200727154550.3409-1-peter.maydell@linaro.org | ||
17 | --- | ||
18 | hw/timer/imx_epit.c | 13 ++++++++++--- | ||
19 | 1 file changed, 10 insertions(+), 3 deletions(-) | ||
20 | |||
21 | diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c | ||
22 | index XXXXXXX..XXXXXXX 100644 | ||
23 | --- a/hw/timer/imx_epit.c | ||
24 | +++ b/hw/timer/imx_epit.c | ||
25 | @@ -XXX,XX +XXX,XX @@ static void imx_epit_write(void *opaque, hwaddr offset, uint64_t value, | ||
26 | |||
27 | switch (offset >> 2) { | ||
28 | case 0: /* CR */ | ||
29 | - ptimer_transaction_begin(s->timer_cmp); | ||
30 | - ptimer_transaction_begin(s->timer_reload); | ||
31 | |||
32 | oldcr = s->cr; | ||
33 | s->cr = value & 0x03ffffff; | ||
34 | if (s->cr & CR_SWR) { | ||
35 | /* handle the reset */ | ||
36 | imx_epit_reset(DEVICE(s)); | ||
37 | - } else { | ||
38 | + /* | ||
39 | + * TODO: could we 'break' here? following operations appear | ||
40 | + * to duplicate the work imx_epit_reset() already did. | ||
41 | + */ | ||
42 | + } | ||
43 | + | ||
44 | + ptimer_transaction_begin(s->timer_cmp); | ||
45 | + ptimer_transaction_begin(s->timer_reload); | ||
46 | + | ||
47 | + if (!(s->cr & CR_SWR)) { | ||
48 | imx_epit_set_freq(s); | ||
49 | } | ||
50 | |||
51 | -- | ||
52 | 2.20.1 | ||
53 | |||
54 | diff view generated by jsdifflib |