On Fri, Nov 9, 2018 at 2:48 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> This reverts commit 8a0fc3a29fc2315325400c738f807d0d4ae0ab7f.
>
> The implementation of HCR.VI and VF in that commit is not
> correct -- they do not track the overall "is there a pending
> VIRQ or VFIQ" status, but whether there is a pending interrupt
> due to "this mechanism", ie the hypervisor having set the VI/VF
> bits. The overall pending state for VIRQ and VFIQ is effectively
> the logical OR of the inbound lines from the GIC with the
> VI and VF bits. Commit 8a0fc3a29fc231 would result in pending
> VIRQ/VFIQ possibly being lost when the hypervisor wrote to HCR.
>
> As a preliminary to implementing the HCR.VI/VF feature properly,
> revert the broken one entirely.
>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
On Fri, Nov 9, 2018 at 2:49 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> Currently we track the state of the four irq lines from the GIC
> only via the cs->interrupt_request or KVM irq state. That means
> that we assume that an interrupt is asserted if and only if the
> external line is set. This assumption is incorrect for VIRQ
> and VFIQ, because the HCR_EL2.{VI,VF} bits allow assertion
> of VIRQ and VFIQ separately from the state of the external line.
>
> To handle this, start tracking the state of the external lines
> explicitly in a CPU state struct field, as is common practice
> for devices.
>
> The complicated part of this is dealing with inbound migration
> from an older QEMU which didn't have this state. We assume in
> that case that the older QEMU did not implement the HCR_EL2.{VI,VF}
> bits as generating interrupts, and so the line state matches
> the current state in cs->interrupt_request. (This is not quite
> true between commit 8a0fc3a29fc2315325400c7 and its revert, but
> that commit is broken and never made it into any released QEMU
> version.)
>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> ---
> target/arm/helper.c | 47 ++++-----------------------------------------
> 1 file changed, 4 insertions(+), 43 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 851ea9aa977..f3878f505b7 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3931,7 +3931,6 @@ static const ARMCPRegInfo el3_no_el2_v8_cp_reginfo[] = {
> static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t
> value)
> {
> ARMCPU *cpu = arm_env_get_cpu(env);
> - CPUState *cs = ENV_GET_CPU(env);
> uint64_t valid_mask = HCR_MASK;
>
> if (arm_feature(env, ARM_FEATURE_EL3)) {
> @@ -3950,28 +3949,6 @@ static void hcr_write(CPUARMState *env, const
> ARMCPRegInfo *ri, uint64_t value)
> /* Clear RES0 bits. */
> value &= valid_mask;
>
> - /*
> - * VI and VF are kept in cs->interrupt_request. Modifying that
> - * requires that we have the iothread lock, which is done by
> - * marking the reginfo structs as ARM_CP_IO.
> - * Note that if a write to HCR pends a VIRQ or VFIQ it is never
> - * possible for it to be taken immediately, because VIRQ and
> - * VFIQ are masked unless running at EL0 or EL1, and HCR
> - * can only be written at EL2.
> - */
> - g_assert(qemu_mutex_iothread_locked());
> - if (value & HCR_VI) {
> - cs->interrupt_request |= CPU_INTERRUPT_VIRQ;
> - } else {
> - cs->interrupt_request &= ~CPU_INTERRUPT_VIRQ;
> - }
> - if (value & HCR_VF) {
> - cs->interrupt_request |= CPU_INTERRUPT_VFIQ;
> - } else {
> - cs->interrupt_request &= ~CPU_INTERRUPT_VFIQ;
> - }
> - value &= ~(HCR_VI | HCR_VF);
> -
> /* These bits change the MMU setup:
> * HCR_VM enables stage 2 translation
> * HCR_PTW forbids certain page-table setups
> @@ -3999,32 +3976,16 @@ static void hcr_writelow(CPUARMState *env, const
> ARMCPRegInfo *ri,
> hcr_write(env, NULL, value);
> }
>
> -static uint64_t hcr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> -{
> - /* The VI and VF bits live in cs->interrupt_request */
> - uint64_t ret = env->cp15.hcr_el2 & ~(HCR_VI | HCR_VF);
> - CPUState *cs = ENV_GET_CPU(env);
> -
> - if (cs->interrupt_request & CPU_INTERRUPT_VIRQ) {
> - ret |= HCR_VI;
> - }
> - if (cs->interrupt_request & CPU_INTERRUPT_VFIQ) {
> - ret |= HCR_VF;
> - }
> - return ret;
> -}
> -
> static const ARMCPRegInfo el2_cp_reginfo[] = {
> { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
> - .type = ARM_CP_IO,
> .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
> - .writefn = hcr_write, .readfn = hcr_read },
> + .writefn = hcr_write },
> { .name = "HCR", .state = ARM_CP_STATE_AA32,
> - .type = ARM_CP_ALIAS | ARM_CP_IO,
> + .type = ARM_CP_ALIAS,
> .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0,
> .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2),
> - .writefn = hcr_writelow, .readfn = hcr_read },
> + .writefn = hcr_writelow },
> { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64,
> .type = ARM_CP_ALIAS,
> .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
> @@ -4261,7 +4222,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>
> static const ARMCPRegInfo el2_v8_cp_reginfo[] = {
> { .name = "HCR2", .state = ARM_CP_STATE_AA32,
> - .type = ARM_CP_ALIAS | ARM_CP_IO,
> + .type = ARM_CP_ALIAS,
> .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4,
> .access = PL2_RW,
> .fieldoffset = offsetofhigh32(CPUARMState, cp15.hcr_el2),
> --
> 2.19.1
>
>
