Re: [PATCH 1/3] ACPI: APEI: GHES: Improve ghes_notify_nmi() status check

[Shuai Xue <xueshuai@linux.alibaba.com>]

Hi, Hanjun

On 12/17/25 9:13 AM, Hanjun Guo wrote:
> Hi Shuai,
> 
> Some minor comments inline.
> 
> On 2025/12/3 21:02, Shuai Xue wrote:
>> From: Tony Luck <tony.luck@intel.com>
>>
>> ghes_notify_nmi() is called for every NMI and must check whether the NMI was
>> generated because an error was signalled by platform firmware.
>>
>> This check is very expensive as for each registered GHES NMI source it reads
>> from the acpi generic address attached to this error source to get the physical
>> address of the acpi_hest_generic_status block.  It then checks the "block_status"
>> to see if an error was logged.
>>
>> The ACPI/APEI code must create virtual mappings for each of those physical
>> addresses, and tear them down afterwards. On an Icelake system this takes around
>> 15,000 TSC cycles. Enough to disturb efforts to profile system performance.
>>
>> If that were not bad enough, there are some atomic accesses in the code path
>> that will cause cache line bounces between CPUs. A problem that gets worse as
>> the core count increases.
>>
>> But BIOS changes neither the acpi generic address nor the physical address of
>> the acpi_hest_generic_status block. So this walk can be done once when the NMI is
>> registered to save the virtual address (unmapping if the NMI is ever unregistered).
>> The "block_status" can be checked directly in the NMI handler. This can be done
>> without any atomic accesses.
>>
>> Resulting time to check that there is not an error record is around 900 cycles.
>>
>> Reported-by: Andi Kleen <andi.kleen@intel.com>
>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>> ---
>>   drivers/acpi/apei/ghes.c | 39 ++++++++++++++++++++++++++++++++++++---
>>   include/acpi/ghes.h      |  1 +
>>   2 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 97ee19f2cae0..62713b612865 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -1425,7 +1425,21 @@ static LIST_HEAD(ghes_nmi);
>>   static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
>>   {
>>       static DEFINE_RAW_SPINLOCK(ghes_notify_lock_nmi);
>> +    bool active_error = false;
>>       int ret = NMI_DONE;
>> +    struct ghes *ghes;
>> +
>> +    rcu_read_lock();
>> +    list_for_each_entry_rcu(ghes, &ghes_nmi, list) {
>> +        if (ghes->error_status_vaddr && readl(ghes->error_status_vaddr)) {
>> +            active_error = true;
>> +            break;
>> +        }
>> +    }
>> +    rcu_read_unlock();
>> +
>> +    if (!active_error)
>> +        return ret;
>>       if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
>>           return ret;
>> @@ -1439,13 +1453,26 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
>>       return ret;
>>   }
>> -static void ghes_nmi_add(struct ghes *ghes)
>> +static int ghes_nmi_add(struct ghes *ghes)
>>   {
>> +    struct acpi_hest_generic *g = ghes->generic;
>> +    u64 paddr;
>> +    int rc;
>> +
>> +    rc = apei_read(&paddr, &g->error_status_address);
>> +    if (rc)
>> +        return rc;
> 
> It will be good to add a empty line here.

Sure, will fix it.

> 
>> +    ghes->error_status_vaddr = acpi_os_ioremap(paddr, sizeof(ghes->estatus->block_status));
>> +    if (!ghes->error_status_vaddr)
>> +        return AE_BAD_ADDRESS;
> 
> It's static int for ghes_nmi_add(), and AE_BAD_ADDRESS is the type of
> acpi_status, it's better to return -EINVAL here.

Thanks for pointing it out, will fix it.

> 
> Thanks
> Hanjun

Thanks.
Shuai