Re: [PATCH v1 3/4] vfio/pci: use an invalid fd to enable MSI-X

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 30 Aug 2023 10:03:33 +0000
"Liu, Jing2" <jing2.liu@intel.com> wrote:

> Hi Cédric，
> 
> On 8/29/2023 10:04 PM, Cédric Le Goater wrote:
> > On 8/22/23 09:29, Jing Liu wrote:  
> > > Guests typically enable MSI-X with all of the vectors masked in the
> > > MSI-X vector table. To match the guest state of device, Qemu enables
> > > MSI-X by  
> > 
> > QEMU is preferred to Qemu.  
> Got it. 
> 
> >   
> > > enabling vector 0 with userspace triggering and immediately release.
> > > However the release function actually does not release it due to
> > > already using userspace mode.
> > >
> > > It is no need to enable triggering on host and rely on the mask bit to
> > > avoid spurious interrupts. Use an invalid fd (i.e. fd = -1) is enough
> > > to get MSI-X enabled.
> > >
> > > After dynamic MSI-X allocation is supported, the interrupt restoring
> > > also need use such way to enable MSI-X, therefore, create a function
> > > for that.
> > >
> > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > Signed-off-by: Jing Liu <jing2.liu@intel.com>
> > > ---
> > > Changes since RFC v1:
> > > - A new patch. Use an invalid fd to get MSI-X enabled instead of using
> > >    userspace triggering. (Alex)
> > > ---
> > >   hw/vfio/pci.c | 50 ++++++++++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 42 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
> > > 31f36d68bb19..e24c21241a0c 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -369,6 +369,39 @@ static void vfio_msi_interrupt(void *opaque)
> > >       notify(&vdev->pdev, nr);
> > >   }
> > >
> > > +/*
> > > + * Get MSI-X enabled, but no vector enabled, by setting vector 0 with
> > > +an invalid
> > > + * fd to kernel.
> > > + */
> > > +static int vfio_enable_msix_no_vec(VFIOPCIDevice *vdev)> +{
> > > +    struct vfio_irq_set *irq_set;  
> > 
> > This could be a 'g_autofree' variable.  
> 
> Thanks for pointing this to me. I just realized QEMU doc recommends 
> g_autofree/g_autoptr to do automatic memory deallocation.
> 
> I will replace to g_autofree style in next version.
> 
> I have a question for a specific example (not related to this patch), and
> I failed to find an example in current QEMU code to understand it.
> If one g_autofree structure includes a pointer that we also allocate
> space for the inside pointer, could g_autofree release the inside space?

it might be too cumbersome for 1-off use
see following for how 'auto' works https://docs.gtk.org/glib/auto-cleanup.html
 
> struct dummy1 {
>     int data;
>     struct *p;
> }
> struct p {
>     char member[];
> }
> void func() {
>     g_autofree struct *dummy1 = NULL;
> 
>     dummy1 = g_malloc();
>     dummy1->p = g_malloc();
>     ...
>     return;	// is this correct or need g_free(p)?
> }
> 
> >   
> > > +    int ret = 0, argsz;
> > > +    int32_t *fd;
> > > +
> > > +    argsz = sizeof(*irq_set) + sizeof(*fd);
> > > +
> > > +    irq_set = g_malloc0(argsz);
> > > +    irq_set->argsz = argsz;
> > > +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> > > +                     VFIO_IRQ_SET_ACTION_TRIGGER;
> > > +    irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
> > > +    irq_set->start = 0;
> > > +    irq_set->count = 1;
> > > +    fd = (int32_t *)&irq_set->data;
> > > +    *fd = -1;
> > > +
> > > +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> > > +    if (ret) {
> > > +        error_report("vfio: failed to enable MSI-X with vector 0 trick, %d",
> > > +                     ret);  
> > 
> > The above message seems redundant. I would simply return 'ret' and let the
> > caller report the error. Same as vfio_enable_vectors().  
> 
> Understood. Will change.
> 
> Thanks,
> Jing
> 
> > Thanks,
> > 
> > C.
> > 
> >   
> > > +    }
> > > +
> > > +    g_free(irq_set);
> > > +
> > > +    return ret;
> > > +}
> > > +
> > >   static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool msix)
> > >   {
> > >       struct vfio_irq_set *irq_set;
> > > @@ -618,6 +651,8 @@ static void
> > > vfio_commit_kvm_msi_virq_batch(VFIOPCIDevice *vdev)
> > >
> > >   static void vfio_msix_enable(VFIOPCIDevice *vdev)
> > >   {
> > > +    int ret;
> > > +
> > >       vfio_disable_interrupts(vdev);
> > >
> > >       vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
> > > @@ -640,8 +675,6 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
> > >       vfio_commit_kvm_msi_virq_batch(vdev);
> > >
> > >       if (vdev->nr_vectors) {
> > > -        int ret;
> > > -
> > >           ret = vfio_enable_vectors(vdev, true);
> > >           if (ret) {
> > >               error_report("vfio: failed to enable vectors, %d", ret);
> > > @@ -655,13 +688,14 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
> > >            * MSI-X capability, but leaves the vector table masked.  We therefore
> > >            * can't rely on a vector_use callback (from request_irq() in the guest)
> > >            * to switch the physical device into MSI-X mode because that may come  
> > a  
> > > -         * long time after pci_enable_msix().  This code enables vector 0 with
> > > -         * triggering to userspace, then immediately release the vector, leaving
> > > -         * the physical device with no vectors enabled, but MSI-X enabled, just
> > > -         * like the guest view.
> > > +         * long time after pci_enable_msix().  This code sets vector 0 with an
> > > +         * invalid fd to make the physical device MSI-X enabled, but with no
> > > +         * vectors enabled, just like the guest view.
> > >            */
> > > -        vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
> > > -        vfio_msix_vector_release(&vdev->pdev, 0);
> > > +        ret = vfio_enable_msix_no_vec(vdev);
> > > +        if (ret) {
> > > +            error_report("vfio: failed to enable MSI-X, %d", ret);
> > > +        }
> > >       }
> > >
> > >       trace_vfio_msix_enable(vdev->vbasedev.name);  
>