preserve pending interrupts during cpr on arm

[PATCH V1 2/2] hw/intc/arm_gicv3_kvm: preserve pending interrupts during cpr

Posted by Steve Sistare 4 months ago

Close a race condition that causes cpr-transfer to lose VFIO
interrupts on ARM.

CPR stops VCPUs but does not disable VFIO interrupts, which may continue
to arrive throughout the transition to new QEMU.

CPR calls kvm_irqchip_remove_irqfd_notifier_gsi in old QEMU to force
future interrupts to the producer eventfd, where they are preserved.
Old QEMU then destroys the old KVM instance.  However, interrupts may
already be pended in KVM state.  To preserve them, call ioctl
KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES to flush them to guest RAM, where
they will be picked up when the new KVM+VCPU instance is created.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/intc/arm_gicv3_kvm.c            | 16 +++++++++++++++-
 include/hw/intc/arm_gicv3_common.h |  3 +++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 43cba6e3f1..15245af2fd 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -30,6 +30,7 @@
 #include "gicv3_internal.h"
 #include "vgic_common.h"
 #include "migration/blocker.h"
+#include "migration/misc.h"
 #include "qom/object.h"
 #include "target/arm/cpregs.h"
 
@@ -783,6 +784,15 @@ static void vm_change_state_handler(void *opaque, bool running,
     }
 }
 
+static int kvm_arm_gicv3_notifier(NotifierWithReturn *notifier,
+                                  MigrationEvent *e, Error **errp)
+{
+    if (e->type == MIG_EVENT_PRECOPY_DONE) {
+        GICv3State *s = container_of(notifier, GICv3State, cpr_notifier);
+        kvm_arm_save_pending_tables(s);
+    }
+    return 0;
+}
 
 static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
 {
@@ -889,13 +899,17 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
                                GICD_CTLR)) {
         error_setg(&s->migration_blocker, "This operating system kernel does "
                                           "not support vGICv3 migration");
-        if (migrate_add_blocker(&s->migration_blocker, errp) < 0) {
+        if (migrate_add_blocker_modes(&s->migration_blocker, MIG_MODE_NORMAL,
+                                      MIG_MODE_CPR_TRANSFER, errp) < 0) {
             return;
         }
     }
     if (kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
                               KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES)) {
         qemu_add_vm_change_state_handler(vm_change_state_handler, s);
+        migration_add_notifier_mode(&s->cpr_notifier,
+                                    kvm_arm_gicv3_notifier,
+                                    MIG_MODE_CPR_TRANSFER);
     }
 }
 
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index a3d6a0e507..75bbc403c7 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -27,6 +27,7 @@
 #include "hw/sysbus.h"
 #include "hw/intc/arm_gic_common.h"
 #include "qom/object.h"
+#include "qemu/notify.h"
 
 /*
  * Maximum number of possible interrupts, determined by the GIC architecture.
@@ -270,6 +271,8 @@ struct GICv3State {
     GICv3CPUState *cpu;
     /* List of all ITSes connected to this GIC */
     GPtrArray *itslist;
+
+    NotifierWithReturn cpr_notifier;
 };
 
 #define GICV3_BITMAP_ACCESSORS(BMP)                                     \
-- 
2.39.3

Re: [PATCH V1 2/2] hw/intc/arm_gicv3_kvm: preserve pending interrupts during cpr

Posted by Peter Maydell 4 months ago

On Mon, 14 Jul 2025 at 15:29, Steve Sistare <steven.sistare@oracle.com> wrote:
>
> Close a race condition that causes cpr-transfer to lose VFIO
> interrupts on ARM.
>
> CPR stops VCPUs but does not disable VFIO interrupts, which may continue
> to arrive throughout the transition to new QEMU.
>
> CPR calls kvm_irqchip_remove_irqfd_notifier_gsi in old QEMU to force
> future interrupts to the producer eventfd, where they are preserved.
> Old QEMU then destroys the old KVM instance.  However, interrupts may
> already be pended in KVM state.  To preserve them, call ioctl
> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES to flush them to guest RAM, where
> they will be picked up when the new KVM+VCPU instance is created.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  hw/intc/arm_gicv3_kvm.c            | 16 +++++++++++++++-
>  include/hw/intc/arm_gicv3_common.h |  3 +++
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 43cba6e3f1..15245af2fd 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -30,6 +30,7 @@
>  #include "gicv3_internal.h"
>  #include "vgic_common.h"
>  #include "migration/blocker.h"
> +#include "migration/misc.h"
>  #include "qom/object.h"
>  #include "target/arm/cpregs.h"
>
> @@ -783,6 +784,15 @@ static void vm_change_state_handler(void *opaque, bool running,
>      }
>  }
>
> +static int kvm_arm_gicv3_notifier(NotifierWithReturn *notifier,
> +                                  MigrationEvent *e, Error **errp)
> +{
> +    if (e->type == MIG_EVENT_PRECOPY_DONE) {
> +        GICv3State *s = container_of(notifier, GICv3State, cpr_notifier);
> +        kvm_arm_save_pending_tables(s);

This kvm_arm_gicv3_notifier() function reports an error via
an Error pointer, and the function we call inside
kvm_arm_save_pending_tables() can report errors via an
Error pointer. So I think kvm_arm_save_pending_tables()
should propagate the Error up, not ignore it.

(Or if there's a good reason we should silently ignore
the error here, we should have a comment saying why.)

> +    }
> +    return 0;
> +}

thanks
-- PMM

Re: [PATCH V1 2/2] hw/intc/arm_gicv3_kvm: preserve pending interrupts during cpr

Posted by Steven Sistare 4 months ago

On 7/14/2025 10:32 AM, Peter Maydell wrote:
> On Mon, 14 Jul 2025 at 15:29, Steve Sistare <steven.sistare@oracle.com> wrote:
>>
>> Close a race condition that causes cpr-transfer to lose VFIO
>> interrupts on ARM.
>>
>> CPR stops VCPUs but does not disable VFIO interrupts, which may continue
>> to arrive throughout the transition to new QEMU.
>>
>> CPR calls kvm_irqchip_remove_irqfd_notifier_gsi in old QEMU to force
>> future interrupts to the producer eventfd, where they are preserved.
>> Old QEMU then destroys the old KVM instance.  However, interrupts may
>> already be pended in KVM state.  To preserve them, call ioctl
>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES to flush them to guest RAM, where
>> they will be picked up when the new KVM+VCPU instance is created.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   hw/intc/arm_gicv3_kvm.c            | 16 +++++++++++++++-
>>   include/hw/intc/arm_gicv3_common.h |  3 +++
>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> index 43cba6e3f1..15245af2fd 100644
>> --- a/hw/intc/arm_gicv3_kvm.c
>> +++ b/hw/intc/arm_gicv3_kvm.c
>> @@ -30,6 +30,7 @@
>>   #include "gicv3_internal.h"
>>   #include "vgic_common.h"
>>   #include "migration/blocker.h"
>> +#include "migration/misc.h"
>>   #include "qom/object.h"
>>   #include "target/arm/cpregs.h"
>>
>> @@ -783,6 +784,15 @@ static void vm_change_state_handler(void *opaque, bool running,
>>       }
>>   }
>>
>> +static int kvm_arm_gicv3_notifier(NotifierWithReturn *notifier,
>> +                                  MigrationEvent *e, Error **errp)
>> +{
>> +    if (e->type == MIG_EVENT_PRECOPY_DONE) {
>> +        GICv3State *s = container_of(notifier, GICv3State, cpr_notifier);
>> +        kvm_arm_save_pending_tables(s);
> 
> This kvm_arm_gicv3_notifier() function reports an error via
> an Error pointer, and the function we call inside
> kvm_arm_save_pending_tables() can report errors via an
> Error pointer. So I think kvm_arm_save_pending_tables()
> should propagate the Error up, not ignore it.
> 
> (Or if there's a good reason we should silently ignore
> the error here, we should have a comment saying why.)

No good reason, I was just mirroring the behavior of vm_change_state_handler.
I'll propagate the error.

- Steve

Re: [PATCH V1 2/2] hw/intc/arm_gicv3_kvm: preserve pending interrupts during cpr

Posted by Steven Sistare 4 months ago

On 7/14/2025 10:51 AM, Steven Sistare wrote:
> On 7/14/2025 10:32 AM, Peter Maydell wrote:
>> On Mon, 14 Jul 2025 at 15:29, Steve Sistare <steven.sistare@oracle.com> wrote:
>>>
>>> Close a race condition that causes cpr-transfer to lose VFIO
>>> interrupts on ARM.
>>>
>>> CPR stops VCPUs but does not disable VFIO interrupts, which may continue
>>> to arrive throughout the transition to new QEMU.
>>>
>>> CPR calls kvm_irqchip_remove_irqfd_notifier_gsi in old QEMU to force
>>> future interrupts to the producer eventfd, where they are preserved.
>>> Old QEMU then destroys the old KVM instance.  However, interrupts may
>>> already be pended in KVM state.  To preserve them, call ioctl
>>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES to flush them to guest RAM, where
>>> they will be picked up when the new KVM+VCPU instance is created.
>>>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>>   hw/intc/arm_gicv3_kvm.c            | 16 +++++++++++++++-
>>>   include/hw/intc/arm_gicv3_common.h |  3 +++
>>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>>> index 43cba6e3f1..15245af2fd 100644
>>> --- a/hw/intc/arm_gicv3_kvm.c
>>> +++ b/hw/intc/arm_gicv3_kvm.c
>>> @@ -30,6 +30,7 @@
>>>   #include "gicv3_internal.h"
>>>   #include "vgic_common.h"
>>>   #include "migration/blocker.h"
>>> +#include "migration/misc.h"
>>>   #include "qom/object.h"
>>>   #include "target/arm/cpregs.h"
>>>
>>> @@ -783,6 +784,15 @@ static void vm_change_state_handler(void *opaque, bool running,
>>>       }
>>>   }
>>>
>>> +static int kvm_arm_gicv3_notifier(NotifierWithReturn *notifier,
>>> +                                  MigrationEvent *e, Error **errp)
>>> +{
>>> +    if (e->type == MIG_EVENT_PRECOPY_DONE) {
>>> +        GICv3State *s = container_of(notifier, GICv3State, cpr_notifier);
>>> +        kvm_arm_save_pending_tables(s);
>>
>> This kvm_arm_gicv3_notifier() function reports an error via
>> an Error pointer, and the function we call inside
>> kvm_arm_save_pending_tables() can report errors via an
>> Error pointer. So I think kvm_arm_save_pending_tables()
>> should propagate the Error up, not ignore it.
>>
>> (Or if there's a good reason we should silently ignore
>> the error here, we should have a comment saying why.)
> 
> No good reason, I was just mirroring the behavior of vm_change_state_handler.
> I'll propagate the error.

The kvm_arm_save_pending_tables becomes trivial, so I will just delete it, and
delete patch 1.  I will submit a V2 for patch 2 alone shortly.

- Steve