Re: [PATCH v10 14/23] hw/intc/arm_gicv3: Add irq non-maskable property
On Sat, 30 Mar 2024 at 01:42, Jinjie Ruan wrote:
>
>
>
> On 2024/3/28 22:54, Peter Maydell wrote:
> > On Mon, 25 Mar 2024 at 08:52, Jinjie Ruan 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
> >> Acked-by: Richard Henderson
> >> ---
> >> 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
Re: [PATCH v10 14/23] hw/intc/arm_gicv3: Add irq non-maskable property
On 2024/3/28 22:54, Peter Maydell wrote:
> On Mon, 25 Mar 2024 at 08:52, Jinjie Ruan 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
>> Acked-by: Richard Henderson
>> ---
>> 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 = {
>> _gicv3_cpu_sre_el1,
>> _gicv3_gicv4,
>> NULL
>> +},
>> +.subsections = (const VMStateDescription * const []) {
>> +_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 []) {
>> _gicv3_gicd_no_migration_shift_bug,
>> NULL
>> +},
>> +.subsections = (const VMStateDescription * const []) {
>> +_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
Re: [PATCH v10 14/23] hw/intc/arm_gicv3: Add irq non-maskable property
On Mon, 25 Mar 2024 at 08:52, Jinjie Ruan 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
> Acked-by: Richard Henderson
> ---
> 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 = {
> _gicv3_cpu_sre_el1,
> _gicv3_gicv4,
> NULL
> +},
> +.subsections = (const VMStateDescription * const []) {
> +_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 []) {
> _gicv3_gicd_no_migration_shift_bug,
> NULL
> +},
> +.subsections = (const VMStateDescription * const []) {
> +_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
[PATCH v10 14/23] hw/intc/arm_gicv3: Add irq non-maskable property
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
Acked-by: Richard Henderson
---
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;
+}
+
+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 = {
_gicv3_cpu_sre_el1,
_gicv3_gicv4,
NULL
+},
+.subsections = (const VMStateDescription * const []) {
+_gicv3_cpu_nmi,
+NULL
}
};
@@ -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;
+}
+
+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 []) {
_gicv3_gicd_no_migration_shift_bug,
NULL
+},
+.subsections = (const VMStateDescription * const []) {
+_gicv3_gicd_nmi,
+NULL
}
};
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
