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

["Alex Bennée" <alex.bennee@linaro.org>]

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