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

[Marc Zyngier]

On Wed, 19 Feb 2020 10:09:39 +1100
Gavin Shan <address@hidden> wrote:

> Hi Marc,
> 
> On 2/19/20 3:28 AM, Marc Zyngier wrote:
> > 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).
> >   
> 
> Absolutely, will be corrected in v5, which will be hold. I hope to receive
> comments from Peter and Richard before going to do another respin :)

Sure, there is no hurry at all.

> 
> >>
> >> Signed-off-by: Gavin Shan <address@hidden>
> >> ---
> >>  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 close  
> ly
> > related.
> >   
> 
> Sorry, I didn't get the point. Maybe you're suggesting something like below
> ?
> If yes, I'm not sure if it's necessary.
> 
>      pstate_unmasked = !(env->daif & (PSTATE_A | PSTATE_I));
> 
> I think PSTATE_A is enough to mask out SError according to ARMv8 architectu
> re
> reference manual (D1.7), as below:
> 
>     A, I, F Asynchronous exception mask bits:
>     A
>        SError interrupt mask bit.
>     I
>        IRQ interrupt mask bit.
>     F
>        FIQ interrupt mask bit.

No, all I'm suggesting is that you keep the cases for IRQ, FIQ and
SError close together in the switch statement, instead of placing
SError after the virtual interrupts. Given that they use the same code
pattern, it makes sense to order them this way. But as I said, this is
a nit, and not something that affects the outcome of this code.

> >>      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 DEFIN  
> ED. */
> >>
> >> +    if (interrupt_request & CPU_INTERRUPT_SERROR) {
> >> +        excp_idx = EXCP_SERROR;
> >> +        target_el = arm_phys_excp_target_el(cs, excp_id  
> x, 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 bi  
> ts
> >> @@ -594,13 +607,26 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState
> >> *cs, int interrupt_request)
> >>       * (which depends on state like BASEPRI, FAULTMASK and th  
> e
> >>       * currently active exception).
> >>       */
> >> -    if (interrupt_request & CPU_INTERRUPT_HARD
> >> -        && (armv7m_nvic_can_take_pending_exception(env->n  
> vic))) {
> >> -        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 n  
> ot have
> > inject_irq at all.
> >   
> 
> Sure, will be improved in v5.
> 
> >> +        kvm_arm_set_irq(cs->cpu_index, KVM_ARM_IRQ_TYPE_C  
> PU, 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 statemen  
> t.
> >   
> 
> The combination of SError and level == 0 isn't existing for now,
> meaning the SError is always coming with level == 1. We can't exit
> early in above s witch statement because env->irq_line_state needs to
> be updated accordingly.

I'm not sure level==0 makes much sense. A common implementation of
SError is as an edge interrupt (it is consumed by being handled, and
you can't "retire" it). I'm not familiar enough with QEMU's interrupt
delivery mechanism, but I'd expect SError to differ significantly from
IRQ/FIQ in that respect.

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