| 1 | Hi; this is a collection of mostly GIC related patches for rc3. | 1 | Only thing for Arm for rc1 is RTH's fix for the KVM SVE probe code. |
|---|---|---|---|
| 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). | ||
| 7 | 2 | ||
| 8 | thanks | ||
| 9 | -- PMM | 3 | -- PMM |
| 10 | 4 | ||
| 11 | The following changes since commit dd4b0de45965538f19bb40c7ddaaba384a8c613a: | 5 | The following changes since commit 4e06b3fc1b5e1ec03f22190eabe56891dc9c2236: |
| 12 | 6 | ||
| 13 | Fix version for v6.2.0-rc2 release (2021-11-26 11:58:54 +0100) | 7 | Merge tag 'pull-hex-20220731' of https://github.com/quic/qemu into staging (2022-07-31 21:38:54 -0700) |
| 14 | 8 | ||
| 15 | are available in the Git repository at: | 9 | are available in the Git repository at: |
| 16 | 10 | ||
| 17 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20211129 | 11 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20220801 |
| 18 | 12 | ||
| 19 | for you to fetch changes up to 90feffad2aafe856ed2af75313b2c1669ba671e9: | 13 | for you to fetch changes up to 5265d24c981dfdda8d29b44f7e84a514da75eedc: |
| 20 | 14 | ||
| 21 | hw/intc/arm_gicv3: fix handling of LPIs in list registers (2021-11-29 10:10:21 +0000) | 15 | target/arm: Move sve probe inside kvm >= 4.15 branch (2022-08-01 16:21:18 +0100) |
| 22 | 16 | ||
| 23 | ---------------------------------------------------------------- | 17 | ---------------------------------------------------------------- |
| 24 | target-arm queue: | 18 | target-arm queue: |
| 25 | * virt: Diagnose attempts to enable MTE or virt when using HVF accelerator | 19 | * Fix KVM SVE ID register probe code |
| 26 | * GICv3 ITS: Allow clearing of ITS CTLR Enabled bit | ||
| 27 | * GICv3: Update cached state after LPI state changes | ||
| 28 | * GICv3: Fix handling of LPIs in list registers | ||
| 29 | 20 | ||
| 30 | ---------------------------------------------------------------- | 21 | ---------------------------------------------------------------- |
| 31 | Alexander Graf (1): | 22 | Richard Henderson (3): |
| 32 | hw/arm/virt: Extend nested and mte checks to hvf | 23 | target/arm: Use kvm_arm_sve_supported in kvm_arm_get_host_cpu_features |
| 24 | target/arm: Set KVM_ARM_VCPU_SVE while probing the host | ||
| 25 | target/arm: Move sve probe inside kvm >= 4.15 branch | ||
| 33 | 26 | ||
| 34 | Peter Maydell (3): | 27 | target/arm/kvm64.c | 45 ++++++++++++++++++++++----------------------- |
| 35 | hw/intc/arm_gicv3: Update cached state after LPI state changes | 28 | 1 file changed, 22 insertions(+), 23 deletions(-) |
| 36 | hw/intc/arm_gicv3: Add new gicv3_intid_is_special() function | ||
| 37 | hw/intc/arm_gicv3: fix handling of LPIs in list registers | ||
| 38 | |||
| 39 | Shashi Mallela (1): | ||
| 40 | hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit | ||
| 41 | |||
| 42 | hw/intc/gicv3_internal.h | 30 ++++++++++++++++++++++++++++++ | ||
| 43 | hw/arm/virt.c | 15 +++++++++------ | ||
| 44 | hw/intc/arm_gicv3.c | 6 ++++-- | ||
| 45 | hw/intc/arm_gicv3_cpuif.c | 9 ++++----- | ||
| 46 | hw/intc/arm_gicv3_its.c | 7 ++++--- | ||
| 47 | hw/intc/arm_gicv3_redist.c | 14 ++++++++++---- | ||
| 48 | 6 files changed, 61 insertions(+), 20 deletions(-) | ||
| 49 | diff view generated by jsdifflib |
| 1 | It is valid for an OS to put virtual interrupt ID values into the | 1 | From: Richard Henderson <richard.henderson@linaro.org> |
|---|---|---|---|
| 2 | list registers ICH_LR<n> which are greater than 1023. This | ||
| 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. | ||
| 7 | 2 | ||
| 8 | QEMU's code for handling writes to ICV_IARn (which happen when the L2 | 3 | Indication for support for SVE will not depend on whether we |
| 9 | guest acknowledges an interrupt) and to ICV_EOIRn (which happen at | 4 | perform the query on the main kvm_state or the temp vcpu. |
| 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(). | ||
| 13 | 5 | ||
| 14 | Note that the condition in icv_dir_write() is correct -- LPIs | 6 | Signed-off-by: Richard Henderson <richard.henderson@linaro.org> |
| 15 | are not valid there and so we want to ignore both "special" ID | 7 | Message-id: 20220726045828.53697-2-richard.henderson@linaro.org |
| 16 | values and LPIs. | 8 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> |
| 9 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
| 10 | --- | ||
| 11 | target/arm/kvm64.c | 2 +- | ||
| 12 | 1 file changed, 1 insertion(+), 1 deletion(-) | ||
| 17 | 13 | ||
| 18 | (In the pseudocode this logic is in: | 14 | diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c |
| 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.) | ||
| 33 | |||
| 34 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
| 35 | Reviewed-by: Marc Zyngier <maz@kernel.org> | ||
| 36 | --- | ||
| 37 | hw/intc/arm_gicv3_cpuif.c | 5 ++--- | ||
| 38 | 1 file changed, 2 insertions(+), 3 deletions(-) | ||
| 39 | |||
| 40 | diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c | ||
| 41 | index XXXXXXX..XXXXXXX 100644 | 15 | index XXXXXXX..XXXXXXX 100644 |
| 42 | --- a/hw/intc/arm_gicv3_cpuif.c | 16 | --- a/target/arm/kvm64.c |
| 43 | +++ b/hw/intc/arm_gicv3_cpuif.c | 17 | +++ b/target/arm/kvm64.c |
| 44 | @@ -XXX,XX +XXX,XX @@ static uint64_t icv_iar_read(CPUARMState *env, const ARMCPRegInfo *ri) | 18 | @@ -XXX,XX +XXX,XX @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) |
| 45 | 19 | } | |
| 46 | if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) { | ||
| 47 | intid = ich_lr_vintid(lr); | ||
| 48 | - if (intid < INTID_SECURE) { | ||
| 49 | + if (!gicv3_intid_is_special(intid)) { | ||
| 50 | icv_activate_irq(cs, idx, grp); | ||
| 51 | } else { | ||
| 52 | /* Interrupt goes from Pending to Invalid */ | ||
| 53 | @@ -XXX,XX +XXX,XX @@ static void icv_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri, | ||
| 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)) { | ||
| 60 | return; | ||
| 61 | } | 20 | } |
| 62 | 21 | ||
| 22 | - sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0; | ||
| 23 | + sve_supported = kvm_arm_sve_supported(); | ||
| 24 | |||
| 25 | /* Add feature bits that can't appear until after VCPU init. */ | ||
| 26 | if (sve_supported) { | ||
| 63 | -- | 27 | -- |
| 64 | 2.25.1 | 28 | 2.25.1 |
| 65 | |||
| 66 | diff view generated by jsdifflib |
| 1 | The GICv3/v4 pseudocode has a function IsSpecial() which returns true | 1 | From: Richard Henderson <richard.henderson@linaro.org> |
|---|---|---|---|
| 2 | if passed a "special" interrupt ID number (anything between 1020 and | ||
| 3 | 1023 inclusive). We open-code this condition in a couple of places, | ||
| 4 | so abstract it out into a new function gicv3_intid_is_special(). | ||
| 5 | 2 | ||
| 3 | Because we weren't setting this flag, our probe of ID_AA64ZFR0 | ||
| 4 | was always returning zero. This also obviates the adjustment | ||
| 5 | of ID_AA64PFR0, which had sanitized the SVE field. | ||
| 6 | |||
| 7 | The effects of the bug are not visible, because the only thing that | ||
| 8 | ID_AA64ZFR0 is used for within qemu at present is tcg translation. | ||
| 9 | The other tests for SVE within KVM are via ID_AA64PFR0.SVE. | ||
| 10 | |||
| 11 | Reported-by: Zenghui Yu <yuzenghui@huawei.com> | ||
| 12 | Signed-off-by: Richard Henderson <richard.henderson@linaro.org> | ||
| 13 | Message-id: 20220726045828.53697-3-richard.henderson@linaro.org | ||
| 14 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | ||
| 6 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 15 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
| 7 | Reviewed-by: Marc Zyngier <maz@kernel.org> | ||
| 8 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
| 9 | --- | 16 | --- |
| 10 | hw/intc/gicv3_internal.h | 13 +++++++++++++ | 17 | target/arm/kvm64.c | 27 +++++++++++++-------------- |
| 11 | hw/intc/arm_gicv3_cpuif.c | 4 ++-- | 18 | 1 file changed, 13 insertions(+), 14 deletions(-) |
| 12 | 2 files changed, 15 insertions(+), 2 deletions(-) | ||
| 13 | 19 | ||
| 14 | diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h | 20 | diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c |
| 15 | index XXXXXXX..XXXXXXX 100644 | 21 | index XXXXXXX..XXXXXXX 100644 |
| 16 | --- a/hw/intc/gicv3_internal.h | 22 | --- a/target/arm/kvm64.c |
| 17 | +++ b/hw/intc/gicv3_internal.h | 23 | +++ b/target/arm/kvm64.c |
| 18 | @@ -XXX,XX +XXX,XX @@ FIELD(MAPC, RDBASE, 16, 32) | 24 | @@ -XXX,XX +XXX,XX @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) |
| 19 | 25 | bool sve_supported; | |
| 20 | /* Functions internal to the emulated GICv3 */ | 26 | bool pmu_supported = false; |
| 21 | 27 | uint64_t features = 0; | |
| 22 | +/** | 28 | - uint64_t t; |
| 23 | + * gicv3_intid_is_special: | 29 | int err; |
| 24 | + * @intid: interrupt ID | 30 | |
| 25 | + * | 31 | /* Old kernels may not know about the PREFERRED_TARGET ioctl: however |
| 26 | + * Return true if @intid is a special interrupt ID (1020 to | 32 | @@ -XXX,XX +XXX,XX @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) |
| 27 | + * 1023 inclusive). This corresponds to the GIC spec pseudocode | 33 | struct kvm_vcpu_init init = { .target = -1, }; |
| 28 | + * IsSpecial() function. | 34 | |
| 29 | + */ | 35 | /* |
| 30 | +static inline bool gicv3_intid_is_special(int intid) | 36 | - * Ask for Pointer Authentication if supported. We can't play the |
| 31 | +{ | 37 | - * SVE trick of synthesising the ID reg as KVM won't tell us |
| 32 | + return intid >= INTID_SECURE && intid <= INTID_SPURIOUS; | 38 | - * whether we have the architected or IMPDEF version of PAuth, so |
| 33 | +} | 39 | - * we have to use the actual ID regs. |
| 40 | + * Ask for SVE if supported, so that we can query ID_AA64ZFR0, | ||
| 41 | + * which is otherwise RAZ. | ||
| 42 | + */ | ||
| 43 | + sve_supported = kvm_arm_sve_supported(); | ||
| 44 | + if (sve_supported) { | ||
| 45 | + init.features[0] |= 1 << KVM_ARM_VCPU_SVE; | ||
| 46 | + } | ||
| 34 | + | 47 | + |
| 35 | /** | 48 | + /* |
| 36 | * gicv3_redist_update: | 49 | + * Ask for Pointer Authentication if supported, so that we get |
| 37 | * @cs: GICv3CPUState for this redistributor | 50 | + * the unsanitized field values for AA64ISAR1_EL1. |
| 38 | diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c | 51 | */ |
| 39 | index XXXXXXX..XXXXXXX 100644 | 52 | if (kvm_arm_pauth_supported()) { |
| 40 | --- a/hw/intc/arm_gicv3_cpuif.c | 53 | init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS | |
| 41 | +++ b/hw/intc/arm_gicv3_cpuif.c | 54 | @@ -XXX,XX +XXX,XX @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) |
| 42 | @@ -XXX,XX +XXX,XX @@ static uint64_t icc_iar0_read(CPUARMState *env, const ARMCPRegInfo *ri) | 55 | } |
| 43 | intid = icc_hppir0_value(cs, env); | ||
| 44 | } | 56 | } |
| 45 | 57 | ||
| 46 | - if (!(intid >= INTID_SECURE && intid <= INTID_SPURIOUS)) { | 58 | - sve_supported = kvm_arm_sve_supported(); |
| 47 | + if (!gicv3_intid_is_special(intid)) { | 59 | - |
| 48 | icc_activate_irq(cs, intid); | 60 | - /* Add feature bits that can't appear until after VCPU init. */ |
| 49 | } | 61 | if (sve_supported) { |
| 50 | 62 | - t = ahcf->isar.id_aa64pfr0; | |
| 51 | @@ -XXX,XX +XXX,XX @@ static uint64_t icc_iar1_read(CPUARMState *env, const ARMCPRegInfo *ri) | 63 | - t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1); |
| 52 | intid = icc_hppir1_value(cs, env); | 64 | - ahcf->isar.id_aa64pfr0 = t; |
| 53 | } | 65 | - |
| 54 | 66 | /* | |
| 55 | - if (!(intid >= INTID_SECURE && intid <= INTID_SPURIOUS)) { | 67 | * There is a range of kernels between kernel commit 73433762fcae |
| 56 | + if (!gicv3_intid_is_special(intid)) { | 68 | * and f81cb2c3ad41 which have a bug where the kernel doesn't expose |
| 57 | icc_activate_irq(cs, intid); | 69 | * SYS_ID_AA64ZFR0_EL1 via the ONE_REG API unless the VM has enabled |
| 58 | } | 70 | - * SVE support, so we only read it here, rather than together with all |
| 59 | 71 | - * the other ID registers earlier. | |
| 72 | + * SVE support, which resulted in an error rather than RAZ. | ||
| 73 | + * So only read the register if we set KVM_ARM_VCPU_SVE above. | ||
| 74 | */ | ||
| 75 | err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64zfr0, | ||
| 76 | ARM64_SYS_REG(3, 0, 0, 4, 4)); | ||
| 60 | -- | 77 | -- |
| 61 | 2.25.1 | 78 | 2.25.1 |
| 62 | |||
| 63 | diff view generated by jsdifflib |
| 1 | From: Alexander Graf <agraf@csgraf.de> | 1 | From: Richard Henderson <richard.henderson@linaro.org> |
|---|---|---|---|
| 2 | 2 | ||
| 3 | The virt machine has properties to enable MTE and Nested Virtualization | 3 | The test for the IF block indicates no ID registers are exposed, much |
| 4 | support. However, its check to ensure the backing accel implementation | 4 | less host support for SVE. Move the SVE probe into the ELSE block. |
| 5 | supports it today only looks for KVM and bails out if it finds it. | ||
| 6 | 5 | ||
| 7 | Extend the checks to HVF as well as it does not support either today. | 6 | Signed-off-by: Richard Henderson <richard.henderson@linaro.org> |
| 8 | This will cause QEMU to print a useful error message rather than | 7 | Message-id: 20220726045828.53697-4-richard.henderson@linaro.org |
| 9 | silently ignoring the attempt by the user to enable either MTE or | ||
| 10 | the Virtualization extensions. | ||
| 11 | |||
| 12 | Reported-by: saar amar <saaramar5@gmail.com> | ||
| 13 | Signed-off-by: Alexander Graf <agraf@csgraf.de> | ||
| 14 | Message-id: 20211123122859.22452-1-agraf@csgraf.de | ||
| 15 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | 8 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> |
| 16 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | 9 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
| 17 | --- | 10 | --- |
| 18 | hw/arm/virt.c | 15 +++++++++------ | 11 | target/arm/kvm64.c | 22 +++++++++++----------- |
| 19 | 1 file changed, 9 insertions(+), 6 deletions(-) | 12 | 1 file changed, 11 insertions(+), 11 deletions(-) |
| 20 | 13 | ||
| 21 | diff --git a/hw/arm/virt.c b/hw/arm/virt.c | 14 | diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c |
| 22 | index XXXXXXX..XXXXXXX 100644 | 15 | index XXXXXXX..XXXXXXX 100644 |
| 23 | --- a/hw/arm/virt.c | 16 | --- a/target/arm/kvm64.c |
| 24 | +++ b/hw/arm/virt.c | 17 | +++ b/target/arm/kvm64.c |
| 25 | @@ -XXX,XX +XXX,XX @@ | 18 | @@ -XXX,XX +XXX,XX @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) |
| 26 | #include "sysemu/runstate.h" | 19 | err |= read_sys_reg64(fdarray[2], &ahcf->isar.reset_pmcr_el0, |
| 27 | #include "sysemu/tpm.h" | 20 | ARM64_SYS_REG(3, 3, 9, 12, 0)); |
| 28 | #include "sysemu/kvm.h" | 21 | } |
| 29 | +#include "sysemu/hvf.h" | 22 | - } |
| 30 | #include "hw/loader.h" | 23 | |
| 31 | #include "qapi/error.h" | 24 | - if (sve_supported) { |
| 32 | #include "qemu/bitops.h" | 25 | - /* |
| 33 | @@ -XXX,XX +XXX,XX @@ static void machvirt_init(MachineState *machine) | 26 | - * There is a range of kernels between kernel commit 73433762fcae |
| 34 | exit(1); | 27 | - * and f81cb2c3ad41 which have a bug where the kernel doesn't expose |
| 28 | - * SYS_ID_AA64ZFR0_EL1 via the ONE_REG API unless the VM has enabled | ||
| 29 | - * SVE support, which resulted in an error rather than RAZ. | ||
| 30 | - * So only read the register if we set KVM_ARM_VCPU_SVE above. | ||
| 31 | - */ | ||
| 32 | - err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64zfr0, | ||
| 33 | - ARM64_SYS_REG(3, 0, 0, 4, 4)); | ||
| 34 | + if (sve_supported) { | ||
| 35 | + /* | ||
| 36 | + * There is a range of kernels between kernel commit 73433762fcae | ||
| 37 | + * and f81cb2c3ad41 which have a bug where the kernel doesn't | ||
| 38 | + * expose SYS_ID_AA64ZFR0_EL1 via the ONE_REG API unless the VM has | ||
| 39 | + * enabled SVE support, which resulted in an error rather than RAZ. | ||
| 40 | + * So only read the register if we set KVM_ARM_VCPU_SVE above. | ||
| 41 | + */ | ||
| 42 | + err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64zfr0, | ||
| 43 | + ARM64_SYS_REG(3, 0, 0, 4, 4)); | ||
| 44 | + } | ||
| 35 | } | 45 | } |
| 36 | 46 | ||
| 37 | - if (vms->virt && kvm_enabled()) { | 47 | kvm_arm_destroy_scratch_host_vcpu(fdarray); |
| 38 | - error_report("mach-virt: KVM does not support providing " | ||
| 39 | - "Virtualization extensions to the guest CPU"); | ||
| 40 | + if (vms->virt && (kvm_enabled() || hvf_enabled())) { | ||
| 41 | + error_report("mach-virt: %s does not support providing " | ||
| 42 | + "Virtualization extensions to the guest CPU", | ||
| 43 | + kvm_enabled() ? "KVM" : "HVF"); | ||
| 44 | exit(1); | ||
| 45 | } | ||
| 46 | |||
| 47 | - if (vms->mte && kvm_enabled()) { | ||
| 48 | - error_report("mach-virt: KVM does not support providing " | ||
| 49 | - "MTE to the guest CPU"); | ||
| 50 | + if (vms->mte && (kvm_enabled() || hvf_enabled())) { | ||
| 51 | + error_report("mach-virt: %s does not support providing " | ||
| 52 | + "MTE to the guest CPU", | ||
| 53 | + kvm_enabled() ? "KVM" : "HVF"); | ||
| 54 | exit(1); | ||
| 55 | } | ||
| 56 | |||
| 57 | -- | 48 | -- |
| 58 | 2.25.1 | 49 | 2.25.1 |
| 59 | |||
| 60 | diff view generated by jsdifflib |
Deleted patch | |||
|---|---|---|---|
| 1 | From: Shashi Mallela <shashi.mallela@linaro.org> | ||
| 2 | 1 | ||
| 3 | When Enabled bit is cleared in GITS_CTLR,ITS feature continues | ||
| 4 | to be enabled.This patch fixes the issue. | ||
| 5 | |||
| 6 | Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> | ||
| 7 | Tested-by: Alex Bennée <alex.bennee@linaro.org> | ||
| 8 | Reviewed-by: Peter Maydell <peter.maydell@linaro.org> | ||
| 9 | Message-id: 20211124182246.67691-1-shashi.mallela@linaro.org | ||
| 10 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
| 11 | --- | ||
| 12 | hw/intc/arm_gicv3_its.c | 7 ++++--- | ||
| 13 | 1 file changed, 4 insertions(+), 3 deletions(-) | ||
| 14 | |||
| 15 | diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c | ||
| 16 | index XXXXXXX..XXXXXXX 100644 | ||
| 17 | --- a/hw/intc/arm_gicv3_its.c | ||
| 18 | +++ b/hw/intc/arm_gicv3_its.c | ||
| 19 | @@ -XXX,XX +XXX,XX @@ static bool its_writel(GICv3ITSState *s, hwaddr offset, | ||
| 20 | |||
| 21 | switch (offset) { | ||
| 22 | case GITS_CTLR: | ||
| 23 | - s->ctlr |= (value & ~(s->ctlr)); | ||
| 24 | - | ||
| 25 | - if (s->ctlr & ITS_CTLR_ENABLED) { | ||
| 26 | + if (value & R_GITS_CTLR_ENABLED_MASK) { | ||
| 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: | ||
| 37 | -- | ||
| 38 | 2.25.1 | ||
| 39 | |||
| 40 | diff view generated by jsdifflib |
Deleted patch | |||
|---|---|---|---|
| 1 | The logic of gicv3_redist_update() is as follows: | ||
| 2 | * it must be called in any code path that changes the state of | ||
| 3 | (only) redistributor interrupts | ||
| 4 | * if it finds a redistributor interrupt that is (now) higher | ||
| 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 | 1 | ||
| 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.) | ||
| 54 | |||
| 55 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
| 56 | Tested-by: Alex Bennée <alex.bennee@linaro.org> | ||
| 57 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
| 58 | Message-id: 20211124202005.989935-1-peter.maydell@linaro.org | ||
| 59 | --- | ||
| 60 | hw/intc/gicv3_internal.h | 17 +++++++++++++++++ | ||
| 61 | hw/intc/arm_gicv3.c | 6 ++++-- | ||
| 62 | hw/intc/arm_gicv3_redist.c | 14 ++++++++++---- | ||
| 63 | 3 files changed, 31 insertions(+), 6 deletions(-) | ||
| 64 | |||
| 65 | diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h | ||
| 66 | index XXXXXXX..XXXXXXX 100644 | ||
| 67 | --- a/hw/intc/gicv3_internal.h | ||
| 68 | +++ b/hw/intc/gicv3_internal.h | ||
| 69 | @@ -XXX,XX +XXX,XX @@ void gicv3_dist_set_irq(GICv3State *s, int irq, int level); | ||
| 70 | void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level); | ||
| 71 | void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level); | ||
| 72 | void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level); | ||
| 73 | +/** | ||
| 74 | + * gicv3_redist_update_lpi: | ||
| 75 | + * @cs: GICv3CPUState | ||
| 76 | + * | ||
| 77 | + * Scan the LPI pending table and recalculate the highest priority | ||
| 78 | + * pending LPI and also the overall highest priority pending interrupt. | ||
| 79 | + */ | ||
| 80 | void gicv3_redist_update_lpi(GICv3CPUState *cs); | ||
| 81 | +/** | ||
| 82 | + * gicv3_redist_update_lpi_only: | ||
| 83 | + * @cs: GICv3CPUState | ||
| 84 | + * | ||
| 85 | + * Scan the LPI pending table and recalculate cs->hpplpi only, | ||
| 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); | ||
| 168 | - | ||
| 169 | - gicv3_redist_update(cs); | ||
| 170 | } | ||
| 171 | |||
| 172 | void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level) | ||
| 173 | -- | ||
| 174 | 2.25.1 | ||
| 175 | |||
| 176 | diff view generated by jsdifflib |