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

[Greg Kurz <groug@kaod.org>]

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

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);
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]