qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-arm] [PATCH for-v3.1 1/3] Revert "target/arm: Implement HCR.VI and VF"
Date: Mon, 12 Nov 2018 01:15:29 +0100

On Fri, Nov 9, 2018 at 2:48 PM Peter Maydell <address@hidden> 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 <address@hidden>

On Fri, Nov 9, 2018 at 2:49 PM Peter Maydell <address@hidden> 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 <address@hidden>

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]