[Qemu-devel] [PATCH] spapr_pci: fix MSI/MSIX selection

Greg Kurz posted 1 patch 6 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/151700552398.7196.2573848773899364520.stgit@bahia.lan
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test ppc passed
Test s390x passed
hw/ppc/spapr_pci.c |   61 ++++++++++++++++++++++++++++++++++++----------------
1 file changed, 42 insertions(+), 19 deletions(-)
[Qemu-devel] [PATCH] spapr_pci: fix MSI/MSIX selection
Posted by Greg Kurz 6 years, 2 months ago
In various place we don't correctly check if the device supports MSI or
MSI-X. This can cause devices to be advertised with MSI support, even
if they only support MSI-X (like virtio-pci-* devices for example):

                ethernet@0 {
                        ibm,req#msi = <0x1>; <--- wrong!
			.
			ibm,loc-code = "qemu_virtio-net-pci:0000:00:00.0";
			.
			ibm,req#msi-x = <0x3>;
                };

Worse, this can also cause the "ibm,change-msi" RTAS call to corrupt the
PCI status and cause migration to fail:

  qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x6
    read: 0 device: 10 cmask: 10 wmask: 0 w1cmask:0
                              ^^
           PCI_STATUS_CAP_LIST bit which is assumed to be constant

This patch changes spapr_populate_pci_child_dt() to properly check for
MSI support using msi_present(): this ensures that PCIDevice::msi_cap
was set by msi_init() and that msi_nr_vectors_allocated() will look at
the right place in the config space.

Checking PCIDevice::msix_entries_nr is enough for MSI-X but let's add
a call to msix_present() there as well for consistency.

It also changes rtas_ibm_change_msi() to select the appropriate MSI
type in Function 1 instead of always selecting plain MSI. This new
behaviour is compliant with LoPAPR 1.1, as described in "Table 71.
ibm,change-msi Argument Call Buffer":

  Function 1: If Number Outputs is equal to 3, request to set to a new
           number of MSIs (including set to 0).
           If the “ibm,change-msix-capable” property exists and Number
           Outputs is equal to 4, request is to set to a new number of
           MSI or MSI-X (platform choice) interrupts (including set to
           0).

Since MSI is the the platform default (LoPAPR 6.2.3 MSI Option), let's
check for MSI support first.

And finally, it checks the input parameters are valid, as described in
LoPAPR 1.1 "R1–7.3.10.5.1–3":

  For the MSI option: The platform must return a Status of -3 (Parameter
  error) from ibm,change-msi, with no change in interrupt assignments if
  the PCI configuration address does not support MSI and Function 3 was
  requested (that is, the “ibm,req#msi” property must exist for the PCI
  configuration address in order to use Function 3), or does not support
  MSI-X and Function 4 is requested (that is, the “ibm,req#msi-x” property
  must exist for the PCI configuration address in order to use Function 4),
  or if neither MSIs nor MSI-Xs are supported and Function 1 is requested.

This ensures that the ret_intr_type variable contains a valid MSI type
for this device, and that spapr_msi_setmsg() won't corrupt the PCI status.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c |   61 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 37f18b3d3235..39a14980d397 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -280,13 +280,42 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     int *config_addr_key;
     Error *err = NULL;
 
+    /* Fins sPAPRPHBState */
+    phb = spapr_pci_find_phb(spapr, buid);
+    if (phb) {
+        pdev = spapr_pci_find_dev(spapr, buid, config_addr);
+    }
+    if (!phb || !pdev) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
     switch (func) {
-    case RTAS_CHANGE_MSI_FN:
     case RTAS_CHANGE_FN:
-        ret_intr_type = RTAS_TYPE_MSI;
+        if (msi_present(pdev)) {
+            ret_intr_type = RTAS_TYPE_MSI;
+        } else if (msix_present(pdev)) {
+            ret_intr_type = RTAS_TYPE_MSIX;
+        } else {
+            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+            return;
+        }
+        break;
+    case RTAS_CHANGE_MSI_FN:
+        if (msi_present(pdev)) {
+            ret_intr_type = RTAS_TYPE_MSI;
+        } else {
+            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+            return;
+        }
         break;
     case RTAS_CHANGE_MSIX_FN:
-        ret_intr_type = RTAS_TYPE_MSIX;
+        if (msix_present(pdev)) {
+            ret_intr_type = RTAS_TYPE_MSIX;
+        } else {
+            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+            return;
+        }
         break;
     default:
         error_report("rtas_ibm_change_msi(%u) is not implemented", func);
@@ -294,16 +323,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         return;
     }
 
-    /* Fins sPAPRPHBState */
-    phb = spapr_pci_find_phb(spapr, buid);
-    if (phb) {
-        pdev = spapr_pci_find_dev(spapr, buid, config_addr);
-    }
-    if (!phb || !pdev) {
-        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
-        return;
-    }
-
     msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
 
     /* Releasing MSIs */
@@ -1286,13 +1305,17 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
     _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
                           RESOURCE_CELLS_SIZE));
 
-    max_msi = msi_nr_vectors_allocated(dev);
-    if (max_msi) {
-        _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
+    if (msi_present(dev)) {
+        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));
+    if (msix_present(dev)) {
+        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);


Re: [Qemu-devel] [PATCH] spapr_pci: fix MSI/MSIX selection
Posted by David Gibson 6 years, 2 months ago
On Fri, Jan 26, 2018 at 11:25:24PM +0100, Greg Kurz wrote:
> In various place we don't correctly check if the device supports MSI or
> MSI-X. This can cause devices to be advertised with MSI support, even
> if they only support MSI-X (like virtio-pci-* devices for example):
> 
>                 ethernet@0 {
>                         ibm,req#msi = <0x1>; <--- wrong!
> 			.
> 			ibm,loc-code = "qemu_virtio-net-pci:0000:00:00.0";
> 			.
> 			ibm,req#msi-x = <0x3>;
>                 };
> 
> Worse, this can also cause the "ibm,change-msi" RTAS call to corrupt the
> PCI status and cause migration to fail:
> 
>   qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x6
>     read: 0 device: 10 cmask: 10 wmask: 0 w1cmask:0
>                               ^^
>            PCI_STATUS_CAP_LIST bit which is assumed to be constant
> 
> This patch changes spapr_populate_pci_child_dt() to properly check for
> MSI support using msi_present(): this ensures that PCIDevice::msi_cap
> was set by msi_init() and that msi_nr_vectors_allocated() will look at
> the right place in the config space.
> 
> Checking PCIDevice::msix_entries_nr is enough for MSI-X but let's add
> a call to msix_present() there as well for consistency.
> 
> It also changes rtas_ibm_change_msi() to select the appropriate MSI
> type in Function 1 instead of always selecting plain MSI. This new
> behaviour is compliant with LoPAPR 1.1, as described in "Table 71.
> ibm,change-msi Argument Call Buffer":
> 
>   Function 1: If Number Outputs is equal to 3, request to set to a new
>            number of MSIs (including set to 0).
>            If the “ibm,change-msix-capable” property exists and Number
>            Outputs is equal to 4, request is to set to a new number of
>            MSI or MSI-X (platform choice) interrupts (including set to
>            0).
> 
> Since MSI is the the platform default (LoPAPR 6.2.3 MSI Option), let's
> check for MSI support first.
> 
> And finally, it checks the input parameters are valid, as described in
> LoPAPR 1.1 "R1–7.3.10.5.1–3":
> 
>   For the MSI option: The platform must return a Status of -3 (Parameter
>   error) from ibm,change-msi, with no change in interrupt assignments if
>   the PCI configuration address does not support MSI and Function 3 was
>   requested (that is, the “ibm,req#msi” property must exist for the PCI
>   configuration address in order to use Function 3), or does not support
>   MSI-X and Function 4 is requested (that is, the “ibm,req#msi-x” property
>   must exist for the PCI configuration address in order to use Function 4),
>   or if neither MSIs nor MSI-Xs are supported and Function 1 is requested.
> 
> This ensures that the ret_intr_type variable contains a valid MSI type
> for this device, and that spapr_msi_setmsg() won't corrupt the PCI status.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

Alexey, is this the migration bug you were mentioning to me?

+lvivier

Laurent, could this cover any of the migration bugs you're looking at?
If not we should probably file a new downstream BZ for it.

> ---
>  hw/ppc/spapr_pci.c |   61 ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 37f18b3d3235..39a14980d397 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -280,13 +280,42 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      int *config_addr_key;
>      Error *err = NULL;
>  
> +    /* Fins sPAPRPHBState */
> +    phb = spapr_pci_find_phb(spapr, buid);
> +    if (phb) {
> +        pdev = spapr_pci_find_dev(spapr, buid, config_addr);
> +    }
> +    if (!phb || !pdev) {
> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +        return;
> +    }
> +
>      switch (func) {
> -    case RTAS_CHANGE_MSI_FN:
>      case RTAS_CHANGE_FN:
> -        ret_intr_type = RTAS_TYPE_MSI;
> +        if (msi_present(pdev)) {
> +            ret_intr_type = RTAS_TYPE_MSI;
> +        } else if (msix_present(pdev)) {
> +            ret_intr_type = RTAS_TYPE_MSIX;
> +        } else {
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
> +        break;
> +    case RTAS_CHANGE_MSI_FN:
> +        if (msi_present(pdev)) {
> +            ret_intr_type = RTAS_TYPE_MSI;
> +        } else {
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
>          break;
>      case RTAS_CHANGE_MSIX_FN:
> -        ret_intr_type = RTAS_TYPE_MSIX;
> +        if (msix_present(pdev)) {
> +            ret_intr_type = RTAS_TYPE_MSIX;
> +        } else {
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
>          break;
>      default:
>          error_report("rtas_ibm_change_msi(%u) is not implemented", func);
> @@ -294,16 +323,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    /* Fins sPAPRPHBState */
> -    phb = spapr_pci_find_phb(spapr, buid);
> -    if (phb) {
> -        pdev = spapr_pci_find_dev(spapr, buid, config_addr);
> -    }
> -    if (!phb || !pdev) {
> -        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> -        return;
> -    }
> -
>      msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
>  
>      /* Releasing MSIs */
> @@ -1286,13 +1305,17 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>      _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
>                            RESOURCE_CELLS_SIZE));
>  
> -    max_msi = msi_nr_vectors_allocated(dev);
> -    if (max_msi) {
> -        _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
> +    if (msi_present(dev)) {
> +        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));
> +    if (msix_present(dev)) {
> +        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);
> 

-- 
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
Re: [Qemu-devel] [PATCH] spapr_pci: fix MSI/MSIX selection
Posted by Greg Kurz 6 years, 1 month ago
On Sat, 27 Jan 2018 20:15:52 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jan 26, 2018 at 11:25:24PM +0100, Greg Kurz wrote:
> > In various place we don't correctly check if the device supports MSI or
> > MSI-X. This can cause devices to be advertised with MSI support, even
> > if they only support MSI-X (like virtio-pci-* devices for example):
> > 
> >                 ethernet@0 {
> >                         ibm,req#msi = <0x1>; <--- wrong!
> > 			.
> > 			ibm,loc-code = "qemu_virtio-net-pci:0000:00:00.0";
> > 			.
> > 			ibm,req#msi-x = <0x3>;
> >                 };
> > 
> > Worse, this can also cause the "ibm,change-msi" RTAS call to corrupt the
> > PCI status and cause migration to fail:
> > 
> >   qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x6
> >     read: 0 device: 10 cmask: 10 wmask: 0 w1cmask:0
> >                               ^^
> >            PCI_STATUS_CAP_LIST bit which is assumed to be constant
> > 
> > This patch changes spapr_populate_pci_child_dt() to properly check for
> > MSI support using msi_present(): this ensures that PCIDevice::msi_cap
> > was set by msi_init() and that msi_nr_vectors_allocated() will look at
> > the right place in the config space.
> > 
> > Checking PCIDevice::msix_entries_nr is enough for MSI-X but let's add
> > a call to msix_present() there as well for consistency.
> > 
> > It also changes rtas_ibm_change_msi() to select the appropriate MSI
> > type in Function 1 instead of always selecting plain MSI. This new
> > behaviour is compliant with LoPAPR 1.1, as described in "Table 71.
> > ibm,change-msi Argument Call Buffer":
> > 
> >   Function 1: If Number Outputs is equal to 3, request to set to a new
> >            number of MSIs (including set to 0).
> >            If the “ibm,change-msix-capable” property exists and Number
> >            Outputs is equal to 4, request is to set to a new number of
> >            MSI or MSI-X (platform choice) interrupts (including set to
> >            0).
> > 
> > Since MSI is the the platform default (LoPAPR 6.2.3 MSI Option), let's
> > check for MSI support first.
> > 
> > And finally, it checks the input parameters are valid, as described in
> > LoPAPR 1.1 "R1–7.3.10.5.1–3":
> > 
> >   For the MSI option: The platform must return a Status of -3 (Parameter
> >   error) from ibm,change-msi, with no change in interrupt assignments if
> >   the PCI configuration address does not support MSI and Function 3 was
> >   requested (that is, the “ibm,req#msi” property must exist for the PCI
> >   configuration address in order to use Function 3), or does not support
> >   MSI-X and Function 4 is requested (that is, the “ibm,req#msi-x” property
> >   must exist for the PCI configuration address in order to use Function 4),
> >   or if neither MSIs nor MSI-Xs are supported and Function 1 is requested.
> > 
> > This ensures that the ret_intr_type variable contains a valid MSI type
> > for this device, and that spapr_msi_setmsg() won't corrupt the PCI status.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> Applied, thanks.
> 
> Alexey, is this the migration bug you were mentioning to me?
> 
> +lvivier
> 
> Laurent, could this cover any of the migration bugs you're looking at?
> If not we should probably file a new downstream BZ for it.
> 

This bug has been around for a long time, but maybe worth pushing this to
stable as well ?

Cc'ing stable.

> > ---
> >  hw/ppc/spapr_pci.c |   61 ++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 42 insertions(+), 19 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 37f18b3d3235..39a14980d397 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -280,13 +280,42 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      int *config_addr_key;
> >      Error *err = NULL;
> >  
> > +    /* Fins sPAPRPHBState */
> > +    phb = spapr_pci_find_phb(spapr, buid);
> > +    if (phb) {
> > +        pdev = spapr_pci_find_dev(spapr, buid, config_addr);
> > +    }
> > +    if (!phb || !pdev) {
> > +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +        return;
> > +    }
> > +
> >      switch (func) {
> > -    case RTAS_CHANGE_MSI_FN:
> >      case RTAS_CHANGE_FN:
> > -        ret_intr_type = RTAS_TYPE_MSI;
> > +        if (msi_present(pdev)) {
> > +            ret_intr_type = RTAS_TYPE_MSI;
> > +        } else if (msix_present(pdev)) {
> > +            ret_intr_type = RTAS_TYPE_MSIX;
> > +        } else {
> > +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +            return;
> > +        }
> > +        break;
> > +    case RTAS_CHANGE_MSI_FN:
> > +        if (msi_present(pdev)) {
> > +            ret_intr_type = RTAS_TYPE_MSI;
> > +        } else {
> > +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +            return;
> > +        }
> >          break;
> >      case RTAS_CHANGE_MSIX_FN:
> > -        ret_intr_type = RTAS_TYPE_MSIX;
> > +        if (msix_present(pdev)) {
> > +            ret_intr_type = RTAS_TYPE_MSIX;
> > +        } else {
> > +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +            return;
> > +        }
> >          break;
> >      default:
> >          error_report("rtas_ibm_change_msi(%u) is not implemented", func);
> > @@ -294,16 +323,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >          return;
> >      }
> >  
> > -    /* Fins sPAPRPHBState */
> > -    phb = spapr_pci_find_phb(spapr, buid);
> > -    if (phb) {
> > -        pdev = spapr_pci_find_dev(spapr, buid, config_addr);
> > -    }
> > -    if (!phb || !pdev) {
> > -        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > -        return;
> > -    }
> > -
> >      msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
> >  
> >      /* Releasing MSIs */
> > @@ -1286,13 +1305,17 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> >      _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
> >                            RESOURCE_CELLS_SIZE));
> >  
> > -    max_msi = msi_nr_vectors_allocated(dev);
> > -    if (max_msi) {
> > -        _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
> > +    if (msi_present(dev)) {
> > +        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));
> > +    if (msix_present(dev)) {
> > +        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);
> >   
> 

Re: [Qemu-devel] [PATCH] spapr_pci: fix MSI/MSIX selection
Posted by Laurent Vivier 6 years, 1 month ago
On 27/01/2018 10:15, David Gibson wrote:
> On Fri, Jan 26, 2018 at 11:25:24PM +0100, Greg Kurz wrote:
>> In various place we don't correctly check if the device supports MSI or
>> MSI-X. This can cause devices to be advertised with MSI support, even
>> if they only support MSI-X (like virtio-pci-* devices for example):
>>
>>                 ethernet@0 {
>>                         ibm,req#msi = <0x1>; <--- wrong!
>> 			.
>> 			ibm,loc-code = "qemu_virtio-net-pci:0000:00:00.0";
>> 			.
>> 			ibm,req#msi-x = <0x3>;
>>                 };
>>
>> Worse, this can also cause the "ibm,change-msi" RTAS call to corrupt the
>> PCI status and cause migration to fail:
>>
>>   qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x6
>>     read: 0 device: 10 cmask: 10 wmask: 0 w1cmask:0
>>                               ^^
>>            PCI_STATUS_CAP_LIST bit which is assumed to be constant
>>
>> This patch changes spapr_populate_pci_child_dt() to properly check for
>> MSI support using msi_present(): this ensures that PCIDevice::msi_cap
>> was set by msi_init() and that msi_nr_vectors_allocated() will look at
>> the right place in the config space.
>>
>> Checking PCIDevice::msix_entries_nr is enough for MSI-X but let's add
>> a call to msix_present() there as well for consistency.
>>
>> It also changes rtas_ibm_change_msi() to select the appropriate MSI
>> type in Function 1 instead of always selecting plain MSI. This new
>> behaviour is compliant with LoPAPR 1.1, as described in "Table 71.
>> ibm,change-msi Argument Call Buffer":
>>
>>   Function 1: If Number Outputs is equal to 3, request to set to a new
>>            number of MSIs (including set to 0).
>>            If the “ibm,change-msix-capable” property exists and Number
>>            Outputs is equal to 4, request is to set to a new number of
>>            MSI or MSI-X (platform choice) interrupts (including set to
>>            0).
>>
>> Since MSI is the the platform default (LoPAPR 6.2.3 MSI Option), let's
>> check for MSI support first.
>>
>> And finally, it checks the input parameters are valid, as described in
>> LoPAPR 1.1 "R1–7.3.10.5.1–3":
>>
>>   For the MSI option: The platform must return a Status of -3 (Parameter
>>   error) from ibm,change-msi, with no change in interrupt assignments if
>>   the PCI configuration address does not support MSI and Function 3 was
>>   requested (that is, the “ibm,req#msi” property must exist for the PCI
>>   configuration address in order to use Function 3), or does not support
>>   MSI-X and Function 4 is requested (that is, the “ibm,req#msi-x” property
>>   must exist for the PCI configuration address in order to use Function 4),
>>   or if neither MSIs nor MSI-Xs are supported and Function 1 is requested.
>>
>> This ensures that the ret_intr_type variable contains a valid MSI type
>> for this device, and that spapr_msi_setmsg() won't corrupt the PCI status.
>>
>> Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Applied, thanks.
> 
> Alexey, is this the migration bug you were mentioning to me?
> 
> +lvivier
> 
> Laurent, could this cover any of the migration bugs you're looking at?
> If not we should probably file a new downstream BZ for it.

It doesn't fix my problem:. I have always this kind of error after a
migration on P9:

[   39.305470] Unable to handle kernel paging request for d6
[   39.305534] Faulting instruction address: 0xc000000000694ac0

[   39.305578] Oops: Kernel access of bad area, sig: 11 [#1]
...
[   39.306625] NIP [c000000000694ac0] ioread16+0x30/0x1a0

[   39.306655] LR [c008000000bb074c] vp_get+0x15c/0x190 [virtio_pci]

[   39.306690] Call Trace:

[   39.306707] [c00000000315fb50] [c00000000001c9c0]
__switch_to+0x330/0x660 (u)
[   39.306761] [c00000000315fbc0] [c008000000bb074c] vp_get+0x15c/0x190
[virtio]
[   39.306812] [c00000000315fc00] [c008000000d41328]
virtnet_config_changed_wor]

Greg, do you have a test case for the bug your patch fixes?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] spapr_pci: fix MSI/MSIX selection
Posted by Alexey Kardashevskiy 6 years, 1 month ago
On 27/01/18 09:25, Greg Kurz wrote:
> In various place we don't correctly check if the device supports MSI or
> MSI-X. This can cause devices to be advertised with MSI support, even
> if they only support MSI-X (like virtio-pci-* devices for example):
> 
>                 ethernet@0 {
>                         ibm,req#msi = <0x1>; <--- wrong!
> 			.
> 			ibm,loc-code = "qemu_virtio-net-pci:0000:00:00.0";
> 			.
> 			ibm,req#msi-x = <0x3>;
>                 };
> 
> Worse, this can also cause the "ibm,change-msi" RTAS call to corrupt the
> PCI status and cause migration to fail:
> 
>   qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x6
>     read: 0 device: 10 cmask: 10 wmask: 0 w1cmask:0
>                               ^^
>            PCI_STATUS_CAP_LIST bit which is assumed to be constant
> 
> This patch changes spapr_populate_pci_child_dt() to properly check for
> MSI support using msi_present(): this ensures that PCIDevice::msi_cap
> was set by msi_init() and that msi_nr_vectors_allocated() will look at
> the right place in the config space.
> 
> Checking PCIDevice::msix_entries_nr is enough for MSI-X but let's add
> a call to msix_present() there as well for consistency.
> 
> It also changes rtas_ibm_change_msi() to select the appropriate MSI
> type in Function 1 instead of always selecting plain MSI. This new
> behaviour is compliant with LoPAPR 1.1, as described in "Table 71.
> ibm,change-msi Argument Call Buffer":
> 
>   Function 1: If Number Outputs is equal to 3, request to set to a new
>            number of MSIs (including set to 0).
>            If the “ibm,change-msix-capable” property exists and Number
>            Outputs is equal to 4, request is to set to a new number of
>            MSI or MSI-X (platform choice) interrupts (including set to
>            0).
> 
> Since MSI is the the platform default (LoPAPR 6.2.3 MSI Option), let's
> check for MSI support first.
> 
> And finally, it checks the input parameters are valid, as described in
> LoPAPR 1.1 "R1–7.3.10.5.1–3":
> 
>   For the MSI option: The platform must return a Status of -3 (Parameter
>   error) from ibm,change-msi, with no change in interrupt assignments if
>   the PCI configuration address does not support MSI and Function 3 was
>   requested (that is, the “ibm,req#msi” property must exist for the PCI
>   configuration address in order to use Function 3), or does not support
>   MSI-X and Function 4 is requested (that is, the “ibm,req#msi-x” property
>   must exist for the PCI configuration address in order to use Function 4),
>   or if neither MSIs nor MSI-Xs are supported and Function 1 is requested.
> 
> This ensures that the ret_intr_type variable contains a valid MSI type
> for this device, and that spapr_msi_setmsg() won't corrupt the PCI status.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_pci.c |   61 ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 37f18b3d3235..39a14980d397 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -280,13 +280,42 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      int *config_addr_key;
>      Error *err = NULL;
>  
> +    /* Fins sPAPRPHBState */
> +    phb = spapr_pci_find_phb(spapr, buid);
> +    if (phb) {
> +        pdev = spapr_pci_find_dev(spapr, buid, config_addr);
> +    }
> +    if (!phb || !pdev) {
> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +        return;
> +    }
> +
>      switch (func) {
> -    case RTAS_CHANGE_MSI_FN:
>      case RTAS_CHANGE_FN:
> -        ret_intr_type = RTAS_TYPE_MSI;
> +        if (msi_present(pdev)) {
> +            ret_intr_type = RTAS_TYPE_MSI;
> +        } else if (msix_present(pdev)) {
> +            ret_intr_type = RTAS_TYPE_MSIX;
> +        } else {
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
> +        break;
> +    case RTAS_CHANGE_MSI_FN:
> +        if (msi_present(pdev)) {
> +            ret_intr_type = RTAS_TYPE_MSI;
> +        } else {
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
>          break;
>      case RTAS_CHANGE_MSIX_FN:
> -        ret_intr_type = RTAS_TYPE_MSIX;
> +        if (msix_present(pdev)) {
> +            ret_intr_type = RTAS_TYPE_MSIX;
> +        } else {
> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +            return;
> +        }
>          break;
>      default:
>          error_report("rtas_ibm_change_msi(%u) is not implemented", func);
> @@ -294,16 +323,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    /* Fins sPAPRPHBState */
> -    phb = spapr_pci_find_phb(spapr, buid);
> -    if (phb) {
> -        pdev = spapr_pci_find_dev(spapr, buid, config_addr);
> -    }
> -    if (!phb || !pdev) {
> -        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> -        return;
> -    }
> -
>      msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
>  
>      /* Releasing MSIs */
> @@ -1286,13 +1305,17 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
>      _FDT(fdt_setprop_cell(fdt, offset, "#size-cells",
>                            RESOURCE_CELLS_SIZE));
>  
> -    max_msi = msi_nr_vectors_allocated(dev);
> -    if (max_msi) {
> -        _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
> +    if (msi_present(dev)) {
> +        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));
> +    if (msix_present(dev)) {
> +        max_msix = dev->msix_entries_nr;
> +        if (max_msix) {
> +            _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x", max_msix));
> +        }


Oh. I was under impression this is set up by SLOF; and everything capable
of MSIX can do MSI too (too big assumption, I know).

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>




David, no, this was not a bug I told you about.



>      }
>  
>      populate_resource_props(dev, &rp);
> 


-- 
Alexey