From: Ahmed Tiba <ahmed.tiba@arm.com>
To: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
Cc: devicetree@vger.kernel.org, linux-acpi@vger.kernel.org,
Dmitry.Lamerov@arm.com, catalin.marinas@arm.com, bp@alien8.de,
robh@kernel.org, rafael@kernel.org, will@kernel.org,
conor@kernel.org, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, krzk+dt@kernel.org,
Michael.Zhao2@arm.com, tony.luck@intel.com
Subject: Re: [PATCH v2 03/11] ACPI: APEI: GHES: move CPER read helpers
Date: Wed, 11 Mar 2026 13:18:28 +0000 [thread overview]
Message-ID: <df306958-bb99-4728-b461-e5ef1f731be2@arm.com> (raw)
In-Reply-To: <CA+Ht8=aL8wp0nwH3vnnm8FnRmfp1nUasfXh3HjeWwi8UadBLOg@mail.gmail.com>
On 26/02/2026 05:58, Himanshu Chauhan wrote:
> On Fri, Feb 20, 2026 at 7:13 PM Ahmed Tiba <ahmed.tiba@arm.com> wrote:
>>
>> Relocate the CPER buffer mapping, peek, and clear helpers from ghes.c into
>> ghes_cper.c so they can be shared with other firmware-first providers.
>> This commit only shuffles code; behavior stays the same.
>>
>> Signed-off-by: Ahmed Tiba <ahmed.tiba@arm.com>
>> ---
>> drivers/acpi/apei/ghes.c | 170 +-----------------------------------------
>> drivers/acpi/apei/ghes_cper.c | 170 +++++++++++++++++++++++++++++++++++++++++-
>> include/acpi/ghes_cper.h | 14 ++--
>> 3 files changed, 177 insertions(+), 177 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 07b70bcb8342..b159dbee90ac 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -118,26 +118,6 @@ static struct gen_pool *ghes_estatus_pool;
>> static struct ghes_estatus_cache __rcu *ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
>> static atomic_t ghes_estatus_cache_alloced;
>>
>> -static void __iomem *ghes_map(u64 pfn, enum fixed_addresses fixmap_idx)
>> -{
>> - phys_addr_t paddr;
>> - pgprot_t prot;
>> -
>> - paddr = PFN_PHYS(pfn);
>> - prot = arch_apei_get_mem_attribute(paddr);
>> - __set_fixmap(fixmap_idx, paddr, prot);
>> -
>> - return (void __iomem *) __fix_to_virt(fixmap_idx);
>> -}
>> -
>> -static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
>> -{
>> - int _idx = virt_to_fix((unsigned long)vaddr);
>> -
>> - WARN_ON_ONCE(fixmap_idx != _idx);
>> - clear_fixmap(fixmap_idx);
>> -}
>> -
>> int ghes_estatus_pool_init(unsigned int num_ghes)
>> {
>> unsigned long addr, len;
>> @@ -193,22 +173,7 @@ static void unmap_gen_v2(struct ghes *ghes)
>> apei_unmap_generic_address(&ghes->generic_v2->read_ack_register);
>> }
>>
>> -static void ghes_ack_error(struct acpi_hest_generic_v2 *gv2)
>> -{
>> - int rc;
>> - u64 val = 0;
>> -
>> - rc = apei_read(&val, &gv2->read_ack_register);
>> - if (rc)
>> - return;
>> -
>> - val &= gv2->read_ack_preserve << gv2->read_ack_register.bit_offset;
>> - val |= gv2->read_ack_write << gv2->read_ack_register.bit_offset;
>> -
>> - apei_write(val, &gv2->read_ack_register);
>> -}
>> -
>> -static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>> +struct ghes *ghes_new(struct acpi_hest_generic *generic)
>> {
>> struct ghes *ghes;
>> unsigned int error_block_length;
>> @@ -255,7 +220,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>> return ERR_PTR(rc);
>> }
>>
>> -static void ghes_fini(struct ghes *ghes)
>> +void ghes_fini(struct ghes *ghes)
>> {
>> kfree(ghes->estatus);
>> apei_unmap_generic_address(&ghes->generic->error_status_address);
>> @@ -280,137 +245,6 @@ static inline int ghes_severity(int severity)
>> }
>> }
>
> Can it be "ghes_finish"? We already have "creat" without 'e'.
This is a pure mechanical move from ghes.c. I’m keeping the original
name here to avoid churn. If we want a rename, I can do that separately
with justification.
>>
>> -static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
>> - int from_phys,
>> - enum fixed_addresses fixmap_idx)
>> -{
>> - void __iomem *vaddr;
>> - u64 offset;
>> - u32 trunk;
>> -
>> - while (len > 0) {
>> - offset = paddr - (paddr & PAGE_MASK);
>> - vaddr = ghes_map(PHYS_PFN(paddr), fixmap_idx);
>> - trunk = PAGE_SIZE - offset;
>> - trunk = min(trunk, len);
>> - if (from_phys)
>> - memcpy_fromio(buffer, vaddr + offset, trunk);
>> - else
>> - memcpy_toio(vaddr + offset, buffer, trunk);
>> - len -= trunk;
>> - paddr += trunk;
>> - buffer += trunk;
>> - ghes_unmap(vaddr, fixmap_idx);
>> - }
>> -}
>> -
>> -/* Check the top-level record header has an appropriate size. */
>> -static int __ghes_check_estatus(struct ghes *ghes,
>> - struct acpi_hest_generic_status *estatus)
>> -{
>> - u32 len = cper_estatus_len(estatus);
>> - u32 max_len = min(ghes->generic->error_block_length,
>> - ghes->estatus_length);
>> -
>> - if (len < sizeof(*estatus)) {
>> - pr_warn_ratelimited(FW_WARN GHES_PFX "Truncated error status block!\n");
>> - return -EIO;
>> - }
>> -
>> - if (!len || len > max_len) {
>> - pr_warn_ratelimited(FW_WARN GHES_PFX "Invalid error status block length!\n");
>> - return -EIO;
>> - }
>> -
>> - if (cper_estatus_check_header(estatus)) {
>> - pr_warn_ratelimited(FW_WARN GHES_PFX "Invalid CPER header!\n");
>> - return -EIO;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> -/* Read the CPER block, returning its address, and header in estatus. */
>> -static int __ghes_peek_estatus(struct ghes *ghes,
>> - struct acpi_hest_generic_status *estatus,
>> - u64 *buf_paddr, enum fixed_addresses fixmap_idx)
>> -{
>> - struct acpi_hest_generic *g = ghes->generic;
>> - int rc;
>> -
>> - rc = apei_read(buf_paddr, &g->error_status_address);
>> - if (rc) {
>> - *buf_paddr = 0;
>> - pr_warn_ratelimited(FW_WARN GHES_PFX
>> -"Failed to read error status block address for hardware error source: %d.\n",
>> - g->header.source_id);
>> - return -EIO;
>> - }
>> - if (!*buf_paddr)
>> - return -ENOENT;
>> -
>> - ghes_copy_tofrom_phys(estatus, *buf_paddr, sizeof(*estatus), 1,
>> - fixmap_idx);
>> - if (!estatus->block_status) {
>> - *buf_paddr = 0;
>> - return -ENOENT;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> -static int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>> - u64 buf_paddr, enum fixed_addresses fixmap_idx,
>> - size_t buf_len)
>> -{
>> - ghes_copy_tofrom_phys(estatus, buf_paddr, buf_len, 1, fixmap_idx);
>> - if (cper_estatus_check(estatus)) {
>> - pr_warn_ratelimited(FW_WARN GHES_PFX
>> - "Failed to read error status block!\n");
>> - return -EIO;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> -static int ghes_read_estatus(struct ghes *ghes,
>> - struct acpi_hest_generic_status *estatus,
>> - u64 *buf_paddr, enum fixed_addresses fixmap_idx)
>> -{
>> - int rc;
>> -
>> - rc = __ghes_peek_estatus(ghes, estatus, buf_paddr, fixmap_idx);
>> - if (rc)
>> - return rc;
>> -
>> - rc = __ghes_check_estatus(ghes, estatus);
>> - if (rc)
>> - return rc;
>> -
>> - return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
>> - cper_estatus_len(estatus));
>> -}
>> -
>> -static void ghes_clear_estatus(struct ghes *ghes,
>> - struct acpi_hest_generic_status *estatus,
>> - u64 buf_paddr, enum fixed_addresses fixmap_idx)
>> -{
>> - estatus->block_status = 0;
>> -
>> - if (!buf_paddr)
>> - return;
>> -
>> - ghes_copy_tofrom_phys(estatus, buf_paddr,
>> - sizeof(estatus->block_status), 0,
>> - fixmap_idx);
>> -
>> - /*
>> - * GHESv2 type HEST entries introduce support for error acknowledgment,
>> - * so only acknowledge the error if this support is present.
>> - */
>> - if (is_hest_type_generic_v2(ghes))
>> - ghes_ack_error(ghes->generic_v2);
>> -}
>>
>> /**
>> * struct ghes_task_work - for synchronous RAS event
>> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
>> index 63047322a3d9..7e0015e960c1 100644
>> --- a/drivers/acpi/apei/ghes_cper.c
>> +++ b/drivers/acpi/apei/ghes_cper.c
>
> IMO, just "cper.c" would be fine.
"cper.c" and "include/acpi/cper.h" already exist under EFI. This code is
GHES‑specific CPER handling (the GHES view of CPER), not a generic UEFI
CPER API, so I’m keeping the GHES‑scoped naming to avoid ambiguity.
>> @@ -1,7 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> *
>> - * APEI GHES CPER helper translation unit - staging file for helper moves
>> + * APEI GHES CPER helper translation unit - code mechanically moved from ghes.c
>> *
>> * Copyright (C) 2026 ARM Ltd.
>> * Author: Ahmed Tiba <ahmed.tiba@arm.com>
>> @@ -17,10 +17,176 @@
>> #include <linux/slab.h>
>>
>> #include <acpi/apei.h>
>> +#include <acpi/ghes_cper.h>
>>
>> #include <asm/fixmap.h>
>> #include <asm/tlbflush.h>
>>
>> #include "apei-internal.h"
>>
>> -/* Helper bodies will be moved here in follow-up commits. */
>> +static void __iomem *ghes_map(u64 pfn, enum fixed_addresses fixmap_idx)
>> +{
>> + phys_addr_t paddr;
>> + pgprot_t prot;
>> +
>> + paddr = PFN_PHYS(pfn);
>> + prot = arch_apei_get_mem_attribute(paddr);
>> + __set_fixmap(fixmap_idx, paddr, prot);
>> +
>> + return (void __iomem *) __fix_to_virt(fixmap_idx);
>> +}
>> +
>> +static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
>> +{
>> + int _idx = virt_to_fix((unsigned long)vaddr);
>> +
>> + WARN_ON_ONCE(fixmap_idx != _idx);
>> + clear_fixmap(fixmap_idx);
>> +}
>> +
>> +static void ghes_ack_error(struct acpi_hest_generic_v2 *gv2)
>> +{
>> + int rc;
>> + u64 val = 0;
>> +
>> + rc = apei_read(&val, &gv2->read_ack_register);
>> + if (rc)
>> + return;
>> +
>> + val &= gv2->read_ack_preserve << gv2->read_ack_register.bit_offset;
>> + val |= gv2->read_ack_write << gv2->read_ack_register.bit_offset;
>> +
>> + apei_write(val, &gv2->read_ack_register);
>> +}
>> +
>> +static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
>> + int from_phys,
>> + enum fixed_addresses fixmap_idx)
>> +{
>> + void __iomem *vaddr;
>> + u64 offset;
>> + u32 trunk;
>> +
>> + while (len > 0) {
>> + offset = paddr - (paddr & PAGE_MASK);
>> + vaddr = ghes_map(PHYS_PFN(paddr), fixmap_idx);
>> + trunk = PAGE_SIZE - offset;
>> + trunk = min(trunk, len);
>> + if (from_phys)
>> + memcpy_fromio(buffer, vaddr + offset, trunk);
>> + else
>> + memcpy_toio(vaddr + offset, buffer, trunk);
>> + len -= trunk;
>> + paddr += trunk;
>> + buffer += trunk;
>> + ghes_unmap(vaddr, fixmap_idx);
>> + }
>> +}
>> +
>> +/* Check the top-level record header has an appropriate size. */
>> +int __ghes_check_estatus(struct ghes *ghes,
>> + struct acpi_hest_generic_status *estatus)
>> +{
>> + u32 len = cper_estatus_len(estatus);
>> + u32 max_len = min(ghes->generic->error_block_length,
>> + ghes->estatus_length);
>> +
>> + if (len < sizeof(*estatus)) {
>> + pr_warn_ratelimited(FW_WARN GHES_PFX "Truncated error status block!\n");
>> + return -EIO;
>> + }
>> +
>> + if (!len || len > max_len) {
>> + pr_warn_ratelimited(FW_WARN GHES_PFX "Invalid error status block length!\n");
>> + return -EIO;
>> + }
>> +
>> + if (cper_estatus_check_header(estatus)) {
>> + pr_warn_ratelimited(FW_WARN GHES_PFX "Invalid CPER header!\n");
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* Read the CPER block, returning its address, and header in estatus. */
>> +int __ghes_peek_estatus(struct ghes *ghes,
>> + struct acpi_hest_generic_status *estatus,
>> + u64 *buf_paddr, enum fixed_addresses fixmap_idx)
>> +{
>> + struct acpi_hest_generic *g = ghes->generic;
>> + int rc;
>> +
>> + rc = apei_read(buf_paddr, &g->error_status_address);
>> + if (rc) {
>> + *buf_paddr = 0;
>> + pr_warn_ratelimited(FW_WARN GHES_PFX
>> +"Failed to read error status block address for hardware error source: %d.\n",
>> + g->header.source_id);
>> + return -EIO;
>> + }
>> + if (!*buf_paddr)
>> + return -ENOENT;
>> +
>> + ghes_copy_tofrom_phys(estatus, *buf_paddr, sizeof(*estatus), 1,
>> + fixmap_idx);
>> + if (!estatus->block_status) {
>> + *buf_paddr = 0;
>> + return -ENOENT;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>> + u64 buf_paddr, enum fixed_addresses fixmap_idx,
>> + size_t buf_len)
>> +{
>> + ghes_copy_tofrom_phys(estatus, buf_paddr, buf_len, 1, fixmap_idx);
>> + if (cper_estatus_check(estatus)) {
>> + pr_warn_ratelimited(FW_WARN GHES_PFX
>> + "Failed to read error status block!\n");
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int ghes_read_estatus(struct ghes *ghes,
>> + struct acpi_hest_generic_status *estatus,
>> + u64 *buf_paddr, enum fixed_addresses fixmap_idx)
>> +{
>> + int rc;
>> +
>> + rc = __ghes_peek_estatus(ghes, estatus, buf_paddr, fixmap_idx);
>> + if (rc)
>> + return rc;
>> +
>> + rc = __ghes_check_estatus(ghes, estatus);
>> + if (rc)
>> + return rc;
>> +
>> + return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
>> + cper_estatus_len(estatus));
>> +}
>> +
>> +void ghes_clear_estatus(struct ghes *ghes,
>> + struct acpi_hest_generic_status *estatus,
>> + u64 buf_paddr, enum fixed_addresses fixmap_idx)
>> +{
>> + estatus->block_status = 0;
>> +
>> + if (!buf_paddr)
>> + return;
>> +
>> + ghes_copy_tofrom_phys(estatus, buf_paddr,
>> + sizeof(estatus->block_status), 0,
>> + fixmap_idx);
>> +
>> + /*
>> + * GHESv2 type HEST entries introduce support for error acknowledgment,
>> + * so only acknowledge the error if this support is present.
>> + */
>> + if (is_hest_type_generic_v2(ghes))
>> + ghes_ack_error(ghes->generic_v2);
>> +}
>> diff --git a/include/acpi/ghes_cper.h b/include/acpi/ghes_cper.h
>> index 2597fbadc4f3..2e3919f0c3e7 100644
>> --- a/include/acpi/ghes_cper.h
>> +++ b/include/acpi/ghes_cper.h
>> @@ -74,21 +74,21 @@ struct ghes_vendor_record_entry {
>> char vendor_record[];
>> };
>>
>
> ditto. "include/acpi/cper.h"
As above.
>> -static struct ghes *ghes_new(struct acpi_hest_generic *generic);
>> -static void ghes_fini(struct ghes *ghes);
>> +struct ghes *ghes_new(struct acpi_hest_generic *generic);
>> +void ghes_fini(struct ghes *ghes);
>>
>> -static int ghes_read_estatus(struct ghes *ghes,
>> +int ghes_read_estatus(struct ghes *ghes,
>> struct acpi_hest_generic_status *estatus,
>> u64 *buf_paddr, enum fixed_addresses fixmap_idx);
>> -static void ghes_clear_estatus(struct ghes *ghes,
>> +void ghes_clear_estatus(struct ghes *ghes,
>> struct acpi_hest_generic_status *estatus,
>> u64 buf_paddr, enum fixed_addresses fixmap_idx);
>> -static int __ghes_peek_estatus(struct ghes *ghes,
>> +int __ghes_peek_estatus(struct ghes *ghes,
>> struct acpi_hest_generic_status *estatus,
>> u64 *buf_paddr, enum fixed_addresses fixmap_idx);
>> -static int __ghes_check_estatus(struct ghes *ghes,
>> +int __ghes_check_estatus(struct ghes *ghes,
>> struct acpi_hest_generic_status *estatus);
>> -static int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>> +int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>> u64 buf_paddr, enum fixed_addresses fixmap_idx,
>> size_t buf_len);
>>
>>
>> --
>> 2.43.0
>>
>>
next prev parent reply other threads:[~2026-03-11 13:19 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-20 13:42 [PATCH v2 00/11] ACPI: APEI: share GHES CPER helpers and add DT FFH provider Ahmed Tiba
2026-02-20 13:42 ` [PATCH v2 01/11] ACPI: APEI: GHES: share macros via a private header Ahmed Tiba
2026-02-24 15:22 ` Jonathan Cameron
2026-03-11 11:39 ` Ahmed Tiba
2026-03-11 12:39 ` Jonathan Cameron
2026-03-11 12:56 ` Ahmed Tiba
2026-02-26 6:44 ` Himanshu Chauhan
2026-03-11 11:55 ` Ahmed Tiba
2026-02-20 13:42 ` [PATCH v2 02/11] ACPI: APEI: GHES: add ghes_cper.o stub Ahmed Tiba
2026-02-24 15:25 ` Jonathan Cameron
2026-03-11 12:19 ` Ahmed Tiba
2026-02-20 13:42 ` [PATCH v2 03/11] ACPI: APEI: GHES: move CPER read helpers Ahmed Tiba
2026-02-24 15:32 ` Jonathan Cameron
2026-03-11 12:38 ` Ahmed Tiba
2026-02-26 5:58 ` Himanshu Chauhan
2026-03-11 13:18 ` Ahmed Tiba [this message]
2026-02-20 13:42 ` [PATCH v2 04/11] ACPI: APEI: GHES: move GHESv2 ack and alloc helpers Ahmed Tiba
2026-02-20 13:42 ` [PATCH v2 05/11] ACPI: APEI: GHES: move estatus cache helpers Ahmed Tiba
2026-02-20 13:42 ` [PATCH v2 06/11] ACPI: APEI: GHES: move vendor record helpers Ahmed Tiba
2026-02-20 13:42 ` [PATCH v2 07/11] ACPI: APEI: GHES: move CXL CPER helpers Ahmed Tiba
2026-02-24 15:34 ` Jonathan Cameron
2026-02-20 13:42 ` [PATCH v2 08/11] ACPI: APEI: introduce GHES helper Ahmed Tiba
2026-02-20 13:42 ` [PATCH v2 09/11] ACPI: APEI: share GHES CPER helpers Ahmed Tiba
2026-02-20 19:19 ` kernel test robot
2026-02-20 19:24 ` kernel test robot
2026-02-20 20:37 ` kernel test robot
2026-02-20 21:16 ` kernel test robot
2026-02-20 13:42 ` [PATCH v2 10/11] dt-bindings: firmware: add arm,ras-ffh Ahmed Tiba
2026-02-26 7:03 ` Himanshu Chauhan
2026-03-11 13:41 ` Ahmed Tiba
2026-02-20 13:42 ` [PATCH v2 11/11] RAS: add DeviceTree firmware-first CPER provider Ahmed Tiba
2026-02-21 9:06 ` Krzysztof Kozlowski
2026-02-23 19:10 ` Ahmed Tiba
2026-02-24 15:55 ` Jonathan Cameron
2026-03-12 12:23 ` Ahmed Tiba
2026-03-12 14:50 ` Jonathan Cameron
2026-02-26 7:01 ` Himanshu Chauhan
2026-02-26 7:05 ` [PATCH v2 00/11] ACPI: APEI: share GHES CPER helpers and add DT FFH provider Himanshu Chauhan
2026-03-11 10:44 ` Ahmed Tiba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=df306958-bb99-4728-b461-e5ef1f731be2@arm.com \
--to=ahmed.tiba@arm.com \
--cc=Dmitry.Lamerov@arm.com \
--cc=Michael.Zhao2@arm.com \
--cc=bp@alien8.de \
--cc=catalin.marinas@arm.com \
--cc=conor@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=himanshu.chauhan@oss.qualcomm.com \
--cc=krzk+dt@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
--cc=tony.luck@intel.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox