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 <peter.mayd...@linaro.org>
Looks good to me
Reviewed-by: Eric Auger <eric.au...@redhat.com>
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;
> }
>
> + /*
> + * The reset value of the GICD_TYPER2 ID register should be whatever
> + * the kernel says it is; otherwise the attempt to put the value
> + * in kvm_arm_gicv3_put() will fail.
> + */
> + kvm_gicd_access(s, GICD_TYPER2, ®, false);
> + s->gicd_typer2 = reg;
> +
> kvm_arm_gicv3_put(s);
> }
>
