Message-ID: <318974284.1316210.1750437945118.JavaMail.zimbra@raptorengineeringinc.com>
Date: Fri, 20 Jun 2025 11:45:45 -0500 (CDT)
From: Timothy Pearson <tpearson@...torengineering.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: Bjorn Helgaas <helgaas@...nel.org>, 
	linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>, 
	linux-kernel <linux-kernel@...r.kernel.org>, 
	linux-pci <linux-pci@...r.kernel.org>, 
	Madhavan Srinivasan <maddy@...ux.ibm.com>, 
	Michael Ellerman <mpe@...erman.id.au>, 
	christophe leroy <christophe.leroy@...roup.eu>, 
	Naveen N Rao <naveen@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, 
	Shawn Anastasio <sanastasio@...torengineering.com>
Subject: Re: [PATCH v2 2/6] pci/hotplug/pnv_php: Work around switches with
 broken



----- Original Message -----
> From: "Lukas Wunner" <lukas@...ner.de>
> To: "Timothy Pearson" <tpearson@...torengineering.com>
> Cc: "Bjorn Helgaas" <helgaas@...nel.org>, "linuxppc-dev" <linuxppc-dev@...ts.ozlabs.org>, "linux-kernel"
> <linux-kernel@...r.kernel.org>, "linux-pci" <linux-pci@...r.kernel.org>, "Madhavan Srinivasan" <maddy@...ux.ibm.com>,
> "Michael Ellerman" <mpe@...erman.id.au>, "christophe leroy" <christophe.leroy@...roup.eu>, "Naveen N Rao"
> <naveen@...nel.org>, "Bjorn Helgaas" <bhelgaas@...gle.com>, "Shawn Anastasio" <sanastasio@...torengineering.com>
> Sent: Friday, June 20, 2025 2:52:55 AM
> Subject: Re: [PATCH v2 2/6] pci/hotplug/pnv_php: Work around switches with broken

> On Thu, Jun 19, 2025 at 02:29:33PM -0500, Timothy Pearson wrote:
>> To be perfectly frank the existing code quality in this driver
>> (and the associated EEH driver) is not the best, and it's been
>> a frustrating experience trying to hack it into semi-stable
>> operation.
>> 
>> I would vastly prefer to rewrite / integrate into the pciehp driver,
>> and we have plans to do so, but that will take an unacceptable amount
>> of time vs. trying to fix up the existing driver as a stopgap.
>> 
>> As you mentioned, pciehp already has this fix, so we just have to
>> deal with the duplicated code until we (Raptor) figures out how to
>> merge PowerNV support into pciehp.
> 
> I don't know how much PCIe hotplug on PowerNV differs from native,
> spec-compliant PCIe hotplug.  If the differences are vast (and I
> get the feeling they might be if I read terms like "PHB" and
> "EEH unfreeze", which sound completely foreign to me), it might
> be easier to refactor pnv_php.c and copy patterns or code from
> pciehp, than to integrate the functionality from pnv_php.c into
> pciehp.

The differences at the root level (PHB) are quite significant -- the controller is more advanced in many ways than standard PCIe root complexes [1] -- and those differences need very special handling.  Once we are looking at devices downstream of the root complex, standard PCIe hotplug and AER specifications apply, so we're in a strange spot of really wanting to use pciehp (to handle all nested downstream bridges), but pciehp still needs to understand how to deal with our unqiue root complex.

One idea I had was to use the existing modularity of pciehp's source and add a new pciehp_powernv.c file with all of our root complex handling methods.  We could then #ifdef swap the assocated root complex calls to that external file when compiled in PowerNV mode.

> pciehp does carry some historic baggage of its own (such as poll mode),
> which you may not want to deal with on PowerNV.

At the root level we probably don't care about polling mode, but in terms of nested downtream bridges polling may still be desireable.  I don't have a strong opinion either way.

> One thing I don't quite understand is, it sounds like you've
> attached a PCIe switch to a Root Port and the hotplug ports
> are on the PCIe switch.  Aren't those hotplug ports just
> bog-standard ones that can be driven by pciehp?  My expectation
> would have been that a PowerNV-specific hotplug driver would
> only be necessary for hotplug-capable Root Ports.

That's the problem -- the pciehp driver assumes x86 root ports, and the powernv driver ends up duplicating (badly) parts of the pciehp functionality for downstream bridges.  That's one reason I'd like to abstract the root port handling in pciehp and eventually move the PowerNV root port handling into that module.

> Thanks,
> 
> Lukas

[1] Among other interesting differences, it is both capable of and has been tested to properly block and report all invalid accesses from a PCIe device to system memory.  This breaks assumptions in many PCIe device drivers, but is also a significant security advantage.  EEH freeze is effectively this security mechanism kicking in -- on detecting an invalid access, the PHB itself will block the access and put the PE into a frozen state where no PCIe traffic is permitted.  It simultaneously rases an error to the host (colloquially referred to as EEH) and punts the decision making on whether to reset or resume the device to the kernel itself.  We've caught various cards doing fun things like reading host RAM or trying to write to 0x0 via this mechanism, one of the most egregious was a Chinese telecom card that apparently tries to reset the host with a write to low memory on detecting an interrupt servicing delay (!).

Powered by blists - more mailing lists