* [PATCH 8/8] powernv/eeh: Do hotplug on devices without EEH aware driver
From: Gavin Shan @ 2013-06-27 5:46 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Gavin Shan
In-Reply-To: <1372312009-13710-1-git-send-email-shangw@linux.vnet.ibm.com>
During recovery for EEH errors, the device driver requires reset
explicitly (most of cases). The EEH core doesn't do hotplug during
reset. However, there might have some device drivers that can't
support EEH. So the deivce can't be put into quite state during
the reset and possibly requesting PCI config or MMIO access. That
would lead to the failure of the reset, and we don't expect that.
The patch intends to fix that by removing those devices whose drivers
can't support EEH before reset and added into the system after reset.
In the result, it would avoid the race condition mentioned as above.
The idea was proposed by Ben.
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/eeh.h | 4 +-
arch/powerpc/include/asm/pci.h | 1 +
arch/powerpc/kernel/eeh_driver.c | 134 +++++++++++++++++++++++++++++++++++++-
3 files changed, 137 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 09a8743..dd62006 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -82,7 +82,8 @@ struct eeh_pe {
* another tree except the currently existing tree of PCI
* buses and PCI devices
*/
-#define EEH_DEV_IRQ_DISABLED (1<<0) /* Interrupt disabled */
+#define EEH_DEV_IRQ_DISABLED (1 << 0) /* Interrupt disabled */
+#define EEH_DEV_REMOVED (1 << 1) /* PCI device removed */
struct eeh_dev {
int mode; /* EEH mode */
@@ -95,6 +96,7 @@ struct eeh_dev {
struct pci_controller *phb; /* Associated PHB */
struct device_node *dn; /* Associated device node */
struct pci_dev *pdev; /* Associated PCI device */
+ struct pci_bus *bus; /* PCI bus used in hotplug */
};
static inline struct device_node *eeh_dev_to_of_node(struct eeh_dev *edev)
diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index 6653f27..af10ec5 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -183,6 +183,7 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
resource_size_t *start, resource_size_t *end);
extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
+extern void pcibios_setup_device(struct pci_dev *dev);
extern void pcibios_setup_bus_devices(struct pci_bus *bus);
extern void pcibios_setup_bus_self(struct pci_bus *bus);
extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 2b1ce17..cb3baab 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -244,6 +244,7 @@ static void *eeh_report_reset(void *data, void *userdata)
struct pci_dev *dev = eeh_dev_to_pci_dev(edev);
enum pci_ers_result rc, *res = userdata;
struct pci_driver *driver;
+ bool enable_irq = true;
if (!dev) return NULL;
dev->error_state = pci_channel_io_normal;
@@ -251,7 +252,21 @@ static void *eeh_report_reset(void *data, void *userdata)
driver = eeh_pcid_get(dev);
if (!driver) return NULL;
- eeh_enable_irq(dev);
+ /*
+ * For those PCI devices just added, we reloaded its driver
+ * and needn't to enable the interrupt. The driver should
+ * take care of that. Otherwise, complaint raised from IRQ
+ * subsystem.
+ */
+ if (eeh_probe_mode_dev() && (edev->mode & EEH_DEV_REMOVED)) {
+ edev->mode &= ~(EEH_DEV_REMOVED | EEH_DEV_IRQ_DISABLED);
+ edev->bus = NULL;
+ enable_irq = false;
+
+ }
+
+ if (enable_irq)
+ eeh_enable_irq(dev);
if (!driver->err_handler ||
!driver->err_handler->slot_reset) {
@@ -338,6 +353,115 @@ static void *eeh_report_failure(void *data, void *userdata)
return NULL;
}
+static void *eeh_rmv_device(void *data, void *userdata)
+{
+ struct pci_driver *driver;
+ struct eeh_dev *edev = (struct eeh_dev *)data;
+ struct pci_dev *dev = eeh_dev_to_pci_dev(edev);
+ int *removed = (int *)userdata;
+
+ /*
+ * Actually, we should remove the PCI bridges as well.
+ * However, that's lots of complexity to do that,
+ * particularly some of devices under the bridge might
+ * support EEH. So we just care about PCI devices for
+ * simplicity here.
+ */
+ if (!dev || (dev->hdr_type & PCI_HEADER_TYPE_BRIDGE))
+ return NULL;
+ driver = eeh_pcid_get(dev);
+ if (driver && driver->err_handler)
+ return NULL;
+
+ /* If the driver doesn't support EEH, remove it */
+ pr_info("EEH: Removing device %s without EEH support\n",
+ pci_name(dev));
+
+ /* Detach EEH device from PCI device */
+ edev->pdev = NULL;
+ dev->dev.archdata.edev = NULL;
+ pci_dev_put(dev);
+
+ /* Remove and address cache */
+ eeh_addr_cache_rmv_dev(dev);
+ eeh_sysfs_remove_device(dev);
+
+ /* Remove it from PCI subsystem */
+ edev->mode |= EEH_DEV_REMOVED;
+ edev->bus = dev->bus;
+ pci_stop_and_remove_bus_device(dev);
+ (*removed)++;
+
+ return NULL;
+}
+
+static void *eeh_add_device(void *data, void *userdata)
+{
+ struct eeh_dev *edev = (struct eeh_dev *)data;
+ struct pci_dev *dev;
+ struct pci_bus *bus;
+ struct resource *r;
+ int *removed = (int *)userdata;
+ int devfn, i;
+
+ if (!edev || !(edev->mode & EEH_DEV_REMOVED))
+ return NULL;
+ if (*removed <= 0)
+ return edev;
+
+ /*
+ * We don't clear EEH_DEV_REMOVED flag here.
+ * Instead, do that before enabling IRQ to
+ * avoid complain from IRQ subsystem.
+ */
+ *removed -= 1;
+ bus = edev->bus;
+ devfn = edev->config_addr & 0xFF;
+ pr_info("EEH: Adding PCI device %04x:%02x:%02x.%01x\n",
+ edev->phb->global_number, bus->number,
+ PCI_SLOT(devfn), PCI_FUNC(devfn));
+
+ /* Scan PCI function */
+ dev = pci_scan_single_device(bus, devfn);
+ if (!dev) {
+ pr_err("%s: Can't scan PCI function %02x:%02x.%01x\n",
+ __func__, bus->number, PCI_SLOT(devfn),
+ PCI_FUNC(devfn));
+ return NULL;
+ }
+
+ /*
+ * Setup the PCI device. It's not enough to
+ * claim the resource and we need assign or
+ * reassign that.
+ */
+ pcibios_setup_device(dev);
+ for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+ r = &dev->resource[i];
+ if (r->parent || !r->flags)
+ continue;
+ if (pci_assign_resource(dev, i)) {
+ pr_err("%s: Can't allocate %pR for %s\n",
+ __func__, r, pci_name(dev));
+ /* Clear it out */
+ r->start = 0;
+ r->end = 0;
+ r->flags = 0;
+ }
+ }
+
+ /* Associate EEH device with PCI device */
+ pci_dev_get(dev);
+ edev->pdev = dev;
+ dev->dev.archdata.edev = edev;
+ eeh_addr_cache_insert_dev(dev);
+
+ pci_bus_add_devices(bus);
+ eeh_sysfs_add_device(dev);
+
+ return NULL;
+}
+
/**
* eeh_reset_device - Perform actual reset of a pci slot
* @pe: EEH PE
@@ -351,6 +475,7 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus)
{
struct timeval tstamp;
int cnt, rc;
+ int removed = 0;
/* pcibios will clear the counter; save the value */
cnt = pe->freeze_count;
@@ -364,6 +489,8 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus)
*/
if (bus)
__pcibios_remove_pci_devices(bus, 0);
+ else if (eeh_probe_mode_dev())
+ eeh_pe_dev_traverse(pe, eeh_rmv_device, &removed);
/* Reset the pci controller. (Asserts RST#; resets config space).
* Reconfigure bridges and devices. Don't try to bring the system
@@ -384,8 +511,13 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus)
* potentially weird things happen.
*/
if (bus) {
+ pr_info("EEH: Hold for 5 seconds after reset\n");
ssleep(5);
pcibios_add_pci_devices(bus);
+ } else if (eeh_probe_mode_dev() && removed) {
+ pr_info("EEH: Hold for 5 seconds after reset\n");
+ ssleep(5);
+ eeh_pe_dev_traverse(pe, eeh_add_device, &removed);
}
pe->tstamp = tstamp;
--
1.7.5.4
^ permalink raw reply related
* [PATCH 7/8] powerpc/powernv: Use dev-node in PCI config accessors
From: Gavin Shan @ 2013-06-27 5:46 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Gavin Shan
In-Reply-To: <1372312009-13710-1-git-send-email-shangw@linux.vnet.ibm.com>
Currently, we're using the combo (PCI bus + devfn) in the PCI
config accessors and PCI config accessors in EEH depends on them.
However, it's not safe to refer the PCI bus which might have been
removed during hotplug. So we're using device node in the PCI
config accessors and the corresponding backends just reuse them.
The patch also fix one potential risk: We possiblly have frozen
PE during the early PCI probe time, but we haven't setup the PE
mapping yet. So the errors should be counted to PE#0.
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/eeh-powernv.c | 44 +---------
arch/powerpc/platforms/powernv/pci.c | 120 ++++++++++++++++----------
arch/powerpc/platforms/powernv/pci.h | 4 +
3 files changed, 79 insertions(+), 89 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index 9559115..969cce7 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -315,46 +315,6 @@ static int powernv_eeh_configure_bridge(struct eeh_pe *pe)
}
/**
- * powernv_eeh_read_config - Read PCI config space
- * @dn: device node
- * @where: PCI address
- * @size: size to read
- * @val: return value
- *
- * Read config space from the speicifed device
- */
-static int powernv_eeh_read_config(struct device_node *dn, int where,
- int size, u32 *val)
-{
- struct eeh_dev *edev = of_node_to_eeh_dev(dn);
- struct pci_dev *dev = eeh_dev_to_pci_dev(edev);
- struct pci_controller *hose = edev->phb;
-
- return hose->ops->read(dev->bus, dev->devfn, where, size, val);
-}
-
-/**
- * powernv_eeh_write_config - Write PCI config space
- * @dn: device node
- * @where: PCI address
- * @size: size to write
- * @val: value to be written
- *
- * Write config space to the specified device
- */
-static int powernv_eeh_write_config(struct device_node *dn, int where,
- int size, u32 val)
-{
- struct eeh_dev *edev = of_node_to_eeh_dev(dn);
- struct pci_dev *dev = eeh_dev_to_pci_dev(edev);
- struct pci_controller *hose = edev->phb;
-
- hose = pci_bus_to_host(dev->bus);
-
- return hose->ops->write(dev->bus, dev->devfn, where, size, val);
-}
-
-/**
* powernv_eeh_next_error - Retrieve next EEH error to handle
* @pe: Affected PE
*
@@ -389,8 +349,8 @@ static struct eeh_ops powernv_eeh_ops = {
.wait_state = powernv_eeh_wait_state,
.get_log = powernv_eeh_get_log,
.configure_bridge = powernv_eeh_configure_bridge,
- .read_config = powernv_eeh_read_config,
- .write_config = powernv_eeh_write_config,
+ .read_config = pnv_pci_cfg_read,
+ .write_config = pnv_pci_cfg_write,
.next_error = powernv_eeh_next_error
};
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index 1f31826..229b034 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -230,47 +230,50 @@ static void pnv_pci_handle_eeh_config(struct pnv_phb *phb, u32 pe_no)
spin_unlock_irqrestore(&phb->lock, flags);
}
-static void pnv_pci_config_check_eeh(struct pnv_phb *phb, struct pci_bus *bus,
- u32 bdfn)
+static void pnv_pci_config_check_eeh(struct pnv_phb *phb,
+ struct device_node *dn)
{
s64 rc;
u8 fstate;
u16 pcierr;
u32 pe_no;
- /* Get PE# if we support IODA */
- pe_no = phb->bdfn_to_pe ? phb->bdfn_to_pe(phb, bus, bdfn & 0xff) : 0;
+ /*
+ * Get the PE#. During the PCI probe stage, we might not
+ * setup that yet. So all ER errors should be mapped to
+ * PE#0
+ */
+ pe_no = PCI_DN(dn)->pe_number;
+ if (pe_no == IODA_INVALID_PE)
+ pe_no = 0;
/* Read freeze status */
rc = opal_pci_eeh_freeze_status(phb->opal_id, pe_no, &fstate, &pcierr,
NULL);
if (rc) {
- pr_warning("PCI %d: Failed to read EEH status for PE#%d,"
- " err %lld\n", phb->hose->global_number, pe_no, rc);
+ pr_warning("%s: Can't read EEH status (PE#%d) for "
+ "%s, err %lld\n",
+ __func__, pe_no, dn->full_name, rc);
return;
}
- cfg_dbg(" -> EEH check, bdfn=%04x PE%d fstate=%x\n",
- bdfn, pe_no, fstate);
+ cfg_dbg(" -> EEH check, bdfn=%04x PE#%d fstate=%x\n",
+ (PCI_DN(dn)->busno << 8) | (PCI_DN(dn)->devfn),
+ pe_no, fstate);
if (fstate != 0)
pnv_pci_handle_eeh_config(phb, pe_no);
}
-static int pnv_pci_read_config(struct pci_bus *bus,
- unsigned int devfn,
- int where, int size, u32 *val)
+int pnv_pci_cfg_read(struct device_node *dn,
+ int where, int size, u32 *val)
{
- struct pci_controller *hose = pci_bus_to_host(bus);
- struct pnv_phb *phb = hose->private_data;
+ struct pci_dn *pdn = PCI_DN(dn);
+ struct pnv_phb *phb = pdn->phb->private_data;
+ u32 bdfn = (pdn->busno << 8) | pdn->devfn;
#ifdef CONFIG_EEH
- struct device_node *busdn, *dn;
struct eeh_pe *phb_pe = NULL;
#endif
- u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
s64 rc;
- if (hose == NULL)
- return PCIBIOS_DEVICE_NOT_FOUND;
-
switch (size) {
case 1: {
u8 v8;
@@ -294,8 +297,8 @@ static int pnv_pci_read_config(struct pci_bus *bus,
default:
return PCIBIOS_FUNC_NOT_SUPPORTED;
}
- cfg_dbg("pnv_pci_read_config bus: %x devfn: %x +%x/%x -> %08x\n",
- bus->number, devfn, where, size, *val);
+ cfg_dbg("%s: bus: %x devfn: %x +%x/%x -> %08x\n",
+ __func__, pdn->busno, pdn->devfn, where, size, *val);
/*
* Check if the specified PE has been put into frozen
@@ -304,44 +307,33 @@ static int pnv_pci_read_config(struct pci_bus *bus,
* PHB-fatal errors.
*/
#ifdef CONFIG_EEH
- phb_pe = eeh_phb_pe_get(hose);
+ phb_pe = eeh_phb_pe_get(pdn->phb);
if (phb_pe && (phb_pe->state & EEH_PE_ISOLATED))
return PCIBIOS_SUCCESSFUL;
if (phb->eeh_state & PNV_EEH_STATE_ENABLED) {
- if (*val == EEH_IO_ERROR_VALUE(size)) {
- busdn = pci_bus_to_OF_node(bus);
- for (dn = busdn->child; dn; dn = dn->sibling) {
- struct pci_dn *pdn = PCI_DN(dn);
-
- if (pdn && pdn->devfn == devfn &&
- eeh_dev_check_failure(of_node_to_eeh_dev(dn)))
- return PCIBIOS_DEVICE_NOT_FOUND;
- }
- }
+ if (*val == EEH_IO_ERROR_VALUE(size) &&
+ eeh_dev_check_failure(of_node_to_eeh_dev(dn)))
+ return PCIBIOS_DEVICE_NOT_FOUND;
} else {
- pnv_pci_config_check_eeh(phb, bus, bdfn);
+ pnv_pci_config_check_eeh(phb, dn);
}
#else
- pnv_pci_config_check_eeh(phb, bus, bdfn);
+ pnv_pci_config_check_eeh(phb, dn);
#endif
return PCIBIOS_SUCCESSFUL;
}
-static int pnv_pci_write_config(struct pci_bus *bus,
- unsigned int devfn,
- int where, int size, u32 val)
+int pnv_pci_cfg_write(struct device_node *dn,
+ int where, int size, u32 val)
{
- struct pci_controller *hose = pci_bus_to_host(bus);
- struct pnv_phb *phb = hose->private_data;
- u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
+ struct pci_dn *pdn = PCI_DN(dn);
+ struct pnv_phb *phb = pdn->phb->private_data;
+ u32 bdfn = (pdn->busno << 8) | pdn->devfn;
- if (hose == NULL)
- return PCIBIOS_DEVICE_NOT_FOUND;
-
- cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n",
- bus->number, devfn, where, size, val);
+ cfg_dbg("%s: bus: %x devfn: %x +%x/%x -> %08x\n",
+ pdn->busno, pdn->devfn, where, size, val);
switch (size) {
case 1:
opal_pci_config_write_byte(phb->opal_id, bdfn, where, val);
@@ -359,16 +351,50 @@ static int pnv_pci_write_config(struct pci_bus *bus,
/* Check if the PHB got frozen due to an error (no response) */
#ifdef CONFIG_EEH
if (!(phb->eeh_state & PNV_EEH_STATE_ENABLED))
- pnv_pci_config_check_eeh(phb, bus, bdfn);
+ pnv_pci_config_check_eeh(phb, dn);
#else
- pnv_pci_config_check_eeh(phb, bus, bdfn);
+ pnv_pci_config_check_eeh(phb, dn);
#endif
return PCIBIOS_SUCCESSFUL;
}
+static int pnv_pci_read_config(struct pci_bus *bus,
+ unsigned int devfn,
+ int where, int size, u32 *val)
+{
+ struct device_node *dn, *busdn = pci_bus_to_OF_node(bus);
+ struct pci_dn *pdn;
+
+ for (dn = busdn->child; dn; dn = dn->sibling) {
+ pdn = PCI_DN(dn);
+ if (pdn && pdn->devfn == devfn)
+ return pnv_pci_cfg_read(dn, where, size, val);
+ }
+
+ *val = 0xFFFFFFFF;
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+}
+
+static int pnv_pci_write_config(struct pci_bus *bus,
+ unsigned int devfn,
+ int where, int size, u32 val)
+{
+ struct device_node *dn, *busdn = pci_bus_to_OF_node(bus);
+ struct pci_dn *pdn;
+
+ for (dn = busdn->child; dn; dn = dn->sibling) {
+ pdn = PCI_DN(dn);
+ if (pdn && pdn->devfn == devfn)
+ return pnv_pci_cfg_write(dn, where, size, val);
+ }
+
+ return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
struct pci_ops pnv_pci_ops = {
- .read = pnv_pci_read_config,
+ .read = pnv_pci_read_config,
.write = pnv_pci_write_config,
};
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 40bdf02..d633c64 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -182,6 +182,10 @@ extern struct pci_ops pnv_pci_ops;
extern struct pnv_eeh_ops ioda_eeh_ops;
#endif
+int pnv_pci_cfg_read(struct device_node *dn,
+ int where, int size, u32 *val);
+int pnv_pci_cfg_write(struct device_node *dn,
+ int where, int size, u32 val);
extern void pnv_pci_setup_iommu_table(struct iommu_table *tbl,
void *tce_mem, u64 tce_size,
u64 dma_offset);
--
1.7.5.4
^ permalink raw reply related
* [PATCH 4/8] powerpc/eeh: Fix address catch for PowerNV
From: Gavin Shan @ 2013-06-27 5:46 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Gavin Shan
In-Reply-To: <1372312009-13710-1-git-send-email-shangw@linux.vnet.ibm.com>
On the PowerNV platform, the EEH address cache isn't built correctly
because we skipped the EEH devices without binding PE. The patch
fixes that.
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
arch/powerpc/kernel/eeh_cache.c | 2 +-
arch/powerpc/platforms/powernv/pci-ioda.c | 1 +
2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
index 1d5d9a6..858ebea 100644
--- a/arch/powerpc/kernel/eeh_cache.c
+++ b/arch/powerpc/kernel/eeh_cache.c
@@ -194,7 +194,7 @@ static void __eeh_addr_cache_insert_dev(struct pci_dev *dev)
}
/* Skip any devices for which EEH is not enabled. */
- if (!edev->pe) {
+ if (!eeh_probe_mode_dev() && !edev->pe) {
#ifdef DEBUG
pr_info("PCI: skip building address cache for=%s - %s\n",
pci_name(dev), dn->full_name);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 3e5c3d5..0ff9a3a 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -998,6 +998,7 @@ static void pnv_pci_ioda_fixup(void)
pnv_pci_ioda_create_dbgfs();
#ifdef CONFIG_EEH
+ eeh_probe_mode_set(EEH_PROBE_MODE_DEV);
eeh_addr_cache_build();
eeh_init();
#endif
--
1.7.5.4
^ permalink raw reply related
* [PATCH 5/8] powerpc/eeh: Refactor the output message
From: Gavin Shan @ 2013-06-27 5:46 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Gavin Shan
In-Reply-To: <1372312009-13710-1-git-send-email-shangw@linux.vnet.ibm.com>
We needn't the the whole backtrace other than one-line message in
the error reporting interrupt handler. For errors triggered by
access PCI config space or MMIO, we replace "WARN(1, ...)" with
pr_err() and dump_stack(). The patch also adds more output messages
to indicate what EEH core is doing. Besides, some printk() are
replaced with pr_warning().
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
arch/powerpc/kernel/eeh.c | 9 +++++++--
arch/powerpc/kernel/eeh_driver.c | 23 ++++++++++++++++++-----
arch/powerpc/platforms/powernv/eeh-ioda.c | 25 ++++++++++++++++---------
3 files changed, 41 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 2dd0bd1..f7f2775 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -324,7 +324,9 @@ static int eeh_phb_check_failure(struct eeh_pe *pe)
eeh_serialize_unlock(flags);
eeh_send_failure_event(phb_pe);
- WARN(1, "EEH: PHB failure detected\n");
+ pr_err("EEH: PHB#%x failure detected\n",
+ phb_pe->phb->global_number);
+ dump_stack();
return 1;
out:
@@ -453,7 +455,10 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
* a stack trace will help the device-driver authors figure
* out what happened. So print that out.
*/
- WARN(1, "EEH: failure detected\n");
+ pr_err("EEH: Frozen PE#%x detected on PHB#%x\n",
+ pe->addr, pe->phb->global_number);
+ dump_stack();
+
return 1;
dn_unlock:
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 0974e13..2b1ce17 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -425,6 +425,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
* status ... if any child can't handle the reset, then the entire
* slot is dlpar removed and added.
*/
+ pr_info("EEH: Notify device drivers to shutdown\n");
eeh_pe_dev_traverse(pe, eeh_report_error, &result);
/* Get the current PCI slot state. This can take a long time,
@@ -432,7 +433,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
*/
rc = eeh_ops->wait_state(pe, MAX_WAIT_FOR_RECOVERY*1000);
if (rc < 0 || rc == EEH_STATE_NOT_SUPPORT) {
- printk(KERN_WARNING "EEH: Permanent failure\n");
+ pr_warning("EEH: Permanent failure\n");
goto hard_fail;
}
@@ -440,6 +441,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
* don't post the error log until after all dev drivers
* have been informed.
*/
+ pr_info("EEH: Collect temporary log\n");
eeh_slot_error_detail(pe, EEH_LOG_TEMP);
/* If all device drivers were EEH-unaware, then shut
@@ -447,15 +449,18 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
* go down willingly, without panicing the system.
*/
if (result == PCI_ERS_RESULT_NONE) {
+ pr_info("EEH: Reset with hotplug activity\n");
rc = eeh_reset_device(pe, frozen_bus);
if (rc) {
- printk(KERN_WARNING "EEH: Unable to reset, rc=%d\n", rc);
+ pr_warning("%s: Unable to reset, err=%d\n",
+ __func__, rc);
goto hard_fail;
}
}
/* If all devices reported they can proceed, then re-enable MMIO */
if (result == PCI_ERS_RESULT_CAN_RECOVER) {
+ pr_info("EEH: Enable I/O for affected devices\n");
rc = eeh_pci_enable(pe, EEH_OPT_THAW_MMIO);
if (rc < 0)
@@ -463,6 +468,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
if (rc) {
result = PCI_ERS_RESULT_NEED_RESET;
} else {
+ pr_info("EEH: Notify device drivers to resume I/O\n");
result = PCI_ERS_RESULT_NONE;
eeh_pe_dev_traverse(pe, eeh_report_mmio_enabled, &result);
}
@@ -470,6 +476,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
/* If all devices reported they can proceed, then re-enable DMA */
if (result == PCI_ERS_RESULT_CAN_RECOVER) {
+ pr_info("EEH: Enabled DMA for affected devices\n");
rc = eeh_pci_enable(pe, EEH_OPT_THAW_DMA);
if (rc < 0)
@@ -482,17 +489,22 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
/* If any device has a hard failure, then shut off everything. */
if (result == PCI_ERS_RESULT_DISCONNECT) {
- printk(KERN_WARNING "EEH: Device driver gave up\n");
+ pr_warning("EEH: Device driver gave up\n");
goto hard_fail;
}
/* If any device called out for a reset, then reset the slot */
if (result == PCI_ERS_RESULT_NEED_RESET) {
+ pr_info("EEH: Reset without hotplug activity\n");
rc = eeh_reset_device(pe, NULL);
if (rc) {
- printk(KERN_WARNING "EEH: Cannot reset, rc=%d\n", rc);
+ pr_warning("%s: Cannot reset, err=%d\n",
+ __func__, rc);
goto hard_fail;
}
+
+ pr_info("EEH: Notify device drivers "
+ "the completion of reset\n");
result = PCI_ERS_RESULT_NONE;
eeh_pe_dev_traverse(pe, eeh_report_reset, &result);
}
@@ -500,11 +512,12 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
/* All devices should claim they have recovered by now. */
if ((result != PCI_ERS_RESULT_RECOVERED) &&
(result != PCI_ERS_RESULT_NONE)) {
- printk(KERN_WARNING "EEH: Not recovered\n");
+ pr_warning("EEH: Not recovered\n");
goto hard_fail;
}
/* Tell all device drivers that they can resume operations */
+ pr_info("EEH: Notify device driver to resume\n");
eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
return;
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 85025d7..0cd1c4a 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -853,11 +853,14 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
phb->eeh_state |= PNV_EEH_STATE_REMOVED;
}
- WARN(1, "EEH: dead IOC detected\n");
+ pr_err("EEH: dead IOC detected\n");
ret = 4;
goto out;
- } else if (severity == OPAL_EEH_SEV_INF)
+ } else if (severity == OPAL_EEH_SEV_INF) {
+ pr_info("EEH: IOC informative error "
+ "detected\n");
ioda_eeh_hub_diag(hose);
+ }
break;
case OPAL_EEH_PHB_ERROR:
@@ -865,8 +868,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
if (ioda_eeh_get_phb_pe(hose, pe))
break;
- WARN(1, "EEH: dead PHB#%x detected\n",
- hose->global_number);
+ pr_err("EEH: dead PHB#%x detected\n",
+ hose->global_number);
phb->eeh_state |= PNV_EEH_STATE_REMOVED;
ret = 3;
goto out;
@@ -874,20 +877,24 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
if (ioda_eeh_get_phb_pe(hose, pe))
break;
- WARN(1, "EEH: fenced PHB#%x detected\n",
- hose->global_number);
+ pr_err("EEH: fenced PHB#%x detected\n",
+ hose->global_number);
ret = 2;
goto out;
- } else if (severity == OPAL_EEH_SEV_INF)
+ } else if (severity == OPAL_EEH_SEV_INF) {
+ pr_info("EEH: PHB#%x informative error "
+ "detected\n",
+ hose->global_number);
ioda_eeh_phb_diag(hose);
+ }
break;
case OPAL_EEH_PE_ERROR:
if (ioda_eeh_get_pe(hose, frozen_pe_no, pe))
break;
- WARN(1, "EEH: Frozen PE#%x on PHB#%x detected\n",
- (*pe)->addr, (*pe)->phb->global_number);
+ pr_err("EEH: Frozen PE#%x on PHB#%x detected\n",
+ (*pe)->addr, (*pe)->phb->global_number);
ret = 1;
goto out;
}
--
1.7.5.4
^ permalink raw reply related
* [PATCH 2/8] powerpc/eeh: Check PCIe link after reset
From: Gavin Shan @ 2013-06-27 5:46 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Gavin Shan
In-Reply-To: <1372312009-13710-1-git-send-email-shangw@linux.vnet.ibm.com>
After reset (e.g. complete reset) in order to bring the fenced PHB
back, the PCIe link might not be ready yet. The patch intends to
make sure the PCIe link is ready before accessing its subordinate
PCI devices. The patch also fixes that wrong values restored to
PCI_COMMAND register for PCI bridges.
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
arch/powerpc/kernel/eeh_pe.c | 157 ++++++++++++++++++++++++++++++++++++++----
1 files changed, 144 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c
index 55943fc..016588a 100644
--- a/arch/powerpc/kernel/eeh_pe.c
+++ b/arch/powerpc/kernel/eeh_pe.c
@@ -22,6 +22,7 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#include <linux/delay.h>
#include <linux/export.h>
#include <linux/gfp.h>
#include <linux/init.h>
@@ -567,30 +568,132 @@ void eeh_pe_state_clear(struct eeh_pe *pe, int state)
eeh_pe_traverse(pe, __eeh_pe_state_clear, &state);
}
-/**
- * eeh_restore_one_device_bars - Restore the Base Address Registers for one device
- * @data: EEH device
- * @flag: Unused
+/*
+ * Some PCI bridges (e.g. PLX bridges) have primary/secondary
+ * buses assigned explicitly by firmware, and we probably have
+ * lost that after reset. So we have to delay the check until
+ * the PCI-CFG registers have been restored for the parent
+ * bridge.
*
- * Loads the PCI configuration space base address registers,
- * the expansion ROM base address, the latency timer, and etc.
- * from the saved values in the device node.
+ * Don't use normal PCI-CFG accessors, which probably has been
+ * blocked on normal path during the stage. So we need utilize
+ * eeh operations, which is always permitted.
*/
-static void *eeh_restore_one_device_bars(void *data, void *flag)
+static void eeh_bridge_check_link(struct pci_dev *pdev,
+ struct device_node *dn)
+{
+ int cap;
+ uint32_t val;
+ int timeout = 0;
+
+ /*
+ * We only check root port and downstream ports of
+ * PCIe switches
+ */
+ if (!pci_is_pcie(pdev) ||
+ (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT &&
+ pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM))
+ return;
+
+ pr_debug("%s: Check PCIe link for %s ...\n",
+ __func__, pci_name(pdev));
+
+ /* Check slot status */
+ cap = pdev->pcie_cap;
+ eeh_ops->read_config(dn, cap + PCI_EXP_SLTSTA, 2, &val);
+ if (!(val & PCI_EXP_SLTSTA_PDS)) {
+ pr_debug(" No card in the slot (0x%04x) !\n", val);
+ return;
+ }
+
+ /* Check power status if we have the capability */
+ eeh_ops->read_config(dn, cap + PCI_EXP_SLTCAP, 2, &val);
+ if (val & PCI_EXP_SLTCAP_PCP) {
+ eeh_ops->read_config(dn, cap + PCI_EXP_SLTCTL, 2, &val);
+ if (val & PCI_EXP_SLTCTL_PCC) {
+ pr_debug(" In power-off state, power it on ...\n");
+ val &= ~(PCI_EXP_SLTCTL_PCC | PCI_EXP_SLTCTL_PIC);
+ val |= (0x0100 & PCI_EXP_SLTCTL_PIC);
+ eeh_ops->write_config(dn, cap + PCI_EXP_SLTCTL, 2, val);
+ msleep(2 * 1000);
+ }
+ }
+
+ /* Enable link */
+ eeh_ops->read_config(dn, cap + PCI_EXP_LNKCTL, 2, &val);
+ val &= ~PCI_EXP_LNKCTL_LD;
+ eeh_ops->write_config(dn, cap + PCI_EXP_LNKCTL, 2, val);
+
+ /* Check link */
+ eeh_ops->read_config(dn, cap + PCI_EXP_LNKCAP, 4, &val);
+ if (!(val & PCI_EXP_LNKCAP_DLLLARC)) {
+ pr_debug(" No link reporting capability (0x%08x) \n", val);
+ msleep(1000);
+ return;
+ }
+
+ /* Wait the link is up until timeout (5s) */
+ timeout = 0;
+ while (timeout < 5000) {
+ msleep(20);
+ timeout += 20;
+
+ eeh_ops->read_config(dn, cap + PCI_EXP_LNKSTA, 2, &val);
+ if (val & PCI_EXP_LNKSTA_DLLLA)
+ break;
+ }
+
+ if (val & PCI_EXP_LNKSTA_DLLLA)
+ pr_debug(" Link up (%s)\n",
+ (val & PCI_EXP_LNKSTA_CLS_2_5GB) ? "2.5GB" : "5GB");
+ else
+ pr_debug(" Link not ready (0x%04x)\n", val);
+}
+
+#define BYTE_SWAP(OFF) (8*((OFF)/4)+3-(OFF))
+#define SAVED_BYTE(OFF) (((u8 *)(edev->config_space))[BYTE_SWAP(OFF)])
+
+static void eeh_restore_bridge_bars(struct pci_dev *pdev,
+ struct eeh_dev *edev,
+ struct device_node *dn)
+{
+ int i;
+
+ /*
+ * Device BARs: 0x10 - 0x18
+ * Bus numbers and windows: 0x18 - 0x30
+ */
+ for (i = 4; i < 13; i++)
+ eeh_ops->write_config(dn, i*4, 4, edev->config_space[i]);
+ /* Rom: 0x38 */
+ eeh_ops->write_config(dn, 14*4, 4, edev->config_space[14]);
+
+ /* Cache line & Latency timer: 0xC 0xD */
+ eeh_ops->write_config(dn, PCI_CACHE_LINE_SIZE, 1,
+ SAVED_BYTE(PCI_CACHE_LINE_SIZE));
+ eeh_ops->write_config(dn, PCI_LATENCY_TIMER, 1,
+ SAVED_BYTE(PCI_LATENCY_TIMER));
+ /* Max latency, min grant, interrupt ping and line: 0x3C */
+ eeh_ops->write_config(dn, 15*4, 4, edev->config_space[15]);
+
+ /* PCI Command: 0x4 */
+ eeh_ops->write_config(dn, PCI_COMMAND, 4, edev->config_space[1]);
+
+ /* Check the PCIe link is ready */
+ eeh_bridge_check_link(pdev, dn);
+}
+
+static void eeh_restore_device_bars(struct eeh_dev *edev,
+ struct device_node *dn)
{
int i;
u32 cmd;
- struct eeh_dev *edev = (struct eeh_dev *)data;
- struct device_node *dn = eeh_dev_to_of_node(edev);
for (i = 4; i < 10; i++)
eeh_ops->write_config(dn, i*4, 4, edev->config_space[i]);
/* 12 == Expansion ROM Address */
eeh_ops->write_config(dn, 12*4, 4, edev->config_space[12]);
-#define BYTE_SWAP(OFF) (8*((OFF)/4)+3-(OFF))
-#define SAVED_BYTE(OFF) (((u8 *)(edev->config_space))[BYTE_SWAP(OFF)])
-
eeh_ops->write_config(dn, PCI_CACHE_LINE_SIZE, 1,
SAVED_BYTE(PCI_CACHE_LINE_SIZE));
eeh_ops->write_config(dn, PCI_LATENCY_TIMER, 1,
@@ -613,6 +716,34 @@ static void *eeh_restore_one_device_bars(void *data, void *flag)
else
cmd &= ~PCI_COMMAND_SERR;
eeh_ops->write_config(dn, PCI_COMMAND, 4, cmd);
+}
+
+/**
+ * eeh_restore_one_device_bars - Restore the Base Address Registers for one device
+ * @data: EEH device
+ * @flag: Unused
+ *
+ * Loads the PCI configuration space base address registers,
+ * the expansion ROM base address, the latency timer, and etc.
+ * from the saved values in the device node.
+ */
+static void *eeh_restore_one_device_bars(void *data, void *flag)
+{
+ struct pci_dev *pdev = NULL;
+ struct eeh_dev *edev = (struct eeh_dev *)data;
+ struct device_node *dn = eeh_dev_to_of_node(edev);
+
+ /* Trace the PCI bridge */
+ if (eeh_probe_mode_dev()) {
+ pdev = eeh_dev_to_pci_dev(edev);
+ if (pdev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
+ pdev = NULL;
+ }
+
+ if (pdev)
+ eeh_restore_bridge_bars(pdev, edev, dn);
+ else
+ eeh_restore_device_bars(edev, dn);
return NULL;
}
--
1.7.5.4
^ permalink raw reply related
* [PATCH 6/8] powerpc/eeh: Avoid build warnings
From: Gavin Shan @ 2013-06-27 5:46 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Gavin Shan
In-Reply-To: <1372312009-13710-1-git-send-email-shangw@linux.vnet.ibm.com>
The patch is for avoiding following build warnings:
The function .pnv_pci_ioda_fixup() references
the function __init .eeh_init().
This is often because .pnv_pci_ioda_fixup lacks a __init
The function .pnv_pci_ioda_fixup() references
the function __init .eeh_addr_cache_build().
This is often because .pnv_pci_ioda_fixup lacks a __init
Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/eeh.h | 4 ++--
arch/powerpc/kernel/eeh.c | 2 +-
arch/powerpc/kernel/eeh_cache.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index dd65e31..09a8743 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -202,13 +202,13 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe);
void *eeh_dev_init(struct device_node *dn, void *data);
void eeh_dev_phb_init_dynamic(struct pci_controller *phb);
-int __init eeh_init(void);
+int eeh_init(void);
int __init eeh_ops_register(struct eeh_ops *ops);
int __exit eeh_ops_unregister(const char *name);
unsigned long eeh_check_failure(const volatile void __iomem *token,
unsigned long val);
int eeh_dev_check_failure(struct eeh_dev *edev);
-void __init eeh_addr_cache_build(void);
+void eeh_addr_cache_build(void);
void eeh_add_device_tree_early(struct device_node *);
void eeh_add_device_tree_late(struct pci_bus *);
void eeh_add_sysfs_files(struct pci_bus *);
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index f7f2775..14475f6 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -751,7 +751,7 @@ int __exit eeh_ops_unregister(const char *name)
* Even if force-off is set, the EEH hardware is still enabled, so that
* newer systems can boot.
*/
-int __init eeh_init(void)
+int eeh_init(void)
{
struct pci_controller *hose, *tmp;
struct device_node *phb;
diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
index 858ebea..ea9a94c 100644
--- a/arch/powerpc/kernel/eeh_cache.c
+++ b/arch/powerpc/kernel/eeh_cache.c
@@ -285,7 +285,7 @@ void eeh_addr_cache_rmv_dev(struct pci_dev *dev)
* Must be run late in boot process, after the pci controllers
* have been scanned for devices (after all device resources are known).
*/
-void __init eeh_addr_cache_build(void)
+void eeh_addr_cache_build(void)
{
struct device_node *dn;
struct eeh_dev *edev;
--
1.7.5.4
^ permalink raw reply related
* Re: From: Gavin Shan <shangw@linux.vnet.ibm.com>
From: Gavin Shan @ 2013-06-27 5:51 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev
In-Reply-To: <1372312009-13710-1-git-send-email-shangw@linux.vnet.ibm.com>
On Thu, Jun 27, 2013 at 01:46:41PM +0800, Gavin Shan wrote:
The subject went wrong. I didn't make the format correct and here's
the makeup:
[PATCH v4 00/8] Follow-up fixes for EEH on PowerNV
>The series of patches are follow-up in order to make EEH workable for PowerNV
>platform on Juno-IOC-L machine. Couple of issues have been fixed with help of
>Ben:
>
> - Check PCIe link after PHB complete reset
> - Restore config space for bridges
> - The EEH address cache wasn't built successfully
> - Misc cleanup on output messages
> - Misc cleanup on EEH flags maintained by "struct pnv_phb"
> - Misc cleanup on properties of functions to avoid build warnings
> - Let PCI config accessors rely on device node
> - Do hotplug during reset for those devices whose drivers can't
> support EEH
>
>---
>
>Trigger frozen PE:
>
> echo 0x0000000002000000 > /sys/kernel/debug/powerpc/PCI0000/err_injct
> sleep 1
> echo 0x0 > /sys/kernel/debug/powerpc/PCI0000/err_injct
>
>Trigger fenced PHB:
>
> echo 0x8000000000000000 > /sys/kernel/debug/powerpc/PCI0000/err_injct
>
>
>---
>
>Changelog:
>==========
>v3 -> v4:
> * Add more output messages in EEH core to let users know what the EEH
> core is doing.
> * Add one patch to use device node in the PCI config accessors since
> the accessors used by EEH and it's not safe enough to refer PCI device
> and bus. We instead fully utilize the information from PCI_DN.
> * Add one patch to remove those deivces whose drivers can't support EEH
> before reset, and add them to the system after reset.
>v2 -> v3:
> * Fix overwritten buffer while collecting data from PCI config space.
>v1 -> v2:
> * Remove the mechanism to block PCI-CFG and MMIO.
> * Add one patch to do cleanup on output messages.
> * Add one patch to avoid build warnings.
> * Split functions to restore BARs for PCI devices and bridges separately.
>
>---
>
>arch/powerpc/include/asm/eeh.h | 8 +-
>arch/powerpc/include/asm/pci.h | 1 +
>arch/powerpc/kernel/eeh.c | 43 +++++--
>arch/powerpc/kernel/eeh_cache.c | 4 +-
>arch/powerpc/kernel/eeh_driver.c | 157 +++++++++++++++++++++++-
>arch/powerpc/kernel/eeh_pe.c | 166 ++++++++++++++++++++++++--
>arch/powerpc/kernel/pci_hotplug.c | 8 +-
>arch/powerpc/platforms/powernv/eeh-ioda.c | 33 +++--
>arch/powerpc/platforms/powernv/eeh-powernv.c | 44 +-------
>arch/powerpc/platforms/powernv/pci-ioda.c | 1 +
>arch/powerpc/platforms/powernv/pci.c | 124 ++++++++++++--------
>arch/powerpc/platforms/powernv/pci.h | 11 ++-
>drivers/pci/probe.c | 6 +-
>13 files changed, 462 insertions(+), 144 deletions(-)
>
>Thanks,
>Gavin
>
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev
>
^ permalink raw reply
* Re: [PATCH] powerpc: don't flush/invalidate the d/icache for an unknown relocation type
From: Suzuki K. Poulose @ 2013-06-27 6:36 UTC (permalink / raw)
To: Kevin Hao; +Cc: linuxppc
In-Reply-To: <1372295383-19740-1-git-send-email-haokexin@gmail.com>
On 06/27/2013 06:39 AM, Kevin Hao wrote:
> For an unknown relocation type since the value of r4 is just the 8bit
> relocation type, the sum of r4 and r7 may yield an invalid memory
> address. For example:
> In normal case:
> r4 = c00xxxxx
> r7 = 40000000
> r4 + r7 = 000xxxxx
>
> For an unknown relocation type:
> r4 = 000000xx
> r7 = 40000000
> r4 + r7 = 400000xx
> 400000xx is an invalid memory address for a board which has just
> 512M memory.
>
> And for operations such as dcbst or icbi may cause bus error for an
> invalid memory address on some platforms and then cause the board
> reset. So we should skip the flush/invalidate the d/icache for
> an unknown relocation type.
>
Good catch. Thanks for the fix.
Acked-by: Suzuki K. Poulose <suzuki@in.ibm.com>
^ permalink raw reply
* Re: [PATCH 3/8] vfio: add external user support
From: Stephen Rothwell @ 2013-06-27 6:47 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-doc, linux-kernel, kvm-ppc, Alexander Graf,
Alex Williamson, Paul Mackerras, Paul E . McKenney, linuxppc-dev,
David Gibson
In-Reply-To: <1372309356-28320-4-git-send-email-aik@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 960 bytes --]
Hi Alexy,
On Thu, 27 Jun 2013 15:02:31 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> index c488da5..54192b2 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1370,6 +1370,59 @@ static const struct file_operations vfio_device_fops = {
> };
>
> /**
> + * External user API, exported by symbols to be linked dynamically.
> + */
> +
> +/* Allows an external user (for example, KVM) to lock an IOMMU group */
> +static int vfio_group_add_external_user(struct file *filep)
> +{
> + struct vfio_group *group = filep->private_data;
> +
> + if (filep->f_op != &vfio_group_fops)
> + return -EINVAL;
> +
> + if (!atomic_inc_not_zero(&group->container_users))
> + return -EINVAL;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(vfio_group_add_external_user);
You cannot EXPORT a static symbol ... The same through the rest of the
file.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Srivatsa S. Bhat @ 2013-06-27 6:53 UTC (permalink / raw)
To: Tejun Heo
Cc: peterz, fweisbec, linux-kernel, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, Steven Rostedt, namhyung, tglx, laijs, zhong,
netdev, oleg, sbw, David Laight, akpm, linuxppc-dev
In-Reply-To: <20130626213458.GC4536@htj.dyndns.org>
On 06/27/2013 03:04 AM, Tejun Heo wrote:
> Hey,
>
> On Wed, Jun 26, 2013 at 11:58:48PM +0530, Srivatsa S. Bhat wrote:
>> Yes, we were discussing hot-unplug latency for use-cases such as
>> suspend/resume. We didn't want to make those operations slower in the
>> process of removing stop_machine() from hotplug.
>
> Can you please explain why tho?
Sure.
> How much would it help in terms of
> power-saving? Or are there other issues in taking longer to shut down
> cpus?
>
Basically, power-management techniques (like suspend/resume as used on
smartphones etc) are implemented using a controlling algorithm which
controls the aggressiveness of power-management depending on the load
on the system.
Today, the cpuidle subsystem in Linux handles cpuidle transitions using
that technique, so I'll explain using that as an example. Similar
strategies are used for suspend/resume also.
For every cpuidle state, we have atleast 2 parameters that determine
how usable it is - 1. exit latency 2. target residency.
The exit latency captures the amount of time it takes to come out of
the power-saving state, from the time you asked it to come out. This
is an estimate of how "deep" the power-saving state is. The deeper it
is, the longer it takes to come out. (And of course, deeper states give
higher power-savings).
The target residency is a value which represents the break-even for
that state. It tells us how long we should stay in that idle state
(at a minimum) before we ask it to come out, to actually get some good
power-savings out of that state. (Note that entry and exit to an idle
state itself consumes some power, so there won't be any power-savings
if we keep entering and exiting a deep state without staying in that
state long enough).
Once the idle states are characterized like this, the cpuidle subsystem
uses a cpuidle "governor" to actually decide which of the states to
enter, given an idle period of time. The governor keeps track of the
load on the system and predicts the length of the next idle period, and
based on that prediction, it selects the idle state whose characteristics
(exit latency/target residency) match the predicted idle time closely.
So as we can see, the "deeper" the idle state, the lower its chance of
getting used during regular workload runs, because it is deemed too costly.
So entry/exit latency of an idle state is a very important aspect which
determines how often you can use that state. Ideally we want states which
are "deep" in the sense that they give huge power-savings, but "light"
in the sense that their entry/exit latencies are low. Such states give
us the best benefits, since we can use them aggressively.
Now a similar argument applies for suspend/resume (on smartphones etc).
Suspend/resume already gives good power-savings. But if we make it slower,
the chances of using it profitably begins to reduce. That's why, IMHO,
its important to keep a check on the latency of CPU hotplug and reduce
it if possible. And that becomes even more important as systems keep
sporting more and more CPUs.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH 3/8] vfio: add external user support
From: Stephen Rothwell @ 2013-06-27 6:59 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-doc, linux-kernel, kvm-ppc, Alexander Graf,
Alex Williamson, Paul Mackerras, Paul E . McKenney, linuxppc-dev,
David Gibson
In-Reply-To: <1372309356-28320-4-git-send-email-aik@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 587 bytes --]
Hi Alexy,
On Thu, 27 Jun 2013 15:02:31 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> +/* Allows an external user (for example, KVM) to unlock an IOMMU group */
> +static void vfio_group_del_external_user(struct file *filep)
> +{
> + struct vfio_group *group = filep->private_data;
> +
> + BUG_ON(filep->f_op != &vfio_group_fops);
We usually reserve BUG_ON for situations where there is no way to
continue running or continuing will corrupt the running kernel. Maybe
WARN_ON() and return?
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 38/45] MIPS: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Srivatsa S. Bhat @ 2013-06-27 7:08 UTC (permalink / raw)
To: Ralf Baechle
Cc: linux-mips, David Daney, peterz, fweisbec, linux-kernel, walken,
mingo, linux-arch, vincent.guittot, xiaoguangrong, Steven J. Hill,
wangyun, paulmck, nikunj, linux-pm, rusty, rostedt, Yong Zhang,
namhyung, tglx, Florian Fainelli, John Crispin, laijs, zhong,
Sanjay Lal, netdev, oleg, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <20130626133912.GA4559@linux-mips.org>
On 06/26/2013 07:09 PM, Ralf Baechle wrote:
> On Wed, Jun 26, 2013 at 02:02:57AM +0530, Srivatsa S. Bhat wrote:
>
>> Once stop_machine() is gone from the CPU offline path, we won't be able
>> to depend on disabling preemption to prevent CPUs from going offline
>> from under us.
>>
>> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going
>> offline, while invoking from atomic context.
>
> I think the same change also needs to be applied to r4k_on_each_cpu() in
> arch/mips/mm/c-r4k.c which currently looks like:
>
> static inline void r4k_on_each_cpu(void (*func) (void *info), void *info)
> {
> preempt_disable();
>
> #if !defined(CONFIG_MIPS_MT_SMP) && !defined(CONFIG_MIPS_MT_SMTC)
> smp_call_function(func, info, 1);
> #endif
> func(info);
> preempt_enable();
> }
>
Thanks for pointing this out! I'll include changes to this code in my
next version.
Regards,
Srivatsa S. Bhat
> This is a slightly specialized version of on_each_cpu() which only calls
> out to other CPUs in actual multi-core environments and also - unlike
> on_each_cpu() doesn't disable interrupts for the sake of better
> interrupt latencies.
>
> Which reminds me ...
>
> Andrew, I was wondering why did 78eef01b0fae087c5fadbd85dd4fe2918c3a015f
> [[PATCH] on_each_cpu(): disable local interrupts] disable interrupts?
> The log is:
>
> ----- snip -----
> When on_each_cpu() runs the callback on other CPUs, it runs with local
> interrupts disabled. So we should run the function with local interrupts
> disabled on this CPU, too.
>
> And do the same for UP, so the callback is run in the same environment on bo
> UP and SMP. (strictly it should do preempt_disable() too, but I think
> local_irq_disable is sufficiently equivalent).
> [...]
> ----- snip -----
>
> I'm not entirely convinced the symmetry between UP and SMP environments is
> really worth it. Would anybody mind removing the local_irq_disable() ...
> local_irq_enable() from on_each_cpu()?
>
^ permalink raw reply
* [PATCH v2] vfio: add external user support
From: Alexey Kardashevskiy @ 2013-06-27 7:14 UTC (permalink / raw)
Cc: kvm, Alexey Kardashevskiy, linux-kernel, Alex Williamson,
Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <20130627164752.657c165ec4492e5248945a51@canb.auug.org.au>
VFIO is designed to be used via ioctls on file descriptors
returned by VFIO.
However in some situations support for an external user is required.
The first user is KVM on PPC64 (SPAPR TCE protocol) which is going to
use the existing VFIO groups for exclusive access in real/virtual mode
in the host kernel to avoid passing map/unmap requests to the user
space which would made things pretty slow.
The proposed protocol includes:
1. do normal VFIO init stuff such as opening a new container, attaching
group(s) to it, setting an IOMMU driver for a container. When IOMMU is
set for a container, all groups in it are considered ready to use by
an external user.
2. pass a fd of the group we want to accelerate to KVM. KVM calls
vfio_group_iommu_id_from_file() to verify if the group is initialized
and IOMMU is set for it. The current TCE IOMMU driver marks the whole
IOMMU table as busy when IOMMU is set for a container what this prevents
other DMA users from allocating from it so it is safe to pass the group
to the user space.
3. KVM increases the container users counter via
vfio_group_add_external_user(). This prevents the VFIO group from
being disposed prior to exiting KVM.
4. When KVM is finished and doing cleanup, it releases the group file
and decrements the container users counter. Everything gets released.
5. KVM also keeps the group file as otherwise its fd might have been
closed at the moment of KVM finish so vfio_group_del_external_user()
call will not be possible.
The "vfio: Limit group opens" patch is also required for the consistency.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
v1->v2: added definitions to vfio.h :)
Should not compile but compiled. Hm.
---
drivers/vfio/vfio.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/vfio.h | 7 +++++++
2 files changed, 61 insertions(+)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index c488da5..40875d2 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1370,6 +1370,60 @@ static const struct file_operations vfio_device_fops = {
};
/**
+ * External user API, exported by symbols to be linked dynamically.
+ */
+
+/* Allows an external user (for example, KVM) to lock an IOMMU group */
+int vfio_group_add_external_user(struct file *filep)
+{
+ struct vfio_group *group = filep->private_data;
+
+ if (filep->f_op != &vfio_group_fops)
+ return -EINVAL;
+
+ if (!atomic_inc_not_zero(&group->container_users))
+ return -EINVAL;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_group_add_external_user);
+
+/* Allows an external user (for example, KVM) to unlock an IOMMU group */
+void vfio_group_del_external_user(struct file *filep)
+{
+ struct vfio_group *group = filep->private_data;
+
+ if (WARN_ON(filep->f_op != &vfio_group_fops))
+ return;
+
+ vfio_group_try_dissolve_container(group);
+}
+EXPORT_SYMBOL_GPL(vfio_group_del_external_user);
+
+/*
+ * Checks if a group for the specified file can be used by
+ * an external user and returns the IOMMU ID if external use is possible.
+ */
+int vfio_group_iommu_id_from_file(struct file *filep)
+{
+ int ret;
+ struct vfio_group *group = filep->private_data;
+
+ if (WARN_ON(filep->f_op != &vfio_group_fops))
+ return -EINVAL;
+
+ if (0 == atomic_read(&group->container_users) ||
+ !group->container->iommu_driver ||
+ !vfio_group_viable(group))
+ return -EINVAL;
+
+ ret = iommu_group_id(group->iommu_group);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(vfio_group_iommu_id_from_file);
+
+/**
* Module/class support
*/
static char *vfio_devnode(struct device *dev, umode_t *mode)
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index ac8d488..7ee6575 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -90,4 +90,11 @@ extern void vfio_unregister_iommu_driver(
TYPE tmp; \
offsetof(TYPE, MEMBER) + sizeof(tmp.MEMBER); }) \
+/*
+ * External user API
+ */
+int vfio_group_add_external_user(struct file *filep);
+void vfio_group_del_external_user(struct file *filep);
+int vfio_group_iommu_id_from_file(struct file *filep);
+
#endif /* VFIO_H */
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH v2] vfio: add external user support
From: Stephen Rothwell @ 2013-06-27 7:50 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Alex Williamson, Paul Mackerras, linuxppc-dev,
David Gibson
In-Reply-To: <1372317260-6438-1-git-send-email-aik@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 815 bytes --]
Hi Alexy,
Thanks for the changes.
On Thu, 27 Jun 2013 17:14:20 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index ac8d488..7ee6575 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -90,4 +90,11 @@ extern void vfio_unregister_iommu_driver(
> TYPE tmp; \
> offsetof(TYPE, MEMBER) + sizeof(tmp.MEMBER); }) \
>
> +/*
> + * External user API
> + */
> +int vfio_group_add_external_user(struct file *filep);
> +void vfio_group_del_external_user(struct file *filep);
> +int vfio_group_iommu_id_from_file(struct file *filep);
Just being picky, but all the other function declarations in that file
state "extern" explicitly.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v2 0/3] Nvram-to-pstore: compression support for oops data
From: Aruna Balakrishnaiah @ 2013-06-27 8:32 UTC (permalink / raw)
To: tony.luck, keescook, benh, linux-kernel, linuxppc-dev, paulus
Cc: jkenisto, mahesh, cbouatmailru, anton, ccross
Changes from v1:
- Add header size argument in the pstore write callback
instead of a separate API to return header size.
The patchset takes care of compressing oops messages while writing to NVRAM,
so that more oops data can be captured in the given space.
big_oops_buf (2.22 * oops_data_sz) is allocated for compression.
oops_data_sz is oops header size less of oops partition size.
Pstore will internally call kmsg_dump to capture messages from printk
buffer. While returning the data to nvram it adds is own header.
For compression:
Register pstore with big_oops_buf.
In case compression fails, copy header added by pstore and
last oops_data_sz bytes (recent messages) of big_oops_buf to
nvram for which we need to know header size.
patch 01/03 adds an additional argument for header size in pstore_write callback.
pstore read callback of nvram will read the compressed data and return the
decompressed data so that dmesg file (under /dev/pstore) is readable.
In case decompression fails, instead of having the compressed data (junk) in the
dmesg file it will skip and continue reading other partitions. This results in
absence of dmesg file but will still have files relating to other parititons.
---
Aruna Balakrishnaiah (3):
Pass header size in the pstore write callback
powerpc/pseries: Re-organise the oops compression code
powerpc/pseries: Support compression of oops text via pstore
arch/powerpc/platforms/pseries/nvram.c | 239 +++++++++++++++++++++++---------
drivers/acpi/apei/erst.c | 4 -
drivers/firmware/efi/efi-pstore.c | 2
fs/pstore/platform.c | 10 +
fs/pstore/ram.c | 3
include/linux/pstore.h | 8 +
6 files changed, 187 insertions(+), 79 deletions(-)
--
^ permalink raw reply
* [PATCH v2 1/3] Pass header size in the pstore write callback
From: Aruna Balakrishnaiah @ 2013-06-27 8:32 UTC (permalink / raw)
To: tony.luck, keescook, benh, linux-kernel, linuxppc-dev, paulus
Cc: jkenisto, mahesh, cbouatmailru, anton, ccross
In-Reply-To: <20130627082941.16749.67023.stgit@aruna-ThinkPad-T420>
Header size is needed to distinguish between header and the dump data.
Incorporate the addition of new argument (hsize) in the pstore write
callback.
Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/nvram.c | 4 +++-
drivers/acpi/apei/erst.c | 4 ++--
drivers/firmware/efi/efi-pstore.c | 2 +-
fs/pstore/platform.c | 10 ++++++----
fs/pstore/ram.c | 3 ++-
include/linux/pstore.h | 8 ++++----
6 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index 14cc486..3f0e7d6 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -502,6 +502,7 @@ static int nvram_pstore_open(struct pstore_info *psi)
* @part: pstore writes data to registered buffer in parts,
* part number will indicate the same.
* @count: Indicates oops count
+ * @hsize: Size of header added by pstore
* @size: number of bytes written to the registered buffer
* @psi: registered pstore_info structure
*
@@ -512,7 +513,8 @@ static int nvram_pstore_open(struct pstore_info *psi)
static int nvram_pstore_write(enum pstore_type_id type,
enum kmsg_dump_reason reason,
u64 *id, unsigned int part, int count,
- size_t size, struct pstore_info *psi)
+ size_t hsize, size_t size,
+ struct pstore_info *psi)
{
int rc;
struct oops_log_info *oops_hdr = (struct oops_log_info *) oops_buf;
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 6d894bf..a9cf960 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -935,7 +935,7 @@ static ssize_t erst_reader(u64 *id, enum pstore_type_id *type, int *count,
struct timespec *time, char **buf,
struct pstore_info *psi);
static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
- u64 *id, unsigned int part, int count,
+ u64 *id, unsigned int part, int count, size_t hsize,
size_t size, struct pstore_info *psi);
static int erst_clearer(enum pstore_type_id type, u64 id, int count,
struct timespec time, struct pstore_info *psi);
@@ -1055,7 +1055,7 @@ out:
}
static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
- u64 *id, unsigned int part, int count,
+ u64 *id, unsigned int part, int count, size_t hsize,
size_t size, struct pstore_info *psi)
{
struct cper_pstore_record *rcd = (struct cper_pstore_record *)
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 202d2c8..452800e0 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -104,7 +104,7 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
static int efi_pstore_write(enum pstore_type_id type,
enum kmsg_dump_reason reason, u64 *id,
- unsigned int part, int count, size_t size,
+ unsigned int part, int count, size_t hsize, size_t size,
struct pstore_info *psi)
{
char name[DUMP_NAME_LEN];
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 86d1038..4637ec4 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -159,7 +159,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
break;
ret = psinfo->write(PSTORE_TYPE_DMESG, reason, &id, part,
- oopscount, hsize + len, psinfo);
+ oopscount, hsize, hsize + len, psinfo);
if (ret == 0 && reason == KMSG_DUMP_OOPS && pstore_is_mounted())
pstore_new_entry = 1;
@@ -196,7 +196,7 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
spin_lock_irqsave(&psinfo->buf_lock, flags);
}
memcpy(psinfo->buf, s, c);
- psinfo->write(PSTORE_TYPE_CONSOLE, 0, &id, 0, 0, c, psinfo);
+ psinfo->write(PSTORE_TYPE_CONSOLE, 0, &id, 0, 0, 0, c, psinfo);
spin_unlock_irqrestore(&psinfo->buf_lock, flags);
s += c;
c = e - s;
@@ -221,9 +221,11 @@ static void pstore_register_console(void) {}
static int pstore_write_compat(enum pstore_type_id type,
enum kmsg_dump_reason reason,
u64 *id, unsigned int part, int count,
- size_t size, struct pstore_info *psi)
+ size_t hsize, size_t size,
+ struct pstore_info *psi)
{
- return psi->write_buf(type, reason, id, part, psinfo->buf, size, psi);
+ return psi->write_buf(type, reason, id, part, psinfo->buf, hsize,
+ size, psi);
}
/*
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1376e5a..c6bb77c 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -195,7 +195,8 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz)
static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
enum kmsg_dump_reason reason,
u64 *id, unsigned int part,
- const char *buf, size_t size,
+ const char *buf,
+ size_t hsize, size_t size,
struct pstore_info *psi)
{
struct ramoops_context *cxt = psi->data;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 656699f..4aa80ba 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -58,12 +58,12 @@ struct pstore_info {
struct pstore_info *psi);
int (*write)(enum pstore_type_id type,
enum kmsg_dump_reason reason, u64 *id,
- unsigned int part, int count, size_t size,
- struct pstore_info *psi);
+ unsigned int part, int count, size_t hsize,
+ size_t size, struct pstore_info *psi);
int (*write_buf)(enum pstore_type_id type,
enum kmsg_dump_reason reason, u64 *id,
- unsigned int part, const char *buf, size_t size,
- struct pstore_info *psi);
+ unsigned int part, const char *buf, size_t hsize,
+ size_t size, struct pstore_info *psi);
int (*erase)(enum pstore_type_id type, u64 id,
int count, struct timespec time,
struct pstore_info *psi);
^ permalink raw reply related
* [PATCH v2 2/3] powerpc/pseries: Re-organise the oops compression code
From: Aruna Balakrishnaiah @ 2013-06-27 8:33 UTC (permalink / raw)
To: tony.luck, keescook, benh, linux-kernel, linuxppc-dev, paulus
Cc: jkenisto, mahesh, cbouatmailru, anton, ccross
In-Reply-To: <20130627082941.16749.67023.stgit@aruna-ThinkPad-T420>
nvram_compress() and zip_oops() is used by the nvram_pstore_write
API to compress oops messages hence re-organise the functions
accordingly to avoid forward declarations.
Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/nvram.c | 104 ++++++++++++++++----------------
1 file changed, 52 insertions(+), 52 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index 3f0e7d6..588bab5 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -486,6 +486,58 @@ static int clobbering_unread_rtas_event(void)
NVRAM_RTAS_READ_TIMEOUT);
}
+/* Derived from logfs_compress() */
+static int nvram_compress(const void *in, void *out, size_t inlen,
+ size_t outlen)
+{
+ int err, ret;
+
+ ret = -EIO;
+ err = zlib_deflateInit2(&stream, COMPR_LEVEL, Z_DEFLATED, WINDOW_BITS,
+ MEM_LEVEL, Z_DEFAULT_STRATEGY);
+ if (err != Z_OK)
+ goto error;
+
+ stream.next_in = in;
+ stream.avail_in = inlen;
+ stream.total_in = 0;
+ stream.next_out = out;
+ stream.avail_out = outlen;
+ stream.total_out = 0;
+
+ err = zlib_deflate(&stream, Z_FINISH);
+ if (err != Z_STREAM_END)
+ goto error;
+
+ err = zlib_deflateEnd(&stream);
+ if (err != Z_OK)
+ goto error;
+
+ if (stream.total_out >= stream.total_in)
+ goto error;
+
+ ret = stream.total_out;
+error:
+ return ret;
+}
+
+/* Compress the text from big_oops_buf into oops_buf. */
+static int zip_oops(size_t text_len)
+{
+ struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf;
+ int zipped_len = nvram_compress(big_oops_buf, oops_data, text_len,
+ oops_data_sz);
+ if (zipped_len < 0) {
+ pr_err("nvram: compression failed; returned %d\n", zipped_len);
+ pr_err("nvram: logging uncompressed oops/panic report\n");
+ return -1;
+ }
+ oops_hdr->version = OOPS_HDR_VERSION;
+ oops_hdr->report_length = (u16) zipped_len;
+ oops_hdr->timestamp = get_seconds();
+ return 0;
+}
+
#ifdef CONFIG_PSTORE
static int nvram_pstore_open(struct pstore_info *psi)
{
@@ -759,58 +811,6 @@ int __init pSeries_nvram_init(void)
}
-/* Derived from logfs_compress() */
-static int nvram_compress(const void *in, void *out, size_t inlen,
- size_t outlen)
-{
- int err, ret;
-
- ret = -EIO;
- err = zlib_deflateInit2(&stream, COMPR_LEVEL, Z_DEFLATED, WINDOW_BITS,
- MEM_LEVEL, Z_DEFAULT_STRATEGY);
- if (err != Z_OK)
- goto error;
-
- stream.next_in = in;
- stream.avail_in = inlen;
- stream.total_in = 0;
- stream.next_out = out;
- stream.avail_out = outlen;
- stream.total_out = 0;
-
- err = zlib_deflate(&stream, Z_FINISH);
- if (err != Z_STREAM_END)
- goto error;
-
- err = zlib_deflateEnd(&stream);
- if (err != Z_OK)
- goto error;
-
- if (stream.total_out >= stream.total_in)
- goto error;
-
- ret = stream.total_out;
-error:
- return ret;
-}
-
-/* Compress the text from big_oops_buf into oops_buf. */
-static int zip_oops(size_t text_len)
-{
- struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf;
- int zipped_len = nvram_compress(big_oops_buf, oops_data, text_len,
- oops_data_sz);
- if (zipped_len < 0) {
- pr_err("nvram: compression failed; returned %d\n", zipped_len);
- pr_err("nvram: logging uncompressed oops/panic report\n");
- return -1;
- }
- oops_hdr->version = OOPS_HDR_VERSION;
- oops_hdr->report_length = (u16) zipped_len;
- oops_hdr->timestamp = get_seconds();
- return 0;
-}
-
/*
* This is our kmsg_dump callback, called after an oops or panic report
* has been written to the printk buffer. We want to capture as much
^ permalink raw reply related
* [PATCH v2 3/3] powerpc/pseries: Support compression of oops text via pstore
From: Aruna Balakrishnaiah @ 2013-06-27 8:33 UTC (permalink / raw)
To: tony.luck, keescook, benh, linux-kernel, linuxppc-dev, paulus
Cc: jkenisto, mahesh, cbouatmailru, anton, ccross
In-Reply-To: <20130627082941.16749.67023.stgit@aruna-ThinkPad-T420>
The patch set supports compression of oops messages while writing to NVRAM,
this helps in capturing more of oops data to lnx,oops-log. The pstore file
for oops messages will be in decompressed format making it readable.
In case compression fails, the patch takes care of copying the header added
by pstore and last oops_data_sz bytes of big_oops_buf to NVRAM so that we
have recent oops messages in lnx,oops-log.
In case decompression fails, it will result in absence of oops file but still
have files (in /dev/pstore) for other partitions.
Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/nvram.c | 131 +++++++++++++++++++++++++++++---
1 file changed, 117 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/nvram.c b/arch/powerpc/platforms/pseries/nvram.c
index 588bab5..9f8671a 100644
--- a/arch/powerpc/platforms/pseries/nvram.c
+++ b/arch/powerpc/platforms/pseries/nvram.c
@@ -539,6 +539,65 @@ static int zip_oops(size_t text_len)
}
#ifdef CONFIG_PSTORE
+/* Derived from logfs_uncompress */
+int nvram_decompress(void *in, void *out, size_t inlen, size_t outlen)
+{
+ int err, ret;
+
+ ret = -EIO;
+ err = zlib_inflateInit(&stream);
+ if (err != Z_OK)
+ goto error;
+
+ stream.next_in = in;
+ stream.avail_in = inlen;
+ stream.total_in = 0;
+ stream.next_out = out;
+ stream.avail_out = outlen;
+ stream.total_out = 0;
+
+ err = zlib_inflate(&stream, Z_FINISH);
+ if (err != Z_STREAM_END)
+ goto error;
+
+ err = zlib_inflateEnd(&stream);
+ if (err != Z_OK)
+ goto error;
+
+ ret = stream.total_out;
+error:
+ return ret;
+}
+
+static int unzip_oops(char *oops_buf, char *big_buf)
+{
+ struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf;
+ u64 timestamp = oops_hdr->timestamp;
+ char *big_oops_data = NULL;
+ char *oops_data_buf = NULL;
+ size_t big_oops_data_sz;
+ int unzipped_len;
+
+ big_oops_data = big_buf + sizeof(struct oops_log_info);
+ big_oops_data_sz = big_oops_buf_sz - sizeof(struct oops_log_info);
+ oops_data_buf = oops_buf + sizeof(struct oops_log_info);
+
+ unzipped_len = nvram_decompress(oops_data_buf, big_oops_data,
+ oops_hdr->report_length,
+ big_oops_data_sz);
+
+ if (unzipped_len < 0) {
+ pr_err("nvram: decompression failed; returned %d\n",
+ unzipped_len);
+ return -1;
+ }
+ oops_hdr = (struct oops_log_info *)big_buf;
+ oops_hdr->version = OOPS_HDR_VERSION;
+ oops_hdr->report_length = (u16) unzipped_len;
+ oops_hdr->timestamp = timestamp;
+ return 0;
+}
+
static int nvram_pstore_open(struct pstore_info *psi)
{
/* Reset the iterator to start reading partitions again */
@@ -569,6 +628,7 @@ static int nvram_pstore_write(enum pstore_type_id type,
struct pstore_info *psi)
{
int rc;
+ unsigned int err_type = ERR_TYPE_KERNEL_PANIC;
struct oops_log_info *oops_hdr = (struct oops_log_info *) oops_buf;
/* part 1 has the recent messages from printk buffer */
@@ -579,8 +639,30 @@ static int nvram_pstore_write(enum pstore_type_id type,
oops_hdr->version = OOPS_HDR_VERSION;
oops_hdr->report_length = (u16) size;
oops_hdr->timestamp = get_seconds();
+
+ if (big_oops_buf) {
+ rc = zip_oops(size);
+ /*
+ * If compression fails copy recent log messages from
+ * big_oops_buf to oops_data.
+ */
+ if (rc != 0) {
+ size_t diff = size - oops_data_sz + hsize;
+
+ if (size > oops_data_sz) {
+ memcpy(oops_data, big_oops_buf, hsize);
+ memcpy(oops_data + hsize, big_oops_buf + diff,
+ oops_data_sz - hsize);
+
+ oops_hdr->report_length = (u16) oops_data_sz;
+ } else
+ memcpy(oops_data, big_oops_buf, size);
+ } else
+ err_type = ERR_TYPE_KERNEL_PANIC_GZ;
+ }
+
rc = nvram_write_os_partition(&oops_log_partition, oops_buf,
- (int) (sizeof(*oops_hdr) + size), ERR_TYPE_KERNEL_PANIC,
+ (int) (sizeof(*oops_hdr) + oops_hdr->report_length), err_type,
count);
if (rc != 0)
@@ -602,10 +684,11 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
struct oops_log_info *oops_hdr;
unsigned int err_type, id_no, size = 0;
struct nvram_os_partition *part = NULL;
- char *buff = NULL;
- int sig = 0;
+ char *buff = NULL, *big_buff = NULL;
+ int rc, sig = 0;
loff_t p;
+read_partition:
read_type++;
switch (nvram_type_ids[read_type]) {
@@ -668,6 +751,25 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
if (nvram_type_ids[read_type] == PSTORE_TYPE_DMESG) {
oops_hdr = (struct oops_log_info *)buff;
*buf = buff + sizeof(*oops_hdr);
+
+ if (err_type == ERR_TYPE_KERNEL_PANIC_GZ) {
+ big_buff = kmalloc(big_oops_buf_sz, GFP_KERNEL);
+ if (!big_buff)
+ return -ENOMEM;
+
+ rc = unzip_oops(buff, big_buff);
+
+ if (rc != 0) {
+ kfree(buff);
+ kfree(big_buff);
+ goto read_partition;
+ }
+
+ oops_hdr = (struct oops_log_info *)big_buff;
+ *buf = big_buff + sizeof(*oops_hdr);
+ kfree(buff);
+ }
+
time->tv_sec = oops_hdr->timestamp;
time->tv_nsec = 0;
return oops_hdr->report_length;
@@ -689,17 +791,18 @@ static int nvram_pstore_init(void)
{
int rc = 0;
- nvram_pstore_info.buf = oops_data;
- nvram_pstore_info.bufsize = oops_data_sz;
+ if (big_oops_buf) {
+ nvram_pstore_info.buf = big_oops_buf;
+ nvram_pstore_info.bufsize = big_oops_buf_sz;
+ } else {
+ nvram_pstore_info.buf = oops_data;
+ nvram_pstore_info.bufsize = oops_data_sz;
+ }
rc = pstore_register(&nvram_pstore_info);
if (rc != 0)
pr_err("nvram: pstore_register() failed, defaults to "
"kmsg_dump; returned %d\n", rc);
- else
- /*TODO: Support compression when pstore is configured */
- pr_info("nvram: Compression of oops text supported only when "
- "pstore is not configured");
return rc;
}
@@ -733,11 +836,6 @@ static void __init nvram_init_oops_partition(int rtas_partition_exists)
oops_data = oops_buf + sizeof(struct oops_log_info);
oops_data_sz = oops_log_partition.size - sizeof(struct oops_log_info);
- rc = nvram_pstore_init();
-
- if (!rc)
- return;
-
/*
* Figure compression (preceded by elimination of each line's <n>
* severity prefix) will reduce the oops/panic report to at most
@@ -761,6 +859,11 @@ static void __init nvram_init_oops_partition(int rtas_partition_exists)
stream.workspace = NULL;
}
+ rc = nvram_pstore_init();
+
+ if (!rc)
+ return;
+
rc = kmsg_dump_register(&nvram_kmsg_dumper);
if (rc != 0) {
pr_err("nvram: kmsg_dump_register() failed; returned %d\n", rc);
^ permalink raw reply related
* RE: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: David Laight @ 2013-06-27 8:54 UTC (permalink / raw)
To: Srivatsa S. Bhat, Tejun Heo
Cc: peterz, fweisbec, oleg, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, Steven Rostedt, namhyung, tglx, laijs, zhong,
netdev, linux-kernel, sbw, akpm, linuxppc-dev
In-Reply-To: <51CB3159.8000509@linux.vnet.ibm.com>
> >>> It would also increase the latency of CPU-hotunplug operations.
> >>
> >> Is that a big deal?
> >
> > I thought that was the whole deal with this patchset - making cpu
> > hotunplugs lighter and faster mostly for powersaving. That said, =
just
> > removing stop_machine call would be a pretty good deal and I don't
> > know how meaningful reducing CPU hotunplug latency is. Srivatsa?
> >
>=20
> Keeping the hotunplug latency is important for suspend/resume, where
> we take all non-boot CPUs in a loop. That's an interesting use-case
> where intrusiveness doesn't matter much, but latency does. So yes,
> making CPU hotplug faster is also one of the goals of this patchset.
If you are removing all but one of the cpu, the you only need
one rcu cycle (remove everything from the list first).
I'd also guess that you can't suspend a cpu until you can sleep
the process that is running on it - so if a process has pre-emption
disabled you aren't going to complete suspend until the process
sleeps (this wouldn't be true if you suspended the cpu with its
current stack - but if suspend is removing the non-boot cpus first
it must be doing so from the scheduler idle loop).
If you are doing suspend for aggressive power saving, then all the
processes (and processors) will already be idle. However you
probably wouldn't want the memory accesses to determine this on
a large NUMA system with 1024+ processors.
David
^ permalink raw reply
* Re: [PATCH v3] powerpc: Rework iommu_table locks
From: Benjamin Herrenschmidt @ 2013-06-27 9:39 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Paul Mackerras, linuxppc-dev, Anton Blanchard, linux-kernel,
David Gibson
In-Reply-To: <1372308796-27796-1-git-send-email-aik@ozlabs.ru>
On Thu, 2013-06-27 at 14:53 +1000, Alexey Kardashevskiy wrote:
>
> 2. remove locks from functions being called by VFIO. The whole table
> is given to the user space so it is responsible now for races.
Sure but you still need to be careful that userspace cannot cause things
that crash the kernel. For example, look careful at what could happen if
two user space threads try to manipulate the same TCE entry at the same
time and whether that can cause a deadly kernel race... something tells
me it can.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 3/3] powerpc/pseries: Support compression of oops text via pstore
From: Aruna Balakrishnaiah @ 2013-06-27 9:42 UTC (permalink / raw)
To: Luck, Tony
Cc: jkenisto@linux.vnet.ibm.com, Kees Cook, mahesh@linux.vnet.ibm.com,
Colin Cross, LKML, linuxppc-dev@ozlabs.org, paulus@samba.org,
Anton Blanchard, Anton Vorontsov
In-Reply-To: <3908561D78D1C84285E8C5FCA982C28F31C5376F@ORSMSX106.amr.corp.intel.com>
Hi Tony,
On Tuesday 25 June 2013 09:32 PM, Luck, Tony wrote:
>> Introducing headersize in pstore_write() API would need changes at
>> multiple places whereits being called. The idea is to move the
>> compression support to pstore infrastructure so that other platforms
>> could also make use of it.
> Any thoughts on the back/forward compatibility as we switch to compressed
> pstore data? E.g. imagine I have a system installed with some Linux distribution
> with a kernel too old to know about compressed pstore. I use that machine to
> run the latest kernels that do compression ... and one fine day one of them crashes
> hard - logging in compressed form to pstore. Now I boot my distro kernel to pick
> up the pieces ... what do I see in /sys/fs/pstore/*? Some compressed files? Can I
> read them with some tool?
>
> This somewhat of a corner case - but not completely unrealistic ... I'd at least
> like to be reassured that the old kernel won't choke when it sees the compressed
> blobs.
openssl command line tool can be used to decompress the compressed data of
the pstore file in the above scenario.
Usage:
cat <file> | openssl zlib -d
> -Tony
>
^ permalink raw reply
* Re: [PATCH 3/8] vfio: add external user support
From: Benjamin Herrenschmidt @ 2013-06-27 9:42 UTC (permalink / raw)
To: Stephen Rothwell
Cc: kvm, linux-doc, Alexey Kardashevskiy, Alexander Graf, kvm-ppc,
linux-kernel, Alex Williamson, Paul Mackerras, Paul E . McKenney,
linuxppc-dev, David Gibson
In-Reply-To: <20130627165929.3828c261c957845a549ad95b@canb.auug.org.au>
On Thu, 2013-06-27 at 16:59 +1000, Stephen Rothwell wrote:
> > +/* Allows an external user (for example, KVM) to unlock an IOMMU
> group */
> > +static void vfio_group_del_external_user(struct file *filep)
> > +{
> > + struct vfio_group *group = filep->private_data;
> > +
> > + BUG_ON(filep->f_op != &vfio_group_fops);
>
> We usually reserve BUG_ON for situations where there is no way to
> continue running or continuing will corrupt the running kernel. Maybe
> WARN_ON() and return?
Not even that. This is a user space provided "fd", we shouldn't oops the
kernel because we passed a wrong argument, just return -EINVAL or
something like that (add a return code).
Ben.
^ permalink raw reply
* Re: [PATCH v2 15/45] rcu: Use get/put_online_cpus_atomic() to prevent CPU offline
From: Srivatsa S. Bhat @ 2013-06-27 10:06 UTC (permalink / raw)
To: David Laight
Cc: peterz, fweisbec, linux-kernel, walken, mingo, linux-arch,
vincent.guittot, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, Steven Rostedt, namhyung, tglx, laijs, zhong,
netdev, oleg, sbw, Tejun Heo, akpm, linuxppc-dev
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B72AB@saturn3.aculab.com>
On 06/27/2013 02:24 PM, David Laight wrote:
>>>>> It would also increase the latency of CPU-hotunplug operations.
>>>>
>>>> Is that a big deal?
>>>
>>> I thought that was the whole deal with this patchset - making cpu
>>> hotunplugs lighter and faster mostly for powersaving. That said, just
>>> removing stop_machine call would be a pretty good deal and I don't
>>> know how meaningful reducing CPU hotunplug latency is. Srivatsa?
>>>
>>
>> Keeping the hotunplug latency is important for suspend/resume, where
>> we take all non-boot CPUs in a loop. That's an interesting use-case
>> where intrusiveness doesn't matter much, but latency does. So yes,
>> making CPU hotplug faster is also one of the goals of this patchset.
>
> If you are removing all but one of the cpu, the you only need
> one rcu cycle (remove everything from the list first).
>
Hmm, yeah, but IIRC, back when we discussed this last time[1], we felt that
would make the code a little bit hard to understand. But I think we can
give it a shot to see how that goes and decide based on that. So thanks
for bringing that up again!
BTW, one thing I'd like to emphasize again here is that we will not use
the RCU-like concept to have 2 different masks - a stable online mask
and an actual online mask (this is one of the approaches that we had
discussed earlier[2]). The reason why we don't wanna go down that path is,
its hard to determine who can survive by just looking at the stable online
mask, and who needs to be aware of the actual online mask. That will
surely lead to more bugs and headache.
So the use of an RCU-like concept here would only be to ensure that all
preempt-disabled sections complete, and we can switch the synchronization
scheme to global rwlocks, like what we had proposed earlier[3]. So, that
still requires call-sites to be converted from preempt_disable() to
get/put_online_cpus_atomic().
I just wanted to clarify where exactly the RCU concept would fit in,
in the stop-machine() replacement scheme...
> I'd also guess that you can't suspend a cpu until you can sleep
> the process that is running on it - so if a process has pre-emption
> disabled you aren't going to complete suspend until the process
> sleeps (this wouldn't be true if you suspended the cpu with its
> current stack - but if suspend is removing the non-boot cpus first
> it must be doing so from the scheduler idle loop).
>
> If you are doing suspend for aggressive power saving, then all the
> processes (and processors) will already be idle. However you
> probably wouldn't want the memory accesses to determine this on
> a large NUMA system with 1024+ processors.
>
References:
[1]. http://lkml.indiana.edu/hypermail/linux/kernel/1212.2/01979.html
[2]. http://thread.gmane.org/gmane.linux.kernel/1405145/focus=29336
[3]. http://thread.gmane.org/gmane.linux.documentation/9520/focus=1443258
http://thread.gmane.org/gmane.linux.power-management.general/29464/focus=1407948
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH 3/8] vfio: add external user support
From: Alexey Kardashevskiy @ 2013-06-27 10:48 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, kvm, linux-doc, Alexander Graf, kvm-ppc,
linux-kernel, Alex Williamson, Paul Mackerras, Paul E . McKenney,
linuxppc-dev, David Gibson
In-Reply-To: <1372326166.18612.42.camel@pasglop>
On 06/27/2013 07:42 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-06-27 at 16:59 +1000, Stephen Rothwell wrote:
>>> +/* Allows an external user (for example, KVM) to unlock an IOMMU
>> group */
>>> +static void vfio_group_del_external_user(struct file *filep)
>>> +{
>>> + struct vfio_group *group = filep->private_data;
>>> +
>>> + BUG_ON(filep->f_op != &vfio_group_fops);
>>
>> We usually reserve BUG_ON for situations where there is no way to
>> continue running or continuing will corrupt the running kernel. Maybe
>> WARN_ON() and return?
>
> Not even that. This is a user space provided "fd", we shouldn't oops the
> kernel because we passed a wrong argument, just return -EINVAL or
> something like that (add a return code).
I'll change to WARN_ON but...
This is going to be called on KVM exit on a file pointer previously
verified for correctness. If it is a wrong file*, then something went
terribly wrong.
--
Alexey
^ permalink raw reply
* Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
From: David Gibson @ 2013-06-27 11:01 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm@vger.kernel.org mailing list, Alexey Kardashevskiy,
Joerg Roedel, Rusty Russell, Alexander Graf, kvm-ppc, open list,
Paul Mackerras, linuxppc-dev
In-Reply-To: <1372048884.30572.168.camel@ul30vt.home>
[-- Attachment #1: Type: text/plain, Size: 4030 bytes --]
On Sun, Jun 23, 2013 at 10:41:24PM -0600, Alex Williamson wrote:
> On Mon, 2013-06-24 at 13:52 +1000, David Gibson wrote:
> > On Sat, Jun 22, 2013 at 08:28:06AM -0600, Alex Williamson wrote:
> > > On Sat, 2013-06-22 at 22:03 +1000, David Gibson wrote:
> > > > On Thu, Jun 20, 2013 at 08:55:13AM -0600, Alex Williamson wrote:
> > > > > On Thu, 2013-06-20 at 18:48 +1000, Alexey Kardashevskiy wrote:
> > > > > > On 06/20/2013 05:47 PM, Benjamin Herrenschmidt wrote:
> > > > > > > On Thu, 2013-06-20 at 15:28 +1000, David Gibson wrote:
> > > > > > >>> Just out of curiosity - would not get_file() and fput_atomic() on a
> > > > > > >> group's
> > > > > > >>> file* do the right job instead of vfio_group_add_external_user() and
> > > > > > >>> vfio_group_del_external_user()?
> > > > > > >>
> > > > > > >> I was thinking that too. Grabbing a file reference would certainly be
> > > > > > >> the usual way of handling this sort of thing.
> > > > > > >
> > > > > > > But that wouldn't prevent the group ownership to be returned to
> > > > > > > the kernel or another user would it ?
> > > > > >
> > > > > >
> > > > > > Holding the file pointer does not let the group->container_users counter go
> > > > > > to zero
> > > > >
> > > > > How so? Holding the file pointer means the file won't go away, which
> > > > > means the group release function won't be called. That means the group
> > > > > won't go away, but that doesn't mean it's attached to an IOMMU. A user
> > > > > could call UNSET_CONTAINER.
> > > >
> > > > Uhh... *thinks*. Ah, I see.
> > > >
> > > > I think the interface should not take the group fd, but the container
> > > > fd. Holding a reference to *that* would keep the necessary things
> > > > around. But more to the point, it's the right thing semantically:
> > > >
> > > > The container is essentially the handle on a host iommu address space,
> > > > and so that's what should be bound by the KVM call to a particular
> > > > guest iommu address space. e.g. it would make no sense to bind two
> > > > different groups to different guest iommu address spaces, if they were
> > > > in the same container - the guest thinks they are different spaces,
> > > > but if they're in the same container they must be the same space.
> > >
> > > While the container is the gateway to the iommu, what empowers the
> > > container to maintain an iommu is the group. What happens to a
> > > container when all the groups are disconnected or closed? Groups are
> > > the unit that indicates hardware access, not containers. Thanks,
> >
> > Uh... huh? I'm really not sure what you're getting at.
> >
> > The operation we're doing for KVM here is binding a guest iommu
> > address space to a particular host iommu address space. Why would we
> > not want to use the obvious handle on the host iommu address space,
> > which is the container fd?
>
> AIUI, the request isn't for an interface through which to do iommu
> mappings. The request is for an interface to show that the user has
> sufficient privileges to do mappings. Groups are what gives the user
> that ability. The iommu is also possibly associated with multiple iommu
> groups and I believe what is being asked for here is a way to hold and
> lock a single iommu group with iommu protection.
>
> >From a practical point of view, the iommu interface is de-privileged
> once the groups are disconnected or closed. Holding a reference count
> on the iommu fd won't prevent that. That means we'd have to use a
> notifier to have KVM stop the side-channel iommu access. Meanwhile
> holding the file descriptor for the group and adding an interface that
> bumps use counter allows KVM to lock itself in, just as if it had a
> device opened itself. Thanks,
Ah, good point.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
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