Re: [PATCH v2 2/3] hw/intc/arm_gicv3_kvm: Migrate GICD_TYPER2
Hi Peter,
On 7/8/25 1:52 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
Looks good to me
Reviewed-by: Eric Auger
Eric
> ---
> v1->v2:
> * fix comment missing bracket
> * fix reset handling so this works on GICv4.1 hosts
> * move get/put code to be with the other GICD regs
> ---
> include/hw/intc/arm_gicv3_common.h | 6 ++
> hw/intc/arm_gicv3_common.c | 24
> hw/intc/arm_gicv3_kvm.c| 25 +++--
> 3 files changed, 53 insertions(+), 2 deletions(-)
>
> 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..8e34d08b415 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -405,7 +405,16 @@ static void kvm_arm_gicv3_put(GICv3State *s)
> }
> }
>
> -/* Distributor state (shared between all CPUs */
> +/* Distributor state (shared between all CPUs) */
> +
> +/*
> + * This ID register is restored so that the kernel can fail
> + * the write if the migration source is GICv4.1 but the
> + * destination is not.
> + */
> +reg = s->gicd_typer2;
> +kvm_gicd_access(s, GICD_TYPER2, ®, true);
> +
> reg = s->gicd_statusr[GICV3_NS];
> kvm_gicd_access(s, GICD_STATUSR, ®, true);
>
> @@ -572,7 +581,10 @@ static void kvm_arm_gicv3_get(GICv3State *s)
> }
> }
>
> -/* Distributor state (shared between all CPUs */
> +/* Distributor state (shared between all CPUs) */
> +
> +kvm_gicd_access(s, GICD_TYPER2, ®, false);
> +s->gicd_typer2 = reg;
>
> kvm_gicd_access(s, GICD_STATUSR, ®, false);
> s->gicd_statusr[GICV3_NS] = reg;
> @@ -707,6 +719,7 @@ static void kvm_arm_gicv3_reset_hold(Object *obj,
> ResetType type)
> {
> GICv3State *s = ARM_GICV3_COMMON(obj);
> KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> +uint32_t reg;
>
> DPRINTF("Reset\n");
>
> @@ -719,6 +732,14 @@ static void kvm_arm_gicv3_reset_hold(Object *obj,
> ResetType type)
> return;
> }
>
> +/*
Re: [PATCH v2 2/3] hw/intc/arm_gicv3_kvm: Migrate GICD_TYPER2
Hi Peter,
On 7/8/25 1:52 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
Looks good to me
Reviewed-by: Eric Auger
Eric
> ---
> v1->v2:
> * fix comment missing bracket
> * fix reset handling so this works on GICv4.1 hosts
> * move get/put code to be with the other GICD regs
> ---
> include/hw/intc/arm_gicv3_common.h | 6 ++
> hw/intc/arm_gicv3_common.c | 24
> hw/intc/arm_gicv3_kvm.c| 25 +++--
> 3 files changed, 53 insertions(+), 2 deletions(-)
>
> 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..8e34d08b415 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -405,7 +405,16 @@ static void kvm_arm_gicv3_put(GICv3State *s)
> }
> }
>
> -/* Distributor state (shared between all CPUs */
> +/* Distributor state (shared between all CPUs) */
> +
> +/*
> + * This ID register is restored so that the kernel can fail
> + * the write if the migration source is GICv4.1 but the
> + * destination is not.
> + */
> +reg = s->gicd_typer2;
> +kvm_gicd_access(s, GICD_TYPER2, ®, true);
> +
> reg = s->gicd_statusr[GICV3_NS];
> kvm_gicd_access(s, GICD_STATUSR, ®, true);
>
> @@ -572,7 +581,10 @@ static void kvm_arm_gicv3_get(GICv3State *s)
> }
> }
>
> -/* Distributor state (shared between all CPUs */
> +/* Distributor state (shared between all CPUs) */
> +
> +kvm_gicd_access(s, GICD_TYPER2, ®, false);
> +s->gicd_typer2 = reg;
>
> kvm_gicd_access(s, GICD_STATUSR, ®, false);
> s->gicd_statusr[GICV3_NS] = reg;
> @@ -707,6 +719,7 @@ static void kvm_arm_gicv3_reset_hold(Object *obj,
> ResetType type)
> {
> GICv3State *s = ARM_GICV3_COMMON(obj);
> KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> +uint32_t reg;
>
> DPRINTF("Reset\n");
>
> @@ -719,6 +732,14 @@ static void kvm_arm_gicv3_reset_hold(Object *obj,
> ResetType type)
> return;
> }
>
> +/*
