Re: [PATCH 0/8]

[Tyrel Datwyler <tyreld@linux.vnet.ibm.com>]

On 03/19/2019 07:58 PM, Sam Bobroff wrote:
> Hi all,
> 
> This patch set adds support for EEH recovery of hot plugged devices on pSeries
> machines. Specifically, devices discovered by PCI rescanning using
> /sys/bus/pci/rescan, which includes devices hotplugged by QEMU's device_add
> command. (pSeries doesn't currently use slot power control for hotplugging.)

Slight nit that its not that pSeries doesn't support slot power control
hotplugging, its that QEMU pSeries guests don't support it. We most definitely
use the slot power control for hotplugging in PowerVM pSeries Linux guests. More
specifically we had to work around short comings in the rpaphp driver when
dealing with QEMU. This being that while at initial glance the design implies
that it had multiple devices per PHB in mind, it didn't, and only actually
supported a single slot per PHB. Further, when we developed the QEMU pci hotplug
feature we had to deal with only having a single PHB per QEMU guest and as a
result needed a way to plug multiple pci devices into a single PHB. Hence, came
the pci rescan work around in drmgr.

Mike Roth and I have had discussions over the years to get the slot power
control hotplugging working properly with QEMU, and while I did get the RPA
hotplug driver fixed to register all available slots associated with a PHB, EEH
remained an issue. So, I'm very happy to see this patchset get that working with
the rescan work around.

> 
> As a side effect this also provides EEH support for devices removed by
> /sys/bus/pci/devices/*/remove and re-discovered by writing to /sys/bus/pci/rescan,
> on all platforms.

+1, this seems like icing on the cake. ;)

> 
> The approach I've taken is to use the fact that the existing
> pcibios_bus_add_device() platform hooks (which are used to set up EEH on
> Virtual Function devices (VFs)) are actually called for all devices, so I've
> widened their scope and made other adjustments necessary to allow them to work
> for hotplugged and boot-time devices as well.
> 
> Because some of the changes are in generic PowerPC code, it's
> possible that I've disturbed something for another PowerPC platform. I've tried
> to minimize this by leaving that code alone as much as possible and so there
> are a few cases where eeh_add_device_{early,late}() or eeh_add_sysfs_files() is
> called more than once. I think these can be looked at later, as duplicate calls
> are not harmful.
> 
> The patch "Convert PNV_PHB_FLAG_EEH" isn't strictly necessary and I'm not sure
> if it's better to keep it, because it simplifies the code or drop it, because
> we may need a separate flag per PHB later on. Thoughts anyone?
> 
> The first patch is a rework of the pcibios_init reordering patch I posted
> earlier, which I've included here because it's necessary for this set.
> 
> I have done some testing for PowerNV on Power9 using a modified pnv_php module
> and some testing on pSeries with slot power control using a modified rpaphp
> module, and the EEH-related parts seem to work.

I'm interested in what modifications with rpaphp. Its unclear if you are saying
rpaphp modified so that slot power hotplug works with a QEMU pSeries guest? If
thats the case it would be optimal to get that upstream and remove the work
rescan workaround for guests that don't need it.

-Tyrel

> 
> Cheers,
> Sam.
> 
> Sam Bobroff (8):
>   powerpc/64: Adjust order in pcibios_init()
>   powerpc/eeh: Clear stale EEH_DEV_NO_HANDLER flag
>   powerpc/eeh: Convert PNV_PHB_FLAG_EEH to global flag
>   powerpc/eeh: Improve debug messages around device addition
>   powerpc/eeh: Add eeh_show_enabled()
>   powerpc/eeh: Initialize EEH address cache earlier
>   powerpc/eeh: EEH for pSeries hot plug
>   powerpc/eeh: Remove eeh_probe_devices() and eeh_addr_cache_build()
> 
>  arch/powerpc/include/asm/eeh.h               | 19 +++--
>  arch/powerpc/kernel/eeh.c                    | 33 ++++-----
>  arch/powerpc/kernel/eeh_cache.c              | 29 +-------
>  arch/powerpc/kernel/eeh_driver.c             |  4 ++
>  arch/powerpc/kernel/of_platform.c            |  3 +-
>  arch/powerpc/kernel/pci-common.c             |  4 --
>  arch/powerpc/kernel/pci_32.c                 |  4 ++
>  arch/powerpc/kernel/pci_64.c                 | 12 +++-
>  arch/powerpc/platforms/powernv/eeh-powernv.c | 41 +++++------
>  arch/powerpc/platforms/powernv/pci.c         |  7 +-
>  arch/powerpc/platforms/powernv/pci.h         |  2 -
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 75 +++++++++++---------
>  arch/powerpc/platforms/pseries/pci.c         |  7 +-
>  13 files changed, 122 insertions(+), 118 deletions(-)
>