On Sat, 27 Jan 2018 20:15:52 +1100 David Gibson <da...@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 <gr...@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);
> >
>
pgpCALo_Hlfsl.pgp
Description: OpenPGP digital signature
