Re: [PATCH v1 5/6] hw/arm/virt: Enable backup bitmap for dirty ring

[Peter Maydell]

On Mon, 13 Feb 2023 at 00:40, Gavin Shan <gshan@redhat.com> wrote:
>
> When KVM device "kvm-arm-gicv3" or "arm-its-kvm" is used, we have to
> enable the backup bitmap for the dirty ring. Otherwise, the migration
> will fail because those two devices are using the backup bitmap to track
> dirty guest memory, corresponding to various hardware tables.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/arm/virt.c        | 26 ++++++++++++++++++++++++++
>  target/arm/kvm64.c   | 25 +++++++++++++++++++++++++
>  target/arm/kvm_arm.h | 12 ++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 75f28947de..ea9bd98a65 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2024,6 +2024,8 @@ static void machvirt_init(MachineState *machine)
>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      const CPUArchIdList *possible_cpus;
> +    const char *gictype = NULL;
> +    const char *itsclass = NULL;
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *secure_sysmem = NULL;
>      MemoryRegion *tag_sysmem = NULL;
> @@ -2071,6 +2073,30 @@ static void machvirt_init(MachineState *machine)
>       */
>      finalize_gic_version(vms);
>
> +    /*
> +     * When "kvm-arm-gicv3" or "arm-its-kvm" is used, the backup dirty
> +     * bitmap has to be enabled for KVM dirty ring, before any memory
> +     * slot is added. Otherwise, the migration will fail with the dirty
> +     * ring.
> +     */
> +    if (kvm_enabled()) {
> +        if (vms->gic_version != VIRT_GIC_VERSION_2) {
> +            gictype = gicv3_class_name();
> +        }
> +
> +        if (vms->gic_version != VIRT_GIC_VERSION_2 && vms->its) {
> +            itsclass = its_class_name();
> +        }
> +
> +        if (((gictype && !strcmp(gictype, "kvm-arm-gicv3")) ||
> +             (itsclass && !strcmp(itsclass, "arm-its-kvm"))) &&
> +            !kvm_arm_enable_dirty_ring_with_bitmap()) {
> +            error_report("Failed to enable the backup bitmap for "
> +                         "KVM dirty ring");
> +            exit(1);
> +        }
> +    }

Why does this need to be board-specific code? Is there
some way we can just do the right thing automatically?
Why does the GIC/ITS matter?

The kernel should already know whether we have asked it
to do something that needs this extra extension, so
I think we ought to be able in the generic "enable the
dirty ring" code say "if the kernel says we need this
extra thing, also enable this extra thing". Or if that's
too early, we can do the extra part in a generic hook a
bit later.

In the future there might be other things, presumably,
that need the backup bitmap, so it would be more future
proof not to need to also change QEMU to add extra
logic checks that duplicate the logic the kernel already has.

thanks
-- PMM