Re: [PATCH V7 08/10] powerpc/powernv: Support PCI config restore for VFs

[Bjorn Helgaas <bhelgaas@google.com>]

On Tue, May 19, 2015 at 06:50:10PM +0800, Wei Yang wrote:
> After PE reset, OPAL API opal_pci_reinit() is called on all devices
> contained in the PE to reinitialize them. However, VFs can't be seen
> from skiboot firmware. We have to implement the functions, similar
> those in skiboot firmware, to reinitialize VFs after reset on PE
> for VFs.
> 
> [gwshan: changelog and code refactoring]
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/pci-bridge.h        |    1 +
>  arch/powerpc/platforms/powernv/eeh-powernv.c |   70 +++++++++++++++++++++++++-
>  arch/powerpc/platforms/powernv/pci.c         |   18 +++++++
>  3 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> index c324882..ad60263 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -206,6 +206,7 @@ struct pci_dn {
>  #define IODA_INVALID_M64        (-1)
>  	int     m64_wins[PCI_SRIOV_NUM_BARS][M64_PER_IOV];
>  #endif /* CONFIG_PCI_IOV */
> +	int	mps;
>  #endif
>  	struct list_head child_list;
>  	struct list_head list;
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index 7af3c1e..33deb78 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -1612,6 +1612,67 @@ static int pnv_eeh_next_error(struct eeh_pe **pe)
>  	return ret;
>  }
>  
> +static int pnv_eeh_restore_vf_config(struct pci_dn *pdn)
> +{
> +	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> +	u32 devctl, cmd, cap2, aer_capctl;
> +	int old_mps;
> +
> +	/* Restore MPS */
> +	if (edev->pcie_cap) {
> +		old_mps = (ffs(pdn->mps) - 8) << 5;
> +		eeh_ops->read_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL,
> +				     2, &devctl);
> +		devctl &= ~PCI_EXP_DEVCTL_PAYLOAD;
> +		devctl |= old_mps;
> +		eeh_ops->write_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL,
> +				      2, devctl);
> +	}
> +
> +	/* Disable Completion Timeout */
> +	if (edev->pcie_cap) {
> +		eeh_ops->read_config(pdn, edev->pcie_cap + PCI_EXP_DEVCAP2,
> +				     4, &cap2);
> +		if (cap2 & 0x10) {
> +			eeh_ops->read_config(pdn,
> +					edev->pcie_cap + PCI_EXP_DEVCTL2,
> +					4, &cap2);
> +			cap2 |= 0x10;
> +			eeh_ops->write_config(pdn,
> +					edev->pcie_cap + PCI_EXP_DEVCTL2,
> +					4, cap2);
> +		}
> +	}
> +
> +	/* Enable SERR and parity checking */
> +	eeh_ops->read_config(pdn, PCI_COMMAND, 2, &cmd);
> +	cmd |= (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
> +	eeh_ops->write_config(pdn, PCI_COMMAND, 2, cmd);
> +
> +	/* Enable report various errors */
> +	if (edev->pcie_cap) {
> +		eeh_ops->read_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL,
> +				2, &devctl);
> +		devctl &= ~PCI_EXP_DEVCTL_CERE;
> +		devctl |= (PCI_EXP_DEVCTL_NFERE |
> +			   PCI_EXP_DEVCTL_FERE |
> +			   PCI_EXP_DEVCTL_URRE);
> +		eeh_ops->write_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL,
> +				2, devctl);
> +	}
> +
> +	/* Enable ECRC generation and check */
> +	if (edev->pcie_cap && edev->aer_cap) {
> +		eeh_ops->read_config(pdn, edev->aer_cap + PCI_ERR_CAP,
> +				4, &aer_capctl);
> +		aer_capctl |= (PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
> +		eeh_ops->write_config(pdn, edev->aer_cap + PCI_ERR_CAP,
> +				4, aer_capctl);
> +	}

Where is all this stuff set up the first time?  It'd be nice if we could
use the same path that did it the first time.

Are we setting up a PF or a VF here?  The function is called
pnv_eeh_restore_vf_config(), but it's called when "edev->physfn", so it's a
little confusing.

> +
> +	return 0;
> +}
> +
>  static int pnv_eeh_restore_config(struct pci_dn *pdn)
>  {
>  	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> @@ -1622,7 +1683,14 @@ static int pnv_eeh_restore_config(struct pci_dn *pdn)
>  		return -EEXIST;
>  
>  	phb = edev->phb->private_data;
> -	ret = opal_pci_reinit(phb->opal_id,
> +	/*
> +	 * We have to restore the PCI config space after reset since the
> +	 * firmware can't see SRIOV VFs.
> +	 */
> +	if (edev->physfn)
> +		ret = pnv_eeh_restore_vf_config(pdn);
> +	else
> +		ret = opal_pci_reinit(phb->opal_id,
>  			      OPAL_REINIT_PCI_DEV, edev->config_addr);
>  	if (ret) {
>  		pr_warn("%s: Can't reinit PCI dev 0x%x (%lld)\n",
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index bca2aeb..10bc8c3 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -729,6 +729,24 @@ static void pnv_p7ioc_rc_quirk(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_IBM, 0x3b9, pnv_p7ioc_rc_quirk);
>  
> +#ifdef CONFIG_PCI_IOV
> +static void pnv_pci_fixup_vf_mps(struct pci_dev *pdev)
> +{
> +	struct pci_dn *pdn = pci_get_pdn(pdev);
> +	int parent_mps;
> +
> +	if (!pdev->is_virtfn)
> +		return;
> +
> +	/* Synchronize MPS for VF and PF */
> +	parent_mps = pcie_get_mps(pdev->physfn);
> +	if ((128 << pdev->pcie_mpss) >= parent_mps)
> +		pcie_set_mps(pdev, parent_mps);
> +	pdn->mps = pcie_get_mps(pdev);
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pnv_pci_fixup_vf_mps);

Same comment as before -- I don't like this usage of fixups.  Would it work
to do this in pcibios_add_device()?

I assume you need this to happen when you hot-remove and hot-add a VF
during EEH recovery?  Where does this happen in the normal hotplug path,
e.g., pciehp, and can you do it in a corresponding place for EEH hotplug?


> +#endif /* CONFIG_PCI_IOV */
> +
>  void __init pnv_pci_init(void)
>  {
>  	struct device_node *np;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html