Re: [PATCH v4 4/5] powerpc/pseries/eeh: Implement RTAS-based EEH error injection

[Narayana Murty N <nnmlinux@linux.ibm.com>]

On 02/09/26 10:46 AM, Sourabh Jain wrote:
> 
> 
> On 31/08/26 12:24, Narayana Murty N wrote:
>> Replace the legacy MMIO stub in pseries_eeh_err_inject() with a full
>> PAPR-compliant RTAS error injection path using the existing RTAS
>> work-area allocator.
>>
>> The mutex is not a buffer lock;
> Sorry but I didn't get this...
> 
>> it serializes the firmware session
>> open/inject/close sequence as required by PAPR.  No global buffer
>> is allocated or used.
> Nit:
> I think the above para is influenced form old suggestion to not use global
> dedicated buffer. Lets drop the global buffer thing from commit message
> explain just the current approach.
> 
Agreed. I will clean up the commit message and describe only the current
implementation. I will drop the "mutex is not a buffer lock" and "no 
global buffer is used" wording.
>>
>> VFIO EEH error injection exposes a generic userspace ABI.  pSeries maps
>> the generic EEH error types to RTAS ibm,errinjct encodings via
>> pseries_eeh_type_to_rtas().  EEH_ERR_TYPE_32 and EEH_ERR_TYPE_64 are
>> unchanged; their values are defined in arch/powerpc/include/uapi/asm/ 
>> eeh.h
>> and are not renumbered.
>>
>> Tested with corresponding QEMU patches:
>> https://lore.kernel.org/all/20251029150618.186803-1- 
>> nnmlinux@linux.ibm.com/
>>
>> Signed-off-by: Narayana Murty N <nnmlinux@linux.ibm.com>
>> ---
>>   arch/powerpc/platforms/pseries/eeh_pseries.c | 123 ++++++++++++++-----
>>   1 file changed, 95 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/ 
>> powerpc/platforms/pseries/eeh_pseries.c
>> index fafe0004e738..fcb8c560d813 100644
>> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
>> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/rbtree.h>
>>   #include <linux/sched.h>
>>   #include <linux/seq_file.h>
>> +#include <linux/mutex.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/crash_dump.h>
>> @@ -34,6 +35,7 @@
>>   #include <asm/machdep.h>
>>   #include <asm/ppc-pci.h>
>>   #include <asm/rtas.h>
>> +#include <asm/rtas-work-area.h>
>>   /* RTAS tokens */
>>   static int ibm_set_eeh_option;
>> @@ -958,8 +960,6 @@ static int prepare_errinjct_buffer(void *buf, 
>> struct eeh_pe *pe,
>>               return -EINVAL;
>>           if (upper_32_bits(addr) || upper_32_bits(mask)) {
>> -            pr_err("32-bit IOA injection cannot encode addr=%#lx 
>> mask=%#lx\n",
>> -                   addr, mask);
> 
> We are returning -EINVAL remove the error message, what is the need?
> 
Agreed. I will remove this unnecessary error message and just return
-EINVAL.

>>               return -EINVAL;
>>           }
>> @@ -992,50 +992,117 @@ static int prepare_errinjct_buffer(void *buf, 
>> struct eeh_pe *pe,
>>           break;
>>       default:
>> -        pr_err("unsupported RTAS error injection type 0x%x\n", 
>> rtas_type);
>> +        pr_err("unsupported RTAS error injection type 0x%x\n",
>> +               rtas_type);
> 
> Above change is not necessary..
> 
Agreed. I will drop this unrelated formatting-only change.
>>           return -EINVAL;
>>       }
>> -    pr_debug("errinjct buffer ready: rtas_type=0x%x func=%d 
>> addr=0x%lx mask=0x%lx\n",
>> -         rtas_type, func, addr, mask);
> 
> What is need to remove this debug message?
There is no strong reason to remove it. I will keep the existing debug 
message.
> 
>>       return 0;
>>   }
>> +/* pseries-local mutex serializes the open/inject/close RTAS session */
>> +static DEFINE_MUTEX(pseries_errinjct_mutex);
>> +
>>   /**
>>    * pseries_eeh_err_inject - Inject specified error to the indicated PE
>>    * @pe: the indicated PE
>> - * @type: error type
>> - * @func: specific error type
>> - * @addr: address
>> - * @mask: address mask
>> - * The routine is called to inject specified error, which is
>> - * determined by @type and @func, to the indicated PE
>> + * @type: generic EEH error type (EEH_ERR_TYPE_32 or EEH_ERR_TYPE_64)
>> + * @func: specific error function
>> + * @addr: address argument (type-dependent, may be zero)
>> + * @mask: address mask (type-dependent, may be zero)
>> + *
>> + * Implements PAPR-compliant error injection using:
>> + *   ibm,open-errinjct -> ibm,errinjct -> ibm,close-errinjct
>> + *
>> + * A short-lived RTAS work area is allocated per call; no global buffer
>> + * is used.
> Mentioning "no global buffer is used" is not adding any value, I think.
> 
Agreed.
>> pseries_errinjct_mutex serializes the open/inject/close
>> + * session sequence.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>>    */
>>   static int pseries_eeh_err_inject(struct eeh_pe *pe, int type, int 
>> func,
>>                     unsigned long addr, unsigned long mask)
>>   {
>> -    struct    eeh_dev    *pdev;
>> +    struct rtas_work_area *area;
>> +    phys_addr_t area_phys;
>> +    u32 buf_phys;
>> +    void *buf;
>> +    int open_token, errinjct_token, close_token;
>> +    int session_token;
>> +    int rtas_type;
>> +    int close_rc;
>> +    int rc;
>> +
>> +    rc = validate_errinjct_args(pe, type, func, addr, mask);
>> +    if (rc)
>> +        return rc;
>> -    /* Check on PCI error type */
>> -    if (type != EEH_ERR_TYPE_32 && type != EEH_ERR_TYPE_64)
>> +    rtas_type = pseries_eeh_type_to_rtas(type);
>> +    if (rtas_type < 0)
>>           return -EINVAL;
>> -    switch (func) {
>> -    case EEH_ERR_FUNC_LD_MEM_ADDR:
>> -    case EEH_ERR_FUNC_LD_MEM_DATA:
>> -    case EEH_ERR_FUNC_ST_MEM_ADDR:
>> -    case EEH_ERR_FUNC_ST_MEM_DATA:
>> -        /* injects a MMIO error for all pdev's belonging to PE */
>> -        pci_lock_rescan_remove();
>> -        list_for_each_entry(pdev, &pe->edevs, entry)
>> -            eeh_pe_inject_mmio_error(pdev->pdev);
>> -        pci_unlock_rescan_remove();
>> -        break;
>> -    default:
>> -        return -ERANGE;
>> +    open_token    = rtas_function_token(RTAS_FN_IBM_OPEN_ERRINJCT);
>> +    errinjct_token = rtas_function_token(RTAS_FN_IBM_ERRINJCT);
>> +    close_token   = rtas_function_token(RTAS_FN_IBM_CLOSE_ERRINJCT);
>> +
>> +    if (open_token    == RTAS_UNKNOWN_SERVICE ||
>> +        errinjct_token == RTAS_UNKNOWN_SERVICE ||
>> +        close_token   == RTAS_UNKNOWN_SERVICE)
>> +        return -ENODEV;
>> +
>> +    area = rtas_work_area_alloc(RTAS_ERRINJCT_BUF_SIZE);
>> +    buf  = rtas_work_area_raw_buf(area);
>> +    area_phys = rtas_work_area_phys(area);
> 
> 
> I was wondering if there is any need to hold the work area buffer until
> we acquire pseries_errinjct_mutex.
> 
> Would it make sense to allocate the buffer only after acquiring the
> mutex? I suggested an order for the open, errinjct, and close calls
> below, which I think could also help address the above comment.
> 
Agreed. I will reorder the flow as suggested:

1. acquire the mutex
2. call ibm,open-errinjct
3. allocate the RTAS work area
4. populate the work buffer
5. call ibm,errinjct
6. free the RTAS work area
7. call ibm,close-errinjct
8. release the mutex

This avoids allocating the work area if ibm,open-errinjct fails and 
keeps the
work area held only while it is needed.
> Nit: this file is under pseries platform so pseries_errinjct_mutex can 
> renamed to errinct_mutex.
Agreed. I will rename it to errinjct_mutex.
>> +
>> +    if (WARN_ON_ONCE(upper_32_bits(area_phys))) {
>> +        rc = -ERANGE;
>> +        goto out_free_area;
>>       }
>> -    return 0;
>> +    buf_phys = lower_32_bits(area_phys);
> 
> I don't understand the above logic. First, we check area_phys and exit
> early if the address is above 4G, and then we take the lower 32 bits of
> the same address.
> 
> I think the RTAS work area allocation API should be responsible for
> allocating the work area buffer at the right location. The user
> shouldn't have to worry about where exactly the buffer is allocated
> unless they have a specific requirement.
> 
> Is the work area buffer allocated by the API not meeting your
> requirement? If so, could you please explain what the issue is? Let's
> see if we can fix it in the work area allocation API. Otherwise, I
> would suggest removing the checking and truncation done around
> area_phys.
> 
> Let me know your opinion.
> 
Agreed. Since this path uses the RTAS work area allocator, I will remove 
the explicit upper_32_bits(area_phys) check and the local address 
truncation logic. If the allocator does not provide an RTAS-suitable 
address, that should be handled in the allocator rather than in this EEH 
path.
> 
>> +
>> +    rc = prepare_errinjct_buffer(buf, pe, rtas_type, func, addr, mask);
>> +    if (rc)
>> +        goto out_free_area;
>> +
>> +    mutex_lock(&pseries_errinjct_mutex);
>> +
>> +    do {
>> +        rc = rtas_call(open_token, 0, 2, &session_token);
>> +    } while (rtas_busy_delay(rc));
>> +
>> +    if (rc) {
>> +        pr_err("ibm,open-errinjct failed: status=%d\n", rc);
>> +        rc = rtas_error_rc(rc);
>> +        goto out_unlock;
>> +    }
> 
> I think we should allocate the work area buffer only after the
> open-errinjct call succeeds. My preferred order is:
> 
> - Call RTAS open-errinjct
> - Allocate the work area buffer
> - Populate the work area buffer and make the errinjct RTAS call
> - Release the work area buffer
> - Call RTAS close-errinjct
> 
As stated above, will address in next version.
> This way, the work area buffer is held only for as long as it is needed,
> and we also avoid allocating it if open-errinjct fails.
Agreed.
> 
>> +
>> +    do {
>> +        rc = rtas_call(errinjct_token, 3, 1, NULL,
>> +                   rtas_type, session_token, buf_phys);
>> +    } while (rtas_busy_delay(rc));
>> +
>> +    if (rc) {
>> +        pr_err("ibm,errinjct failed: status=%d\n", rc);
>> +        rc = rtas_error_rc(rc);
>> +    }
>> +
>> +    do {
>> +        close_rc = rtas_call(close_token, 1, 1, NULL, session_token);
>> +    } while (rtas_busy_delay(close_rc));
>> +
>> +    if (close_rc) {
>> +        pr_warn("ibm,close-errinjct failed: status=%d\n", close_rc);
>> +        if (!rc)
>> +            rc = rtas_error_rc(close_rc);
>> +    }
>> +
>> +out_unlock:
>> +    mutex_unlock(&pseries_errinjct_mutex);
>> +
>> +out_free_area:
>> +    rtas_work_area_free(area);
>> +    return rc;
>>   }
>>   static struct eeh_ops pseries_eeh_ops = {
>