[RFC PATCH v2 00/22] target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI

[RFC PATCH v2 00/22] target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI

[Jinjie Ruan via]

This patch set implements FEAT_NMI and FEAT_GICv3_NMI for armv8. These
introduce support for a new category of interrupts in the architecture
which we can use to provide NMI like functionality.

There are two modes for using this FEAT_NMI. When PSTATE.ALLINT or
PSTATE.SP & SCTLR_ELx.SCTLR_SPINTMASK is set, any entry to ELx causes all
interrupts including those with superpriority to be masked on entry to ELn
until the mask is explicitly removed by software or hardware. PSTATE.ALLINT
can be managed by software using the new register control ALLINT.ALLINT.
Independent controls are provided for this feature at each EL, usage at EL1
should not disrupt EL2 or EL3.

I have tested it with the following linux patches which try to support
FEAT_NMI in linux kernel:

	https://lore.kernel.org/linux-arm-kernel/Y4sH5qX5bK9xfEBp@lpieralisi/T/#mb4ba4a2c045bf72c10c2202c1dd1b82d3240dc88

In the test, SGI, PPI and SPI interrupts can all be set to have super priority
to be converted to a hardware NMI interrupt. The SGI is tested with kernel
IPI as NMI framework, and the PPI interrupt is tested with "perf top" command
with hardware NMI enabled, and the PPI interrupt is tested with a custom
test module, in which NMI interrupts can be received and transmitted normally.

Changes in v2:
- Break up the patches so that each one does only one thing.
- Remove the command line option and just implement it in "max" cpu.

Jinjie Ruan (22):
  target/arm: Add FEAT_NMI to max
  target/arm: Handle HCR_EL2 accesses for bits introduced with FEAT_NMI
  target/arm: Add PSTATE.ALLINT
  target/arm: Implement ALLINT MSR (immediate)
  target/arm: Support MSR access to ALLINT
  target/arm: Add support for Non-maskable Interrupt
  target/arm: Add support for NMI event state
  target/arm: Handle IS/FS in ISR_EL1 for NMI
  target/arm: Add support for FEAT_NMI, Non-maskable Interrupt
  target/arm: Handle PSTATE.ALLINT on taking an exception
  target/arm: Set pstate.ALLINT in arm_cpu_reset_hold
  hw/arm/virt: Wire NMI irq line from GIC to CPU
  hw/intc/arm_gicv3: Add external IRQ lines for NMI
  target/arm: Handle NMI in arm_cpu_do_interrupt_aarch64()
  hw/intc/arm_gicv3_redist: Implement GICR_INMIR0
  hw/intc/arm_gicv3: Implement GICD_INMIR
  hw/intc: Enable FEAT_GICv3_NMI Feature
  hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC
  hw/intc/arm_gicv3: Add irq superpriority information
  hw/intc/arm_gicv3: Add NMI handling CPU interface registers
  hw/intc/arm_gicv3: Implement NMI interrupt prioirty
  hw/intc/arm_gicv3: Report the NMI interrupt in gicv3_cpuif_update()

 docs/system/arm/emulation.rst      |  1 +
 hw/arm/virt.c                      |  7 ++-
 hw/intc/arm_gicv3.c                | 61 ++++++++++++++++++++++---
 hw/intc/arm_gicv3_common.c         |  4 ++
 hw/intc/arm_gicv3_cpuif.c          | 53 ++++++++++++++++++++--
 hw/intc/arm_gicv3_dist.c           | 40 +++++++++++++++++
 hw/intc/arm_gicv3_redist.c         | 23 ++++++++++
 hw/intc/gicv3_internal.h           |  5 +++
 include/hw/intc/arm_gic_common.h   |  1 +
 include/hw/intc/arm_gicv3_common.h |  6 +++
 target/arm/cpu-features.h          |  5 +++
 target/arm/cpu-qom.h               |  3 +-
 target/arm/cpu.c                   | 46 ++++++++++++++++---
 target/arm/cpu.h                   | 15 ++++++-
 target/arm/helper.c                | 72 ++++++++++++++++++++++++++++++
 target/arm/internals.h             |  3 ++
 target/arm/tcg/a64.decode          |  1 +
 target/arm/tcg/cpu64.c             |  1 +
 target/arm/tcg/helper-a64.c        | 24 ++++++++++
 target/arm/tcg/helper-a64.h        |  1 +
 target/arm/tcg/translate-a64.c     | 10 +++++
 21 files changed, 362 insertions(+), 20 deletions(-)

-- 
2.34.1

[RFC PATCH v2 01/22] target/arm: Add FEAT_NMI to max

[Jinjie Ruan via]

Enable FEAT_NMI on the 'max' CPU.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 docs/system/arm/emulation.rst | 1 +
 target/arm/tcg/cpu64.c        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/docs/system/arm/emulation.rst b/docs/system/arm/emulation.rst
index f67aea2d83..91baf7ad69 100644
--- a/docs/system/arm/emulation.rst
+++ b/docs/system/arm/emulation.rst
@@ -63,6 +63,7 @@ the following architecture extensions:
 - FEAT_MTE (Memory Tagging Extension)
 - FEAT_MTE2 (Memory Tagging Extension)
 - FEAT_MTE3 (MTE Asymmetric Fault Handling)
+- FEAT_NMI (Non-maskable Interrupt)
 - FEAT_NV (Nested Virtualization)
 - FEAT_NV2 (Enhanced nested virtualization support)
 - FEAT_PACIMP (Pointer authentication - IMPLEMENTATION DEFINED algorithm)
diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
index 5fba2c0f04..60f0dcd799 100644
--- a/target/arm/tcg/cpu64.c
+++ b/target/arm/tcg/cpu64.c
@@ -1175,6 +1175,7 @@ void aarch64_max_tcg_initfn(Object *obj)
     t = FIELD_DP64(t, ID_AA64PFR1, RAS_FRAC, 0);  /* FEAT_RASv1p1 + FEAT_DoubleFault */
     t = FIELD_DP64(t, ID_AA64PFR1, SME, 1);       /* FEAT_SME */
     t = FIELD_DP64(t, ID_AA64PFR1, CSV2_FRAC, 0); /* FEAT_CSV2_2 */
+    t = FIELD_DP64(t, ID_AA64PFR1, NMI, 1);       /* FEAT_NMI */
     cpu->isar.id_aa64pfr1 = t;
 
     t = cpu->isar.id_aa64mmfr0;
-- 
2.34.1

Re: [RFC PATCH v2 01/22] target/arm: Add FEAT_NMI to max

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Enable FEAT_NMI on the 'max' CPU.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   docs/system/arm/emulation.rst | 1 +
>   target/arm/tcg/cpu64.c        | 1 +
>   2 files changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

However, this patch must be sorted last, after all support for FEAT_NMI is present.


r~

> 
> diff --git a/docs/system/arm/emulation.rst b/docs/system/arm/emulation.rst
> index f67aea2d83..91baf7ad69 100644
> --- a/docs/system/arm/emulation.rst
> +++ b/docs/system/arm/emulation.rst
> @@ -63,6 +63,7 @@ the following architecture extensions:
>   - FEAT_MTE (Memory Tagging Extension)
>   - FEAT_MTE2 (Memory Tagging Extension)
>   - FEAT_MTE3 (MTE Asymmetric Fault Handling)
> +- FEAT_NMI (Non-maskable Interrupt)
>   - FEAT_NV (Nested Virtualization)
>   - FEAT_NV2 (Enhanced nested virtualization support)
>   - FEAT_PACIMP (Pointer authentication - IMPLEMENTATION DEFINED algorithm)
> diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
> index 5fba2c0f04..60f0dcd799 100644
> --- a/target/arm/tcg/cpu64.c
> +++ b/target/arm/tcg/cpu64.c
> @@ -1175,6 +1175,7 @@ void aarch64_max_tcg_initfn(Object *obj)
>       t = FIELD_DP64(t, ID_AA64PFR1, RAS_FRAC, 0);  /* FEAT_RASv1p1 + FEAT_DoubleFault */
>       t = FIELD_DP64(t, ID_AA64PFR1, SME, 1);       /* FEAT_SME */
>       t = FIELD_DP64(t, ID_AA64PFR1, CSV2_FRAC, 0); /* FEAT_CSV2_2 */
> +    t = FIELD_DP64(t, ID_AA64PFR1, NMI, 1);       /* FEAT_NMI */
>       cpu->isar.id_aa64pfr1 = t;
>   
>       t = cpu->isar.id_aa64mmfr0;

Re: [RFC PATCH v2 01/22] target/arm: Add FEAT_NMI to max

[Jinjie Ruan via]

On 2024/2/22 5:22, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> Enable FEAT_NMI on the 'max' CPU.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   docs/system/arm/emulation.rst | 1 +
>>   target/arm/tcg/cpu64.c        | 1 +
>>   2 files changed, 2 insertions(+)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> However, this patch must be sorted last, after all support for FEAT_NMI
> is present.

Good suggestion! Placed it last is more reasonable.

> 
> 
> r~
> 
>>
>> diff --git a/docs/system/arm/emulation.rst
>> b/docs/system/arm/emulation.rst
>> index f67aea2d83..91baf7ad69 100644
>> --- a/docs/system/arm/emulation.rst
>> +++ b/docs/system/arm/emulation.rst
>> @@ -63,6 +63,7 @@ the following architecture extensions:
>>   - FEAT_MTE (Memory Tagging Extension)
>>   - FEAT_MTE2 (Memory Tagging Extension)
>>   - FEAT_MTE3 (MTE Asymmetric Fault Handling)
>> +- FEAT_NMI (Non-maskable Interrupt)
>>   - FEAT_NV (Nested Virtualization)
>>   - FEAT_NV2 (Enhanced nested virtualization support)
>>   - FEAT_PACIMP (Pointer authentication - IMPLEMENTATION DEFINED
>> algorithm)
>> diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
>> index 5fba2c0f04..60f0dcd799 100644
>> --- a/target/arm/tcg/cpu64.c
>> +++ b/target/arm/tcg/cpu64.c
>> @@ -1175,6 +1175,7 @@ void aarch64_max_tcg_initfn(Object *obj)
>>       t = FIELD_DP64(t, ID_AA64PFR1, RAS_FRAC, 0);  /* FEAT_RASv1p1 +
>> FEAT_DoubleFault */
>>       t = FIELD_DP64(t, ID_AA64PFR1, SME, 1);       /* FEAT_SME */
>>       t = FIELD_DP64(t, ID_AA64PFR1, CSV2_FRAC, 0); /* FEAT_CSV2_2 */
>> +    t = FIELD_DP64(t, ID_AA64PFR1, NMI, 1);       /* FEAT_NMI */
>>       cpu->isar.id_aa64pfr1 = t;
>>         t = cpu->isar.id_aa64mmfr0;
> 

[RFC PATCH v2 02/22] target/arm: Handle HCR_EL2 accesses for bits introduced with FEAT_NMI

[Jinjie Ruan via]

FEAT_NMI defines another new bit in HCRX_EL2: TALLINT. When the
feature is enabled, allow this bit to be written in HCRX_EL2.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/cpu-features.h | 5 +++++
 target/arm/helper.c       | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
index 7567854db6..2ad1179be7 100644
--- a/target/arm/cpu-features.h
+++ b/target/arm/cpu-features.h
@@ -681,6 +681,11 @@ static inline bool isar_feature_aa64_sme(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, SME) != 0;
 }
 
+static inline bool isar_feature_aa64_nmi(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, NMI) != 0;
+}
+
 static inline bool isar_feature_aa64_tgran4_lpa2(const ARMISARegisters *id)
 {
     return FIELD_SEX64(id->id_aa64mmfr0, ID_AA64MMFR0, TGRAN4) >= 1;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 90c4fb72ce..a3062cb2ad 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6056,6 +6056,11 @@ static void hcrx_write(CPUARMState *env, const ARMCPRegInfo *ri,
         valid_mask |= HCRX_MSCEN | HCRX_MCE2;
     }
 
+    /* FEAT_NMI adds TALLINT */
+    if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
+        valid_mask |= HCRX_TALLINT;
+    }
+
     /* Clear RES0 bits.  */
     env->cp15.hcrx_el2 = value & valid_mask;
 }
-- 
2.34.1

Re: [RFC PATCH v2 02/22] target/arm: Handle HCR_EL2 accesses for bits introduced with FEAT_NMI

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> FEAT_NMI defines another new bit in HCRX_EL2: TALLINT. When the
> feature is enabled, allow this bit to be written in HCRX_EL2.
> 
> Signed-off-by: Jinjie Ruan<ruanjinjie@huawei.com>
> ---
>   target/arm/cpu-features.h | 5 +++++
>   target/arm/helper.c       | 5 +++++
>   2 files changed, 10 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[RFC PATCH v2 03/22] target/arm: Add PSTATE.ALLINT

[Jinjie Ruan via]

The ALLINT bit in PSTATE is used to mask all IRQ or FIQ interrupts.

Place this in its own field within ENV, as that will
make it easier to reset from within TCG generated code.

With the change to pstate_read/write, exception entry
and return are automatically handled.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/cpu.c | 3 +++
 target/arm/cpu.h | 9 +++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5fa86bc8d5..5e5978c302 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1104,6 +1104,9 @@ static void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
     if (cpu_isar_feature(aa64_bti, cpu)) {
         qemu_fprintf(f, "  BTYPE=%d", (psr & PSTATE_BTYPE) >> 10);
     }
+    if (cpu_isar_feature(aa64_nmi, cpu)) {
+        qemu_fprintf(f, "  ALLINT=%d", (psr & PSTATE_ALLINT) >> 13);
+    }
     qemu_fprintf(f, "%s%s%s",
                  (hcr & HCR_NV) ? " NV" : "",
                  (hcr & HCR_NV1) ? " NV1" : "",
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 63f31e0d98..f9646dbbfb 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -224,6 +224,7 @@ typedef struct CPUArchState {
      *    semantics as for AArch32, as described in the comments on each field)
      *  nRW (also known as M[4]) is kept, inverted, in env->aarch64
      *  DAIF (exception masks) are kept in env->daif
+     *  ALLINT (all IRQ or FIQ interrupts masks) are kept in env->allint
      *  BTYPE is kept in env->btype
      *  SM and ZA are kept in env->svcr
      *  all other bits are stored in their correct places in env->pstate
@@ -261,6 +262,7 @@ typedef struct CPUArchState {
     uint32_t btype;  /* BTI branch type.  spsr[11:10].  */
     uint64_t daif; /* exception masks, in the bits they are in PSTATE */
     uint64_t svcr; /* PSTATE.{SM,ZA} in the bits they are in SVCR */
+    uint64_t allint; /* All IRQ or FIQ interrupt mask, in the bit in PSTATE */
 
     uint64_t elr_el[4]; /* AArch64 exception link regs  */
     uint64_t sp_el[4]; /* AArch64 banked stack pointers */
@@ -1543,6 +1545,7 @@ FIELD(VTCR, SL2, 33, 1)
 #define PSTATE_D (1U << 9)
 #define PSTATE_BTYPE (3U << 10)
 #define PSTATE_SSBS (1U << 12)
+#define PSTATE_ALLINT (1U << 13)
 #define PSTATE_IL (1U << 20)
 #define PSTATE_SS (1U << 21)
 #define PSTATE_PAN (1U << 22)
@@ -1555,7 +1558,8 @@ FIELD(VTCR, SL2, 33, 1)
 #define PSTATE_N (1U << 31)
 #define PSTATE_NZCV (PSTATE_N | PSTATE_Z | PSTATE_C | PSTATE_V)
 #define PSTATE_DAIF (PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F)
-#define CACHED_PSTATE_BITS (PSTATE_NZCV | PSTATE_DAIF | PSTATE_BTYPE)
+#define CACHED_PSTATE_BITS (PSTATE_NZCV | PSTATE_DAIF | PSTATE_BTYPE | \
+                            PSTATE_ALLINT)
 /* Mode values for AArch64 */
 #define PSTATE_MODE_EL3h 13
 #define PSTATE_MODE_EL3t 12
@@ -1595,7 +1599,7 @@ static inline uint32_t pstate_read(CPUARMState *env)
     ZF = (env->ZF == 0);
     return (env->NF & 0x80000000) | (ZF << 30)
         | (env->CF << 29) | ((env->VF & 0x80000000) >> 3)
-        | env->pstate | env->daif | (env->btype << 10);
+        | env->pstate | env->allint | env->daif | (env->btype << 10);
 }
 
 static inline void pstate_write(CPUARMState *env, uint32_t val)
@@ -1604,6 +1608,7 @@ static inline void pstate_write(CPUARMState *env, uint32_t val)
     env->NF = val;
     env->CF = (val >> 29) & 1;
     env->VF = (val << 3) & 0x80000000;
+    env->allint = val & PSTATE_ALLINT;
     env->daif = val & PSTATE_DAIF;
     env->btype = (val >> 10) & 3;
     env->pstate = val & ~CACHED_PSTATE_BITS;
-- 
2.34.1

Re: [RFC PATCH v2 03/22] target/arm: Add PSTATE.ALLINT

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> The ALLINT bit in PSTATE is used to mask all IRQ or FIQ interrupts.
> 
> Place this in its own field within ENV, as that will
> make it easier to reset from within TCG generated code.
> 
> With the change to pstate_read/write, exception entry
> and return are automatically handled.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/cpu.c | 3 +++
>   target/arm/cpu.h | 9 +++++++--
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5fa86bc8d5..5e5978c302 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1104,6 +1104,9 @@ static void aarch64_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>       if (cpu_isar_feature(aa64_bti, cpu)) {
>           qemu_fprintf(f, "  BTYPE=%d", (psr & PSTATE_BTYPE) >> 10);
>       }
> +    if (cpu_isar_feature(aa64_nmi, cpu)) {
> +        qemu_fprintf(f, "  ALLINT=%d", (psr & PSTATE_ALLINT) >> 13);
> +    }

This is one bit -- !!(psr & ALLINT) is better

We don't individually print DAIF either; why is this bit more special?

> @@ -224,6 +224,7 @@ typedef struct CPUArchState {
>        *    semantics as for AArch32, as described in the comments on each field)
>        *  nRW (also known as M[4]) is kept, inverted, in env->aarch64
>        *  DAIF (exception masks) are kept in env->daif
> +     *  ALLINT (all IRQ or FIQ interrupts masks) are kept in env->allint
>        *  BTYPE is kept in env->btype
>        *  SM and ZA are kept in env->svcr
>        *  all other bits are stored in their correct places in env->pstate
> @@ -261,6 +262,7 @@ typedef struct CPUArchState {
>       uint32_t btype;  /* BTI branch type.  spsr[11:10].  */
>       uint64_t daif; /* exception masks, in the bits they are in PSTATE */
>       uint64_t svcr; /* PSTATE.{SM,ZA} in the bits they are in SVCR */
> +    uint64_t allint; /* All IRQ or FIQ interrupt mask, in the bit in PSTATE */

Why is this split out from env->pstate?

The allint bit matches the documentation for SPSR_EL1, which is how env->pstate is 
documented.  The other exclusions have some performance imperative which I don't see for 
allint.


r~

Re: [RFC PATCH v2 03/22] target/arm: Add PSTATE.ALLINT

[Jinjie Ruan via]

On 2024/2/22 2:50, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> The ALLINT bit in PSTATE is used to mask all IRQ or FIQ interrupts.
>>
>> Place this in its own field within ENV, as that will
>> make it easier to reset from within TCG generated code.
>>
>> With the change to pstate_read/write, exception entry
>> and return are automatically handled.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/cpu.c | 3 +++
>>   target/arm/cpu.h | 9 +++++++--
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 5fa86bc8d5..5e5978c302 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1104,6 +1104,9 @@ static void aarch64_cpu_dump_state(CPUState *cs,
>> FILE *f, int flags)
>>       if (cpu_isar_feature(aa64_bti, cpu)) {
>>           qemu_fprintf(f, "  BTYPE=%d", (psr & PSTATE_BTYPE) >> 10);
>>       }
>> +    if (cpu_isar_feature(aa64_nmi, cpu)) {
>> +        qemu_fprintf(f, "  ALLINT=%d", (psr & PSTATE_ALLINT) >> 13);
>> +    }
> 
> This is one bit -- !!(psr & ALLINT) is better
> 
> We don't individually print DAIF either; why is this bit more special?

That makes sense, it should be removed.

> 
>> @@ -224,6 +224,7 @@ typedef struct CPUArchState {
>>        *    semantics as for AArch32, as described in the comments on
>> each field)
>>        *  nRW (also known as M[4]) is kept, inverted, in env->aarch64
>>        *  DAIF (exception masks) are kept in env->daif
>> +     *  ALLINT (all IRQ or FIQ interrupts masks) are kept in env->allint
>>        *  BTYPE is kept in env->btype
>>        *  SM and ZA are kept in env->svcr
>>        *  all other bits are stored in their correct places in
>> env->pstate
>> @@ -261,6 +262,7 @@ typedef struct CPUArchState {
>>       uint32_t btype;  /* BTI branch type.  spsr[11:10].  */
>>       uint64_t daif; /* exception masks, in the bits they are in
>> PSTATE */
>>       uint64_t svcr; /* PSTATE.{SM,ZA} in the bits they are in SVCR */
>> +    uint64_t allint; /* All IRQ or FIQ interrupt mask, in the bit in
>> PSTATE */
> 
> Why is this split out from env->pstate?
> 
> The allint bit matches the documentation for SPSR_EL1, which is how
> env->pstate is documented.  The other exclusions have some performance
> imperative which I don't see for allint.

It seems to me that allint is a bit like daif, so it is reasonable to
maintain it separately in ENV as what daif do it.

> 
> 
> r~

Re: [RFC PATCH v2 03/22] target/arm: Add PSTATE.ALLINT

[Richard Henderson]

On 2/21/24 15:48, Jinjie Ruan wrote:
>> Why is this split out from env->pstate?
>>
>> The allint bit matches the documentation for SPSR_EL1, which is how
>> env->pstate is documented.  The other exclusions have some performance
>> imperative which I don't see for allint.
> 
> It seems to me that allint is a bit like daif, so it is reasonable to
> maintain it separately in ENV as what daif do it.

daif is shared between aa64 pstate and aa32 cpsr; keeping those bits separate means we 
don't have to check the execution state in order to find them.

allint has no aa32 counterpart, so it can live in pstate always.


r~

[RFC PATCH v2 04/22] target/arm: Implement ALLINT MSR (immediate)

[Jinjie Ruan via]

Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
to ALLINT. Avoid the unconditional write to pc and use raise_exception_ra
to unwind.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/tcg/a64.decode      |  1 +
 target/arm/tcg/helper-a64.c    | 24 ++++++++++++++++++++++++
 target/arm/tcg/helper-a64.h    |  1 +
 target/arm/tcg/translate-a64.c | 10 ++++++++++
 4 files changed, 36 insertions(+)

diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
index 8a20dce3c8..3588080024 100644
--- a/target/arm/tcg/a64.decode
+++ b/target/arm/tcg/a64.decode
@@ -207,6 +207,7 @@ MSR_i_DIT       1101 0101 0000 0 011 0100 .... 010 11111 @msr_i
 MSR_i_TCO       1101 0101 0000 0 011 0100 .... 100 11111 @msr_i
 MSR_i_DAIFSET   1101 0101 0000 0 011 0100 .... 110 11111 @msr_i
 MSR_i_DAIFCLEAR 1101 0101 0000 0 011 0100 .... 111 11111 @msr_i
+MSR_i_ALLINT    1101 0101 0000 0 001 0100 .... 000 11111 @msr_i
 MSR_i_SVCR      1101 0101 0000 0 011 0100 0 mask:2 imm:1 011 11111
 
 # MRS, MSR (register), SYS, SYSL. These are all essentially the
diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
index ebaa7f00df..3686926ada 100644
--- a/target/arm/tcg/helper-a64.c
+++ b/target/arm/tcg/helper-a64.c
@@ -66,6 +66,30 @@ void HELPER(msr_i_spsel)(CPUARMState *env, uint32_t imm)
     update_spsel(env, imm);
 }
 
+static void allint_check(CPUARMState *env, uint32_t op,
+                       uint32_t imm, uintptr_t ra)
+{
+    /* ALLINT update to PSTATE. */
+    if (arm_current_el(env) == 0) {
+        raise_exception_ra(env, EXCP_UDEF,
+                           syn_aa64_sysregtrap(0, extract32(op, 0, 3),
+                                               extract32(op, 3, 3), 4,
+                                               imm, 0x1f, 0),
+                           exception_target_el(env), ra);
+    }
+}
+
+void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
+{
+    allint_check(env, 0x8, imm, GETPC());
+    if (imm == 1) {
+        env->allint |= PSTATE_ALLINT;
+    } else {
+        env->allint &= ~PSTATE_ALLINT;
+    }
+    arm_rebuild_hflags(env);
+}
+
 static void daif_check(CPUARMState *env, uint32_t op,
                        uint32_t imm, uintptr_t ra)
 {
diff --git a/target/arm/tcg/helper-a64.h b/target/arm/tcg/helper-a64.h
index 575a5dab7d..3aec703d4a 100644
--- a/target/arm/tcg/helper-a64.h
+++ b/target/arm/tcg/helper-a64.h
@@ -22,6 +22,7 @@ DEF_HELPER_FLAGS_1(rbit64, TCG_CALL_NO_RWG_SE, i64, i64)
 DEF_HELPER_2(msr_i_spsel, void, env, i32)
 DEF_HELPER_2(msr_i_daifset, void, env, i32)
 DEF_HELPER_2(msr_i_daifclear, void, env, i32)
+DEF_HELPER_2(msr_i_allint, void, env, i32)
 DEF_HELPER_3(vfp_cmph_a64, i64, f16, f16, ptr)
 DEF_HELPER_3(vfp_cmpeh_a64, i64, f16, f16, ptr)
 DEF_HELPER_3(vfp_cmps_a64, i64, f32, f32, ptr)
diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
index 340265beb0..f1800f7c71 100644
--- a/target/arm/tcg/translate-a64.c
+++ b/target/arm/tcg/translate-a64.c
@@ -2036,6 +2036,16 @@ static bool trans_MSR_i_DAIFCLEAR(DisasContext *s, arg_i *a)
     return true;
 }
 
+static bool trans_MSR_i_ALLINT(DisasContext *s, arg_i *a)
+{
+    if (!dc_isar_feature(aa64_nmi, s) || s->current_el == 0) {
+        return false;
+    }
+    gen_helper_msr_i_allint(tcg_env, tcg_constant_i32(a->imm));
+    s->base.is_jmp = DISAS_TOO_MANY;
+    return true;
+}
+
 static bool trans_MSR_i_SVCR(DisasContext *s, arg_MSR_i_SVCR *a)
 {
     if (!dc_isar_feature(aa64_sme, s) || a->mask == 0) {
-- 
2.34.1

Re: [RFC PATCH v2 04/22] target/arm: Implement ALLINT MSR (immediate)

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
> to ALLINT. Avoid the unconditional write to pc and use raise_exception_ra
> to unwind.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/tcg/a64.decode      |  1 +
>   target/arm/tcg/helper-a64.c    | 24 ++++++++++++++++++++++++
>   target/arm/tcg/helper-a64.h    |  1 +
>   target/arm/tcg/translate-a64.c | 10 ++++++++++
>   4 files changed, 36 insertions(+)
> 
> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
> index 8a20dce3c8..3588080024 100644
> --- a/target/arm/tcg/a64.decode
> +++ b/target/arm/tcg/a64.decode
> @@ -207,6 +207,7 @@ MSR_i_DIT       1101 0101 0000 0 011 0100 .... 010 11111 @msr_i
>   MSR_i_TCO       1101 0101 0000 0 011 0100 .... 100 11111 @msr_i
>   MSR_i_DAIFSET   1101 0101 0000 0 011 0100 .... 110 11111 @msr_i
>   MSR_i_DAIFCLEAR 1101 0101 0000 0 011 0100 .... 111 11111 @msr_i
> +MSR_i_ALLINT    1101 0101 0000 0 001 0100 .... 000 11111 @msr_i
>   MSR_i_SVCR      1101 0101 0000 0 011 0100 0 mask:2 imm:1 011 11111
>   
>   # MRS, MSR (register), SYS, SYSL. These are all essentially the
> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
> index ebaa7f00df..3686926ada 100644
> --- a/target/arm/tcg/helper-a64.c
> +++ b/target/arm/tcg/helper-a64.c
> @@ -66,6 +66,30 @@ void HELPER(msr_i_spsel)(CPUARMState *env, uint32_t imm)
>       update_spsel(env, imm);
>   }
>   
> +static void allint_check(CPUARMState *env, uint32_t op,
> +                       uint32_t imm, uintptr_t ra)
> +{
> +    /* ALLINT update to PSTATE. */
> +    if (arm_current_el(env) == 0) {
> +        raise_exception_ra(env, EXCP_UDEF,
> +                           syn_aa64_sysregtrap(0, extract32(op, 0, 3),
> +                                               extract32(op, 3, 3), 4,
> +                                               imm, 0x1f, 0),
> +                           exception_target_el(env), ra);
> +    }
> +}

A runtime check for EL0 is not necessary; you've already handled that in 
trans_MSR_i_ALLINT().  However, what *is* missing here is the test against TALLINT for EL1.

> +
> +void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
> +{
> +    allint_check(env, 0x8, imm, GETPC());
> +    if (imm == 1) {
> +        env->allint |= PSTATE_ALLINT;
> +    } else {
> +        env->allint &= ~PSTATE_ALLINT;
> +    }

I think you should not write an immediate-specific helper, but one which can also handle 
the variable "MSR allint, <xt>".  This is no more difficult than

void HELPER(msr_allint)(CPUARMState *env, target_ulong val)
{
     ... check ...
     env->pstate = (env->pstate & ~PSTATE_ALLINT) | (val & PSTATE_ALLINT);
}

> +    arm_rebuild_hflags(env);
> +}

allint does not affect hflags; no rebuild required.

> +static bool trans_MSR_i_ALLINT(DisasContext *s, arg_i *a)
> +{
> +    if (!dc_isar_feature(aa64_nmi, s) || s->current_el == 0) {
> +        return false;
> +    }
> +    gen_helper_msr_i_allint(tcg_env, tcg_constant_i32(a->imm));

You're passing all of #imm4, not #imm1, which meant the test in your msr_i_allint helper 
was wrong.

To work with the generalized helper above, this would be

     tcg_constant_tl((a->imm & 1) * PSTATE_ALLINT);


r~

Re: [RFC PATCH v2 04/22] target/arm: Implement ALLINT MSR (immediate)

[Richard Henderson]

On 2/21/24 09:09, Richard Henderson wrote:
>> +static bool trans_MSR_i_ALLINT(DisasContext *s, arg_i *a)
>> +{
>> +    if (!dc_isar_feature(aa64_nmi, s) || s->current_el == 0) {
>> +        return false;
>> +    }
>> +    gen_helper_msr_i_allint(tcg_env, tcg_constant_i32(a->imm));
> 
> You're passing all of #imm4, not #imm1, which meant the test in your msr_i_allint helper 
> was wrong.
> 
> To work with the generalized helper above, this would be
> 
>      tcg_constant_tl((a->imm & 1) * PSTATE_ALLINT);

Actually, I should have read further in the pseudocode -- (imm & 0xe) != 0 is undefined.


r~

Re: [RFC PATCH v2 04/22] target/arm: Implement ALLINT MSR (immediate)

[Richard Henderson]

On 2/21/24 09:09, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
>> to ALLINT. Avoid the unconditional write to pc and use raise_exception_ra
>> to unwind.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/tcg/a64.decode      |  1 +
>>   target/arm/tcg/helper-a64.c    | 24 ++++++++++++++++++++++++
>>   target/arm/tcg/helper-a64.h    |  1 +
>>   target/arm/tcg/translate-a64.c | 10 ++++++++++
>>   4 files changed, 36 insertions(+)
>>
>> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
>> index 8a20dce3c8..3588080024 100644
>> --- a/target/arm/tcg/a64.decode
>> +++ b/target/arm/tcg/a64.decode
>> @@ -207,6 +207,7 @@ MSR_i_DIT       1101 0101 0000 0 011 0100 .... 010 11111 @msr_i
>>   MSR_i_TCO       1101 0101 0000 0 011 0100 .... 100 11111 @msr_i
>>   MSR_i_DAIFSET   1101 0101 0000 0 011 0100 .... 110 11111 @msr_i
>>   MSR_i_DAIFCLEAR 1101 0101 0000 0 011 0100 .... 111 11111 @msr_i
>> +MSR_i_ALLINT    1101 0101 0000 0 001 0100 .... 000 11111 @msr_i
>>   MSR_i_SVCR      1101 0101 0000 0 011 0100 0 mask:2 imm:1 011 11111
>>   # MRS, MSR (register), SYS, SYSL. These are all essentially the
>> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
>> index ebaa7f00df..3686926ada 100644
>> --- a/target/arm/tcg/helper-a64.c
>> +++ b/target/arm/tcg/helper-a64.c
>> @@ -66,6 +66,30 @@ void HELPER(msr_i_spsel)(CPUARMState *env, uint32_t imm)
>>       update_spsel(env, imm);
>>   }
>> +static void allint_check(CPUARMState *env, uint32_t op,
>> +                       uint32_t imm, uintptr_t ra)
>> +{
>> +    /* ALLINT update to PSTATE. */
>> +    if (arm_current_el(env) == 0) {
>> +        raise_exception_ra(env, EXCP_UDEF,
>> +                           syn_aa64_sysregtrap(0, extract32(op, 0, 3),
>> +                                               extract32(op, 3, 3), 4,
>> +                                               imm, 0x1f, 0),
>> +                           exception_target_el(env), ra);
>> +    }
>> +}
> 
> A runtime check for EL0 is not necessary; you've already handled that in 
> trans_MSR_i_ALLINT().  However, what *is* missing here is the test against TALLINT for EL1.
> 
>> +
>> +void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
>> +{
>> +    allint_check(env, 0x8, imm, GETPC());
>> +    if (imm == 1) {
>> +        env->allint |= PSTATE_ALLINT;
>> +    } else {
>> +        env->allint &= ~PSTATE_ALLINT;
>> +    }
> 
> I think you should not write an immediate-specific helper, but one which can also handle 
> the variable "MSR allint, <xt>".  This is no more difficult than
> 
> void HELPER(msr_allint)(CPUARMState *env, target_ulong val)
> {
>      ... check ...
>      env->pstate = (env->pstate & ~PSTATE_ALLINT) | (val & PSTATE_ALLINT);
> }

Ho hum..  I just noticed that TALLINT only traps immediate write of 1, not also immediate 
write of 0.  So one helper for both MSR Xt and MSR imm is not practical.


r~

Re: [RFC PATCH v2 04/22] target/arm: Implement ALLINT MSR (immediate)

[Jinjie Ruan via]

On 2024/2/22 4:41, Richard Henderson wrote:
> On 2/21/24 09:09, Richard Henderson wrote:
>> On 2/21/24 03:08, Jinjie Ruan via wrote:
>>> Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
>>> to ALLINT. Avoid the unconditional write to pc and use
>>> raise_exception_ra
>>> to unwind.
>>>
>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>>> ---
>>>   target/arm/tcg/a64.decode      |  1 +
>>>   target/arm/tcg/helper-a64.c    | 24 ++++++++++++++++++++++++
>>>   target/arm/tcg/helper-a64.h    |  1 +
>>>   target/arm/tcg/translate-a64.c | 10 ++++++++++
>>>   4 files changed, 36 insertions(+)
>>>
>>> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
>>> index 8a20dce3c8..3588080024 100644
>>> --- a/target/arm/tcg/a64.decode
>>> +++ b/target/arm/tcg/a64.decode
>>> @@ -207,6 +207,7 @@ MSR_i_DIT       1101 0101 0000 0 011 0100 ....
>>> 010 11111 @msr_i
>>>   MSR_i_TCO       1101 0101 0000 0 011 0100 .... 100 11111 @msr_i
>>>   MSR_i_DAIFSET   1101 0101 0000 0 011 0100 .... 110 11111 @msr_i
>>>   MSR_i_DAIFCLEAR 1101 0101 0000 0 011 0100 .... 111 11111 @msr_i
>>> +MSR_i_ALLINT    1101 0101 0000 0 001 0100 .... 000 11111 @msr_i
>>>   MSR_i_SVCR      1101 0101 0000 0 011 0100 0 mask:2 imm:1 011 11111
>>>   # MRS, MSR (register), SYS, SYSL. These are all essentially the
>>> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
>>> index ebaa7f00df..3686926ada 100644
>>> --- a/target/arm/tcg/helper-a64.c
>>> +++ b/target/arm/tcg/helper-a64.c
>>> @@ -66,6 +66,30 @@ void HELPER(msr_i_spsel)(CPUARMState *env,
>>> uint32_t imm)
>>>       update_spsel(env, imm);
>>>   }
>>> +static void allint_check(CPUARMState *env, uint32_t op,
>>> +                       uint32_t imm, uintptr_t ra)
>>> +{
>>> +    /* ALLINT update to PSTATE. */
>>> +    if (arm_current_el(env) == 0) {
>>> +        raise_exception_ra(env, EXCP_UDEF,
>>> +                           syn_aa64_sysregtrap(0, extract32(op, 0, 3),
>>> +                                               extract32(op, 3, 3), 4,
>>> +                                               imm, 0x1f, 0),
>>> +                           exception_target_el(env), ra);
>>> +    }
>>> +}
>>
>> A runtime check for EL0 is not necessary; you've already handled that
>> in trans_MSR_i_ALLINT().  However, what *is* missing here is the test
>> against TALLINT for EL1.
>>
>>> +
>>> +void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
>>> +{
>>> +    allint_check(env, 0x8, imm, GETPC());
>>> +    if (imm == 1) {
>>> +        env->allint |= PSTATE_ALLINT;
>>> +    } else {
>>> +        env->allint &= ~PSTATE_ALLINT;
>>> +    }
>>
>> I think you should not write an immediate-specific helper, but one
>> which can also handle the variable "MSR allint, <xt>".  This is no
>> more difficult than
>>
>> void HELPER(msr_allint)(CPUARMState *env, target_ulong val)
>> {
>>      ... check ...
>>      env->pstate = (env->pstate & ~PSTATE_ALLINT) | (val &
>> PSTATE_ALLINT);
>> }
> 
> Ho hum..  I just noticed that TALLINT only traps immediate write of 1,
> not also immediate write of 0.  So one helper for both MSR Xt and MSR
> imm is not practical.

it is a real problem.

> 
> 
> r~

Re: [RFC PATCH v2 04/22] target/arm: Implement ALLINT MSR (immediate)

[Richard Henderson]

On 2/21/24 10:41, Richard Henderson wrote:
> Ho hum..  I just noticed that TALLINT only traps immediate write of 1, not also immediate 
> write of 0.  So one helper for both MSR Xt and MSR imm is not practical.

Quick follow up to say that means you can do

static bool trans_MSR_i_ALLINT(DisasContext *s, arg_i *a)
{
     if (!dc_isar_feature(aa64_nmi, s)
         || s->current_el == 0
         || (a->imm & ~1)) {
         return false;
     }
     if (!a->imm) {
         clear_pstate_bits(PSTATE_ALLINT);
     } else if (arm_dc_feature(s, ARM_FEATURE_EL2) && s->current_el == 1) {
         /* Use helper for runtime check against HCRX_EL2.TALLINT. */
         gen_helper_msr_set_allint_el1(tcg_env);
     } else {
         set_pstate_bits(PSTATE_ALLINT);
     }
     return true;
}

Generate inline bit ops whenever TALLINT does not apply.
This also means the helper need not check current_el, because we've already done it here.


r~

Re: [RFC PATCH v2 04/22] target/arm: Implement ALLINT MSR (immediate)

[Jinjie Ruan via]

On 2024/2/22 3:09, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
>> to ALLINT. Avoid the unconditional write to pc and use raise_exception_ra
>> to unwind.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/tcg/a64.decode      |  1 +
>>   target/arm/tcg/helper-a64.c    | 24 ++++++++++++++++++++++++
>>   target/arm/tcg/helper-a64.h    |  1 +
>>   target/arm/tcg/translate-a64.c | 10 ++++++++++
>>   4 files changed, 36 insertions(+)
>>
>> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
>> index 8a20dce3c8..3588080024 100644
>> --- a/target/arm/tcg/a64.decode
>> +++ b/target/arm/tcg/a64.decode
>> @@ -207,6 +207,7 @@ MSR_i_DIT       1101 0101 0000 0 011 0100 .... 010
>> 11111 @msr_i
>>   MSR_i_TCO       1101 0101 0000 0 011 0100 .... 100 11111 @msr_i
>>   MSR_i_DAIFSET   1101 0101 0000 0 011 0100 .... 110 11111 @msr_i
>>   MSR_i_DAIFCLEAR 1101 0101 0000 0 011 0100 .... 111 11111 @msr_i
>> +MSR_i_ALLINT    1101 0101 0000 0 001 0100 .... 000 11111 @msr_i
>>   MSR_i_SVCR      1101 0101 0000 0 011 0100 0 mask:2 imm:1 011 11111
>>     # MRS, MSR (register), SYS, SYSL. These are all essentially the
>> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
>> index ebaa7f00df..3686926ada 100644
>> --- a/target/arm/tcg/helper-a64.c
>> +++ b/target/arm/tcg/helper-a64.c
>> @@ -66,6 +66,30 @@ void HELPER(msr_i_spsel)(CPUARMState *env, uint32_t
>> imm)
>>       update_spsel(env, imm);
>>   }
>>   +static void allint_check(CPUARMState *env, uint32_t op,
>> +                       uint32_t imm, uintptr_t ra)
>> +{
>> +    /* ALLINT update to PSTATE. */
>> +    if (arm_current_el(env) == 0) {
>> +        raise_exception_ra(env, EXCP_UDEF,
>> +                           syn_aa64_sysregtrap(0, extract32(op, 0, 3),
>> +                                               extract32(op, 3, 3), 4,
>> +                                               imm, 0x1f, 0),
>> +                           exception_target_el(env), ra);
>> +    }
>> +}
> 
> A runtime check for EL0 is not necessary; you've already handled that in
> trans_MSR_i_ALLINT().  However, what *is* missing here is the test
> against TALLINT for EL1.

Thank you! I'll fix it.

> 
>> +
>> +void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
>> +{
>> +    allint_check(env, 0x8, imm, GETPC());
>> +    if (imm == 1) {
>> +        env->allint |= PSTATE_ALLINT;
>> +    } else {
>> +        env->allint &= ~PSTATE_ALLINT;
>> +    }
> 
> I think you should not write an immediate-specific helper, but one which
> can also handle the variable "MSR allint, <xt>".  This is no more
> difficult than

The kernel patches now uses the immediate-specific helper only, so it is
necessary for test.

> 
> void HELPER(msr_allint)(CPUARMState *env, target_ulong val)
> {
>     ... check ...
>     env->pstate = (env->pstate & ~PSTATE_ALLINT) | (val & PSTATE_ALLINT);
> }
> 
>> +    arm_rebuild_hflags(env);
>> +}
> 
> allint does not affect hflags; no rebuild required.

Thank you! I'll fix it.

> 
>> +static bool trans_MSR_i_ALLINT(DisasContext *s, arg_i *a)
>> +{
>> +    if (!dc_isar_feature(aa64_nmi, s) || s->current_el == 0) {
>> +        return false;
>> +    }
>> +    gen_helper_msr_i_allint(tcg_env, tcg_constant_i32(a->imm));
> 
> You're passing all of #imm4, not #imm1, which meant the test in your
> msr_i_allint helper was wrong.

In fact, it's either 0 or 1 for CRm according to the arm manual, so it
is not necessary.

> 
> To work with the generalized helper above, this would be
> 
>     tcg_constant_tl((a->imm & 1) * PSTATE_ALLINT);
> 
> 
> r~

[RFC PATCH v2 05/22] target/arm: Support MSR access to ALLINT

[Jinjie Ruan via]

Support ALLINT msr access as follow:
	mrs <xt>, ALLINT	// read allint
	msr ALLINT, <xt>	// write allint with imm

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/helper.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index a3062cb2ad..211156d640 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4618,6 +4618,31 @@ static void aa64_daif_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->daif = value & PSTATE_DAIF;
 }
 
+static void aa64_allint_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    env->allint = value & PSTATE_ALLINT;
+}
+
+static uint64_t aa64_allint_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return env->allint & PSTATE_ALLINT;
+}
+
+static CPAccessResult aa64_allint_access(CPUARMState *env,
+                                         const ARMCPRegInfo *ri, bool isread)
+{
+    if (arm_current_el(env) == 0) {
+        return CP_ACCESS_TRAP_UNCATEGORIZED;
+    }
+
+    if (arm_current_el(env) == 1 && arm_is_el2_enabled(env) &&
+        cpu_isar_feature(aa64_hcx, env_archcpu(env)) &&
+        (env->cp15.hcrx_el2 & HCRX_TALLINT))
+        return CP_ACCESS_TRAP_EL2;
+    return CP_ACCESS_OK;
+}
+
 static uint64_t aa64_pan_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     return env->pstate & PSTATE_PAN;
@@ -5437,6 +5462,13 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
       .access = PL0_RW, .accessfn = aa64_daif_access,
       .fieldoffset = offsetof(CPUARMState, daif),
       .writefn = aa64_daif_write, .resetfn = arm_cp_reset_ignore },
+    { .name = "ALLINT", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 0, .opc2 = 0, .crn = 4, .crm = 3,
+      .type = ARM_CP_NO_RAW,
+      .access = PL1_RW, .accessfn = aa64_allint_access,
+      .fieldoffset = offsetof(CPUARMState, allint),
+      .writefn = aa64_allint_write, .readfn = aa64_allint_read,
+      .resetfn = arm_cp_reset_ignore },
     { .name = "FPCR", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .opc2 = 0, .crn = 4, .crm = 4,
       .access = PL0_RW, .type = ARM_CP_FPU | ARM_CP_SUPPRESS_TB_END,
-- 
2.34.1

Re: [RFC PATCH v2 05/22] target/arm: Support MSR access to ALLINT

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Support ALLINT msr access as follow:
> 	mrs <xt>, ALLINT	// read allint
> 	msr ALLINT, <xt>	// write allint with imm
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/helper.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a3062cb2ad..211156d640 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -4618,6 +4618,31 @@ static void aa64_daif_write(CPUARMState *env, const ARMCPRegInfo *ri,
>       env->daif = value & PSTATE_DAIF;
>   }
>   
> +static void aa64_allint_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                              uint64_t value)
> +{
> +    env->allint = value & PSTATE_ALLINT;
> +}
> +
> +static uint64_t aa64_allint_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    return env->allint & PSTATE_ALLINT;
> +}
> +
> +static CPAccessResult aa64_allint_access(CPUARMState *env,
> +                                         const ARMCPRegInfo *ri, bool isread)
> +{
> +    if (arm_current_el(env) == 0) {
> +        return CP_ACCESS_TRAP_UNCATEGORIZED;
> +    }

This is handled by .access PL1_RW.

> +
> +    if (arm_current_el(env) == 1 && arm_is_el2_enabled(env) &&
> +        cpu_isar_feature(aa64_hcx, env_archcpu(env)) &&
> +        (env->cp15.hcrx_el2 & HCRX_TALLINT))
> +        return CP_ACCESS_TRAP_EL2;

You should be using arm_hcrx_el2_eff(env).
Missing braces.

> @@ -5437,6 +5462,13 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>         .access = PL0_RW, .accessfn = aa64_daif_access,
>         .fieldoffset = offsetof(CPUARMState, daif),
>         .writefn = aa64_daif_write, .resetfn = arm_cp_reset_ignore },
> +    { .name = "ALLINT", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 0, .opc2 = 0, .crn = 4, .crm = 3,
> +      .type = ARM_CP_NO_RAW,
> +      .access = PL1_RW, .accessfn = aa64_allint_access,
> +      .fieldoffset = offsetof(CPUARMState, allint),
> +      .writefn = aa64_allint_write, .readfn = aa64_allint_read,
> +      .resetfn = arm_cp_reset_ignore },

You cannot add ALLINT here in v8_cp_reginfo[].
Compare fgt_reginfo[], and how it is registered.


r~

Re: [RFC PATCH v2 05/22] target/arm: Support MSR access to ALLINT

[Jinjie Ruan via]

On 2024/2/22 3:28, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> Support ALLINT msr access as follow:
>>     mrs <xt>, ALLINT    // read allint
>>     msr ALLINT, <xt>    // write allint with imm
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/helper.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index a3062cb2ad..211156d640 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -4618,6 +4618,31 @@ static void aa64_daif_write(CPUARMState *env,
>> const ARMCPRegInfo *ri,
>>       env->daif = value & PSTATE_DAIF;
>>   }
>>   +static void aa64_allint_write(CPUARMState *env, const ARMCPRegInfo
>> *ri,
>> +                              uint64_t value)
>> +{
>> +    env->allint = value & PSTATE_ALLINT;
>> +}
>> +
>> +static uint64_t aa64_allint_read(CPUARMState *env, const ARMCPRegInfo
>> *ri)
>> +{
>> +    return env->allint & PSTATE_ALLINT;
>> +}
>> +
>> +static CPAccessResult aa64_allint_access(CPUARMState *env,
>> +                                         const ARMCPRegInfo *ri, bool
>> isread)
>> +{
>> +    if (arm_current_el(env) == 0) {
>> +        return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +    }
> 
> This is handled by .access PL1_RW.
> 
>> +
>> +    if (arm_current_el(env) == 1 && arm_is_el2_enabled(env) &&
>> +        cpu_isar_feature(aa64_hcx, env_archcpu(env)) &&
>> +        (env->cp15.hcrx_el2 & HCRX_TALLINT))
>> +        return CP_ACCESS_TRAP_EL2;
> 
> You should be using arm_hcrx_el2_eff(env).
> Missing braces.

I'll fix it, thank you!

> 
>> @@ -5437,6 +5462,13 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>>         .access = PL0_RW, .accessfn = aa64_daif_access,
>>         .fieldoffset = offsetof(CPUARMState, daif),
>>         .writefn = aa64_daif_write, .resetfn = arm_cp_reset_ignore },
>> +    { .name = "ALLINT", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 0, .opc2 = 0, .crn = 4, .crm = 3,
>> +      .type = ARM_CP_NO_RAW,
>> +      .access = PL1_RW, .accessfn = aa64_allint_access,
>> +      .fieldoffset = offsetof(CPUARMState, allint),
>> +      .writefn = aa64_allint_write, .readfn = aa64_allint_read,
>> +      .resetfn = arm_cp_reset_ignore },
> 
> You cannot add ALLINT here in v8_cp_reginfo[].
> Compare fgt_reginfo[], and how it is registered.

I'll fix it, thank you!

> 
> 
> r~

[RFC PATCH v2 06/22] target/arm: Add support for Non-maskable Interrupt

[Jinjie Ruan via]

This only implements the external delivery method via the GICv3.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/cpu-qom.h |  3 ++-
 target/arm/cpu.c     | 39 ++++++++++++++++++++++++++++++++++-----
 target/arm/cpu.h     |  2 ++
 target/arm/helper.c  |  1 +
 4 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
index 8e032691db..66d555a605 100644
--- a/target/arm/cpu-qom.h
+++ b/target/arm/cpu-qom.h
@@ -36,11 +36,12 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
 #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
 #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
 
-/* Meanings of the ARMCPU object's four inbound GPIO lines */
+/* Meanings of the ARMCPU object's five 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_NMI 4
 
 /* For M profile, some registers are banked secure vs non-secure;
  * these are represented as a 2-element array where the first element
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5e5978c302..055670343e 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -128,7 +128,7 @@ static bool arm_cpu_has_work(CPUState *cs)
 
     return (cpu->power_state != PSCI_OFF)
         && cs->interrupt_request &
-        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
+        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI
          | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR
          | CPU_INTERRUPT_EXITTB);
 }
@@ -668,6 +668,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
     CPUARMState *env = cpu_env(cs);
     bool pstate_unmasked;
     bool unmasked = false;
+    bool nmi_unmasked = false;
 
     /*
      * Don't take exceptions if they target a lower EL.
@@ -678,13 +679,29 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
         return false;
     }
 
+    nmi_unmasked = (!(env->allint & PSTATE_ALLINT)) &
+                   (!((env->cp15.sctlr_el[target_el] & SCTLR_SPINTMASK) &&
+                   (env->pstate & PSTATE_SP) && cur_el == target_el));
+
     switch (excp_idx) {
+    case EXCP_NMI:
+        pstate_unmasked = nmi_unmasked;
+        break;
+
     case EXCP_FIQ:
-        pstate_unmasked = !(env->daif & PSTATE_F);
+        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
+            pstate_unmasked = (!(env->daif & PSTATE_F)) & nmi_unmasked;
+        } else {
+            pstate_unmasked = !(env->daif & PSTATE_F);
+        }
         break;
 
     case EXCP_IRQ:
-        pstate_unmasked = !(env->daif & PSTATE_I);
+        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
+            pstate_unmasked = (!(env->daif & PSTATE_I)) & nmi_unmasked;
+        } else {
+            pstate_unmasked = !(env->daif & PSTATE_I);
+        }
         break;
 
     case EXCP_VFIQ:
@@ -804,6 +821,16 @@ static bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 
     /* The prioritization of interrupts is IMPLEMENTATION DEFINED. */
 
+    if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
+        if (interrupt_request & CPU_INTERRUPT_NMI) {
+            excp_idx = EXCP_NMI;
+            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);
@@ -929,7 +956,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_NMI] = CPU_INTERRUPT_NMI
     };
 
     if (!arm_feature(env, ARM_FEATURE_EL2) &&
@@ -957,6 +985,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_NMI:
         if (level) {
             cpu_interrupt(cs, mask[irq]);
         } else {
@@ -1358,7 +1387,7 @@ static void arm_cpu_initfn(Object *obj)
          */
         qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 4);
     } else {
-        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 4);
+        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 5);
     }
 
     qdev_init_gpio_out(DEVICE(cpu), cpu->gt_timer_outputs,
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f9646dbbfb..5257343bcb 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -60,6 +60,7 @@
 #define EXCP_DIVBYZERO      23   /* v7M DIVBYZERO UsageFault */
 #define EXCP_VSERR          24
 #define EXCP_GPC            25   /* v9 Granule Protection Check Fault */
+#define EXCP_NMI            26
 /* NB: add new EXCP_ defines to the array in arm_log_exception() too */
 
 #define ARMV7M_EXCP_RESET   1
@@ -79,6 +80,7 @@
 #define CPU_INTERRUPT_VIRQ  CPU_INTERRUPT_TGT_EXT_2
 #define CPU_INTERRUPT_VFIQ  CPU_INTERRUPT_TGT_EXT_3
 #define CPU_INTERRUPT_VSERR CPU_INTERRUPT_TGT_INT_0
+#define CPU_INTERRUPT_NMI   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
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 211156d640..bd7858e02e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10626,6 +10626,7 @@ void arm_log_exception(CPUState *cs)
             [EXCP_DIVBYZERO] = "v7M DIVBYZERO UsageFault",
             [EXCP_VSERR] = "Virtual SERR",
             [EXCP_GPC] = "Granule Protection Check",
+            [EXCP_NMI] = "NMI"
         };
 
         if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
-- 
2.34.1

Re: [RFC PATCH v2 06/22] target/arm: Add support for Non-maskable Interrupt

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> This only implements the external delivery method via the GICv3.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/cpu-qom.h |  3 ++-
>   target/arm/cpu.c     | 39 ++++++++++++++++++++++++++++++++++-----
>   target/arm/cpu.h     |  2 ++
>   target/arm/helper.c  |  1 +
>   4 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
> index 8e032691db..66d555a605 100644
> --- a/target/arm/cpu-qom.h
> +++ b/target/arm/cpu-qom.h
> @@ -36,11 +36,12 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
>   #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
>   #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
>   
> -/* Meanings of the ARMCPU object's four inbound GPIO lines */
> +/* Meanings of the ARMCPU object's five 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_NMI 4
>   
>   /* For M profile, some registers are banked secure vs non-secure;
>    * these are represented as a 2-element array where the first element
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5e5978c302..055670343e 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -128,7 +128,7 @@ static bool arm_cpu_has_work(CPUState *cs)
>   
>       return (cpu->power_state != PSCI_OFF)
>           && cs->interrupt_request &
> -        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
> +        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI
>            | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR
>            | CPU_INTERRUPT_EXITTB);
>   }

I think you should not include CPU_INTERRUPT_NMI when it cannot be delivered, e.g. 
FEAT_NMI not enabled.


> @@ -668,6 +668,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>       CPUARMState *env = cpu_env(cs);
>       bool pstate_unmasked;
>       bool unmasked = false;
> +    bool nmi_unmasked = false;
>   
>       /*
>        * Don't take exceptions if they target a lower EL.
> @@ -678,13 +679,29 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>           return false;
>       }
>   
> +    nmi_unmasked = (!(env->allint & PSTATE_ALLINT)) &
> +                   (!((env->cp15.sctlr_el[target_el] & SCTLR_SPINTMASK) &&
> +                   (env->pstate & PSTATE_SP) && cur_el == target_el));

I don't see SCTLR_ELx.NMI being tested anywhere, which is required to enable everything else.

>       case EXCP_FIQ:
> -        pstate_unmasked = !(env->daif & PSTATE_F);
> +        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
> +            pstate_unmasked = (!(env->daif & PSTATE_F)) & nmi_unmasked;
> +        } else {
> +            pstate_unmasked = !(env->daif & PSTATE_F);
> +        }
>           break;
>   
>       case EXCP_IRQ:
> -        pstate_unmasked = !(env->daif & PSTATE_I);
> +        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
> +            pstate_unmasked = (!(env->daif & PSTATE_I)) & nmi_unmasked;
> +        } else {
> +            pstate_unmasked = !(env->daif & PSTATE_I);
> +        }
>           break;

I don't see what this is doing.  While Superpriority is IMPLEMENTATION DEFINED, how are 
you defining it for QEMU?  Is there a definition from real hw which makes sense under 
emulation?


r~

Re: [RFC PATCH v2 06/22] target/arm: Add support for Non-maskable Interrupt

[Jinjie Ruan via]

On 2024/2/22 4:06, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> This only implements the external delivery method via the GICv3.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/cpu-qom.h |  3 ++-
>>   target/arm/cpu.c     | 39 ++++++++++++++++++++++++++++++++++-----
>>   target/arm/cpu.h     |  2 ++
>>   target/arm/helper.c  |  1 +
>>   4 files changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
>> index 8e032691db..66d555a605 100644
>> --- a/target/arm/cpu-qom.h
>> +++ b/target/arm/cpu-qom.h
>> @@ -36,11 +36,12 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
>>   #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
>>   #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
>>   -/* Meanings of the ARMCPU object's four inbound GPIO lines */
>> +/* Meanings of the ARMCPU object's five 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_NMI 4
>>     /* For M profile, some registers are banked secure vs non-secure;
>>    * these are represented as a 2-element array where the first element
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 5e5978c302..055670343e 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -128,7 +128,7 @@ static bool arm_cpu_has_work(CPUState *cs)
>>         return (cpu->power_state != PSCI_OFF)
>>           && cs->interrupt_request &
>> -        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
>> +        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI
>>            | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ |
>> CPU_INTERRUPT_VSERR
>>            | CPU_INTERRUPT_EXITTB);
>>   }
> 
> I think you should not include CPU_INTERRUPT_NMI when it cannot be
> delivered, e.g. FEAT_NMI not enabled.

OK! I'll fix it.
> 
> 
>> @@ -668,6 +668,7 @@ static inline bool arm_excp_unmasked(CPUState *cs,
>> unsigned int excp_idx,
>>       CPUARMState *env = cpu_env(cs);
>>       bool pstate_unmasked;
>>       bool unmasked = false;
>> +    bool nmi_unmasked = false;
>>         /*
>>        * Don't take exceptions if they target a lower EL.
>> @@ -678,13 +679,29 @@ static inline bool arm_excp_unmasked(CPUState
>> *cs, unsigned int excp_idx,
>>           return false;
>>       }
>>   +    nmi_unmasked = (!(env->allint & PSTATE_ALLINT)) &
>> +                   (!((env->cp15.sctlr_el[target_el] &
>> SCTLR_SPINTMASK) &&
>> +                   (env->pstate & PSTATE_SP) && cur_el == target_el));
> 
> I don't see SCTLR_ELx.NMI being tested anywhere, which is required to
> enable everything else.

OK! I'll fix it.

> 
>>       case EXCP_FIQ:
>> -        pstate_unmasked = !(env->daif & PSTATE_F);
>> +        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
>> +            pstate_unmasked = (!(env->daif & PSTATE_F)) & nmi_unmasked;
>> +        } else {
>> +            pstate_unmasked = !(env->daif & PSTATE_F);
>> +        }
>>           break;
>>         case EXCP_IRQ:
>> -        pstate_unmasked = !(env->daif & PSTATE_I);
>> +        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
>> +            pstate_unmasked = (!(env->daif & PSTATE_I)) & nmi_unmasked;
>> +        } else {
>> +            pstate_unmasked = !(env->daif & PSTATE_I);
>> +        }
>>           break;
> 
> I don't see what this is doing.  While Superpriority is IMPLEMENTATION
> DEFINED, how are you defining it for QEMU?  Is there a definition from
> real hw which makes sense under emulation?

These code add allint mask for IRQ or FIQ,since ALLINT also mask them.

The superpriority is DEFINED in  gicv3 code and it is a NMI exception
for PE.

> 
> 
> r~

Re: [RFC PATCH v2 06/22] target/arm: Add support for Non-maskable Interrupt

[Jinjie Ruan via]

On 2024/2/22 4:06, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> This only implements the external delivery method via the GICv3.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/cpu-qom.h |  3 ++-
>>   target/arm/cpu.c     | 39 ++++++++++++++++++++++++++++++++++-----
>>   target/arm/cpu.h     |  2 ++
>>   target/arm/helper.c  |  1 +
>>   4 files changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
>> index 8e032691db..66d555a605 100644
>> --- a/target/arm/cpu-qom.h
>> +++ b/target/arm/cpu-qom.h
>> @@ -36,11 +36,12 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
>>   #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
>>   #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
>>   -/* Meanings of the ARMCPU object's four inbound GPIO lines */
>> +/* Meanings of the ARMCPU object's five 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_NMI 4
>>     /* For M profile, some registers are banked secure vs non-secure;
>>    * these are represented as a 2-element array where the first element
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 5e5978c302..055670343e 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -128,7 +128,7 @@ static bool arm_cpu_has_work(CPUState *cs)
>>         return (cpu->power_state != PSCI_OFF)
>>           && cs->interrupt_request &
>> -        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
>> +        (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI
>>            | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ |
>> CPU_INTERRUPT_VSERR
>>            | CPU_INTERRUPT_EXITTB);
>>   }
> 
> I think you should not include CPU_INTERRUPT_NMI when it cannot be
> delivered, e.g. FEAT_NMI not enabled.

I'll fix it.

> 
> 
>> @@ -668,6 +668,7 @@ static inline bool arm_excp_unmasked(CPUState *cs,
>> unsigned int excp_idx,
>>       CPUARMState *env = cpu_env(cs);
>>       bool pstate_unmasked;
>>       bool unmasked = false;
>> +    bool nmi_unmasked = false;
>>         /*
>>        * Don't take exceptions if they target a lower EL.
>> @@ -678,13 +679,29 @@ static inline bool arm_excp_unmasked(CPUState
>> *cs, unsigned int excp_idx,
>>           return false;
>>       }
>>   +    nmi_unmasked = (!(env->allint & PSTATE_ALLINT)) &
>> +                   (!((env->cp15.sctlr_el[target_el] &
>> SCTLR_SPINTMASK) &&
>> +                   (env->pstate & PSTATE_SP) && cur_el == target_el));
> 
> I don't see SCTLR_ELx.NMI being tested anywhere, which is required to
> enable everything else.

I'll Add it

> 
>>       case EXCP_FIQ:
>> -        pstate_unmasked = !(env->daif & PSTATE_F);
>> +        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
>> +            pstate_unmasked = (!(env->daif & PSTATE_F)) & nmi_unmasked;
>> +        } else {
>> +            pstate_unmasked = !(env->daif & PSTATE_F);
>> +        }
>>           break;
>>         case EXCP_IRQ:
>> -        pstate_unmasked = !(env->daif & PSTATE_I);
>> +        if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
>> +            pstate_unmasked = (!(env->daif & PSTATE_I)) & nmi_unmasked;
>> +        } else {
>> +            pstate_unmasked = !(env->daif & PSTATE_I);
>> +        }
>>           break;
> 
> I don't see what this is doing.  While Superpriority is IMPLEMENTATION
> DEFINED, how are you defining it for QEMU?  Is there a definition from
> real hw which makes sense under emulation?
> 
> 
> r~

Re: [RFC PATCH v2 06/22] target/arm: Add support for Non-maskable Interrupt

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> This only implements the external delivery method via the GICv3.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/cpu-qom.h |  3 ++-
>   target/arm/cpu.c     | 39 ++++++++++++++++++++++++++++++++++-----
>   target/arm/cpu.h     |  2 ++
>   target/arm/helper.c  |  1 +
>   4 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
> index 8e032691db..66d555a605 100644
> --- a/target/arm/cpu-qom.h
> +++ b/target/arm/cpu-qom.h
> @@ -36,11 +36,12 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
>   #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
>   #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
>   
> -/* Meanings of the ARMCPU object's four inbound GPIO lines */
> +/* Meanings of the ARMCPU object's five 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_NMI 4

You need a 6th GPIO for vNMI.


r~

Re: [RFC PATCH v2 06/22] target/arm: Add support for Non-maskable Interrupt

[Jinjie Ruan via]

On 2024/2/22 5:23, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> This only implements the external delivery method via the GICv3.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/cpu-qom.h |  3 ++-
>>   target/arm/cpu.c     | 39 ++++++++++++++++++++++++++++++++++-----
>>   target/arm/cpu.h     |  2 ++
>>   target/arm/helper.c  |  1 +
>>   4 files changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
>> index 8e032691db..66d555a605 100644
>> --- a/target/arm/cpu-qom.h
>> +++ b/target/arm/cpu-qom.h
>> @@ -36,11 +36,12 @@ DECLARE_CLASS_CHECKERS(AArch64CPUClass, AARCH64_CPU,
>>   #define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
>>   #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
>>   -/* Meanings of the ARMCPU object's four inbound GPIO lines */
>> +/* Meanings of the ARMCPU object's five 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_NMI 4
> 
> You need a 6th GPIO for vNMI.
Thank you! I'll fix it.
> 
> 
> r~

[RFC PATCH v2 07/22] target/arm: Add support for NMI event state

[Jinjie Ruan via]

The NMI exception state include whether the interrupt with super priority
is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish it.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/cpu.h    | 2 ++
 target/arm/helper.c | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5257343bcb..051e589e19 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -603,6 +603,8 @@ typedef struct CPUArchState {
     /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */
     uint32_t irq_line_state;
 
+    bool nmi_is_irq;
+
     /* Thumb-2 EE state.  */
     uint32_t teecr;
     uint32_t teehbr;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index bd7858e02e..0bd7a87e51 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10575,6 +10575,15 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
         scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ);
         hcr = hcr_el2 & HCR_FMO;
         break;
+    case EXCP_NMI:
+        if (env->nmi_is_irq) {
+            scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
+            hcr = hcr_el2 & HCR_IMO;
+        } else {
+            scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ);
+            hcr = hcr_el2 & HCR_FMO;
+        }
+        break;
     default:
         scr = ((env->cp15.scr_el3 & SCR_EA) == SCR_EA);
         hcr = hcr_el2 & HCR_AMO;
-- 
2.34.1

Re: [RFC PATCH v2 07/22] target/arm: Add support for NMI event state

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> The NMI exception state include whether the interrupt with super priority
> is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish it.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/cpu.h    | 2 ++
>   target/arm/helper.c | 9 +++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 5257343bcb..051e589e19 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -603,6 +603,8 @@ typedef struct CPUArchState {
>       /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */
>       uint32_t irq_line_state;
>   
> +    bool nmi_is_irq;

Why would you need to add this to CPUARMState?
This has the appearance of requiring only a local variable.
But it is hard to tell since you do not set it within this patch at all.


r~

Re: [RFC PATCH v2 07/22] target/arm: Add support for NMI event state

[Richard Henderson]

On 2/21/24 10:10, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> The NMI exception state include whether the interrupt with super priority
>> is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish it.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/cpu.h    | 2 ++
>>   target/arm/helper.c | 9 +++++++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 5257343bcb..051e589e19 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -603,6 +603,8 @@ typedef struct CPUArchState {
>>       /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */
>>       uint32_t irq_line_state;
>> +    bool nmi_is_irq;
> 
> Why would you need to add this to CPUARMState?
> This has the appearance of requiring only a local variable.
> But it is hard to tell since you do not set it within this patch at all.

According to Arm GIC section 4.6.3 Interrupt superpriority, NMI is always IRQ, never FIQ, 
so this is never required.


r~

Re: [RFC PATCH v2 07/22] target/arm: Add support for NMI event state

[Jinjie Ruan via]

On 2024/2/22 5:25, Richard Henderson wrote:
> On 2/21/24 10:10, Richard Henderson wrote:
>> On 2/21/24 03:08, Jinjie Ruan via wrote:
>>> The NMI exception state include whether the interrupt with super
>>> priority
>>> is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish
>>> it.
>>>
>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>>> ---
>>>   target/arm/cpu.h    | 2 ++
>>>   target/arm/helper.c | 9 +++++++++
>>>   2 files changed, 11 insertions(+)
>>>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 5257343bcb..051e589e19 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -603,6 +603,8 @@ typedef struct CPUArchState {
>>>       /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */
>>>       uint32_t irq_line_state;
>>> +    bool nmi_is_irq;
>>
>> Why would you need to add this to CPUARMState?
>> This has the appearance of requiring only a local variable.
>> But it is hard to tell since you do not set it within this patch at all.
> 
> According to Arm GIC section 4.6.3 Interrupt superpriority, NMI is
> always IRQ, never FIQ, so this is never required.

There is a bit of ambiguity here. The processor manual says that both
irq and fiq can have superpriority attributes, but the gic manual only
says that the IRQ has superpriority attributes.

> 
> 
> r~
> 

Re: [RFC PATCH v2 07/22] target/arm: Add support for NMI event state

[Richard Henderson]

On 2/22/24 01:52, Jinjie Ruan wrote:
> 
> 
> On 2024/2/22 5:25, Richard Henderson wrote:
>> On 2/21/24 10:10, Richard Henderson wrote:
>>> On 2/21/24 03:08, Jinjie Ruan via wrote:
>>>> The NMI exception state include whether the interrupt with super
>>>> priority
>>>> is IRQ or FIQ, so add a nmi_is_irq flag in CPUARMState to distinguish
>>>> it.
>>>>
>>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>>>> ---
>>>>    target/arm/cpu.h    | 2 ++
>>>>    target/arm/helper.c | 9 +++++++++
>>>>    2 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>> index 5257343bcb..051e589e19 100644
>>>> --- a/target/arm/cpu.h
>>>> +++ b/target/arm/cpu.h
>>>> @@ -603,6 +603,8 @@ typedef struct CPUArchState {
>>>>        /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */
>>>>        uint32_t irq_line_state;
>>>> +    bool nmi_is_irq;
>>>
>>> Why would you need to add this to CPUARMState?
>>> This has the appearance of requiring only a local variable.
>>> But it is hard to tell since you do not set it within this patch at all.
>>
>> According to Arm GIC section 4.6.3 Interrupt superpriority, NMI is
>> always IRQ, never FIQ, so this is never required.
> 
> There is a bit of ambiguity here. The processor manual says that both
> irq and fiq can have superpriority attributes, but the gic manual only
> says that the IRQ has superpriority attributes.

Yes, it is possible to inject a superpriority FIQ via HCRX_EL2.  But you don't need an 
extra variable to handle this.


r~

[RFC PATCH v2 08/22] target/arm: Handle IS/FS in ISR_EL1 for NMI

[Jinjie Ruan via]

Add IS and FS bit in ISR_EL1 and handle the read according to whether the
NMI is IRQ or FIQ.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/cpu.h    | 2 ++
 target/arm/helper.c | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 051e589e19..e2d07e3312 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1476,6 +1476,8 @@ FIELD(CPTR_EL3, TCPAC, 31, 1)
 #define CPSR_N (1U << 31)
 #define CPSR_NZCV (CPSR_N | CPSR_Z | CPSR_C | CPSR_V)
 #define CPSR_AIF (CPSR_A | CPSR_I | CPSR_F)
+#define ISR_FS (1U << 9)
+#define ISR_IS (1U << 10)
 
 #define CPSR_IT (CPSR_IT_0_1 | CPSR_IT_2_7)
 #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT | CPSR_Q \
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0bd7a87e51..62c8e5d611 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2022,6 +2022,10 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
         if (cs->interrupt_request & CPU_INTERRUPT_HARD) {
             ret |= CPSR_I;
         }
+
+        if ((cs->interrupt_request & CPU_INTERRUPT_NMI) && env->nmi_is_irq) {
+            ret |= ISR_IS;
+        }
     }
 
     if (hcr_el2 & HCR_FMO) {
@@ -2032,6 +2036,11 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
         if (cs->interrupt_request & CPU_INTERRUPT_FIQ) {
             ret |= CPSR_F;
         }
+
+        if ((cs->interrupt_request & CPU_INTERRUPT_NMI) &&
+            (!env->nmi_is_irq)) {
+            ret |= ISR_FS;
+        }
     }
 
     if (hcr_el2 & HCR_AMO) {
-- 
2.34.1

Re: [RFC PATCH v2 08/22] target/arm: Handle IS/FS in ISR_EL1 for NMI

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Add IS and FS bit in ISR_EL1 and handle the read according to whether the
> NMI is IRQ or FIQ.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/cpu.h    | 2 ++
>   target/arm/helper.c | 9 +++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 051e589e19..e2d07e3312 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1476,6 +1476,8 @@ FIELD(CPTR_EL3, TCPAC, 31, 1)
>   #define CPSR_N (1U << 31)
>   #define CPSR_NZCV (CPSR_N | CPSR_Z | CPSR_C | CPSR_V)
>   #define CPSR_AIF (CPSR_A | CPSR_I | CPSR_F)
> +#define ISR_FS (1U << 9)
> +#define ISR_IS (1U << 10)
>   
>   #define CPSR_IT (CPSR_IT_0_1 | CPSR_IT_2_7)
>   #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT | CPSR_Q \
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0bd7a87e51..62c8e5d611 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2022,6 +2022,10 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>           if (cs->interrupt_request & CPU_INTERRUPT_HARD) {
>               ret |= CPSR_I;
>           }
> +
> +        if ((cs->interrupt_request & CPU_INTERRUPT_NMI) && env->nmi_is_irq) {
> +            ret |= ISR_IS;
> +        }
>       }
>   
>       if (hcr_el2 & HCR_FMO) {
> @@ -2032,6 +2036,11 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>           if (cs->interrupt_request & CPU_INTERRUPT_FIQ) {
>               ret |= CPSR_F;
>           }
> +
> +        if ((cs->interrupt_request & CPU_INTERRUPT_NMI) &&
> +            (!env->nmi_is_irq)) {
> +            ret |= ISR_FS;
> +        }
>       }

The external CPU_INTERRUPT_NMI will never signal FIQ.

With CPU_INTERRUPT_NMI, both CPSR_I and ISR_IS must be set.

Missing is the handling of HCRX_EL2.{VFNMI,VINMI} to signal vFIQ and vIRQ with 
superpriority.  Unless I missed it, I don't see HCRX_EL2 adjusted for FEAT_NMI at all.


r~

Re: [RFC PATCH v2 08/22] target/arm: Handle IS/FS in ISR_EL1 for NMI

[Jinjie Ruan via]

On 2024/2/22 5:36, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> Add IS and FS bit in ISR_EL1 and handle the read according to whether the
>> NMI is IRQ or FIQ.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/cpu.h    | 2 ++
>>   target/arm/helper.c | 9 +++++++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 051e589e19..e2d07e3312 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -1476,6 +1476,8 @@ FIELD(CPTR_EL3, TCPAC, 31, 1)
>>   #define CPSR_N (1U << 31)
>>   #define CPSR_NZCV (CPSR_N | CPSR_Z | CPSR_C | CPSR_V)
>>   #define CPSR_AIF (CPSR_A | CPSR_I | CPSR_F)
>> +#define ISR_FS (1U << 9)
>> +#define ISR_IS (1U << 10)
>>     #define CPSR_IT (CPSR_IT_0_1 | CPSR_IT_2_7)
>>   #define CACHED_CPSR_BITS (CPSR_T | CPSR_AIF | CPSR_GE | CPSR_IT |
>> CPSR_Q \
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 0bd7a87e51..62c8e5d611 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -2022,6 +2022,10 @@ static uint64_t isr_read(CPUARMState *env,
>> const ARMCPRegInfo *ri)
>>           if (cs->interrupt_request & CPU_INTERRUPT_HARD) {
>>               ret |= CPSR_I;
>>           }
>> +
>> +        if ((cs->interrupt_request & CPU_INTERRUPT_NMI) &&
>> env->nmi_is_irq) {
>> +            ret |= ISR_IS;
>> +        }
>>       }
>>         if (hcr_el2 & HCR_FMO) {
>> @@ -2032,6 +2036,11 @@ static uint64_t isr_read(CPUARMState *env,
>> const ARMCPRegInfo *ri)
>>           if (cs->interrupt_request & CPU_INTERRUPT_FIQ) {
>>               ret |= CPSR_F;
>>           }
>> +
>> +        if ((cs->interrupt_request & CPU_INTERRUPT_NMI) &&
>> +            (!env->nmi_is_irq)) {
>> +            ret |= ISR_FS;
>> +        }
>>       }
> 
> The external CPU_INTERRUPT_NMI will never signal FIQ.
> 
> With CPU_INTERRUPT_NMI, both CPSR_I and ISR_IS must be set.

Right! The IRQ and NMI signal are both set when it is a NMI.
> 
> Missing is the handling of HCRX_EL2.{VFNMI,VINMI} to signal vFIQ and
> vIRQ with superpriority.  Unless I missed it, I don't see HCRX_EL2
> adjusted for FEAT_NMI at all.

Sorry! The vNMI has not yet been implemented.

The HCRX_EL2.TALLINT has yet been handled.

> 
> 
> r~
> 

[RFC PATCH v2 09/22] target/arm: Add support for FEAT_NMI, Non-maskable Interrupt

[Jinjie Ruan via]

Add support for FEAT_NMI. NMI (FEAT_NMI) is an mandatory feature in
ARMv8.8-A and ARM v9.3-A.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/internals.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 50bff44549..fee65caba5 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1078,6 +1078,9 @@ static inline uint32_t aarch64_pstate_valid_mask(const ARMISARegisters *id)
     if (isar_feature_aa64_mte(id)) {
         valid |= PSTATE_TCO;
     }
+    if (isar_feature_aa64_nmi(id)) {
+        valid |= PSTATE_ALLINT;
+    }
 
     return valid;
 }
-- 
2.34.1

Re: [RFC PATCH v2 09/22] target/arm: Add support for FEAT_NMI, Non-maskable Interrupt

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Add support for FEAT_NMI. NMI (FEAT_NMI) is an mandatory feature in
> ARMv8.8-A and ARM v9.3-A.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/internals.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 50bff44549..fee65caba5 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1078,6 +1078,9 @@ static inline uint32_t aarch64_pstate_valid_mask(const ARMISARegisters *id)
>       if (isar_feature_aa64_mte(id)) {
>           valid |= PSTATE_TCO;
>       }
> +    if (isar_feature_aa64_nmi(id)) {
> +        valid |= PSTATE_ALLINT;
> +    }
>   
>       return valid;
>   }

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

This patch probably should have been sorted before the MSR patches.


r~

[RFC PATCH v2 10/22] target/arm: Handle PSTATE.ALLINT on taking an exception

[Jinjie Ruan via]

Set or clear PSTATE.ALLINT on taking an exception to ELx according to the
SCTLR_ELx.SPINTMASK bit.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 62c8e5d611..952ea7c02a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11540,6 +11540,15 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
         }
     }
 
+    if (cpu_isar_feature(aa64_nmi, cpu) &&
+        (env->cp15.sctlr_el[new_el] & SCTLR_NMI)) {
+        if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPINTMASK)) {
+            new_mode |= PSTATE_ALLINT;
+        } else {
+            new_mode &= ~PSTATE_ALLINT;
+        }
+    }
+
     pstate_write(env, PSTATE_DAIF | new_mode);
     env->aarch64 = true;
     aarch64_restore_sp(env, new_el);
-- 
2.34.1

Re: [RFC PATCH v2 10/22] target/arm: Handle PSTATE.ALLINT on taking an exception

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Set or clear PSTATE.ALLINT on taking an exception to ELx according to the
> SCTLR_ELx.SPINTMASK bit.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/helper.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 62c8e5d611..952ea7c02a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11540,6 +11540,15 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>           }
>       }
>   
> +    if (cpu_isar_feature(aa64_nmi, cpu) &&
> +        (env->cp15.sctlr_el[new_el] & SCTLR_NMI)) {
> +        if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPINTMASK)) {
> +            new_mode |= PSTATE_ALLINT;
> +        } else {
> +            new_mode &= ~PSTATE_ALLINT;
> +        }
> +    }

new_mode starts at zero and adds state as required -- there is no need to clear allint 
here... though I see that we do the same for SSBS.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[RFC PATCH v2 11/22] target/arm: Set pstate.ALLINT in arm_cpu_reset_hold

[Jinjie Ruan via]

Set pstate.ALLINT in arm_cpu_reset_hold as daif do it.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 055670343e..e850763158 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -357,6 +357,10 @@ static void arm_cpu_reset_hold(Object *obj)
     }
     env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
 
+    if (cpu_isar_feature(aa64_nmi, cpu)) {
+        env->allint = PSTATE_ALLINT;
+    }
+
     /* AArch32 has a hard highvec setting of 0xFFFF0000.  If we are currently
      * executing as AArch32 then check if highvecs are enabled and
      * adjust the PC accordingly.
-- 
2.34.1

Re: [RFC PATCH v2 11/22] target/arm: Set pstate.ALLINT in arm_cpu_reset_hold

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Set pstate.ALLINT in arm_cpu_reset_hold as daif do it.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/cpu.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 055670343e..e850763158 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -357,6 +357,10 @@ static void arm_cpu_reset_hold(Object *obj)
>       }
>       env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
>   
> +    if (cpu_isar_feature(aa64_nmi, cpu)) {
> +        env->allint = PSTATE_ALLINT;
> +    }
> +

Reset value of ALLINT is UNKNOWN.
I think you should drop this patch.


r~

Re: [RFC PATCH v2 11/22] target/arm: Set pstate.ALLINT in arm_cpu_reset_hold

[Jinjie Ruan via]

On 2024/2/22 4:43, Richard Henderson wrote:
> On 2/21/24 03:08, Jinjie Ruan via wrote:
>> Set pstate.ALLINT in arm_cpu_reset_hold as daif do it.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>   target/arm/cpu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 055670343e..e850763158 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -357,6 +357,10 @@ static void arm_cpu_reset_hold(Object *obj)
>>       }
>>       env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
>>   +    if (cpu_isar_feature(aa64_nmi, cpu)) {
>> +        env->allint = PSTATE_ALLINT;
>> +    }
>> +
> 
> Reset value of ALLINT is UNKNOWN.
> I think you should drop this patch.

Yes. I have ignore the mannual comment.

> 
> 
> r~
> 

[RFC PATCH v2 12/22] hw/arm/virt: Wire NMI irq line from GIC to CPU

[Jinjie Ruan via]

Wire the new NMI interrupt line from the GIC to each CPU.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/arm/virt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0af1943697..c442652d0f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -804,7 +804,8 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
 
     /* Wire the outputs from each CPU's generic timer and the GICv3
      * maintenance interrupt signal to the appropriate GIC PPI inputs,
-     * and the GIC's IRQ/FIQ/VIRQ/VFIQ interrupt outputs to the CPU's inputs.
+     * and the GIC's IRQ/FIQ/VIRQ/VFIQ/NMI interrupt outputs to the CPU's
+     * inputs.
      */
     for (i = 0; i < smp_cpus; i++) {
         DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
@@ -848,6 +849,8 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
                            qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
         sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus,
                            qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
+        sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_NMI));
     }
 
     fdt_add_gic_node(vms);
-- 
2.34.1

[RFC PATCH v2 13/22] hw/intc/arm_gicv3: Add external IRQ lines for NMI

[Jinjie Ruan via]

Augment the GICv3's QOM device interface by adding one
new set of sysbus IRQ line, to signal NMI to each CPU.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/intc/arm_gicv3_common.c         | 3 +++
 include/hw/intc/arm_gic_common.h   | 1 +
 include/hw/intc/arm_gicv3_common.h | 1 +
 3 files changed, 5 insertions(+)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index cb55c72681..6249c3edc9 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -299,6 +299,9 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
     for (i = 0; i < s->num_cpu; i++) {
         sysbus_init_irq(sbd, &s->cpu[i].parent_vfiq);
     }
+    for (i = 0; i < s->num_cpu; i++) {
+        sysbus_init_irq(sbd, &s->cpu[i].parent_nmi);
+    }
 
     memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
                           "gicv3_dist", 0x10000);
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 7080375008..fc89be96d4 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -71,6 +71,7 @@ struct GICState {
     qemu_irq parent_fiq[GIC_NCPU];
     qemu_irq parent_virq[GIC_NCPU];
     qemu_irq parent_vfiq[GIC_NCPU];
+    qemu_irq parent_nmi[GIC_NCPU];
     qemu_irq maintenance_irq[GIC_NCPU];
 
     /* GICD_CTLR; for a GIC with the security extensions the NS banked version
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 4e2fb518e7..1eb8c39239 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -155,6 +155,7 @@ struct GICv3CPUState {
     qemu_irq parent_fiq;
     qemu_irq parent_virq;
     qemu_irq parent_vfiq;
+    qemu_irq parent_nmi;
 
     /* Redistributor */
     uint32_t level;                  /* Current IRQ level */
-- 
2.34.1

[RFC PATCH v2 14/22] target/arm: Handle NMI in arm_cpu_do_interrupt_aarch64()

[Jinjie Ruan via]

The NMI exception trap entry behave like IRQ or FIQ which depends on
the NMI interrupt type.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 target/arm/helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 952ea7c02a..ac5f998e32 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11466,6 +11466,13 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
     case EXCP_VFIQ:
         addr += 0x100;
         break;
+    case EXCP_NMI:
+        if (env->nmi_is_irq) {
+            addr += 0x80;
+        } else {
+            addr += 0x100;
+        }
+        break;
     case EXCP_VSERR:
         addr += 0x180;
         /* Construct the SError syndrome from IDS and ISS fields. */
-- 
2.34.1

[RFC PATCH v2 15/22] hw/intc/arm_gicv3_redist: Implement GICR_INMIR0

[Jinjie Ruan via]

Add GICR_INMIR0 register and support access GICR_INMIR0.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/intc/arm_gicv3_redist.c | 23 +++++++++++++++++++++++
 hw/intc/gicv3_internal.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index 8153525849..87e7823f34 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -35,6 +35,15 @@ static int gicr_ns_access(GICv3CPUState *cs, int irq)
     return extract32(cs->gicr_nsacr, irq * 2, 2);
 }
 
+static void gicr_write_bitmap_reg(GICv3CPUState *cs, MemTxAttrs attrs,
+                                  uint32_t *reg, uint32_t val)
+{
+    /* Helper routine to implement writing to a "set" register */
+    val &= mask_group(cs, attrs);
+    *reg = val;
+    gicv3_redist_update(cs);
+}
+
 static void gicr_write_set_bitmap_reg(GICv3CPUState *cs, MemTxAttrs attrs,
                                       uint32_t *reg, uint32_t val)
 {
@@ -406,6 +415,13 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr offset,
         *data = value;
         return MEMTX_OK;
     }
+    case GICR_INMIR0:
+        if (!cs->gic->nmi_support) {
+            *data = 0;
+            return MEMTX_OK;
+        }
+        *data = gicr_read_bitmap_reg(cs, attrs, cs->gicr_isuperprio);
+        return MEMTX_OK;
     case GICR_ICFGR0:
     case GICR_ICFGR1:
     {
@@ -555,6 +571,13 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
         gicv3_redist_update(cs);
         return MEMTX_OK;
     }
+    case GICR_INMIR0:
+        if (!cs->gic->nmi_support) {
+            return MEMTX_OK;
+        }
+        gicr_write_bitmap_reg(cs, attrs, &cs->gicr_isuperprio, value);
+        return MEMTX_OK;
+
     case GICR_ICFGR0:
         /* Register is all RAZ/WI or RAO/WI bits */
         return MEMTX_OK;
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 29d5cdc1b6..f35b7d2f03 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -109,6 +109,7 @@
 #define GICR_ICFGR1           (GICR_SGI_OFFSET + 0x0C04)
 #define GICR_IGRPMODR0        (GICR_SGI_OFFSET + 0x0D00)
 #define GICR_NSACR            (GICR_SGI_OFFSET + 0x0E00)
+#define GICR_INMIR0           (GICR_SGI_OFFSET + 0x0F80)
 
 /* VLPI redistributor registers, offsets from VLPI_base */
 #define GICR_VPROPBASER       (GICR_VLPI_OFFSET + 0x70)
-- 
2.34.1

Re: [RFC PATCH v2 15/22] hw/intc/arm_gicv3_redist: Implement GICR_INMIR0

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> +        gicr_write_bitmap_reg(cs, attrs, &cs->gicr_isuperprio, value);

Where is gicr_isuperprio defined?
Ah, incorrect patch splitting and ordering -- introduced in patch 19.


r~

[RFC PATCH v2 16/22] hw/intc/arm_gicv3: Implement GICD_INMIR

[Jinjie Ruan via]

Add GICD_INMIR0, GICD_INMIRnE register and support access GICD_INMIR0.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/intc/arm_gicv3_dist.c | 38 ++++++++++++++++++++++++++++++++++++++
 hw/intc/gicv3_internal.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index 35e850685c..2f7280c524 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -89,6 +89,29 @@ static int gicd_ns_access(GICv3State *s, int irq)
     return extract32(s->gicd_nsacr[irq / 16], (irq % 16) * 2, 2);
 }
 
+static void gicd_write_bitmap_reg(GICv3State *s, MemTxAttrs attrs,
+                                  uint32_t *bmp, maskfn *maskfn,
+                                  int offset, uint32_t val)
+{
+    /*
+     * Helper routine to implement writing to a "set" register
+     * (GICD_INMIR, etc).
+     * Semantics implemented here:
+     * RAZ/WI for SGIs, PPIs, unimplemented IRQs
+     * Bits corresponding to Group 0 or Secure Group 1 interrupts RAZ/WI.
+     * offset should be the offset in bytes of the register from the start
+     * of its group.
+     */
+    int irq = offset * 8;
+
+    if (irq < GIC_INTERNAL || irq >= s->num_irq) {
+        return;
+    }
+    val &= mask_group_and_nsacr(s, attrs, maskfn, irq);
+    *gic_bmp_ptr32(bmp, irq) = val;
+    gicv3_update(s, irq, 32);
+}
+
 static void gicd_write_set_bitmap_reg(GICv3State *s, MemTxAttrs attrs,
                                       uint32_t *bmp,
                                       maskfn *maskfn,
@@ -543,6 +566,14 @@ static bool gicd_readl(GICv3State *s, hwaddr offset,
         /* RAZ/WI since affinity routing is always enabled */
         *data = 0;
         return true;
+    case GICD_INMIR ... GICD_INMIR + 0x7f:
+        if (!s->nmi_support) {
+            *data = 0;
+            return true;
+        }
+        *data = gicd_read_bitmap_reg(s, attrs, s->superprio, NULL,
+                                     offset - GICD_INMIR);
+        return true;
     case GICD_IROUTER ... GICD_IROUTER + 0x1fdf:
     {
         uint64_t r;
@@ -752,6 +783,13 @@ static bool gicd_writel(GICv3State *s, hwaddr offset,
     case GICD_SPENDSGIR ... GICD_SPENDSGIR + 0xf:
         /* RAZ/WI since affinity routing is always enabled */
         return true;
+    case GICD_INMIR ... GICD_INMIR + 0x7f:
+        if (!s->nmi_support) {
+            return true;
+        }
+        gicd_write_bitmap_reg(s, attrs, s->superprio, NULL,
+                              offset - GICD_INMIR, value);
+        return true;
     case GICD_IROUTER ... GICD_IROUTER + 0x1fdf:
     {
         uint64_t r;
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index f35b7d2f03..a1fc34597e 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -52,6 +52,8 @@
 #define GICD_SGIR            0x0F00
 #define GICD_CPENDSGIR       0x0F10
 #define GICD_SPENDSGIR       0x0F20
+#define GICD_INMIR           0x0F80
+#define GICD_INMIRnE         0x3B00
 #define GICD_IROUTER         0x6000
 #define GICD_IDREGS          0xFFD0
 
-- 
2.34.1

Re: [RFC PATCH v2 16/22] hw/intc/arm_gicv3: Implement GICD_INMIR

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Add GICD_INMIR0, GICD_INMIRnE register and support access GICD_INMIR0.
> 
> Signed-off-by: Jinjie Ruan<ruanjinjie@huawei.com>
> ---
>   hw/intc/arm_gicv3_dist.c | 38 ++++++++++++++++++++++++++++++++++++++
>   hw/intc/gicv3_internal.h |  2 ++
>   2 files changed, 40 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[RFC PATCH v2 17/22] hw/intc: Enable FEAT_GICv3_NMI Feature

[Jinjie Ruan via]

Added properties to enable FEAT_GICv3_NMI feature, setup distributor
and redistributor registers to indicate NMI support.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/intc/arm_gicv3_common.c         | 1 +
 hw/intc/arm_gicv3_dist.c           | 2 ++
 hw/intc/gicv3_internal.h           | 1 +
 include/hw/intc/arm_gicv3_common.h | 1 +
 4 files changed, 5 insertions(+)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 6249c3edc9..9abbe9b71d 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -566,6 +566,7 @@ static Property arm_gicv3_common_properties[] = {
     DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
     DEFINE_PROP_UINT32("revision", GICv3State, revision, 3),
     DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0),
+    DEFINE_PROP_BOOL("has-nmi", GICv3State, nmi_support, 0),
     DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0),
     /*
      * Compatibility property: force 8 bits of physical priority, even
diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index 2f7280c524..65e7ca29cf 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -412,6 +412,7 @@ static bool gicd_readl(GICv3State *s, hwaddr offset,
          *                      by GICD_TYPER.IDbits)
          * MBIS == 0 (message-based SPIs not supported)
          * SecurityExtn == 1 if security extns supported
+         * NMI = 1 if Non-maskable interrupt property is supported
          * CPUNumber == 0 since for us ARE is always 1
          * ITLinesNumber == (((max SPI IntID + 1) / 32) - 1)
          */
@@ -425,6 +426,7 @@ static bool gicd_readl(GICv3State *s, hwaddr offset,
         bool dvis = s->revision >= 4;
 
         *data = (1 << 25) | (1 << 24) | (dvis << 18) | (sec_extn << 10) |
+            (s->nmi_support << GICD_TYPER_NMI_SHIFT) |
             (s->lpi_enable << GICD_TYPER_LPIS_SHIFT) |
             (0xf << 19) | itlinesnumber;
         return true;
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index a1fc34597e..8d793243f4 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -70,6 +70,7 @@
 #define GICD_CTLR_E1NWF             (1U << 7)
 #define GICD_CTLR_RWP               (1U << 31)
 
+#define GICD_TYPER_NMI_SHIFT           9
 #define GICD_TYPER_LPIS_SHIFT          17
 
 /* 16 bits EventId */
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 1eb8c39239..c9d31c7c7d 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -248,6 +248,7 @@ struct GICv3State {
     uint32_t num_irq;
     uint32_t revision;
     bool lpi_enable;
+    bool nmi_support;
     bool security_extn;
     bool force_8bit_prio;
     bool irq_reset_nonsecure;
-- 
2.34.1

[RFC PATCH v2 18/22] hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC

[Jinjie Ruan via]

Included support FEAT_GICv3_NMI feature as part of virt platform
GIC initialization.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c442652d0f..0359dbd8bd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -772,6 +772,8 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
         qdev_prop_set_array(vms->gic, "redist-region-count",
                             redist_region_count);
 
+        qdev_prop_set_bit(vms->gic, "has-nmi", true);
+
         if (!kvm_irqchip_in_kernel()) {
             if (vms->tcg_its) {
                 object_property_set_link(OBJECT(vms->gic), "sysmem",
-- 
2.34.1

Re: [RFC PATCH v2 18/22] hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Included support FEAT_GICv3_NMI feature as part of virt platform
> GIC initialization.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   hw/arm/virt.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c442652d0f..0359dbd8bd 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -772,6 +772,8 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
>           qdev_prop_set_array(vms->gic, "redist-region-count",
>                               redist_region_count);
>   
> +        qdev_prop_set_bit(vms->gic, "has-nmi", true);

This should be set based on whether the cpu class created has FEAT_NMI.


r~

[RFC PATCH v2 19/22] hw/intc/arm_gicv3: Add irq superpriority information

[Jinjie Ruan via]

A SPI, PPI or SGI interrupt can have a superpriority property. So
maintain superpriority information in PendingIrq and GICR/GICD.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 include/hw/intc/arm_gicv3_common.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index c9d31c7c7d..8f9bcdfac9 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -146,6 +146,7 @@ typedef struct {
     int irq;
     uint8_t prio;
     int grp;
+    bool superprio;
 } PendingIrq;
 
 struct GICv3CPUState {
@@ -171,6 +172,7 @@ struct GICv3CPUState {
     uint32_t gicr_ienabler0;
     uint32_t gicr_ipendr0;
     uint32_t gicr_iactiver0;
+    uint32_t gicr_isuperprio;
     uint32_t edge_trigger; /* ICFGR0 and ICFGR1 even bits */
     uint32_t gicr_igrpmodr0;
     uint32_t gicr_nsacr;
@@ -274,6 +276,7 @@ struct GICv3State {
     GIC_DECLARE_BITMAP(active);       /* GICD_ISACTIVER */
     GIC_DECLARE_BITMAP(level);        /* Current level */
     GIC_DECLARE_BITMAP(edge_trigger); /* GICD_ICFGR even bits */
+    GIC_DECLARE_BITMAP(superprio);    /* GICD_INMIR */
     uint8_t gicd_ipriority[GICV3_MAXIRQ];
     uint64_t gicd_irouter[GICV3_MAXIRQ];
     /* Cached information: pointer to the cpu i/f for the CPUs specified
@@ -313,6 +316,7 @@ GICV3_BITMAP_ACCESSORS(pending)
 GICV3_BITMAP_ACCESSORS(active)
 GICV3_BITMAP_ACCESSORS(level)
 GICV3_BITMAP_ACCESSORS(edge_trigger)
+GICV3_BITMAP_ACCESSORS(superprio)
 
 #define TYPE_ARM_GICV3_COMMON "arm-gicv3-common"
 typedef struct ARMGICv3CommonClass ARMGICv3CommonClass;
-- 
2.34.1

Re: [RFC PATCH v2 19/22] hw/intc/arm_gicv3: Add irq superpriority information

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> A SPI, PPI or SGI interrupt can have a superpriority property. So
> maintain superpriority information in PendingIrq and GICR/GICD.
> 
> Signed-off-by: Jinjie Ruan<ruanjinjie@huawei.com>
> ---
>   include/hw/intc/arm_gicv3_common.h | 4 ++++
>   1 file changed, 4 insertions(+)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

Though this patch is mis-ordered compared to its uses.


r~

[RFC PATCH v2 20/22] hw/intc/arm_gicv3: Add NMI handling CPU interface registers

[Jinjie Ruan via]

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;
+}
+
+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);
     }
 
     if (!gicv3_intid_is_special(intid)) {
@@ -1555,7 +1589,7 @@ static uint64_t icc_hppir1_read(CPUARMState *env, const ARMCPRegInfo *ri)
         return icv_hppir_read(env, ri);
     }
 
-    value = icc_hppir1_value(cs, env);
+    value = icc_hppir1_value(cs, env, false, true);
     trace_gicv3_icc_hppir1_read(gicv3_redist_affid(cs), value);
     return value;
 }
@@ -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,
+    },
     { .name = "ICC_EOIR1_EL1", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 12, .opc2 = 1,
       .type = ARM_CP_IO | ARM_CP_NO_RAW,
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 8d793243f4..93e56b3726 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -511,6 +511,7 @@ FIELD(VTE, RDBASE, 42, RDBASE_PROCNUM_LENGTH)
 /* Special interrupt IDs */
 #define INTID_SECURE 1020
 #define INTID_NONSECURE 1021
+#define INTID_NMI 1022
 #define INTID_SPURIOUS 1023
 
 /* Functions internal to the emulated GICv3 */
-- 
2.34.1

[RFC PATCH v2 21/22] hw/intc/arm_gicv3: Implement NMI interrupt prioirty

[Jinjie Ruan via]

If GICD_CTLR_DS bit is zero and the NMI is non-secure, the NMI prioirty
is higher than 0x80, otherwise it is higher than 0x0. And save NMI
super prioirty information in hppi.superprio to deliver NMI exception.
Since both GICR and GICD can deliver NMI, it is both necessary to check
whether the pending irq is NMI in gicv3_redist_update_noirqset and
gicv3_update_noirqset. And In irqbetter(), only a non-NMI with the same
priority and a smaller interrupt number can be preempted but not NMI.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/intc/arm_gicv3.c | 61 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
index 0b8f79a122..e3281cffdc 100644
--- a/hw/intc/arm_gicv3.c
+++ b/hw/intc/arm_gicv3.c
@@ -21,7 +21,7 @@
 #include "hw/intc/arm_gicv3.h"
 #include "gicv3_internal.h"
 
-static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t prio)
+static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t prio, bool is_nmi)
 {
     /* Return true if this IRQ at this priority should take
      * precedence over the current recorded highest priority
@@ -33,11 +33,21 @@ static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t prio)
     if (prio < cs->hppi.prio) {
         return true;
     }
+
+    /*
+     * Current highest prioirity pending interrupt is not a NMI
+     * and the new IRQ is a NMI with same priority.
+     */
+    if (prio == cs->hppi.prio && !cs->hppi.superprio && is_nmi) {
+        return true;
+    }
+
     /* If multiple pending interrupts have the same priority then it is an
      * IMPDEF choice which of them to signal to the CPU. We choose to
      * signal the one with the lowest interrupt number.
      */
-    if (prio == cs->hppi.prio && irq <= cs->hppi.irq) {
+    if (prio == cs->hppi.prio && !cs->hppi.superprio &&
+        !is_nmi && irq <= cs->hppi.irq) {
         return true;
     }
     return false;
@@ -141,6 +151,8 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs)
     uint8_t prio;
     int i;
     uint32_t pend;
+    bool is_nmi = 0;
+    uint32_t superprio = 0;
 
     /* Find out which redistributor interrupts are eligible to be
      * signaled to the CPU interface.
@@ -152,10 +164,26 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs)
             if (!(pend & (1 << i))) {
                 continue;
             }
-            prio = cs->gicr_ipriorityr[i];
-            if (irqbetter(cs, i, prio)) {
+            superprio = extract32(cs->gicr_isuperprio, i, 1);
+
+            /* NMI */
+            if (superprio) {
+                is_nmi = 1;
+
+                /* DS = 0 & Non-secure NMI */
+                if ((!(cs->gic->gicd_ctlr & GICD_CTLR_DS)) &&
+                    extract32(cs->gicr_igroupr0, i, 1))
+                    prio = 0x80;
+                else
+                    prio = 0x0;
+            } else {
+               is_nmi = 0;
+               prio = cs->gicr_ipriorityr[i];
+            }
+            if (irqbetter(cs, i, prio, is_nmi)) {
                 cs->hppi.irq = i;
                 cs->hppi.prio = prio;
+                cs->hppi.superprio = is_nmi;
                 seenbetter = true;
             }
         }
@@ -168,7 +196,7 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs)
     if ((cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) && cs->gic->lpi_enable &&
         (cs->gic->gicd_ctlr & GICD_CTLR_EN_GRP1NS) &&
         (cs->hpplpi.prio != 0xff)) {
-        if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) {
+        if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio, false)) {
             cs->hppi.irq = cs->hpplpi.irq;
             cs->hppi.prio = cs->hpplpi.prio;
             cs->hppi.grp = cs->hpplpi.grp;
@@ -212,7 +240,9 @@ static void gicv3_update_noirqset(GICv3State *s, int start, int len)
 {
     int i;
     uint8_t prio;
+    bool is_nmi = 0;
     uint32_t pend = 0;
+    uint32_t superprio = 0;
 
     assert(start >= GIC_INTERNAL);
     assert(len > 0);
@@ -240,10 +270,27 @@ static void gicv3_update_noirqset(GICv3State *s, int start, int len)
              */
             continue;
         }
-        prio = s->gicd_ipriority[i];
-        if (irqbetter(cs, i, prio)) {
+
+        superprio = *gic_bmp_ptr32(s->superprio, i);
+        /* NMI */
+        if (superprio & (1 << (i & 0x1f))) {
+            is_nmi = 1;
+
+            /* DS = 0 & Non-secure NMI */
+            if ((!(s->gicd_ctlr & GICD_CTLR_DS)) &&
+                gicv3_gicd_group_test(s, i))
+                    prio = 0x80;
+            else
+                    prio = 0x0;
+        } else {
+            is_nmi = 0;
+            prio = s->gicd_ipriority[i];
+        }
+
+        if (irqbetter(cs, i, prio, is_nmi)) {
             cs->hppi.irq = i;
             cs->hppi.prio = prio;
+            cs->hppi.superprio = is_nmi;
             cs->seenbetter = true;
         }
     }
-- 
2.34.1

[RFC PATCH v2 22/22] hw/intc/arm_gicv3: Report the NMI interrupt in gicv3_cpuif_update()

[Jinjie Ruan via]

In CPU Interface, if the IRQ or FIQ has the superpriority property, report
NMI to the corresponding PE and record the NMI interrupt type.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/intc/arm_gicv3_cpuif.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index f5bf8df32b..87bde1f897 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -931,6 +931,7 @@ void gicv3_cpuif_update(GICv3CPUState *cs)
     /* Tell the CPU about its highest priority pending interrupt */
     int irqlevel = 0;
     int fiqlevel = 0;
+    int nmilevel = 0;
     ARMCPU *cpu = ARM_CPU(cs->cpu);
     CPUARMState *env = &cpu->env;
 
@@ -967,7 +968,10 @@ void gicv3_cpuif_update(GICv3CPUState *cs)
             g_assert_not_reached();
         }
 
-        if (isfiq) {
+        if (cs->hppi.superprio) {
+            nmilevel = 1;
+            env->nmi_is_irq = !isfiq;
+        } else if (isfiq) {
             fiqlevel = 1;
         } else {
             irqlevel = 1;
@@ -978,6 +982,7 @@ void gicv3_cpuif_update(GICv3CPUState *cs)
 
     qemu_set_irq(cs->parent_fiq, fiqlevel);
     qemu_set_irq(cs->parent_irq, irqlevel);
+    qemu_set_irq(cs->parent_nmi, nmilevel);
 }
 
 static uint64_t icc_pmr_read(CPUARMState *env, const ARMCPRegInfo *ri)
-- 
2.34.1