diff mbox

[v3,07/12] ACPI / APEI: Make the nmi_fixmap_idx per-ghes to allow multiple in_nmi() users

Message ID 20180427153510.5799-8-james.morse@arm.com (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

James Morse April 27, 2018, 3:35 p.m. UTC
Arm64 has multiple NMI-like notifications, but ghes.c only has one
in_nmi() path, risking deadlock if one NMI-like notification can
interrupt another.

To support this we need a fixmap entry and lock for each notification
type. But ghes_probe() attempts to process each struct ghes at probe
time, to ensure any error that was notified before ghes_probe() was
called has been done, and the buffer released (and maybe acknowledge
to firmware) so that future errors can be delivered.

This means NMI-like notifications need two fixmap entries and locks,
one for the ghes_probe() time call, and another for the actual NMI
that could interrupt ghes_probe().

Split this single path up by adding an NMI fixmap idx and lock into
the struct ghes. Any notification that can be called as an NMI can
use these to separate its resources from any other notification it
may interrupt.

The majority of notifications occur in IRQ context, so unless its
called in_nmi(), ghes_copy_tofrom_phys() will use the FIX_APEI_GHES_IRQ
fixmap entry and the ghes_fixmap_lock_irq lock. This allows
NMI-notifications to be processed by ghes_probe(), and then taken
as an NMI.

The double-underscore version of fix_to_virt() is used because the index
to be mapped can't be tested against the end of the enum at compile
time.

Signed-off-by: James Morse <james.morse@arm.com>

---
Changes since v1:
 * Fixed for ghes_proc() always calling every notification in process context.
   Now only NMI-like notifications need an additional fixmap-slot/lock.

 drivers/acpi/apei/ghes.c | 70 +++++++++++++++++-------------------------------
 include/acpi/ghes.h      |  4 +++
 2 files changed, 28 insertions(+), 46 deletions(-)

Comments

Borislav Petkov May 5, 2018, 12:27 p.m. UTC | #1
On Fri, Apr 27, 2018 at 04:35:05PM +0100, James Morse wrote:
> Arm64 has multiple NMI-like notifications, but ghes.c only has one
> in_nmi() path, risking deadlock if one NMI-like notification can
> interrupt another.
> 
> To support this we need a fixmap entry and lock for each notification
> type. But ghes_probe() attempts to process each struct ghes at probe
> time, to ensure any error that was notified before ghes_probe() was
> called has been done, and the buffer released (and maybe acknowledge
> to firmware) so that future errors can be delivered.
> 
> This means NMI-like notifications need two fixmap entries and locks,
> one for the ghes_probe() time call, and another for the actual NMI
> that could interrupt ghes_probe().
> 
> Split this single path up by adding an NMI fixmap idx and lock into
> the struct ghes. Any notification that can be called as an NMI can
> use these to separate its resources from any other notification it
> may interrupt.
> 
> The majority of notifications occur in IRQ context, so unless its
> called in_nmi(), ghes_copy_tofrom_phys() will use the FIX_APEI_GHES_IRQ
> fixmap entry and the ghes_fixmap_lock_irq lock. This allows
> NMI-notifications to be processed by ghes_probe(), and then taken
> as an NMI.
> 
> The double-underscore version of fix_to_virt() is used because the index
> to be mapped can't be tested against the end of the enum at compile
> time.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> 
> ---
> Changes since v1:
>  * Fixed for ghes_proc() always calling every notification in process context.
>    Now only NMI-like notifications need an additional fixmap-slot/lock.

...

> @@ -986,6 +960,8 @@ int ghes_notify_sea(void)
>  
>  static void ghes_sea_add(struct ghes *ghes)
>  {
> +	ghes->nmi_fixmap_lock = &ghes_fixmap_lock_nmi;
> +	ghes->nmi_fixmap_idx = FIX_APEI_GHES_NMI;
>  	ghes_estatus_queue_grow_pool(ghes);
>  
>  	mutex_lock(&ghes_list_mutex);
> @@ -1032,6 +1008,8 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
>  
>  static void ghes_nmi_add(struct ghes *ghes)
>  {
> +	ghes->nmi_fixmap_lock = &ghes_fixmap_lock_nmi;

Ewww, we're assigning the spinlock to a pointer which we'll take later?
Yuck.

Why?

Do I see it correctly that one can have ACPI_HEST_NOTIFY_SEA and
ACPI_HEST_NOTIFY_NMI coexist in parallel on a single system?

If not, you can use a single spinlock.

If yes, then I'd prefer to make it less ugly and do the notification
type check ghes_probe() does:

	switch (generic->notify.type)

and take the respective spinlock in ghes_copy_tofrom_phys(). This way it
is a bit better than using a spinlock ptr.

Thx.
James Morse May 8, 2018, 8:45 a.m. UTC | #2
Hi Borislav,

On 05/05/18 13:27, Borislav Petkov wrote:
> On Fri, Apr 27, 2018 at 04:35:05PM +0100, James Morse wrote:
>> Arm64 has multiple NMI-like notifications, but ghes.c only has one
>> in_nmi() path, risking deadlock if one NMI-like notification can
>> interrupt another.
>>
>> To support this we need a fixmap entry and lock for each notification
>> type. But ghes_probe() attempts to process each struct ghes at probe
>> time, to ensure any error that was notified before ghes_probe() was
>> called has been done, and the buffer released (and maybe acknowledge
>> to firmware) so that future errors can be delivered.
>>
>> This means NMI-like notifications need two fixmap entries and locks,
>> one for the ghes_probe() time call, and another for the actual NMI
>> that could interrupt ghes_probe().
>>
>> Split this single path up by adding an NMI fixmap idx and lock into
>> the struct ghes. Any notification that can be called as an NMI can
>> use these to separate its resources from any other notification it
>> may interrupt.
>>
>> The majority of notifications occur in IRQ context, so unless its
>> called in_nmi(), ghes_copy_tofrom_phys() will use the FIX_APEI_GHES_IRQ
>> fixmap entry and the ghes_fixmap_lock_irq lock. This allows
>> NMI-notifications to be processed by ghes_probe(), and then taken
>> as an NMI.
>>
>> The double-underscore version of fix_to_virt() is used because the index
>> to be mapped can't be tested against the end of the enum at compile
>> time.

>> @@ -986,6 +960,8 @@ int ghes_notify_sea(void)
>>  
>>  static void ghes_sea_add(struct ghes *ghes)
>>  {
>> +	ghes->nmi_fixmap_lock = &ghes_fixmap_lock_nmi;
>> +	ghes->nmi_fixmap_idx = FIX_APEI_GHES_NMI;
>>  	ghes_estatus_queue_grow_pool(ghes);
>>  
>>  	mutex_lock(&ghes_list_mutex);
>> @@ -1032,6 +1008,8 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
>>  
>>  static void ghes_nmi_add(struct ghes *ghes)
>>  {
>> +	ghes->nmi_fixmap_lock = &ghes_fixmap_lock_nmi;
> 
> Ewww, we're assigning the spinlock to a pointer which we'll take later?
> Yuck.

> Why?

So that APEI doesn't need to know which lock goes with which fixmap page, and
how these notifications interact.


> Do I see it correctly that one can have ACPI_HEST_NOTIFY_SEA and
> ACPI_HEST_NOTIFY_NMI coexist in parallel on a single system?

NOTIFY_NMI is x86's NMI, arm doesn't have anything that behaves in the same way,
so doesn't use it. The equivalent notifications with NMI-like behaviour are:
* SEA (synchronous external abort)
* SEI (SError Interrupt)
* SDEI (software delegated exception interface)


> If not, you can use a single spinlock.

Today we could, but once we have SDEI and SEI this won't work:
SDEI behaves as two notifications, 'normal' and 'critical', a different fixmap
page is needed for these as they can interrupt each other, and a different lock.

SEA can interrupt SEI, so they need a different fixmap-pages and locks.
We can always disable SEI when we're handling another NMI-like notification.

I doubt anyone would implement all three, but if they did SEA can interrupt the lot.


I'd like to avoid describing any of these interactions in ghes.c, I think it
should be possible that any notification can interrupt any other notification
without the risk of deadlock.


> If yes, then I'd prefer to make it less ugly and do the notification
> type check ghes_probe() does:
> 
> 	switch (generic->notify.type)
> 
> and take the respective spinlock in ghes_copy_tofrom_phys(). This way it
> is a bit better than using a spinlock ptr.

I wanted to avoid duplicating that list, some of the locks are #ifdef'd so it
gets ugly quickly. (We would only need the NMI-like notifications though).

I'd really like to avoid the GHES code having to know about normal/critical SDEI
events.


Alternatively, I can put the fixmap-page and spinlock in some 'struct
ghes_notification' that only the NMI-like struct-ghes need. This is just moving
the indirection up a level, but it does pair the lock with the thing it locks,
and gets rid of assigning spinlock pointers.


Thanks,

James
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov May 16, 2018, 11:05 a.m. UTC | #3
On Tue, May 08, 2018 at 09:45:01AM +0100, James Morse wrote:
> NOTIFY_NMI is x86's NMI, arm doesn't have anything that behaves in the same way,
> so doesn't use it. The equivalent notifications with NMI-like behaviour are:
> * SEA (synchronous external abort)
> * SEI (SError Interrupt)
> * SDEI (software delegated exception interface)

Oh wow, three! :)

> Alternatively, I can put the fixmap-page and spinlock in some 'struct
> ghes_notification' that only the NMI-like struct-ghes need. This is just moving
> the indirection up a level, but it does pair the lock with the thing it locks,
> and gets rid of assigning spinlock pointers.

Keeping the lock and what it protects in one place certainly sounds
better. I guess you could so something like this:

struct ghes_fixmap {
 union {
  raw_spinlock_t nmi_lock;
   spinlock_t lock;
 };
 void __iomem *(map)(struct ghes_fixmap *);
};

and assign the proper ghes_ioremap function to ->map.

The spin_lock_irqsave() call in ghes_copy_tofrom_phys() is kinda
questionable. Because we should have disabled interrupts so that you can
do

spin_lock(map->lock);

Except that we do get called with IRQs on and looking at that call of
ghes_proc() at the end of ghes_probe(), that's a deadlock waiting to
happen.

And that comes from:

  77b246b32b2c ("acpi: apei: check for pending errors when probing GHES entries")

Tyler, this can't work in any context: imagine the GHES NMI or IRQ or
the timer fires while that ghes_proc() runs...

What's up?
James Morse May 16, 2018, 2:51 p.m. UTC | #4
Hi Borislav,

On 16/05/18 12:05, Borislav Petkov wrote:
> On Tue, May 08, 2018 at 09:45:01AM +0100, James Morse wrote:
>> NOTIFY_NMI is x86's NMI, arm doesn't have anything that behaves in the same way,
>> so doesn't use it. The equivalent notifications with NMI-like behaviour are:
>> * SEA (synchronous external abort)
>> * SEI (SError Interrupt)
>> * SDEI (software delegated exception interface)
> > Oh wow, three! :)

The first two overload existing architectural behavior, the third improves all
this with a third standard option. Its the standard story!


>> Alternatively, I can put the fixmap-page and spinlock in some 'struct
>> ghes_notification' that only the NMI-like struct-ghes need. This is just moving
>> the indirection up a level, but it does pair the lock with the thing it locks,
>> and gets rid of assigning spinlock pointers.
> 
> Keeping the lock and what it protects in one place certainly sounds
> better.

Yup, I was about to post a v4...


> I guess you could so something like this:
> 
> struct ghes_fixmap {
>  union {
>   raw_spinlock_t nmi_lock;
>    spinlock_t lock;
>  };

(heh, spinlock_t already contains a raw_spinlock_t)

>  void __iomem *(map)(struct ghes_fixmap *);
> };
> 
> and assign the proper ghes_ioremap function to ->map.

The function pointer is a problem because SDEI is effectively two notification
methods. Critical can interrupt normal. I'd really like to keep the differences
buried in the SDEI driver.

v4 has a separate structure for the fixmap-entry and lock, which
ghes_copy_tofrom_phys() reaches into if in_nmi().


> The spin_lock_irqsave() call in ghes_copy_tofrom_phys() is kinda
> questionable. Because we should have disabled interrupts so that you can
> do
> 
> spin_lock(map->lock);

I thought this was for the polled driver, but that must be backed by an
interrupt too...

linux/timer.h has:
|  * An irqsafe timer is executed with IRQ disabled and it's safe to wait for
|  * the completion of the running instance from IRQ handlers, for example,
|  * by calling del_timer_sync().
|  *
|  * Note: The irq disabled callback execution is a special case for
|  * workqueue locking issues. It's not meant for executing random crap
|  * with interrupts disabled. Abuse is monitored!

This irq-disable behaviour is controlled by the flags field:
| #define TIMER_DEFERRABLE	0x00080000
| #define TIMER_IRQSAFE		0x00200000

and ghes_probe() does this:
| timer_setup(&ghes->timer, ghes_poll_func, TIMER_DEFERRABLE);

So I think the ghes_poll_func() can be called with IRQs unmasked, hence the
spin_lock_irqsave().


> Except that we do get called with IRQs on and looking at that call of
> ghes_proc() at the end of ghes_probe(), that's a deadlock waiting to
> happen.
> 
> And that comes from:
> 
>   77b246b32b2c ("acpi: apei: check for pending errors when probing GHES entries")
> 
> Tyler, this can't work in any context: imagine the GHES NMI or IRQ or
> the timer fires while that ghes_proc() runs...

I thought this was safe because its just ghes_copy_tofrom_phys()s access to the
fixmap slots that needs mutual exclusion.

Polled and all the IRQ flavours are kept apart by the spin_lock_irqsave(), and
the NMIs have their own fixmap entry. (This is fine until there is more than
once source of NMI)


Thanks,

James
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tyler Baicar May 16, 2018, 3:38 p.m. UTC | #5
On 5/16/2018 7:05 AM, Borislav Petkov wrote:
> On Tue, May 08, 2018 at 09:45:01AM +0100, James Morse wrote:
>> Alternatively, I can put the fixmap-page and spinlock in some 'struct
>> ghes_notification' that only the NMI-like struct-ghes need. This is just moving
>> the indirection up a level, but it does pair the lock with the thing it locks,
>> and gets rid of assigning spinlock pointers.
> Keeping the lock and what it protects in one place certainly sounds
> better. I guess you could so something like this:
>
> struct ghes_fixmap {
>   union {
>    raw_spinlock_t nmi_lock;
>     spinlock_t lock;
>   };
>   void __iomem *(map)(struct ghes_fixmap *);
> };
>
> and assign the proper ghes_ioremap function to ->map.
>
> The spin_lock_irqsave() call in ghes_copy_tofrom_phys() is kinda
> questionable. Because we should have disabled interrupts so that you can
> do
>
> spin_lock(map->lock);
>
> Except that we do get called with IRQs on and looking at that call of
> ghes_proc() at the end of ghes_probe(), that's a deadlock waiting to
> happen.
>
> And that comes from:
>
>    77b246b32b2c ("acpi: apei: check for pending errors when probing GHES entries")
>
> Tyler, this can't work in any context: imagine the GHES NMI or IRQ or
> the timer fires while that ghes_proc() runs...
>
> What's up?
Hello Boris,

I haven't seen a deadlock from that, but it looks possible. What if the 
ghes_proc() call in ghes_probe()
is moved before the second switch statement? That way it is before the 
NMI/IRQ/poll is setup. At quick
glance I think that should avoid the deadlock and still provide the 
functionality that call was added for. I
can test that out if you all agree.

Thanks,
Tyler
Borislav Petkov May 17, 2018, 1:36 p.m. UTC | #6
On Wed, May 16, 2018 at 03:51:14PM +0100, James Morse wrote:
> The first two overload existing architectural behavior, the third improves all
> this with a third standard option. Its the standard story!

:-)

> I thought this was safe because its just ghes_copy_tofrom_phys()s access to the
> fixmap slots that needs mutual exclusion.
>
> Polled and all the IRQ flavours are kept apart by the spin_lock_irqsave(), and
> the NMIs have their own fixmap entry. (This is fine until there is more than
> once source of NMI)

For example:

ghes_probe()

	switch (generic->notify.type) {

	...

        case ACPI_HEST_NOTIFY_NMI:
		ghes_nmi_add(ghes);
	}

	...

	ghes_proc();
	  ghes_read_estatus();
		 spin_lock_irqsave(&ghes_ioremap_lock_irq, flags);

		 memcpy...

	-> NMI

		ghes_notify_nmi();
		 ghes_read_estatus();
		 ..
		   if (in_nmi) {
			   raw_spin_lock(&ghes_ioremap_lock_nmi);

		...
	<- NMI

ghes->estatus from above, before the NMI fired, has gotten some nice
scribbling over. AFAICT.

Now, I don't know whether this can happen with the ARM facilities but if
they're NMI-like, I don't see why not.

Which means, that this code is not really reentrant and if should be
fixed to be callable from different contexts, then it should use private
buffers and be careful about locking.

Oh, and that

	if (in_nmi)
		lock()
	else
		lock_irqsave()

pattern is really yucky. And it is an explosion waiting to happen.
Borislav Petkov May 17, 2018, 1:39 p.m. UTC | #7
On Wed, May 16, 2018 at 11:38:16AM -0400, Tyler Baicar wrote:
> I haven't seen a deadlock from that, but it looks possible. What if
> the ghes_proc() call in ghes_probe() is moved before the second switch
> statement? That way it is before the NMI/IRQ/poll is setup. At quick
> glance I think that should avoid the deadlock and still provide the
> functionality that call was added for. I can test that out if you all
> agree.

Makes sense but please audit it properly before doing the change. That
code is full of landmines and could use a proper scrubbing first.

Thx.
James Morse May 17, 2018, 6:11 p.m. UTC | #8
Hi Borislav,

On 17/05/18 14:36, Borislav Petkov wrote:
> On Wed, May 16, 2018 at 03:51:14PM +0100, James Morse wrote:
>> I thought this was safe because its just ghes_copy_tofrom_phys()s access to the
>> fixmap slots that needs mutual exclusion.

and here is where I was wrong: I was only looking at reading the data, we then
dump it in struct ghes assuming it can only be notified on once CPU at a time. Oops.

> For example:

> ghes->estatus from above, before the NMI fired, has gotten some nice
> scribbling over. AFAICT.

Yup, thanks for the example!


> Now, I don't know whether this can happen with the ARM facilities but if
> they're NMI-like, I don't see why not.

NOTIFY_SEA is synchronous so the error has to be something to do with the
instruction that was interrupted. In your example this would mean the APEI
code/data was corrupted, which there is little point trying to handle.

NOTIFY_{SEI, SDEI} on the other hand are asynchronous, so this could happen.


> Which means, that this code is not really reentrant and if should be
> fixed to be callable from different contexts, then it should use private
> buffers and be careful about locking.

... I need to go through this thing again to work out how the firmware-buffers
map on to estatus=>ghes ...


> Oh, and that
> 
> 	if (in_nmi)
> 		lock()
> 	else
> 		lock_irqsave()
> 
> pattern is really yucky. And it is an explosion waiting to happen.

The whole in_nmi()=>other-lock think looks like a hack to make a warning go
away. We could get the notification to take whatever lock is appropriate further
out, but it may mean some code duplication. (I'll put it on my list...)


Thanks,

James
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 1859f27c37ff..48d9eb55ebb8 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -117,12 +117,9 @@  static DEFINE_MUTEX(ghes_list_mutex);
  * from BIOS to Linux can be determined only in NMI, IRQ or timer
  * handler, but general ioremap can not be used in atomic context, so
  * the fixmap is used instead.
- *
- * These 2 spinlocks are used to prevent the fixmap entries from being used
- * simultaneously.
  */
-static DEFINE_RAW_SPINLOCK(ghes_ioremap_lock_nmi);
-static DEFINE_SPINLOCK(ghes_ioremap_lock_irq);
+static DEFINE_RAW_SPINLOCK(ghes_fixmap_lock_nmi);
+static DEFINE_SPINLOCK(ghes_fixmap_lock_irq);
 
 static struct gen_pool *ghes_estatus_pool;
 static unsigned long ghes_estatus_pool_size_request;
@@ -132,38 +129,16 @@  static atomic_t ghes_estatus_cache_alloced;
 
 static int ghes_panic_timeout __read_mostly = 30;
 
-static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
-{
-	phys_addr_t paddr;
-	pgprot_t prot;
-
-	paddr = pfn << PAGE_SHIFT;
-	prot = arch_apei_get_mem_attribute(paddr);
-	__set_fixmap(FIX_APEI_GHES_NMI, paddr, prot);
-
-	return (void __iomem *) fix_to_virt(FIX_APEI_GHES_NMI);
-}
-
-static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
+static void __iomem *ghes_fixmap_pfn(int fixmap_idx, u64 pfn)
 {
 	phys_addr_t paddr;
 	pgprot_t prot;
 
 	paddr = pfn << PAGE_SHIFT;
 	prot = arch_apei_get_mem_attribute(paddr);
-	__set_fixmap(FIX_APEI_GHES_IRQ, paddr, prot);
+	__set_fixmap(fixmap_idx, paddr, prot);
 
-	return (void __iomem *) fix_to_virt(FIX_APEI_GHES_IRQ);
-}
-
-static void ghes_iounmap_nmi(void)
-{
-	clear_fixmap(FIX_APEI_GHES_NMI);
-}
-
-static void ghes_iounmap_irq(void)
-{
-	clear_fixmap(FIX_APEI_GHES_IRQ);
+	return (void __iomem *) __fix_to_virt(fixmap_idx);
 }
 
 static int ghes_estatus_pool_init(void)
@@ -291,10 +266,11 @@  static inline int ghes_severity(int severity)
 	}
 }
 
-static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
-				  int from_phys)
+static void ghes_copy_tofrom_phys(struct ghes *ghes, void *buffer, u64 paddr,
+				  u32 len, int from_phys)
 {
 	void __iomem *vaddr;
+	int fixmap_idx = FIX_APEI_GHES_IRQ;
 	unsigned long flags = 0;
 	int in_nmi = in_nmi();
 	u64 offset;
@@ -303,12 +279,12 @@  static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
 	while (len > 0) {
 		offset = paddr - (paddr & PAGE_MASK);
 		if (in_nmi) {
-			raw_spin_lock(&ghes_ioremap_lock_nmi);
-			vaddr = ghes_ioremap_pfn_nmi(paddr >> PAGE_SHIFT);
+			raw_spin_lock(ghes->nmi_fixmap_lock);
+			fixmap_idx = ghes->nmi_fixmap_idx;
 		} else {
-			spin_lock_irqsave(&ghes_ioremap_lock_irq, flags);
-			vaddr = ghes_ioremap_pfn_irq(paddr >> PAGE_SHIFT);
+			spin_lock_irqsave(&ghes_fixmap_lock_irq, flags);
 		}
+		vaddr = ghes_fixmap_pfn(fixmap_idx, paddr >> PAGE_SHIFT);
 		trunk = PAGE_SIZE - offset;
 		trunk = min(trunk, len);
 		if (from_phys)
@@ -318,13 +294,11 @@  static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
 		len -= trunk;
 		paddr += trunk;
 		buffer += trunk;
-		if (in_nmi) {
-			ghes_iounmap_nmi();
-			raw_spin_unlock(&ghes_ioremap_lock_nmi);
-		} else {
-			ghes_iounmap_irq();
-			spin_unlock_irqrestore(&ghes_ioremap_lock_irq, flags);
-		}
+		clear_fixmap(fixmap_idx);
+		if (in_nmi)
+			raw_spin_unlock(ghes->nmi_fixmap_lock);
+		else
+			spin_unlock_irqrestore(&ghes_fixmap_lock_irq, flags);
 	}
 }
 
@@ -346,7 +320,7 @@  static int ghes_read_estatus(struct ghes *ghes, int silent)
 	if (!buf_paddr)
 		return -ENOENT;
 
-	ghes_copy_tofrom_phys(ghes->estatus, buf_paddr,
+	ghes_copy_tofrom_phys(ghes, ghes->estatus, buf_paddr,
 			      sizeof(*ghes->estatus), 1);
 	if (!ghes->estatus->block_status)
 		return -ENOENT;
@@ -362,7 +336,7 @@  static int ghes_read_estatus(struct ghes *ghes, int silent)
 		goto err_read_block;
 	if (cper_estatus_check_header(ghes->estatus))
 		goto err_read_block;
-	ghes_copy_tofrom_phys(ghes->estatus + 1,
+	ghes_copy_tofrom_phys(ghes, ghes->estatus + 1,
 			      buf_paddr + sizeof(*ghes->estatus),
 			      len - sizeof(*ghes->estatus), 1);
 	if (cper_estatus_check(ghes->estatus))
@@ -381,7 +355,7 @@  static void ghes_clear_estatus(struct ghes *ghes)
 	ghes->estatus->block_status = 0;
 	if (!(ghes->flags & GHES_TO_CLEAR))
 		return;
-	ghes_copy_tofrom_phys(ghes->estatus, ghes->buffer_paddr,
+	ghes_copy_tofrom_phys(ghes, ghes->estatus, ghes->buffer_paddr,
 			      sizeof(ghes->estatus->block_status), 0);
 	ghes->flags &= ~GHES_TO_CLEAR;
 }
@@ -986,6 +960,8 @@  int ghes_notify_sea(void)
 
 static void ghes_sea_add(struct ghes *ghes)
 {
+	ghes->nmi_fixmap_lock = &ghes_fixmap_lock_nmi;
+	ghes->nmi_fixmap_idx = FIX_APEI_GHES_NMI;
 	ghes_estatus_queue_grow_pool(ghes);
 
 	mutex_lock(&ghes_list_mutex);
@@ -1032,6 +1008,8 @@  static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 
 static void ghes_nmi_add(struct ghes *ghes)
 {
+	ghes->nmi_fixmap_lock = &ghes_fixmap_lock_nmi;
+	ghes->nmi_fixmap_idx = FIX_APEI_GHES_NMI;
 	ghes_estatus_queue_grow_pool(ghes);
 
 	mutex_lock(&ghes_list_mutex);
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 8feb0c866ee0..9c6a92730699 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -29,6 +29,10 @@  struct ghes {
 		struct timer_list timer;
 		unsigned int irq;
 	};
+
+	/* If this ghes can be called in NMI contet, these must be populated. */
+	raw_spinlock_t *nmi_fixmap_lock;
+	int nmi_fixmap_idx;
 };
 
 struct ghes_estatus_node {