[Qemu-devel] [PATCH for-v3.1 0/3] Fix handling of HCR.VI and VF

[Qemu-devel] [PATCH for-v3.1 0/3] Fix handling of HCR.VI and VF

[Peter Maydell]

This patchset reverts commit 8a0fc3a29fc2315325400, which tried
to implement HCR.VI and VF, and then reimplements it correctly.

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.

Patch 1 reverts the broken implementation.
Patch 2 adds tracking of the state of the input lines from the
GIC (most of the complexity here is from having to handle
inbound migration from older QEMU versions).
Patch 3 implements HCR.{VI,VF} correctly.

I plan to put the revert into rc1 at least, because it
fixes a regression.

thanks
-- PMM

Peter Maydell (3):
  Revert "target/arm: Implement HCR.VI and VF"
  target/arm: Track the state of our irq lines from the GIC explicitly
  target/arm: Correctly implement handling of HCR_EL2.{VI,VF}

 target/arm/cpu.h       |  3 ++
 target/arm/internals.h | 18 ++++++++++++
 target/arm/cpu.c       | 64 +++++++++++++++++++++++++++++++++++++++++-
 target/arm/helper.c    | 57 +++++++++++--------------------------
 target/arm/machine.c   | 51 +++++++++++++++++++++++++++++++++
 5 files changed, 152 insertions(+), 41 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH for-v3.1 1/3] Revert "target/arm: Implement HCR.VI and VF"

[Peter Maydell]

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.maydell@linaro.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

Re: [Qemu-devel] [Qemu-arm] [PATCH for-v3.1 1/3] Revert "target/arm: Implement HCR.VI and VF"

[Philippe Mathieu-Daudé]

On Fri, Nov 9, 2018 at 2:48 PM Peter Maydell <peter.maydell@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.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/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
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH for-v3.1 1/3] Revert "target/arm: Implement HCR.VI and VF"

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> 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.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.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),


--
Alex Bennée

[Qemu-devel] [PATCH for-v3.1 2/3] target/arm: Track the state of our irq lines from the GIC explicitly

[Peter Maydell]

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(+)

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

Re: [Qemu-devel] [Qemu-arm] [PATCH for-v3.1 2/3] target/arm: Track the state of our irq lines from the GIC explicitly

[Philippe Mathieu-Daudé]

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
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH for-v3.1 2/3] target/arm: Track the state of our irq lines from the GIC explicitly

[Alex Bennée]

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

[Qemu-devel] [PATCH for-v3.1 3/3] target/arm: Correctly implement handling of HCR_EL2.{VI, VF}

[Peter Maydell]

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(-)

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

Re: [Qemu-devel] [Qemu-arm] [PATCH for-v3.1 3/3] target/arm: Correctly implement handling of HCR_EL2.{VI, VF}

[Philippe Mathieu-Daudé]

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
>
>