| Message ID | 20210825075620.2607-2-longpeng2@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | optimize the downtime for vfio migration | expand |
On Wed, 25 Aug 2021 15:56:16 +0800 "Longpeng(Mike)" <longpeng2@huawei.com> wrote: > The main difference of the failure path in vfio_msi_enable and > vfio_msi_disable_common is enable INTX or not. > > Extend the vfio_msi_disable_common to provide a arg to decide "an arg" > whether need to fallback, and then we can use this helper to > instead the redundant code in vfio_msi_enable. Do you mean s/instead/replace/? > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > --- > hw/vfio/pci.c | 34 ++++++++++++---------------------- > 1 file changed, 12 insertions(+), 22 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index e1ea1d8..7cc43fe 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -47,6 +47,7 @@ > > 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, bool enable_intx); > > /* > * Disabling BAR mmaping can be slow, but toggling it around INTx can > @@ -650,29 +651,17 @@ retry: > if (ret) { > if (ret < 0) { > error_report("vfio: Error: Failed to setup MSI fds: %m"); > - } else if (ret != vdev->nr_vectors) { This part of the change is subtle and not mentioned in the commit log. It does seem unnecessary to test against this specific return value since any positive return is an error indicating the number of vectors we should retry with, but this change should be described in a separate patch. > + } else { > error_report("vfio: Error: Failed to enable %d " > "MSI vectors, retry with %d", vdev->nr_vectors, ret); > } > > - for (i = 0; i < vdev->nr_vectors; i++) { > - VFIOMSIVector *vector = &vdev->msi_vectors[i]; > - if (vector->virq >= 0) { > - vfio_remove_kvm_msi_virq(vector); > - } > - qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt), > - NULL, NULL, NULL); > - event_notifier_cleanup(&vector->interrupt); > - } > - > - g_free(vdev->msi_vectors); > - vdev->msi_vectors = NULL; > + vfio_msi_disable_common(vdev, false); > > - if (ret > 0 && ret != vdev->nr_vectors) { > + if (ret > 0) { > vdev->nr_vectors = ret; > goto retry; > } > - vdev->nr_vectors = 0; > > /* > * Failing to setup MSI doesn't really fall within any specification. > @@ -680,7 +669,6 @@ retry: > * out to fall back to INTx for this device. > */ > error_report("vfio: Error: Failed to enable MSI"); > - vdev->interrupt = VFIO_INT_NONE; > > return; > } > @@ -688,7 +676,7 @@ retry: > trace_vfio_msi_enable(vdev->vbasedev.name, vdev->nr_vectors); > } > > -static void vfio_msi_disable_common(VFIOPCIDevice *vdev) > +static void vfio_msi_disable_common(VFIOPCIDevice *vdev, bool enable_intx) I'd rather avoid these sort of modal options to functions where we can. Maybe this suggests instead that re-enabling INTx should be removed from the common helper and callers needing to do so should do it outside of the common helper. Thanks, Alex > { > Error *err = NULL; > int i; > @@ -710,9 +698,11 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev) > vdev->nr_vectors = 0; > vdev->interrupt = VFIO_INT_NONE; > > - vfio_intx_enable(vdev, &err); > - if (err) { > - error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + if (enable_intx) { > + vfio_intx_enable(vdev, &err); > + if (err) { > + error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + } > } > } > > @@ -737,7 +727,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev) > vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX); > } > > - vfio_msi_disable_common(vdev); > + vfio_msi_disable_common(vdev, true); > > memset(vdev->msix->pending, 0, > BITS_TO_LONGS(vdev->msix->entries) * sizeof(unsigned long)); > @@ -748,7 +738,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev) > static void vfio_msi_disable(VFIOPCIDevice *vdev) > { > vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSI_IRQ_INDEX); > - vfio_msi_disable_common(vdev); > + vfio_msi_disable_common(vdev, true); > > trace_vfio_msi_disable(vdev->vbasedev.name); > }
在 2021/9/4 5:55, Alex Williamson 写道: > On Wed, 25 Aug 2021 15:56:16 +0800 > "Longpeng(Mike)" <longpeng2@huawei.com> wrote: > >> The main difference of the failure path in vfio_msi_enable and >> vfio_msi_disable_common is enable INTX or not. >> >> Extend the vfio_msi_disable_common to provide a arg to decide > > "an arg" > Thanks a lot, I'll fix all of the grammatical errors and typos in this version together. >> whether need to fallback, and then we can use this helper to >> instead the redundant code in vfio_msi_enable. > > Do you mean s/instead/replace/? > >> >> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> >> --- >> hw/vfio/pci.c | 34 ++++++++++++---------------------- >> 1 file changed, 12 insertions(+), 22 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index e1ea1d8..7cc43fe 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -47,6 +47,7 @@ >> >> 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, bool enable_intx); >> >> /* >> * Disabling BAR mmaping can be slow, but toggling it around INTx can >> @@ -650,29 +651,17 @@ retry: >> if (ret) { >> if (ret < 0) { >> error_report("vfio: Error: Failed to setup MSI fds: %m"); >> - } else if (ret != vdev->nr_vectors) { > > This part of the change is subtle and not mentioned in the commit log. > It does seem unnecessary to test against this specific return value > since any positive return is an error indicating the number of vectors > we should retry with, but this change should be described in a separate > patch. > Ok, thanks, I'll split in the next version. >> + } else { >> error_report("vfio: Error: Failed to enable %d " >> "MSI vectors, retry with %d", vdev->nr_vectors, ret); >> } >> >> - for (i = 0; i < vdev->nr_vectors; i++) { >> - VFIOMSIVector *vector = &vdev->msi_vectors[i]; >> - if (vector->virq >= 0) { >> - vfio_remove_kvm_msi_virq(vector); >> - } >> - qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt), >> - NULL, NULL, NULL); >> - event_notifier_cleanup(&vector->interrupt); >> - } >> - >> - g_free(vdev->msi_vectors); >> - vdev->msi_vectors = NULL; >> + vfio_msi_disable_common(vdev, false); >> >> - if (ret > 0 && ret != vdev->nr_vectors) { >> + if (ret > 0) { >> vdev->nr_vectors = ret; >> goto retry; >> } >> - vdev->nr_vectors = 0; >> >> /* >> * Failing to setup MSI doesn't really fall within any specification. >> @@ -680,7 +669,6 @@ retry: >> * out to fall back to INTx for this device. >> */ >> error_report("vfio: Error: Failed to enable MSI"); >> - vdev->interrupt = VFIO_INT_NONE; >> >> return; >> } >> @@ -688,7 +676,7 @@ retry: >> trace_vfio_msi_enable(vdev->vbasedev.name, vdev->nr_vectors); >> } >> >> -static void vfio_msi_disable_common(VFIOPCIDevice *vdev) >> +static void vfio_msi_disable_common(VFIOPCIDevice *vdev, bool enable_intx) > > I'd rather avoid these sort of modal options to functions where we can. > Maybe this suggests instead that re-enabling INTx should be removed > from the common helper and callers needing to do so should do it > outside of the common helper. Thanks, > Ok, thanks. > Alex > > >> { >> Error *err = NULL; >> int i; >> @@ -710,9 +698,11 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev) >> vdev->nr_vectors = 0; >> vdev->interrupt = VFIO_INT_NONE; >> >> - vfio_intx_enable(vdev, &err); >> - if (err) { >> - error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); >> + if (enable_intx) { >> + vfio_intx_enable(vdev, &err); >> + if (err) { >> + error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); >> + } >> } >> } >> >> @@ -737,7 +727,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev) >> vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX); >> } >> >> - vfio_msi_disable_common(vdev); >> + vfio_msi_disable_common(vdev, true); >> >> memset(vdev->msix->pending, 0, >> BITS_TO_LONGS(vdev->msix->entries) * sizeof(unsigned long)); >> @@ -748,7 +738,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev) >> static void vfio_msi_disable(VFIOPCIDevice *vdev) >> { >> vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSI_IRQ_INDEX); >> - vfio_msi_disable_common(vdev); >> + vfio_msi_disable_common(vdev, true); >> >> trace_vfio_msi_disable(vdev->vbasedev.name); >> } > > . >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e1ea1d8..7cc43fe 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -47,6 +47,7 @@ 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, bool enable_intx); /* * Disabling BAR mmaping can be slow, but toggling it around INTx can @@ -650,29 +651,17 @@ retry: if (ret) { if (ret < 0) { error_report("vfio: Error: Failed to setup MSI fds: %m"); - } else if (ret != vdev->nr_vectors) { + } else { error_report("vfio: Error: Failed to enable %d " "MSI vectors, retry with %d", vdev->nr_vectors, ret); } - for (i = 0; i < vdev->nr_vectors; i++) { - VFIOMSIVector *vector = &vdev->msi_vectors[i]; - if (vector->virq >= 0) { - vfio_remove_kvm_msi_virq(vector); - } - qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt), - NULL, NULL, NULL); - event_notifier_cleanup(&vector->interrupt); - } - - g_free(vdev->msi_vectors); - vdev->msi_vectors = NULL; + vfio_msi_disable_common(vdev, false); - if (ret > 0 && ret != vdev->nr_vectors) { + if (ret > 0) { vdev->nr_vectors = ret; goto retry; } - vdev->nr_vectors = 0; /* * Failing to setup MSI doesn't really fall within any specification. @@ -680,7 +669,6 @@ retry: * out to fall back to INTx for this device. */ error_report("vfio: Error: Failed to enable MSI"); - vdev->interrupt = VFIO_INT_NONE; return; } @@ -688,7 +676,7 @@ retry: trace_vfio_msi_enable(vdev->vbasedev.name, vdev->nr_vectors); } -static void vfio_msi_disable_common(VFIOPCIDevice *vdev) +static void vfio_msi_disable_common(VFIOPCIDevice *vdev, bool enable_intx) { Error *err = NULL; int i; @@ -710,9 +698,11 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev) vdev->nr_vectors = 0; vdev->interrupt = VFIO_INT_NONE; - vfio_intx_enable(vdev, &err); - if (err) { - error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); + if (enable_intx) { + vfio_intx_enable(vdev, &err); + if (err) { + error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); + } } } @@ -737,7 +727,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev) vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX); } - vfio_msi_disable_common(vdev); + vfio_msi_disable_common(vdev, true); memset(vdev->msix->pending, 0, BITS_TO_LONGS(vdev->msix->entries) * sizeof(unsigned long)); @@ -748,7 +738,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev) static void vfio_msi_disable(VFIOPCIDevice *vdev) { vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSI_IRQ_INDEX); - vfio_msi_disable_common(vdev); + vfio_msi_disable_common(vdev, true); trace_vfio_msi_disable(vdev->vbasedev.name); }
The main difference of the failure path in vfio_msi_enable and vfio_msi_disable_common is enable INTX or not. Extend the vfio_msi_disable_common to provide a arg to decide whether need to fallback, and then we can use this helper to instead the redundant code in vfio_msi_enable. Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> --- hw/vfio/pci.c | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-)