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

[Sheng Yang <sheng@linux.intel.com>]

On Friday 13 February 2009 04:40:05 Marcelo Tosatti wrote:
> 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?
>

The allocation only happen once, at the second time it would report error in 
current code. But allocate/deallocate is also acceptable for future.

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

Yeah.

But for the modifying the MSI-X table, the most critical problem is, current 
Linux didn't support it IIRC. So I have to disable MSI-X then enable it again 
with new table, and it would result in lost interrupt.

So seems the most reasonable method is to modify pci_enable_msix() and related 
function's action to support this...

-- 
regards
Yang, Sheng

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