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

["Michael S. Tsirkin" <mst@redhat.com>]

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: qemu-stable@nongnu.org
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > 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?

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

> > > ---
> > >  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;
> > >          }
> > >  
> 
>