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

[Narayana Murty N]

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@xxxxxxxxxxxxx/
> >
> > Signed-off-by: Narayana Murty N <nnmlinux@xxxxxxxxxxxxx>
> > ---
> >   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 = {