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

[Jinjie Ruan]

On 2024/3/28 22:54, Peter Maydell wrote:
> 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.

The input opaque pointer seems not same, one is "GICv3CPUState *", but
another is "GICv3State *"

> 
>> +
>> +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