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-sta...@nongnu.org
> > Signed-off-by: Alex Williamson <alex.william...@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. We should
follow the rules, play be the existing kernel interfaces, and work to
eventually improve those interfaces. 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;
> > }
> >
