[PATCH 2/2] pci-assign: Fix MSI-X registration

2011-09-21 Thread Alex Williamson
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.  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, 0x);
 }
 /* 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;
+}
+
 /* intercept MSI-X entry page in the MMIO */
 if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX)
 if (assigned_dev_register_msix_mmio(dev))

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] pci-assign: Fix MSI-X registration

2011-09-22 Thread Jan Kiszka
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, 0x);
>  }
>  /* 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] pci-assign: Fix MSI-X registration

2011-10-10 Thread Avi Kivity

On 09/22/2011 12:04 PM, Jan Kiszka wrote:

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



Or just add KVM_CAP_DEVICE_MSIX to the kernel and backport it where needed?

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html