The GICD_TYPER2 register is new for GICv4.1.  As an ID register, we
migrate it so that on the destination the kernel can check that the
destination supports the same configuration that the source system
had.  This avoids confusing behaviour if the user tries to migrate a
VM from a GICv3 system to a GICv4.1 system or vice-versa.  (In
practice the inability to migrate between different CPU types
probably already prevented this.)

Note that older kernels pre-dating GICv4.1 support in the KVM GIC
will just ignore whatever userspace writes to GICD_TYPER2, so
migration where the destination is one of those kernels won't have
this safety-check.

(The reporting of a mismatch will be a bit clunky, because this
file currently uses error_abort for all failures to write the
data to the kernel. Ideally we would fix this by making them
propagate the failure information back up to the post_load hook
return value, to cause a migration failure.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/intc/arm_gicv3_common.h |  6 ++++++
 hw/intc/arm_gicv3_common.c         | 24 ++++++++++++++++++++++++
 hw/intc/arm_gicv3_kvm.c            |  6 ++++++
 3 files changed, 36 insertions(+)

diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index a3d6a0e5077..bcbcae1fbe7 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -267,6 +267,12 @@ struct GICv3State {
     GICv3CPUState *gicd_irouter_target[GICV3_MAXIRQ];
     uint32_t gicd_nsacr[DIV_ROUND_UP(GICV3_MAXIRQ, 16)];
 
+    /*
+     * GICv4.1 extended ID information. This is currently only needed
+     * for migration of a KVM GIC.
+     */
+    uint32_t gicd_typer2;
+
     GICv3CPUState *cpu;
     /* List of all ITSes connected to this GIC */
     GPtrArray *itslist;
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 1cee68193ca..7b09771310e 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -275,6 +275,29 @@ const VMStateDescription vmstate_gicv3_gicd_nmi = {
     }
 };
 
+static bool gicv3_typer2_needed(void *opaque)
+{
+    /*
+     * GICD_TYPER2 ID register for GICv4.1. Was RES0 for
+     * GICv3 and GICv4.0; don't migrate if zero for backwards
+     * compatibility.
+     */
+    GICv3State *cs = opaque;
+
+    return cs->gicd_typer2 != 0;
+}
+
+const VMStateDescription vmstate_gicv3_gicd_typer2 = {
+    .name = "arm_gicv3/gicd_typer2",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = gicv3_typer2_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT32(gicd_typer2, GICv3State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_gicv3 = {
     .name = "arm_gicv3",
     .version_id = 1,
@@ -304,6 +327,7 @@ static const VMStateDescription vmstate_gicv3 = {
     .subsections = (const VMStateDescription * const []) {
         &vmstate_gicv3_gicd_no_migration_shift_bug,
         &vmstate_gicv3_gicd_nmi,
+        &vmstate_gicv3_gicd_typer2,
         NULL
     }
 };
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 3be3bf6c28d..0302beb0c07 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -332,6 +332,9 @@ static void kvm_arm_gicv3_put(GICv3State *s)
     kvm_gicr_access(s, GICR_TYPER + 4, 0, &regh, false);
     redist_typer = ((uint64_t)regh << 32) | regl;
 
+    reg = s->gicd_typer2;
+    kvm_gicd_access(s, GICD_TYPER2, &reg, true);
+
     reg = s->gicd_ctlr;
     kvm_gicd_access(s, GICD_CTLR, &reg, true);
 
@@ -519,6 +522,9 @@ static void kvm_arm_gicv3_get(GICv3State *s)
     kvm_gicr_access(s, GICR_TYPER + 4, 0, &regh, false);
     redist_typer = ((uint64_t)regh << 32) | regl;
 
+    kvm_gicd_access(s, GICD_TYPER2, &reg, false);
+    s->gicd_typer2 = reg;
+
     kvm_gicd_access(s, GICD_CTLR, &reg, false);
     s->gicd_ctlr = reg;
 
-- 
2.43.0

Hi Peter,

On 7/7/25 6:10 PM, Peter Maydell wrote:
> The GICD_TYPER2 register is new for GICv4.1.  As an ID register, we
> migrate it so that on the destination the kernel can check that the
> destination supports the same configuration that the source system
> had.  This avoids confusing behaviour if the user tries to migrate a
> VM from a GICv3 system to a GICv4.1 system or vice-versa.  (In
> practice the inability to migrate between different CPU types
> probably already prevented this.)
>
> Note that older kernels pre-dating GICv4.1 support in the KVM GIC
> will just ignore whatever userspace writes to GICD_TYPER2, so
> migration where the destination is one of those kernels won't have
> this safety-check.
>
> (The reporting of a mismatch will be a bit clunky, because this
> file currently uses error_abort for all failures to write the
> data to the kernel. Ideally we would fix this by making them
> propagate the failure information back up to the post_load hook
> return value, to cause a migration failure.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/intc/arm_gicv3_common.h |  6 ++++++
>  hw/intc/arm_gicv3_common.c         | 24 ++++++++++++++++++++++++
>  hw/intc/arm_gicv3_kvm.c            |  6 ++++++
>  3 files changed, 36 insertions(+)
>
> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
> index a3d6a0e5077..bcbcae1fbe7 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -267,6 +267,12 @@ struct GICv3State {
>      GICv3CPUState *gicd_irouter_target[GICV3_MAXIRQ];
>      uint32_t gicd_nsacr[DIV_ROUND_UP(GICV3_MAXIRQ, 16)];
>  
> +    /*
> +     * GICv4.1 extended ID information. This is currently only needed
> +     * for migration of a KVM GIC.
> +     */
> +    uint32_t gicd_typer2;
> +
>      GICv3CPUState *cpu;
>      /* List of all ITSes connected to this GIC */
>      GPtrArray *itslist;
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 1cee68193ca..7b09771310e 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -275,6 +275,29 @@ const VMStateDescription vmstate_gicv3_gicd_nmi = {
>      }
>  };
>  
> +static bool gicv3_typer2_needed(void *opaque)
> +{
> +    /*
> +     * GICD_TYPER2 ID register for GICv4.1. Was RES0 for
> +     * GICv3 and GICv4.0; don't migrate if zero for backwards
> +     * compatibility.
> +     */
> +    GICv3State *cs = opaque;
> +
> +    return cs->gicd_typer2 != 0;
> +}
> +
> +const VMStateDescription vmstate_gicv3_gicd_typer2 = {
> +    .name = "arm_gicv3/gicd_typer2",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = gicv3_typer2_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT32(gicd_typer2, GICv3State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_gicv3 = {
>      .name = "arm_gicv3",
>      .version_id = 1,
> @@ -304,6 +327,7 @@ static const VMStateDescription vmstate_gicv3 = {
>      .subsections = (const VMStateDescription * const []) {
>          &vmstate_gicv3_gicd_no_migration_shift_bug,
>          &vmstate_gicv3_gicd_nmi,
> +        &vmstate_gicv3_gicd_typer2,
>          NULL
>      }
>  };
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 3be3bf6c28d..0302beb0c07 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -332,6 +332,9 @@ static void kvm_arm_gicv3_put(GICv3State *s)
>      kvm_gicr_access(s, GICR_TYPER + 4, 0, &regh, false);
>      redist_typer = ((uint64_t)regh << 32) | regl;
>  
> +    reg = s->gicd_typer2;
> +    kvm_gicd_access(s, GICD_TYPER2, &reg, true);
> +
I don't know if there is any rationale about the access order. I see
most of the dist reg accesses are done below in the function after rdist
ones, although the GICD_CTLR is also here. If there is a rationale a
comment may be useful to avoid another dev to do something wrong. I
remember that for the ITS for instance some speicifc ordering needed to
be enforced.

While at it you can also fix the missing bracket at
 /* Distributor state (shared between all CPUs */

same remark on the guest side and same missing bracket ;-)
>      reg = s->gicd_ctlr;
>      kvm_gicd_access(s, GICD_CTLR, &reg, true);
>  
> @@ -519,6 +522,9 @@ static void kvm_arm_gicv3_get(GICv3State *s)
>      kvm_gicr_access(s, GICR_TYPER + 4, 0, &regh, false);
>      redist_typer = ((uint64_t)regh << 32) | regl;
>  
> +    kvm_gicd_access(s, GICD_TYPER2, &reg, false);
> +    s->gicd_typer2 = reg;
> +
>      kvm_gicd_access(s, GICD_CTLR, &reg, false);
>      s->gicd_ctlr = reg;
>  
Others looks good to me

Eric

On Mon, 7 Jul 2025 at 18:25, Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Peter,
>
> On 7/7/25 6:10 PM, Peter Maydell wrote:
> > The GICD_TYPER2 register is new for GICv4.1.  As an ID register, we
> > migrate it so that on the destination the kernel can check that the
> > destination supports the same configuration that the source system
> > had.  This avoids confusing behaviour if the user tries to migrate a
> > VM from a GICv3 system to a GICv4.1 system or vice-versa.  (In
> > practice the inability to migrate between different CPU types
> > probably already prevented this.)
> >
> > Note that older kernels pre-dating GICv4.1 support in the KVM GIC
> > will just ignore whatever userspace writes to GICD_TYPER2, so
> > migration where the destination is one of those kernels won't have
> > this safety-check.
> >
> > (The reporting of a mismatch will be a bit clunky, because this
> > file currently uses error_abort for all failures to write the
> > data to the kernel. Ideally we would fix this by making them
> > propagate the failure information back up to the post_load hook
> > return value, to cause a migration failure.)


> > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> > index 3be3bf6c28d..0302beb0c07 100644
> > --- a/hw/intc/arm_gicv3_kvm.c
> > +++ b/hw/intc/arm_gicv3_kvm.c
> > @@ -332,6 +332,9 @@ static void kvm_arm_gicv3_put(GICv3State *s)
> >      kvm_gicr_access(s, GICR_TYPER + 4, 0, &regh, false);
> >      redist_typer = ((uint64_t)regh << 32) | regl;
> >
> > +    reg = s->gicd_typer2;
> > +    kvm_gicd_access(s, GICD_TYPER2, &reg, true);
> > +
> I don't know if there is any rationale about the access order. I see
> most of the dist reg accesses are done below in the function after rdist
> ones, although the GICD_CTLR is also here. If there is a rationale a
> comment may be useful to avoid another dev to do something wrong. I
> remember that for the ITS for instance some speicifc ordering needed to
> be enforced.

I think in general we have tried to put in comments to note where
the ordering matters. (Though I do suspect the GICD_CTLR
write may be early because we need to write the enable bit
before doing anything else -- if that's true, we failed
to document it.)

In this case I just put this one at the top because "bail out
early if the config is wrong" seemed better than doing it late.
But it doesn't matter and it's probably more sensible to do
it with the other GICD registers. A comment that we only
write this ID register to validate the config would also help.

> While at it you can also fix the missing bracket at
>  /* Distributor state (shared between all CPUs */

Sure.

thanks
-- PMM

On Mon, 7 Jul 2025 at 18:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 7 Jul 2025 at 18:25, Eric Auger <eric.auger@redhat.com> wrote:
> >
> > Hi Peter,
> >
> > On 7/7/25 6:10 PM, Peter Maydell wrote:
> > > The GICD_TYPER2 register is new for GICv4.1.  As an ID register, we
> > > migrate it so that on the destination the kernel can check that the
> > > destination supports the same configuration that the source system
> > > had.  This avoids confusing behaviour if the user tries to migrate a
> > > VM from a GICv3 system to a GICv4.1 system or vice-versa.  (In
> > > practice the inability to migrate between different CPU types
> > > probably already prevented this.)
> > >
> > > Note that older kernels pre-dating GICv4.1 support in the KVM GIC
> > > will just ignore whatever userspace writes to GICD_TYPER2, so
> > > migration where the destination is one of those kernels won't have
> > > this safety-check.
> > >
> > > (The reporting of a mismatch will be a bit clunky, because this
> > > file currently uses error_abort for all failures to write the
> > > data to the kernel. Ideally we would fix this by making them
> > > propagate the failure information back up to the post_load hook
> > > return value, to cause a migration failure.)
>
>
> > > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> > > index 3be3bf6c28d..0302beb0c07 100644
> > > --- a/hw/intc/arm_gicv3_kvm.c
> > > +++ b/hw/intc/arm_gicv3_kvm.c
> > > @@ -332,6 +332,9 @@ static void kvm_arm_gicv3_put(GICv3State *s)
> > >      kvm_gicr_access(s, GICR_TYPER + 4, 0, &regh, false);
> > >      redist_typer = ((uint64_t)regh << 32) | regl;
> > >
> > > +    reg = s->gicd_typer2;
> > > +    kvm_gicd_access(s, GICD_TYPER2, &reg, true);
> > > +
> > I don't know if there is any rationale about the access order. I see
> > most of the dist reg accesses are done below in the function after rdist
> > ones, although the GICD_CTLR is also here. If there is a rationale a
> > comment may be useful to avoid another dev to do something wrong. I
> > remember that for the ITS for instance some speicifc ordering needed to
> > be enforced.
>
> I think in general we have tried to put in comments to note where
> the ordering matters. (Though I do suspect the GICD_CTLR
> write may be early because we need to write the enable bit
> before doing anything else -- if that's true, we failed
> to document it.)
>
> In this case I just put this one at the top because "bail out
> early if the config is wrong" seemed better than doing it late.
> But it doesn't matter and it's probably more sensible to do
> it with the other GICD registers. A comment that we only
> write this ID register to validate the config would also help.

Also I realised that this probably fails on a GICv4.1 host, because
I think I have the reset handling wrong. On reset the gicd_typer2
field in the struct will be zero, and then the reset_hold function
calls kvm_arm_gicv3_put(), which will try to write the 0 into the
kernel, which will fail.

-- PMM