Re: [Qemu-devel] [PATCH v3 16/16] target-arm: Add support for VIRQ and VFIQ

[Peter Maydell]

On 17 June 2014 09:45, Edgar E. Iglesias <address@hidden> wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> Acked-by: Greg Bellows <address@hidden>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>  cpu-exec.c              | 12 ++++++++++++
>  target-arm/cpu.c        | 20 ++++++++++++++++++--
>  target-arm/cpu.h        | 24 ++++++++++++++++++++++--
>  target-arm/helper-a64.c |  2 ++
>  target-arm/helper.c     |  4 ++++
>  target-arm/internals.h  |  2 ++
>  6 files changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index a579ffc..baf5643 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -498,6 +498,18 @@ int cpu_exec(CPUArchState *env)
>                          cc->do_interrupt(cpu);
>                          next_tb = 0;
>                      }
> +                    if (interrupt_request & CPU_INTERRUPT_VIRQ
> +                        && arm_excp_unmasked(cpu, EXCP_VIRQ)) {
> +                        cpu->exception_index = EXCP_VIRQ;
> +                        cc->do_interrupt(cpu);
> +                        next_tb = 0;
> +                    }
> +                    if (interrupt_request & CPU_INTERRUPT_VFIQ
> +                        && arm_excp_unmasked(cpu, EXCP_VFIQ)) {
> +                        cpu->exception_index = EXCP_VFIQ;
> +                        cc->do_interrupt(cpu);
> +                        next_tb = 0;
> +                    }
>  #elif defined(TARGET_UNICORE32)
>                      if (interrupt_request & CPU_INTERRUPT_HARD
>                          && !(env->uncached_asr & ASR_I)) {
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 8e64c5a..cd7a5df 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -195,6 +195,20 @@ static void arm_cpu_set_irq(void *opaque, int irq, int 
> level)
>              cpu_reset_interrupt(cs, CPU_INTERRUPT_FIQ);
>          }
>          break;
> +    case ARM_CPU_VIRQ:
> +        if (level) {
> +            cpu_interrupt(cs, CPU_INTERRUPT_VIRQ);
> +        } else {
> +            cpu_reset_interrupt(cs, CPU_INTERRUPT_VIRQ);
> +        }
> +        break;
> +    case ARM_CPU_VFIQ:
> +        if (level) {
> +            cpu_interrupt(cs, CPU_INTERRUPT_VFIQ);
> +        } else {
> +            cpu_reset_interrupt(cs, CPU_INTERRUPT_VFIQ);
> +        }
> +        break;

With four cases this kind of wants a lookup of irq to CPU_INTERRUPT_
value I think, rather than the code duplication.

>      default:
>          hw_error("arm_cpu_set_irq: Bad interrupt line %d\n", irq);
>      }
> @@ -242,9 +256,11 @@ static void arm_cpu_initfn(Object *obj)
>  #ifndef CONFIG_USER_ONLY
>      /* Our inbound IRQ and FIQ lines */
>      if (kvm_enabled()) {
> -        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 2);
> +        /* VIRQ and VFIQ are unused with KVM but we add them to maintain
> +           the same interface as non-KVM CPUs.  */

    /* Multiline comments
     * should look like this.
     */

> +        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 4);
>      } else {
> -        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 2);
> +        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 4);
>      }
>
>      cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index bb123bd..df61bbd 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -53,6 +53,8 @@
>  #define EXCP_STREX          10
>  #define EXCP_HVC            11   /* HyperVisor Call */
>  #define EXCP_SMC            12   /* Secure Monitor Call */
> +#define EXCP_VIRQ           13
> +#define EXCP_VFIQ           14
>
>  #define ARMV7M_EXCP_RESET   1
>  #define ARMV7M_EXCP_NMI     2
> @@ -67,6 +69,8 @@
>
>  /* ARM-specific interrupt pending bits.  */
>  #define CPU_INTERRUPT_FIQ   CPU_INTERRUPT_TGT_EXT_1
> +#define CPU_INTERRUPT_VIRQ  CPU_INTERRUPT_TGT_EXT_2
> +#define CPU_INTERRUPT_VFIQ  CPU_INTERRUPT_TGT_EXT_3
>
>  /* The usual mapping for an AArch64 system register to its AArch32
>   * counterpart is for the 32 bit world to have access to the lower
> @@ -85,6 +89,9 @@
>  /* Meanings of the ARMCPU object's two inbound GPIO lines */

You forgot to update this comment ^^^

>  #define ARM_CPU_IRQ 0
>  #define ARM_CPU_FIQ 1
> +#define ARM_CPU_VIRQ 2
> +#define ARM_CPU_VFIQ 3

>
>  typedef void ARMWriteCPFunc(void *opaque, int cp_info,
>                              int srcreg, int operand, uint32_t value);
> @@ -1142,6 +1149,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx)
>       * EL2 if we are in NS EL0/1.
>       */
>      bool irq_can_hyp = !secure && cur_el < 2 && target_el == 2;
> +    bool irq_unmasked = ((IS_M(env) && env->regs[15] < 0xfffffff0)
> +                          || !(env->daif & PSTATE_I));

The M profile stuff is starting to look increasingly weird here.

>
>      /* Don't take exceptions if they target a lower EL.  */
>      if (cur_el > target_el) {
> @@ -1158,8 +1167,19 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx)
>          if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_IMO)) {
>              return true;
>          }
> -        return ((IS_M(env) && env->regs[15] < 0xfffffff0)
> -                            || !(env->daif & PSTATE_I));
> +        return irq_unmasked;
> +    case EXCP_VFIQ:
> +        if (!secure && !(env->cp15.hcr_el2 & HCR_FMO)) {
> +            /* VFIQs are only taken when hypervized and non-secure.  */
> +            return false;
> +        }
> +        return !(env->daif & PSTATE_F);
> +    case EXCP_VIRQ:
> +        if (!secure && !(env->cp15.hcr_el2 & HCR_IMO)) {
> +            /* VIRQs are only taken when hypervized and non-secure.  */
> +            return false;
> +        }
> +        return irq_unmasked;
>      default:
>          g_assert_not_reached();
>          break;
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index f0f33af..d7522b6 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -480,9 +480,11 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>          env->cp15.esr_el[new_el] = env->exception.syndrome;
>          break;
>      case EXCP_IRQ:
> +    case EXCP_VIRQ:
>          addr += 0x80;
>          break;
>      case EXCP_FIQ:
> +    case EXCP_VFIQ:
>          addr += 0x100;
>          break;
>      default:
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3afcbb2..182306c 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3325,6 +3325,10 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned 
> int excp_idx)
>              }
>              break;
>          }
> +    case EXCP_VIRQ:
> +    case EXCP_VFIQ:
> +        target_el = 1;
> +        break;
>      }
>      return target_el;
>  }
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index f6b7156..a57aa7d 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -55,6 +55,8 @@ static const char * const excnames[] = {
>      [EXCP_STREX] = "QEMU intercept of STREX",
>      [EXCP_HVC] = "Hypervisor Call",
>      [EXCP_SMC] = "Secure Monitor Call",
> +    [EXCP_VIRQ] = "Virtual IRQ",
> +    [EXCP_VFIQ] = "Virtual FIQ",
>  };
>
>  static inline void arm_log_exception(int idx)
> --
> 1.8.3.2

thanks
-- PMM