Re: [RFC PATCH v3 17/21] hw/intc/arm_gicv3: Add NMI handling CPU interface registers

[Jinjie Ruan]

On 2024/2/24 4:52, Richard Henderson wrote:
> On 2/23/24 00:32, Jinjie Ruan via wrote:
>> Add the NMIAR CPU interface registers which deal with acknowledging NMI.
>>
>> When introduce NMI interrupt, there are some updates to the semantics
>> for the
>> register ICC_IAR1_EL1 and ICC_HPPIR1_EL1. For ICC_IAR1_EL1 register, it
>> should return 1022 if the intid has super priority. And for
>> ICC_NMIAR1_EL1
>> register, it should return 1023 if the intid do not have super priority.
>> Howerever, these are not necessary for ICC_HPPIR1_EL1 register.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   hw/intc/arm_gicv3_cpuif.c | 46 ++++++++++++++++++++++++++++++++++++---
>>   hw/intc/gicv3_internal.h  |  1 +
>>   2 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
>> index e1a60d8c15..f5bf8df32b 100644
>> --- a/hw/intc/arm_gicv3_cpuif.c
>> +++ b/hw/intc/arm_gicv3_cpuif.c
>> @@ -1097,7 +1097,8 @@ static uint64_t icc_hppir0_value(GICv3CPUState
>> *cs, CPUARMState *env)
>>       return cs->hppi.irq;
>>   }
>>   -static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env)
>> +static uint64_t icc_hppir1_value(GICv3CPUState *cs, CPUARMState *env,
>> +                                 bool is_nmi, bool is_hppi)
>>   {
>>       /* Return the highest priority pending interrupt register value
>>        * for group 1.
>> @@ -1108,6 +1109,16 @@ static uint64_t icc_hppir1_value(GICv3CPUState
>> *cs, CPUARMState *env)
>>           return INTID_SPURIOUS;
>>       }
>>   +    if (!is_hppi) {
>> +        if (is_nmi && (!cs->hppi.superprio)) {
>> +            return INTID_SPURIOUS;
>> +        }
>> +
>> +        if ((!is_nmi) && cs->hppi.superprio) {
>> +            return INTID_NMI;
>> +        }
>> +    }
>> +
>>       /* Check whether we can return the interrupt or if we should return
>>        * a special identifier, as per the
>> CheckGroup1ForSpecialIdentifiers
>>        * pseudocode. (We can simplify a little because for us
>> ICC_SRE_EL1.RM
>> @@ -1168,7 +1179,30 @@ static uint64_t icc_iar1_read(CPUARMState *env,
>> const ARMCPRegInfo *ri)
>>       if (!icc_hppi_can_preempt(cs)) {
>>           intid = INTID_SPURIOUS;
>>       } else {
>> -        intid = icc_hppir1_value(cs, env);
>> +        intid = icc_hppir1_value(cs, env, false, false);
>> +    }
>> +
>> +    if (!gicv3_intid_is_special(intid)) {
>> +        icc_activate_irq(cs, intid);
>> +    }
>> +
>> +    trace_gicv3_icc_iar1_read(gicv3_redist_affid(cs), intid);
>> +    return intid;
>> +}
> 
> This is incorrect.  For icc_iar1_read, you need something like
> 
>     if (!is_hppi
>         && cs->hppi.superprio
>         && env->cp15.sctlr_el[current_el] & SCTLR_NMI) {
>         return INTID_NMI;
>     }
> 
> I think that if SCTLR_NMI is not set, the whole system ignores
> Superpriority entirely, so returning SPURIOUS here would be incorrect. 
> This would make sense, letting an OS that is not configured for FEAT_NMI
> to run on ARMv8.8 hardware without modification.

You are right. SCTLR_ELx.NMI decide whether IRQ and FIQ interrupts to
have Superpriority as an additional attribute.

> 
> 
>> +
>> +static uint64_t icc_nmiar1_read(CPUARMState *env, const ARMCPRegInfo
>> *ri)
>> +{
>> +    GICv3CPUState *cs = icc_cs_from_env(env);
>> +    uint64_t intid;
>> +
>> +    if (icv_access(env, HCR_IMO)) {
>> +        return icv_iar_read(env, ri);
>> +    }
>> +
>> +    if (!icc_hppi_can_preempt(cs)) {
>> +        intid = INTID_SPURIOUS;
>> +    } else {
>> +        intid = icc_hppir1_value(cs, env, true, false);
> 
> Here... believe that the result *should* only consider superpriority.  I
> guess SPURIOUS is the correct result when there is no pending interrupt
> with superpriority?  It's really unclear to me from the register
> description.
> 
> Peter?
> 
>> @@ -2344,6 +2378,12 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[]
>> = {
>>         .access = PL1_R, .accessfn = gicv3_irq_access,
>>         .readfn = icc_iar1_read,
>>       },
>> +    { .name = "ICC_NMIAR1_EL1", .state = ARM_CP_STATE_BOTH,
>> +      .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 9, .opc2 = 5,
>> +      .type = ARM_CP_IO | ARM_CP_NO_RAW,
>> +      .access = PL1_R, .accessfn = gicv3_irq_access,
>> +      .readfn = icc_nmiar1_read,
>> +    },
> 
> This register is UNDEFINED if FEAT_GICv3_NMI is not implemented.
> You need to register this separately.
> 
> 
> r~