Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

Marcelo Tosatti
2009-02-12T20:40:41Z

On Wed, Feb 11, 2009 at 04:08:51PM +0800, Sheng Yang wrote:
> This patch finally enable MSI-X.
>
> What we need for MSI-X:
> 1. Intercept one page in MMIO region of device. So that we can get guest desired
> MSI-X table and set up the real one. Now this have been done by guest, and
> transfer to kernel using ioctl KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY.
>
> 2. Information for incoming interrupt. Now one device can have more than one
> interrupt, and they are all handled by one workqueue structure. So we need to
> identify them. The previous patch enable gsi_msg_pending_bitmap get this done.
>
> 3. Mapping from host IRQ to guest gsi as well as guest gsi to real MSI/MSI-X
> message address/data. We used same entry number for the host and guest here, so
> that it's easy to find the correlated guest gsi.
>
> What we lack for now:
> 1. The PCI spec said nothing can existed with MSI-X table in the same page of
> MMIO region, except pending bits. The patch ignore pending bits as the first
> step (so they are always 0 - no pending).
>
> 2. The PCI spec allowed to change MSI-X table dynamically. That means, the OS
> can enable MSI-X, then mask one MSI-X entry, modify it, and unmask it. The patch
> didn't support this, and Linux also don't work in this way.

Have you thought about supporting this with the current ioctl interface?
Point is that once ioctl's are in and userspace code uses it, you can't
change. So:

- If userspace calls kvm_vm_ioctl_set_msix_entry twice don't you leak
memory (that is a bug actually unless I'm mistaken)? So in the future
kvm_vm_ioctl_set_msix_entry will deallocate the currently allocated
guest/host entries and replace with userspace supplied entries?

- interrupt context can read the table while kvm_vm_ioctl_set_msix_entry
is modifying it. So you either need to forbid more than one
kvm_vm_ioctl_set_msix_entry call in the lifetime of a guest (which
you can later allow when you support MSI table change), or handle
accesses from multiple contexes now. It seems forbidding is enough for
the moment from what you said.

But in general looks good to me (not familiar with the internals of
pci / msi).