[V9,32/46] vfio-pci: cpr part 2 (msi)

Finish cpr for vfio-pci MSI/MSI-X devices by preserving eventfd's and
vector state.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/vfio/pci.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 118 insertions(+), 1 deletion(-)

On 7/5/2023 4:56 AM, Kunkun Jiang wrote:
> Hi Steve,
> 
> I have a few questions about the msi part of the vfio device.
> In the reboot mode, you mentioned "The guest drivers' suspend methods
> flush outstanding requests and re-initialize the devices". This means,
> during the downtime,the vfio device dose not generate interrupts. So
> no special processing is required for the msi interrupt of the vfio
> device. Am I right?

Correct.

> In the cpr-exec mode, will the vfio device be "stopped"? If the vfio device
> is running all the time,it is possible to generate interrrupts during the
> downtime. How to deal with these interrupts?

The vfio device is not stopped, but its connection to the kvm instance is severed.
Interrupts are pended in the vfio kernel state, and that state is preserved across
exec, by preserving the vfio descriptors.  After exec, qemu creates a new kvm instance,
attaches vfio to it, and the interrupts are delivered.

> In addition, ARM GICv4 provides support for the direct injection of vLPIs.
> Interrupts are more difficult to handle. In this case, what should be done?

I have not studied this feature or tried it.

- Steve

> 
> Look forward to your reply.
> 
> Kunkun Jiang
> 
> On 2022/7/27 0:10, Steve Sistare wrote:
>> Finish cpr for vfio-pci MSI/MSI-X devices by preserving eventfd's and
>> vector state.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   hw/vfio/pci.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index b5fd2ec..1d0e8db 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -54,17 +54,47 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
>>   static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
>>   static void vfio_msi_disable_common(VFIOPCIDevice *vdev);
>>   +#define EVENT_FD_NAME(vdev, name)   \
>> +    g_strdup_printf("%s_%s", (vdev)->vbasedev.name, (name))
>> +
>> +static void save_event_fd(VFIOPCIDevice *vdev, const char *name, int nr,
>> +                          EventNotifier *ev)
>> +{
>> +    int fd = event_notifier_get_fd(ev);
>> +
>> +    if (fd >= 0) {
>> +        g_autofree char *fdname = EVENT_FD_NAME(vdev, name);
>> +
>> +        cpr_resave_fd(fdname, nr, fd);
>> +    }
>> +}
>> +
>> +static int load_event_fd(VFIOPCIDevice *vdev, const char *name, int nr)
>> +{
>> +    g_autofree char *fdname = EVENT_FD_NAME(vdev, name);
>> +    return cpr_find_fd(fdname, nr);
>> +}
>> +
>> +static void delete_event_fd(VFIOPCIDevice *vdev, const char *name, int nr)
>> +{
>> +    g_autofree char *fdname = EVENT_FD_NAME(vdev, name);
>> +    cpr_delete_fd(fdname, nr);
>> +}
>> +
>>   /* Create new or reuse existing eventfd */
>>   static int vfio_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e,
>>                                 const char *name, int nr)
>>   {
>> -    int fd = -1;   /* placeholder until a subsequent patch */
>>       int ret = 0;
>> +    int fd = load_event_fd(vdev, name, nr);
>>         if (fd >= 0) {
>>           event_notifier_init_fd(e, fd);
>>       } else {
>>           ret = event_notifier_init(e, 0);
>> +        if (!ret) {
>> +            save_event_fd(vdev, name, nr, e);
>> +        }
>>       }
>>       return ret;
>>   }
>> @@ -72,6 +102,7 @@ static int vfio_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e,
>>   static void vfio_notifier_cleanup(VFIOPCIDevice *vdev, EventNotifier *e,
>>                                     const char *name, int nr)
>>   {
>> +    delete_event_fd(vdev, name, nr);
>>       event_notifier_cleanup(e);
>>   }
>>   @@ -512,6 +543,15 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>>       VFIOMSIVector *vector;
>>       int ret;
>>   +    /*
>> +     * Ignore the callback from msix_set_vector_notifiers during resume.
>> +     * The necessary subset of these actions is called from vfio_claim_vectors
>> +     * during post load.
>> +     */
>> +    if (vdev->vbasedev.reused) {
>> +        return 0;
>> +    }
>> +
>>       trace_vfio_msix_vector_do_use(vdev->vbasedev.name, nr);
>>         vector = &vdev->msi_vectors[nr];
>> @@ -2784,6 +2824,11 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
>>       fd = event_notifier_get_fd(&vdev->err_notifier);
>>       qemu_set_fd_handler(fd, vfio_err_notifier_handler, NULL, vdev);
>>   +    /* Do not alter irq_signaling during vfio_realize for cpr */
>> +    if (vdev->vbasedev.reused) {
>> +        return;
>> +    }
>> +
>>       if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
>>                                  VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
>>           error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> @@ -2849,6 +2894,12 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
>>       fd = event_notifier_get_fd(&vdev->req_notifier);
>>       qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev);
>>   +    /* Do not alter irq_signaling during vfio_realize for cpr */
>> +    if (vdev->vbasedev.reused) {
>> +        vdev->req_enabled = true;
>> +        return;
>> +    }
>> +
>>       if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
>>                              VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
>>           error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> @@ -3357,6 +3408,46 @@ static Property vfio_pci_dev_properties[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   +static void vfio_claim_vectors(VFIOPCIDevice *vdev, int nr_vectors, bool msix)
>> +{
>> +    int i, fd;
>> +    bool pending = false;
>> +    PCIDevice *pdev = &vdev->pdev;
>> +
>> +    vdev->nr_vectors = nr_vectors;
>> +    vdev->msi_vectors = g_new0(VFIOMSIVector, nr_vectors);
>> +    vdev->interrupt = msix ? VFIO_INT_MSIX : VFIO_INT_MSI;
>> +
>> +    vfio_prepare_kvm_msi_virq_batch(vdev);
>> +
>> +    for (i = 0; i < nr_vectors; i++) {
>> +        VFIOMSIVector *vector = &vdev->msi_vectors[i];
>> +
>> +        fd = load_event_fd(vdev, "interrupt", i);
>> +        if (fd >= 0) {
>> +            vfio_vector_init(vdev, i);
>> +            qemu_set_fd_handler(fd, vfio_msi_interrupt, NULL, vector);
>> +        }
>> +
>> +        if (load_event_fd(vdev, "kvm_interrupt", i) >= 0) {
>> +            vfio_add_kvm_msi_virq(vdev, vector, i, msix);
>> +        } else {
>> +            vdev->msi_vectors[i].virq = -1;
>> +        }
>> +
>> +        if (msix && msix_is_pending(pdev, i) && msix_is_masked(pdev, i)) {
>> +            set_bit(i, vdev->msix->pending);
>> +            pending = true;
>> +        }
>> +    }
>> +
>> +    vfio_commit_kvm_msi_virq_batch(vdev);
>> +
>> +    if (msix) {
>> +        memory_region_set_enabled(&pdev->msix_pba_mmio, pending);
>> +    }
>> +}
>> +
>>   /*
>>    * The kernel may change non-emulated config bits.  Exclude them from the
>>    * changed-bits check in get_pci_config_device.
>> @@ -3375,6 +3466,29 @@ static int vfio_pci_pre_load(void *opaque)
>>       return 0;
>>   }
>>   +static int vfio_pci_post_load(void *opaque, int version_id)
>> +{
>> +    VFIOPCIDevice *vdev = opaque;
>> +    PCIDevice *pdev = &vdev->pdev;
>> +    int nr_vectors;
>> +
>> +    if (msix_enabled(pdev)) {
>> +        msix_set_vector_notifiers(pdev, vfio_msix_vector_use,
>> +                                   vfio_msix_vector_release, NULL);
>> +        nr_vectors = vdev->msix->entries;
>> +        vfio_claim_vectors(vdev, nr_vectors, true);
>> +
>> +    } else if (msi_enabled(pdev)) {
>> +        nr_vectors = msi_nr_vectors_allocated(pdev);
>> +        vfio_claim_vectors(vdev, nr_vectors, false);
>> +
>> +    } else if (vfio_pci_read_config(pdev, PCI_INTERRUPT_PIN, 1)) {
>> +        assert(0);      /* completed in a subsequent patch */
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static bool vfio_pci_needed(void *opaque)
>>   {
>>       return migrate_mode() == MIG_MODE_CPR_EXEC;
>> @@ -3386,8 +3500,11 @@ static const VMStateDescription vfio_pci_vmstate = {
>>       .minimum_version_id = 0,
>>       .priority = MIG_PRI_VFIO_PCI,       /* must load before container */
>>       .pre_load = vfio_pci_pre_load,
>> +    .post_load = vfio_pci_post_load,
>>       .needed = vfio_pci_needed,
>>       .fields = (VMStateField[]) {
>> +        VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>> +        VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };

On Thu, 13 Jul 2023 13:35:57 +0100,
Kunkun Jiang <jiangkunkun@huawei.com> wrote:
> 
> For ARM, it will first send a DISCARD command to ITS and then
> establish the interrupt reporting channel for GICv3. The DISCARD
> will remove the pending interrupt. Interrupts that come before
> channel re-establishment are silently discarded.  Do you guys have
> any good ideas?

I'm missing the context, but if you're worried about interrupts that
are lost between the DISCARD and the MAPTI commands, the only way to
solve the problem is to inject a spurious interrupt after the MAPTI
has taken place.

If it hurts, don't do that.

	M.