* Re: [PATCH v2 1/1] powerpc/pseries: fix EEH recovery of some IOV devices
2018-05-25 0:31 [PATCH v2 1/1] powerpc/pseries: fix EEH recovery of some IOV devices Sam Bobroff
@ 2018-05-30 13:55 ` Bryant G. Ly
2018-06-30 1:53 ` Bjorn Helgaas
2018-07-02 0:59 ` Michael Ellerman
2 siblings, 0 replies; 5+ messages in thread
From: Bryant G. Ly @ 2018-05-30 13:55 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev, linux-pci; +Cc: mpe, bhelgaas, bryantly
On 5/24/18 7:31 PM, Sam Bobroff wrote:
> EEH recovery currently fails on pSeries for some IOV capable PCI
> devices, if CONFIG_PCI_IOV is on and the hypervisor doesn't provide
> certain device tree properties for the device. (Found on an IOV
> capable device using the ipr driver.)
>
> 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.
>
> Correct this by zeroing the resource flags for IOV BARs when they
> can't be configured.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
> 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.
>
> As with the RFC version, the IOV BARs are disabled by setting the resource
> flags to 0 but the other fields are now left as-is because that is what is done
> elsewhere (see sriov_init() and __pci_read_base()).
>
> I've also examined the concern raised by Bjorn Helgaas, that VFs could be
> enabled later after the BARs are disabled, and it already seems safe: enabling
> VFs (on pseries) depends on another device tree property,
> "ibm,number-of-configurable-vfs" as well as support for the RTAS function
> "ibm_map_pes". Since these are all part of the hypervisor's support for IOV it
> seems unlikely that we would ever see some of them but not all. (None are
> currently provided by QEMU/KVM.) (Additionally, the ipr driver on which the EEH
> recovery failure was discovered doesn't even seem to have SR-IOV support so it
> certainly can't enable VFs.)
>
> Cheers,
> Sam.
> ====== v1 -> v2: ======
>
> Patch 1/1: powerpc/pseries: fix EEH recovery of some IOV devices
> * Moved the BAR disabling code to a function.
> * Also check in pseries_pci_fixup_resources().
>
> ====== v1: ======
>
> Patch 1/1: powerpc/pseries: fix EEH recovery of IOV devices
>
> arch/powerpc/platforms/pseries/setup.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index b55ad4286dc7..0a9e4243ae1d 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -645,6 +645,15 @@ void of_pci_parse_iov_addrs(struct pci_dev *dev, const int *indexes)
> }
> }
>
> +static void pseries_disable_sriov_resources(struct pci_dev *pdev)
> +{
> + int i;
> +
> + pci_warn(pdev, "No hypervisor support for SR-IOV on this device, IOV BARs disabled.\n");
> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> + pdev->resource[i + PCI_IOV_RESOURCES].flags = 0;
> +}
> +
> static void pseries_pci_fixup_resources(struct pci_dev *pdev)
> {
> const int *indexes;
> @@ -652,10 +661,10 @@ static void pseries_pci_fixup_resources(struct pci_dev *pdev)
>
> /*Firmware must support open sriov otherwise dont configure*/
> indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL);
> - if (!indexes)
> - return;
> - /* Assign the addresses from device tree*/
> - of_pci_set_vf_bar_size(pdev, indexes);
> + if (indexes)
> + of_pci_set_vf_bar_size(pdev, indexes);
> + else
> + pseries_disable_sriov_resources(pdev);
> }
>
> static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev)
> @@ -667,10 +676,10 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev)
> return;
> /*Firmware must support open sriov otherwise dont configure*/
> indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL);
> - if (!indexes)
> - return;
> - /* Assign the addresses from device tree*/
> - of_pci_parse_iov_addrs(pdev, indexes);
> + if (indexes)
> + of_pci_parse_iov_addrs(pdev, indexes);
> + else
> + pseries_disable_sriov_resources(pdev);
> }
>
> static resource_size_t pseries_pci_iov_resource_alignment(struct pci_dev *pdev,
Acked-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/1] powerpc/pseries: fix EEH recovery of some IOV devices
2018-05-25 0:31 [PATCH v2 1/1] powerpc/pseries: fix EEH recovery of some IOV devices Sam Bobroff
2018-05-30 13:55 ` Bryant G. Ly
@ 2018-06-30 1:53 ` Bjorn Helgaas
2018-07-02 0:59 ` Michael Ellerman
2 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2018-06-30 1:53 UTC (permalink / raw)
To: Sam Bobroff; +Cc: linuxppc-dev, linux-pci, mpe, bhelgaas, bryantly
On Fri, May 25, 2018 at 10:31:36AM +1000, Sam Bobroff wrote:
> EEH recovery currently fails on pSeries for some IOV capable PCI
> devices, if CONFIG_PCI_IOV is on and the hypervisor doesn't provide
> certain device tree properties for the device. (Found on an IOV
> capable device using the ipr driver.)
>
> 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.
>
> Correct this by zeroing the resource flags for IOV BARs when they
> can't be configured.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
I assume this will be handled by the powerpc folks, since it doesn't touch
drivers/pci. Let me know if you need something from me.
> ---
> 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.
>
> As with the RFC version, the IOV BARs are disabled by setting the resource
> flags to 0 but the other fields are now left as-is because that is what is done
> elsewhere (see sriov_init() and __pci_read_base()).
>
> I've also examined the concern raised by Bjorn Helgaas, that VFs could be
> enabled later after the BARs are disabled, and it already seems safe: enabling
> VFs (on pseries) depends on another device tree property,
> "ibm,number-of-configurable-vfs" as well as support for the RTAS function
> "ibm_map_pes". Since these are all part of the hypervisor's support for IOV it
> seems unlikely that we would ever see some of them but not all. (None are
> currently provided by QEMU/KVM.) (Additionally, the ipr driver on which the EEH
> recovery failure was discovered doesn't even seem to have SR-IOV support so it
> certainly can't enable VFs.)
>
> Cheers,
> Sam.
> ====== v1 -> v2: ======
>
> Patch 1/1: powerpc/pseries: fix EEH recovery of some IOV devices
> * Moved the BAR disabling code to a function.
> * Also check in pseries_pci_fixup_resources().
>
> ====== v1: ======
>
> Patch 1/1: powerpc/pseries: fix EEH recovery of IOV devices
>
> arch/powerpc/platforms/pseries/setup.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index b55ad4286dc7..0a9e4243ae1d 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -645,6 +645,15 @@ void of_pci_parse_iov_addrs(struct pci_dev *dev, const int *indexes)
> }
> }
>
> +static void pseries_disable_sriov_resources(struct pci_dev *pdev)
> +{
> + int i;
> +
> + pci_warn(pdev, "No hypervisor support for SR-IOV on this device, IOV BARs disabled.\n");
> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> + pdev->resource[i + PCI_IOV_RESOURCES].flags = 0;
> +}
> +
> static void pseries_pci_fixup_resources(struct pci_dev *pdev)
> {
> const int *indexes;
> @@ -652,10 +661,10 @@ static void pseries_pci_fixup_resources(struct pci_dev *pdev)
>
> /*Firmware must support open sriov otherwise dont configure*/
> indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL);
> - if (!indexes)
> - return;
> - /* Assign the addresses from device tree*/
> - of_pci_set_vf_bar_size(pdev, indexes);
> + if (indexes)
> + of_pci_set_vf_bar_size(pdev, indexes);
> + else
> + pseries_disable_sriov_resources(pdev);
> }
>
> static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev)
> @@ -667,10 +676,10 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev)
> return;
> /*Firmware must support open sriov otherwise dont configure*/
> indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL);
> - if (!indexes)
> - return;
> - /* Assign the addresses from device tree*/
> - of_pci_parse_iov_addrs(pdev, indexes);
> + if (indexes)
> + of_pci_parse_iov_addrs(pdev, indexes);
> + else
> + pseries_disable_sriov_resources(pdev);
> }
>
> static resource_size_t pseries_pci_iov_resource_alignment(struct pci_dev *pdev,
> --
> 2.16.1.74.g9b0b1f47b
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/1] powerpc/pseries: fix EEH recovery of some IOV devices
2018-05-25 0:31 [PATCH v2 1/1] powerpc/pseries: fix EEH recovery of some IOV devices Sam Bobroff
2018-05-30 13:55 ` Bryant G. Ly
2018-06-30 1:53 ` Bjorn Helgaas
@ 2018-07-02 0:59 ` Michael Ellerman
2018-07-30 1:58 ` Sam Bobroff
2 siblings, 1 reply; 5+ messages in thread
From: Michael Ellerman @ 2018-07-02 0:59 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev, linux-pci; +Cc: bhelgaas, bryantly
Sam Bobroff <sbobroff@linux.ibm.com> writes:
> EEH recovery currently fails on pSeries for some IOV capable PCI
> devices, if CONFIG_PCI_IOV is on and the hypervisor doesn't provide
> certain device tree properties for the device. (Found on an IOV
> capable device using the ipr driver.)
>
> 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.
>
> Correct this by zeroing the resource flags for IOV BARs when they
> can't be configured.
>
> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
> 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.
>
> As with the RFC version, the IOV BARs are disabled by setting the resource
> flags to 0 but the other fields are now left as-is because that is what is done
> elsewhere (see sriov_init() and __pci_read_base()).
>
> I've also examined the concern raised by Bjorn Helgaas, that VFs could be
> enabled later after the BARs are disabled, and it already seems safe: enabling
> VFs (on pseries) depends on another device tree property,
> "ibm,number-of-configurable-vfs" as well as support for the RTAS function
> "ibm_map_pes". Since these are all part of the hypervisor's support for IOV it
> seems unlikely that we would ever see some of them but not all. (None are
> currently provided by QEMU/KVM.) (Additionally, the ipr driver on which the EEH
> recovery failure was discovered doesn't even seem to have SR-IOV support so it
> certainly can't enable VFs.)
Can you fold/reword the above into the change log, it seems like useful
detail.
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] powerpc/pseries: fix EEH recovery of some IOV devices
2018-07-02 0:59 ` Michael Ellerman
@ 2018-07-30 1:58 ` Sam Bobroff
0 siblings, 0 replies; 5+ messages in thread
From: Sam Bobroff @ 2018-07-30 1:58 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-pci, bhelgaas, bryantly
[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]
On Mon, Jul 02, 2018 at 10:59:24AM +1000, Michael Ellerman wrote:
> Sam Bobroff <sbobroff@linux.ibm.com> writes:
>
> > EEH recovery currently fails on pSeries for some IOV capable PCI
> > devices, if CONFIG_PCI_IOV is on and the hypervisor doesn't provide
> > certain device tree properties for the device. (Found on an IOV
> > capable device using the ipr driver.)
> >
> > 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.
> >
> > Correct this by zeroing the resource flags for IOV BARs when they
> > can't be configured.
> >
> > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > ---
> > 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.
> >
> > As with the RFC version, the IOV BARs are disabled by setting the resource
> > flags to 0 but the other fields are now left as-is because that is what is done
> > elsewhere (see sriov_init() and __pci_read_base()).
> >
> > I've also examined the concern raised by Bjorn Helgaas, that VFs could be
> > enabled later after the BARs are disabled, and it already seems safe: enabling
> > VFs (on pseries) depends on another device tree property,
> > "ibm,number-of-configurable-vfs" as well as support for the RTAS function
> > "ibm_map_pes". Since these are all part of the hypervisor's support for IOV it
> > seems unlikely that we would ever see some of them but not all. (None are
> > currently provided by QEMU/KVM.) (Additionally, the ipr driver on which the EEH
> > recovery failure was discovered doesn't even seem to have SR-IOV support so it
> > certainly can't enable VFs.)
>
> Can you fold/reword the above into the change log, it seems like useful
> detail.
>
> cheers
Sure, sounds good.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread