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

[Liu, Jing2]

Hi Igor,

On Wed, August 30, 2023 6:49 PM, Igor Mammedov  wrote:
> 
> On Wed, 30 Aug 2023 10:03:33 +0000
> "Liu, Jing2" <jing2.liu@intel.com> wrote:
> 
...
> > > > +/*
> > > > + * 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

Thank you for the guide. It's now clear to me that for such type, there need 
define 
specific free function by macros and g_auto can help call it automatically. 

Jing

> 
> > 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.