Re: [PATCH v10 14/23] hw/intc/arm_gicv3: Add irq non-maskable property

[Peter Maydell]

On Mon, 25 Mar 2024 at 08:52, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
> A SPI, PPI or SGI interrupt can have non-maskable property. So maintain
> non-maskable property in PendingIrq and GICR/GICD. Since add new device
> state, it also needs to be migrated, so also save NMI info in
> vmstate_gicv3_cpu and vmstate_gicv3.
>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> v10:
> - superprio -> nmi, gicr_isuperprio -> gicr_inmir0.
> - Save NMI state in vmstate_gicv3_cpu and vmstate_gicv3.
> - Update the commit message.
> v3:
> - Place this ahead of implement GICR_INMIR.
> - Add Acked-by.
> ---
>  hw/intc/arm_gicv3_common.c         | 44 ++++++++++++++++++++++++++++++
>  include/hw/intc/arm_gicv3_common.h |  4 +++
>  2 files changed, 48 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 2d2cea6858..be76ae0be6 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -164,6 +164,24 @@ const VMStateDescription vmstate_gicv3_gicv4 = {
>      }
>  };
>
> +static bool nmi_needed(void *opaque)
> +{
> +    GICv3CPUState *cs = opaque;
> +
> +    return cs->gic->nmi_support != 0;

nmi_support is already a bool, so you can
       return cs->gic_nmi_support;


> +}
> +
> +static const VMStateDescription vmstate_gicv3_cpu_nmi = {
> +    .name = "arm_gicv3_cpu/nmi",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = nmi_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT32(gicr_inmir0, GICv3CPUState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_gicv3_cpu = {
>      .name = "arm_gicv3_cpu",
>      .version_id = 1,
> @@ -197,6 +215,10 @@ static const VMStateDescription vmstate_gicv3_cpu = {
>          &vmstate_gicv3_cpu_sre_el1,
>          &vmstate_gicv3_gicv4,
>          NULL
> +    },
> +    .subsections = (const VMStateDescription * const []) {
> +        &vmstate_gicv3_cpu_nmi,
> +        NULL

You add your subsection to the existing .subsections[] list.
Otherwise this field initializer overwrites the previous one.

>      }
>  };
>
> @@ -238,6 +260,24 @@ const VMStateDescription 
> vmstate_gicv3_gicd_no_migration_shift_bug = {
>      }
>  };
>
> +static bool needed_nmi(void *opaque)
> +{
> +    GICv3State *cs = opaque;
> +
> +    return cs->nmi_support != 0;
> +}

You already have nmi_needed() above, so you can use it
as the .needed function for both vmstate struct definitions.

> +
> +const VMStateDescription vmstate_gicv3_gicd_nmi = {
> +    .name = "arm_gicv3/gicd_nmi",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = needed_nmi,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(nmi, GICv3State, GICV3_BMP_SIZE),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_gicv3 = {
>      .name = "arm_gicv3",
>      .version_id = 1,
> @@ -267,6 +307,10 @@ static const VMStateDescription vmstate_gicv3 = {
>      .subsections = (const VMStateDescription * const []) {
>          &vmstate_gicv3_gicd_no_migration_shift_bug,
>          NULL
> +    },
> +    .subsections = (const VMStateDescription * const []) {
> +        &vmstate_gicv3_gicd_nmi,
> +        NULL

Similarly here this must be added to the existing list.

>      }
>  };
>
> diff --git a/include/hw/intc/arm_gicv3_common.h 
> b/include/hw/intc/arm_gicv3_common.h
> index 4358c5319c..88533749eb 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 nmi;
>  } PendingIrq;
>
>  struct GICv3CPUState {
> @@ -172,6 +173,7 @@ struct GICv3CPUState {
>      uint32_t gicr_ienabler0;
>      uint32_t gicr_ipendr0;
>      uint32_t gicr_iactiver0;
> +    uint32_t gicr_inmir0;
>      uint32_t edge_trigger; /* ICFGR0 and ICFGR1 even bits */
>      uint32_t gicr_igrpmodr0;
>      uint32_t gicr_nsacr;
> @@ -275,6 +277,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(nmi);          /* 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
> @@ -314,6 +317,7 @@ GICV3_BITMAP_ACCESSORS(pending)
>  GICV3_BITMAP_ACCESSORS(active)
>  GICV3_BITMAP_ACCESSORS(level)
>  GICV3_BITMAP_ACCESSORS(edge_trigger)
> +GICV3_BITMAP_ACCESSORS(nmi)
>
>  #define TYPE_ARM_GICV3_COMMON "arm-gicv3-common"
>  typedef struct ARMGICv3CommonClass ARMGICv3CommonClass;
> --
> 2.34.1

thanks
-- PMM