ACPI: Memory Mapped I/O (MMIO) pre-mapping

This patch optimizes ACPI MMIO remappings by keeping track of the remappings on a PAGE_SIZE granularity.

When an ioremap() occurs, the underlying infrastructure works on a 'page' based granularity. As such, an ioremap() request for 1 byte for example, will end up mapping in an entire (PAGE_SIZE) page. Huang Ying took advantage of this in commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262 by checking if subsequent ioremap() requests reside within any of the list's existing remappings still in place, and if so, incrementing a reference count on the existing mapping as opposed to performing another ioremap().

Signed-off-by Myron Stowe <[email protected]>

---

 drivers/acpi/osl.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 51 insertions(+), 11 deletions(-)

drivers/acpi/osl.c +22 -6

index 3282689..885e222 100644
@@ -104,6 +104,7 @@ struct acpi_ioremap {
 	void __iomem *virt;
 	acpi_physical_address phys;
 	acpi_size size;
+	struct kref ref;
 };
 
 static LIST_HEAD(acpi_ioremaps);

@@ -245,15 +246,28 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 }
 
 /* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
-static void __iomem *
-acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
+static struct acpi_ioremap *
+acpi_map_lookup(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
 	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
 		if (map->phys <= phys &&
 		    phys + size <= map->phys + map->size)
-			return map->virt + (phys - map->phys);
+			return map;
+
+	return NULL;
+}
+
+/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
+static void __iomem *
+acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
+{
+	struct acpi_ioremap *map;
+
+	map = acpi_map_lookup(phys, size);
+	if (map)
+		return map->virt + (phys - map->phys);
 
 	return NULL;
 }

@@ -265,7 +279,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 	struct acpi_ioremap *map;
 
 	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
-		if (map->virt == virt && map->size == size)
+		if (map->virt <= virt &&
+		    virt + size <= map->virt + map->size)
 			return map;
 
 	return NULL;

@@ -274,9 +289,10 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 void __iomem *__init_refok
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	struct acpi_ioremap *map;
-	unsigned long flags;
+	struct acpi_ioremap *map, *tmp_map;
+	unsigned long flags, pg_sz;
 	void __iomem *virt;
+	phys_addr_t pg_off;
 
 	if (phys > ULONG_MAX) {
 		printk(KERN_ERR PREFIX "Cannot map memory that high\n");

drivers/acpi/osl.c +26 -4

@@ -298,21 +316,40 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 
 	INIT_LIST_HEAD(&map->list);
 	map->virt = virt;
-	map->phys = phys;
-	map->size = size;
+	map->phys = pg_off;
+	map->size = pg_sz;
+	kref_init(&map->ref);
 
 	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	/* Check if page has already been mapped. */
+	tmp_map = acpi_map_lookup(phys, size);
+	if (tmp_map) {
+		kref_get(&tmp_map->ref);
+		spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+		iounmap(map->virt);
+		kfree(map);
+		return tmp_map->virt + (phys - tmp_map->phys);
+	}
 	list_add_tail_rcu(&map->list, &acpi_ioremaps);
 	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
 
-	return virt;
+	return map->virt + (phys - map->phys);
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_memory);
 
+static void acpi_kref_del_iomap(struct kref *ref)
+{
+	struct acpi_ioremap *map;
+
+	map = container_of(ref, struct acpi_ioremap, ref);
+	list_del_rcu(&map->list);
+}
+
 void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 {
 	struct acpi_ioremap *map;
 	unsigned long flags;
+	int del;
 
 	if (!acpi_gbl_permanent_mmap) {
 		__acpi_unmap_table(virt, size);

@@ -328,9 +365,12 @@ void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 		return;
 	}
 
-	list_del_rcu(&map->list);
+	del = kref_put(&map->ref, acpi_kref_del_iomap);
 	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
 
+	if (!del)
+		return;
+
 	synchronize_rcu();
 	iounmap(map->virt);
 	kfree(map);

Hi, Myron,

Quoted 89 lines · Myron Stowe

This patch optimizes ACPI MMIO remappings by keeping track of the remappings on a PAGE_SIZE granularity.

When an ioremap() occurs, the underlying infrastructure works on a 'page' based granularity. As such, an ioremap() request for 1 byte for example, will end up mapping in an entire (PAGE_SIZE) page. Huang Ying took advantage of this in commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262 by checking if subsequent ioremap() requests reside within any of the list's existing remappings still in place, and if so, incrementing a reference count on the existing mapping as opposed to performing another ioremap().

Signed-off-by Myron Stowe <[email protected]>

---

 drivers/acpi/osl.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 51 insertions(+), 11 deletions(-)

drivers/acpi/osl.c +22 -6

index 3282689..885e222 100644
@@ -104,6 +104,7 @@ struct acpi_ioremap {
 	void __iomem *virt;
 	acpi_physical_address phys;
 	acpi_size size;
+	struct kref ref;
 };
 
 static LIST_HEAD(acpi_ioremaps);

@@ -245,15 +246,28 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 }
 
 /* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
-static void __iomem *
-acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
+static struct acpi_ioremap *
+acpi_map_lookup(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
 	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
 		if (map->phys <= phys &&
 		    phys + size <= map->phys + map->size)
-			return map->virt + (phys - map->phys);
+			return map;
+
+	return NULL;
+}
+
+/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
+static void __iomem *
+acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
+{
+	struct acpi_ioremap *map;
+
+	map = acpi_map_lookup(phys, size);
+	if (map)
+		return map->virt + (phys - map->phys);
 
 	return NULL;
 }

@@ -265,7 +279,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 	struct acpi_ioremap *map;
 
 	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
-		if (map->virt == virt && map->size == size)
+		if (map->virt <= virt &&
+		    virt + size <= map->virt + map->size)
 			return map;
 
 	return NULL;

@@ -274,9 +289,10 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 void __iomem *__init_refok
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	struct acpi_ioremap *map;
-	unsigned long flags;
+	struct acpi_ioremap *map, *tmp_map;
+	unsigned long flags, pg_sz;
 	void __iomem *virt;
+	phys_addr_t pg_off;
 
 	if (phys > ULONG_MAX) {
 		printk(KERN_ERR PREFIX "Cannot map memory that high\n");

@@ -290,7 +306,9 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 	if (!map)
 		return NULL;
 

drivers/acpi/osl.c acpi_os_map_memory(acpi_physical_address phys, acpi_size size) · line 314

Quoted 7 lines

drivers/acpi/osl.c +3 -1

-	virt = ioremap(phys, size);
+	pg_off = round_down(phys, PAGE_SIZE);
+	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
+	virt = ioremap(pg_off, pg_sz);
 	if (!virt) {
 		kfree(map);
 		return NULL;

Why not check the existing map (that is, acpi_map_lookup()) before the ioremap()? In APEI tables, several GAS may be in same page, so we need ioremap() for the first page only.

Best Regards,
Huang Ying

Quoted 59 lines

drivers/acpi/osl.c +26 -4

@@ -298,21 +316,40 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 
 	INIT_LIST_HEAD(&map->list);
 	map->virt = virt;
-	map->phys = phys;
-	map->size = size;
+	map->phys = pg_off;
+	map->size = pg_sz;
+	kref_init(&map->ref);
 
 	spin_lock_irqsave(&acpi_ioremap_lock, flags);
+	/* Check if page has already been mapped. */
+	tmp_map = acpi_map_lookup(phys, size);
+	if (tmp_map) {
+		kref_get(&tmp_map->ref);
+		spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
+		iounmap(map->virt);
+		kfree(map);
+		return tmp_map->virt + (phys - tmp_map->phys);
+	}
 	list_add_tail_rcu(&map->list, &acpi_ioremaps);
 	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
 
-	return virt;
+	return map->virt + (phys - map->phys);
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_memory);
 
+static void acpi_kref_del_iomap(struct kref *ref)
+{
+	struct acpi_ioremap *map;
+
+	map = container_of(ref, struct acpi_ioremap, ref);
+	list_del_rcu(&map->list);
+}
+
 void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 {
 	struct acpi_ioremap *map;
 	unsigned long flags;
+	int del;
 
 	if (!acpi_gbl_permanent_mmap) {
 		__acpi_unmap_table(virt, size);

@@ -328,9 +365,12 @@ void __ref acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 		return;
 	}
 
-	list_del_rcu(&map->list);
+	del = kref_put(&map->ref, acpi_kref_del_iomap);
 	spin_unlock_irqrestore(&acpi_ioremap_lock, flags);
 
+	if (!del)
+		return;
+
 	synchronize_rcu();
 	iounmap(map->virt);
 	kfree(map);

Quoted 92 lines · Huang Ying

Hi, Myron,

Quoted 89 lines · Myron Stowe

This patch optimizes ACPI MMIO remappings by keeping track of the remappings on a PAGE_SIZE granularity.

When an ioremap() occurs, the underlying infrastructure works on a 'page' based granularity. As such, an ioremap() request for 1 byte for example, will end up mapping in an entire (PAGE_SIZE) page. Huang Ying took advantage of this in commit 15651291a2f8c11e7e6a42d8bfde7a213ff13262 by checking if subsequent ioremap() requests reside within any of the list's existing remappings still in place, and if so, incrementing a reference count on the existing mapping as opposed to performing another ioremap().

Signed-off-by Myron Stowe <[email protected]>

---

 drivers/acpi/osl.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 51 insertions(+), 11 deletions(-)

drivers/acpi/osl.c +22 -6

index 3282689..885e222 100644
@@ -104,6 +104,7 @@ struct acpi_ioremap {
 	void __iomem *virt;
 	acpi_physical_address phys;
 	acpi_size size;
+	struct kref ref;
 };
 
 static LIST_HEAD(acpi_ioremaps);

@@ -245,15 +246,28 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 }
 
 /* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
-static void __iomem *
-acpi_map_vaddr_lookup(acpi_physical_address phys, acpi_size size)
+static struct acpi_ioremap *
+acpi_map_lookup(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
 	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
 		if (map->phys <= phys &&
 		    phys + size <= map->phys + map->size)
-			return map->virt + (phys - map->phys);
+			return map;
+
+	return NULL;
+}
+
+/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
+static void __iomem *
+acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
+{
+	struct acpi_ioremap *map;
+
+	map = acpi_map_lookup(phys, size);
+	if (map)
+		return map->virt + (phys - map->phys);
 
 	return NULL;
 }

@@ -265,7 +279,8 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 	struct acpi_ioremap *map;
 
 	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
-		if (map->virt == virt && map->size == size)
+		if (map->virt <= virt &&
+		    virt + size <= map->virt + map->size)
 			return map;
 
 	return NULL;

@@ -274,9 +289,10 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 void __iomem *__init_refok
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	struct acpi_ioremap *map;
-	unsigned long flags;
+	struct acpi_ioremap *map, *tmp_map;
+	unsigned long flags, pg_sz;
 	void __iomem *virt;
+	phys_addr_t pg_off;
 
 	if (phys > ULONG_MAX) {
 		printk(KERN_ERR PREFIX "Cannot map memory that high\n");

@@ -290,7 +306,9 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 	if (!map)
 		return NULL;
 

Quoted 11 lines

drivers/acpi/osl.c acpi_os_map_memory(acpi_physical_address phys, acpi_size size) · line 314

Quoted 7 lines

drivers/acpi/osl.c +3 -1

-	virt = ioremap(phys, size);
+	pg_off = round_down(phys, PAGE_SIZE);
+	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
+	virt = ioremap(pg_off, pg_sz);
 	if (!virt) {
 		kfree(map);
 		return NULL;

Why not check the existing map (that is, acpi_map_lookup()) before the ioremap()? In APEI tables, several GAS may be in same page, so we need ioremap() for the first page only.

Yes, even in other instances, several registers (GAS) may reside within the same page(s) of a previously, still active, ioremapping and thus only one ioremapping needs to be in place (with the subsequent mapping(s) that fall within the same page(s) of the existing, active, remapping then only needing to increment its reference).

In the existing implementation - atomicio.c:acpi_pre_map() - there is a check at the beginning of the routine to see if a new mapping fits within the page(s) of a previous entry's page(s) that is still active:
__acpi_try_ioremap(). Then later in the same routine, the same __acpi_try_ioremap() check has to be made again due to a potential mp race between the initial check and the insertion of a potentially new entry onto the list.

Since the second check *must* take place,

due to the possible race, I
decided to skip the initial check in the re-factored implementation knowing that such a situation would ultimately still be caught by it (the second check) and handled correctly: a subsequent, overlapping, mapping is caught by the "tmp_map = acpi_map_lookup()" below, the reference to the initial, active, remapping incremented, and the overlapping mapping is "backed out".

Do you see any issues with such an approach?

Quoted 3 lines · Myron Stowe

Quoted 2 lines · Huang Ying

Hi, Myron,

Quoted 20 lines

Quoted 4 lines

Why not check the existing map (that is, acpi_map_lookup()) before the ioremap()? In APEI tables, several GAS may be in same page, so we need ioremap() for the first page only.

Yes, even in other instances, several registers (GAS) may reside within the same page(s) of a previously, still active, ioremapping and thus only one ioremapping needs to be in place (with the subsequent mapping(s) that fall within the same page(s) of the existing, active, remapping then only needing to increment its reference).

In the existing implementation - atomicio.c:acpi_pre_map() - there is a check at the beginning of the routine to see if a new mapping fits within the page(s) of a previous entry's page(s) that is still active:
__acpi_try_ioremap(). Then later in the same routine, the same __acpi_try_ioremap() check has to be made again due to a potential mp race between the initial check and the insertion of a potentially new entry onto the list.

Quoted 1 line

Since the second check *must* take place,

No. If the first check succeeds, the second check is not necessary, it is sufficient just to increase the reference count and return the mapped virtual address. The first check will eliminate kmalloc/ioremap/iounmap/kfree for physical address has been mapped by previous calling.

Quoted 10 lines

due to the possible race, I
decided to skip the initial check in the re-factored implementation knowing that such a situation would ultimately still be caught by it (the second check) and handled correctly: a subsequent, overlapping, mapping is caught by the "tmp_map = acpi_map_lookup()" below, the reference to the initial, active, remapping incremented, and the overlapping mapping is "backed out".

Do you see any issues with such an approach?

Best Regards,
Huang Ying

Quoted 4 lines · Huang Ying

Quoted 3 lines · Myron Stowe

Quoted 2 lines · Huang Ying

Hi, Myron,

<snip>

Quoted 20 lines

Quoted 20 lines

Quoted 4 lines

Why not check the existing map (that is, acpi_map_lookup()) before the ioremap()? In APEI tables, several GAS may be in same page, so we need ioremap() for the first page only.

Yes, even in other instances, several registers (GAS) may reside within the same page(s) of a previously, still active, ioremapping and thus only one ioremapping needs to be in place (with the subsequent mapping(s) that fall within the same page(s) of the existing, active, remapping then only needing to increment its reference).

In the existing implementation - atomicio.c:acpi_pre_map() - there is a check at the beginning of the routine to see if a new mapping fits within the page(s) of a previous entry's page(s) that is still active:
__acpi_try_ioremap(). Then later in the same routine, the same __acpi_try_ioremap() check has to be made again due to a potential mp race between the initial check and the insertion of a potentially new entry onto the list.

Quoted 7 lines

Quoted 1 line

Since the second check *must* take place,

No. If the first check succeeds, the second check is not necessary, it is sufficient just to increase the reference count and return the mapped virtual address. The first check will eliminate kmalloc/ioremap/iounmap/kfree for physical address has been mapped by previous calling.

Hay Haung:

Let's take two scenarios and see if I can express myself better why I believe that the second check must take place. Let's also stick with the existing atomicio.c:acpi_pre_map() implementation for now just to keep things simpler.

Scenario 1: Assume that an MMIO remapping is already in place - mapping A. At some later point in time another call is made to map in some other MMIO area and this mapping - mapping B - resides within the same page(s) of mapping A which is still active, and thus, still on the list.

In this scenario, the first __acpi_try_ioremap() check will detect that mapping B resides within mapping A, increment the reference of mapping A, and then acpi_pre_map() short circuits out. Things are handled correctly and the first check nicely short circuits out so the second check was never even reached.

Now let's take another scenario, Scenario 2: Assume that an MMIO remapping is occurring on some processor of a MP system to put mapping A into place (so the the processor is processing code somewhere between "pg_off = ..." and "kref_init()"). While the processor is in the process of putting mapping A into place it becomes blocked for any of a number of reasons. Another processor now runs and puts a mapping into place - mapping C - that is equal to or greater in size than mapping A which also encompasses the same page(s) of mapping A. So now, at this point in time, mapping C is on the list but mapping A is not.

At some later point in time the processor running mapping A will continue. It acquires the spin_lock immediately following the "kref_init()" and must now check again to see if some other processor put a similar mapping into place. In this scenario, exactly such has happened - which is why I think you put this check in place to begin with. The MP race is properly detected by the second __acpi_try_ioremap() check which increments the reference of mapping C and mapping A is backed out (iounmap/kfree).

So with the existing atomicio.c:acpi_pre_map() implementation there are two __acpi_try_ioremap() checks. The first check handles situations like Scenario 1 and the second check handles situations like Scenario 2.

It was situations like Scenario 2 that I was thinking of in my initial response when I said that the second check *must* take place. I agree, as indicated above, in situations like Scenario 1 the second check is never even reached.

Hopefully I have now expressed better why I believe the second check *must* take place?

Now back to your original question - why did I not include the first check in the re-factoring?

My thinking was along the lines of: "Yes, the first check is nice in that it short circuits additional remappings that match existing mappings. Thinking further, consider that since the second check must still occur due to situations like Scenario 2, and, since the second check would not only catch situations like Scenario 2 but also additionally catches any additional remappings matching existing ones in place like Scenario 1 - why not just keep the code simpler/smaller and not include the first check?"

I agree that such a tack does have trade offs. I chose to keep the code simpler/smaller at the expense of giving up the "short circuit" benefits that the first check provides in situations like Scenario 1. The code, without the first check still handles situations like Scenario 1 - just not as efficiently (i.e. it incurs the expense of "kmalloc/ioremap/iounmap/kfree for physical address has been mapped by previous calling" as you properly pointed out.

With this explanation of my actions do you see any issues with such an approach?

ACPI's system event related IRQ handing accesses specific fixed hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1 which are declared in the FADT. If these registers are backed by MMIO, as opposed to I/O port space, accessing them within interrupt context will incur a panic since acpi_read() and acpi_write() end up calling ioremap(), which may block to allocate memory - BZ 18012.

Quoted 6 lines · Myron Stowe

ACPI's system event related IRQ handing accesses specific fixed hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1 which are declared in the FADT. If these registers are backed by MMIO, as opposed to I/O port space, accessing them within interrupt context will incur a panic since acpi_read() and acpi_write() end up calling ioremap(), which may block to allocate memory - BZ 18012.

since you just access several bytes mmio in interrupt context, can't you use kmap_atomic_pfn() here?

Quoted 7 lines · Shaohua

Quoted 6 lines · Myron Stowe

ACPI's system event related IRQ handing accesses specific fixed hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1 which are declared in the FADT. If these registers are backed by MMIO, as opposed to I/O port space, accessing them within interrupt context will incur a panic since acpi_read() and acpi_write() end up calling ioremap(), which may block to allocate memory - BZ 18012.

Quoted 2 lines

since you just access several bytes mmio in interrupt context, can't you use kmap_atomic_pfn() here?

On x86_64, kmap_atomic_pfn() is defined as kmap_atomic(), which requires struct page for the physical address. But the MMIO address may have no struct page.

Quoted 8 lines · Ying

Quoted 7 lines · Shaohua

Quoted 6 lines · Myron Stowe

ACPI's system event related IRQ handing accesses specific fixed hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1 which are declared in the FADT. If these registers are backed by MMIO, as opposed to I/O port space, accessing them within interrupt context will incur a panic since acpi_read() and acpi_write() end up calling ioremap(), which may block to allocate memory - BZ 18012.

Quoted 6 lines

Quoted 2 lines

since you just access several bytes mmio in interrupt context, can't you use kmap_atomic_pfn() here?

On x86_64, kmap_atomic_pfn() is defined as kmap_atomic(), which requires struct page for the physical address. But the MMIO address may have no struct page.

ok, can we add a new entry in fix map and the entry is dedicated for mmio mapping? The ioremap list looks like a hack.

Quoted 8 lines · Ying

Quoted 7 lines · Shaohua

Quoted 6 lines · Myron Stowe

ACPI's system event related IRQ handing accesses specific fixed hardware registers; namely PM1a event, PM1b event, GPE0, and GPE1 which are declared in the FADT. If these registers are backed by MMIO, as opposed to I/O port space, accessing them within interrupt context will incur a panic since acpi_read() and acpi_write() end up calling ioremap(), which may block to allocate memory - BZ 18012.

Quoted 6 lines

Quoted 2 lines

since you just access several bytes mmio in interrupt context, can't you use kmap_atomic_pfn() here?

On x86_64, kmap_atomic_pfn() is defined as kmap_atomic(), which requires struct page for the physical address. But the MMIO address may have no struct page.

ok, can we add a new entry in fix map and the entry is dedicated for mmio mapping? The ioremap list looks like a hack.

On ia64 (which also uses ACPI), kmap_atomic_pfn() also requires a struct page. An ia64-specific version that doesn't need a struct page would be trivial, but I still don't know how to make it work on x86_64.

Quoted 4 lines

On ia64 (which also uses ACPI), kmap_atomic_pfn() also requires a struct page. An ia64-specific version that doesn't need a struct page would be trivial, but I still don't know how to make it work on x86_64.

Doing a struct page less kmap_atomic() shouldn't be too hard on x86 either.

Just to avoid problems with concurrency you likely need per CPU mappings.

Quoted 5 lines · Andi Kleen

Quoted 4 lines

On ia64 (which also uses ACPI), kmap_atomic_pfn() also requires a struct page. An ia64-specific version that doesn't need a struct page would be trivial, but I still don't know how to make it work on x86_64.

Quoted 5 lines

Doing a struct page less kmap_atomic() shouldn't be too hard on x86 either.

Just to avoid problems with concurrency you likely need per CPU mappings.

One issue is PAT management, like in general ioremap implementation (reserve_memtype).

applied to acpi-test

My comfort-level with these changes is not high.

I'm open to your recommendations on if they are
too late/risky for this merge window -- or too
important to miss this merge window.

thanks,
Len Brown, Intel Open Source Technology Center

applied to acpi-test

My comfort-level with these changes is not high.

I'm open to your recommendations on if they are
too late/risky for this merge window -- or too
important to miss this merge window.

Quoted 7 lines

applied to acpi-test

My comfort-level with these changes is not high.

I'm open to your recommendations on if they are
too late/risky for this merge window -- or too
important to miss this merge window.

The NMI handling is fairly important by itself, on the other hand it has been broken for a long time.

But yes the patches were late unfortunately.

I still think it would be better to push them out earlier than later simply to get more testing on more systems. That's essentially what they need and unfortunately the only really good way to get that is being part of a release.