Some ACPI IO accessing need to be done in atomic context. For example,
APEI ERST operations may be used for permanent storage in hardware
error handler. That is, it may be called in atomic contexts such as
IRQ or NMI, etc. And, ERST/EINJ implement their operations via IO
memory/port accessing. But the IO memory accessing method provided by
ACPI (acpi_read/acpi_write) maps the IO memory during it is accessed,
so it can not be used in atomic context. To solve the issue, the IO
memory should be pre-mapped during EINJ/ERST initializing. A linked
list is used to record which memory area has been mapped, when memory
is accessed in hardware error handler, search the linked list for the
mapped virtual address from the given physical address.
Signed-off-by: Huang Ying <[email protected]>
---
drivers/acpi/Makefile | 2
drivers/acpi/atomicio.c | 392 ++++++++++++++++++++++++++++++++++++++++++++++++
include/acpi/atomicio.h | 10 +
3 files changed, 403 insertions(+), 1 deletion(-)
--- /dev/null
+++ b/drivers/acpi/atomicio.c
@@ -0,0 +1,392 @@
+/*
+ * atomicio.c - ACPI IO memory pre-mapping/post-unmapping, then
+ * accessing in atomic context.
+ *
+ * This is used for NMI handler to access IO memory area, because
+ * ioremap/iounmap can not be used in NMI handler. The IO memory area
+ * is pre-mapped in process context and accessed in NMI handler.
+ *
+ * Copyright (C) 2009, Intel Corp.
+ * Author: Huang Ying <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/kref.h>
+#include <linux/rculist.h>
+#include <linux/interrupt.h>
+#include <acpi/atomicio.h>
+
+static LIST_HEAD(acpi_iomaps);
+/*
+ * Used for mutual exclusion between writers of acpi_iomaps list, for
+ * synchronization between readers and writer, RCU is used.
+ */
+static DEFINE_SPINLOCK(acpi_iomaps_lock);
+
+struct acpi_iomap {
+ struct list_head list;
+ void __iomem *vaddr;
+ unsigned long size;
+ phys_addr_t paddr;
+ struct kref ref;
+};
+
+/* acpi_iomaps_lock or RCU read lock must be held before calling */
+static struct acpi_iomap *__acpi_find_iomap(phys_addr_t paddr,
+ unsigned long size)
+{
+ struct acpi_iomap *map;
+
+ list_for_each_entry_rcu(map, &acpi_iomaps, list) {
+ if (map->paddr + map->size >= paddr + size &&
+ map->paddr <= paddr)
+ return map;
+ }
+ return NULL;
+}
+
+/*
+ * Atomic "ioremap" used by NMI handler, if the specified IO memory
+ * area is not pre-mapped, NULL will be returned.
+ *
+ * acpi_iomaps_lock or RCU read lock must be held before calling
+ */
+static void __iomem *__acpi_ioremap_fast(phys_addr_t paddr,
+ unsigned long size)
+{
+ struct acpi_iomap *map;
+
+ map = __acpi_find_iomap(paddr, size);
+ if (map)
+ return map->vaddr + (paddr - map->paddr);
+ else
+ return NULL;
+}
+
+/* acpi_iomaps_lock must be held before calling */
+static void __iomem *__acpi_try_ioremap(phys_addr_t paddr,
+ unsigned long size)
+{
+ struct acpi_iomap *map;
+
+ map = __acpi_find_iomap(paddr, size);
+ if (map) {
+ kref_get(&map->ref);
+ return map->vaddr + (paddr - map->paddr);
+ } else
+ return NULL;
+}
+
+/*
+ * Used to pre-map the specified IO memory area. First try to find
+ * whether the area is already pre-mapped, if it is, increase the
+ * reference count (in __acpi_try_ioremap) and return; otherwise, do
+ * the real ioremap, and add the mapping into acpi_iomaps list.
+ */
+static void __iomem *acpi_pre_map(phys_addr_t paddr,
+ unsigned long size)
+{
+ void __iomem *vaddr;
+ struct acpi_iomap *map;
+ unsigned long pg_sz, flags;
+ phys_addr_t pg_off;
+
+ spin_lock_irqsave(&acpi_iomaps_lock, flags);
+ vaddr = __acpi_try_ioremap(paddr, size);
+ spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
+ if (vaddr)
+ return vaddr;
+
+ pg_off = paddr & PAGE_MASK;
+ pg_sz = ((paddr + size + PAGE_SIZE - 1) & PAGE_MASK) - pg_off;
+ vaddr = ioremap(pg_off, pg_sz);
+ if (!vaddr)
+ return NULL;
+ map = kmalloc(sizeof(*map), GFP_KERNEL);
+ if (!map)
+ goto err_unmap;
+ INIT_LIST_HEAD(&map->list);
+ map->paddr = pg_off;
+ map->size = pg_sz;
+ map->vaddr = vaddr;
+ kref_init(&map->ref);
+
+ spin_lock_irqsave(&acpi_iomaps_lock, flags);
+ vaddr = __acpi_try_ioremap(paddr, size);
+ if (vaddr) {
+ spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
+ iounmap(map->vaddr);
+ kfree(map);
+ return vaddr;
+ }
+ list_add_tail_rcu(&map->list, &acpi_iomaps);
+ spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
+
+ return vaddr + (paddr - pg_off);
+err_unmap:
+ iounmap(vaddr);
+ return NULL;
+}
+
+/* acpi_iomaps_lock must be held before calling */
+static void __acpi_kref_del_iomap(struct kref *ref)
+{
+ struct acpi_iomap *map;
+
+ map = container_of(ref, struct acpi_iomap, ref);
+ list_del_rcu(&map->list);
+}
+
+/*
+ * Used to post-unmap the specified IO memory area. The iounmap is
+ * done only if the reference count goes zero.
+ */
+static void acpi_post_unmap(phys_addr_t paddr, unsigned long size)
+{
+ struct acpi_iomap *map;
+ unsigned long flags;
+ int del;
+
+ spin_lock_irqsave(&acpi_iomaps_lock, flags);
+ map = __acpi_find_iomap(paddr, size);
+ BUG_ON(!map);
+ del = kref_put(&map->ref, __acpi_kref_del_iomap);
+ spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
+
+ if (!del)
+ return;
+
+ synchronize_rcu();
+ iounmap(map->vaddr);
+ kfree(map);
+}
+
+/* In NMI handler, should set silent = 1 */
+static int acpi_check_gar(struct acpi_generic_address *reg,
+ u64 *paddr, int silent)
+{
+ u32 width;
+
+ /* Handle possible alignment issues */
+ memcpy(paddr, ®->address, sizeof(*paddr));
+ if (!*paddr) {
+ if (!silent)
+ pr_info(
+ "Invalid physical address in GAR, firmware bug?\n");
+ return -EINVAL;
+ }
+
+ width = reg->bit_width;
+ if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
+ if (!silent)
+ pr_info(
+ "Invalid bit width in GAR, firmware bug?\n");
+ return -EINVAL;
+ }
+
+ if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
+ reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+ if (!silent)
+ pr_info(
+ "Invalid address space type in GAR, firmware bug?\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* Pre-map, working on GAR */
+int acpi_pre_map_gar(struct acpi_generic_address *reg)
+{
+ u64 paddr;
+ void __iomem *vaddr;
+ int rc;
+
+ if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
+ return 0;
+
+ rc = acpi_check_gar(reg, &paddr, 0);
+ if (rc)
+ return rc;
+
+ vaddr = acpi_pre_map(paddr, reg->bit_width / 8);
+ if (!vaddr)
+ return -EIO;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_pre_map_gar);
+
+/* Post-unmap, working on GAR */
+int acpi_post_unmap_gar(struct acpi_generic_address *reg)
+{
+ u64 paddr;
+ int rc;
+
+ if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
+ return 0;
+
+ rc = acpi_check_gar(reg, &paddr, 0);
+ if (rc)
+ return rc;
+
+ acpi_post_unmap(paddr, reg->bit_width / 8);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_post_unmap_gar);
+
+/*
+ * Can be used in atomic (including NMI) or process context. RCU read
+ * lock can only be released after the IO memory area accessing.
+ */
+static int acpi_atomic_read_mem(u64 paddr, u64 *val, u32 width)
+{
+ void __iomem *addr;
+
+ rcu_read_lock();
+ addr = __acpi_ioremap_fast(paddr, width);
+ switch (width) {
+ case 8:
+ *val = readb(addr);
+ break;
+ case 16:
+ *val = readw(addr);
+ break;
+ case 32:
+ *val = readl(addr);
+ break;
+ case 64:
+ *val = readq(addr);
+ break;
+ default:
+ return -EINVAL;
+ }
+ rcu_read_unlock();
+
+ return 0;
+}
+
+static int acpi_atomic_write_mem(u64 paddr, u64 val, u32 width)
+{
+ void __iomem *addr;
+
+ rcu_read_lock();
+ addr = __acpi_ioremap_fast(paddr, width);
+ switch (width) {
+ case 8:
+ writeb(val, addr);
+ break;
+ case 16:
+ writew(val, addr);
+ break;
+ case 32:
+ writel(val, addr);
+ break;
+ case 64:
+ writeq(val, addr);
+ break;
+ default:
+ return -EINVAL;
+ }
+ rcu_read_unlock();
+
+ return 0;
+}
+
+static int acpi_atomic_read_port(u64 port, u32 *val, u32 width)
+{
+ switch (width) {
+ case 8:
+ *val = inb(port);
+ break;
+ case 16:
+ *val = inw(port);
+ break;
+ case 32:
+ *val = inl(port);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int acpi_atomic_write_port(u64 port, u32 val, u32 width)
+{
+ switch (width) {
+ case 8:
+ outb(val, port);
+ break;
+ case 16:
+ outw(val, port);
+ break;
+ case 32:
+ outl(val, port);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* GAR accessing in atomic (including NMI) or process context */
+int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg)
+{
+ u64 paddr;
+ int rc;
+
+ rc = acpi_check_gar(reg, &paddr, 1);
+ if (rc)
+ return rc;
+
+ *val = 0;
+ switch (reg->space_id) {
+ case ACPI_ADR_SPACE_SYSTEM_MEMORY:
+ return acpi_atomic_read_mem(paddr, val, reg->bit_width);
+ case ACPI_ADR_SPACE_SYSTEM_IO:
+ return acpi_atomic_read_port(paddr, (u32 *)val, reg->bit_width);
+ default:
+ return -EINVAL;
+ }
+}
+EXPORT_SYMBOL_GPL(acpi_atomic_read);
+
+int acpi_atomic_write(u64 val, struct acpi_generic_address *reg)
+{
+ u64 paddr;
+ int rc;
+
+ rc = acpi_check_gar(reg, &paddr, 1);
+ if (rc)
+ return rc;
+
+ switch (reg->space_id) {
+ case ACPI_ADR_SPACE_SYSTEM_MEMORY:
+ return acpi_atomic_write_mem(paddr, val, reg->bit_width);
+ case ACPI_ADR_SPACE_SYSTEM_IO:
+ return acpi_atomic_write_port(paddr, val, reg->bit_width);
+ default:
+ return -EINVAL;
+ }
+}
+EXPORT_SYMBOL_GPL(acpi_atomic_write);
--- /dev/null
+++ b/include/acpi/atomicio.h
@@ -0,0 +1,10 @@
+#ifndef ACPI_ATOMIC_IO_H
+#define ACPI_ATOMIC_IO_H
+
+int acpi_pre_map_gar(struct acpi_generic_address *reg);
+int acpi_post_unmap_gar(struct acpi_generic_address *reg);
+
+int acpi_atomic_read(u64 *val, struct acpi_generic_address *reg);
+int acpi_atomic_write(u64 val, struct acpi_generic_address *reg);
+
+#endif
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -18,7 +18,7 @@ obj-y += acpi.o \
acpica/
# All the builtin files are in the "acpi." module_param namespace.
-acpi-y += osl.o utils.o reboot.o
+acpi-y += osl.o utils.o reboot.o atomicio.o
# sleep related files
acpi-y += wakeup.o
I see you posted a first version of this series a couple days
ago, but there weren't any responses (at least on linux-acpi),
and you didn't say anything about what you changed between
-v1 and -v2.
On Thursday 10 December 2009 12:16:53 am Huang Ying wrote:
> Some ACPI IO accessing need to be done in atomic context. For example,
> APEI ERST operations may be used for permanent storage in hardware
> error handler. That is, it may be called in atomic contexts such as
> IRQ or NMI, etc. And, ERST/EINJ implement their operations via IO
> memory/port accessing. But the IO memory accessing method provided by
> ACPI (acpi_read/acpi_write) maps the IO memory during it is accessed,
> so it can not be used in atomic context. To solve the issue, the IO
> memory should be pre-mapped during EINJ/ERST initializing. A linked
> list is used to record which memory area has been mapped, when memory
> is accessed in hardware error handler, search the linked list for the
> mapped virtual address from the given physical address.
The ACPI CA has functions called acpi_hw_read() and acpi_hw_write()
that have similar prototypes and functionality (but of course, they
don't work in atomic context). It'd be nice if your new functions
had similar names, e.g., acpi_hw_map(), acpi_hw_unmap(),
acpi_hw_read_atomic(), acpi_hw_write_atomic().
I think your code would be simpler if acpi_pre_map_gar() returned a
struct acpi_iomap pointer (from the caller's point of view, this would
be an opaque cookie). Then you could just supply that cookie to
acpi_atomic_write(), and you wouldn't have to look it up again. Maybe
you could even get rid of the list and all the fancy RCU & kref stuff
then, too.
> +/* In NMI handler, should set silent = 1 */
> +static int acpi_check_gar(struct acpi_generic_address *reg,
> + u64 *paddr, int silent)
> +{
> + u32 width;
> +
> + /* Handle possible alignment issues */
> + memcpy(paddr, ®->address, sizeof(*paddr));
> + if (!*paddr) {
> + if (!silent)
> + pr_info(
> + "Invalid physical address in GAR, firmware bug?\n");
> + return -EINVAL;
> + }
> +
> + width = reg->bit_width;
> + if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> + if (!silent)
> + pr_info(
> + "Invalid bit width in GAR, firmware bug?\n");
> + return -EINVAL;
> + }
> +
> + if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> + reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> + if (!silent)
> + pr_info(
> + "Invalid address space type in GAR, firmware bug?\n");
Error messages with constant text are nearly useless because they
don't give much of a clue about where to look for a problem.
Personally, for something this, I would just return failure and
never print anything. If a map fails, the caller should notice
and you then have a good idea of where to look.
> +static int acpi_atomic_read_port(u64 port, u32 *val, u32 width)
> +{
> + switch (width) {
> + case 8:
> + *val = inb(port);
> + break;
> + case 16:
> + *val = inw(port);
> + break;
> + case 32:
> + *val = inl(port);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Can you use acpi_os_read_port() and acpi_os_write_port() instead of
duplicating this code?
> +static int acpi_atomic_write_port(u64 port, u32 val, u32 width)
> +{
> + switch (width) {
> + case 8:
> + outb(val, port);
> + break;
> + case 16:
> + outw(val, port);
> + break;
> + case 32:
> + outl(val, port);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Bjorn
On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote:
> I see you posted a first version of this series a couple days
> ago, but there weren't any responses (at least on linux-acpi),
> and you didn't say anything about what you changed between
> -v1 and -v2.
The only change between -v1 and -v2 is that atomic ACPI read/write is
separated from drivers/acpi/apei/apei-base.c to drivers/acpi/atomicio.c
and in a separated patch.
> On Thursday 10 December 2009 12:16:53 am Huang Ying wrote:
> > Some ACPI IO accessing need to be done in atomic context. For example,
> > APEI ERST operations may be used for permanent storage in hardware
> > error handler. That is, it may be called in atomic contexts such as
> > IRQ or NMI, etc. And, ERST/EINJ implement their operations via IO
> > memory/port accessing. But the IO memory accessing method provided by
> > ACPI (acpi_read/acpi_write) maps the IO memory during it is accessed,
> > so it can not be used in atomic context. To solve the issue, the IO
> > memory should be pre-mapped during EINJ/ERST initializing. A linked
> > list is used to record which memory area has been mapped, when memory
> > is accessed in hardware error handler, search the linked list for the
> > mapped virtual address from the given physical address.
>
> The ACPI CA has functions called acpi_hw_read() and acpi_hw_write()
> that have similar prototypes and functionality (but of course, they
> don't work in atomic context). It'd be nice if your new functions
> had similar names, e.g., acpi_hw_map(), acpi_hw_unmap(),
> acpi_hw_read_atomic(), acpi_hw_write_atomic().
acpi_hw_read/write is the 32-bit optimized version of acpi_read/write.
So I think it is better to follow the naming convention of
acpi_read/write.
> I think your code would be simpler if acpi_pre_map_gar() returned a
> struct acpi_iomap pointer (from the caller's point of view, this would
> be an opaque cookie). Then you could just supply that cookie to
> acpi_atomic_write(), and you wouldn't have to look it up again. Maybe
> you could even get rid of the list and all the fancy RCU & kref stuff
> then, too.
The interface chosen is based on usage model, which is:
1. In init function, all GARs needed are pre-mapped
2. In atomic context, pre-mapped GARs are accessed
3. In exit function, all GARs are post-unmapped
In 3), if struct acpi_iomap* is used as parameter for post-unmap
function, we need to record that pointer in another list. In 2), we need
find mapped address from GAR.
> > +/* In NMI handler, should set silent = 1 */
> > +static int acpi_check_gar(struct acpi_generic_address *reg,
> > + u64 *paddr, int silent)
> > +{
> > + u32 width;
> > +
> > + /* Handle possible alignment issues */
> > + memcpy(paddr, ®->address, sizeof(*paddr));
> > + if (!*paddr) {
> > + if (!silent)
> > + pr_info(
> > + "Invalid physical address in GAR, firmware bug?\n");
> > + return -EINVAL;
> > + }
> > +
> > + width = reg->bit_width;
> > + if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> > + if (!silent)
> > + pr_info(
> > + "Invalid bit width in GAR, firmware bug?\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> > + reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> > + if (!silent)
> > + pr_info(
> > + "Invalid address space type in GAR, firmware bug?\n");
>
> Error messages with constant text are nearly useless because they
> don't give much of a clue about where to look for a problem.
> Personally, for something this, I would just return failure and
> never print anything. If a map fails, the caller should notice
> and you then have a good idea of where to look.
The checking here is for bug in firmware not software. I think it is
necessary for the user to know where the bugs may come from, and it is
hard to express the bug in return code.
> > +static int acpi_atomic_read_port(u64 port, u32 *val, u32 width)
> > +{
> > + switch (width) {
> > + case 8:
> > + *val = inb(port);
> > + break;
> > + case 16:
> > + *val = inw(port);
> > + break;
> > + case 32:
> > + *val = inl(port);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
>
> Can you use acpi_os_read_port() and acpi_os_write_port() instead of
> duplicating this code?
Yes. You are right. I will use that.
Thanks,
Huang Ying
On Monday 14 December 2009 06:04:13 pm Huang Ying wrote:
> On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote:
> > The ACPI CA has functions called acpi_hw_read() and acpi_hw_write()
> > that have similar prototypes and functionality (but of course, they
> > don't work in atomic context). It'd be nice if your new functions
> > had similar names, e.g., acpi_hw_map(), acpi_hw_unmap(),
> > acpi_hw_read_atomic(), acpi_hw_write_atomic().
>
> acpi_hw_read/write is the 32-bit optimized version of acpi_read/write.
> So I think it is better to follow the naming convention of
> acpi_read/write.
You're right; I didn't notice that. I agree that it's better to
follow the convention of acpi_read().
> > I think your code would be simpler if acpi_pre_map_gar() returned a
> > struct acpi_iomap pointer (from the caller's point of view, this would
> > be an opaque cookie). Then you could just supply that cookie to
> > acpi_atomic_write(), and you wouldn't have to look it up again. Maybe
> > you could even get rid of the list and all the fancy RCU & kref stuff
> > then, too.
>
> The interface chosen is based on usage model, which is:
>
> 1. In init function, all GARs needed are pre-mapped
> 2. In atomic context, pre-mapped GARs are accessed
> 3. In exit function, all GARs are post-unmapped
>
> In 3), if struct acpi_iomap* is used as parameter for post-unmap
> function, we need to record that pointer in another list. In 2), we need
> find mapped address from GAR.
I understand that my proposal would require a slight change in your
usage model. I am suggesting that you make it follow the same pattern
as pci_iomap(), e.g.:
void *pci_iomap(struct pci_dev *, int bar, unsigned long len);
unsigned int ioread32(void *);
void pci_iounmap(struct pci_dev *, void *);
void *acpi_map_generic_address(struct acpi_generic_address *);
u64 acpi_read_atomic(void *);
void acpi_unmap_generic_address(void *);
acpi_map_generic_address() would validate the GAR and do the ioremap().
If the validation or ioremap() failed, it would return a NULL pointer.
This would require the caller of acpi_map_generic_address() to hang onto
the pointer returned (just as callers of pci_iomap() must hang onto the
pointer it returns).
The pointer would be supplied to acpi_read_atomic(), so it would not
need to do acpi_check_gar() because we know it was done successfully
in acpi_map_generic_address(). It wouldn't need to look up the GAR
in the list because the list entry was passed in. Since all the
possible failure conditions were checked in acpi_map_generic_address(),
acpi_read_atomic() doesn't need to return status and could simply
return the u64 directly.
> > > +/* In NMI handler, should set silent = 1 */
> > > +static int acpi_check_gar(struct acpi_generic_address *reg,
> > > + u64 *paddr, int silent)
> > > +{
> > > + u32 width;
> > > +
> > > + /* Handle possible alignment issues */
> > > + memcpy(paddr, ®->address, sizeof(*paddr));
> > > + if (!*paddr) {
> > > + if (!silent)
> > > + pr_info(
> > > + "Invalid physical address in GAR, firmware bug?\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + width = reg->bit_width;
> > > + if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> > > + if (!silent)
> > > + pr_info(
> > > + "Invalid bit width in GAR, firmware bug?\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> > > + reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> > > + if (!silent)
> > > + pr_info(
> > > + "Invalid address space type in GAR, firmware bug?\n");
> >
> > Error messages with constant text are nearly useless because they
> > don't give much of a clue about where to look for a problem.
> > Personally, for something this, I would just return failure and
> > never print anything. If a map fails, the caller should notice
> > and you then have a good idea of where to look.
>
> The checking here is for bug in firmware not software. I think it is
> necessary for the user to know where the bugs may come from, and it is
> hard to express the bug in return code.
Yes, I understand that this is checking for firmware bugs. My point
is that when a user sees this in his dmesg log:
Invalid bit width in GAR, firmware bug?
we have no context, and even a kernel developer can't figure out what
to do. We could ask for a copy of the FADT and DSDT, but even then,
we don't know *which* GAR structure to look at, so we'll probably have
to add some instrumentation and ask the user to reproduce the problem.
If the check were in the caller, it could at least say something like:
ACPI: couldn't map generic address [io 0xcf8] for PCI config access
which would give us more useful information.
I guess your assumption is that *both* the caller and acpi_check_gar()
would print something, so then we'd know which GAR was bad and also
exactly what was wrong with it. My opinion (and this is just my opinion)
is that knowing with GAR is bad is the important part. A GAR is simple
enough that if we know which one to look at, the problem will be obvious,
so we only need the message from the caller.
Bjorn
On Wed, 2009-12-16 at 01:47 +0800, Bjorn Helgaas wrote:
> On Monday 14 December 2009 06:04:13 pm Huang Ying wrote:
> > On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote:
> > > I think your code would be simpler if acpi_pre_map_gar() returned a
> > > struct acpi_iomap pointer (from the caller's point of view, this would
> > > be an opaque cookie). Then you could just supply that cookie to
> > > acpi_atomic_write(), and you wouldn't have to look it up again. Maybe
> > > you could even get rid of the list and all the fancy RCU & kref stuff
> > > then, too.
> >
> > The interface chosen is based on usage model, which is:
> >
> > 1. In init function, all GARs needed are pre-mapped
> > 2. In atomic context, pre-mapped GARs are accessed
> > 3. In exit function, all GARs are post-unmapped
> >
> > In 3), if struct acpi_iomap* is used as parameter for post-unmap
> > function, we need to record that pointer in another list. In 2), we need
> > find mapped address from GAR.
>
> I understand that my proposal would require a slight change in your
> usage model. I am suggesting that you make it follow the same pattern
> as pci_iomap(), e.g.:
>
> void *pci_iomap(struct pci_dev *, int bar, unsigned long len);
> unsigned int ioread32(void *);
> void pci_iounmap(struct pci_dev *, void *);
>
> void *acpi_map_generic_address(struct acpi_generic_address *);
> u64 acpi_read_atomic(void *);
> void acpi_unmap_generic_address(void *);
>
> acpi_map_generic_address() would validate the GAR and do the ioremap().
> If the validation or ioremap() failed, it would return a NULL pointer.
>
> This would require the caller of acpi_map_generic_address() to hang onto
> the pointer returned (just as callers of pci_iomap() must hang onto the
> pointer it returns).
>
> The pointer would be supplied to acpi_read_atomic(), so it would not
> need to do acpi_check_gar() because we know it was done successfully
> in acpi_map_generic_address(). It wouldn't need to look up the GAR
> in the list because the list entry was passed in. Since all the
> possible failure conditions were checked in acpi_map_generic_address(),
> acpi_read_atomic() doesn't need to return status and could simply
> return the u64 directly.
The usage model is different. Please take a look at
__apei_exec_read_register(). We need to get virtual address from
physical address in GAR. And many small-size GARs in ERST or EINJ may
share same page, so some kind of virtual space optimization is
necessary.
> > > > +/* In NMI handler, should set silent = 1 */
> > > > +static int acpi_check_gar(struct acpi_generic_address *reg,
> > > > + u64 *paddr, int silent)
> > > > +{
> > > > + u32 width;
> > > > +
> > > > + /* Handle possible alignment issues */
> > > > + memcpy(paddr, ®->address, sizeof(*paddr));
> > > > + if (!*paddr) {
> > > > + if (!silent)
> > > > + pr_info(
> > > > + "Invalid physical address in GAR, firmware bug?\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + width = reg->bit_width;
> > > > + if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> > > > + if (!silent)
> > > > + pr_info(
> > > > + "Invalid bit width in GAR, firmware bug?\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> > > > + reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> > > > + if (!silent)
> > > > + pr_info(
> > > > + "Invalid address space type in GAR, firmware bug?\n");
> > >
> > > Error messages with constant text are nearly useless because they
> > > don't give much of a clue about where to look for a problem.
> > > Personally, for something this, I would just return failure and
> > > never print anything. If a map fails, the caller should notice
> > > and you then have a good idea of where to look.
> >
> > The checking here is for bug in firmware not software. I think it is
> > necessary for the user to know where the bugs may come from, and it is
> > hard to express the bug in return code.
>
> Yes, I understand that this is checking for firmware bugs. My point
> is that when a user sees this in his dmesg log:
>
> Invalid bit width in GAR, firmware bug?
>
> we have no context, and even a kernel developer can't figure out what
> to do. We could ask for a copy of the FADT and DSDT, but even then,
> we don't know *which* GAR structure to look at, so we'll probably have
> to add some instrumentation and ask the user to reproduce the problem.
>
> If the check were in the caller, it could at least say something like:
>
> ACPI: couldn't map generic address [io 0xcf8] for PCI config access
>
> which would give us more useful information.
En... Yes, some information about the invalid GAR is helpful. But the
GAR information is available in acpi_check_gar too, so we can output
something as follow in acpi_check_gar:
Invalid bit width in GAR [mem 0x8029ff8 24], firmware bug?
Your message looks like a software issue instead of firmware bug. This
may confuse the user and developer.
Best Regards,
Huang Ying
Please use git send-email for sending a patch series,
or some other tool that will properly format the Subject line,
and result in the series being properly threaded in my e-mail reader
rather than being a collection of unrelated messages.
As these patches were sent as independent e-mails
and had a typo in the manaully typed in subject line,
I didn't find 1/5 when I was looking at 2/5.
(and these should be preceeded with 0/5, per previous feedback)
thanks,
Len Brown, Intel Open Source Technology Center
On Wed, 2009-12-16 at 13:38 +0800, Len Brown wrote:
> Please use git send-email for sending a patch series,
> or some other tool that will properly format the Subject line,
> and result in the series being properly threaded in my e-mail reader
> rather than being a collection of unrelated messages.
>
> As these patches were sent as independent e-mails
> and had a typo in the manaully typed in subject line,
> I didn't find 1/5 when I was looking at 2/5.
> (and these should be preceeded with 0/5, per previous feedback)
OK. I will send with git send-email in following versions.
Best Regards,
Huang Ying
On Tuesday 15 December 2009 06:23:15 pm Huang Ying wrote:
> On Wed, 2009-12-16 at 01:47 +0800, Bjorn Helgaas wrote:
> > On Monday 14 December 2009 06:04:13 pm Huang Ying wrote:
> > > On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote:
> > > > I think your code would be simpler if acpi_pre_map_gar() returned a
> > > > struct acpi_iomap pointer (from the caller's point of view, this would
> > > > be an opaque cookie). Then you could just supply that cookie to
> > > > acpi_atomic_write(), and you wouldn't have to look it up again. Maybe
> > > > you could even get rid of the list and all the fancy RCU & kref stuff
> > > > then, too.
> > >
> > > The interface chosen is based on usage model, which is:
> > >
> > > 1. In init function, all GARs needed are pre-mapped
> > > 2. In atomic context, pre-mapped GARs are accessed
> > > 3. In exit function, all GARs are post-unmapped
> > >
> > > In 3), if struct acpi_iomap* is used as parameter for post-unmap
> > > function, we need to record that pointer in another list. In 2), we need
> > > find mapped address from GAR.
> >
> > I understand that my proposal would require a slight change in your
> > usage model. I am suggesting that you make it follow the same pattern
> > as pci_iomap(), e.g.:
> >
> > void *pci_iomap(struct pci_dev *, int bar, unsigned long len);
> > unsigned int ioread32(void *);
> > void pci_iounmap(struct pci_dev *, void *);
> >
> > void *acpi_map_generic_address(struct acpi_generic_address *);
> > u64 acpi_read_atomic(void *);
> > void acpi_unmap_generic_address(void *);
> >
> > acpi_map_generic_address() would validate the GAR and do the ioremap().
> > If the validation or ioremap() failed, it would return a NULL pointer.
> >
> > This would require the caller of acpi_map_generic_address() to hang onto
> > the pointer returned (just as callers of pci_iomap() must hang onto the
> > pointer it returns).
> >
> > The pointer would be supplied to acpi_read_atomic(), so it would not
> > need to do acpi_check_gar() because we know it was done successfully
> > in acpi_map_generic_address(). It wouldn't need to look up the GAR
> > in the list because the list entry was passed in. Since all the
> > possible failure conditions were checked in acpi_map_generic_address(),
> > acpi_read_atomic() doesn't need to return status and could simply
> > return the u64 directly.
>
> The usage model is different. Please take a look at
> __apei_exec_read_register(). We need to get virtual address from
> physical address in GAR. And many small-size GARs in ERST or EINJ may
> share same page, so some kind of virtual space optimization is
> necessary.
The sharing could be done with a reference count in the map/unmap
path.
I don't have time to fully understand your patch series, but I suspect
the issue is that you're interpreting opcodes, and those contain or
reference generic_address structures, and you think it's easier just
pass around the GAR pointer than it is to remember a (GAR, mapped-GAR)
association made by pre_map_gar() and use the mapped-GAR when executing
the "read" opcode.
Bjorn
On Thu, 2009-12-17 at 01:54 +0800, Bjorn Helgaas wrote:
> On Tuesday 15 December 2009 06:23:15 pm Huang Ying wrote:
> > On Wed, 2009-12-16 at 01:47 +0800, Bjorn Helgaas wrote:
> > > On Monday 14 December 2009 06:04:13 pm Huang Ying wrote:
> > > > On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote:
> > > > > I think your code would be simpler if acpi_pre_map_gar() returned a
> > > > > struct acpi_iomap pointer (from the caller's point of view, this would
> > > > > be an opaque cookie). Then you could just supply that cookie to
> > > > > acpi_atomic_write(), and you wouldn't have to look it up again. Maybe
> > > > > you could even get rid of the list and all the fancy RCU & kref stuff
> > > > > then, too.
> > > >
> > > > The interface chosen is based on usage model, which is:
> > > >
> > > > 1. In init function, all GARs needed are pre-mapped
> > > > 2. In atomic context, pre-mapped GARs are accessed
> > > > 3. In exit function, all GARs are post-unmapped
> > > >
> > > > In 3), if struct acpi_iomap* is used as parameter for post-unmap
> > > > function, we need to record that pointer in another list. In 2), we need
> > > > find mapped address from GAR.
> > >
> > > I understand that my proposal would require a slight change in your
> > > usage model. I am suggesting that you make it follow the same pattern
> > > as pci_iomap(), e.g.:
> > >
> > > void *pci_iomap(struct pci_dev *, int bar, unsigned long len);
> > > unsigned int ioread32(void *);
> > > void pci_iounmap(struct pci_dev *, void *);
> > >
> > > void *acpi_map_generic_address(struct acpi_generic_address *);
> > > u64 acpi_read_atomic(void *);
> > > void acpi_unmap_generic_address(void *);
> > >
> > > acpi_map_generic_address() would validate the GAR and do the ioremap().
> > > If the validation or ioremap() failed, it would return a NULL pointer.
> > >
> > > This would require the caller of acpi_map_generic_address() to hang onto
> > > the pointer returned (just as callers of pci_iomap() must hang onto the
> > > pointer it returns).
> > >
> > > The pointer would be supplied to acpi_read_atomic(), so it would not
> > > need to do acpi_check_gar() because we know it was done successfully
> > > in acpi_map_generic_address(). It wouldn't need to look up the GAR
> > > in the list because the list entry was passed in. Since all the
> > > possible failure conditions were checked in acpi_map_generic_address(),
> > > acpi_read_atomic() doesn't need to return status and could simply
> > > return the u64 directly.
> >
> > The usage model is different. Please take a look at
> > __apei_exec_read_register(). We need to get virtual address from
> > physical address in GAR. And many small-size GARs in ERST or EINJ may
> > share same page, so some kind of virtual space optimization is
> > necessary.
>
> The sharing could be done with a reference count in the map/unmap
> path.
>
> I don't have time to fully understand your patch series, but I suspect
> the issue is that you're interpreting opcodes, and those contain or
> reference generic_address structures, and you think it's easier just
> pass around the GAR pointer than it is to remember a (GAR, mapped-GAR)
> association made by pre_map_gar() and use the mapped-GAR when executing
> the "read" opcode.
So we need the (GAR, mapped-GAR) association anyway. Variable
acpi_iomaps in atomicio.c is such a association. When executing the
"read" opcode, we still need get mapped-GAR from the GAR. So I think
your proposal and my implementation is similar, with different naming
convention and you want to put the association in APEI code, and I want
to put it in ACPI code.
Best Regards,
Huang Ying
On Thursday 17 December 2009 01:33:07 am Huang Ying wrote:
> On Thu, 2009-12-17 at 01:54 +0800, Bjorn Helgaas wrote:
> > On Tuesday 15 December 2009 06:23:15 pm Huang Ying wrote:
> > > On Wed, 2009-12-16 at 01:47 +0800, Bjorn Helgaas wrote:
> > > > On Monday 14 December 2009 06:04:13 pm Huang Ying wrote:
> > > > > On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote:
> > > > > > I think your code would be simpler if acpi_pre_map_gar() returned a
> > > > > > struct acpi_iomap pointer (from the caller's point of view, this would
> > > > > > be an opaque cookie). Then you could just supply that cookie to
> > > > > > acpi_atomic_write(), and you wouldn't have to look it up again. Maybe
> > > > > > you could even get rid of the list and all the fancy RCU & kref stuff
> > > > > > then, too.
> > > > >
> > > > > The interface chosen is based on usage model, which is:
> > > > >
> > > > > 1. In init function, all GARs needed are pre-mapped
> > > > > 2. In atomic context, pre-mapped GARs are accessed
> > > > > 3. In exit function, all GARs are post-unmapped
> > > > >
> > > > > In 3), if struct acpi_iomap* is used as parameter for post-unmap
> > > > > function, we need to record that pointer in another list. In 2), we need
> > > > > find mapped address from GAR.
> > > >
> > > > I understand that my proposal would require a slight change in your
> > > > usage model. I am suggesting that you make it follow the same pattern
> > > > as pci_iomap(), e.g.:
> > > >
> > > > void *pci_iomap(struct pci_dev *, int bar, unsigned long len);
> > > > unsigned int ioread32(void *);
> > > > void pci_iounmap(struct pci_dev *, void *);
> > > >
> > > > void *acpi_map_generic_address(struct acpi_generic_address *);
> > > > u64 acpi_read_atomic(void *);
> > > > void acpi_unmap_generic_address(void *);
> > > >
> > > > acpi_map_generic_address() would validate the GAR and do the ioremap().
> > > > If the validation or ioremap() failed, it would return a NULL pointer.
> > > >
> > > > This would require the caller of acpi_map_generic_address() to hang onto
> > > > the pointer returned (just as callers of pci_iomap() must hang onto the
> > > > pointer it returns).
> > > >
> > > > The pointer would be supplied to acpi_read_atomic(), so it would not
> > > > need to do acpi_check_gar() because we know it was done successfully
> > > > in acpi_map_generic_address(). It wouldn't need to look up the GAR
> > > > in the list because the list entry was passed in. Since all the
> > > > possible failure conditions were checked in acpi_map_generic_address(),
> > > > acpi_read_atomic() doesn't need to return status and could simply
> > > > return the u64 directly.
> > >
> > > The usage model is different. Please take a look at
> > > __apei_exec_read_register(). We need to get virtual address from
> > > physical address in GAR. And many small-size GARs in ERST or EINJ may
> > > share same page, so some kind of virtual space optimization is
> > > necessary.
> >
> > The sharing could be done with a reference count in the map/unmap
> > path.
> >
> > I don't have time to fully understand your patch series, but I suspect
> > the issue is that you're interpreting opcodes, and those contain or
> > reference generic_address structures, and you think it's easier just
> > pass around the GAR pointer than it is to remember a (GAR, mapped-GAR)
> > association made by pre_map_gar() and use the mapped-GAR when executing
> > the "read" opcode.
>
> So we need the (GAR, mapped-GAR) association anyway. Variable
> acpi_iomaps in atomicio.c is such a association. When executing the
> "read" opcode, we still need get mapped-GAR from the GAR. So I think
> your proposal and my implementation is similar, with different naming
> convention and you want to put the association in APEI code, and I want
> to put it in ACPI code.
Well, yes, they're similar in the sense that both do remember the (GAR,
mapped-GAR) association. Your implementation remembers it in the
acpi_iomaps list, and I proposed having the caller keep it. The
advantage I see with having the caller keep it is that it would make
atomicio.c MUCH simpler -- no list, no lookup, no repeated validation,
no RCU, probably no kref stuff. Obviously this would come at the cost
of some increased complexity in the callers.
And I think it's worth something to use the pattern of pci_iomap(),
because that's a well-known Linux design pattern.
Anyway, just my opinion; feel free to ignore.
Bjorn
On 12/16/2009 02:23 AM, Huang Ying wrote:
> On Wed, 2009-12-16 at 01:47 +0800, Bjorn Helgaas wrote:
>> On Monday 14 December 2009 06:04:13 pm Huang Ying wrote:
...
>>> The checking here is for bug in firmware not software. I think it is
>>> necessary for the user to know where the bugs may come from, and it is
>>> hard to express the bug in return code.
>>
>> Yes, I understand that this is checking for firmware bugs. My point
>> is that when a user sees this in his dmesg log:
>>
>> Invalid bit width in GAR, firmware bug?
>>
>> we have no context, and even a kernel developer can't figure out what
>> to do. We could ask for a copy of the FADT and DSDT, but even then,
>> we don't know *which* GAR structure to look at, so we'll probably have
>> to add some instrumentation and ask the user to reproduce the problem.
>>
>> If the check were in the caller, it could at least say something like:
>>
>> ACPI: couldn't map generic address [io 0xcf8] for PCI config access
>>
>> which would give us more useful information.
>
> En... Yes, some information about the invalid GAR is helpful. But the
> GAR information is available in acpi_check_gar too, so we can output
> something as follow in acpi_check_gar:
>
> Invalid bit width in GAR [mem 0x8029ff8 24], firmware bug?
>
> Your message looks like a software issue instead of firmware bug. This
> may confuse the user and developer.
Please use one of these (cmp. with include/linux/kernel.h when to use
which):
#define FW_BUG "[Firmware Bug]: "
#define FW_WARN "[Firmware Warn]: "
#define FW_INFO "[Firmware Info]: "
It's great that someone cares to tell syslog/users that it's a firmware
bug, that should get more common...
Thomas
Hi!
> Some ACPI IO accessing need to be done in atomic context. For example,
Does it? Maybe better fix would be to make sure acpi interpretter
can't be ran from interrupt context -- just create threads for it...?
It would solve a lot of uglyness with acpi trying to guess if it can
sleep etc...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html