On Thu, Feb 22, 2018 at 11:23:35AM +1100, Sam Bobroff wrote:
> Currently EEH recovery will fail on pSeries platforms for
> passed-through physical devices that support IOV, when CONFIG_PCI_IOV
> is set and the hypervisor doesn't provide a device tree node
> "ibm,open-sriov-vf-bar-info" for the device. (Found on an IOV capable
> device using the ipr driver.)
>
> EEH recovery fails in pci_enable_resources() at the check on
> r->parent, because r->flags is set and r->parent is not. This state
> is due to sriov_init() setting the start, end and flags members of the
> IOV BARs but the parent not being set later in
> pseries_pci_fixup_iov_resources(), because the
> "ibm,open-sriov-vf-bar-info" property is missing, causing it to bail
> out early.
>
> Correct this by zeroing the IOV resources in the bailout path, so that
> they are not seen by pci_enable_resources().
>
> Signed-off-by: Sam Bobroff <sam.bobroff@xxxxxxxxxxx>
> ---
> Hi,
>
> This is a fix to allow EEH recovery to succeed in a specific
> situation, which I've tried to explain in the commit message. But
> while I'm fairly sure of the situation leading up to the problem,
> I'm not sure how it should be fixed, which is why I've posted this
> as an RFC.
>
> Just zeroing out the flags is sufficient in my tests but it seemed
> cleaner to also clear start and end, but other changes would work as
> well: just clearing the flags, or just removing IORESOURCE_IO and
> IORESOURCE_MEM or even adding some new flag and also adjusting the
> test. Perhaps there's something else. I'm not sure what
> implications these choices have for the generic PCI code so I would
> appreciate feedback on it.
>
> Note that this problem doesn't seem to have been introduced by
> pseries_pci_fixup_iov_resources() (which was added recently): recovery
> already failed (in this way) before it's introduction.
>
> arch/powerpc/platforms/pseries/setup.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index 372d7ada1a0c..019836ffe53d 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -618,13 +618,24 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev)
> {
> const int *indexes;
> struct device_node *dn = pci_device_to_OF_node(pdev);
> + int i;
> + struct resource *r;
>
> if (!pdev->is_physfn || pdev->is_added)
> return;
> /*Firmware must support open sriov otherwise dont configure*/
> indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL);
> - if (!indexes)
> + if (!indexes) {
> + /* Hide the BARs we can't configure, otherwise other
> + * code may see r->flags != 0 and r->parent == 0
> + * and raise an error. */
> + pci_warn(pdev, "No hypervisor support for SRIOV on this device, IOV BARs disabled.\n");
> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> + r = &pdev->resource[PCI_IOV_RESOURCES + i];
> + r->start = r->end = r->flags = 0;
> + }
> return;
Just to make sure I'm understanding the situation, "pdev" here is a PF
(not a VF), right? And if "ibm,open-sriov-vf-bar-info" existed,
of_pci_parse_iov_addrs() would fill in the pdev resources
corresponding to VF BAR0, VF BAR1, etc, in the PF's SR-IOV Capability?
The VF BARs are only used by the VFs, so I guess it should be safe to
enable the PF even if the VF BARs are not assigned, so I think this
patch is OK as far as it goes.
But I think you would also want to do something to prevent the VFs
from being enabled, i.e., we never want to set PCI_SRIOV_CTRL_VFE and
PCI_SRIOV_CTRL_MSE on this PF. That would cause VFs to decode memory
space that we have not configured and don't know about. I don't know
off the top of my head what the mechanism for that would be or if we
even have one.
> + }
> /* Assign the addresses from device tree*/
> of_pci_parse_iov_addrs(pdev, indexes);
> }
> --
> 2.16.1.74.g9b0b1f47b
>
