diff mbox series

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

Message ID 20181109134731.11605-2-peter.maydell@linaro.org
State Superseded
Headers show
Series Fix handling of HCR.VI and VF | expand

Commit Message

Peter Maydell Nov. 9, 2018, 1:47 p.m. UTC 
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(-)

-- 
2.19.1

Comments

Philippe Mathieu-Daudé Nov. 12, 2018, 12:15 a.m. UTC | #1 
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

>

>
Alex Bennée Nov. 12, 2018, 11:54 a.m. UTC | #2 
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
diff mbox series

Patch

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