[PATCH RFCv4 0/4] EEH Support for VFIO PCI device

[PATCH RFCv4 0/4] EEH Support for VFIO PCI device

[Gavin Shan]

The series of patches intends to support EEH for PCI devices, which are
passed through to PowerKVM based guest via VFIO. The implementation is
straightforward based on the issues or problems we have to resolve to
support EEH for PowerKVM based guest.

- Emulation for EEH RTAS requests. All EEH RTAS requests goes to QEMU firstly.
  If QEMU can't handle it, the request will be sent to host via newly introduced
  VFIO container IOCTL command (VFIO_EEH_OP) and gets handled in host kernel.

The series of patches requires corresponding QEMU changes.

Change log
==========
v1 -> v2:
        * EEH RTAS requests are routed to QEMU, and then possiblly to host kerenl.
          The mechanism KVM in-kernel handling is dropped.
        * Error injection is reimplemented based syscall, instead of KVM in-kerenl
          handling. The logic for error injection token management is moved to
          QEMU. The error injection request is routed to QEMU and then possiblly
          to host kernel.
v2 -> v3:
        * Make the fields in struct eeh_vfio_pci_addr, struct vfio_eeh_info based
          on the comments from Alexey.
        * Define macros for EEH VFIO operations (Alexey).
        * Clear frozen state after successful PE reset.
        * Merge original [PATCH 1/2/3] to one.
v3 -> v4:
        * Remove the error injection from the patchset. Mike or I will work on that
          later.
        * Rename CONFIG_VFIO_EEH to VFIO_PCI_EEH.
        * Rename the IOCTL command to VFIO_EEH_OP and it's handled by VFIO-PCI device
          instead of VFIO container.
        * Rename the IOCTL argument structure to "vfio_eeh_op" accordingly. Also, more
          fields added to hold return values for RTAS requests.
        * The address mapping stuff is totally removed. When opening or releasing VFIO
          PCI device, notification sent to EEH to update the flags indicates the device
          is passed to guest or not.
        * Change pr_warn() to pr_debug() to avoid DOS as pointed by Alex.W
        * Argument size check issue pointed by Alex.W.

Testing on P7
=============

- Emulex adapter

Testing on P8
=============
Need testing for more after the code is finalized. The logic is required by P7/P8
machines shouldn't be different.

Gavin Shan (4):
  drivers/vfio: Introduce CONFIG_VFIO_PCI_EEH
  powerpc/eeh: Flags for passed device and PE
  drivers/vfio: New IOCTL command VFIO_EEH_INFO
  powerpc/eeh: Avoid event on passed PE

 arch/powerpc/include/asm/eeh.h            |  32 +++
 arch/powerpc/kernel/eeh.c                 |   8 +
 arch/powerpc/platforms/powernv/Makefile   |   1 +
 arch/powerpc/platforms/powernv/eeh-ioda.c |   3 +-
 arch/powerpc/platforms/powernv/eeh-vfio.c | 445 ++++++++++++++++++++++++++++++
 drivers/vfio/pci/Kconfig                  |   6 +
 drivers/vfio/pci/vfio_pci.c               |  24 +-
 drivers/vfio/pci/vfio_pci_private.h       |  16 ++
 include/uapi/linux/vfio.h                 |  43 +++
 9 files changed, 571 insertions(+), 7 deletions(-)
 create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c

-- 
1.8.3.2

[PATCH 1/4] drivers/vfio: Introduce CONFIG_VFIO_PCI_EEH

[Gavin Shan]

The patch introduces CONFIG_VFIO_PCI_EEH for more IOCTL commands
on VFIO PCI device support EEH funtionality for PCI devices that
are passed through from host to guest.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/vfio/pci/Kconfig | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index c41b01e..dd0e0f0 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -1,6 +1,7 @@
 config VFIO_PCI
 	tristate "VFIO support for PCI devices"
 	depends on VFIO && PCI && EVENTFD
+	select VFIO_PCI_EEH if PPC_POWERNV
 	help
 	  Support for the PCI VFIO bus driver.  This is required to make
 	  use of PCI drivers using the VFIO framework.
@@ -16,3 +17,8 @@ config VFIO_PCI_VGA
 	  BIOS and generic video drivers.
 
 	  If you don't know what to do here, say N.
+
+config VFIO_PCI_EEH
+	tristate
+	depends on EEH && VFIO_IOMMU_SPAPR_TCE
+	default n
-- 
1.8.3.2

[PATCH 2/4] powerpc/eeh: Flags for passed device and PE

[Gavin Shan]

The patch introduces new flags for EEH device and PE to indicate
that the device or PE has been passed through to guest. In turn,
we will deliver EEH errors to guest for further handling, which
will be done in subsequent patches.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/eeh.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 7782056..34a2d83 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -72,6 +72,7 @@ struct device_node;
 #define EEH_PE_RESET		(1 << 2)	/* PE reset in progress	*/
 
 #define EEH_PE_KEEP		(1 << 8)	/* Keep PE on hotplug	*/
+#define EEH_PE_PASSTHROUGH	(1 << 9)	/* PE owned by guest	*/
 
 struct eeh_pe {
 	int type;			/* PE type: PHB/Bus/Device	*/
@@ -93,6 +94,21 @@ struct eeh_pe {
 #define eeh_pe_for_each_dev(pe, edev, tmp) \
 		list_for_each_entry_safe(edev, tmp, &pe->edevs, list)
 
+static inline bool eeh_pe_passed(struct eeh_pe *pe)
+{
+	return pe ? !!(pe->state & EEH_PE_PASSTHROUGH) : false;
+}
+
+static inline void eeh_pe_set_passed(struct eeh_pe *pe, bool passed)
+{
+	if (pe) {
+		if (passed)
+			pe->state |= EEH_PE_PASSTHROUGH;
+		else
+			pe->state &= ~EEH_PE_PASSTHROUGH;
+	}
+}
+
 /*
  * The struct is used to trace EEH state for the associated
  * PCI device node or PCI device. In future, it might
@@ -110,6 +126,7 @@ struct eeh_pe {
 #define EEH_DEV_SYSFS		(1 << 9)	/* Sysfs created	*/
 #define EEH_DEV_REMOVED		(1 << 10)	/* Removed permanently	*/
 #define EEH_DEV_FRESET		(1 << 11)	/* Fundamental reset	*/
+#define EEH_DEV_PASSTHROUGH	(1 << 12)	/* Owned by guest	*/
 
 struct eeh_dev {
 	int mode;			/* EEH mode			*/
@@ -138,6 +155,21 @@ static inline struct pci_dev *eeh_dev_to_pci_dev(struct eeh_dev *edev)
 	return edev ? edev->pdev : NULL;
 }
 
+static inline bool eeh_dev_passed(struct eeh_dev *dev)
+{
+	return dev ? !!(dev->mode & EEH_DEV_PASSTHROUGH) : false;
+}
+
+static inline void eeh_dev_set_passed(struct eeh_dev *dev, bool passed)
+{
+	if (dev) {
+		if (passed)
+			dev->mode |= EEH_DEV_PASSTHROUGH;
+		else
+			dev->mode &= ~EEH_DEV_PASSTHROUGH;
+	}
+}
+
 /* Return values from eeh_ops::next_error */
 enum {
 	EEH_NEXT_ERR_NONE = 0,
-- 
1.8.3.2

[PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Gavin Shan]

The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
to support EEH functionality for PCI devices, which have been
passed from host to guest via VFIO.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/Makefile   |   1 +
 arch/powerpc/platforms/powernv/eeh-vfio.c | 445 ++++++++++++++++++++++++++++++
 drivers/vfio/pci/vfio_pci.c               |  24 +-
 drivers/vfio/pci/vfio_pci_private.h       |  16 ++
 include/uapi/linux/vfio.h                 |  43 +++
 5 files changed, 523 insertions(+), 6 deletions(-)
 create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c

diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
index 63cebb9..45cd833 100644
--- a/arch/powerpc/platforms/powernv/Makefile
+++ b/arch/powerpc/platforms/powernv/Makefile
@@ -6,5 +6,6 @@ obj-y			+= opal-msglog.o
 obj-$(CONFIG_SMP)	+= smp.o
 obj-$(CONFIG_PCI)	+= pci.o pci-p5ioc2.o pci-ioda.o
 obj-$(CONFIG_EEH)	+= eeh-ioda.o eeh-powernv.o
+obj-$(CONFIG_VFIO_PCI_EEH)	+= eeh-vfio.o
 obj-$(CONFIG_PPC_SCOM)	+= opal-xscom.o
 obj-$(CONFIG_MEMORY_FAILURE)	+= opal-memory-errors.o
diff --git a/arch/powerpc/platforms/powernv/eeh-vfio.c b/arch/powerpc/platforms/powernv/eeh-vfio.c
new file mode 100644
index 0000000..11adc55
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/eeh-vfio.c
@@ -0,0 +1,445 @@
+/*
+  * The file intends to support EEH funtionality for those PCI devices,
+  * which have been passed through from host to guest via VFIO. So this
+  * file is naturally part of VFIO implementation on PowerNV platform.
+  *
+  * Copyright Benjamin Herrenschmidt & Gavin Shan, IBM Corporation 2014.
+  *
+  * This program is free software; you can redistribute it and/or modify
+  * it under the terms of the GNU General Public License as published by
+  * the Free Software Foundation; either version 2 of the License, or
+  * (at your option) any later version.
+  */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/kvm_host.h>
+#include <linux/msi.h>
+#include <linux/pci.h>
+#include <linux/string.h>
+#include <linux/vfio.h>
+
+#include <asm/eeh.h>
+#include <asm/eeh_event.h>
+#include <asm/io.h>
+#include <asm/iommu.h>
+#include <asm/opal.h>
+#include <asm/msi_bitmap.h>
+#include <asm/pci-bridge.h>
+#include <asm/ppc-pci.h>
+#include <asm/tce.h>
+#include <asm/uaccess.h>
+
+#include "powernv.h"
+#include "pci.h"
+
+static int powernv_eeh_vfio_check_dev(struct pci_dev *pdev,
+				      struct eeh_dev **pedev,
+				      struct eeh_pe **ppe,
+				      struct pnv_phb **pphb)
+{
+	struct eeh_dev *edev;
+	struct pnv_phb *phb;
+
+	/* No device ? */
+	if (!pdev)
+		return -ENODEV;
+
+	edev = pci_dev_to_eeh_dev(pdev);
+	if (!edev || !eeh_dev_passed(edev) ||
+	    !edev->pe || !eeh_pe_passed(edev->pe))
+		return -ENODEV;
+
+	/* EEH isn't supported ? */
+	phb = edev->phb->private_data;
+	if (!(phb->flags & PNV_PHB_FLAG_EEH))
+		return -EACCES;
+
+	if (pedev)
+		*pedev = edev;
+	if (ppe)
+		*ppe = edev->pe;
+	if (pphb)
+		*pphb = phb;
+
+	return 0;
+}
+
+static int powernv_eeh_vfio_set_option(struct pci_dev *pdev,
+				       struct vfio_eeh_op *info)
+{
+	struct eeh_dev *edev;
+	struct eeh_pe *pe;
+	struct pnv_phb *phb;
+	int opcode = info->option.option;
+	int ret = 0;
+
+	/* Device existing ? */
+	ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+	if (ret) {
+		pr_debug("%s: Cannot find device\n",
+			__func__);
+		info->option.ret = -7;
+		goto out;
+	}
+
+	/* Invalid opcode ? */
+	if (opcode < EEH_OPT_DISABLE ||
+	    opcode > EEH_OPT_THAW_DMA) {
+		pr_debug("%s: Opcode#%d out of range (%d, %d)\n",
+			 __func__, opcode, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
+		info->option.ret = -3;
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (opcode == EEH_OPT_DISABLE ||
+	    opcode == EEH_OPT_ENABLE) {
+		info->option.ret = 0;
+	} else {
+		if (!phb->eeh_ops || !phb->eeh_ops->set_option) {
+			info->option.ret = -7;
+			ret = -ENOENT;
+			goto out;
+		}
+
+		ret = phb->eeh_ops->set_option(pe, opcode);
+		if (ret) {
+			pr_debug("%s: Failure %d from backend\n",
+				__func__, ret);
+			info->option.ret = -3;
+			goto out;
+		}
+
+		info->option.ret = 0;
+	}
+out:
+	return ret;
+}
+
+static int powernv_eeh_vfio_get_addr(struct pci_dev *pdev,
+				     struct vfio_eeh_op *info)
+{
+	struct pci_bus *bus;
+	struct eeh_dev *edev;
+	struct eeh_pe *pe;
+	struct pnv_phb *phb;
+	int opcode = info->addr.option;
+	int ret = 0;
+
+	/* Device existing ? */
+	ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+	if (ret) {
+		info->addr.ret = -3;
+		goto out;
+	}
+
+	/* Invalid opcode ? */
+	if (opcode != 0 && opcode != 1) {
+		pr_debug("%s: opcode %d out of range (0, 1)\n",
+			__func__, opcode);
+		info->addr.ret = -3;
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * Fill result according to opcode. We don't differentiate
+	 * PCI bus and device sensitive PE here.
+	 */
+	if (opcode == 0) {
+		bus = eeh_pe_bus_get(pe);
+		if (!bus) {
+			info->addr.ret = -3;
+			ret = -ENODEV;
+			goto out;
+		}
+
+		info->addr.ret = 0;
+		info->addr.info = bus->number << 16;
+	} else {
+		info->addr.info = 1;
+		info->addr.ret = 1;
+	}
+out:
+	return ret;
+}
+
+static int powernv_eeh_vfio_get_state(struct pci_dev *pdev,
+				      struct vfio_eeh_op *info)
+{
+	struct eeh_dev *edev;
+	struct eeh_pe *pe;
+	struct pnv_phb *phb;
+	int result, ret = 0;
+
+	/* Device existing ? */
+	ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+	if (ret) {
+		info->state.ret = -3;
+		goto out;
+	}
+
+	if (!phb->eeh_ops || !phb->eeh_ops->get_state) {
+		pr_debug("%s: Unsupported request\n",
+			__func__);
+		ret = -ENOENT;
+		info->state.ret = -3;
+		goto out;
+	}
+
+	result = phb->eeh_ops->get_state(pe);
+
+	if (!(result & EEH_STATE_RESET_ACTIVE) &&
+	     (result & EEH_STATE_DMA_ENABLED) &&
+	     (result & EEH_STATE_MMIO_ENABLED))
+		info->state.reset_state = 0;
+	else if (result & EEH_STATE_RESET_ACTIVE)
+		info->state.reset_state = 1;
+	else if (!(result & EEH_STATE_RESET_ACTIVE) &&
+		 !(result & EEH_STATE_DMA_ENABLED) &&
+		 !(result & EEH_STATE_MMIO_ENABLED))
+		info->state.reset_state = 2;
+	else if (!(result & EEH_STATE_RESET_ACTIVE) &&
+		 (result & EEH_STATE_DMA_ENABLED) &&
+		 !(result & EEH_STATE_MMIO_ENABLED))
+		info->state.reset_state = 4;
+	else
+		info->state.reset_state = 5;
+
+	info->state.ret = 0;
+	info->state.cfg_cap = 1;
+	info->state.pe_unavail_info = 1000;
+	info->state.pe_recovery_info = 0;
+
+out:
+	return ret;
+}
+
+static int powernv_eeh_vfio_pe_reset(struct pci_dev *pdev,
+				     struct vfio_eeh_op *info)
+{
+	struct eeh_dev *edev;
+	struct eeh_pe *pe;
+	struct pnv_phb *phb;
+	int opcode = info->reset.option;
+	int ret = 0;
+
+	/* Device existing ? */
+	ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+	if (ret) {
+		info->addr.ret = -3;
+		goto out;
+	}
+
+	/* Invalid opcode ? */
+	if (opcode != EEH_RESET_DEACTIVATE &&
+	    opcode != EEH_RESET_HOT &&
+	    opcode != EEH_RESET_FUNDAMENTAL) {
+		pr_debug("%s: Unsupported opcode %d\n",
+			__func__, opcode);
+		ret = -EINVAL;
+		info->reset.ret = -3;
+		goto out;
+	}
+
+	/* Call into the IODA dependent backend to do the reset */
+	if (!phb->eeh_ops ||
+	    !phb->eeh_ops->set_option ||
+	    !phb->eeh_ops->reset) {
+		pr_debug("%s: Unsupported request\n",
+			__func__);
+		ret = -ENOENT;
+		info->reset.ret = -7;
+		goto out;
+	}
+
+	/*
+	 * The frozen PE might be caused by the mechanism called
+	 * PAPR error injection, which is supposed to be one-shot
+	 * without "sticky" bit as being stated by the spec. But
+	 * the reality isn't that, at least on P7IOC. So we have
+	 * to clear that to avoid recrusive error, which fails the
+	 * recovery eventually.
+	 */
+	if (opcode == EEH_RESET_DEACTIVATE)
+		opal_pci_reset(phb->opal_id,
+			       OPAL_PHB_ERROR,
+			       OPAL_ASSERT_RESET);
+
+	ret = phb->eeh_ops->reset(pe, opcode);
+	if (ret) {
+		pr_debug("%s: Failure %d from backend\n",
+			__func__, ret);
+		info->reset.ret = -1;
+		goto out;
+	}
+
+	/*
+	 * The PE is still in frozen state and we need clear that.
+	 * It's good to clear frozen state after deassert to avoid
+	 * messy IO access during reset, which might cause recrusive
+	 * frozen PE.
+	 */
+	if (opcode == EEH_RESET_DEACTIVATE) {
+		ret = phb->eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
+		if (ret) {
+			pr_debug("%s: Cannot enable DMA for PHB#%d-PE#%d (%d)\n",
+				__func__, pe->phb->global_number, pe->addr, ret);
+			info->reset.ret = -1;
+			goto out;
+		}
+
+		ret = phb->eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
+		if (ret) {
+			pr_debug("%s: Cannot enable IO for PHB#%d-PE#%d (%d)\n",
+				__func__, pe->phb->global_number, pe->addr, ret);
+			info->reset.ret = -1;
+			goto out;
+		}
+
+		eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
+	}
+
+	info->reset.ret = 0;
+out:
+	return ret;
+}
+
+static int powernv_eeh_vfio_pe_config(struct pci_dev *pdev,
+				      struct vfio_eeh_op *info)
+{
+	struct eeh_dev *edev;
+	struct eeh_pe *pe;
+	struct pnv_phb *phb;
+	int ret = 0;
+
+	/* Device existing ? */
+	ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+	if (ret) {
+		info->config.ret = -3;
+		goto out;
+	}
+
+	/*
+	 * The access to PCI config space on VFIO device has some
+	 * limitations. Part of PCI config space, including BAR
+	 * registers are not readable and writable. So the guest
+	 * should have stale values for those registers and we have
+	 * to restore them in host side.
+	 */
+	eeh_pe_restore_bars(pe);
+	info->config.ret = 0;
+
+out:
+	return ret;
+}
+
+int eeh_vfio_pci_open(struct pci_dev *pdev)
+{
+	struct eeh_dev *edev;
+
+	/* No PCI device ? */
+	if (!pdev)
+		return -ENODEV;
+
+	/* No EEH device ? */
+	edev = pci_dev_to_eeh_dev(pdev);
+	if (!edev || !edev->pe)
+		return -ENODEV;
+
+	eeh_dev_set_passed(edev, true);
+	eeh_pe_set_passed(edev->pe, true);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(eeh_vfio_pci_open);
+
+void eeh_vfio_pci_release(struct pci_dev *pdev)
+{
+	bool release_pe = true;
+	struct eeh_pe *pe = NULL;
+	struct eeh_dev *tmp, *edev;
+
+	/* No PCI device ? */
+	if (!pdev)
+		return;
+
+	/* No EEH device ? */
+	edev = pci_dev_to_eeh_dev(pdev);
+	if (!edev || !eeh_dev_passed(edev) ||
+	    !edev->pe || !eeh_pe_passed(pe))
+		return;
+
+	/* Release device */
+	pe = edev->pe;
+	eeh_dev_set_passed(edev, false);
+
+	/* Release PE */
+	eeh_pe_for_each_dev(pe, edev, tmp) {
+		if (eeh_dev_passed(edev)) {
+			release_pe = false;
+			break;
+		}
+	}
+
+	if (release_pe)
+		eeh_pe_set_passed(pe, false);
+}
+EXPORT_SYMBOL(eeh_vfio_pci_release);
+
+int eeh_vfio_pci_ioctl(struct pci_dev *pdev,
+		       unsigned long arg)
+{
+	struct vfio_eeh_op info;
+	unsigned long minsz = sizeof(info);
+	int ret = -EINVAL;
+
+	/* Copy over user argument */
+	if (copy_from_user(&info, (void __user *)arg, minsz)) {
+		pr_debug("%s: Cannot copy parameter 0x%lx\n",
+			__func__, arg);
+		return -EFAULT;
+	}
+
+	/* Sanity check */
+	if (info.argsz < minsz) {
+		pr_debug("%s: Invalid size (%d, %ld)\n",
+			__func__, info.argsz, minsz);
+		return -EINVAL;
+	}
+
+	/* Route according to operation */
+	switch (info.op) {
+	case VFIO_EEH_OP_SET_OPTION:
+		ret = powernv_eeh_vfio_set_option(pdev, &info);
+		break;
+	case VFIO_EEH_OP_GET_ADDR:
+		ret = powernv_eeh_vfio_get_addr(pdev, &info);
+		break;
+	case VFIO_EEH_OP_GET_STATE:
+		ret = powernv_eeh_vfio_get_state(pdev, &info);
+		break;
+	case VFIO_EEH_OP_PE_RESET:
+		ret = powernv_eeh_vfio_pe_reset(pdev, &info);
+		break;
+	case VFIO_EEH_OP_PE_CONFIG:
+		ret = powernv_eeh_vfio_pe_config(pdev, &info);
+		break;
+	default:
+		pr_debug("%s: Cannot handle op#%d\n",
+			__func__, info.op);
+	}
+
+	/* Copy data back */
+	if (copy_to_user((void __user *)arg, &info, minsz)) {
+		pr_debug("%s: Cannot copy parameter to user 0x%lx\n",
+			__func__, arg);
+		return -EFAULT;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(eeh_vfio_pci_ioctl);
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 7ba0424..ee82c7f 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -156,8 +156,11 @@ static void vfio_pci_release(void *device_data)
 {
 	struct vfio_pci_device *vdev = device_data;
 
-	if (atomic_dec_and_test(&vdev->refcnt))
+
+	if (atomic_dec_and_test(&vdev->refcnt)) {
+		eeh_vfio_pci_release(vdev->pdev);
 		vfio_pci_disable(vdev);
+	}
 
 	module_put(THIS_MODULE);
 }
@@ -165,19 +168,26 @@ static void vfio_pci_release(void *device_data)
 static int vfio_pci_open(void *device_data)
 {
 	struct vfio_pci_device *vdev = device_data;
+	int ret;
 
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
 
 	if (atomic_inc_return(&vdev->refcnt) == 1) {
-		int ret = vfio_pci_enable(vdev);
-		if (ret) {
-			module_put(THIS_MODULE);
-			return ret;
-		}
+		ret = vfio_pci_enable(vdev);
+		if (ret)
+			goto error;
+
+		ret = eeh_vfio_pci_open(vdev->pdev);
+		if (ret)
+			goto error;
 	}
 
 	return 0;
+
+error:
+	module_put(THIS_MODULE);
+	return ret;
 }
 
 static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
@@ -682,6 +692,8 @@ hot_reset_release:
 
 		kfree(groups);
 		return ret;
+	} else if (cmd == VFIO_EEH_OP) {
+		return eeh_vfio_pci_ioctl(vdev->pdev, arg);
 	}
 
 	return -ENOTTY;
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 9c6d5d0..1273bb6 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -90,4 +90,20 @@ extern void vfio_pci_virqfd_exit(void);
 
 extern int vfio_config_init(struct vfio_pci_device *vdev);
 extern void vfio_config_free(struct vfio_pci_device *vdev);
+
+#ifdef CONFIG_VFIO_PCI_EEH
+extern int eeh_vfio_pci_open(struct pci_dev *pdev);
+extern void eeh_vfio_pci_release(struct pci_dev *pdev);
+extern int eeh_vfio_pci_ioctl(struct pci_dev *pdev, unsigned long arg);
+#else
+static inline int eeh_vfio_pci_open(struct pci_dev *pdev)
+{
+	return 0;
+}
+static inline eeh_vfio_pci_release(struct pci_dev *pdev) { }
+static int eeh_vfio_pci_ioctl(struct pci_dev *pdev, unsigned long arg)
+{
+	return -ENOENT;
+}
+#endif /* COFNIG_VFIO_PCI_EEH */
 #endif /* VFIO_PCI_PRIVATE_H */
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index cb9023d..6e7f033 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -455,6 +455,49 @@ struct vfio_iommu_spapr_tce_info {
 
 #define VFIO_IOMMU_SPAPR_TCE_GET_INFO	_IO(VFIO_TYPE, VFIO_BASE + 12)
 
+/*
+ * The VFIO operation struct provides way to support EEH functionality
+ * for PCI device that is passed from host to guest via VFIO.
+ */
+#define VFIO_EEH_OP_SET_OPTION	0
+#define VFIO_EEH_OP_GET_ADDR	1
+#define VFIO_EEH_OP_GET_STATE	2
+#define VFIO_EEH_OP_PE_RESET	3
+#define VFIO_EEH_OP_PE_CONFIG	4
+
+struct vfio_eeh_op {
+	__u32 argsz;
+	__u32 op;
+
+	union {
+		struct vfio_eeh_set_option {
+			__u32 option;
+			__s32 ret;
+		} option;
+		struct vfio_eeh_pe_addr {
+			__u32 option;
+			__s32 ret;
+			__u32 info;
+		} addr;
+		struct vfio_eeh_pe_state {
+			__s32 ret;
+			__u32 reset_state;
+			__u32 cfg_cap;
+			__u32 pe_unavail_info;
+			__u32 pe_recovery_info;
+                } state;
+		struct vfio_eeh_reset {
+			__u32 option;
+			__s32 ret;
+		} reset;
+		struct vfio_eeh_config {
+			__s32 ret;
+		} config;
+	};
+};
+
+#define VFIO_EEH_OP	_IO(VFIO_TYPE, VFIO_BASE + 21)
+
 /* ***************************************************************** */
 
 #endif /* _UAPIVFIO_H */
-- 
1.8.3.2

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Alexander Graf]

On 20.05.14 10:30, Gavin Shan wrote:
> The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
> to support EEH functionality for PCI devices, which have been
> passed from host to guest via VFIO.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>   arch/powerpc/platforms/powernv/Makefile   |   1 +
>   arch/powerpc/platforms/powernv/eeh-vfio.c | 445 ++++++++++++++++++++++++++++++
>   drivers/vfio/pci/vfio_pci.c               |  24 +-
>   drivers/vfio/pci/vfio_pci_private.h       |  16 ++
>   include/uapi/linux/vfio.h                 |  43 +++
>   5 files changed, 523 insertions(+), 6 deletions(-)
>   create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c

Why doesn't this code live inside the vfio module? If I don't load the 
vfio module, I don't need that code to waste memory in my kernel, no?


Alex

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Alexander Graf]

On 20.05.14 13:21, Alexander Graf wrote:
>
> On 20.05.14 10:30, Gavin Shan wrote:
>> The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
>> to support EEH functionality for PCI devices, which have been
>> passed from host to guest via VFIO.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/platforms/powernv/Makefile   |   1 +
>>   arch/powerpc/platforms/powernv/eeh-vfio.c | 445 
>> ++++++++++++++++++++++++++++++
>>   drivers/vfio/pci/vfio_pci.c               |  24 +-
>>   drivers/vfio/pci/vfio_pci_private.h       |  16 ++
>>   include/uapi/linux/vfio.h                 |  43 +++
>>   5 files changed, 523 insertions(+), 6 deletions(-)
>>   create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>
> Why doesn't this code live inside the vfio module? If I don't load the 
> vfio module, I don't need that code to waste memory in my kernel, no?

So I think from a modeling point of view, you want VFIO code that calls 
reasonably generic helpers inside the kernel to deal with errors.

The "generic helpers" don't have anything to do with VFIO. Everything 
that interfaces via ioctls with user space is 100% VFIO code.

The latter should be tristate inside vfio.ko, the former can be =y.


Alex

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Gavin Shan]

On Tue, May 20, 2014 at 01:28:40PM +0200, Alexander Graf wrote:
>
>On 20.05.14 13:21, Alexander Graf wrote:
>>
>>On 20.05.14 10:30, Gavin Shan wrote:
>>>The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
>>>to support EEH functionality for PCI devices, which have been
>>>passed from host to guest via VFIO.
>>>
>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>---
>>>  arch/powerpc/platforms/powernv/Makefile   |   1 +
>>>  arch/powerpc/platforms/powernv/eeh-vfio.c | 445
>>>++++++++++++++++++++++++++++++
>>>  drivers/vfio/pci/vfio_pci.c               |  24 +-
>>>  drivers/vfio/pci/vfio_pci_private.h       |  16 ++
>>>  include/uapi/linux/vfio.h                 |  43 +++
>>>  5 files changed, 523 insertions(+), 6 deletions(-)
>>>  create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>>
>>Why doesn't this code live inside the vfio module? If I don't load
>>the vfio module, I don't need that code to waste memory in my
>>kernel, no?

Yes, It saves some memory.

>
>So I think from a modeling point of view, you want VFIO code that
>calls reasonably generic helpers inside the kernel to deal with
>errors.
>
>The "generic helpers" don't have anything to do with VFIO. Everything
>that interfaces via ioctls with user space is 100% VFIO code.
>
>The latter should be tristate inside vfio.ko, the former can be =y.
>

The main reason I put eeh-vfio.c to arch/powerpc/platforms/powernv/ is
the source file needs access data structures (struct pnv_phb) defined
in "pci.h" under that directory.

Thanks,
Gavin

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Alexander Graf]

On 20.05.14 13:40, Gavin Shan wrote:
> On Tue, May 20, 2014 at 01:28:40PM +0200, Alexander Graf wrote:
>> On 20.05.14 13:21, Alexander Graf wrote:
>>> On 20.05.14 10:30, Gavin Shan wrote:
>>>> The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
>>>> to support EEH functionality for PCI devices, which have been
>>>> passed from host to guest via VFIO.
>>>>
>>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>> ---
>>>>   arch/powerpc/platforms/powernv/Makefile   |   1 +
>>>>   arch/powerpc/platforms/powernv/eeh-vfio.c | 445
>>>> ++++++++++++++++++++++++++++++
>>>>   drivers/vfio/pci/vfio_pci.c               |  24 +-
>>>>   drivers/vfio/pci/vfio_pci_private.h       |  16 ++
>>>>   include/uapi/linux/vfio.h                 |  43 +++
>>>>   5 files changed, 523 insertions(+), 6 deletions(-)
>>>>   create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>>> Why doesn't this code live inside the vfio module? If I don't load
>>> the vfio module, I don't need that code to waste memory in my
>>> kernel, no?
> Yes, It saves some memory.
>
>> So I think from a modeling point of view, you want VFIO code that
>> calls reasonably generic helpers inside the kernel to deal with
>> errors.
>>
>> The "generic helpers" don't have anything to do with VFIO. Everything
>> that interfaces via ioctls with user space is 100% VFIO code.
>>
>> The latter should be tristate inside vfio.ko, the former can be =y.
>>
> The main reason I put eeh-vfio.c to arch/powerpc/platforms/powernv/ is
> the source file needs access data structures (struct pnv_phb) defined
> in "pci.h" under that directory.

Then create a good in-kernel framework from that directory and make use 
of it from the VFIO code :). But please don't mesh together VFIO, 
powernv EEH handling and RTAS.


Alex

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Gavin Shan]

On Tue, May 20, 2014 at 01:44:21PM +0200, Alexander Graf wrote:
>
>On 20.05.14 13:40, Gavin Shan wrote:
>>On Tue, May 20, 2014 at 01:28:40PM +0200, Alexander Graf wrote:
>>>On 20.05.14 13:21, Alexander Graf wrote:
>>>>On 20.05.14 10:30, Gavin Shan wrote:
>>>>>The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
>>>>>to support EEH functionality for PCI devices, which have been
>>>>>passed from host to guest via VFIO.
>>>>>
>>>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>>---
>>>>>  arch/powerpc/platforms/powernv/Makefile   |   1 +
>>>>>  arch/powerpc/platforms/powernv/eeh-vfio.c | 445
>>>>>++++++++++++++++++++++++++++++
>>>>>  drivers/vfio/pci/vfio_pci.c               |  24 +-
>>>>>  drivers/vfio/pci/vfio_pci_private.h       |  16 ++
>>>>>  include/uapi/linux/vfio.h                 |  43 +++
>>>>>  5 files changed, 523 insertions(+), 6 deletions(-)
>>>>>  create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>>>>Why doesn't this code live inside the vfio module? If I don't load
>>>>the vfio module, I don't need that code to waste memory in my
>>>>kernel, no?
>>Yes, It saves some memory.
>>
>>>So I think from a modeling point of view, you want VFIO code that
>>>calls reasonably generic helpers inside the kernel to deal with
>>>errors.
>>>
>>>The "generic helpers" don't have anything to do with VFIO. Everything
>>>that interfaces via ioctls with user space is 100% VFIO code.
>>>
>>>The latter should be tristate inside vfio.ko, the former can be =y.
>>>
>>The main reason I put eeh-vfio.c to arch/powerpc/platforms/powernv/ is
>>the source file needs access data structures (struct pnv_phb) defined
>>in "pci.h" under that directory.
>
>Then create a good in-kernel framework from that directory and make
>use of it from the VFIO code :). But please don't mesh together VFIO,
>powernv EEH handling and RTAS.
>

Yeah. How about this? :-)

- Move eeh-vfio.c to drivers/vfio/pci/
- From eeh-vfio.c, dereference arch/powerpc/kernel/eeh.c::eeh_ops, which
  is arch/powerpc/plaforms/powernv/eeh-powernv.c::powernv_eeh_ops. Call
  to the corresponding callbacks in "eeh_ops" based on incoming RTAS request.

  The file would be renamed to "vfio_eeh.c" as well after moving to VFIO
  driver directory.

Thanks,
Gavin

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Alexander Graf]

On 20.05.14 14:21, Gavin Shan wrote:
> On Tue, May 20, 2014 at 01:44:21PM +0200, Alexander Graf wrote:
>> On 20.05.14 13:40, Gavin Shan wrote:
>>> On Tue, May 20, 2014 at 01:28:40PM +0200, Alexander Graf wrote:
>>>> On 20.05.14 13:21, Alexander Graf wrote:
>>>>> On 20.05.14 10:30, Gavin Shan wrote:
>>>>>> The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
>>>>>> to support EEH functionality for PCI devices, which have been
>>>>>> passed from host to guest via VFIO.
>>>>>>
>>>>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>   arch/powerpc/platforms/powernv/Makefile   |   1 +
>>>>>>   arch/powerpc/platforms/powernv/eeh-vfio.c | 445
>>>>>> ++++++++++++++++++++++++++++++
>>>>>>   drivers/vfio/pci/vfio_pci.c               |  24 +-
>>>>>>   drivers/vfio/pci/vfio_pci_private.h       |  16 ++
>>>>>>   include/uapi/linux/vfio.h                 |  43 +++
>>>>>>   5 files changed, 523 insertions(+), 6 deletions(-)
>>>>>>   create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>>>>> Why doesn't this code live inside the vfio module? If I don't load
>>>>> the vfio module, I don't need that code to waste memory in my
>>>>> kernel, no?
>>> Yes, It saves some memory.
>>>
>>>> So I think from a modeling point of view, you want VFIO code that
>>>> calls reasonably generic helpers inside the kernel to deal with
>>>> errors.
>>>>
>>>> The "generic helpers" don't have anything to do with VFIO. Everything
>>>> that interfaces via ioctls with user space is 100% VFIO code.
>>>>
>>>> The latter should be tristate inside vfio.ko, the former can be =y.
>>>>
>>> The main reason I put eeh-vfio.c to arch/powerpc/platforms/powernv/ is
>>> the source file needs access data structures (struct pnv_phb) defined
>>> in "pci.h" under that directory.
>> Then create a good in-kernel framework from that directory and make
>> use of it from the VFIO code :). But please don't mesh together VFIO,
>> powernv EEH handling and RTAS.
>>
> Yeah. How about this? :-)
>
> - Move eeh-vfio.c to drivers/vfio/pci/
> - From eeh-vfio.c, dereference arch/powerpc/kernel/eeh.c::eeh_ops, which
>    is arch/powerpc/plaforms/powernv/eeh-powernv.c::powernv_eeh_ops. Call

Hrm, I think it'd be nicer to just export individual functions that do 
thing you want to do from eeh.c.


Alex

>    to the corresponding callbacks in "eeh_ops" based on incoming RTAS request.
>
>    The file would be renamed to "vfio_eeh.c" as well after moving to VFIO
>    driver directory.
>
> Thanks,
> Gavin
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Gavin Shan]

On Tue, May 20, 2014 at 02:25:45PM +0200, Alexander Graf wrote:
>
>On 20.05.14 14:21, Gavin Shan wrote:
>>On Tue, May 20, 2014 at 01:44:21PM +0200, Alexander Graf wrote:
>>>On 20.05.14 13:40, Gavin Shan wrote:
>>>>On Tue, May 20, 2014 at 01:28:40PM +0200, Alexander Graf wrote:
>>>>>On 20.05.14 13:21, Alexander Graf wrote:
>>>>>>On 20.05.14 10:30, Gavin Shan wrote:
>>>>>>>The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
>>>>>>>to support EEH functionality for PCI devices, which have been
>>>>>>>passed from host to guest via VFIO.
>>>>>>>
>>>>>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>>>>---
>>>>>>>  arch/powerpc/platforms/powernv/Makefile   |   1 +
>>>>>>>  arch/powerpc/platforms/powernv/eeh-vfio.c | 445
>>>>>>>++++++++++++++++++++++++++++++
>>>>>>>  drivers/vfio/pci/vfio_pci.c               |  24 +-
>>>>>>>  drivers/vfio/pci/vfio_pci_private.h       |  16 ++
>>>>>>>  include/uapi/linux/vfio.h                 |  43 +++
>>>>>>>  5 files changed, 523 insertions(+), 6 deletions(-)
>>>>>>>  create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>>>>>>Why doesn't this code live inside the vfio module? If I don't load
>>>>>>the vfio module, I don't need that code to waste memory in my
>>>>>>kernel, no?
>>>>Yes, It saves some memory.
>>>>
>>>>>So I think from a modeling point of view, you want VFIO code that
>>>>>calls reasonably generic helpers inside the kernel to deal with
>>>>>errors.
>>>>>
>>>>>The "generic helpers" don't have anything to do with VFIO. Everything
>>>>>that interfaces via ioctls with user space is 100% VFIO code.
>>>>>
>>>>>The latter should be tristate inside vfio.ko, the former can be =y.
>>>>>
>>>>The main reason I put eeh-vfio.c to arch/powerpc/platforms/powernv/ is
>>>>the source file needs access data structures (struct pnv_phb) defined
>>>>in "pci.h" under that directory.
>>>Then create a good in-kernel framework from that directory and make
>>>use of it from the VFIO code :). But please don't mesh together VFIO,
>>>powernv EEH handling and RTAS.
>>>
>>Yeah. How about this? :-)
>>
>>- Move eeh-vfio.c to drivers/vfio/pci/
>>- From eeh-vfio.c, dereference arch/powerpc/kernel/eeh.c::eeh_ops, which
>>   is arch/powerpc/plaforms/powernv/eeh-powernv.c::powernv_eeh_ops. Call
>
>Hrm, I think it'd be nicer to just export individual functions that
>do thing you want to do from eeh.c.
>

Ok. Got it. Thanks for your comments :)

Thanks,
Gavin

>
>Alex
>
>>   to the corresponding callbacks in "eeh_ops" based on incoming RTAS request.
>>
>>   The file would be renamed to "vfio_eeh.c" as well after moving to VFIO
>>   driver directory.
>>
>>Thanks,
>>Gavin
>>
>>--
>>To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>--
>To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Benjamin Herrenschmidt]

On Tue, 2014-05-20 at 22:39 +1000, Gavin Shan wrote:
> >>Yeah. How about this? :-)
> >>
> >>- Move eeh-vfio.c to drivers/vfio/pci/
> >>- From eeh-vfio.c, dereference arch/powerpc/kernel/eeh.c::eeh_ops, which
> >>   is arch/powerpc/plaforms/powernv/eeh-powernv.c::powernv_eeh_ops. Call
> >
> >Hrm, I think it'd be nicer to just export individual functions that
> >do thing you want to do from eeh.c.
> >
> 
> Ok. Got it. Thanks for your comments :)

The interesting thing with this approach is that VFIO per-se can work
with EEH RTAS backend too in the host.

IE, with PR KVM for example or with non-KVM uses of VFIO, it would be
possible to use a device in a user process and exploit EEH even when
running under a PAPR hypervisor.

That is, vfio-eeh uses "generic" exported EEH APIs from the EEH core
that will work on both powernv and RTAS backends.

Note to Alex: This definitely kills the notifier idea for now though,
at least as a first class citizen of the design. We can add it as an
optional optimization on top later.

Cheers,
Ben.

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Gavin Shan]

On Wed, May 21, 2014 at 10:23:52AM +1000, Benjamin Herrenschmidt wrote:
>On Tue, 2014-05-20 at 22:39 +1000, Gavin Shan wrote:
>> >>Yeah. How about this? :-)
>> >>
>> >>- Move eeh-vfio.c to drivers/vfio/pci/
>> >>- From eeh-vfio.c, dereference arch/powerpc/kernel/eeh.c::eeh_ops, which
>> >>   is arch/powerpc/plaforms/powernv/eeh-powernv.c::powernv_eeh_ops. Call
>> >
>> >Hrm, I think it'd be nicer to just export individual functions that
>> >do thing you want to do from eeh.c.
>> >
>> 
>> Ok. Got it. Thanks for your comments :)
>
>The interesting thing with this approach is that VFIO per-se can work
>with EEH RTAS backend too in the host.
>

Yeah, it's another benefit for the approach. I need think about it
later. Thanks, Ben. It's really good point.

>IE, with PR KVM for example or with non-KVM uses of VFIO, it would be
>possible to use a device in a user process and exploit EEH even when
>running under a PAPR hypervisor.
>
>That is, vfio-eeh uses "generic" exported EEH APIs from the EEH core
>that will work on both powernv and RTAS backends.
>
>Note to Alex: This definitely kills the notifier idea for now though,
>at least as a first class citizen of the design. We can add it as an
>optional optimization on top later.
>

Yeah, I'll address the notifier later.

Thanks,
Gavin

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Alexander Graf]

> Am 21.05.2014 um 02:23 schrieb Benjamin Herrenschmidt <benh@kernel.crashin=
g.org>:
>=20
> On Tue, 2014-05-20 at 22:39 +1000, Gavin Shan wrote:
>>>> Yeah. How about this? :-)
>>>>=20
>>>> - Move eeh-vfio.c to drivers/vfio/pci/
>>>> - =46rom eeh-vfio.c, dereference arch/powerpc/kernel/eeh.c::eeh_ops, wh=
ich
>>>>  is arch/powerpc/plaforms/powernv/eeh-powernv.c::powernv_eeh_ops. Call
>>>=20
>>> Hrm, I think it'd be nicer to just export individual functions that
>>> do thing you want to do from eeh.c.
>>=20
>> Ok. Got it. Thanks for your comments :)
>=20
> The interesting thing with this approach is that VFIO per-se can work
> with EEH RTAS backend too in the host.
>=20
> IE, with PR KVM for example or with non-KVM uses of VFIO, it would be
> possible to use a device in a user process and exploit EEH even when
> running under a PAPR hypervisor.
>=20
> That is, vfio-eeh uses "generic" exported EEH APIs from the EEH core
> that will work on both powernv and RTAS backends.
>=20
> Note to Alex: This definitely kills the notifier idea for now though,
> at least as a first class citizen of the design. We can add it as an
> optional optimization on top later.

I don't think it does. The notifier would just get triggered on config space=
 read failures for example :). It's really just an aid for the vfio user to h=
ave a common code path for error handling.


Alex

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Benjamin Herrenschmidt]

On Wed, 2014-05-21 at 08:23 +0200, Alexander Graf wrote:
> > Note to Alex: This definitely kills the notifier idea for now
> though,
> > at least as a first class citizen of the design. We can add it as an
> > optional optimization on top later.
> 
> I don't think it does. The notifier would just get triggered on config
> space read failures for example :). It's really just an aid for the
> vfio user to have a common code path for error handling.

I'll let Gavin make the final call on that one, if he thinks we can
reliably trigger it and there isn't too much code churn as a
consequence.

Cheers,
Ben.

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Gavin Shan]

On Wed, May 21, 2014 at 05:24:22PM +1000, Benjamin Herrenschmidt wrote:
>On Wed, 2014-05-21 at 08:23 +0200, Alexander Graf wrote:
>> > Note to Alex: This definitely kills the notifier idea for now
>> though,
>> > at least as a first class citizen of the design. We can add it as an
>> > optional optimization on top later.
>> 
>> I don't think it does. The notifier would just get triggered on config
>> space read failures for example :). It's really just an aid for the
>> vfio user to have a common code path for error handling.
>
>I'll let Gavin make the final call on that one, if he thinks we can
>reliably trigger it and there isn't too much code churn as a
>consequence.
>

Lets postpone it as future improvement. It's not hard to send the
event (EEH errors) out, but I need think about how to extend the
existing guest's infrastructure to accept event. As Ben mentioned,
event-scan might be potential mechanism for that. We can discuss
for more later :-)

Thanks,
Gavin

Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO

[Benjamin Herrenschmidt]

On Tue, 2014-05-20 at 14:25 +0200, Alexander Graf wrote:
> > - Move eeh-vfio.c to drivers/vfio/pci/
> > - From eeh-vfio.c, dereference arch/powerpc/kernel/eeh.c::eeh_ops,
> which
> >    is arch/powerpc/plaforms/powernv/eeh-powernv.c::powernv_eeh_ops.
> Call
> 
> Hrm, I think it'd be nicer to just export individual functions that do
> thing you want to do from eeh.c.

We already have an eeh_ops backend system with callbacks since we have
different backends for RTAS and powernv, so we could do what you suggest
but it would probably just boil down to wrappers around the EEH ops.

No big opinion either way on my side though.

Cheers,
Ben.

[PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Gavin Shan]

If we detects frozen state on PE that has been passed to guest, we
needn't handle it. Instead, we rely on the guest to detect and recover
it. The patch avoid EEH event on the frozen passed PE so that the guest
can have chance to handle that.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/eeh.c                 | 8 ++++++++
 arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 9c6b899..6543f05 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -400,6 +400,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
 	if (ret > 0)
 		return ret;
 
+	/*
+	 * If the PE has been passed to guest, we won't check the
+	 * state. Instead, let the guest handle it if the PE has
+	 * been frozen.
+	 */
+	if (eeh_pe_passed(pe))
+		return 0;
+
 	/* If we already have a pending isolation event for this
 	 * slot, we know it's bad already, we don't need to check.
 	 * Do this checking under a lock; as multiple PCI devices
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 1b5982f..03a3ed2 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
 				opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
 					OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
 				ret = EEH_NEXT_ERR_NONE;
-			} else if ((*pe)->state & EEH_PE_ISOLATED) {
+			} else if ((*pe)->state & EEH_PE_ISOLATED ||
+				   eeh_pe_passed(*pe)) {
 				ret = EEH_NEXT_ERR_NONE;
 			} else {
 				pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",
-- 
1.8.3.2

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Alexander Graf]

On 20.05.14 10:30, Gavin Shan wrote:
> If we detects frozen state on PE that has been passed to guest, we
> needn't handle it. Instead, we rely on the guest to detect and recover
> it. The patch avoid EEH event on the frozen passed PE so that the guest
> can have chance to handle that.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

How does the guest learn about this failure? We'd need to inject an 
error into it, no?

I think what you want is an irqfd that the in-kernel eeh code notifies 
when it sees a failure. When such an fd exists, the kernel skips its own 
error handling.


Alex

> ---
>   arch/powerpc/kernel/eeh.c                 | 8 ++++++++
>   arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
>   2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 9c6b899..6543f05 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -400,6 +400,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
>   	if (ret > 0)
>   		return ret;
>   
> +	/*
> +	 * If the PE has been passed to guest, we won't check the
> +	 * state. Instead, let the guest handle it if the PE has
> +	 * been frozen.
> +	 */
> +	if (eeh_pe_passed(pe))
> +		return 0;
> +
>   	/* If we already have a pending isolation event for this
>   	 * slot, we know it's bad already, we don't need to check.
>   	 * Do this checking under a lock; as multiple PCI devices
> diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
> index 1b5982f..03a3ed2 100644
> --- a/arch/powerpc/platforms/powernv/eeh-ioda.c
> +++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
> @@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
>   				opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
>   					OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
>   				ret = EEH_NEXT_ERR_NONE;
> -			} else if ((*pe)->state & EEH_PE_ISOLATED) {
> +			} else if ((*pe)->state & EEH_PE_ISOLATED ||
> +				   eeh_pe_passed(*pe)) {
>   				ret = EEH_NEXT_ERR_NONE;
>   			} else {
>   				pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Gavin Shan]

On Tue, May 20, 2014 at 01:25:11PM +0200, Alexander Graf wrote:
>
>On 20.05.14 10:30, Gavin Shan wrote:
>>If we detects frozen state on PE that has been passed to guest, we
>>needn't handle it. Instead, we rely on the guest to detect and recover
>>it. The patch avoid EEH event on the frozen passed PE so that the guest
>>can have chance to handle that.
>>
>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>How does the guest learn about this failure? We'd need to inject an
>error into it, no?
>

When error is existing in HW level, 0xFF's will be turned on reading
PCI config space or memory BARs. Guest retrieves the failure state,
which is captured by HW automatically, via RTAS call
"ibm,read-slot-reset-state2" when seeing 0xFF's on reading PCI config
space or memory BARs. If "ibm,read-slot-reset-state2" reports errors in HW,
the guest kernel starts to recovery.

It can be called as "passive" reporting. There possible has one case that
the error can't be reported for ever: No device driver binding to the VFIO
PCI device and no access to device's config space and memory BARs. However,
it doesn't matter. As we don't use the device, we needn't detect and recover
the error at all.

>I think what you want is an irqfd that the in-kernel eeh code
>notifies when it sees a failure. When such an fd exists, the kernel
>skips its own error handling.
>

Yeah, it's a good idea and something for me to improve in phase II. We
can discuss for more later. For now, what I have in my head is something
like this:

      [ Host ] -> Error detected -> irqfd (or eventfd) -> QEMU 
                                                           |
                                   -------------(A)---------
                                   |
                        Send one EEH event to guest kernel
                                   |
                        Guest kernel starts the recovery

(A): I didn't figure out one convienent way to do the EEH event injection yet.

Thanks,
Gavin

>>---
>>  arch/powerpc/kernel/eeh.c                 | 8 ++++++++
>>  arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>index 9c6b899..6543f05 100644
>>--- a/arch/powerpc/kernel/eeh.c
>>+++ b/arch/powerpc/kernel/eeh.c
>>@@ -400,6 +400,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
>>  	if (ret > 0)
>>  		return ret;
>>+	/*
>>+	 * If the PE has been passed to guest, we won't check the
>>+	 * state. Instead, let the guest handle it if the PE has
>>+	 * been frozen.
>>+	 */
>>+	if (eeh_pe_passed(pe))
>>+		return 0;
>>+
>>  	/* If we already have a pending isolation event for this
>>  	 * slot, we know it's bad already, we don't need to check.
>>  	 * Do this checking under a lock; as multiple PCI devices
>>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>index 1b5982f..03a3ed2 100644
>>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>@@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
>>  				opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
>>  					OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
>>  				ret = EEH_NEXT_ERR_NONE;
>>-			} else if ((*pe)->state & EEH_PE_ISOLATED) {
>>+			} else if ((*pe)->state & EEH_PE_ISOLATED ||
>>+				   eeh_pe_passed(*pe)) {
>>  				ret = EEH_NEXT_ERR_NONE;
>>  			} else {
>>  				pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",
>

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Alexander Graf]

On 20.05.14 13:56, Gavin Shan wrote:
> On Tue, May 20, 2014 at 01:25:11PM +0200, Alexander Graf wrote:
>> On 20.05.14 10:30, Gavin Shan wrote:
>>> If we detects frozen state on PE that has been passed to guest, we
>>> needn't handle it. Instead, we rely on the guest to detect and recover
>>> it. The patch avoid EEH event on the frozen passed PE so that the guest
>>> can have chance to handle that.
>>>
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> How does the guest learn about this failure? We'd need to inject an
>> error into it, no?
>>
> When error is existing in HW level, 0xFF's will be turned on reading
> PCI config space or memory BARs. Guest retrieves the failure state,
> which is captured by HW automatically, via RTAS call
> "ibm,read-slot-reset-state2" when seeing 0xFF's on reading PCI config
> space or memory BARs. If "ibm,read-slot-reset-state2" reports errors in HW,
> the guest kernel starts to recovery.
>
> It can be called as "passive" reporting. There possible has one case that
> the error can't be reported for ever: No device driver binding to the VFIO
> PCI device and no access to device's config space and memory BARs. However,
> it doesn't matter. As we don't use the device, we needn't detect and recover
> the error at all.

So if the guest is waiting for an interrupt to happen it will wait 
forever? Not really nice.

>> I think what you want is an irqfd that the in-kernel eeh code
>> notifies when it sees a failure. When such an fd exists, the kernel
>> skips its own error handling.
>>
> Yeah, it's a good idea and something for me to improve in phase II. We
> can discuss for more later.

I think it makes sense to at least walk into that direction immediately. 
The reason I brought it up in the context of this patch is that with an 
irqfd you wouldn't need the passed flag at all.

>   For now, what I have in my head is something
> like this:
>
>        [ Host ] -> Error detected -> irqfd (or eventfd) -> QEMU
>                                                             |
>                                     -------------(A)---------
>                                     |
>                          Send one EEH event to guest kernel
>                                     |
>                          Guest kernel starts the recovery
>
> (A): I didn't figure out one convienent way to do the EEH event injection yet.

How does the guest learn about errors in pHyp?


Alex

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Gavin Shan]

On Tue, May 20, 2014 at 02:14:56PM +0200, Alexander Graf wrote:
>
>On 20.05.14 13:56, Gavin Shan wrote:
>>On Tue, May 20, 2014 at 01:25:11PM +0200, Alexander Graf wrote:
>>>On 20.05.14 10:30, Gavin Shan wrote:
>>>>If we detects frozen state on PE that has been passed to guest, we
>>>>needn't handle it. Instead, we rely on the guest to detect and recover
>>>>it. The patch avoid EEH event on the frozen passed PE so that the guest
>>>>can have chance to handle that.
>>>>
>>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>How does the guest learn about this failure? We'd need to inject an
>>>error into it, no?
>>>
>>When error is existing in HW level, 0xFF's will be turned on reading
>>PCI config space or memory BARs. Guest retrieves the failure state,
>>which is captured by HW automatically, via RTAS call
>>"ibm,read-slot-reset-state2" when seeing 0xFF's on reading PCI config
>>space or memory BARs. If "ibm,read-slot-reset-state2" reports errors in HW,
>>the guest kernel starts to recovery.
>>
>>It can be called as "passive" reporting. There possible has one case that
>>the error can't be reported for ever: No device driver binding to the VFIO
>>PCI device and no access to device's config space and memory BARs. However,
>>it doesn't matter. As we don't use the device, we needn't detect and recover
>>the error at all.
>
>So if the guest is waiting for an interrupt to happen it will wait
>forever? Not really nice.
>

Nope, the error reporting in guest isn't interrupt-driven. It's always
"polling" :-)

>>>I think what you want is an irqfd that the in-kernel eeh code
>>>notifies when it sees a failure. When such an fd exists, the kernel
>>>skips its own error handling.
>>>
>>Yeah, it's a good idea and something for me to improve in phase II. We
>>can discuss for more later.
>
>I think it makes sense to at least walk into that direction
>immediately. The reason I brought it up in the context of this patch
>is that with an irqfd you wouldn't need the passed flag at all.
>

I don't see how it can avoid the "passed" flag. Without the flag, any
PCI config and memory BAR access on host side could trigger EEH recovery
for those PCI devices passed to guest. That's unexpected behaviour. 

For host, we have 2 ways to report errors: interrupt driven and polling.
For the guest, we only have "polling" :-)

>>  For now, what I have in my head is something
>>like this:
>>
>>       [ Host ] -> Error detected -> irqfd (or eventfd) -> QEMU
>>                                                            |
>>                                    -------------(A)---------
>>                                    |
>>                         Send one EEH event to guest kernel
>>                                    |
>>                         Guest kernel starts the recovery
>>
>>(A): I didn't figure out one convienent way to do the EEH event injection yet.
>
>How does the guest learn about errors in pHyp?
>

It relies on "polling".

Thanks,
Gavin

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Alexander Graf]

On 20.05.14 14:45, Gavin Shan wrote:
> On Tue, May 20, 2014 at 02:14:56PM +0200, Alexander Graf wrote:
>> On 20.05.14 13:56, Gavin Shan wrote:
>>> On Tue, May 20, 2014 at 01:25:11PM +0200, Alexander Graf wrote:
>>>> On 20.05.14 10:30, Gavin Shan wrote:
>>>>> If we detects frozen state on PE that has been passed to guest, we
>>>>> needn't handle it. Instead, we rely on the guest to detect and recover
>>>>> it. The patch avoid EEH event on the frozen passed PE so that the guest
>>>>> can have chance to handle that.
>>>>>
>>>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>> How does the guest learn about this failure? We'd need to inject an
>>>> error into it, no?
>>>>
>>> When error is existing in HW level, 0xFF's will be turned on reading
>>> PCI config space or memory BARs. Guest retrieves the failure state,
>>> which is captured by HW automatically, via RTAS call
>>> "ibm,read-slot-reset-state2" when seeing 0xFF's on reading PCI config
>>> space or memory BARs. If "ibm,read-slot-reset-state2" reports errors in HW,
>>> the guest kernel starts to recovery.
>>>
>>> It can be called as "passive" reporting. There possible has one case that
>>> the error can't be reported for ever: No device driver binding to the VFIO
>>> PCI device and no access to device's config space and memory BARs. However,
>>> it doesn't matter. As we don't use the device, we needn't detect and recover
>>> the error at all.
>> So if the guest is waiting for an interrupt to happen it will wait
>> forever? Not really nice.
>>
> Nope, the error reporting in guest isn't interrupt-driven. It's always
> "polling" :-)

That sucks :).

>
>>>> I think what you want is an irqfd that the in-kernel eeh code
>>>> notifies when it sees a failure. When such an fd exists, the kernel
>>>> skips its own error handling.
>>>>
>>> Yeah, it's a good idea and something for me to improve in phase II. We
>>> can discuss for more later.
>> I think it makes sense to at least walk into that direction
>> immediately. The reason I brought it up in the context of this patch
>> is that with an irqfd you wouldn't need the passed flag at all.
>>
> I don't see how it can avoid the "passed" flag. Without the flag, any
> PCI config and memory BAR access on host side could trigger EEH recovery
> for those PCI devices passed to guest. That's unexpected behaviour.

Instead of

   if (passed_flag)
     return;

you would do

   if (trigger_irqfd) {
     trigger_irqfd();
     return;
   }

which would be a much nicer, generic interface.

> For host, we have 2 ways to report errors: interrupt driven and polling.
> For the guest, we only have "polling" :-)

And the interrupt path is powernv specific? Does sPAPR specify anything 
here?

>
>>>   For now, what I have in my head is something
>>> like this:
>>>
>>>        [ Host ] -> Error detected -> irqfd (or eventfd) -> QEMU
>>>                                                             |
>>>                                     -------------(A)---------
>>>                                     |
>>>                          Send one EEH event to guest kernel
>>>                                     |
>>>                          Guest kernel starts the recovery
>>>
>>> (A): I didn't figure out one convienent way to do the EEH event injection yet.
>> How does the guest learn about errors in pHyp?
>>
> It relies on "polling".

Sigh ;).

So how about we just implement this whole thing properly as irqfd? 
Whether QEMU can actually do anything with the interrupt is a different 
question - we can leave it be for now. But we could model all the code 
with the assumption that it should either handle the error itself or 
trigger and irqfd write.


Alex

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Benjamin Herrenschmidt]

On Tue, 2014-05-20 at 15:49 +0200, Alexander Graf wrote:
> Instead of
> 
>    if (passed_flag)
>      return;
> 
> you would do
> 
>    if (trigger_irqfd) {
>      trigger_irqfd();
>      return;
>    }
> 
> which would be a much nicer, generic interface.

But that's not how PAPR works.

Cheers,
Ben.

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Alexander Graf]

> Am 21.05.2014 um 02:13 schrieb Benjamin Herrenschmidt <benh@kernel.crashin=
g.org>:
>=20
>> On Tue, 2014-05-20 at 15:49 +0200, Alexander Graf wrote:
>> Instead of
>>=20
>>   if (passed_flag)
>>     return;
>>=20
>> you would do
>>=20
>>   if (trigger_irqfd) {
>>     trigger_irqfd();
>>     return;
>>   }
>>=20
>> which would be a much nicer, generic interface.
>=20
> But that's not how PAPR works.

But it's what a non-QEMU VFIO user would want, and it should be easy to impl=
ement.


Alex

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Benjamin Herrenschmidt]

On Tue, 2014-05-20 at 15:49 +0200, Alexander Graf wrote:
> So how about we just implement this whole thing properly as irqfd? 
> Whether QEMU can actually do anything with the interrupt is a different 
> question - we can leave it be for now. But we could model all the code 
> with the assumption that it should either handle the error itself or 
> trigger and irqfd write.

I don't object to the idea... however this smells of Deja Vu...

You often tend to want to turn something submitted that fills a specific
gap and implements a specific spec/function into some kind of idealized
grand design :-) And that means nothing gets upstream for weeks or monthes
as we churn and churn...

Sometimes it's probably worth it. Here I would argue against it and would
advocate for doing the basic functionality first, as it is used by guests,
and later add the irqfd option. I don't see any emergency here and adding
the irqfd will not cause fundamental design changes:

The "passed" flag (though I'm not fan of the name) is really something
we want in the low level handlers to avoid triggering host side EEH in
various places, regardless of whether we use irqfd or not.

This is totally orthogonal from the mechanism used for notifications.

Even in host, the detection path doesn't always involve interrupts, and
we can detect some things as a result of a host side config space access
for example etc...

So let's keep things nice and separate here. The interrupt notification
is just an "optimization" which will speed up discovery of the error in
*some* cases later on (but adds its own complexity since we have multiple
discovery path in host, so we need to keep track whether we have notified
yet or not etc...) so let's keep it for later.

Cheers,
Ben.

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Alexander Graf]

> Am 21.05.2014 um 02:19 schrieb Benjamin Herrenschmidt <benh@kernel.crashin=
g.org>:
>=20
>> On Tue, 2014-05-20 at 15:49 +0200, Alexander Graf wrote:
>> So how about we just implement this whole thing properly as irqfd?=20
>> Whether QEMU can actually do anything with the interrupt is a different=20=

>> question - we can leave it be for now. But we could model all the code=20=

>> with the assumption that it should either handle the error itself or=20
>> trigger and irqfd write.
>=20
> I don't object to the idea... however this smells of Deja Vu...
>=20
> You often tend to want to turn something submitted that fills a specific
> gap and implements a specific spec/function into some kind of idealized
> grand design :-) And that means nothing gets upstream for weeks or monthes=

> as we churn and churn...
>=20
> Sometimes it's probably worth it. Here I would argue against it and would
> advocate for doing the basic functionality first, as it is used by guests,=

> and later add the irqfd option. I don't see any emergency here and adding
> the irqfd will not cause fundamental design changes:
>=20
> The "passed" flag (though I'm not fan of the name) is really something
> we want in the low level handlers to avoid triggering host side EEH in
> various places, regardless of whether we use irqfd or not.
>=20
> This is totally orthogonal from the mechanism used for notifications.
>=20
> Even in host, the detection path doesn't always involve interrupts, and
> we can detect some things as a result of a host side config space access
> for example etc...
>=20
> So let's keep things nice and separate here. The interrupt notification
> is just an "optimization" which will speed up discovery of the error in
> *some* cases later on (but adds its own complexity since we have multiple
> discovery path in host, so we need to keep track whether we have notified
> yet or not etc...) so let's keep it for later.

EEH handling is your call, but I only see reduced complexity here. I won't n=
ak the current approach though.


Alex

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Benjamin Herrenschmidt]

On Tue, 2014-05-20 at 21:56 +1000, Gavin Shan wrote:

 .../...

> >I think what you want is an irqfd that the in-kernel eeh code
> >notifies when it sees a failure. When such an fd exists, the kernel
> >skips its own error handling.
> >
> 
> Yeah, it's a good idea and something for me to improve in phase II. We
> can discuss for more later. For now, what I have in my head is something
> like this:

However, this would be a deviation from (or extension of) PAPR. At the
moment, the way things work in PAPR is that the guest is responsible for
querying the EEH state when something "looks" like an error (ie, getting
ff's back). This is also how it works in pHyp.

We have an interrupt path in the host when doing "native" EEH, and it
would be nice to extend PAPR to also be able to shoot an event to the
guest possibly using RTAS events, but let's get the basics working and
upstream first.

Cheers,
Ben.

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Gavin Shan]

On Wed, May 21, 2014 at 10:12:11AM +1000, Benjamin Herrenschmidt wrote:
>On Tue, 2014-05-20 at 21:56 +1000, Gavin Shan wrote:
>
> .../...
>
>> >I think what you want is an irqfd that the in-kernel eeh code
>> >notifies when it sees a failure. When such an fd exists, the kernel
>> >skips its own error handling.
>> >
>> 
>> Yeah, it's a good idea and something for me to improve in phase II. We
>> can discuss for more later. For now, what I have in my head is something
>> like this:
>
>However, this would be a deviation from (or extension of) PAPR. At the
>moment, the way things work in PAPR is that the guest is responsible for
>querying the EEH state when something "looks" like an error (ie, getting
>ff's back). This is also how it works in pHyp.
>
>We have an interrupt path in the host when doing "native" EEH, and it
>would be nice to extend PAPR to also be able to shoot an event to the
>guest possibly using RTAS events, but let's get the basics working and
>upstream first.
>

Got it. Thanks, Ben :-)

Thanks,
Gavin

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Paul Mackerras]

On Tue, May 20, 2014 at 01:25:11PM +0200, Alexander Graf wrote:
> 
> On 20.05.14 10:30, Gavin Shan wrote:
> >If we detects frozen state on PE that has been passed to guest, we
> >needn't handle it. Instead, we rely on the guest to detect and recover
> >it. The patch avoid EEH event on the frozen passed PE so that the guest
> >can have chance to handle that.
> >
> >Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> 
> How does the guest learn about this failure? We'd need to inject an error
> into it, no?
> 
> I think what you want is an irqfd that the in-kernel eeh code notifies when
> it sees a failure. When such an fd exists, the kernel skips its own error
> handling.

Well... we don't have irqfd support for book3s HV upstream yet.  The
way the current code is, we have to turn on GSI routing, which puts a
hard and relatively small limit on the hardware IRQ numbers we can use
as it uses a flat array indexed by hardware IRQ number.  Which is a
problem that I need to solve somehow, but it makes using an irqfd
unattractive in the short term.

Paul.

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Alexander Graf]

> Am 03.06.2014 um 07:54 schrieb Paul Mackerras <paulus@samba.org>:
>=20
>> On Tue, May 20, 2014 at 01:25:11PM +0200, Alexander Graf wrote:
>>=20
>>> On 20.05.14 10:30, Gavin Shan wrote:
>>> If we detects frozen state on PE that has been passed to guest, we
>>> needn't handle it. Instead, we rely on the guest to detect and recover
>>> it. The patch avoid EEH event on the frozen passed PE so that the guest
>>> can have chance to handle that.
>>>=20
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>=20
>> How does the guest learn about this failure? We'd need to inject an error=

>> into it, no?
>>=20
>> I think what you want is an irqfd that the in-kernel eeh code notifies wh=
en
>> it sees a failure. When such an fd exists, the kernel skips its own error=

>> handling.
>=20
> Well... we don't have irqfd support for book3s HV upstream yet.  The
> way the current code is, we have to turn on GSI routing, which puts a
> hard and relatively small limit on the hardware IRQ numbers we can use
> as it uses a flat array indexed by hardware IRQ number.  Which is a
> problem that I need to solve somehow,

Please sync up with the ARM folks on this - they were also unhappy about the=
 routing requirements for irqfd ;).

> but it makes using an irqfd
> unattractive in the short term.

For EEH it could as well be a dumb eventfd - really just a side channel that=
 can tell user space that something happened asynchronously :).


Alex

Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE

[Benjamin Herrenschmidt]

On Tue, 2014-06-03 at 09:45 +0200, Alexander Graf wrote:
> For EEH it could as well be a dumb eventfd - really just a side
> channel that can tell user space that something happened
> asynchronously :).

Which the host kernel may have no way to detect without actively poking
at the device (fences in powernv or anything in PAPR host) and the only
user of this for now has no use for.

I insist don't bother.

Cheers,
Ben.