Hi Alex,
> On July 28, 2023 1:25 AM, Alex Williamson <alex.william...@redhat.com> wrote:
>
> On Thu, 27 Jul 2023 03:24:09 -0400
> Jing Liu <jing2....@intel.com> wrote:
>
> > The vector_use callback is used to enable vector that is unmasked in
> > guest. The kernel used to only support static MSI-X allocation. When
> > allocating a new interrupt using "static MSI-X allocation" kernels,
> > Qemu first disables all previously allocated vectors and then
> > re-allocates all including the new one. The nr_vectors of
> > VFIOPCIDevice indicates that all vectors from 0 to nr_vectors are
> > allocated (and may be enabled), which is used to to loop all the
> > possibly used vectors When, e.g., disabling MSI-X interrupts.
> >
> > Extend the vector_use function to support dynamic MSI-X allocation
> > when host supports the capability. Qemu therefore can individually
> > allocate and enable a new interrupt without affecting others or
> > causing interrupts lost during runtime.
> >
> > Utilize nr_vectors to calculate the upper bound of enabled vectors in
> > dynamic MSI-X allocation mode since looping all msix_entries_nr is not
> > efficient and unnecessary.
> >
> > Signed-off-by: Jing Liu <jing2....@intel.com>
> > Tested-by: Reinette Chatre <reinette.cha...@intel.com>
> > ---
> > hw/vfio/pci.c | 40 +++++++++++++++++++++++++++-------------
> > 1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
> > 0c4ac0873d40..8c485636445c 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -512,12 +512,20 @@ static int vfio_msix_vector_do_use(PCIDevice
> *pdev, unsigned int nr,
> > }
> >
> > /*
> > - * We don't want to have the host allocate all possible MSI vectors
> > - * for a device if they're not in use, so we shutdown and incrementally
> > - * increase them as needed.
> > + * When dynamic allocation is not supported, we don't want to have the
> > + * host allocate all possible MSI vectors for a device if they're not
> > + * in use, so we shutdown and incrementally increase them as needed.
> > + * And nr_vectors stands for the number of vectors being allocated.
>
> "nr_vectors represents the total number of vectors allocated."
Will change.
>
> > + *
> > + * When dynamic allocation is supported, let the host only allocate
> > + * and enable a vector when it is in use in guest. nr_vectors stands
> > + * for the upper bound of vectors being enabled (but not all of the
> > + * ranges is allocated or enabled).
>
> s/stands for/represents/
Will change.
>
> > */
> > - if (vdev->nr_vectors < nr + 1) {
> > + if ((vdev->msix->irq_info_flags & VFIO_IRQ_INFO_NORESIZE) &&
>
> Testing vdev->msix->noresize would be cleaner.
>
> > + (vdev->nr_vectors < nr + 1)) {
> > vdev->nr_vectors = nr + 1;
> > +
> > if (!vdev->defer_kvm_irq_routing) {
> > vfio_disable_irqindex(&vdev->vbasedev,
> > VFIO_PCI_MSIX_IRQ_INDEX);
> > ret = vfio_enable_vectors(vdev, true); @@ -529,16 +537,22
> > @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
> > Error *err = NULL;
> > int32_t fd;
> >
> > - if (vector->virq >= 0) {
> > - fd = event_notifier_get_fd(&vector->kvm_interrupt);
> > - } else {
> > - fd = event_notifier_get_fd(&vector->interrupt);
> > - }
> > + if (!vdev->defer_kvm_irq_routing) {
> > + if (vector->virq >= 0) {
> > + fd = event_notifier_get_fd(&vector->kvm_interrupt);
> > + } else {
> > + fd = event_notifier_get_fd(&vector->interrupt);
> > + }
> >
> > - if (vfio_set_irq_signaling(&vdev->vbasedev,
> > - VFIO_PCI_MSIX_IRQ_INDEX, nr,
> > - VFIO_IRQ_SET_ACTION_TRIGGER, fd,
> > &err)) {
> > - error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> > + if (vfio_set_irq_signaling(&vdev->vbasedev,
> > + VFIO_PCI_MSIX_IRQ_INDEX, nr,
> > + VFIO_IRQ_SET_ACTION_TRIGGER, fd,
> > &err)) {
> > + error_reportf_err(err, VFIO_MSG_PREFIX,
> > vdev->vbasedev.name);
> > + }
> > + }
> > + /* Increase for dynamic allocation case. */
> > + if (vdev->nr_vectors < nr + 1) {
> > + vdev->nr_vectors = nr + 1;
> > }
>
> We now have two branches where the bulk of the code is skipped when
> defer_kvm_irq_routing is enabled and doing effectively the same update to
> nr_vectors otherwise. This suggests we should move the
> defer_kvm_irq_routing test out and create a common place to update
> nr_vectors. Thanks,
I make a new logic as follows that moves the defer_kvm_irq_routing test out.
Since the vfio_enable_vectors() function need an updated nr_vectors value
so need first update and test the different conditions using old value,
e.g. old_nr_vec.
int old_nr_vec = vdev->nr_vectors;
...
...
if (vdev->nr_vectors < nr + 1) {
vdev->nr_vectors = nr + 1;
}
if (!vdev->defer_kvm_irq_routing) {
if (vdev->msix->noresize && (old_nr_vec < nr + 1)) {
vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
ret = vfio_enable_vectors(vdev, true); // use updated nr_vectors
...
} else {
if (vector->virq >= 0) {
fd = event_notifier_get_fd(&vector->kvm_interrupt);
} else {
fd = event_notifier_get_fd(&vector->interrupt);
}
if (vfio_set_irq_signaling(&vdev->vbasedev,
VFIO_PCI_MSIX_IRQ_INDEX, nr,
VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
}
}
Thanks,
Jing
>
> Alex
