Hi Sourabh, On 04/08/26 11:35 PM, Sourabh Jain wrote:
These helpers were intended to validate the userspace-provided EEH error injection parameters before encoding them into the ibm,errinjct RTAS work buffer. They check whether the requested generic EEH error type is supported by the pSeries RTAS backend, whether the function code is valid for the requested error type, and whether an address-scoped IOA error refers to an address that belongs to the target PE.On 21/07/26 09:08, Narayana Murty N wrote:Add validation helpersCan you provide more details about what these validation helpers are for?
The buffer is the RTAS parameter/work buffer passed to ibm,errinjct. The kernel fills this buffer according to the PAPR layout for the requested error type, then passes the RTAS-visible physical address of the buffer to firmware.and buffer-preparation infrastructure used by the RTAS-based EEH error injection implementation.Can you explain what this buffer is and what it is used for?
In v4 this no longer uses a dedicated global rtas_errinjct_buf.
Changes relative to the original submission: - pr_fmt unconditionally defined at the top of the file, not wrapped in an #ifndef guard. - RTAS error-type constants (RTAS_ERR_TYPE_*) are defined as file-local macros in eeh_pseries.c, not in the UAPI header. They are internal PAPR firmware type codes, not generic EEH ABI values. - validate_corrupted_page() drops the unused 'pe' parameter entirely (no __maybe_unused). The mask value is silently accepted since it is not meaningful for this error type. - validate_ioa_bus_error() is removed; callers invoke validate_addr_mask_in_pe() directly. - validate_err_type() and validate_special_event() and validate_corrupted_page() are marked static inline. - prepare_errinjct_buffer() takes an explicit 'void *buf' pointer (kernel virtual address) instead of accessing rtas_errinjct_buf directly, making it easier to test and keeping the global out of the helper. - kernel-doc added for prepare_errinjct_buffer() including a Locking: line stating the caller must hold rtas_errinjct_mutex.Changelog shouldn't be part of commit message.
Agreed.
Agreed. I changed the local helpers in v4 to return normal Linux error codes such as -EINVAL or -EOPNOTSUPP, instead of returning RTAS status codes directly.Reported-by: kernel test robot <[email protected]>Closes: https://lore.kernel.org/oe-kbuild-all/202512101130.EYUo0oZx- [email protected]/Signed-off-by: Narayana Murty N <[email protected]> --- arch/powerpc/include/uapi/asm/eeh.h | 24 +- arch/powerpc/platforms/pseries/eeh_pseries.c | 250 +++++++++++++++++++ 2 files changed, 271 insertions(+), 3 deletions(-)diff --git a/arch/powerpc/include/uapi/asm/eeh.h b/arch/powerpc/ include/uapi/asm/eeh.hindex 3b5c47ff3fc4..2680b22f8917 100644 --- a/arch/powerpc/include/uapi/asm/eeh.h +++ b/arch/powerpc/include/uapi/asm/eeh.h @@ -15,9 +15,27 @@ #define EEH_PE_STATE_STOPPED_DMA 4 /* Stopped DMA only */ #define EEH_PE_STATE_UNAVAIL 5 /* Unavailable */ -/* EEH error types and functions */ -#define EEH_ERR_TYPE_32 0 /* 32-bits error */ -#define EEH_ERR_TYPE_64 1 /* 64-bits error */ +/* + * EEH error types. + *+ * EEH_ERR_TYPE_32 and EEH_ERR_TYPE_64 are the original ABI values and must + * not be renumbered. The additional types below are generic identifiers for + * error-injection types supported by pSeries RTAS. Platform backends may + * return -EOPNOTSUPP for valid generic types that are not supported by their+ * firmware. + */ +#define EEH_ERR_TYPE_32 0 /* 32-bits error */ +#define EEH_ERR_TYPE_64 1 /* 64-bits error */ +#define EEH_ERR_TYPE_RECOVERED_SPECIAL_EVENT 0x03 +#define EEH_ERR_TYPE_CORRUPTED_PAGE 0x04 +#define EEH_ERR_TYPE_CORRUPTED_DCACHE_START 0x09 +#define EEH_ERR_TYPE_CORRUPTED_DCACHE_END 0x0a +#define EEH_ERR_TYPE_CORRUPTED_ICACHE_START 0x0b +#define EEH_ERR_TYPE_CORRUPTED_ICACHE_END 0x0c +#define EEH_ERR_TYPE_CORRUPTED_TLB_START 0x0d +#define EEH_ERR_TYPE_CORRUPTED_TLB_END 0x0e + +/* EEH error functions */ #define EEH_ERR_FUNC_MIN 0 #define EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */ #define EEH_ERR_FUNC_LD_MEM_DATA 1diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/ powerpc/platforms/pseries/eeh_pseries.cindex b12ef382fec7..25aad86c696d 100644 --- a/arch/powerpc/platforms/pseries/eeh_pseries.c +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c @@ -1,4 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-or-later +#define pr_fmt(fmt) "EEH: " fmt /** The file intends to implement the platform dependent EEH operations on pseries. * Actually, the pseries platform is built based on RTAS heavily. That means the@@ -31,8 +32,27 @@ #include <asm/io.h> #include <asm/machdep.h> #include <asm/ppc-pci.h> +#include <linux/mutex.h> #include <asm/rtas.h> +/* + * PAPR RTAS firmware error-injection type codes. + * + * These are internal firmware encodings used by ibm,errinjct. They are + * NOT generic EEH ABI values. Only the types actually handled by + * prepare_errinjct_buffer() are defined here. + */ +#define RTAS_ERR_TYPE_RECOVERED_SPECIAL_EVENT 0x03 +#define RTAS_ERR_TYPE_CORRUPTED_PAGE 0x04 +#define RTAS_ERR_TYPE_IOA_BUS_ERROR 0x07 +#define RTAS_ERR_TYPE_CORRUPTED_DCACHE_START 0x09 +#define RTAS_ERR_TYPE_CORRUPTED_DCACHE_END 0x0a +#define RTAS_ERR_TYPE_CORRUPTED_ICACHE_START 0x0b +#define RTAS_ERR_TYPE_CORRUPTED_ICACHE_END 0x0c +#define RTAS_ERR_TYPE_CORRUPTED_TLB_START 0x0d +#define RTAS_ERR_TYPE_CORRUPTED_TLB_END 0x0e +#define RTAS_ERR_TYPE_IOA_BUS_ERROR_64 0x0f + /* RTAS tokens */ static int ibm_set_eeh_option; static int ibm_set_slot_reset;@@ -786,6 +806,236 @@ static int pseries_notify_resume(struct eeh_dev *edev)} #endif +/**+ * validate_addr_mask_in_pe() - Validate that addr+mask fall within PE BARs+ * @pe: EEH PE containing one or more PCI devices + * @addr: Address to validate + * @mask: Address mask to validate + * + * Checks that @addr is mapped into a BAR/MMIO region of any device + * belonging to the PE. If @mask is non-zero, ensures it is consistent + * with @addr. + * + * Return: 0 if valid, RTAS_INVALID_PARAMETER on failure.Using RTAS error codes for kernel function may not be good idea. How about using -EINVAL?
That check was intended to reject an address/mask pair that did not preserve the requested address bits, but I agree it was not really a BAR membership check and did not belong inside the BAR scanning loop.+ */+static int validate_addr_mask_in_pe(struct eeh_pe *pe, unsigned long addr,+ unsigned long mask) +{ + struct eeh_dev *edev, *tmp; + struct pci_dev *pdev; + int bar; + resource_size_t bar_start, bar_len; + bool valid = false; + + /* nothing to validate */ + if (addr == 0 && mask == 0) + return 0; + + eeh_pe_for_each_dev(pe, edev, tmp) { + pdev = eeh_dev_to_pci_dev(edev); + if (!pdev) + continue; + + for (bar = 0; bar < PCI_NUM_RESOURCES; bar++) { + bar_start = pci_resource_start(pdev, bar); + bar_len = pci_resource_len(pdev, bar); + + if (!bar_len) + continue; + + if (addr >= bar_start && addr < (bar_start + bar_len)) { + if ((addr & mask) != addr) {The check has nothing to do with device or the BAR. Then why this check is inside the loop?
No, that was not intentional. Once a matching BAR is found there is no need to keep scanning. I fixed this in v4 so the helper returns immediately after finding a valid BAR match.For my understanding can you please explain what this check is all about?+ pr_err("Mask 0x%lx invalid for addr 0x%lx in BAR[%d] range 0x%llx-0x%llx\n",+ mask, addr, bar, + (unsigned long long)bar_start, + (unsigned long long)(bar_start + bar_len)); + return RTAS_INVALID_PARAMETER; + }+ pr_debug("addr=0x%lx mask=0x%lx validated in BAR[%d] of %s\n",+ addr, mask, bar, pci_name(pdev)); + valid = true;Once valid is set, the loop keeps scanning all remaining BARs/devices instead of breaking. Is it intentional?
+ } + } + } + + if (!valid) { + pr_err("addr=0x%lx not within any BAR of any device in PE\n", + addr); + return RTAS_INVALID_PARAMETER;-EINVAL would be a better options for these helpers.
Agreed. Thanks, Narayana
+ } + + return 0; +} + +/**+ * validate_special_event() - Validate parameters for special-event injection+ * @addr: Address parameter (must be zero for this type) + * @mask: Mask parameter (must be zero for this type) + * + * Return: 0 if valid, RTAS_INVALID_PARAMETER otherwise. + */+static inline int validate_special_event(unsigned long addr, unsigned long mask)+{ + if (addr || mask) { + pr_err("special-event injection must not specify addr/mask\n"); + return RTAS_INVALID_PARAMETER; + } + return 0; +} + +/**+ * validate_corrupted_page() - Validate parameters for corrupted-page injection+ * @addr: Physical page address (required, must be non-zero) + * + * The mask value is not meaningful for this error type and is ignored. + * + * Return: 0 if valid, RTAS_INVALID_PARAMETER otherwise. + */ +static inline int validate_corrupted_page(unsigned long addr) +{ + if (!addr) { + pr_err("corrupted-page injection requires non-zero addr\n"); + return RTAS_INVALID_PARAMETER; + } + return 0; +} + +/**+ * pseries_eeh_type_to_rtas() - Map a generic EEH error type to its RTAS encoding.+ * @type: Generic EEH error type (EEH_ERR_TYPE_*) + *+ * Translates a userspace-visible generic EEH error type to the corresponding + * PAPR RTAS firmware type code. Every supported value appears as an explicit + * case; coincidental equality between generic EEH and RTAS values is not relied+ * upon. + *+ * Return: RTAS_ERR_TYPE_* value on success, -EINVAL for unsupported types.+ */ +static int pseries_eeh_type_to_rtas(int type) +{ + switch (type) { + case EEH_ERR_TYPE_32: + return RTAS_ERR_TYPE_IOA_BUS_ERROR; + case EEH_ERR_TYPE_64: + return RTAS_ERR_TYPE_IOA_BUS_ERROR_64; + case EEH_ERR_TYPE_RECOVERED_SPECIAL_EVENT: + return RTAS_ERR_TYPE_RECOVERED_SPECIAL_EVENT; + case EEH_ERR_TYPE_CORRUPTED_PAGE: + return RTAS_ERR_TYPE_CORRUPTED_PAGE; + case EEH_ERR_TYPE_CORRUPTED_DCACHE_START: + return RTAS_ERR_TYPE_CORRUPTED_DCACHE_START; + case EEH_ERR_TYPE_CORRUPTED_DCACHE_END: + return RTAS_ERR_TYPE_CORRUPTED_DCACHE_END; + case EEH_ERR_TYPE_CORRUPTED_ICACHE_START: + return RTAS_ERR_TYPE_CORRUPTED_ICACHE_START; + case EEH_ERR_TYPE_CORRUPTED_ICACHE_END: + return RTAS_ERR_TYPE_CORRUPTED_ICACHE_END; + case EEH_ERR_TYPE_CORRUPTED_TLB_START: + return RTAS_ERR_TYPE_CORRUPTED_TLB_START; + case EEH_ERR_TYPE_CORRUPTED_TLB_END: + return RTAS_ERR_TYPE_CORRUPTED_TLB_END; + default: + return -EINVAL; + } +} + +/** + * prepare_errinjct_buffer() - Build ibm,errinjct work buffer + * @buf: RTAS error-injection work buffer (kernel virtual address) + * @pe: EEH PE associated with the injection target+ * @rtas_type: PAPR firmware error-injection type after generic EEH- to-RTAS+ * translation (RTAS_ERR_TYPE_*) + * @func: Error function selector + * @addr: Target address, if applicable + * @mask: Address mask, if applicable + *+ * Zeroes @buf and populates it according to the PAPR layout for @rtas_type.+ * Performs inline parameter validation for each error type. + * + * Locking: Caller must hold rtas_errinjct_mutex. + * + * Return: 0 on success, RTAS_INVALID_PARAMETER on invalid input. + */ +static int prepare_errinjct_buffer(void *buf, struct eeh_pe *pe, + int rtas_type, int func, + unsigned long addr, unsigned long mask) +{ + __be64 *buf64 = (__be64 *)buf; + __be32 *buf32 = (__be32 *)buf; + + memset(buf, 0, RTAS_ERRINJCT_BUF_SIZE); + + switch (rtas_type) { + case RTAS_ERR_TYPE_RECOVERED_SPECIAL_EVENT: + /* func: 1 = non-persistent, 2 = persistent */ + if (func < 1 || func > 2) + return RTAS_INVALID_PARAMETER; + if (validate_special_event(addr, mask)) + return RTAS_INVALID_PARAMETER; + buf32[0] = cpu_to_be32(func); + break; + + case RTAS_ERR_TYPE_CORRUPTED_PAGE: + if (validate_corrupted_page(addr)) + return RTAS_INVALID_PARAMETER; + buf32[0] = cpu_to_be32(upper_32_bits(addr)); + buf32[1] = cpu_to_be32(lower_32_bits(addr)); + break; + + case RTAS_ERR_TYPE_IOA_BUS_ERROR: + if (func < EEH_ERR_FUNC_LD_MEM_ADDR || func > EEH_ERR_FUNC_MAX) + return RTAS_INVALID_PARAMETER; + if (upper_32_bits(addr) || upper_32_bits(mask)) {+ pr_err("32-bit IOA injection cannot encode addr=%#lx mask=%#lx\n",+ addr, mask); + return RTAS_INVALID_PARAMETER; + } + if (validate_addr_mask_in_pe(pe, addr, mask)) + return RTAS_INVALID_PARAMETER; + buf32[0] = cpu_to_be32((u32)addr); + buf32[1] = cpu_to_be32((u32)mask); + buf32[2] = cpu_to_be32(pe->addr); + buf32[3] = cpu_to_be32(BUID_HI(pe->phb->buid)); + buf32[4] = cpu_to_be32(BUID_LO(pe->phb->buid)); + buf32[5] = cpu_to_be32(func); + break; + + case RTAS_ERR_TYPE_IOA_BUS_ERROR_64: + if (func < EEH_ERR_FUNC_MIN || func > EEH_ERR_FUNC_MAX) + return RTAS_INVALID_PARAMETER; + if (validate_addr_mask_in_pe(pe, addr, mask)) + return RTAS_INVALID_PARAMETER; + buf64[0] = cpu_to_be64(addr); + buf64[1] = cpu_to_be64(mask); + buf32[4] = cpu_to_be32(pe->addr); + buf32[5] = cpu_to_be32(BUID_HI(pe->phb->buid)); + buf32[6] = cpu_to_be32(BUID_LO(pe->phb->buid)); + buf32[7] = cpu_to_be32(func); + break; + + case RTAS_ERR_TYPE_CORRUPTED_DCACHE_START: + case RTAS_ERR_TYPE_CORRUPTED_DCACHE_END: + case RTAS_ERR_TYPE_CORRUPTED_ICACHE_START: + case RTAS_ERR_TYPE_CORRUPTED_ICACHE_END: + buf32[0] = cpu_to_be32(lower_32_bits(addr)); + buf32[1] = cpu_to_be32(lower_32_bits(mask)); + break; + + case RTAS_ERR_TYPE_CORRUPTED_TLB_START: + case RTAS_ERR_TYPE_CORRUPTED_TLB_END: + buf32[0] = cpu_to_be32(lower_32_bits(addr)); + break; + + default:+ pr_err("unsupported RTAS error injection type 0x%x\n", rtas_type);+ return RTAS_INVALID_PARAMETER; + } ++ pr_debug("errinjct buffer ready: rtas_type=0x%x func=%d addr=0x%lx mask=0x%lx\n",+ rtas_type, func, addr, mask); + return 0; +} + /** * pseries_eeh_err_inject - Inject specified error to the indicated PE * @pe: the indicated PE
