| Message ID | 20181109134731.11605-3-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix handling of HCR.VI and VF | expand |
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/cpu.h | 3 +++ > target/arm/cpu.c | 16 ++++++++++++++ > target/arm/machine.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 70 insertions(+) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index b5eff79f73b..f1913cdad26 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -538,6 +538,9 @@ typedef struct CPUARMState { > uint64_t esr; > } serror; > > + /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ > + uint32_t irq_line_state; > + > /* Thumb-2 EE state. */ > uint32_t teecr; > uint32_t teehbr; > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 784a4c2dfcc..45c16ae90ba 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -449,6 +449,12 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level) > [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ > }; > > + if (level) { > + env->irq_line_state |= mask[irq]; > + } else { > + env->irq_line_state &= ~mask[irq]; > + } > + > switch (irq) { > case ARM_CPU_VIRQ: > case ARM_CPU_VFIQ: > @@ -473,17 +479,27 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level) > ARMCPU *cpu = opaque; > CPUState *cs = CPU(cpu); > int kvm_irq = KVM_ARM_IRQ_TYPE_CPU << KVM_ARM_IRQ_TYPE_SHIFT; > + uint32_t linestate_bit; > > switch (irq) { > case ARM_CPU_IRQ: > kvm_irq |= KVM_ARM_IRQ_CPU_IRQ; > + linestate_bit = CPU_INTERRUPT_HARD; > break; > case ARM_CPU_FIQ: > kvm_irq |= KVM_ARM_IRQ_CPU_FIQ; > + linestate_bit = CPU_INTERRUPT_FIQ; > break; > default: > g_assert_not_reached(); > } > + > + if (level) { > + env->irq_line_state |= linestate_bit; > + } else { > + env->irq_line_state &= ~linestate_bit; > + } > + > kvm_irq |= cs->cpu_index << KVM_ARM_IRQ_VCPU_SHIFT; > kvm_set_irq(kvm_state, kvm_irq, level ? 1 : 0); > #endif > diff --git a/target/arm/machine.c b/target/arm/machine.c > index 239fe4e84d1..2033816a64e 100644 > --- a/target/arm/machine.c > +++ b/target/arm/machine.c > @@ -192,6 +192,22 @@ static const VMStateDescription vmstate_serror = { > } > }; > > +static bool irq_line_state_needed(void *opaque) > +{ > + return true; > +} > + > +static const VMStateDescription vmstate_irq_line_state = { > + .name = "cpu/irq-line-state", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = irq_line_state_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(env.irq_line_state, ARMCPU), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static bool m_needed(void *opaque) > { > ARMCPU *cpu = opaque; > @@ -625,11 +641,44 @@ static int cpu_pre_save(void *opaque) > return 0; > } > > +static int cpu_pre_load(void *opaque) > +{ > + ARMCPU *cpu = opaque; > + CPUARMState *env = &cpu->env; > + > + /* > + * Pre-initialize irq_line_state to a value that's never valid as > + * real data, so cpu_post_load() can tell whether we've seen the > + * irq-line-state subsection in the incoming migration state. > + */ > + env->irq_line_state = UINT32_MAX; > + > + return 0; > +} > + > static int cpu_post_load(void *opaque, int version_id) > { > ARMCPU *cpu = opaque; > + CPUARMState *env = &cpu->env; > int i, v; > > + /* > + * Handle migration compatibility from old QEMU which didn't > + * send the irq-line-state subsection. A QEMU without it did not > + * implement the HCR_EL2.{VI,VF} bits as generating interrupts, > + * so for TCG the line state matches the bits set in cs->interrupt_request. > + * For KVM the line state is not stored in cs->interrupt_request > + * and so this will leave irq_line_state as 0, but this is OK because > + * we only need to care about it for TCG. > + */ > + if (env->irq_line_state == UINT32_MAX) { > + CPUState *cs = CPU(cpu); > + > + env->irq_line_state = cs->interrupt_request & > + (CPU_INTERRUPT_HARD | CPU_INTERRUPT_FIQ | > + CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VFIQ); > + } > + > /* Update the values list from the incoming migration data. > * Anything in the incoming data which we don't know about is > * a migration failure; anything we know about but the incoming > @@ -680,6 +729,7 @@ const VMStateDescription vmstate_arm_cpu = { > .version_id = 22, > .minimum_version_id = 22, > .pre_save = cpu_pre_save, > + .pre_load = cpu_pre_load, > .post_load = cpu_post_load, > .fields = (VMStateField[]) { > VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16), > @@ -747,6 +797,7 @@ const VMStateDescription vmstate_arm_cpu = { > &vmstate_sve, > #endif > &vmstate_serror, > + &vmstate_irq_line_state, > NULL > } > }; > -- > 2.19.1 > >
Peter Maydell <peter.maydell@linaro.org> writes: > 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: Alex Bennée <alex.bennee@linaro.org> > --- > target/arm/cpu.h | 3 +++ > target/arm/cpu.c | 16 ++++++++++++++ > target/arm/machine.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 70 insertions(+) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index b5eff79f73b..f1913cdad26 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -538,6 +538,9 @@ typedef struct CPUARMState { > uint64_t esr; > } serror; > > + /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ > + uint32_t irq_line_state; > + > /* Thumb-2 EE state. */ > uint32_t teecr; > uint32_t teehbr; > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 784a4c2dfcc..45c16ae90ba 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -449,6 +449,12 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level) > [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ > }; > > + if (level) { > + env->irq_line_state |= mask[irq]; > + } else { > + env->irq_line_state &= ~mask[irq]; > + } > + > switch (irq) { > case ARM_CPU_VIRQ: > case ARM_CPU_VFIQ: > @@ -473,17 +479,27 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level) > ARMCPU *cpu = opaque; > CPUState *cs = CPU(cpu); > int kvm_irq = KVM_ARM_IRQ_TYPE_CPU << KVM_ARM_IRQ_TYPE_SHIFT; > + uint32_t linestate_bit; > > switch (irq) { > case ARM_CPU_IRQ: > kvm_irq |= KVM_ARM_IRQ_CPU_IRQ; > + linestate_bit = CPU_INTERRUPT_HARD; > break; > case ARM_CPU_FIQ: > kvm_irq |= KVM_ARM_IRQ_CPU_FIQ; > + linestate_bit = CPU_INTERRUPT_FIQ; > break; > default: > g_assert_not_reached(); > } > + > + if (level) { > + env->irq_line_state |= linestate_bit; > + } else { > + env->irq_line_state &= ~linestate_bit; > + } > + > kvm_irq |= cs->cpu_index << KVM_ARM_IRQ_VCPU_SHIFT; > kvm_set_irq(kvm_state, kvm_irq, level ? 1 : 0); > #endif > diff --git a/target/arm/machine.c b/target/arm/machine.c > index 239fe4e84d1..2033816a64e 100644 > --- a/target/arm/machine.c > +++ b/target/arm/machine.c > @@ -192,6 +192,22 @@ static const VMStateDescription vmstate_serror = { > } > }; > > +static bool irq_line_state_needed(void *opaque) > +{ > + return true; > +} > + > +static const VMStateDescription vmstate_irq_line_state = { > + .name = "cpu/irq-line-state", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = irq_line_state_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(env.irq_line_state, ARMCPU), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static bool m_needed(void *opaque) > { > ARMCPU *cpu = opaque; > @@ -625,11 +641,44 @@ static int cpu_pre_save(void *opaque) > return 0; > } > > +static int cpu_pre_load(void *opaque) > +{ > + ARMCPU *cpu = opaque; > + CPUARMState *env = &cpu->env; > + > + /* > + * Pre-initialize irq_line_state to a value that's never valid as > + * real data, so cpu_post_load() can tell whether we've seen the > + * irq-line-state subsection in the incoming migration state. > + */ > + env->irq_line_state = UINT32_MAX; > + > + return 0; > +} > + > static int cpu_post_load(void *opaque, int version_id) > { > ARMCPU *cpu = opaque; > + CPUARMState *env = &cpu->env; > int i, v; > > + /* > + * Handle migration compatibility from old QEMU which didn't > + * send the irq-line-state subsection. A QEMU without it did not > + * implement the HCR_EL2.{VI,VF} bits as generating interrupts, > + * so for TCG the line state matches the bits set in cs->interrupt_request. > + * For KVM the line state is not stored in cs->interrupt_request > + * and so this will leave irq_line_state as 0, but this is OK because > + * we only need to care about it for TCG. > + */ > + if (env->irq_line_state == UINT32_MAX) { > + CPUState *cs = CPU(cpu); > + > + env->irq_line_state = cs->interrupt_request & > + (CPU_INTERRUPT_HARD | CPU_INTERRUPT_FIQ | > + CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VFIQ); > + } > + > /* Update the values list from the incoming migration data. > * Anything in the incoming data which we don't know about is > * a migration failure; anything we know about but the incoming > @@ -680,6 +729,7 @@ const VMStateDescription vmstate_arm_cpu = { > .version_id = 22, > .minimum_version_id = 22, > .pre_save = cpu_pre_save, > + .pre_load = cpu_pre_load, > .post_load = cpu_post_load, > .fields = (VMStateField[]) { > VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16), > @@ -747,6 +797,7 @@ const VMStateDescription vmstate_arm_cpu = { > &vmstate_sve, > #endif > &vmstate_serror, > + &vmstate_irq_line_state, > NULL > } > }; -- Alex Bennée
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index b5eff79f73b..f1913cdad26 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -538,6 +538,9 @@ typedef struct CPUARMState { uint64_t esr; } serror; + /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */ + uint32_t irq_line_state; + /* Thumb-2 EE state. */ uint32_t teecr; uint32_t teehbr; diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 784a4c2dfcc..45c16ae90ba 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -449,6 +449,12 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level) [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ }; + if (level) { + env->irq_line_state |= mask[irq]; + } else { + env->irq_line_state &= ~mask[irq]; + } + switch (irq) { case ARM_CPU_VIRQ: case ARM_CPU_VFIQ: @@ -473,17 +479,27 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level) ARMCPU *cpu = opaque; CPUState *cs = CPU(cpu); int kvm_irq = KVM_ARM_IRQ_TYPE_CPU << KVM_ARM_IRQ_TYPE_SHIFT; + uint32_t linestate_bit; switch (irq) { case ARM_CPU_IRQ: kvm_irq |= KVM_ARM_IRQ_CPU_IRQ; + linestate_bit = CPU_INTERRUPT_HARD; break; case ARM_CPU_FIQ: kvm_irq |= KVM_ARM_IRQ_CPU_FIQ; + linestate_bit = CPU_INTERRUPT_FIQ; break; default: g_assert_not_reached(); } + + if (level) { + env->irq_line_state |= linestate_bit; + } else { + env->irq_line_state &= ~linestate_bit; + } + kvm_irq |= cs->cpu_index << KVM_ARM_IRQ_VCPU_SHIFT; kvm_set_irq(kvm_state, kvm_irq, level ? 1 : 0); #endif diff --git a/target/arm/machine.c b/target/arm/machine.c index 239fe4e84d1..2033816a64e 100644 --- a/target/arm/machine.c +++ b/target/arm/machine.c @@ -192,6 +192,22 @@ static const VMStateDescription vmstate_serror = { } }; +static bool irq_line_state_needed(void *opaque) +{ + return true; +} + +static const VMStateDescription vmstate_irq_line_state = { + .name = "cpu/irq-line-state", + .version_id = 1, + .minimum_version_id = 1, + .needed = irq_line_state_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT32(env.irq_line_state, ARMCPU), + VMSTATE_END_OF_LIST() + } +}; + static bool m_needed(void *opaque) { ARMCPU *cpu = opaque; @@ -625,11 +641,44 @@ static int cpu_pre_save(void *opaque) return 0; } +static int cpu_pre_load(void *opaque) +{ + ARMCPU *cpu = opaque; + CPUARMState *env = &cpu->env; + + /* + * Pre-initialize irq_line_state to a value that's never valid as + * real data, so cpu_post_load() can tell whether we've seen the + * irq-line-state subsection in the incoming migration state. + */ + env->irq_line_state = UINT32_MAX; + + return 0; +} + static int cpu_post_load(void *opaque, int version_id) { ARMCPU *cpu = opaque; + CPUARMState *env = &cpu->env; int i, v; + /* + * Handle migration compatibility from old QEMU which didn't + * send the irq-line-state subsection. A QEMU without it did not + * implement the HCR_EL2.{VI,VF} bits as generating interrupts, + * so for TCG the line state matches the bits set in cs->interrupt_request. + * For KVM the line state is not stored in cs->interrupt_request + * and so this will leave irq_line_state as 0, but this is OK because + * we only need to care about it for TCG. + */ + if (env->irq_line_state == UINT32_MAX) { + CPUState *cs = CPU(cpu); + + env->irq_line_state = cs->interrupt_request & + (CPU_INTERRUPT_HARD | CPU_INTERRUPT_FIQ | + CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VFIQ); + } + /* Update the values list from the incoming migration data. * Anything in the incoming data which we don't know about is * a migration failure; anything we know about but the incoming @@ -680,6 +729,7 @@ const VMStateDescription vmstate_arm_cpu = { .version_id = 22, .minimum_version_id = 22, .pre_save = cpu_pre_save, + .pre_load = cpu_pre_load, .post_load = cpu_post_load, .fields = (VMStateField[]) { VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16), @@ -747,6 +797,7 @@ const VMStateDescription vmstate_arm_cpu = { &vmstate_sve, #endif &vmstate_serror, + &vmstate_irq_line_state, NULL } };
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> --- target/arm/cpu.h | 3 +++ target/arm/cpu.c | 16 ++++++++++++++ target/arm/machine.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+)