Re: [PATCH 2/2] PCI/AER: Add partial initial support for RCiEPs using RCEC or firmware first

Jonathan Cameron
2020-06-18T16:04:50Z

On Thu, 18 Jun 2020 09:48:29 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 17 Jun 2020 11:25:55 -0700
> "Kuppuswamy, Sathyanarayanan"
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> > On 6/17/20 10:36 AM, Sean V Kelley wrote:
> > > On Tue, 2020-06-16 at 14:24 -0500, Bjorn Helgaas wrote:
> > >> Bcc:
> > >> Subject: Re: [PATCH 2/2] PCI/AER: Add partial initial support for
> > >> RCiEPs
> > >> using RCEC or firmware first
> > >> Reply-To:
> > >> In-Reply-To:
> > >> <20200521173134.2456773-3-Jonathan.Cameron@huawei.com>
> > >>
> > >> [+cc Sathy, Sean]
> > >>
> > >> On Fri, May 22, 2020 at 01:31:34AM +0800, Jonathan Cameron
> > >> wrote:
> > >>> Note this provides complete support for our usecase on an ARM
> > >>> server using
> > >>> Hardware Reduced ACPI and adds appropriate place for an RCEC
> > >>> driver to hook
> > >>> if someone else cares to write one, either for firmware first
> > >>> handling on
> > >>> non Hardware Reduced ACPI or for kernel first AER handling.
> > >>
> > >> This provides complete support? I'm really confused, since this
> > >> relies on dev->rcec, which is never set. And I don't see
> > >> anything about hooks for RCEC drivers.
> > >>
> > >>> For Root Complex integrated End Points (RCiEPs) there is no root
> > >>> port to
> > >>> discover and hence we cannot walk the bus from the root port to
> > >>> do appropriate resets.
> > >>>
> > >>> The PCI specification provides Root Complex Event Collectors to
> > >>> deal with
> > >>> this circumstance. These are peer RCiEPs that provide (amongst
> > >>> other
> > >>> things) collection + interrupt facilities for AER reporting for
> > >>> a set of
> > >>> RCiEPs in the same root complex.
> > >>>
> > >>> In the case of a Hardware Reduced ACPI platform, the AER errors
> > >>> are reported via a GHESv2 path using CPER records as defined in
> > >>> the UEFI
> > >>> specification. These are intended to provide complete
> > >>> information and
> > >>> appropriate hand shake in a fashion that does not require a
> > >>> specific form
> > >>> of error reporting hardware. This is contrast to AER handling
> > >>> via the
> > >>> various HEST entries for PCI Root Port and PCI Device etc where
> > >>> we do
> > >>> require direct access to the RCEC.
> > >>
> > >> Can you include pointers to relevant spec sections for these
> > >> differences between hardware-reduced and other platforms?
> > >>
> > >> This patch doesn't seem to depend on anything about ACPI, APEI,
> > >> firmware-first, or hardware-reduced platforms.
> > >>
> > >>> As such my interpretation of the spec is that a Reduced Hardware
> > >>> ACPI
> > >>> platform should not access the RCEC from the OS at all during
> > >>> AER handling,
> > >>> and in fact is welcome to use non standard hardware interfaces
> > >>> to provide
> > >>> the equivalent functionality in any fashion it wishes (as all
> > >>> hidden beind
> > >>> the firmware).
> > >
> > >
> > > I'm not sure what you mean by Hardware Reduced ACPI platform, but
> > > you seem to be implying that in this case your hardware lacks
> > > RCECs and so are using firmware specific handling.
> > >
> > > In 1.3.2.3 (Root Complex Integrated Endpoint rules)
> > >
> > > If an RCiEP is associated with an optional Root Complex Event
> > > Collector it must signal PME and error conditions through a Root
> > > Complex Event Collector.
> > >
> > > If the RCEC is not supported/present then the expectation prior
> > > to PCIe 5 is that the RCiEP will use the same mechanism as PCI
> > > systems. However, if the RCiEP asserts say an error signal there
> > > is no Root Port and the OS has no way of knowing what interrupt
> > > the error is conntected to. Linux doesn't have support for that
> > > and this was discussed prior here:
> > >
> > > https://lore.kernel.org/lkml/20190709134538.GA35486@google.com/
> > >
> > > In such a case, are you then implying that the _OSC method is not
> > > granting control of PCIe Native Power Management Events to the OS
> > > and so are falling back to your defined ACPI mechanism on your
> > > platform?
> > >
> > > I'm currently working on adding support for RCECs in AER that
> > > would make use of the extended capabilities for identifying the
> > > assocated RCeIPs for purposes of the PME and error condition
> > > signaling.
> >
> > IIUC, we are trying to solve multiple issues here.
> >
> > 1. Error detection and recovery support for RCiEPs and RCEC.
> > 2. Firmware first exception for case 1.
> > 3. AEPI based handling for case 1 (I think this is the case
> > Jonathan is trying to handle)
>
> I'm not sure it separates that cleanly but the flow I'm interested
> in is firmware first with errors reported using APEI / GHESv2 etc.
>
> In particular without an RCEC, as it should (I think) play no part
> in that path. One of the main aims of me bringing this forwards at
> this stage is to establish whether I need to get our hardware teams
> to put an RCEC in place for future hardware or not. Right now we have
> some work arounds in place as we can reroute some of these errors
> directly to device interrupts.
>
> We haven't been able to come up with a reason why we need an RCEC
> given our approach to error handling.
>
> >
> > For adding support for case 1,
> >
> > 1. I think we need to first make the AER driver RCEC aware.
> > 2. Once AER driver is modified to receive IRQs for RCEC error
> > events, then we can modify pcie_do_recovery() to handle
> > recovery for RCiEPs and RCEC.
> >
> > I recommend adding support for basic case first and then add
> > exceptions for Firmware First and AEPI based support.
>
> Sounds good as long as progress is timely.
>
> As both Bjorn and yourself have suggested, we can perhaps
> make pci_walk_bus work when it is passed an RCiEP directly.
>
> If that an be done, the case I care about should work with very
> minimal changes.
>
> I'll take a look at how cleanly that can be done.
>
> >
> > >
> > > Thanks,
> > >
> > > Sean
> > >
> > >
> > >>
> > >> A pointer to the spec you're interpreting would be helpful here,
> > >> too.
> > >>
> > >> s/Reduced Hardware/Hardware-Reduced/ to match terminology in spec
> > >> (I'm
> > >> looking at ACPI v6.3, sec 4.1). Also below in code comments.
> > >>
> > >> s/beind/behind/
> > >>
> > >>> Hence I am making the provision of an RCEC optional.
> > >>>
> > >>> The aim of the rest of the code was to replicate the actions
> > >>> that would
> > >>> have occurred if this had been an EP below a root port. Some of
> > >>> them make
> > >>> absolutely no sense, but I hope this RFC can start a discussion
> > >>> on what
> > >>> we should be doing under these circumstances.
> > >>>
> > >>> It probably makes sense to pull this new block of code out to a
> > >>> separate
> > >>> function but for the RFC I've left it in place to keep it next
> > >>> to the
> > >>> existing path.
> > >>
> > >> OK, my comment is: I really hope we don't need a separate path.
> > >> If we
> > >> need a test or two for RCiEPs, that's fine. But two paths sounds
> > >> like
> > >> a nightmare to maintain.
> > >>
> > >>> It appears that the current kernel first code does not support
> > >>> detecting
> > >>> the multiple error bits being set in the root port error status
> > >>> register.
> > >>> This seems like a limitation both the normal EP / Root Port case
> > >>> and
> > >>> for RCiEPs.
> > >>
> > >> Is this paragraph supposed to be a bug report? It doesn't seem
> > >> to say
> > >> anything about what *this* patch does. Maybe this should be
> > >> part of the commit log for a separate patch?
> > >>
> > >>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >>> ---
> > >>> drivers/pci/pcie/err.c | 61
> > >>> ++++++++++++++++++++++++++++++++++++++++++
> > >>> include/linux/pci.h | 1 +
> > >>> 2 files changed, 62 insertions(+)
> > >>>
> > >>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > >>> index 14bb8f54723e..d34be4483f73 100644
> > >>> --- a/drivers/pci/pcie/err.c
> > >>> +++ b/drivers/pci/pcie/err.c
> > >>> @@ -153,6 +153,67 @@ pci_ers_result_t pcie_do_recovery(struct
> > >>> pci_dev *dev,
> > >>> pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
> > >>> struct pci_bus *bus;
> > I am curious what bus (dev->subordinate) does RCEC and RCiEP
> > belongs to ?
>
> I'm not quite sure what you are asking so...
>
> They are effectively the same as root ports, and so sit on a bus
> specified via ACPI. In our case IORT. The root complex includes a
> bunch of bus numbers on which these devices can be discovered.
> 0, 74, 78, 7a, 7b, 7c, 80, b4, ba, bb, b8, bc on the 2 socket machine
> I have to hand. Those buses have a mix of root ports, and RCiEPs on
> them.
>
> As to subordinate (i.e. where they bridge to) I don't think it would
> have any meaning. For reference I checked one of our RCiEPs and
> it's set to 0 on the config space.
>
>
> > Does all RCiEPs are in same bus ?
>
> No, though for any given RCiEP the associated RCEC would have to be
> on the same bus. So in the above example we would need quite a few
> of them.

One thing to note is we could minimize the changes to the code by
getting the bus on which a device resides directly rather than
bouncing via the root port and back again.

Something like
- if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
- pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
- dev = dev->bus->self;
- bus = dev->subordinate;
-
+ if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
+ bus = dev->subordinate;
+ else
+ bus = dev->bus;
+ if (bus->self)
+ dev = bus->self;

(with some special handling to avoid calling root port specific
parts on the RCiEP)

We could check pci_is_root_bus for that last check and it might be
slightly more informative.

But then an AER error on an RCiEP resets everything on the same bus
within the Root Complex.

I don't think we want to do this as the same logic about resetting the
PCIe bus and everything below doesn't really apply.

Hence I'll try the pci_walk_bus variant as previously suggested.

Jonathan

>
> > >>>
> > >>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> > Also, instead of creating a new path for RCiEPs, I recommend fixing
> > the pci_walk_bus() part. That will reduce code duplication.
> > >>> + struct pci_dev *rcec = dev->rcec;
> > >>> + /* Not clear this makes any sense - we can't
> > >>> reset link anyway...*/
> > >>> + if (state == pci_channel_io_frozen) {
> > >>> + report_frozen_detected(dev, &status);
> > >>> + pci_err(dev, "io is frozen and cannot
> > >>> reset link\n");
> > >>> + goto failed;
> > >>> + } else {
> > >>> + report_normal_detected(dev, &status);
> > >>> + }
> > >>
> > >> I don't understand where you're going with this. I think you're
> > >> adding recovery for RCiEPs (PCI_EXP_TYPE_RC_END). It's true that
> > >> there's no link leading to them, but we should still be able to
> > >> reset the RCiEP (not the RCEC) via FLR, if it supports that.
> > >>
> > >> And all the driver callbacks should be for the RCiEP, not the
> > >> RCEC, shouldn't they? I really hope we can avoid duplicating
> > >> this whole path. It will be hard to keep the two paths in sync.
> > >>
> > >>> + if (status == PCI_ERS_RESULT_CAN_RECOVER) {
> > >>> + status = PCI_ERS_RESULT_RECOVERED;
> > >>> + pci_dbg(dev, "broadcast mmio_enabled
> > >>> message\n");
> > >>> + report_mmio_enabled(dev, &status);
> > >>> + }
> > >>> +
> > >>> + if (status == PCI_ERS_RESULT_NEED_RESET) {
> > >>> + /* No actual slot reset possible */
> > >>> + status = PCI_ERS_RESULT_RECOVERED;
> > >>> + pci_dbg(dev, "broadcast slot_reset
> > >>> message\n");
> > >>> + report_slot_reset(dev, &status);
> > >>> + }
> > >>> +
> > >>> + if (status != PCI_ERS_RESULT_RECOVERED)
> > >>> + goto failed;
> > >>> +
> > >>> + report_resume(dev, &status);
> > >>> +
> > >>> + /*
> > >>> + * These two should be called on the RCEC -
> > >>> but in case
> > >>> + * of firmware first they should be no-ops.
> > >>> Given that
> > >>> + * in a reduced hardware ACPI system, it is
> > >>> possible there
> > >>> + * is no standard compliant RCEC at all.
> > >>> + *
> > >>> + * Add some sort of check on what type of HEST
> > >>> entries we have?
> > >>> + */
> > >>> + if (rcec) {
> > >>> + /*
> > >>> + * Unlike the upstream port case for
> > >>> an EP, we have not
> > >>> + * issued a reset on all device the
> > >>> RCEC handles, so
> > >>> + * perhaps we should be more careful
> > >>> about resetting
> > >>> + * the status registers on the RCEC?
> > >>> + *
> > >>> + * In particular we may need provide a
> > >>> means to handle
> > >>> + * the multiple error bits being set in
> > >>> PCI_ERR_ROOT_STATUS
> > >>> + */
> > >>> + pci_aer_clear_device_status(rcec);
> > >>> + pci_aer_clear_nonfatal_status(rcec);
> > >>> + /*
> > >>> + * Non RCiEP case uses the downstream
> > >>> port above the device
> > >>> + * for this message.
> > >>> + */
> > >>> + pci_info(rcec, "device recovery
> > >>> successful\n");
> > >>> + } else {
> > >>> + pci_info(dev, "device recovery
> > >>> successful\n");
> > >>> + }
> > >>> +
> > >>> + return status;
> > >>> + }
> > >>> +
> > >>> /*
> > >>> * Error recovery runs on all subordinates of the
> > >>> first downstream port.
> > >>> * If the downstream port detected the error, it is
> > >>> cleared at the end.
> > >>> diff --git a/include/linux/pci.h b/include/linux/pci.h
> > >>> index 83ce1cdf5676..cb21dfe05f8c 100644
> > >>> --- a/include/linux/pci.h
> > >>> +++ b/include/linux/pci.h
> > >>> @@ -298,6 +298,7 @@ struct pci_dev {
> > >>> struct list_head bus_list; /* Node in per-bus
> > >>> list */ struct pci_bus *bus; /* Bus this
> > >>> device is on */ struct pci_bus *subordinate; /*
> > >>> Bus this device bridges to */
> > >>> + struct pci_dev *rcec; /* Root
> > >>> Complex Event Collector used */
> > >>
> > >> Nothing ever sets this, so I guess the critical connection
> > >> between RCiEP and RCEC is missing? Each patch needs to make
> > >> sense on its own,
> > >> so the patch that adds this struct member should also add
> > >> something that sets it and uses it.
> > >>
> > >>> void *sysdata; /* Hook for
> > >>> sys-specific extension */
> > >>> struct proc_dir_entry *procent; /* Device
> > >>> entry in /proc/bus/pci */
> > >>> --
> > >>> 2.19.1
> > >>>
> > >
> >
>
>
> _______________________________________________
> Linuxarm mailing list
> Linuxarm@huawei.com
> http://hulk.huawei.com/mailman/listinfo/linuxarm