Re: [PATCH v4 1/3] target/arm: Support SError injection

[Marc Zyngier <maz@kernel.org>]

On 2020-02-18 02:04, Gavin Shan wrote:
> This supports SError injection, which will be used by "virt" board to
> simulating the behavior of NMI injection in next patch. As Peter 
> Maydell
> suggested, this adds a new interrupt (ARM_CPU_SERROR), which is 
> parallel
> to CPU_INTERRUPT_HARD. The backend depends on if kvm is enabled or not.
> kvm_vcpu_ioctl(cpu, KVM_SET_VCPU_EVENTS) is leveraged to inject SError
> or data abort to guest. When TCG is enabled, the behavior is simulated
> by injecting SError and data abort to guest.

s/and/or/ (you can't inject both at the same time).

> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  target/arm/cpu.c      | 69 +++++++++++++++++++++++++++++++++++--------
>  target/arm/cpu.h      | 20 ++++++++-----
>  target/arm/helper.c   | 12 ++++++++
>  target/arm/m_helper.c |  8 +++++
>  target/arm/machine.c  |  3 +-
>  5 files changed, 91 insertions(+), 21 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index de733aceeb..e5750080bc 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -78,7 +78,7 @@ static bool arm_cpu_has_work(CPUState *cs)
>          && cs->interrupt_request &
>          (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
>           | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ
> -         | CPU_INTERRUPT_EXITTB);
> +         | CPU_INTERRUPT_SERROR | CPU_INTERRUPT_EXITTB);
>  }
> 
>  void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn 
> *hook,
> @@ -449,6 +449,9 @@ static inline bool arm_excp_unmasked(CPUState *cs,
> unsigned int excp_idx,
>              return false;
>          }
>          return !(env->daif & PSTATE_I);
> +    case EXCP_SERROR:
> +       pstate_unmasked = !(env->daif & PSTATE_A);
> +       break;

nit: Consider keeping the physical interrupts together, as they are 
closely
related.

>      default:
>          g_assert_not_reached();
>      }
> @@ -538,6 +541,15 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
> 
>      /* The prioritization of interrupts is IMPLEMENTATION DEFINED. */
> 
> +    if (interrupt_request & CPU_INTERRUPT_SERROR) {
> +        excp_idx = EXCP_SERROR;
> +        target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, 
> secure);
> +        if (arm_excp_unmasked(cs, excp_idx, target_el,
> +                              cur_el, secure, hcr_el2)) {
> +            goto found;
> +        }
> +    }
> +
>      if (interrupt_request & CPU_INTERRUPT_FIQ) {
>          excp_idx = EXCP_FIQ;
>          target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, 
> secure);
> @@ -570,6 +582,7 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
>              goto found;
>          }
>      }
> +
>      return false;
> 
>   found:
> @@ -585,7 +598,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState
> *cs, int interrupt_request)
>      CPUClass *cc = CPU_GET_CLASS(cs);
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> -    bool ret = false;
> +    uint32_t excp_idx;
> 
>      /* ARMv7-M interrupt masking works differently than -A or -R.
>       * There is no FIQ/IRQ distinction. Instead of I and F bits
> @@ -594,13 +607,26 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState
> *cs, int interrupt_request)
>       * (which depends on state like BASEPRI, FAULTMASK and the
>       * currently active exception).
>       */
> -    if (interrupt_request & CPU_INTERRUPT_HARD
> -        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
> -        cs->exception_index = EXCP_IRQ;
> -        cc->do_interrupt(cs);
> -        ret = true;
> +    if (!armv7m_nvic_can_take_pending_exception(env->nvic)) {
> +        return false;
> +    }
> +
> +    if (interrupt_request & CPU_INTERRUPT_SERROR) {
> +        excp_idx = EXCP_SERROR;
> +        goto found;
> +    }
> +
> +    if (interrupt_request & CPU_INTERRUPT_HARD) {
> +        excp_idx = EXCP_IRQ;
> +        goto found;
>      }
> -    return ret;
> +
> +    return false;
> +
> +found:
> +    cs->exception_index = excp_idx;
> +    cc->do_interrupt(cs);
> +    return true;
>  }
>  #endif
> 
> @@ -656,7 +682,8 @@ static void arm_cpu_set_irq(void *opaque, int irq,
> int level)
>          [ARM_CPU_IRQ] = CPU_INTERRUPT_HARD,
>          [ARM_CPU_FIQ] = CPU_INTERRUPT_FIQ,
>          [ARM_CPU_VIRQ] = CPU_INTERRUPT_VIRQ,
> -        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ
> +        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ,
> +        [ARM_CPU_SERROR] = CPU_INTERRUPT_SERROR,
>      };
> 
>      if (level) {
> @@ -676,6 +703,7 @@ static void arm_cpu_set_irq(void *opaque, int irq,
> int level)
>          break;
>      case ARM_CPU_IRQ:
>      case ARM_CPU_FIQ:
> +    case ARM_CPU_SERROR:
>          if (level) {
>              cpu_interrupt(cs, mask[irq]);
>          } else {
> @@ -693,8 +721,10 @@ static void arm_cpu_kvm_set_irq(void *opaque, int
> irq, int level)
>      ARMCPU *cpu = opaque;
>      CPUARMState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
> +    struct kvm_vcpu_events events;
>      uint32_t linestate_bit;
>      int irq_id;
> +    bool inject_irq = true;
> 
>      switch (irq) {
>      case ARM_CPU_IRQ:
> @@ -705,6 +735,14 @@ static void arm_cpu_kvm_set_irq(void *opaque, int
> irq, int level)
>          irq_id = KVM_ARM_IRQ_CPU_FIQ;
>          linestate_bit = CPU_INTERRUPT_FIQ;
>          break;
> +    case ARM_CPU_SERROR:
> +        if (!kvm_has_vcpu_events()) {
> +            return;
> +        }
> +
> +        inject_irq = false;
> +        linestate_bit = CPU_INTERRUPT_SERROR;
> +        break;
>      default:
>          g_assert_not_reached();
>      }
> @@ -714,7 +752,14 @@ static void arm_cpu_kvm_set_irq(void *opaque, int
> irq, int level)
>      } else {
>          env->irq_line_state &= ~linestate_bit;
>      }
> -    kvm_arm_set_irq(cs->cpu_index, KVM_ARM_IRQ_TYPE_CPU, irq_id, 
> !!level);
> +
> +    if (inject_irq) {

You could just have (linestate_bit != CPU_INTERRUPT_SERROR) here, and 
not have
inject_irq at all.

> +        kvm_arm_set_irq(cs->cpu_index, KVM_ARM_IRQ_TYPE_CPU, irq_id, 
> !!level);
> +    } else if (level) {

Is there any case where you'd call this function with a SError and level 
== 0?
And even if it happens, you could exit early in the above switch 
statement.

> +        memset(&events, 0, sizeof(events));
> +        events.exception.serror_pending = 1;
> +        kvm_vcpu_ioctl(cs, KVM_SET_VCPU_EVENTS, &events);
> +    }
>  #endif
>  }
> 
> @@ -1064,9 +1109,9 @@ static void arm_cpu_initfn(Object *obj)
>          /* VIRQ and VFIQ are unused with KVM but we add them to 
> maintain
>           * the same interface as non-KVM CPUs.
>           */
> -        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 4);
> +        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 
> ARM_CPU_NUM_IRQ);
>      } else {
> -        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 4);
> +        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 
> ARM_CPU_NUM_IRQ);
>      }
> 
>      qdev_init_gpio_out(DEVICE(cpu), cpu->gt_timer_outputs,
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e943ffe8a9..23e9f7ee2d 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -49,6 +49,7 @@
>  #define EXCP_LAZYFP         20   /* v7M fault during lazy FP stacking 
> */
>  #define EXCP_LSERR          21   /* v8M LSERR SecureFault */
>  #define EXCP_UNALIGNED      22   /* v7M UNALIGNED UsageFault */
> +#define EXCP_SERROR         23   /* SError Interrupt */
>  /* NB: add new EXCP_ defines to the array in arm_log_exception() too 
> */
> 
>  #define ARMV7M_EXCP_RESET   1
> @@ -79,9 +80,10 @@ enum {
>  };
> 
>  /* 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
> +#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
> +#define CPU_INTERRUPT_SERROR  CPU_INTERRUPT_TGT_EXT_4
> 
>  /* The usual mapping for an AArch64 system register to its AArch32
>   * counterpart is for the 32 bit world to have access to the lower
> @@ -97,11 +99,13 @@ enum {
>  #define offsetofhigh32(S, M) (offsetof(S, M) + sizeof(uint32_t))
>  #endif
> 
> -/* Meanings of the ARMCPU object's four inbound GPIO lines */
> -#define ARM_CPU_IRQ 0
> -#define ARM_CPU_FIQ 1
> -#define ARM_CPU_VIRQ 2
> -#define ARM_CPU_VFIQ 3
> +/* ARMCPU object's inbound GPIO lines */
> +#define ARM_CPU_IRQ     0
> +#define ARM_CPU_FIQ     1
> +#define ARM_CPU_VIRQ    2
> +#define ARM_CPU_VFIQ    3
> +#define ARM_CPU_SERROR  4
> +#define ARM_CPU_NUM_IRQ 5

This probably should be turned into an enum, given that it is going to
grow again on the following patch.

> 
>  /* ARM-specific extra insn start words:
>   * 1: Conditional execution bits
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 366dbcf460..3f00af4c41 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1969,6 +1969,12 @@ static uint64_t isr_read(CPUARMState *env,
> const ARMCPRegInfo *ri)
>          }
>      }
> 
> +    if (!allow_virt || !(hcr_el2 & HCR_AMO)) {

nit: It would be nicer to write this as

        if (!(allow_virt && (hcr_el2 & HCR_AMO)))

which fits the current code better, and makes a slightly less ugly
rewrite in the following patch.

> +        if (cs->interrupt_request & CPU_INTERRUPT_SERROR) {
> +            ret |= CPSR_A;
> +        }
> +    }
> +
>      /* External aborts are not possible in QEMU so A bit is always 
> clear */

nit: this comment seems obsolete now.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...