On Wed, 30 Aug 2023 10:03:33 +0000
"Liu, Jing2" <jing2....@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.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?
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);
>
