RE: Virtualizing MSI-X on IMS via VFIO

From: Thomas Gleixner <redacted>
Sent: Thursday, June 24, 2021 12:32 AM

On Wed, Jun 23 2021 at 06:12, Kevin Tian wrote:

quoted

quoted

From: Thomas Gleixner <redacted>
So the only downside today of allocating more MSI-X vectors than
necessary is memory consumption for the irq descriptors.

Curious about irte entry when IRQ remapping is enabled. Is it also
allocated at request_irq()?

Good question. No, it has to be allocated right away. We stick the
shutdown vector into the IRTE and then request_irq() will update it with
the real one.

There are max 64K irte entries per Intel VT-d. Do we consider it as
a limited resource in this new model, though it's much more than
CPU vectors?

quoted

So the correct flow is like below:

    guest::enable_msix()
      trapped_by_host()
        pci_alloc_irq_vectors(); // for all possible vMSI-X entries
          pci_enable_msix();

    guest::unmask()
      trapped_by_host()
        request_irqs();

the first trap calls a new VFIO ioctl e.g. VFIO_DEVICE_ALLOC_IRQS.

the 2nd trap can reuse existing VFIO_DEVICE_SET_IRQS which just
does request_irq() if specified irqs have been allocated.

Then map ims to this flow:

    guest::enable_msix()
      trapped_by_host()
        msi_domain_alloc_irqs(); // for all possible vMSI-X entries
        for_all_allocated_irqs(i)
          pci_update_msi_desc_id(i, default_pasid); // a new helper func

    guest::unmask(entry#0)
      trapped_by_host()
        request_irqs();
          ims_array_irq_startup(); // write msi_desc.id (default_pasid) to ims

entry

quoted

    guest::set_msix_perm(entry#1, guest_sva_pasid)
      trapped_by_host()
        pci_update_msi_desc_id(1, host_sva_pasid);

    guest::unmask(entry#1)
      trapped_by_host()
        request_irqs();
          ims_array_irq_startup(); // write msi_desc.id (host_sva_pasid) to ims

entry

That's one way to do that, but that still has the same problem that the
request_irq() in the guest succeeds even if the host side fails.

yes

As this is really new stuff there is no real good reason to force that
into the existing VFIO/MSIX stuff with all it's known downsides and
limitations.

The point is, that IMS can just add another interrupt to a device on the
fly without doing any of the PCI/MSIX nasties. So why not take advantage
of that?

I can see the point of using PCI to expose the device to the guest
because it's trivial to enumerate, but contrary to VF devices there is

also about compatibility since PCI is supported by almost all OSes.

no legacy and the mechanism how to setup the device interrupts can be
completely different from PCI/MSIX.

Exposing some trappable "IMS" storage in a separate PCI bar won't cut it
because this still has the same problem that the allocation or
request_irq() on the host can fail w/o feedback.

yes to fully fix the said nasty some feedback mechanism is required.

So IMO creating a proper paravirt interface is the right approach.  It
avoids _all_ of the trouble and will be necessary anyway once you want
to support devices which store the message/pasid in system memory and
not in on-device memory.

While I agree a paravirt interface is definitely cleaner, I wonder whether
this should be done in orthogonal or tied to all new ims-capable devices.
Back to earlier discussion about guest ims support, you explained a layered
model where the paravirt interface sits between msi domain and vector
domain to get addr/data pair from the host. In this way it could provide
a feedback mechanism for both msi and ims devices, thus not specific
to ims only. Then considering the transition window where not all guest
OSes may support paravirt interface at the same time (or there are
multiple paravirt interfaces which takes time for host to support all), 
would below staging approach still makes sense?

1)  Fix the lost interrupt issue in existing MSI virtualization flow;
2)  Virtualize MSI-X on IMS, bearing the same request_irq() problem;
3)  Develop a paravirt interface to solve request_irq() problem for
      both msi and ims devices;

Thanks
Kevin