Re: [Qemu-stable] [PATCH] pci-assign: Enable MSIX on device to match guest

[Alex Williamson]

On Fri, 2012-12-21 at 08:46 -0700, Alex Williamson wrote:
> On Fri, 2012-12-21 at 14:17 +0200, Michael S. Tsirkin wrote:
> > On Thu, Dec 20, 2012 at 03:15:38PM -0700, Alex Williamson wrote:
> > > On Thu, 2012-12-20 at 18:38 +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Dec 20, 2012 at 09:05:50AM -0700, Alex Williamson wrote:
> > > > > When a guest enables MSIX on a device we evaluate the MSIX vector
> > > > > table, typically find no unmasked vectors and don't switch the device
> > > > > to MSIX mode.  This generally works fine and the device will be
> > > > > switched once the guest enables and therefore unmasks a vector.
> > > > > Unfortunately some drivers enable MSIX, then use interfaces to send
> > > > > commands between VF & PF or PF & firmware that act based on the host
> > > > > state of the device.  These therefore break when MSIX is managed
> > > > > lazily.  This change re-enables the previous test used to enable MSIX
> > > > > (see qemu-kvm a6b402c9), which basically guesses whether a vector
> > > > > will be used based on the data field of the vector table.
> > > > > 
> > > > > Cc: address@hidden
> > > > > Signed-off-by: Alex Williamson <address@hidden>
> > > > 
> > > > Same question: can't we enable and mask MSIX through config sysfs?
> > > > In this case it can be done in userspace ...
> > > 
> > > In this case userspace could do this, but I think it's still incredibly
> > > dangerous.  Kernel space drivers can also directly enable MSI-X on a
> > > device, but you might get shot for writing one that did.
> > 
> > What would be the reason for the kernel driver to do this?
> 
> Maybe they don't know how many vectors to use until they enable MSI-X
> and query some firmware interface.  It's a hypothetical situation, I'm
> just trying to illustrate that if a kernel driver did want to do this,
> they'd have to develop interfaces to allow it, not just manually poke
> their MSI-X enable bit.
> 
> > >  We should
> > > follow the rules, play be the existing kernel interfaces, and work to
> > > eventually improve those interfaces.  Thanks,
> > > 
> > > Alex
> > 
> > I'm not against adding an interface for this long term but we have
> > existing kernels to support too.  IMHO it would be nicer than
> > the data hack which relies on non-documented guest behaviour
> > that might change without warning in the future.
> 
> We've unwittingly used the data hack for years and only ripped it out
> because it was undocumented.  The patch below adds documentation for it,
> so at least we have a more clear understanding of why it was there if we
> want to try to rip it out again.  This fully supports existing kernels
> and as I mention below, we might be able to do better with limiting how
> many vectors we enabled, but I think this is the right initial fix and
> right fix for stable and we can continue to experiment from here.

Happy new year.  I'd like to close on this as we do currently have a
regression for devices that cannot handle MSI-X being lazily enabled.
The option here is to document and revert to the old style
initialization behavior where we look at the data field of the vector to
get a hint whether the guest intends to make use of the vector.  This
gives us the same behavior as we had previously, but still allows
vectors to be added, so we maintain the current FreeBSD support.  This
much needs to go to stable.

For the development tree, I think we can do better.  Using the data
field is not 100% reliable in giving us the number of vectors the guest
actually intends to use.  Instead we'd like to enable MSI-X with no
vectors and add vectors as the guest unmasks them.  The host Linux MSI
API currently doesn't allow this, so I think the next best thing is to
enable MSI-X with a single vector in the case where MSI-X is enabled but
no vectors are unmasked.  This conserves vectors on the host though we
do potentially allow spurious interrupts through the enabled vector
(though we previously enabled multiple vectors using the above data
method without problems).

The alternative that you're proposing to this longer term solution is to
manually mask all vectors in the physical MSI-X vector table from
userspace then manually enable MSI-X on the physical device (through
pci-sysfs resource and config access respectively).  This puts the
physical device is a state that better matches the guest view of the
devices, but I'm doubtful that the risk is worth the reward.  This adds
a new state to the qemu MSI-X model where we have entirely host kernel
managed physical IRQ state, except for this.  It also creates a
synchronization problem that the physical device moves to a new
interrupt state outside of the control of the host kernel, possibly
bypassing any quirks for the host platform.

Another option is to modify the host MSI API to allow the interface we
want, splitting enabling MSI-X from vector allocation.  That of course
has a much longer lead time. 

We can certainly continue the discussion on this, but we need a fix for
stable and I don't think either of these longer term methods are known
to have the reliability or simplicity of reverting to the previous
initialization criteria.  Please ack if you agree.  Thanks,

Alex

> > > > > ---
> > > > >  hw/kvm/pci-assign.c |   17 +++++++++++++++--
> > > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > > > 
> > > > > I think we might be able to do a little better than this, but I think
> > > > > this is the right fix for stable and we can build on it to perhaps 
> > > > > only
> > > > > enable a single vector.
> > > > > 
> > > > > diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
> > > > > index e80dad0..12a219b 100644
> > > > > --- a/hw/kvm/pci-assign.c
> > > > > +++ b/hw/kvm/pci-assign.c
> > > > > @@ -1025,6 +1025,19 @@ static bool 
> > > > > assigned_dev_msix_masked(MSIXTableEntry *entry)
> > > > >      return (entry->ctrl & cpu_to_le32(0x1)) != 0;
> > > > >  }
> > > > >  
> > > > > +/*
> > > > > + * When MSI-X is first enabled the vector table typically has all the
> > > > > + * vectors masked, so we can't use that as the obvious test to 
> > > > > figure out
> > > > > + * how many vectors to initially enable.  Instead we look at the 
> > > > > data field
> > > > > + * because this is what worked for pci-assign for a long time.  This 
> > > > > makes
> > > > > + * sure the physical MSI-X state tracks the guest's view, which is 
> > > > > important
> > > > > + * for some VF/PF and PF/fw communication channels.
> > > > > + */
> > > > > +static bool assigned_dev_msix_skipped(MSIXTableEntry *entry)
> > > > > +{
> > > > > +    return !entry->data;
> > > > > +}
> > > > > +
> > > > >  static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> > > > >  {
> > > > >      AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> > > > > @@ -1035,7 +1048,7 @@ static int 
> > > > > assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> > > > >  
> > > > >      /* Get the usable entry number for allocating */
> > > > >      for (i = 0; i < adev->msix_max; i++, entry++) {
> > > > > -        if (assigned_dev_msix_masked(entry)) {
> > > > > +        if (assigned_dev_msix_skipped(entry)) {
> > > > >              continue;
> > > > >          }
> > > > >          entries_nr++;
> > > > > @@ -1064,7 +1077,7 @@ static int 
> > > > > assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> > > > >      for (i = 0; i < adev->msix_max; i++, entry++) {
> > > > >          adev->msi_virq[i] = -1;
> > > > >  
> > > > > -        if (assigned_dev_msix_masked(entry)) {
> > > > > +        if (assigned_dev_msix_skipped(entry)) {
> > > > >              continue;
> > > > >          }
> > > > >  
> > > 
> > > 
> 
>