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

[Peter Maydell]

On Sat, 30 Mar 2024 at 01:42, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> 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;
> >
> >
> >> +}

> >> @@ -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 *"

Oops, you're right. In that case let's give these two
functions names that hopefully guide the reader towards
the difference, like gic_cpu_state_nmi_needed() and
gic_state_nmi_needed().

thanks
-- PMM