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

[Steven Sistare <steven.sistare@oracle.com>]

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