Re: [PATCH v3 1/2] target/arm: Support SError injection

[Gavin Shan <gshan@redhat.com>]

On 2/16/20 2:41 PM, Richard Henderson wrote:
> On 2/13/20 9:59 PM, Gavin Shan wrote:
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index b0762a76c4..180e29fb83 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);
>> +         | ARM_CPU_SERROR | CPU_INTERRUPT_EXITTB);
> 
> CPU_INTERRUPT_SERROR, not ARM_CPU_SERROR.
> 

Yep, will be corrected in v4.

>> @@ -570,6 +573,16 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>               goto found;
>>           }
>>       }
>> +
>> +    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;
>> +        }
>> +    }
>> +
>>       return false;
>>   
>>    found:
> 
> If you're intending to use Serror for NMI, perhaps it should be the first bit
> tested, not the last.  Otherwise some bug that leaves a normal hard interrupt
> line high will keep delivering the interrupt, and not the Serror.
> 
> As the comment at the top of the function says, the priority is implementation
> defined, so we can put it anywhere we like.
> 

Yes, SError will have highest priority in v4.

>> @@ -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_HARD) {
>> +        excp_idx = EXCP_IRQ;
>> +        goto found;
>>       }
>> -    return ret;
>> +
>> +    if (interrupt_request & CPU_INTERRUPT_SERROR) {
>> +        excp_idx = EXCP_SERROR;
>> +        goto found;
>> +    }
> 
> Likewise.
> 

Thanks, SError will have highest priority in v4.

>> -        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 4);
>> +        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 5);
>>       } else {
>> -        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 4);
>> +        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 5);
> 
> I wonder if we should have an ARM_CPU_NUM_IRQ define so that this is more
> automatic.
> 

Yes, It makes sense. ARM_CPU_NUM_IRQ will be introduced in v4.

>> @@ -98,10 +100,11 @@ enum {
>>   #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
>> +#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
> 
> Comment is now wrong about the count.
> 

Yes, It will be corrected to "ARMCPU object's inbound GPIO lines" in v4.

Thanks,
Gavin