[Qemu-devel] [PATCH] spapr_pci: fix device tree props for MSI/MSI-X

[Qemu-devel] [PATCH] spapr_pci: fix device tree props for MSI/MSI-X

[Michael Roth]

PAPR requires ibm,req#msi and ibm,req#msi-x to be present in the
device node to define the number of msi/msi-x interrupts the device
supports, respectively.

Currently we have ibm,req#msi-x hardcoded to a non-sensical constant
that happens to be 2, and are missing ibm,req#msi entirely. The result
of that is that msi-x capable devices get limited to 2 msi-x
interrupts (which can impact performance), and msi-only devices likely
wouldn't work at all. Additionally, if devices expect a minimum that
exceeds 2, the guest driver may fail to load entirely.

SLOF still owns the generation of these properties at boot-time
(although other device properties have since been offloaded to QEMU),
but for hotplugged devices we rely on the values generated by QEMU
and thus hit the limitations above.

Fix this by generating these properties in QEMU as expected by guests.

In the future it may make sense to modify SLOF to pass through these
values directly as we do with other props since we're duplicating SLOF
code.

Cc: qemu-ppc@nongnu.org
Cc: qemu-stable@nongnu.org
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2782856..8e9edff 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1012,6 +1012,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
     int pci_status, err;
     char *buf = NULL;
     uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
+    uint32_t max_msi, max_msix;
 
     if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
         PCI_HEADER_TYPE_BRIDGE) {
@@ -1092,8 +1093,15 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
                           RESOURCE_CELLS_ADDRESS));
     _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
                           RESOURCE_CELLS_SIZE));
-    _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x",
-                          RESOURCE_CELLS_SIZE));
+
+    max_msi = msi_nr_vectors_allocated(dev);
+    if (max_msi) {
+        _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
+    }
+    max_msix = dev->msix_entries_nr;
+    if (max_msix) {
+        _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x", max_msix));
+    }
 
     populate_resource_props(dev, &rp);
     _FDT(fdt_setprop(fdt, offset, "reg", (uint8_t *)rp.reg, rp.reg_len));
-- 
1.9.1

Re: [Qemu-devel] [PATCH] spapr_pci: fix device tree props for MSI/MSI-X

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3033 bytes --]

On Tue, Sep 15, 2015 at 04:34:59PM -0500, Michael Roth wrote:
> PAPR requires ibm,req#msi and ibm,req#msi-x to be present in the
> device node to define the number of msi/msi-x interrupts the device
> supports, respectively.
> 
> Currently we have ibm,req#msi-x hardcoded to a non-sensical constant
> that happens to be 2, and are missing ibm,req#msi entirely. The result
> of that is that msi-x capable devices get limited to 2 msi-x
> interrupts (which can impact performance), and msi-only devices likely
> wouldn't work at all. Additionally, if devices expect a minimum that
> exceeds 2, the guest driver may fail to load entirely.
> 
> SLOF still owns the generation of these properties at boot-time
> (although other device properties have since been offloaded to QEMU),
> but for hotplugged devices we rely on the values generated by QEMU
> and thus hit the limitations above.
> 
> Fix this by generating these properties in QEMU as expected by guests.
> 
> In the future it may make sense to modify SLOF to pass through these
> values directly as we do with other props since we're duplicating SLOF
> code.
> 
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-stable@nongnu.org
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Applied to spapr-next, thanks.

> ---
>  hw/ppc/spapr_pci.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2782856..8e9edff 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1012,6 +1012,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>      int pci_status, err;
>      char *buf = NULL;
>      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> +    uint32_t max_msi, max_msix;
>  
>      if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
>          PCI_HEADER_TYPE_BRIDGE) {
> @@ -1092,8 +1093,15 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>                            RESOURCE_CELLS_ADDRESS));
>      _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
>                            RESOURCE_CELLS_SIZE));
> -    _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x",
> -                          RESOURCE_CELLS_SIZE));
> +
> +    max_msi = msi_nr_vectors_allocated(dev);
> +    if (max_msi) {
> +        _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
> +    }
> +    max_msix = dev->msix_entries_nr;
> +    if (max_msix) {
> +        _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x", max_msix));
> +    }
>  
>      populate_resource_props(dev, &rp);
>      _FDT(fdt_setprop(fdt, offset, "reg", (uint8_t *)rp.reg, rp.reg_len));

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]