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

[Gavin Shan <gshan@redhat.com>]

On 2/22/23 3:27 AM, Peter Maydell wrote:
> 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.
> 

When the dirty ring is enabled, a per-vcpu buffer is used to track the dirty pages.
The prerequisite to use the per-vcpu buffer is existing running VCPU context. There
are two cases where no running VCPU context exists and the backup bitmap extension
is needed, as we know until now: (a) save/restore GICv3 tables; (b) save/restore ITS
tables; These two cases are related to KVM device "kvm-arm-gicv3" and "arm-its-kvm",
which are only needed by virt machine at present. So we needn't the backup bitmap
extension for other boards.

The host kernel always exports the capability KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP
for ARM64. The capability isn't exported for x86 because we needn't it there. The
generic path to enable the extension would be in kvm_init(). I think the extension
is enabled unconditionally if it has been exported by host kernel. It means there
will be unnecessary overhead to synchronize the backup bitmap when the aforementioned
KVM devices aren't used, but the overhead should be very small and acceptable. The
only concern is host kernel has to allocate the backup bitmap, which is totally no
use. Please let me know your thoughts, Peter.


qemu_init
   qemu_create_machine
     :
     :
   configure_accelerators
     do_configure_accelerator
       accel_init_machine
         kvm_init                            // Where the dirty ring is eanbled. Would be best
           kvm_arch_init                     // place to enable the backup bitmap extension regardless
     :                                       // of 'kvm-arm-gicv3' and 'arm-its-kvm' are used
     :
   qmp_x_exit_preconfig
     qemu_init_board
       machine_run_board_init
         machvirt_init                      // memory slots are added here and where we know the KVM devices
     :                                      // are used
     :
   accel_setup_post                         // no backend for KVM yet, xen only

Note that the capability KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP can't be enabled if
the dirty ring isn't enabled or memory slots have been added.

Thanks,
Gavin