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.william...@redhat.com>
> > Signed-off-by: Jing Liu <jing2....@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?
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);
