From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 2/2] pci-assign: Fix MSI-X registration Date: Thu, 22 Sep 2011 11:04:02 +0200 Message-ID: <4E7AFA02.5010409@siemens.com> References: <20110922030909.4121.66872.stgit@s20.home> <20110922031242.4121.35090.stgit@s20.home> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: "kvm@vger.kernel.org" , "avi@redhat.com" , "yongjie.ren@intel.com" To: Alex Williamson Return-path: Received: from thoth.sbs.de ([192.35.17.2]:25978 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753014Ab1IVJEM (ORCPT ); Thu, 22 Sep 2011 05:04:12 -0400 In-Reply-To: <20110922031242.4121.35090.stgit@s20.home> Sender: kvm-owner@vger.kernel.org List-ID: On 2011-09-22 05:12, Alex Williamson wrote: > Commit c4525754 added a capability check for KVM_CAP_DEVICE_MSIX, > which is unfortunately not exposed, resulting in MSIX never > being listed as a capability. Oops. Should we fix this nevertheless in the kernel? > This breaks anything depending on > MSIX, such as igbvf. Since we can't specifically check for MSIX > support and KVM_CAP_ASSIGN_DEV_IRQ indicates more than just MSI, > let's just revert c4525754 and replace it with a sanity check that > we need KVM_CAP_ASSIGN_DEV_IRQ if the device supports any kind of > interrupt (which is still mostly paranoia). > > Signed-off-by: Alex Williamson > --- > > hw/device-assignment.c | 13 +++++++++---- > 1 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c > index 93913b3..b5bde68 100644 > --- a/hw/device-assignment.c > +++ b/hw/device-assignment.c > @@ -1189,8 +1189,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev) > > /* Expose MSI capability > * MSI capability is the 1st capability in capability config */ > - pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI, 0); > - if (pos != 0 && kvm_check_extension(kvm_state, KVM_CAP_ASSIGN_DEV_IRQ)) { > + if ((pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI, 0))) { > dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI; > /* Only 32-bit/no-mask currently supported */ > if ((ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSI, pos, 10)) < 0) { > @@ -1211,8 +1210,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev) > pci_set_word(pci_dev->wmask + pos + PCI_MSI_DATA_32, 0xffff); > } > /* Expose MSI-X capability */ > - pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX, 0); > - if (pos != 0 && kvm_check_extension(kvm_state, KVM_CAP_DEVICE_MSIX)) { > + if ((pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX, 0))) { > int bar_nr; > uint32_t msix_table_entry; > > @@ -1606,6 +1604,13 @@ static int assigned_initfn(struct PCIDevice *pci_dev) > if (assigned_device_pci_cap_init(pci_dev) < 0) > goto out; > > + if (!kvm_check_extension(kvm_state, KVM_CAP_ASSIGN_DEV_IRQ) && > + (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX || > + dev->cap.available & ASSIGNED_DEVICE_CAP_MSI || > + assigned_dev_pci_read_byte(pci_dev, PCI_INTERRUPT_PIN) != 0)) { > + goto out; > + } > + That's not equivalent as it needlessly prevents IRQ support in the absence of KVM_CAP_ASSIGN_DEV_IRQ. Let's just fix the core issue and replace the test for KVM_CAP_DEVICE_MSIX with a test call of KVM_ASSIGN_SET_MSIX_NR, passing in a NULL struct. If it returns -EFAULT, the IOCTL is known and MSIX is supported. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux