| Message ID | 20181109134731.11605-4-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix handling of HCR.VI and VF | expand |
On Fri, Nov 9, 2018 at 2:48 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > In commit 8a0fc3a29fc2315325400 we tried to implement HCR_EL2.{VI,VF}, > but we got it wrong and had to revert it. > > In that commit we implemented them as simply tracking whether there > is a pending virtual IRQ or virtual FIQ. This is not correct -- these > bits cause a software-generated VIRQ/VFIQ, which is distinct from > whether there is a hardware-generated VIRQ/VFIQ caused by the > external interrupt controller. So we need to track separately > the HCR_EL2 bit state and the external virq/vfiq line state, and > OR the two together to get the actual pending VIRQ/VFIQ state. > > Fixes: 8a0fc3a29fc2315325400c738f807d0d4ae0ab7f > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> On Fri, Nov 9, 2018 at 2:49 PM Peter Maydell <peter.maydell@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.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/arm/internals.h | 18 ++++++++++++++++ > target/arm/cpu.c | 48 +++++++++++++++++++++++++++++++++++++++++- > target/arm/helper.c | 20 ++++++++++++++++-- > 3 files changed, 83 insertions(+), 3 deletions(-) > > diff --git a/target/arm/internals.h b/target/arm/internals.h > index 6c2bb2deebd..a32d359dd03 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -871,4 +871,22 @@ static inline const char *aarch32_mode_name(uint32_t psr) > return cpu_mode_names[psr & 0xf]; > } > > +/** > + * arm_cpu_update_virq: Update CPU_INTERRUPT_VIRQ bit in cs->interrupt_request > + * > + * Update the CPU_INTERRUPT_VIRQ bit in cs->interrupt_request, following > + * a change to either the input VIRQ line from the GIC or the HCR_EL2.VI bit. > + * Must be called with the iothread lock held. > + */ > +void arm_cpu_update_virq(ARMCPU *cpu); > + > +/** > + * arm_cpu_update_vfiq: Update CPU_INTERRUPT_VFIQ bit in cs->interrupt_request > + * > + * Update the CPU_INTERRUPT_VFIQ bit in cs->interrupt_request, following > + * a change to either the input VFIQ line from the GIC or the HCR_EL2.VF bit. > + * Must be called with the iothread lock held. > + */ > +void arm_cpu_update_vfiq(ARMCPU *cpu); > + > #endif > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 45c16ae90ba..6fbea4dc88c 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -436,6 +436,48 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > } > #endif > > +void arm_cpu_update_virq(ARMCPU *cpu) > +{ > + /* > + * Update the interrupt level for VIRQ, which is the logical OR of > + * the HCR_EL2.VI bit and the input line level from the GIC. > + */ > + CPUARMState *env = &cpu->env; > + CPUState *cs = CPU(cpu); > + > + bool new_state = (env->cp15.hcr_el2 & HCR_VI) || > + (env->irq_line_state & CPU_INTERRUPT_VIRQ); > + > + if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VIRQ) != 0)) { > + if (new_state) { > + cpu_interrupt(cs, CPU_INTERRUPT_VIRQ); > + } else { > + cpu_reset_interrupt(cs, CPU_INTERRUPT_VIRQ); > + } > + } > +} > + > +void arm_cpu_update_vfiq(ARMCPU *cpu) > +{ > + /* > + * Update the interrupt level for VFIQ, which is the logical OR of > + * the HCR_EL2.VF bit and the input line level from the GIC. > + */ > + CPUARMState *env = &cpu->env; > + CPUState *cs = CPU(cpu); > + > + bool new_state = (env->cp15.hcr_el2 & HCR_VF) || > + (env->irq_line_state & CPU_INTERRUPT_VFIQ); > + > + if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VFIQ) != 0)) { > + if (new_state) { > + cpu_interrupt(cs, CPU_INTERRUPT_VFIQ); > + } else { > + cpu_reset_interrupt(cs, CPU_INTERRUPT_VFIQ); > + } > + } > +} > + > #ifndef CONFIG_USER_ONLY > static void arm_cpu_set_irq(void *opaque, int irq, int level) > { > @@ -457,9 +499,13 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level) > > switch (irq) { > case ARM_CPU_VIRQ: > + assert(arm_feature(env, ARM_FEATURE_EL2)); > + arm_cpu_update_virq(cpu); > + break; > case ARM_CPU_VFIQ: > assert(arm_feature(env, ARM_FEATURE_EL2)); > - /* fall through */ > + arm_cpu_update_vfiq(cpu); > + break; > case ARM_CPU_IRQ: > case ARM_CPU_FIQ: > if (level) { > diff --git a/target/arm/helper.c b/target/arm/helper.c > index f3878f505b7..771e4c3d0fb 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -3958,6 +3958,21 @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) > tlb_flush(CPU(cpu)); > } > env->cp15.hcr_el2 = value; > + > + /* > + * Updates to VI and VF require us to update the status of > + * virtual interrupts, which are the logical OR of these bits > + * and the state of the input lines from the GIC. (This 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()); > + arm_cpu_update_virq(cpu); > + arm_cpu_update_vfiq(cpu); > } > > static void hcr_writehigh(CPUARMState *env, const ARMCPRegInfo *ri, > @@ -3978,11 +3993,12 @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri, > > 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 }, > { .name = "HCR", .state = ARM_CP_STATE_AA32, > - .type = ARM_CP_ALIAS, > + .type = ARM_CP_ALIAS | ARM_CP_IO, > .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0, > .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2), > .writefn = hcr_writelow }, > @@ -4222,7 +4238,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, > + .type = ARM_CP_ALIAS | ARM_CP_IO, > .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4, > .access = PL2_RW, > .fieldoffset = offsetofhigh32(CPUARMState, cp15.hcr_el2), > -- > 2.19.1 > >
diff --git a/target/arm/internals.h b/target/arm/internals.h index 6c2bb2deebd..a32d359dd03 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -871,4 +871,22 @@ static inline const char *aarch32_mode_name(uint32_t psr) return cpu_mode_names[psr & 0xf]; } +/** + * arm_cpu_update_virq: Update CPU_INTERRUPT_VIRQ bit in cs->interrupt_request + * + * Update the CPU_INTERRUPT_VIRQ bit in cs->interrupt_request, following + * a change to either the input VIRQ line from the GIC or the HCR_EL2.VI bit. + * Must be called with the iothread lock held. + */ +void arm_cpu_update_virq(ARMCPU *cpu); + +/** + * arm_cpu_update_vfiq: Update CPU_INTERRUPT_VFIQ bit in cs->interrupt_request + * + * Update the CPU_INTERRUPT_VFIQ bit in cs->interrupt_request, following + * a change to either the input VFIQ line from the GIC or the HCR_EL2.VF bit. + * Must be called with the iothread lock held. + */ +void arm_cpu_update_vfiq(ARMCPU *cpu); + #endif diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 45c16ae90ba..6fbea4dc88c 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -436,6 +436,48 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request) } #endif +void arm_cpu_update_virq(ARMCPU *cpu) +{ + /* + * Update the interrupt level for VIRQ, which is the logical OR of + * the HCR_EL2.VI bit and the input line level from the GIC. + */ + CPUARMState *env = &cpu->env; + CPUState *cs = CPU(cpu); + + bool new_state = (env->cp15.hcr_el2 & HCR_VI) || + (env->irq_line_state & CPU_INTERRUPT_VIRQ); + + if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VIRQ) != 0)) { + if (new_state) { + cpu_interrupt(cs, CPU_INTERRUPT_VIRQ); + } else { + cpu_reset_interrupt(cs, CPU_INTERRUPT_VIRQ); + } + } +} + +void arm_cpu_update_vfiq(ARMCPU *cpu) +{ + /* + * Update the interrupt level for VFIQ, which is the logical OR of + * the HCR_EL2.VF bit and the input line level from the GIC. + */ + CPUARMState *env = &cpu->env; + CPUState *cs = CPU(cpu); + + bool new_state = (env->cp15.hcr_el2 & HCR_VF) || + (env->irq_line_state & CPU_INTERRUPT_VFIQ); + + if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VFIQ) != 0)) { + if (new_state) { + cpu_interrupt(cs, CPU_INTERRUPT_VFIQ); + } else { + cpu_reset_interrupt(cs, CPU_INTERRUPT_VFIQ); + } + } +} + #ifndef CONFIG_USER_ONLY static void arm_cpu_set_irq(void *opaque, int irq, int level) { @@ -457,9 +499,13 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level) switch (irq) { case ARM_CPU_VIRQ: + assert(arm_feature(env, ARM_FEATURE_EL2)); + arm_cpu_update_virq(cpu); + break; case ARM_CPU_VFIQ: assert(arm_feature(env, ARM_FEATURE_EL2)); - /* fall through */ + arm_cpu_update_vfiq(cpu); + break; case ARM_CPU_IRQ: case ARM_CPU_FIQ: if (level) { diff --git a/target/arm/helper.c b/target/arm/helper.c index f3878f505b7..771e4c3d0fb 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -3958,6 +3958,21 @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) tlb_flush(CPU(cpu)); } env->cp15.hcr_el2 = value; + + /* + * Updates to VI and VF require us to update the status of + * virtual interrupts, which are the logical OR of these bits + * and the state of the input lines from the GIC. (This 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()); + arm_cpu_update_virq(cpu); + arm_cpu_update_vfiq(cpu); } static void hcr_writehigh(CPUARMState *env, const ARMCPRegInfo *ri, @@ -3978,11 +3993,12 @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri, 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 }, { .name = "HCR", .state = ARM_CP_STATE_AA32, - .type = ARM_CP_ALIAS, + .type = ARM_CP_ALIAS | ARM_CP_IO, .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 0, .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.hcr_el2), .writefn = hcr_writelow }, @@ -4222,7 +4238,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, + .type = ARM_CP_ALIAS | ARM_CP_IO, .cp = 15, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 4, .access = PL2_RW, .fieldoffset = offsetofhigh32(CPUARMState, cp15.hcr_el2),
In commit 8a0fc3a29fc2315325400 we tried to implement HCR_EL2.{VI,VF}, but we got it wrong and had to revert it. In that commit we implemented them as simply tracking whether there is a pending virtual IRQ or virtual FIQ. This is not correct -- these bits cause a software-generated VIRQ/VFIQ, which is distinct from whether there is a hardware-generated VIRQ/VFIQ caused by the external interrupt controller. So we need to track separately the HCR_EL2 bit state and the external virq/vfiq line state, and OR the two together to get the actual pending VIRQ/VFIQ state. Fixes: 8a0fc3a29fc2315325400c738f807d0d4ae0ab7f Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/internals.h | 18 ++++++++++++++++ target/arm/cpu.c | 48 +++++++++++++++++++++++++++++++++++++++++- target/arm/helper.c | 20 ++++++++++++++++-- 3 files changed, 83 insertions(+), 3 deletions(-) -- 2.19.1