* [PATCH 1/4] drivers/vfio: Introduce CONFIG_VFIO_PCI_EEH
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-1-git-send-email-gwshan@linux.vnet.ibm.com>
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
^ permalink raw reply related
* [PATCH 2/4] powerpc/eeh: Flags for passed device and PE
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-1-git-send-email-gwshan@linux.vnet.ibm.com>
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
^ permalink raw reply related
* [PATCH 4/4] powerpc/eeh: Avoid event on passed PE
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-1-git-send-email-gwshan@linux.vnet.ibm.com>
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
^ permalink raw reply related
* [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-1-git-send-email-gwshan@linux.vnet.ibm.com>
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
^ permalink raw reply related
* [PATCH RFCv4 0/4] EEH Support for VFIO PCI device
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
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
^ permalink raw reply
* Re: [PATCH 3/8] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Gavin Shan @ 2014-05-20 8:28 UTC (permalink / raw)
To: Alex Williamson; +Cc: aik, Gavin Shan, kvm-ppc, agraf, qiudayu, linuxppc-dev
In-Reply-To: <1400546244.3289.341.camel@ul30vt.home>
On Mon, May 19, 2014 at 06:37:24PM -0600, Alex Williamson wrote:
>On Tue, 2014-05-20 at 10:22 +1000, Gavin Shan wrote:
>> On Mon, May 19, 2014 at 04:33:10PM -0600, Alex Williamson wrote:
>> >On Wed, 2014-05-14 at 14:11 +1000, Gavin Shan wrote:
>> >> The patch adds new IOCTL command VFIO_EEH_INFO to VFIO container
>> >> to support EEH functionality for PCI devices, which have been
>> >> passed from host to guest via VFIO.
>>
>> Thanks for your comments, Alex.W :-)
>>
>> >
>> >Some comments throughout, but overall this seems to forgo every bit of
>> >the device ownership and protection model used by VFIO and lets the user
>> >pick arbitrary host devices and do various operations, mostly unchecked.
>> >That's not acceptable.
>> >
>>
>> As what I replied to patch[2], I'm going to let VFIO-PCI-dev fd handle
>> the newly introduced IOCTL command. That way, we should follow the VFIO
>> design principles (ownership and protection) because VFIO-PCI-dev fd
>> is owned by QEMU process usually.
>>
>> Also, the address mapping maintained in EEH will be removed.
>>
>> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> >> ---
>> >> arch/powerpc/platforms/powernv/Makefile | 1 +
>> >> arch/powerpc/platforms/powernv/eeh-vfio.c | 593 ++++++++++++++++++++++++++++++
>> >> drivers/vfio/vfio_iommu_spapr_tce.c | 12 +
>> >> include/uapi/linux/vfio.h | 57 +++
>> >> 4 files changed, 663 insertions(+)
>> >> 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..2b15a03 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_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..69d5f2d
>> >> --- /dev/null
>> >> +++ b/arch/powerpc/platforms/powernv/eeh-vfio.c
>> >> @@ -0,0 +1,593 @@
>> >> +/*
>> >> + * 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_map(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pci_bus *bus, *pe_bus;
>> >> + struct pci_dev *pdev;
>> >> + struct eeh_dev *edev;
>> >> + struct eeh_pe *pe;
>> >> + int domain, bus_no, devfn;
>> >> +
>> >> + /* Host address */
>> >> + domain = info->map.host_domain;
>> >> + bus_no = (info->map.host_cfg_addr >> 8) & 0xff;
>> >> + devfn = info->map.host_cfg_addr & 0xff;
>> >
>> >Where are we validating that the user has any legitimate claim to be
>> >touching this device?
>> >
>>
>> I'll let VFIO-PCI-dev fd handle the IOCTL command. With that, we shouldn't
>> have the problem.
>>
>> >> + /* Find PCI bus */
>> >> + bus = pci_find_bus(domain, bus_no);
>> >> + if (!bus) {
>> >> + pr_warn("%s: PCI bus %04x:%02x not found\n",
>> >> + __func__, domain, bus_no);
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* Find PCI device */
>> >> + pdev = pci_get_slot(bus, devfn);
>> >> + if (!pdev) {
>> >> + pr_warn("%s: PCI device %04x:%02x:%02x.%01x not found\n",
>> >> + __func__, domain, bus_no,
>> >> + PCI_SLOT(devfn), PCI_FUNC(devfn));
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* No EEH device - almost impossible */
>> >> + edev = pci_dev_to_eeh_dev(pdev);
>> >> + if (unlikely(!edev)) {
>> >> + pci_dev_put(pdev);
>> >> + pr_warn("%s: No EEH dev for PCI device %s\n",
>> >> + __func__, pci_name(pdev));
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* Doesn't support PE migration between different PHBs */
>> >> + pe = edev->pe;
>> >> + if (!eeh_pe_passed(pe)) {
>> >> + pe_bus = eeh_pe_bus_get(pe);
>> >> + BUG_ON(!pe_bus);
>> >
>> >Can a user trigger this maliciously?
>> >
>> >> +
>> >> + /* PE# has format 00BBSS00 */
>> >> + pe->guest_addr.buid = info->map.guest_buid;
>> >> + pe->guest_addr.pe_addr = pe_bus->number << 16;
>> >> + eeh_pe_set_passed(pe, true);
>> >> + } else if (pe->guest_addr.buid != info->map.guest_buid) {
>> >> + pci_dev_put(pdev);
>> >> + pr_warn("%s: Mismatched PHB BUID (0x%llx, 0x%llx)\n",
>> >> + __func__, pe->guest_addr.buid, info->map.guest_buid);
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + edev->guest_addr.buid = info->map.guest_buid;
>> >> + edev->guest_addr.config_addr = info->map.guest_cfg_addr;
>> >> + eeh_dev_set_passed(edev, true);
>> >> +
>> >> + pr_debug("EEH: Host PCI dev %s to %llx-%02x:%02x.%01x\n",
>> >> + pci_name(pdev), info->map.guest_buid,
>> >> + (info->map.guest_cfg_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(info->map.guest_cfg_addr & 0xFF),
>> >> + PCI_FUNC(info->map.guest_cfg_addr & 0xFF));
>> >> +
>> >> + pci_dev_put(pdev);
>> >> + return 0;
>> >> +}
>> >
>> >So the effect of this function is that a user gets to setup an arbitrary
>> >guest mapping for an arbitrary host device and associated pe. Is that
>> >right? It seems bad.
>> >
>>
>> I'm going to remove this mapping in next revision.
>>
>> >> +
>> >> +static int powernv_eeh_vfio_unmap(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + struct pci_dev *pdev;
>> >> + struct eeh_dev *edev, *tmp;
>> >> + struct eeh_pe *pe;
>> >> + bool passed;
>> >> +
>> >> + /* Get EEH device */
>> >> + addr.buid = info->unmap.buid;
>> >> + addr.config_addr = info->unmap.cfg_addr;
>> >> + edev = eeh_vfio_dev_get(&addr);
>> >
>> >eeh_vfio_dev_get() just looks for a "passed" dev and a match for a well
>> >known address space. Seems very exploitable.
>> >
>> >> + if (!edev) {
>> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> >> + __func__, info->unmap.buid,
>> >> + (info->unmap.cfg_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(info->unmap.cfg_addr & 0xFF),
>> >> + PCI_FUNC(info->unmap.cfg_addr & 0xFF));
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* Return EEH device */
>> >> + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
>> >> + eeh_dev_set_passed(edev, false);
>> >> + pdev = eeh_dev_to_pci_dev(edev);
>> >> + pr_debug("EEH: Host PCI dev %s returned\n",
>> >> + pdev ? pci_name(pdev) : "NULL");
>> >> +
>> >> + /* Return PE if no EEH device is owned by guest */
>> >> + pe = edev->pe;
>> >> + passed = false;
>> >> + eeh_pe_for_each_dev(pe, edev, tmp) {
>> >> + pdev = eeh_dev_to_pci_dev(edev);
>> >> + if (pdev && pdev->subordinate)
>> >> + continue;
>> >> +
>> >> + if (eeh_dev_passed(edev)) {
>> >> + passed = true;
>> >> + break;
>> >> + }
>> >> + }
>> >> +
>> >> + if (!passed) {
>> >> + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
>> >> + eeh_pe_set_passed(pe, false);
>> >> + pr_debug("EEH: PHB#%x-PE#%x returned to host\n",
>> >> + pe->phb->global_number, pe->addr);
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_set_option(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_dev *edev;
>> >> + struct eeh_pe *pe;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int opcode = info->option.option;
>> >> + int ret = 0;
>> >> +
>> >> + /* Check opcode */
>> >> + if (opcode < EEH_OPT_DISABLE || opcode > EEH_OPT_THAW_DMA) {
>> >> + pr_warn("%s: opcode %d out of range (%d, %d)\n",
>> >> + __func__, opcode, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
>> >> + ret = 3;
>> >
>> >Please don't make up arbitrary return values.
>> >
>>
>> Nope, it will be turned to "-3" eventually by QEMU.
>
>Don't assume QEMU is your userspace.
>
>> That means "Invalid Parameter"
>> defined in PAPR spec.
>
>Is there value in matching the PAPR spec (which most people can't read)?
>If there is...
>
>> The IOCTL command handler return 3 values:
>>
>> < 0: Linux kernel error. For example, error from copy_from_user().
>> > 0: Error code to the EEH RTAS request, which will be returned to guest.
>> = 0: Success
>
>Maybe the ioctl return should match normal ioctl return values and the
>EEH error code can be stored somewhere in the structure.
>
Good idea and will apply it to next revision. Thanks.
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Option "enable" uses PCI config address */
>> >> + if (opcode == EEH_OPT_ENABLE) {
>> >> + addr.buid = info->option.buid;
>> >> + addr.config_addr = (info->option.addr >> 8) & 0xFFFF;
>> >> + edev = eeh_vfio_dev_get(&addr);
>> >> + if (!edev) {
>> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> >> + __func__, addr.buid,
>> >> + (addr.config_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(addr.config_addr & 0xFF),
>> >> + PCI_FUNC(addr.config_addr & 0xFF));
>> >> + ret = 7;
>> >> + goto out;
>> >> + }
>> >> + phb = edev->phb->private_data;
>> >> + } else {
>> >> + addr.buid = info->option.buid;
>> >> + addr.pe_addr = info->option.addr;
>> >> + pe = eeh_vfio_pe_get(&addr);
>> >> + if (!pe) {
>> >> + pr_warn("%s: Cannot find PE %llx:%x\n",
>> >> + __func__, addr.buid, addr.pe_addr);
>> >> + ret = 7;
>> >> + goto out;
>> >> + }
>> >> + phb = pe->phb->private_data;
>> >> + }
>> >> +
>> >> + /* Insure that the EEH stuff has been initialized */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + ret = 7;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /*
>> >> + * The EEH functionality has been enabled on all PEs
>> >> + * by default. So just return success. The same situation
>> >> + * would be applied while we disable EEH functionality.
>> >> + * However, the guest isn't expected to disable that
>> >> + * at all.
>> >> + */
>> >> + if (opcode == EEH_OPT_DISABLE ||
>> >> + opcode == EEH_OPT_ENABLE) {
>> >> + ret = 0;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /*
>> >> + * Call into the IODA dependent backend in order
>> >> + * to enable DMA or MMIO for the indicated PE.
>> >> + */
>> >> + if (phb->eeh_ops && phb->eeh_ops->set_option) {
>> >> + if (phb->eeh_ops->set_option(pe, opcode)) {
>> >> + pr_warn("%s: Failure from backend\n",
>> >> + __func__);
>> >> + ret = 1;
>> >> + }
>> >> + } else {
>> >> + pr_warn("%s: Unsupported request\n",
>> >> + __func__);
>> >> + ret = 7;
>> >> + }
>> >> +
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_get_addr(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_dev *edev;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int opcode = info->addr.option;
>> >> + int ret = 0;
>> >> +
>> >> + /* Check opcode */
>> >> + if (opcode != 0 && opcode != 1) {
>> >> + pr_warn("%s: opcode %d out of range (0, 1)\n",
>> >> + __func__, opcode);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Find EEH device */
>> >> + addr.buid = info->addr.buid;
>> >> + addr.config_addr = (info->addr.cfg_addr >> 8 ) & 0xFFFF;
>> >> + edev = eeh_vfio_dev_get(&addr);
>> >> + if (!edev) {
>> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> >> + __func__, addr.buid,
>> >> + (addr.config_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(addr.config_addr & 0xFF),
>> >> + PCI_FUNC(addr.config_addr & 0xFF));
>> >> + ret = 7;
>> >> + goto out;
>> >> + }
>> >> + phb = edev->phb->private_data;
>> >> +
>> >> + /* EEH enabled ? */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* EEH device passed ? */
>> >> + if (!eeh_dev_passed(edev)) {
>> >> + pr_warn("%s: EEH dev %llx:%02x:%02x.%01x owned by host\n",
>> >> + __func__, addr.buid,
>> >> + (addr.config_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(addr.config_addr & 0xFF),
>> >> + PCI_FUNC(addr.config_addr & 0xFF));
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /*
>> >> + * Fill result according to opcode. We don't differentiate
>> >> + * PCI bus and device sensitive PE here.
>> >> + */
>> >> + if (opcode == 0)
>> >> + info->addr.ret = edev->pe->guest_addr.pe_addr;
>> >> + else
>> >> + info->addr.ret = 1;
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_get_state(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_pe *pe;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int result, ret = 0;
>> >> +
>> >> + /* Locate the PE */
>> >> + addr.buid = info->state.buid;
>> >> + addr.pe_addr = info->state.pe_addr;
>> >> + pe = eeh_vfio_pe_get(&addr);
>> >> + if (!pe) {
>> >> + pr_warn("%s: Cannot locate %llx:%x\n",
>> >> + __func__, addr.buid, addr.pe_addr);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> + phb = pe->phb->private_data;
>> >> +
>> >> + /* EEH enabled ? */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Call to the IOC dependent function */
>> >> + if (phb->eeh_ops && phb->eeh_ops->get_state) {
>> >> + 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.state = 0;
>> >> + else if (result & EEH_STATE_RESET_ACTIVE)
>> >> + info->state.state = 1;
>> >> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> >> + !(result & EEH_STATE_DMA_ENABLED) &&
>> >> + !(result & EEH_STATE_MMIO_ENABLED))
>> >> + info->state.state = 2;
>> >> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> >> + (result & EEH_STATE_DMA_ENABLED) &&
>> >> + !(result & EEH_STATE_MMIO_ENABLED))
>> >> + info->state.state = 4;
>> >> + else
>> >> + info->state.state = 5;
>> >> +
>> >> + ret = 0;
>> >> + } else {
>> >> + pr_warn("%s: Unsupported request\n", __func__);
>> >> + ret = 3;
>> >> + }
>> >> +
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_pe_reset(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_pe *pe;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int opcode = info->reset.option;
>> >> + int ret = 0;
>> >> +
>> >> + /* Check opcode */
>> >> + if (opcode != EEH_RESET_DEACTIVATE &&
>> >> + opcode != EEH_RESET_HOT &&
>> >> + opcode != EEH_RESET_FUNDAMENTAL) {
>> >> + pr_warn("%s: Unsupported opcode %d\n",
>> >> + __func__, opcode);
>> >
>> >Console warnings are exploitable DoS attacks.
>> >
>>
>> Yep. I'll change all pr_warn() to pr_debug() in next revision.
>>
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Locate the PE */
>> >> + addr.buid = info->reset.buid;
>> >> + addr.pe_addr = info->reset.pe_addr;
>> >> + pe = eeh_vfio_pe_get(&addr);
>> >> + if (!pe) {
>> >> + pr_warn("%s: Cannot locate %llx:%x\n",
>> >> + __func__, addr.buid, addr.pe_addr);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> + phb = pe->phb->private_data;
>> >> +
>> >> + /* EEH enabled ? */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + 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_warn("%s: Unsupported request\n",
>> >> + __func__);
>> >> + ret = 7;
>> >> + } else {
>> >> + /*
>> >> + * 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);
>> >> +
>> >> + if (phb->eeh_ops->reset(pe, opcode)) {
>> >> + pr_warn("%s: Failure from backend\n", __func__);
>> >> + 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) {
>> >> + if (phb->eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO) ||
>> >> + phb->eeh_ops->set_option(pe, EEH_OPT_THAW_DMA)) {
>> >> + pr_warn("%s: Cannot clear frozen state\n",
>> >> + __func__);
>> >> + ret = 1;
>> >> + }
>> >> +
>> >> + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
>> >> + }
>> >> + }
>> >> +
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_pe_config(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_pe *pe;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int ret = 0;
>> >> +
>> >> + /* Locate the PE */
>> >> + addr.buid = info->config.buid;
>> >> + addr.pe_addr = info->config.pe_addr;
>> >> + pe = eeh_vfio_pe_get(&addr);
>> >> + if (!pe) {
>> >> + pr_warn("%s: Cannot locate %llx:%x\n",
>> >> + __func__, addr.buid, addr.pe_addr);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> + phb = pe->phb->private_data;
>> >> +
>> >> + /* EEH enabled ? */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + 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);
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +void eeh_vfio_release(struct iommu_table *tbl)
>> >> +{
>> >> + struct pnv_ioda_pe *pnv_pe = container_of(tbl, struct pnv_ioda_pe,
>> >> + tce32_table);
>> >> + struct pnv_phb *phb = pnv_pe->phb;
>> >> + struct eeh_pe *phb_pe, *pe;
>> >> + struct eeh_dev dev, *edev, *tmp;
>> >> +
>> >> + /* Find PHB PE */
>> >> + phb_pe = eeh_phb_pe_get(phb->hose);
>> >> + if (unlikely(!phb_pe)) {
>> >> + pr_warn("%s: Cannot find PHB#%d PE\n",
>> >> + __func__, phb->hose->global_number);
>> >> + return;
>> >> + }
>> >> +
>> >> + /* Find PE */
>> >> + memset(&dev, 0, sizeof(struct eeh_dev));
>> >> + dev.phb = phb->hose;
>> >> + dev.pe_config_addr = pnv_pe->pe_number;
>> >> + pe = eeh_pe_get(&dev);
>> >> + if (unlikely(!pe)) {
>> >> + pr_warn("%s: Cannot find PE instance for PHB#%d-PE#%d\n",
>> >> + __func__, phb->hose->global_number,
>> >> + pnv_pe->pe_number);
>> >> + return;
>> >> + }
>> >> +
>> >> + /* Release it to host */
>> >> + if (!eeh_pe_passed(pe))
>> >> + return;
>> >> +
>> >> + eeh_pe_for_each_dev(pe, edev, tmp) {
>> >> + if (!eeh_dev_passed(edev))
>> >> + continue;
>> >> +
>> >> + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
>> >
>> >Is guest_addr = { 0 } not valid? As agraf already mentioned, there are
>> >a number of issues with using a guest_address for a token.
>> >
>>
>> For now, PHB BUID can't be "0". Originally, I was planing to have some code
>> in QEMU to have unique PHB BUID across the system so that guest_address could
>> be the unique token. But I'm going to remove the address mapping in next revision
>> as Alex.G suggested.
>>
>> >> + eeh_dev_set_passed(edev, false);
>> >> + }
>> >> +
>> >> + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
>> >> + eeh_pe_set_passed(pe, false);
>> >> +}
>> >> +EXPORT_SYMBOL(eeh_vfio_release);
>> >> +
>> >> +int eeh_vfio_ioctl(unsigned long arg)
>> >> +{
>> >> + struct vfio_eeh_info info;
>> >> + int ret = -EINVAL;
>> >> +
>> >> + /* Copy over user argument */
>> >> + if (copy_from_user(&info, (void __user *)arg, sizeof(info))) {
>> >> + pr_warn("%s: Cannot copy user argument 0x%lx\n",
>> >> + __func__, arg);
>> >> + return -EFAULT;
>> >> + }
>> >> +
>> >> + /* Sanity check */
>> >> + if (info.argsz != sizeof(info)) {
>> >
>> >This breaks compatibility if you need to later add a new ops with a
>> >larger footprint.
>> >
>>
>> Ok. I'll fix it in next revision. Thanks for pointing it out.
>>
>> >> + pr_warn("%s: Invalid argument size (%d, %ld)\n",
>> >> + __func__, info.argsz, sizeof(info));
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + /* Route according to operation */
>> >> + switch (info.op) {
>> >> + case VFIO_EEH_OP_MAP:
>> >> + ret = powernv_eeh_vfio_map(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_UNMAP:
>> >> + ret = powernv_eeh_vfio_unmap(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_SET_OPTION:
>> >> + ret = powernv_eeh_vfio_set_option(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_GET_ADDR:
>> >> + ret = powernv_eeh_vfio_get_addr(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_GET_STATE:
>> >> + ret = powernv_eeh_vfio_get_state(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_PE_RESET:
>> >> + ret = powernv_eeh_vfio_pe_reset(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_PE_CONFIG:
>> >> + ret = powernv_eeh_vfio_pe_config(&info);
>> >> + break;
>> >> + default:
>> >> + pr_info("%s: Cannot handle op#%d\n",
>> >> + __func__, info.op);
>> >> + }
>> >> +
>> >> + /* Copy data back */
>> >> + if (!ret && copy_to_user((void __user *)arg, &info, sizeof(info))) {
>> >> + pr_warn("%s: Cannot copy to user 0x%lx\n",
>> >> + __func__, arg);
>> >> + return -EFAULT;
>> >> + }
>> >> +
>> >> + return ret;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(eeh_vfio_ioctl);
>> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> index a84788b..c45dece 100644
>> >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> @@ -26,6 +26,11 @@
>> >> #define DRIVER_AUTHOR "aik@ozlabs.ru"
>> >> #define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
>> >>
>> >> +#ifdef CONFIG_VFIO_EEH
>> >> +extern void eeh_vfio_release(struct iommu_table *tbl);
>> >> +extern int eeh_vfio_ioctl(unsigned long arg);
>> >> +#endif
>> >> +
>> >> static void tce_iommu_detach_group(void *iommu_data,
>> >> struct iommu_group *iommu_group);
>> >>
>> >> @@ -283,6 +288,10 @@ static long tce_iommu_ioctl(void *iommu_data,
>> >> tce_iommu_disable(container);
>> >> mutex_unlock(&container->lock);
>> >> return 0;
>> >> +#ifdef CONFIG_VFIO_EEH
>> >
>> >I'm not a fan of all these #ifdefs, hide it in eeh_vfio_ioctl() and
>> >eeh_vfio_release() if needed.
>> >
>>
>> Ok. Will do it in next revision.
>>
>> >> + case VFIO_EEH_INFO:
>> >> + return eeh_vfio_ioctl(arg);
>> >> +#endif
>> >> }
>> >>
>> >> return -ENOTTY;
>> >> @@ -342,6 +351,9 @@ static void tce_iommu_detach_group(void *iommu_data,
>> >> /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
>> >> iommu_group_id(iommu_group), iommu_group); */
>> >> container->tbl = NULL;
>> >> +#ifdef CONFIG_VFIO_EEH
>> >> + eeh_vfio_release(tbl);
>> >> +#endif
>> >> iommu_release_ownership(tbl);
>> >> }
>> >> mutex_unlock(&container->lock);
>> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> >> index cb9023d..1fd1bfb 100644
>> >> --- a/include/uapi/linux/vfio.h
>> >> +++ b/include/uapi/linux/vfio.h
>> >> @@ -455,6 +455,63 @@ struct vfio_iommu_spapr_tce_info {
>> >>
>> >> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>> >>
>> >> +/*
>> >> + * The VFIO EEH info struct provides way to support EEH functionality
>> >> + * for PCI device that is passed from host to guest via VFIO.
>> >> + */
>> >> +#define VFIO_EEH_OP_MAP 0
>> >> +#define VFIO_EEH_OP_UNMAP 1
>> >> +#define VFIO_EEH_OP_SET_OPTION 2
>> >> +#define VFIO_EEH_OP_GET_ADDR 3
>> >> +#define VFIO_EEH_OP_GET_STATE 4
>> >> +#define VFIO_EEH_OP_PE_RESET 5
>> >> +#define VFIO_EEH_OP_PE_CONFIG 6
>> >
>> >Is this really an "info" ioctl?
>> >
>>
>> Yeah, "VFIO_EEH_INFO" isn't a good name. How about to have "VFIO_EEH_HANDLER" ?
>
>VFIO_EEH_OP perhaps. Thanks,
>
Ok. Will rename it to VFIO_EEH_OP in next revision.
>Alex
>
>> >> +
>> >> +struct vfio_eeh_info {
>> >> + __u32 argsz;
>> >> + __u32 op;
>> >> +
>> >> + union {
>> >> + struct vfio_eeh_map {
>> >> + __u32 host_domain;
>> >> + __u16 host_cfg_addr;
>> >> + __u64 guest_buid;
>> >> + __u16 guest_cfg_addr;
>> >> + } map;
>> >> + struct vfio_eeh_unmap {
>> >> + __u64 buid;
>> >> + __u16 cfg_addr;
>> >> + } unmap;
>> >> + struct vfio_eeh_set_option {
>> >> + __u64 buid;
>> >> + __u32 addr;
>> >> + __u32 option;
>> >> + } option;
>> >> + struct vfio_eeh_pe_addr {
>> >> + __u64 buid;
>> >> + __u32 cfg_addr;
>> >> + __u32 option;
>> >> + __u32 ret;
>> >> + } addr;
>> >> + struct vfio_eeh_state {
>> >> + __u64 buid;
>> >> + __u32 pe_addr;
>> >> + __u32 state;
>> >> + } state;
>> >> + struct vfio_eeh_reset {
>> >> + __u64 buid;
>> >> + __u32 pe_addr;
>> >> + __u32 option;
>> >> + } reset;
>> >> + struct vfio_eeh_config {
>> >> + __u64 buid;
>> >> + __u32 pe_addr;
>> >> + } config;
>> >> + };
>> >> +};
>> >> +
>> >> +#define VFIO_EEH_INFO _IO(VFIO_TYPE, VFIO_BASE + 21)
>> >> +
>> >> /* ***************************************************************** */
>> >>
>> >> #endif /* _UAPIVFIO_H */
>>
Thanks,
Gavin
>
^ permalink raw reply
* Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional memory register sets
From: Anshuman Khandual @ 2014-05-20 8:14 UTC (permalink / raw)
To: Pedro Alves; +Cc: mikey, avagin, linux-kernel, oleg, michael, linuxppc-dev
In-Reply-To: <537A1889.8030801@redhat.com>
On 05/19/2014 08:13 PM, Pedro Alves wrote:
> On 05/19/2014 12:46 PM, Anshuman Khandual wrote:
>
>>>> I couldn't actually find any arch that currently returns -ENODEV in
>>>> the "active" hook. I see that binfmt_elf.c doesn't handle
>>>> regset->active() returning < 0. Guess that may be why. Looks like
>>>> something that could be cleaned up, to me.
>>>>
>> Also it does not consider the return value of regset->active(t->task, regset)
>> (whose objective is to figure out whether we need to request regset->n number
>> of elements or less than that) in the subsequent call to regset->get function.
>
> Indeed.
>
> TBC, do you plan on fixing this? Otherwise ...
Sure, thinking something like this as mentioned below. But still not sure how to use
the return type of -ENODEV from the function regset->active(). Right now if any
regset does have the active hook and it returns anything but positive value, it will
be ignored and the control moves to the next regset in view. This prevents the thread
core note type being written to the core dump.
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index aa3cb62..80672fb 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1553,7 +1553,15 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
if (regset->core_note_type && regset->get &&
(!regset->active || regset->active(t->task, regset))) {
int ret;
- size_t size = regset->n * regset->size;
+ size_t size;
+
+ /* Request only the active elements in the regset */
+ if (!regset->active)
+ size = regset->n * regset->size;
+ else
+ size = regset->active(t->task, regset)
+ * regset->size;
+
void *data = kmalloc(size, GFP_KERNEL);
if (unlikely(!data))
return 0;
>
>> Now coming to the installation of the .active hooks part for all the new regsets, it
>> should be pretty straight forward as well. Though its optional and used for elf_core_dump
>> purpose only, its worth adding them here. Example of an active function should be something
>> like this. The function is inexpensive as required.
>>
>> +static int tm_spr_active(struct task_struct *target,
>> + const struct user_regset *regset)
>> +{
>> + if (!cpu_has_feature(CPU_FTR_TM))
>> + return -ENODEV;
>
> ... unfortunately this will do the wrong thing.
I am not sure whether I understand this correctly. Are you saying that its wrong to return
-ENODEV in this case as above ?
^ permalink raw reply related
* Re: questions on CONFIG_PPC_ADV_DEBUG_REGS, DBCR0_BRT, and DBCR0_ACTIVE_EVENTS
From: Deepak Pandian @ 2014-05-20 8:09 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <5167457E.1050205@genband.com>
unsubcribe linuxppc-dev
On Fri, Apr 12, 2013 at 4:51 AM, Chris Friesen
<chris.friesen@genband.com> wrote:
> Hi all,
>
> Sorry for the bunch of emails, I'm working on some new stuff and running
> into issues.
>
> For CONFIG_BOOKE it appears that DBCR0_ACTIVE_EVENTS includes DBCR0_ICMP but
> not DBCR0_BRT. Is that just because none of the code paths currently using
> DBCR0_ACTIVE_EVENTS need to check DBCR0_BT?
>
> Also, in sys_debug_setcontext() why does SIG_DBG_BRANCH_TRACING return
> -EINVAL if CONFIG_PPC_ADV_DEBUG_REGS is set? Would it not be possible to
> use DBCR0_BRT?
>
> Thanks,
> Chris
>
> --
>
> Chris Friesen
> Software Designer
>
> 500 Palladium Drive, Suite 2100
> Ottawa, Ontario K2N 1C2, Canada
> www.genband.com
> office:+1.343.883.2717
> chris.friesen@genband.com
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
With Regards,
Deepak Pandian
"Deconstructing world one piece at a time"
^ permalink raw reply
* Re: [PATCH V4 2/2] powerpc/pseries: init fault_around_order for pseries
From: Madhavan Srinivasan @ 2014-05-20 8:03 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-arch, riel, rusty, dave.hansen, peterz, x86, linux-kernel,
linux-mm, ak, paulus, mgorman, linuxppc-dev, mingo,
kirill.shutemov
In-Reply-To: <20140520002834.aefb5a90.akpm@linux-foundation.org>
On Tuesday 20 May 2014 12:58 PM, Andrew Morton wrote:
> On Thu, 8 May 2014 14:58:16 +0530 Madhavan Srinivasan <maddy@linux.vnet.ibm.com> wrote:
>
>> --- a/arch/powerpc/platforms/pseries/pseries.h
>> +++ b/arch/powerpc/platforms/pseries/pseries.h
>> @@ -17,6 +17,8 @@ struct device_node;
>> extern void request_event_sources_irqs(struct device_node *np,
>> irq_handler_t handler, const char *name);
>>
>> +extern unsigned int fault_around_order;
>
> This isn't an appropriate header file for exporting something from core
> mm - what happens if arch/mn10300 wants it?.
>
> I guess include/linux/mm.h is the place.
>
Rusty already suggested this. My bad. Reason for adding it here was
that, I did the performance test for this platform. Will change and send
it out.
Thanks for review
Regards
Maddy
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Madhavan Srinivasan @ 2014-05-20 7:53 UTC (permalink / raw)
To: Andrew Morton, Rusty Russell
Cc: linux-arch, riel, ak, dave.hansen, peterz, x86, Hugh Dickins,
linux-kernel, linux-mm, paulus, mgorman, linuxppc-dev, mingo,
Kirill A. Shutemov
In-Reply-To: <20140520003201.a2360d5d.akpm@linux-foundation.org>
On Tuesday 20 May 2014 01:02 PM, Andrew Morton wrote:
> On Tue, 20 May 2014 15:52:07 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
>
>> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
>>> Andrew Morton wrote:
>>>> On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
>>>>
>>>>> Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
>>>>> the order of the fault-around size in bytes, and fault_around_pages()
>>>>> use 1UL << (fault_around_order - PAGE_SHIFT)
>>>>
>>>> Yes. And shame on me for missing it (this time!) at review.
>>>>
>>>> There's still time to fix this. Patches, please.
>>>
>>> Here it is. Made at 3.30 AM, build tested only.
>>
>> Prefer on top of Maddy's patch which makes it always a variable, rather
>> than CONFIG_DEBUG_FS. It's got enough hair as it is.
>>
>
> We're at 3.15-rc5 and this interface should be finalised for 3.16. So
> Kirrill's patch is pretty urgent and should come first.
>
> Well. It's only a debugfs interface at this stage so we are allowed to
> change it later, but it's better not to.
>
My patchset does not change the interface, but uses the current fault
around order variable from CONFIG_DEBUG_FS block to allow changes at
runtime, instead of having a constant and some cleanup.
Thanks for review
Regards
--Maddy
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Andrew Morton @ 2014-05-20 7:32 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-arch, riel, Madhavan Srinivasan, dave.hansen, peterz, x86,
Hugh Dickins, linux-kernel, linux-mm, ak, paulus, mgorman,
linuxppc-dev, mingo, Kirill A. Shutemov
In-Reply-To: <87oaythsvk.fsf@rustcorp.com.au>
On Tue, 20 May 2014 15:52:07 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
> > Andrew Morton wrote:
> >> On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> >>
> >> > Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> >> > the order of the fault-around size in bytes, and fault_around_pages()
> >> > use 1UL << (fault_around_order - PAGE_SHIFT)
> >>
> >> Yes. And shame on me for missing it (this time!) at review.
> >>
> >> There's still time to fix this. Patches, please.
> >
> > Here it is. Made at 3.30 AM, build tested only.
>
> Prefer on top of Maddy's patch which makes it always a variable, rather
> than CONFIG_DEBUG_FS. It's got enough hair as it is.
>
We're at 3.15-rc5 and this interface should be finalised for 3.16. So
Kirrill's patch is pretty urgent and should come first.
Well. It's only a debugfs interface at this stage so we are allowed to
change it later, but it's better not to.
^ permalink raw reply
* Re: [PATCH V4 2/2] powerpc/pseries: init fault_around_order for pseries
From: Andrew Morton @ 2014-05-20 7:28 UTC (permalink / raw)
To: Madhavan Srinivasan
Cc: linux-arch, riel, rusty, dave.hansen, peterz, x86, linux-kernel,
linux-mm, ak, paulus, mgorman, linuxppc-dev, mingo,
kirill.shutemov
In-Reply-To: <1399541296-18810-3-git-send-email-maddy@linux.vnet.ibm.com>
On Thu, 8 May 2014 14:58:16 +0530 Madhavan Srinivasan <maddy@linux.vnet.ibm.com> wrote:
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -17,6 +17,8 @@ struct device_node;
> extern void request_event_sources_irqs(struct device_node *np,
> irq_handler_t handler, const char *name);
>
> +extern unsigned int fault_around_order;
This isn't an appropriate header file for exporting something from core
mm - what happens if arch/mn10300 wants it?.
I guess include/linux/mm.h is the place.
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Rusty Russell @ 2014-05-20 6:22 UTC (permalink / raw)
To: Kirill A. Shutemov, Andrew Morton
Cc: linux-arch, riel, Madhavan Srinivasan, dave.hansen, peterz, x86,
Hugh Dickins, linux-kernel, linux-mm, ak, paulus, mgorman,
linuxppc-dev, mingo, Kirill A. Shutemov
In-Reply-To: <20140520004429.E660AE009B@blue.fi.intel.com>
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
> Andrew Morton wrote:
>> On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
>>
>> > Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
>> > the order of the fault-around size in bytes, and fault_around_pages()
>> > use 1UL << (fault_around_order - PAGE_SHIFT)
>>
>> Yes. And shame on me for missing it (this time!) at review.
>>
>> There's still time to fix this. Patches, please.
>
> Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Cheers,
Rusty.
^ permalink raw reply
* PowerPC Build error with patch: powerpc/ppc64: Allow allmodconfig to build (finally !)
From: Mike Qiu @ 2014-05-20 6:36 UTC (permalink / raw)
To: linuxppc
Hi all,
I face one build error in linux-next git tree, see below:
The platform is IBM P7.
[root@cena01 linux-next]# make -j60
CHK include/config/kernel.release
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
CALL scripts/checksyscalls.sh
<stdin>:1232:2: warning: #warning syscall renameat2 not implemented [-Wcpp]
CHK include/generated/compile.h
CALL arch/powerpc/kernel/systbl_chk.sh
CALL arch/powerpc/kernel/prom_init_check.sh
AS arch/powerpc/kernel/head_64.o
arch/powerpc/kernel/exceptions-64s.S: Assembler messages:
arch/powerpc/kernel/exceptions-64s.S:269: Error: operand out of range
(0x000000000000814c is not between 0xffffffffffff8000 and
0x0000000000007ffc)
arch/powerpc/kernel/exceptions-64s.S:729: Error: operand out of range
(0x000000000000814c is not between 0xffffffffffff8000 and
0x0000000000007ffc)
make[1]: *** [arch/powerpc/kernel/head_64.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [arch/powerpc/kernel] Error 2
make: *** Waiting for unfinished jobs....
Finally, I find out that it is the commit
"0be9d8b61c0c1f3c8f86292c6e237ff26acd392d powerpc/ppc64: Allow
allmodconfig to build (finally !)" case this error.
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Hugh Dickins @ 2014-05-20 2:34 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-arch, riel, Madhavan Srinivasan, dave.hansen, peterz, x86,
linux-kernel, linux-mm, ak, paulus, mgorman, akpm, linuxppc-dev,
mingo, Kirill A. Shutemov
In-Reply-To: <87d2f9jlpd.fsf@rustcorp.com.au>
On Tue, 20 May 2014, Rusty Russell wrote:
> Hugh Dickins <hughd@google.com> writes:
> >> On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
> >> >
> >> > Perhaps we try to generalize from two data points (a slight improvement
> >> > over doing it from 1!), eg:
> >> >
> >> > /* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
> >> > unsigned int fault_around_order __read_mostly =
> >> > (16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
> >
> > Rusty's bimodal answer doesn't seem the right starting point to me.
>
> ? It's not bimodal, it's graded. I think you misread?
Yikes, worse than misread, more like I was too rude even to read: sorry!
Hugh
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Rusty Russell @ 2014-05-20 1:14 UTC (permalink / raw)
To: Hugh Dickins, Madhavan Srinivasan
Cc: linux-arch, riel, ak, dave.hansen, peterz, x86, linux-kernel,
linux-mm, paulus, mgorman, akpm, linuxppc-dev, mingo,
Kirill A. Shutemov
In-Reply-To: <alpine.LSU.2.11.1405191531150.1317@eggly.anvils>
Hugh Dickins <hughd@google.com> writes:
> On Mon, 19 May 2014, Madhavan Srinivasan wrote:
>> On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
>> > Hugh Dickins <hughd@google.com> writes:
>> >> On Thu, 15 May 2014, Madhavan Srinivasan wrote:
>> >>>
>> >>> Hi Ingo,
>> >>>
>> >>> Do you have any comments for the latest version of the patchset. If
>> >>> not, kindly can you pick it up as is.
>> >>>
>> >>>
>> >>> With regards
>> >>> Maddy
>> >>>
>> >>>> Kirill A. Shutemov with 8c6e50b029 commit introduced
>> >>>> vm_ops->map_pages() for mapping easy accessible pages around
>> >>>> fault address in hope to reduce number of minor page faults.
>> >>>>
>> >>>> This patch creates infrastructure to modify the FAULT_AROUND_ORDER
>> >>>> value using mm/Kconfig. This will enable architecture maintainers
>> >>>> to decide on suitable FAULT_AROUND_ORDER value based on
>> >>>> performance data for that architecture. First patch also defaults
>> >>>> FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
>> >>>> out the performance numbers for powerpc (platform pseries) and
>> >>>> initialize the fault around order variable for pseries platform of
>> >>>> powerpc.
>> >>
>> >> Sorry for not commenting earlier - just reminded by this ping to Ingo.
>> >>
>> >> I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
>> >>
>> >> arch/powerpc/Kconfig suggests that Power supports base page size of
>> >> 4k, 16k, 64k or 256k.
>> >>
>> >> I would expect your optimal fault_around_order to depend very much on
>> >> the base page size.
>> >
>> > It was 64k, which is what PPC64 uses on all the major distributions.
>> > You really only get a choice of 4k and 64k with 64 bit power.
>> >
>> This is true. PPC64 support multiple pagesize and yes the default page
>> size of 64k, is taken as base pagesize for the tests.
>>
>> >> Perhaps fault_around_size would provide a more useful default?
>> >
>> > That seems to fit. With 4k pages and order 4, you're asking for 64k.
>> > Maddy's result shows 64k is also reasonable for 64k pages.
>> >
>> > Perhaps we try to generalize from two data points (a slight improvement
>> > over doing it from 1!), eg:
>> >
>> > /* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
>> > unsigned int fault_around_order __read_mostly =
>> > (16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
>
> Rusty's bimodal answer doesn't seem the right starting point to me.
? It's not bimodal, it's graded. I think you misread?
> Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> the order of the fault-around size in bytes, and fault_around_pages()
> use 1UL << (fault_around_order - PAGE_SHIFT)
> - when that doesn't wrap, of course!
>
> That would at least have a better chance of being appropriate for
> architectures with 8k and 16k pages (Itanium springs to mind).
Well, from our two data points it seems that we want to fault in
64k at a time whatever our page size. Perhaps it's clearer if the
code expresses itself that way.
> Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
> Did other architectures, with other page sizes, back that default?
> Clearly not powerpc.
Yeah, BenH flagged it as "we should test this" for powerpc, which is
what Maddy then did.
>> and also this will remove the
>> compile time option to disable the feature?
>
> Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
> for v3.16?
>
> I'm not sure whether Rusty was arguing against that or not. I think
> we are all three concerned to have a more sensible default than what's
> there at present. I don't feel very strongly about your Kconfig
> option: I've no objection, if it were to default to byte order 16.
I don't mind either.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Madhavan Srinivasan @ 2014-05-20 2:06 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-arch, riel, x86, dave.hansen, peterz, Rusty Russell,
linux-kernel, linux-mm, ak, paulus, mgorman, akpm, linuxppc-dev,
mingo, Kirill A. Shutemov
In-Reply-To: <alpine.LSU.2.11.1405191531150.1317@eggly.anvils>
On Tuesday 20 May 2014 04:53 AM, Hugh Dickins wrote:
> On Mon, 19 May 2014, Madhavan Srinivasan wrote:
>> On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
>>> Hugh Dickins <hughd@google.com> writes:
>>>> On Thu, 15 May 2014, Madhavan Srinivasan wrote:
>>>>>
>>>>> Hi Ingo,
>>>>>
>>>>> Do you have any comments for the latest version of the patchset. If
>>>>> not, kindly can you pick it up as is.
>>>>>
>>>>>
>>>>> With regards
>>>>> Maddy
>>>>>
>>>>>> Kirill A. Shutemov with 8c6e50b029 commit introduced
>>>>>> vm_ops->map_pages() for mapping easy accessible pages around
>>>>>> fault address in hope to reduce number of minor page faults.
>>>>>>
>>>>>> This patch creates infrastructure to modify the FAULT_AROUND_ORDER
>>>>>> value using mm/Kconfig. This will enable architecture maintainers
>>>>>> to decide on suitable FAULT_AROUND_ORDER value based on
>>>>>> performance data for that architecture. First patch also defaults
>>>>>> FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
>>>>>> out the performance numbers for powerpc (platform pseries) and
>>>>>> initialize the fault around order variable for pseries platform of
>>>>>> powerpc.
>>>>
>>>> Sorry for not commenting earlier - just reminded by this ping to Ingo.
>>>>
>>>> I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
>>>>
>>>> arch/powerpc/Kconfig suggests that Power supports base page size of
>>>> 4k, 16k, 64k or 256k.
>>>>
>>>> I would expect your optimal fault_around_order to depend very much on
>>>> the base page size.
>>>
>>> It was 64k, which is what PPC64 uses on all the major distributions.
>>> You really only get a choice of 4k and 64k with 64 bit power.
>>>
>> This is true. PPC64 support multiple pagesize and yes the default page
>> size of 64k, is taken as base pagesize for the tests.
>>
>>>> Perhaps fault_around_size would provide a more useful default?
>>>
>>> That seems to fit. With 4k pages and order 4, you're asking for 64k.
>>> Maddy's result shows 64k is also reasonable for 64k pages.
>>>
>>> Perhaps we try to generalize from two data points (a slight improvement
>>> over doing it from 1!), eg:
>>>
>>> /* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
>>> unsigned int fault_around_order __read_mostly =
>>> (16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
>
> Rusty's bimodal answer doesn't seem the right starting point to me.
>
> Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> the order of the fault-around size in bytes, and fault_around_pages()
> use 1UL << (fault_around_order - PAGE_SHIFT)
> - when that doesn't wrap, of course!
>
> That would at least have a better chance of being appropriate for
> architectures with 8k and 16k pages (Itanium springs to mind).
>
> Not necessarily right for them, since each architecture may have
> different faulting overheads; but a better chance of being right
> than blindly assuming 4k or 64k pages for everyone.
>
> I'd be glad to see that change go into v3.15: what do you think,
> Kirill, are we too late to make such a change now?
> Or do you see some objection to it?
>
>> This may be right. But these are the concerns, will not this make other
>> arch to pick default without any tuning
>
> Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
> Did other architectures, with other page sizes, back that default?
> Clearly not powerpc.
Ok.
>
>> and also this will remove the
>> compile time option to disable the feature?
>
> Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
> for v3.16?
>
> I'm not sure whether Rusty was arguing against that or not I think
> we are all three concerned to have a more sensible default than what's
> there at present. I don't feel very strongly about your Kconfig
Added it as one way to reset or disable the default value. But then I
guess we decided on having FAULT_AROUND_ORDER as a variable which is
more important than Kconfig option.
> option: I've no objection, if it were to default to byte order 16.
>
Thanks for review
With regards
Maddy
> Hugh
>
^ permalink raw reply
* powerpc/powernv: Add calls to support little endian host
From: Benjamin Herrenschmidt @ 2014-05-20 1:01 UTC (permalink / raw)
To: linuxppc-dev list
When running as a powernv "host" system on P8, we need to switch
the endianness of interrupt handlers. This does it via the appropriate
call to the OPAL firmware which may result in just switching HID0:HILE
but depending on the processor version might need to do a few more
things. This call must be done early before any other processor has
been brought out of firmware.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 0394e91..a2a3d0d 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -129,6 +129,7 @@ extern int opal_enter_rtas(struct rtas_args *args,
#define OPAL_LPC_READ 67
#define OPAL_LPC_WRITE 68
#define OPAL_RETURN_CPU 69
+#define OPAL_REINIT_CPUS 70
#define OPAL_FLASH_VALIDATE 76
#define OPAL_FLASH_MANAGE 77
#define OPAL_FLASH_UPDATE 78
@@ -599,6 +600,11 @@ struct OpalIoPhb3ErrorData {
uint64_t pestB[OPAL_PHB3_NUM_PEST_REGS];
};
+enum {
+ OPAL_REINIT_CPUS_HILE_BE = (1 << 0),
+ OPAL_REINIT_CPUS_HILE_LE = (1 << 1),
+};
+
typedef struct oppanel_line {
const char * line;
uint64_t line_len;
@@ -719,6 +725,7 @@ int64_t opal_pci_next_error(uint64_t phb_id, uint64_t *first_frozen_pe,
uint16_t *pci_error_type, uint16_t *severity);
int64_t opal_pci_poll(uint64_t phb_id);
int64_t opal_return_cpu(void);
+int64_t opal_reinit_cpus(uint64_t flags);
int64_t opal_xscom_read(uint32_t gcid, uint64_t pcb_addr, __be64 *val);
int64_t opal_xscom_write(uint32_t gcid, uint64_t pcb_addr, uint64_t val);
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index e780650..3abb518 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -123,6 +123,7 @@ OPAL_CALL(opal_xscom_write, OPAL_XSCOM_WRITE);
OPAL_CALL(opal_lpc_read, OPAL_LPC_READ);
OPAL_CALL(opal_lpc_write, OPAL_LPC_WRITE);
OPAL_CALL(opal_return_cpu, OPAL_RETURN_CPU);
+OPAL_CALL(opal_reinit_cpus, OPAL_REINIT_CPUS);
OPAL_CALL(opal_validate_flash, OPAL_FLASH_VALIDATE);
OPAL_CALL(opal_manage_flash, OPAL_FLASH_MANAGE);
OPAL_CALL(opal_update_flash, OPAL_FLASH_UPDATE);
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 1c798cd..359f18a 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -42,6 +42,21 @@ static DEFINE_SPINLOCK(opal_notifier_lock);
static uint64_t last_notified_mask = 0x0ul;
static atomic_t opal_notifier_hold = ATOMIC_INIT(0);
+static void opal_reinit_cores(void)
+{
+ /* Do the actual re-init, This will clobber all FPRs, VRs, etc...
+ *
+ * It will preserve non volatile GPRs and HSPRG0/1. It will
+ * also restore HIDs and other SPRs to their original value
+ * but it might clobber a bunch.
+ */
+#ifdef __BIG_ENDIAN__
+ opal_reinit_cpus(OPAL_REINIT_CPUS_HILE_BE);
+#else
+ opal_reinit_cpus(OPAL_REINIT_CPUS_HILE_LE);
+#endif
+}
+
int __init early_init_dt_scan_opal(unsigned long node,
const char *uname, int depth, void *data)
{
@@ -77,6 +92,13 @@ int __init early_init_dt_scan_opal(unsigned long node,
printk("OPAL V1 detected !\n");
}
+ /* Reinit all cores with the right endian */
+ opal_reinit_cores();
+
+ /* Restore some bits */
+ if (cur_cpu_spec->cpu_restore)
+ cur_cpu_spec->cpu_restore();
+
return 1;
}
^ permalink raw reply related
* [PATCH net-next 9/9] powerpc/fsl: fsl_soc: remove 'fixed-link' parsing code
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
Parsing and registration of fixed PHY devices was needed with the use of
of_phy_connect_fixed_link() because this function was using the
designated PHY address identifier (first cell of the property) as the
address to bind the PHY on the emulated bus.
Since commit 3be2a49e5c08d268f8af0dd4fe89a24ea8cdc339 ("of: provide a
binding for fixed link PHYs") a new pair of functions has been
introduced which allows for dynamic address allocation of these fixed
PHY devices, but also parses the old 'fixed-link' 5-digit property.
Registration of fixed PHY early in platform code was needed because we
could not issue a fixed MDIO bus re-scan within network drivers. The
fixed PHYs had to be registered before the network drivers would call
of_phy_connect_fixed_link(). All of these caveats are solved now, such
that we can safely remove of_add_fixed_phys() now.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
arch/powerpc/sysdev/fsl_soc.c | 32 --------------------------------
1 file changed, 32 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 228cf91b91c1..ffd1169ebaab 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -25,7 +25,6 @@
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/phy.h>
-#include <linux/phy_fixed.h>
#include <linux/spi/spi.h>
#include <linux/fsl_devices.h>
#include <linux/fs_enet_pd.h>
@@ -178,37 +177,6 @@ u32 get_baudrate(void)
EXPORT_SYMBOL(get_baudrate);
#endif /* CONFIG_CPM2 */
-#ifdef CONFIG_FIXED_PHY
-static int __init of_add_fixed_phys(void)
-{
- int ret;
- struct device_node *np;
- u32 *fixed_link;
- struct fixed_phy_status status = {};
-
- for_each_node_by_name(np, "ethernet") {
- fixed_link = (u32 *)of_get_property(np, "fixed-link", NULL);
- if (!fixed_link)
- continue;
-
- status.link = 1;
- status.duplex = fixed_link[1];
- status.speed = fixed_link[2];
- status.pause = fixed_link[3];
- status.asym_pause = fixed_link[4];
-
- ret = fixed_phy_add(PHY_POLL, fixed_link[0], &status);
- if (ret) {
- of_node_put(np);
- return ret;
- }
- }
-
- return 0;
-}
-arch_initcall(of_add_fixed_phys);
-#endif /* CONFIG_FIXED_PHY */
-
#if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
static __be32 __iomem *rstcr;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 8/9] of: mdio: remove of_phy_connect_fixed_link
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
All in-tree drivers have been converted to use the new pair of
functions: of_is_fixed_phy_link() plus of_phy_register_fixed_link(), we
can now safely remove of_phy_connect_fixed_link.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/of/of_mdio.c | 38 --------------------------------------
include/linux/of_mdio.h | 10 ----------
2 files changed, 48 deletions(-)
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 1def0bb5cb37..4c1e01ed16dc 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -246,44 +246,6 @@ struct phy_device *of_phy_connect(struct net_device *dev,
EXPORT_SYMBOL(of_phy_connect);
/**
- * of_phy_connect_fixed_link - Parse fixed-link property and return a dummy phy
- * @dev: pointer to net_device claiming the phy
- * @hndlr: Link state callback for the network device
- * @iface: PHY data interface type
- *
- * This function is a temporary stop-gap and will be removed soon. It is
- * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers. Do
- * not call this function from new drivers.
- */
-struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
- void (*hndlr)(struct net_device *),
- phy_interface_t iface)
-{
- struct device_node *net_np;
- char bus_id[MII_BUS_ID_SIZE + 3];
- struct phy_device *phy;
- const __be32 *phy_id;
- int sz;
-
- if (!dev->dev.parent)
- return NULL;
-
- net_np = dev->dev.parent->of_node;
- if (!net_np)
- return NULL;
-
- phy_id = of_get_property(net_np, "fixed-link", &sz);
- if (!phy_id || sz < sizeof(*phy_id))
- return NULL;
-
- sprintf(bus_id, PHY_ID_FMT, "fixed-0", be32_to_cpu(phy_id[0]));
-
- phy = phy_connect(dev, bus_id, hndlr, iface);
- return IS_ERR(phy) ? NULL : phy;
-}
-EXPORT_SYMBOL(of_phy_connect_fixed_link);
-
-/**
* of_phy_attach - Attach to a PHY without starting the state machine
* @dev: pointer to net_device claiming the phy
* @phy_np: Node pointer for the PHY
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 0aa367e316cb..d449018d0726 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -22,9 +22,6 @@ extern struct phy_device *of_phy_connect(struct net_device *dev,
struct phy_device *of_phy_attach(struct net_device *dev,
struct device_node *phy_np, u32 flags,
phy_interface_t iface);
-extern struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
- void (*hndlr)(struct net_device *),
- phy_interface_t iface);
extern struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
@@ -59,13 +56,6 @@ static inline struct phy_device *of_phy_attach(struct net_device *dev,
return NULL;
}
-static inline struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
- void (*hndlr)(struct net_device *),
- phy_interface_t iface)
-{
- return NULL;
-}
-
static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)
{
return NULL;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 7/9] ucc_geth: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/freescale/ucc_geth.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index c8299c31b21f..fab39e295441 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -1728,9 +1728,6 @@ static int init_phy(struct net_device *dev)
phydev = of_phy_connect(dev, ug_info->phy_node, &adjust_link, 0,
priv->phy_interface);
- if (!phydev)
- phydev = of_phy_connect_fixed_link(dev, &adjust_link,
- priv->phy_interface);
if (!phydev) {
dev_err(&dev->dev, "Could not attach to PHY\n");
return -ENODEV;
@@ -3790,6 +3787,17 @@ static int ucc_geth_probe(struct platform_device* ofdev)
ug_info->uf_info.irq = irq_of_parse_and_map(np, 0);
ug_info->phy_node = of_parse_phandle(np, "phy-handle", 0);
+ if (!ug_info->phy_node) {
+ /* In the case of a fixed PHY, the DT node associated
+ * to the PHY is the Ethernet MAC DT node.
+ */
+ if (of_phy_is_fixed_link(np)) {
+ err = of_phy_register_fixed_link(np);
+ if (err)
+ return err;
+ }
+ ug_info->phy_node = np;
+ }
/* Find the TBI PHY node. If it's not there, we don't support SGMII */
ug_info->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 6/9] gianfar: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/freescale/gianfar.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index e2d42475b006..282674027c92 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -889,6 +889,17 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
+ /* In the case of a fixed PHY, the DT node associated
+ * to the PHY is the Ethernet MAC DT node.
+ */
+ if (of_phy_is_fixed_link(np)) {
+ err = of_phy_register_fixed_link(np);
+ if (err)
+ goto err_grp_init;
+
+ priv->phy_node = np;
+ }
+
/* Find the TBI PHY. If it's not there, we don't support SGMII */
priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
@@ -1660,9 +1671,6 @@ static int init_phy(struct net_device *dev)
priv->phydev = of_phy_connect(dev, priv->phy_node, &adjust_link, 0,
interface);
- if (!priv->phydev)
- priv->phydev = of_phy_connect_fixed_link(dev, &adjust_link,
- interface);
if (!priv->phydev) {
dev_err(&dev->dev, "could not attach to PHY\n");
return -ENODEV;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 5/9] fs_enet: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index dc80db41d6b3..d602711e00e9 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -792,10 +792,6 @@ static int fs_init_phy(struct net_device *dev)
phydev = of_phy_connect(dev, fep->fpi->phy_node, &fs_adjust_link, 0,
iface);
if (!phydev) {
- phydev = of_phy_connect_fixed_link(dev, &fs_adjust_link,
- iface);
- }
- if (!phydev) {
dev_err(&dev->dev, "Could not attach to PHY\n");
return -ENODEV;
}
@@ -1029,9 +1025,15 @@ static int fs_enet_probe(struct platform_device *ofdev)
fpi->use_napi = 1;
fpi->napi_weight = 17;
fpi->phy_node = of_parse_phandle(ofdev->dev.of_node, "phy-handle", 0);
- if ((!fpi->phy_node) && (!of_get_property(ofdev->dev.of_node, "fixed-link",
- NULL)))
- goto out_free_fpi;
+ if (!fpi->phy_node) {
+ if (of_phy_is_fixed_link(ofdev->dev.of_node)) {
+ err = of_phy_register_fixed_link(ofdev->dev.of_node);
+ if (err)
+ goto out_free_fpi;
+
+ fpi->phy_node = ofdev->dev.of_node;
+ }
+ }
if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec")) {
phy_connection_type = of_get_property(ofdev->dev.of_node,
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 4/9] net: systemport: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/bcmsysport.c | 17 +++++++++++++++--
drivers/net/ethernet/broadcom/bcmsysport.h | 1 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index d40c5b969e9e..dc708a888f80 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1327,8 +1327,8 @@ static int bcm_sysport_open(struct net_device *dev)
/* Read CRC forward */
priv->crc_fwd = !!(umac_readl(priv, UMAC_CMD) & CMD_CRC_FWD);
- priv->phydev = of_phy_connect_fixed_link(dev, bcm_sysport_adj_link,
- priv->phy_interface);
+ priv->phydev = of_phy_connect(dev, priv->phy_dn, bcm_sysport_adj_link,
+ 0, priv->phy_interface);
if (!priv->phydev) {
netdev_err(dev, "could not attach to PHY\n");
return -ENODEV;
@@ -1551,6 +1551,19 @@ static int bcm_sysport_probe(struct platform_device *pdev)
if (priv->phy_interface < 0)
priv->phy_interface = PHY_INTERFACE_MODE_GMII;
+ /* In the case of a fixed PHY, the DT node associated
+ * to the PHY is the Ethernet MAC DT node.
+ */
+ if (of_phy_is_fixed_link(dn)) {
+ ret = of_phy_register_fixed_link(dn);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register fixed PHY\n");
+ goto err;
+ }
+
+ priv->phy_dn = dn;
+ }
+
/* Initialize netdevice members */
macaddr = of_get_mac_address(dn);
if (!macaddr || !is_valid_ether_addr(macaddr)) {
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index abdeb62616df..73fd04a94797 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -656,6 +656,7 @@ struct bcm_sysport_priv {
unsigned int rx_c_index;
/* PHY device */
+ struct device_node *phy_dn;
struct phy_device *phydev;
phy_interface_t phy_interface;
int old_pause;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 3/9] net: bcmgenet: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmmii.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 4608673beaff..add8d8596084 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -298,6 +298,7 @@ int bcmgenet_mii_config(struct net_device *dev)
static int bcmgenet_mii_probe(struct net_device *dev)
{
struct bcmgenet_priv *priv = netdev_priv(dev);
+ struct device_node *dn = priv->pdev->dev.of_node;
struct phy_device *phydev;
unsigned int phy_flags;
int ret;
@@ -307,15 +308,19 @@ static int bcmgenet_mii_probe(struct net_device *dev)
return 0;
}
- if (priv->phy_dn)
- phydev = of_phy_connect(dev, priv->phy_dn,
- bcmgenet_mii_setup, 0,
- priv->phy_interface);
- else
- phydev = of_phy_connect_fixed_link(dev,
- bcmgenet_mii_setup,
- priv->phy_interface);
+ /* In the case of a fixed PHY, the DT node associated
+ * to the PHY is the Ethernet MAC DT node.
+ */
+ if (of_phy_is_fixed_link(dn)) {
+ ret = of_phy_register_fixed_link(dn);
+ if (ret)
+ return ret;
+
+ priv->phy_dn = dn;
+ }
+ phydev = of_phy_connect(dev, priv->phy_dn, bcmgenet_mii_setup, 0,
+ priv->phy_interface);
if (!phydev) {
pr_err("could not attach to PHY\n");
return -ENODEV;
--
1.9.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox