diff mbox

[v16,1/2] sPAPR: Implement EEH RTAS calls

Message ID 1423016856-11256-2-git-send-email-gwshan@linux.vnet.ibm.com
State New
Headers show

Commit Message

Gavin Shan Feb. 4, 2015, 2:27 a.m. UTC
The emulation for EEH RTAS requests from guest isn't covered
by QEMU yet and the patch implements them.

The patch defines constants used by EEH RTAS calls and adds
callback sPAPRPHBClass::eeh_handler, which is going to be used
this way:

  * RTAS calls are received in spapr_pci.c, sanity check is done
    there.
  * RTAS handlers handle what they can. If there is something it
    cannot handle and sPAPRPHBClass::eeh_handler callback is defined,
    it is called.
  * sPAPRPHBClass::eeh_handler is only implemented for VFIO now. It
    does ioctl() to the IOMMU container fd to complete the call. Error
    codes from that ioctl() are transferred back to the guest.

[aik: defined RTAS tokens for EEH RTAS calls]
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c          | 310 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/spapr.h |   7 +
 include/hw/ppc/spapr.h      |  43 +++++-
 3 files changed, 358 insertions(+), 2 deletions(-)

Comments

David Gibson Feb. 5, 2015, 3:19 a.m. UTC | #1
On Wed, Feb 04, 2015 at 01:27:35PM +1100, Gavin Shan wrote:
> The emulation for EEH RTAS requests from guest isn't covered
> by QEMU yet and the patch implements them.
> 
> The patch defines constants used by EEH RTAS calls and adds
> callback sPAPRPHBClass::eeh_handler, which is going to be used
> this way:
> 
>   * RTAS calls are received in spapr_pci.c, sanity check is done
>     there.
>   * RTAS handlers handle what they can. If there is something it
>     cannot handle and sPAPRPHBClass::eeh_handler callback is defined,
>     it is called.
>   * sPAPRPHBClass::eeh_handler is only implemented for VFIO now. It
>     does ioctl() to the IOMMU container fd to complete the call. Error
>     codes from that ioctl() are transferred back to the guest.
> 
> [aik: defined RTAS tokens for EEH RTAS calls]
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c          | 310 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/spapr.h |   7 +
>  include/hw/ppc/spapr.h      |  43 +++++-
>  3 files changed, 358 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 6deeb19..3fac5a9 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -406,6 +406,297 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>  }
>  
> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> +                                    sPAPREnvironment *spapr,
> +                                    uint32_t token, uint32_t nargs,
> +                                    target_ulong args, uint32_t nret,
> +                                    target_ulong rets)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *spc;
> +    uint32_t addr, option;
> +    uint64_t buid;
> +    int ret;
> +
> +    if ((nargs != 4) || (nret != 1)) {
> +        goto param_error_exit;
> +    }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> +    addr = rtas_ld(args, 0);
> +    option = rtas_ld(args, 3);
> +
> +    sphb = find_phb(spapr, buid);
> +    if (!sphb) {
> +        goto param_error_exit;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!spc->eeh_handler) {

Should this case be RTAS_OUT_PARAM_ERROR or RTAS_OUT_NOT_SUPPORTED?

> +        goto param_error_exit;
> +    }
> +
> +    switch (option) {
> +    case RTAS_EEH_ENABLE:
> +        if (!find_dev(spapr, buid, addr)) {
> +            goto param_error_exit;
> +        }
> +        break;
> +    case RTAS_EEH_DISABLE:
> +    case RTAS_EEH_THAW_IO:
> +    case RTAS_EEH_THAW_DMA:
> +        break;
> +    default:
> +        goto param_error_exit;
> +    }
> +
> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_SET_OPTION, option);
> +    if (ret < 0) {
> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);

It seems like it would make sense for the eeh_handler callback to
return rtas error codes, so that if it makes sense it can signal
errors other than RTAS_OUT_HW_ERROR.

> +        return;
> +    }
> +
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    return;
> +
> +param_error_exit:
> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +}
> +
> +static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
> +                                           sPAPREnvironment *spapr,
> +                                           uint32_t token, uint32_t nargs,
> +                                           target_ulong args, uint32_t nret,
> +                                           target_ulong rets)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *spc;
> +    PCIDevice *pdev;
> +    uint32_t addr, option;
> +    uint64_t buid;
> +
> +    if ((nargs != 4) || (nret != 2)) {
> +        goto param_error_exit;
> +    }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> +    sphb = find_phb(spapr, buid);
> +    if (!sphb) {
> +        goto param_error_exit;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!spc->eeh_handler) {
> +        goto param_error_exit;
> +    }
> +
> +    addr = rtas_ld(args, 0);
> +    option = rtas_ld(args, 3);
> +    if (option != RTAS_GET_PE_ADDR && option != RTAS_GET_PE_MODE) {
> +        goto param_error_exit;
> +    }
> +
> +    pdev = find_dev(spapr, buid, addr);
> +    if (!pdev) {
> +        goto param_error_exit;
> +    }
> +
> +    /*
> +     * For now, we always have bus level PE whose address
> +     * has format "00BBSS00". The guest OS might regard
> +     * PE address 0 as invalid. We avoid that simply by
> +     * extending it with one.

This comment is a bit confusing.  Why not just say we have PE
addresses of form "00BBSS01".  Also you should say what the BB and
SS mean in your template.

> +     */
> +    if (option == RTAS_GET_PE_ADDR) {

I think this would be clearer as a switch statement, which folds in
the other if statement above.

> +        rtas_st(rets, 1, (pci_bus_num(pdev->bus) << 16) + 1);
> +    } else {
> +        rtas_st(rets, 1, RTAS_PE_MODE_SHARED);
> +    }
> +
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    return;
> +
> +param_error_exit:
> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +}
> +
> +static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
> +                                            sPAPREnvironment *spapr,
> +                                            uint32_t token, uint32_t nargs,
> +                                            target_ulong args, uint32_t nret,
> +                                            target_ulong rets)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *spc;
> +    uint64_t buid;
> +    int ret;
> +
> +    if ((nargs != 3) || (nret != 4 && nret != 5)) {
> +        goto param_error_exit;
> +    }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> +    sphb = find_phb(spapr, buid);
> +    if (!sphb) {
> +        goto param_error_exit;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!spc->eeh_handler) {
> +        goto param_error_exit;
> +    }
> +
> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_GET_STATE, 0);
> +    if (ret < 0) {
> +        goto param_error_exit;

So here, all errors from eeh_handler are turned into
RTAS_OUT_PARAM_ERROR, whereas above they were all turned into
RTAS_OUT_HW_ERROR.

> +    }
> +
> +    rtas_st(rets, 1, ret);
> +    rtas_st(rets, 2, RTAS_EEH_SUPPORT);
> +    rtas_st(rets, 3, RTAS_EEH_PE_UNAVAIL_INFO);
> +    if (nret >= 5) {
> +        rtas_st(rets, 4, RTAS_EEH_PE_RECOVER_INFO);
> +    }
> +
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    return;
> +
> +param_error_exit:
> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +}
> +
> +static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
> +                                    sPAPREnvironment *spapr,
> +                                    uint32_t token, uint32_t nargs,
> +                                    target_ulong args, uint32_t nret,
> +                                    target_ulong rets)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *spc;
> +    uint32_t option;
> +    uint64_t buid;
> +    int ret;
> +
> +    if ((nargs != 4) || (nret != 1)) {
> +        goto param_error_exit;
> +    }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> +    option = rtas_ld(args, 3);
> +    sphb = find_phb(spapr, buid);
> +    if (!sphb) {
> +        goto param_error_exit;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!spc->eeh_handler) {
> +        goto param_error_exit;
> +    }
> +
> +    switch (option) {
> +    case RTAS_SLOT_RESET_DEACTIVATE:
> +    case RTAS_SLOT_RESET_HOT:
> +    case RTAS_SLOT_RESET_FUNDAMENTAL:
> +        break;
> +    default:
> +        goto param_error_exit;
> +    }
> +
> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_RESET, option);
> +    if (ret < 0) {
> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        return;
> +    }
> +
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    return;
> +
> +param_error_exit:
> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +}
> +
> +static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
> +                                  sPAPREnvironment *spapr,
> +                                  uint32_t token, uint32_t nargs,
> +                                  target_ulong args, uint32_t nret,
> +                                  target_ulong rets)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *spc;
> +    uint64_t buid;
> +    int ret;
> +
> +    if ((nargs != 3) || (nret != 1)) {
> +        goto param_error_exit;
> +    }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> +    sphb = find_phb(spapr, buid);
> +    if (!sphb) {
> +        goto param_error_exit;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!spc->eeh_handler) {
> +        goto param_error_exit;
> +    }
> +
> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_CONFIGURE, 0);
> +    if (ret < 0) {
> +        goto param_error_exit;
> +    }
> +
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    return;
> +
> +param_error_exit:
> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +}
> +
> +/* To support it later */
> +static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
> +                                       sPAPREnvironment *spapr,
> +                                       uint32_t token, uint32_t nargs,
> +                                       target_ulong args, uint32_t nret,
> +                                       target_ulong rets)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *spc;
> +    int option;
> +    uint64_t buid;
> +
> +    if ((nargs != 8) || (nret != 1)) {
> +        goto no_error_log;

Surely this should be RTAS_OUT_PARAM_ERROR, not RTAS_OUT_NO_ERRORS_FOUND.

> +    }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> +    sphb = find_phb(spapr, buid);
> +    if (!sphb) {
> +        goto no_error_log;

.. and this one..

> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!spc->eeh_handler) {
> +        goto no_error_log;

.. possibly not this one ..

> +    }
> +
> +    option = rtas_ld(args, 7);
> +    switch (option) {
> +    case RTAS_SLOT_TEMP_ERR_LOG:
> +    case RTAS_SLOT_PERM_ERR_LOG:
> +        break;
> +    default:
> +        goto no_error_log;

.. but again this one ..

> +    }
> +
> +    /* We don't get error log yet */
> +    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
> +    return;
> +
> +no_error_log:
> +    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
> +}
> +
>  static int pci_spapr_swizzle(int slot, int pin)
>  {
>      return (slot + pin) % PCI_NUM_PINS;
> @@ -964,6 +1255,25 @@ void spapr_pci_rtas_init(void)
>          spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi",
>                              rtas_ibm_change_msi);
>      }
> +
> +    spapr_rtas_register(RTAS_IBM_SET_EEH_OPTION,
> +                        "ibm,set-eeh-option",
> +                        rtas_ibm_set_eeh_option);
> +    spapr_rtas_register(RTAS_IBM_GET_CONFIG_ADDR_INFO2,
> +                        "ibm,get-config-addr-info2",
> +                        rtas_ibm_get_config_addr_info2);
> +    spapr_rtas_register(RTAS_IBM_READ_SLOT_RESET_STATE2,
> +                        "ibm,read-slot-reset-state2",
> +                        rtas_ibm_read_slot_reset_state2);
> +    spapr_rtas_register(RTAS_IBM_SET_SLOT_RESET,
> +                        "ibm,set-slot-reset",
> +                        rtas_ibm_set_slot_reset);
> +    spapr_rtas_register(RTAS_IBM_CONFIGURE_PE,
> +                        "ibm,configure-pe",
> +                        rtas_ibm_configure_pe);
> +    spapr_rtas_register(RTAS_IBM_SLOT_ERROR_DETAIL,
> +                        "ibm,slot-error-detail",
> +                        rtas_ibm_slot_error_detail);
>  }
>  
>  static void spapr_pci_register_types(void)
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 876ecf0..bfde083 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -49,6 +49,7 @@ struct sPAPRPHBClass {
>      PCIHostBridgeClass parent_class;
>  
>      void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
> +    int (*eeh_handler)(sPAPRPHBState *sphb, int req, int opt);
>  };
>  
>  typedef struct spapr_pci_msi {
> @@ -109,6 +110,12 @@ struct sPAPRPHBVFIOState {
>  
>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>  
> +/* EEH related requests */
> +#define RTAS_EEH_REQ_SET_OPTION      0
> +#define RTAS_EEH_REQ_GET_STATE       1
> +#define RTAS_EEH_REQ_RESET           2
> +#define RTAS_EEH_REQ_CONFIGURE       3

AFAICT these are internal constants only, not defined by PAPR, used to
multiplex the eeh_handler callback.

We have to cope with the multiplexed calls defined by PAPR, but we
should avoid adding multiplexers of our own.  I think these functions
should each be separate callbacks in the PHB class.

>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>  {
>      return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 642cdc3..1cf5e89 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -338,6 +338,39 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
>  int spapr_allocate_irq(int hint, bool lsi);
>  int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>  
> +/* ibm,set-eeh-option */
> +#define RTAS_EEH_DISABLE                 0
> +#define RTAS_EEH_ENABLE                  1
> +#define RTAS_EEH_THAW_IO                 2
> +#define RTAS_EEH_THAW_DMA                3
> +
> +/* ibm,get-config-addr-info2 */
> +#define RTAS_GET_PE_ADDR                 0
> +#define RTAS_GET_PE_MODE                 1
> +#define RTAS_PE_MODE_NONE                0
> +#define RTAS_PE_MODE_NOT_SHARED          1
> +#define RTAS_PE_MODE_SHARED              2
> +
> +/* ibm,read-slot-reset-state2 */
> +#define RTAS_EEH_PE_STATE_NORMAL         0
> +#define RTAS_EEH_PE_STATE_RESET          1
> +#define RTAS_EEH_PE_STATE_STOPPED_IO_DMA 2
> +#define RTAS_EEH_PE_STATE_STOPPED_DMA    4
> +#define RTAS_EEH_PE_STATE_UNAVAIL        5
> +#define RTAS_EEH_NOT_SUPPORT             0
> +#define RTAS_EEH_SUPPORT                 1
> +#define RTAS_EEH_PE_UNAVAIL_INFO         1000
> +#define RTAS_EEH_PE_RECOVER_INFO         0
> +
> +/* ibm,set-slot-reset */
> +#define RTAS_SLOT_RESET_DEACTIVATE       0
> +#define RTAS_SLOT_RESET_HOT              1
> +#define RTAS_SLOT_RESET_FUNDAMENTAL      3
> +
> +/* ibm,slot-error-detail */
> +#define RTAS_SLOT_TEMP_ERR_LOG           1
> +#define RTAS_SLOT_PERM_ERR_LOG           2
> +
>  /* RTAS return codes */
>  #define RTAS_OUT_SUCCESS            0
>  #define RTAS_OUT_NO_ERRORS_FOUND    1
> @@ -382,8 +415,14 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>  #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
>  #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
>  #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
> -
> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
> +#define RTAS_IBM_SET_EEH_OPTION                 (RTAS_TOKEN_BASE + 0x20)
> +#define RTAS_IBM_GET_CONFIG_ADDR_INFO2          (RTAS_TOKEN_BASE + 0x21)
> +#define RTAS_IBM_READ_SLOT_RESET_STATE2         (RTAS_TOKEN_BASE + 0x22)
> +#define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
> +#define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
> +#define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
> +
> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
>  
>  /* RTAS ibm,get-system-parameter token values */
>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
Gavin Shan Feb. 5, 2015, 5:50 a.m. UTC | #2
On Thu, Feb 05, 2015 at 02:19:49PM +1100, David Gibson wrote:
>On Wed, Feb 04, 2015 at 01:27:35PM +1100, Gavin Shan wrote:
>> The emulation for EEH RTAS requests from guest isn't covered
>> by QEMU yet and the patch implements them.
>> 
>> The patch defines constants used by EEH RTAS calls and adds
>> callback sPAPRPHBClass::eeh_handler, which is going to be used
>> this way:
>> 
>>   * RTAS calls are received in spapr_pci.c, sanity check is done
>>     there.
>>   * RTAS handlers handle what they can. If there is something it
>>     cannot handle and sPAPRPHBClass::eeh_handler callback is defined,
>>     it is called.
>>   * sPAPRPHBClass::eeh_handler is only implemented for VFIO now. It
>>     does ioctl() to the IOMMU container fd to complete the call. Error
>>     codes from that ioctl() are transferred back to the guest.
>> 
>> [aik: defined RTAS tokens for EEH RTAS calls]
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c          | 310 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/pci-host/spapr.h |   7 +
>>  include/hw/ppc/spapr.h      |  43 +++++-
>>  3 files changed, 358 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 6deeb19..3fac5a9 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -406,6 +406,297 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>>  }
>>  
>> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>> +                                    sPAPREnvironment *spapr,
>> +                                    uint32_t token, uint32_t nargs,
>> +                                    target_ulong args, uint32_t nret,
>> +                                    target_ulong rets)
>> +{
>> +    sPAPRPHBState *sphb;
>> +    sPAPRPHBClass *spc;
>> +    uint32_t addr, option;
>> +    uint64_t buid;
>> +    int ret;
>> +
>> +    if ((nargs != 4) || (nret != 1)) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> +    addr = rtas_ld(args, 0);
>> +    option = rtas_ld(args, 3);
>> +
>> +    sphb = find_phb(spapr, buid);
>> +    if (!sphb) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> +    if (!spc->eeh_handler) {
>
>Should this case be RTAS_OUT_PARAM_ERROR or RTAS_OUT_NOT_SUPPORTED?
>

Those 2 macros have equal values. I'm refering to PAPR spec version 2.7
for all the code. RTAS call "ibm,set-eeh-option" doesn't support
RTAS_OUT_NOT_SUPPORTED as its return value. So RTAS_OUT_PARAM_ERROR
is correct return value here:

#define RTAS_OUT_PARAM_ERROR        -3
#define RTAS_OUT_NOT_SUPPORTED      -3

>> +        goto param_error_exit;
>> +    }
>> +
>> +    switch (option) {
>> +    case RTAS_EEH_ENABLE:
>> +        if (!find_dev(spapr, buid, addr)) {
>> +            goto param_error_exit;
>> +        }
>> +        break;
>> +    case RTAS_EEH_DISABLE:
>> +    case RTAS_EEH_THAW_IO:
>> +    case RTAS_EEH_THAW_DMA:
>> +        break;
>> +    default:
>> +        goto param_error_exit;
>> +    }
>> +
>> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_SET_OPTION, option);
>> +    if (ret < 0) {
>> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>
>It seems like it would make sense for the eeh_handler callback to
>return rtas error codes, so that if it makes sense it can signal
>errors other than RTAS_OUT_HW_ERROR.
>

It's hard for spc->eeh_handler() to return unified and clean RTAS_OUT_*
because some of the callers (RTAS call handlers) supports one specific
return value, but the left won't support. RTAS_OUT_HW_ERROR is one of
the example.

You also suggested to have multiple callbacks maintained (instead of
spc->eeh_handler()). In that way, the sPAPRPHBClass callbacks can
return RTAS_OUT_* precisely for varous cases. However, we have to
pay more cost to maintain multiple callbacks.

>> +        return;
>> +    }
>> +
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +    return;
>> +
>> +param_error_exit:
>> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> +}
>> +
>> +static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
>> +                                           sPAPREnvironment *spapr,
>> +                                           uint32_t token, uint32_t nargs,
>> +                                           target_ulong args, uint32_t nret,
>> +                                           target_ulong rets)
>> +{
>> +    sPAPRPHBState *sphb;
>> +    sPAPRPHBClass *spc;
>> +    PCIDevice *pdev;
>> +    uint32_t addr, option;
>> +    uint64_t buid;
>> +
>> +    if ((nargs != 4) || (nret != 2)) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> +    sphb = find_phb(spapr, buid);
>> +    if (!sphb) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> +    if (!spc->eeh_handler) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    addr = rtas_ld(args, 0);
>> +    option = rtas_ld(args, 3);
>> +    if (option != RTAS_GET_PE_ADDR && option != RTAS_GET_PE_MODE) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    pdev = find_dev(spapr, buid, addr);
>> +    if (!pdev) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    /*
>> +     * For now, we always have bus level PE whose address
>> +     * has format "00BBSS00". The guest OS might regard
>> +     * PE address 0 as invalid. We avoid that simply by
>> +     * extending it with one.
>
>This comment is a bit confusing.  Why not just say we have PE
>addresses of form "00BBSS01".  Also you should say what the BB and
>SS mean in your template.
>

Sure, I'll fix it.

>> +     */
>> +    if (option == RTAS_GET_PE_ADDR) {
>
>I think this would be clearer as a switch statement, which folds in
>the other if statement above.
>

Yeah, it can save couple of CPU cycles at least. I'll fix it.

>> +        rtas_st(rets, 1, (pci_bus_num(pdev->bus) << 16) + 1);
>> +    } else {
>> +        rtas_st(rets, 1, RTAS_PE_MODE_SHARED);
>> +    }
>> +
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +    return;
>> +
>> +param_error_exit:
>> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> +}
>> +
>> +static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
>> +                                            sPAPREnvironment *spapr,
>> +                                            uint32_t token, uint32_t nargs,
>> +                                            target_ulong args, uint32_t nret,
>> +                                            target_ulong rets)
>> +{
>> +    sPAPRPHBState *sphb;
>> +    sPAPRPHBClass *spc;
>> +    uint64_t buid;
>> +    int ret;
>> +
>> +    if ((nargs != 3) || (nret != 4 && nret != 5)) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> +    sphb = find_phb(spapr, buid);
>> +    if (!sphb) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> +    if (!spc->eeh_handler) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_GET_STATE, 0);
>> +    if (ret < 0) {
>> +        goto param_error_exit;
>
>So here, all errors from eeh_handler are turned into
>RTAS_OUT_PARAM_ERROR, whereas above they were all turned into
>RTAS_OUT_HW_ERROR.
>

Yeah, RTAS call "ibm,read-slot-reset-state2" only supports following return
values. I even suspect if I had wrong PAPR spec at hand :(

   0: Success
  -3: Parameter Error

>> +    }
>> +
>> +    rtas_st(rets, 1, ret);
>> +    rtas_st(rets, 2, RTAS_EEH_SUPPORT);
>> +    rtas_st(rets, 3, RTAS_EEH_PE_UNAVAIL_INFO);
>> +    if (nret >= 5) {
>> +        rtas_st(rets, 4, RTAS_EEH_PE_RECOVER_INFO);
>> +    }
>> +
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +    return;
>> +
>> +param_error_exit:
>> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> +}
>> +
>> +static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
>> +                                    sPAPREnvironment *spapr,
>> +                                    uint32_t token, uint32_t nargs,
>> +                                    target_ulong args, uint32_t nret,
>> +                                    target_ulong rets)
>> +{
>> +    sPAPRPHBState *sphb;
>> +    sPAPRPHBClass *spc;
>> +    uint32_t option;
>> +    uint64_t buid;
>> +    int ret;
>> +
>> +    if ((nargs != 4) || (nret != 1)) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> +    option = rtas_ld(args, 3);
>> +    sphb = find_phb(spapr, buid);
>> +    if (!sphb) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> +    if (!spc->eeh_handler) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    switch (option) {
>> +    case RTAS_SLOT_RESET_DEACTIVATE:
>> +    case RTAS_SLOT_RESET_HOT:
>> +    case RTAS_SLOT_RESET_FUNDAMENTAL:
>> +        break;
>> +    default:
>> +        goto param_error_exit;
>> +    }
>> +
>> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_RESET, option);
>> +    if (ret < 0) {
>> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>> +        return;
>> +    }
>> +
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +    return;
>> +
>> +param_error_exit:
>> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> +}
>> +
>> +static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
>> +                                  sPAPREnvironment *spapr,
>> +                                  uint32_t token, uint32_t nargs,
>> +                                  target_ulong args, uint32_t nret,
>> +                                  target_ulong rets)
>> +{
>> +    sPAPRPHBState *sphb;
>> +    sPAPRPHBClass *spc;
>> +    uint64_t buid;
>> +    int ret;
>> +
>> +    if ((nargs != 3) || (nret != 1)) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> +    sphb = find_phb(spapr, buid);
>> +    if (!sphb) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> +    if (!spc->eeh_handler) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_CONFIGURE, 0);
>> +    if (ret < 0) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +    return;
>> +
>> +param_error_exit:
>> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> +}
>> +
>> +/* To support it later */
>> +static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
>> +                                       sPAPREnvironment *spapr,
>> +                                       uint32_t token, uint32_t nargs,
>> +                                       target_ulong args, uint32_t nret,
>> +                                       target_ulong rets)
>> +{
>> +    sPAPRPHBState *sphb;
>> +    sPAPRPHBClass *spc;
>> +    int option;
>> +    uint64_t buid;
>> +
>> +    if ((nargs != 8) || (nret != 1)) {
>> +        goto no_error_log;
>
>Surely this should be RTAS_OUT_PARAM_ERROR, not RTAS_OUT_NO_ERRORS_FOUND.
>

RTAS call "ibm,slot-error-detail" supports following return values:

   1: No Error Log Returned
   0: Success
  -1: Hardware Error (cannot create log)

>> +    }
>> +
>> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> +    sphb = find_phb(spapr, buid);
>> +    if (!sphb) {
>> +        goto no_error_log;
>
>.. and this one..
>

Please refer to above explaination.

>> +    }
>> +
>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> +    if (!spc->eeh_handler) {
>> +        goto no_error_log;
>
>.. possibly not this one ..
>

As above.

>> +    }
>> +
>> +    option = rtas_ld(args, 7);
>> +    switch (option) {
>> +    case RTAS_SLOT_TEMP_ERR_LOG:
>> +    case RTAS_SLOT_PERM_ERR_LOG:
>> +        break;
>> +    default:
>> +        goto no_error_log;
>
>.. but again this one ..
>

As above.

>> +    }
>> +
>> +    /* We don't get error log yet */
>> +    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
>> +    return;
>> +
>> +no_error_log:
>> +    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
>> +}
>> +
>>  static int pci_spapr_swizzle(int slot, int pin)
>>  {
>>      return (slot + pin) % PCI_NUM_PINS;
>> @@ -964,6 +1255,25 @@ void spapr_pci_rtas_init(void)
>>          spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi",
>>                              rtas_ibm_change_msi);
>>      }
>> +
>> +    spapr_rtas_register(RTAS_IBM_SET_EEH_OPTION,
>> +                        "ibm,set-eeh-option",
>> +                        rtas_ibm_set_eeh_option);
>> +    spapr_rtas_register(RTAS_IBM_GET_CONFIG_ADDR_INFO2,
>> +                        "ibm,get-config-addr-info2",
>> +                        rtas_ibm_get_config_addr_info2);
>> +    spapr_rtas_register(RTAS_IBM_READ_SLOT_RESET_STATE2,
>> +                        "ibm,read-slot-reset-state2",
>> +                        rtas_ibm_read_slot_reset_state2);
>> +    spapr_rtas_register(RTAS_IBM_SET_SLOT_RESET,
>> +                        "ibm,set-slot-reset",
>> +                        rtas_ibm_set_slot_reset);
>> +    spapr_rtas_register(RTAS_IBM_CONFIGURE_PE,
>> +                        "ibm,configure-pe",
>> +                        rtas_ibm_configure_pe);
>> +    spapr_rtas_register(RTAS_IBM_SLOT_ERROR_DETAIL,
>> +                        "ibm,slot-error-detail",
>> +                        rtas_ibm_slot_error_detail);
>>  }
>>  
>>  static void spapr_pci_register_types(void)
>> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
>> index 876ecf0..bfde083 100644
>> --- a/include/hw/pci-host/spapr.h
>> +++ b/include/hw/pci-host/spapr.h
>> @@ -49,6 +49,7 @@ struct sPAPRPHBClass {
>>      PCIHostBridgeClass parent_class;
>>  
>>      void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
>> +    int (*eeh_handler)(sPAPRPHBState *sphb, int req, int opt);
>>  };
>>  
>>  typedef struct spapr_pci_msi {
>> @@ -109,6 +110,12 @@ struct sPAPRPHBVFIOState {
>>  
>>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>>  
>> +/* EEH related requests */
>> +#define RTAS_EEH_REQ_SET_OPTION      0
>> +#define RTAS_EEH_REQ_GET_STATE       1
>> +#define RTAS_EEH_REQ_RESET           2
>> +#define RTAS_EEH_REQ_CONFIGURE       3
>
>AFAICT these are internal constants only, not defined by PAPR, used to
>multiplex the eeh_handler callback.
>
>We have to cope with the multiplexed calls defined by PAPR, but we
>should avoid adding multiplexers of our own.  I think these functions
>should each be separate callbacks in the PHB class.
>

Ok. I'm going to split them into following callbacks and all of them will
have RTAS_OUT_* as their return values:

    eeh_set_option();
    eeh_get_state();
    eeh_reset();
    eeh_configure();

David, thank you for your time reviewing this.

Thanks,
Gavin

>>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>>  {
>>      return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 642cdc3..1cf5e89 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -338,6 +338,39 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
>>  int spapr_allocate_irq(int hint, bool lsi);
>>  int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>>  
>> +/* ibm,set-eeh-option */
>> +#define RTAS_EEH_DISABLE                 0
>> +#define RTAS_EEH_ENABLE                  1
>> +#define RTAS_EEH_THAW_IO                 2
>> +#define RTAS_EEH_THAW_DMA                3
>> +
>> +/* ibm,get-config-addr-info2 */
>> +#define RTAS_GET_PE_ADDR                 0
>> +#define RTAS_GET_PE_MODE                 1
>> +#define RTAS_PE_MODE_NONE                0
>> +#define RTAS_PE_MODE_NOT_SHARED          1
>> +#define RTAS_PE_MODE_SHARED              2
>> +
>> +/* ibm,read-slot-reset-state2 */
>> +#define RTAS_EEH_PE_STATE_NORMAL         0
>> +#define RTAS_EEH_PE_STATE_RESET          1
>> +#define RTAS_EEH_PE_STATE_STOPPED_IO_DMA 2
>> +#define RTAS_EEH_PE_STATE_STOPPED_DMA    4
>> +#define RTAS_EEH_PE_STATE_UNAVAIL        5
>> +#define RTAS_EEH_NOT_SUPPORT             0
>> +#define RTAS_EEH_SUPPORT                 1
>> +#define RTAS_EEH_PE_UNAVAIL_INFO         1000
>> +#define RTAS_EEH_PE_RECOVER_INFO         0
>> +
>> +/* ibm,set-slot-reset */
>> +#define RTAS_SLOT_RESET_DEACTIVATE       0
>> +#define RTAS_SLOT_RESET_HOT              1
>> +#define RTAS_SLOT_RESET_FUNDAMENTAL      3
>> +
>> +/* ibm,slot-error-detail */
>> +#define RTAS_SLOT_TEMP_ERR_LOG           1
>> +#define RTAS_SLOT_PERM_ERR_LOG           2
>> +
>>  /* RTAS return codes */
>>  #define RTAS_OUT_SUCCESS            0
>>  #define RTAS_OUT_NO_ERRORS_FOUND    1
>> @@ -382,8 +415,14 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>>  #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
>>  #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
>>  #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
>> -
>> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
>> +#define RTAS_IBM_SET_EEH_OPTION                 (RTAS_TOKEN_BASE + 0x20)
>> +#define RTAS_IBM_GET_CONFIG_ADDR_INFO2          (RTAS_TOKEN_BASE + 0x21)
>> +#define RTAS_IBM_READ_SLOT_RESET_STATE2         (RTAS_TOKEN_BASE + 0x22)
>> +#define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
>> +#define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
>> +#define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
>> +
>> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
>>  
>>  /* RTAS ibm,get-system-parameter token values */
>>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>
>-- 
>David Gibson			| I'll have my music baroque, and my code
>david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
>				| _way_ _around_!
>http://www.ozlabs.org/~dgibson
David Gibson Feb. 6, 2015, 12:58 a.m. UTC | #3
On Thu, Feb 05, 2015 at 04:50:06PM +1100, Gavin Shan wrote:
> On Thu, Feb 05, 2015 at 02:19:49PM +1100, David Gibson wrote:
> >On Wed, Feb 04, 2015 at 01:27:35PM +1100, Gavin Shan wrote:
> >> The emulation for EEH RTAS requests from guest isn't covered
> >> by QEMU yet and the patch implements them.
> >> 
> >> The patch defines constants used by EEH RTAS calls and adds
> >> callback sPAPRPHBClass::eeh_handler, which is going to be used
> >> this way:
> >> 
> >>   * RTAS calls are received in spapr_pci.c, sanity check is done
> >>     there.
> >>   * RTAS handlers handle what they can. If there is something it
> >>     cannot handle and sPAPRPHBClass::eeh_handler callback is defined,
> >>     it is called.
> >>   * sPAPRPHBClass::eeh_handler is only implemented for VFIO now. It
> >>     does ioctl() to the IOMMU container fd to complete the call. Error
> >>     codes from that ioctl() are transferred back to the guest.
> >> 
> >> [aik: defined RTAS tokens for EEH RTAS calls]
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr_pci.c          | 310 ++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/pci-host/spapr.h |   7 +
> >>  include/hw/ppc/spapr.h      |  43 +++++-
> >>  3 files changed, 358 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index 6deeb19..3fac5a9 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -406,6 +406,297 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
> >>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
> >>  }
> >>  
> >> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> >> +                                    sPAPREnvironment *spapr,
> >> +                                    uint32_t token, uint32_t nargs,
> >> +                                    target_ulong args, uint32_t nret,
> >> +                                    target_ulong rets)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    uint32_t addr, option;
> >> +    uint64_t buid;
> >> +    int ret;
> >> +
> >> +    if ((nargs != 4) || (nret != 1)) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> +    addr = rtas_ld(args, 0);
> >> +    option = rtas_ld(args, 3);
> >> +
> >> +    sphb = find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->eeh_handler) {
> >
> >Should this case be RTAS_OUT_PARAM_ERROR or RTAS_OUT_NOT_SUPPORTED?
> >
> 
> Those 2 macros have equal values. I'm refering to PAPR spec version 2.7
> for all the code. RTAS call "ibm,set-eeh-option" doesn't support
> RTAS_OUT_NOT_SUPPORTED as its return value. So RTAS_OUT_PARAM_ERROR
> is correct return value here:
> 
> #define RTAS_OUT_PARAM_ERROR        -3
> #define RTAS_OUT_NOT_SUPPORTED      -3

Ah, ok.

> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    switch (option) {
> >> +    case RTAS_EEH_ENABLE:
> >> +        if (!find_dev(spapr, buid, addr)) {
> >> +            goto param_error_exit;
> >> +        }
> >> +        break;
> >> +    case RTAS_EEH_DISABLE:
> >> +    case RTAS_EEH_THAW_IO:
> >> +    case RTAS_EEH_THAW_DMA:
> >> +        break;
> >> +    default:
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_SET_OPTION, option);
> >> +    if (ret < 0) {
> >> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> >
> >It seems like it would make sense for the eeh_handler callback to
> >return rtas error codes, so that if it makes sense it can signal
> >errors other than RTAS_OUT_HW_ERROR.
> >
> 
> It's hard for spc->eeh_handler() to return unified and clean RTAS_OUT_*
> because some of the callers (RTAS call handlers) supports one specific
> return value, but the left won't support. RTAS_OUT_HW_ERROR is one of
> the example.
> 
> You also suggested to have multiple callbacks maintained (instead of
> spc->eeh_handler()). In that way, the sPAPRPHBClass callbacks can
> return RTAS_OUT_* precisely for varous cases. However, we have to
> pay more cost to maintain multiple callbacks.

See, I disagree there.  A multiplexed callback, which is what you
have now, effectively is multiple callbacks but with an ugly
interface.  Maintaining separate callbacks is generally no harder.

> >> +        return;
> >> +    }
> >> +
> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >> +    return;
> >> +
> >> +param_error_exit:
> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >> +}
> >> +
> >> +static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
> >> +                                           sPAPREnvironment *spapr,
> >> +                                           uint32_t token, uint32_t nargs,
> >> +                                           target_ulong args, uint32_t nret,
> >> +                                           target_ulong rets)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    PCIDevice *pdev;
> >> +    uint32_t addr, option;
> >> +    uint64_t buid;
> >> +
> >> +    if ((nargs != 4) || (nret != 2)) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> +    sphb = find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->eeh_handler) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    addr = rtas_ld(args, 0);
> >> +    option = rtas_ld(args, 3);
> >> +    if (option != RTAS_GET_PE_ADDR && option != RTAS_GET_PE_MODE) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    pdev = find_dev(spapr, buid, addr);
> >> +    if (!pdev) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    /*
> >> +     * For now, we always have bus level PE whose address
> >> +     * has format "00BBSS00". The guest OS might regard
> >> +     * PE address 0 as invalid. We avoid that simply by
> >> +     * extending it with one.
> >
> >This comment is a bit confusing.  Why not just say we have PE
> >addresses of form "00BBSS01".  Also you should say what the BB and
> >SS mean in your template.
> >
> 
> Sure, I'll fix it.
> 
> >> +     */
> >> +    if (option == RTAS_GET_PE_ADDR) {
> >
> >I think this would be clearer as a switch statement, which folds in
> >the other if statement above.
> >
> 
> Yeah, it can save couple of CPU cycles at least. I'll fix it.

I don't really care about CPU cycles here, just code readability.

> >> +        rtas_st(rets, 1, (pci_bus_num(pdev->bus) << 16) + 1);
> >> +    } else {
> >> +        rtas_st(rets, 1, RTAS_PE_MODE_SHARED);
> >> +    }
> >> +
> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >> +    return;
> >> +
> >> +param_error_exit:
> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >> +}
> >> +
> >> +static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
> >> +                                            sPAPREnvironment *spapr,
> >> +                                            uint32_t token, uint32_t nargs,
> >> +                                            target_ulong args, uint32_t nret,
> >> +                                            target_ulong rets)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    uint64_t buid;
> >> +    int ret;
> >> +
> >> +    if ((nargs != 3) || (nret != 4 && nret != 5)) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> +    sphb = find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->eeh_handler) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_GET_STATE, 0);
> >> +    if (ret < 0) {
> >> +        goto param_error_exit;
> >
> >So here, all errors from eeh_handler are turned into
> >RTAS_OUT_PARAM_ERROR, whereas above they were all turned into
> >RTAS_OUT_HW_ERROR.
> >
> 
> Yeah, RTAS call "ibm,read-slot-reset-state2" only supports following return
> values. I even suspect if I had wrong PAPR spec at hand :(
> 
>    0: Success
>   -3: Parameter Error

Heh, ok.

> >> +    }
> >> +
> >> +    rtas_st(rets, 1, ret);
> >> +    rtas_st(rets, 2, RTAS_EEH_SUPPORT);
> >> +    rtas_st(rets, 3, RTAS_EEH_PE_UNAVAIL_INFO);
> >> +    if (nret >= 5) {
> >> +        rtas_st(rets, 4, RTAS_EEH_PE_RECOVER_INFO);
> >> +    }
> >> +
> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >> +    return;
> >> +
> >> +param_error_exit:
> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >> +}
> >> +
> >> +static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
> >> +                                    sPAPREnvironment *spapr,
> >> +                                    uint32_t token, uint32_t nargs,
> >> +                                    target_ulong args, uint32_t nret,
> >> +                                    target_ulong rets)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    uint32_t option;
> >> +    uint64_t buid;
> >> +    int ret;
> >> +
> >> +    if ((nargs != 4) || (nret != 1)) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> +    option = rtas_ld(args, 3);
> >> +    sphb = find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->eeh_handler) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    switch (option) {
> >> +    case RTAS_SLOT_RESET_DEACTIVATE:
> >> +    case RTAS_SLOT_RESET_HOT:
> >> +    case RTAS_SLOT_RESET_FUNDAMENTAL:
> >> +        break;
> >> +    default:
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_RESET, option);
> >> +    if (ret < 0) {
> >> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> >> +        return;
> >> +    }
> >> +
> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >> +    return;
> >> +
> >> +param_error_exit:
> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >> +}
> >> +
> >> +static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
> >> +                                  sPAPREnvironment *spapr,
> >> +                                  uint32_t token, uint32_t nargs,
> >> +                                  target_ulong args, uint32_t nret,
> >> +                                  target_ulong rets)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    uint64_t buid;
> >> +    int ret;
> >> +
> >> +    if ((nargs != 3) || (nret != 1)) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> +    sphb = find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->eeh_handler) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_CONFIGURE, 0);
> >> +    if (ret < 0) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >> +    return;
> >> +
> >> +param_error_exit:
> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >> +}
> >> +
> >> +/* To support it later */
> >> +static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
> >> +                                       sPAPREnvironment *spapr,
> >> +                                       uint32_t token, uint32_t nargs,
> >> +                                       target_ulong args, uint32_t nret,
> >> +                                       target_ulong rets)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    int option;
> >> +    uint64_t buid;
> >> +
> >> +    if ((nargs != 8) || (nret != 1)) {
> >> +        goto no_error_log;
> >
> >Surely this should be RTAS_OUT_PARAM_ERROR, not RTAS_OUT_NO_ERRORS_FOUND.
> >
> 
> RTAS call "ibm,slot-error-detail" supports following return values:
> 
>    1: No Error Log Returned
>    0: Success
>   -1: Hardware Error (cannot create log)

So, in this particular case, I think you should return
RTAS_OUT_PARAM_ERROR despite what PAPR says.  When the number of
arguments isn't even right, you're not really calling this RTAS
function, so what it says about return values doens't really make any
sense.

> >> +    }
> >> +
> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> +    sphb = find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        goto no_error_log;
> >
> >.. and this one..
> >
> 
> Please refer to above explaination.

In this case and the rest though, what PAPR says should be applied,
even if it is a bit stupid.

> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->eeh_handler) {
> >> +        goto no_error_log;
> >
> >.. possibly not this one ..
> >
> 
> As above.
> 
> >> +    }
> >> +
> >> +    option = rtas_ld(args, 7);
> >> +    switch (option) {
> >> +    case RTAS_SLOT_TEMP_ERR_LOG:
> >> +    case RTAS_SLOT_PERM_ERR_LOG:
> >> +        break;
> >> +    default:
> >> +        goto no_error_log;
> >
> >.. but again this one ..
> >
> 
> As above.
> 
> >> +    }
> >> +
> >> +    /* We don't get error log yet */
> >> +    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
> >> +    return;
> >> +
> >> +no_error_log:
> >> +    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
> >> +}
> >> +
> >>  static int pci_spapr_swizzle(int slot, int pin)
> >>  {
> >>      return (slot + pin) % PCI_NUM_PINS;
> >> @@ -964,6 +1255,25 @@ void spapr_pci_rtas_init(void)
> >>          spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi",
> >>                              rtas_ibm_change_msi);
> >>      }
> >> +
> >> +    spapr_rtas_register(RTAS_IBM_SET_EEH_OPTION,
> >> +                        "ibm,set-eeh-option",
> >> +                        rtas_ibm_set_eeh_option);
> >> +    spapr_rtas_register(RTAS_IBM_GET_CONFIG_ADDR_INFO2,
> >> +                        "ibm,get-config-addr-info2",
> >> +                        rtas_ibm_get_config_addr_info2);
> >> +    spapr_rtas_register(RTAS_IBM_READ_SLOT_RESET_STATE2,
> >> +                        "ibm,read-slot-reset-state2",
> >> +                        rtas_ibm_read_slot_reset_state2);
> >> +    spapr_rtas_register(RTAS_IBM_SET_SLOT_RESET,
> >> +                        "ibm,set-slot-reset",
> >> +                        rtas_ibm_set_slot_reset);
> >> +    spapr_rtas_register(RTAS_IBM_CONFIGURE_PE,
> >> +                        "ibm,configure-pe",
> >> +                        rtas_ibm_configure_pe);
> >> +    spapr_rtas_register(RTAS_IBM_SLOT_ERROR_DETAIL,
> >> +                        "ibm,slot-error-detail",
> >> +                        rtas_ibm_slot_error_detail);
> >>  }
> >>  
> >>  static void spapr_pci_register_types(void)
> >> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> >> index 876ecf0..bfde083 100644
> >> --- a/include/hw/pci-host/spapr.h
> >> +++ b/include/hw/pci-host/spapr.h
> >> @@ -49,6 +49,7 @@ struct sPAPRPHBClass {
> >>      PCIHostBridgeClass parent_class;
> >>  
> >>      void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
> >> +    int (*eeh_handler)(sPAPRPHBState *sphb, int req, int opt);
> >>  };
> >>  
> >>  typedef struct spapr_pci_msi {
> >> @@ -109,6 +110,12 @@ struct sPAPRPHBVFIOState {
> >>  
> >>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> >>  
> >> +/* EEH related requests */
> >> +#define RTAS_EEH_REQ_SET_OPTION      0
> >> +#define RTAS_EEH_REQ_GET_STATE       1
> >> +#define RTAS_EEH_REQ_RESET           2
> >> +#define RTAS_EEH_REQ_CONFIGURE       3
> >
> >AFAICT these are internal constants only, not defined by PAPR, used to
> >multiplex the eeh_handler callback.
> >
> >We have to cope with the multiplexed calls defined by PAPR, but we
> >should avoid adding multiplexers of our own.  I think these functions
> >should each be separate callbacks in the PHB class.
> >
> 
> Ok. I'm going to split them into following callbacks and all of them will
> have RTAS_OUT_* as their return values:
> 
>     eeh_set_option();
>     eeh_get_state();
>     eeh_reset();
>     eeh_configure();
> 
> David, thank you for your time reviewing this.
> 
> Thanks,
> Gavin
> 
> >>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
> >>  {
> >>      return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 642cdc3..1cf5e89 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -338,6 +338,39 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
> >>  int spapr_allocate_irq(int hint, bool lsi);
> >>  int spapr_allocate_irq_block(int num, bool lsi, bool msi);
> >>  
> >> +/* ibm,set-eeh-option */
> >> +#define RTAS_EEH_DISABLE                 0
> >> +#define RTAS_EEH_ENABLE                  1
> >> +#define RTAS_EEH_THAW_IO                 2
> >> +#define RTAS_EEH_THAW_DMA                3
> >> +
> >> +/* ibm,get-config-addr-info2 */
> >> +#define RTAS_GET_PE_ADDR                 0
> >> +#define RTAS_GET_PE_MODE                 1
> >> +#define RTAS_PE_MODE_NONE                0
> >> +#define RTAS_PE_MODE_NOT_SHARED          1
> >> +#define RTAS_PE_MODE_SHARED              2
> >> +
> >> +/* ibm,read-slot-reset-state2 */
> >> +#define RTAS_EEH_PE_STATE_NORMAL         0
> >> +#define RTAS_EEH_PE_STATE_RESET          1
> >> +#define RTAS_EEH_PE_STATE_STOPPED_IO_DMA 2
> >> +#define RTAS_EEH_PE_STATE_STOPPED_DMA    4
> >> +#define RTAS_EEH_PE_STATE_UNAVAIL        5
> >> +#define RTAS_EEH_NOT_SUPPORT             0
> >> +#define RTAS_EEH_SUPPORT                 1
> >> +#define RTAS_EEH_PE_UNAVAIL_INFO         1000
> >> +#define RTAS_EEH_PE_RECOVER_INFO         0
> >> +
> >> +/* ibm,set-slot-reset */
> >> +#define RTAS_SLOT_RESET_DEACTIVATE       0
> >> +#define RTAS_SLOT_RESET_HOT              1
> >> +#define RTAS_SLOT_RESET_FUNDAMENTAL      3
> >> +
> >> +/* ibm,slot-error-detail */
> >> +#define RTAS_SLOT_TEMP_ERR_LOG           1
> >> +#define RTAS_SLOT_PERM_ERR_LOG           2
> >> +
> >>  /* RTAS return codes */
> >>  #define RTAS_OUT_SUCCESS            0
> >>  #define RTAS_OUT_NO_ERRORS_FOUND    1
> >> @@ -382,8 +415,14 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
> >>  #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
> >>  #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
> >>  #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
> >> -
> >> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
> >> +#define RTAS_IBM_SET_EEH_OPTION                 (RTAS_TOKEN_BASE + 0x20)
> >> +#define RTAS_IBM_GET_CONFIG_ADDR_INFO2          (RTAS_TOKEN_BASE + 0x21)
> >> +#define RTAS_IBM_READ_SLOT_RESET_STATE2         (RTAS_TOKEN_BASE + 0x22)
> >> +#define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
> >> +#define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
> >> +#define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
> >> +
> >> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
> >>  
> >>  /* RTAS ibm,get-system-parameter token values */
> >>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
> >
> 
>
Gavin Shan Feb. 6, 2015, 5:12 a.m. UTC | #4
On Fri, Feb 06, 2015 at 11:58:25AM +1100, David Gibson wrote:
>On Thu, Feb 05, 2015 at 04:50:06PM +1100, Gavin Shan wrote:
>> On Thu, Feb 05, 2015 at 02:19:49PM +1100, David Gibson wrote:
>> >On Wed, Feb 04, 2015 at 01:27:35PM +1100, Gavin Shan wrote:
>> >> The emulation for EEH RTAS requests from guest isn't covered
>> >> by QEMU yet and the patch implements them.
>> >> 
>> >> The patch defines constants used by EEH RTAS calls and adds
>> >> callback sPAPRPHBClass::eeh_handler, which is going to be used
>> >> this way:
>> >> 
>> >>   * RTAS calls are received in spapr_pci.c, sanity check is done
>> >>     there.
>> >>   * RTAS handlers handle what they can. If there is something it
>> >>     cannot handle and sPAPRPHBClass::eeh_handler callback is defined,
>> >>     it is called.
>> >>   * sPAPRPHBClass::eeh_handler is only implemented for VFIO now. It
>> >>     does ioctl() to the IOMMU container fd to complete the call. Error
>> >>     codes from that ioctl() are transferred back to the guest.
>> >> 
>> >> [aik: defined RTAS tokens for EEH RTAS calls]
>> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> >> ---
>> >>  hw/ppc/spapr_pci.c          | 310 ++++++++++++++++++++++++++++++++++++++++++++
>> >>  include/hw/pci-host/spapr.h |   7 +
>> >>  include/hw/ppc/spapr.h      |  43 +++++-
>> >>  3 files changed, 358 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> >> index 6deeb19..3fac5a9 100644
>> >> --- a/hw/ppc/spapr_pci.c
>> >> +++ b/hw/ppc/spapr_pci.c
>> >> @@ -406,6 +406,297 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>> >>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>> >>  }
>> >>  
>> >> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>> >> +                                    sPAPREnvironment *spapr,
>> >> +                                    uint32_t token, uint32_t nargs,
>> >> +                                    target_ulong args, uint32_t nret,
>> >> +                                    target_ulong rets)
>> >> +{
>> >> +    sPAPRPHBState *sphb;
>> >> +    sPAPRPHBClass *spc;
>> >> +    uint32_t addr, option;
>> >> +    uint64_t buid;
>> >> +    int ret;
>> >> +
>> >> +    if ((nargs != 4) || (nret != 1)) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> >> +    addr = rtas_ld(args, 0);
>> >> +    option = rtas_ld(args, 3);
>> >> +
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (!sphb) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> >> +    if (!spc->eeh_handler) {
>> >
>> >Should this case be RTAS_OUT_PARAM_ERROR or RTAS_OUT_NOT_SUPPORTED?
>> >
>> 
>> Those 2 macros have equal values. I'm refering to PAPR spec version 2.7
>> for all the code. RTAS call "ibm,set-eeh-option" doesn't support
>> RTAS_OUT_NOT_SUPPORTED as its return value. So RTAS_OUT_PARAM_ERROR
>> is correct return value here:
>> 
>> #define RTAS_OUT_PARAM_ERROR        -3
>> #define RTAS_OUT_NOT_SUPPORTED      -3
>
>Ah, ok.
>
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    switch (option) {
>> >> +    case RTAS_EEH_ENABLE:
>> >> +        if (!find_dev(spapr, buid, addr)) {
>> >> +            goto param_error_exit;
>> >> +        }
>> >> +        break;
>> >> +    case RTAS_EEH_DISABLE:
>> >> +    case RTAS_EEH_THAW_IO:
>> >> +    case RTAS_EEH_THAW_DMA:
>> >> +        break;
>> >> +    default:
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_SET_OPTION, option);
>> >> +    if (ret < 0) {
>> >> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>> >
>> >It seems like it would make sense for the eeh_handler callback to
>> >return rtas error codes, so that if it makes sense it can signal
>> >errors other than RTAS_OUT_HW_ERROR.
>> >
>> 
>> It's hard for spc->eeh_handler() to return unified and clean RTAS_OUT_*
>> because some of the callers (RTAS call handlers) supports one specific
>> return value, but the left won't support. RTAS_OUT_HW_ERROR is one of
>> the example.
>> 
>> You also suggested to have multiple callbacks maintained (instead of
>> spc->eeh_handler()). In that way, the sPAPRPHBClass callbacks can
>> return RTAS_OUT_* precisely for varous cases. However, we have to
>> pay more cost to maintain multiple callbacks.
>
>See, I disagree there.  A multiplexed callback, which is what you
>have now, effectively is multiple callbacks but with an ugly
>interface.  Maintaining separate callbacks is generally no harder.
>

Ok. I'll make it to 4 callbacks as follows:

	eeh_set_option();
	eeh_get_state();
	eeh_reset();
	eeh_configure();

>> >> +        return;
>> >> +    }
>> >> +
>> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> >> +    return;
>> >> +
>> >> +param_error_exit:
>> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> >> +}
>> >> +
>> >> +static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
>> >> +                                           sPAPREnvironment *spapr,
>> >> +                                           uint32_t token, uint32_t nargs,
>> >> +                                           target_ulong args, uint32_t nret,
>> >> +                                           target_ulong rets)
>> >> +{
>> >> +    sPAPRPHBState *sphb;
>> >> +    sPAPRPHBClass *spc;
>> >> +    PCIDevice *pdev;
>> >> +    uint32_t addr, option;
>> >> +    uint64_t buid;
>> >> +
>> >> +    if ((nargs != 4) || (nret != 2)) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (!sphb) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> >> +    if (!spc->eeh_handler) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    addr = rtas_ld(args, 0);
>> >> +    option = rtas_ld(args, 3);
>> >> +    if (option != RTAS_GET_PE_ADDR && option != RTAS_GET_PE_MODE) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    pdev = find_dev(spapr, buid, addr);
>> >> +    if (!pdev) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    /*
>> >> +     * For now, we always have bus level PE whose address
>> >> +     * has format "00BBSS00". The guest OS might regard
>> >> +     * PE address 0 as invalid. We avoid that simply by
>> >> +     * extending it with one.
>> >
>> >This comment is a bit confusing.  Why not just say we have PE
>> >addresses of form "00BBSS01".  Also you should say what the BB and
>> >SS mean in your template.
>> >
>> 
>> Sure, I'll fix it.
>> 
>> >> +     */
>> >> +    if (option == RTAS_GET_PE_ADDR) {
>> >
>> >I think this would be clearer as a switch statement, which folds in
>> >the other if statement above.
>> >
>> 
>> Yeah, it can save couple of CPU cycles at least. I'll fix it.
>
>I don't really care about CPU cycles here, just code readability.
>

Sure, it improve code readability as well.

>> >> +        rtas_st(rets, 1, (pci_bus_num(pdev->bus) << 16) + 1);
>> >> +    } else {
>> >> +        rtas_st(rets, 1, RTAS_PE_MODE_SHARED);
>> >> +    }
>> >> +
>> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> >> +    return;
>> >> +
>> >> +param_error_exit:
>> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> >> +}
>> >> +
>> >> +static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
>> >> +                                            sPAPREnvironment *spapr,
>> >> +                                            uint32_t token, uint32_t nargs,
>> >> +                                            target_ulong args, uint32_t nret,
>> >> +                                            target_ulong rets)
>> >> +{
>> >> +    sPAPRPHBState *sphb;
>> >> +    sPAPRPHBClass *spc;
>> >> +    uint64_t buid;
>> >> +    int ret;
>> >> +
>> >> +    if ((nargs != 3) || (nret != 4 && nret != 5)) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (!sphb) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> >> +    if (!spc->eeh_handler) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_GET_STATE, 0);
>> >> +    if (ret < 0) {
>> >> +        goto param_error_exit;
>> >
>> >So here, all errors from eeh_handler are turned into
>> >RTAS_OUT_PARAM_ERROR, whereas above they were all turned into
>> >RTAS_OUT_HW_ERROR.
>> >
>> 
>> Yeah, RTAS call "ibm,read-slot-reset-state2" only supports following return
>> values. I even suspect if I had wrong PAPR spec at hand :(
>> 
>>    0: Success
>>   -3: Parameter Error
>
>Heh, ok.
>
>> >> +    }
>> >> +
>> >> +    rtas_st(rets, 1, ret);
>> >> +    rtas_st(rets, 2, RTAS_EEH_SUPPORT);
>> >> +    rtas_st(rets, 3, RTAS_EEH_PE_UNAVAIL_INFO);
>> >> +    if (nret >= 5) {
>> >> +        rtas_st(rets, 4, RTAS_EEH_PE_RECOVER_INFO);
>> >> +    }
>> >> +
>> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> >> +    return;
>> >> +
>> >> +param_error_exit:
>> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> >> +}
>> >> +
>> >> +static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
>> >> +                                    sPAPREnvironment *spapr,
>> >> +                                    uint32_t token, uint32_t nargs,
>> >> +                                    target_ulong args, uint32_t nret,
>> >> +                                    target_ulong rets)
>> >> +{
>> >> +    sPAPRPHBState *sphb;
>> >> +    sPAPRPHBClass *spc;
>> >> +    uint32_t option;
>> >> +    uint64_t buid;
>> >> +    int ret;
>> >> +
>> >> +    if ((nargs != 4) || (nret != 1)) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> >> +    option = rtas_ld(args, 3);
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (!sphb) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> >> +    if (!spc->eeh_handler) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    switch (option) {
>> >> +    case RTAS_SLOT_RESET_DEACTIVATE:
>> >> +    case RTAS_SLOT_RESET_HOT:
>> >> +    case RTAS_SLOT_RESET_FUNDAMENTAL:
>> >> +        break;
>> >> +    default:
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_RESET, option);
>> >> +    if (ret < 0) {
>> >> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>> >> +        return;
>> >> +    }
>> >> +
>> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> >> +    return;
>> >> +
>> >> +param_error_exit:
>> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> >> +}
>> >> +
>> >> +static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
>> >> +                                  sPAPREnvironment *spapr,
>> >> +                                  uint32_t token, uint32_t nargs,
>> >> +                                  target_ulong args, uint32_t nret,
>> >> +                                  target_ulong rets)
>> >> +{
>> >> +    sPAPRPHBState *sphb;
>> >> +    sPAPRPHBClass *spc;
>> >> +    uint64_t buid;
>> >> +    int ret;
>> >> +
>> >> +    if ((nargs != 3) || (nret != 1)) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (!sphb) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> >> +    if (!spc->eeh_handler) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_CONFIGURE, 0);
>> >> +    if (ret < 0) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> >> +    return;
>> >> +
>> >> +param_error_exit:
>> >> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> >> +}
>> >> +
>> >> +/* To support it later */
>> >> +static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
>> >> +                                       sPAPREnvironment *spapr,
>> >> +                                       uint32_t token, uint32_t nargs,
>> >> +                                       target_ulong args, uint32_t nret,
>> >> +                                       target_ulong rets)
>> >> +{
>> >> +    sPAPRPHBState *sphb;
>> >> +    sPAPRPHBClass *spc;
>> >> +    int option;
>> >> +    uint64_t buid;
>> >> +
>> >> +    if ((nargs != 8) || (nret != 1)) {
>> >> +        goto no_error_log;
>> >
>> >Surely this should be RTAS_OUT_PARAM_ERROR, not RTAS_OUT_NO_ERRORS_FOUND.
>> >
>> 
>> RTAS call "ibm,slot-error-detail" supports following return values:
>> 
>>    1: No Error Log Returned
>>    0: Success
>>   -1: Hardware Error (cannot create log)
>
>So, in this particular case, I think you should return
>RTAS_OUT_PARAM_ERROR despite what PAPR says.  When the number of
>arguments isn't even right, you're not really calling this RTAS
>function, so what it says about return values doens't really make any
>sense.
>

Ok. Will return RTAS_OUT_PARAM_ERROR for this particular case.

>> >> +    }
>> >> +
>> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (!sphb) {
>> >> +        goto no_error_log;
>> >
>> >.. and this one..
>> >
>> 
>> Please refer to above explaination.
>
>In this case and the rest though, what PAPR says should be applied,
>even if it is a bit stupid.
>

Ok. Will return "1: No Error Log Returned" for all left cases.

Thanks,
Gavin

>> >> +    }
>> >> +
>> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> >> +    if (!spc->eeh_handler) {
>> >> +        goto no_error_log;
>> >
>> >.. possibly not this one ..
>> >
>> 
>> As above.
>> 
>> >> +    }
>> >> +
>> >> +    option = rtas_ld(args, 7);
>> >> +    switch (option) {
>> >> +    case RTAS_SLOT_TEMP_ERR_LOG:
>> >> +    case RTAS_SLOT_PERM_ERR_LOG:
>> >> +        break;
>> >> +    default:
>> >> +        goto no_error_log;
>> >
>> >.. but again this one ..
>> >
>> 
>> As above.
>> 
>> >> +    }
>> >> +
>> >> +    /* We don't get error log yet */
>> >> +    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
>> >> +    return;
>> >> +
>> >> +no_error_log:
>> >> +    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
>> >> +}
>> >> +
>> >>  static int pci_spapr_swizzle(int slot, int pin)
>> >>  {
>> >>      return (slot + pin) % PCI_NUM_PINS;
>> >> @@ -964,6 +1255,25 @@ void spapr_pci_rtas_init(void)
>> >>          spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi",
>> >>                              rtas_ibm_change_msi);
>> >>      }
>> >> +
>> >> +    spapr_rtas_register(RTAS_IBM_SET_EEH_OPTION,
>> >> +                        "ibm,set-eeh-option",
>> >> +                        rtas_ibm_set_eeh_option);
>> >> +    spapr_rtas_register(RTAS_IBM_GET_CONFIG_ADDR_INFO2,
>> >> +                        "ibm,get-config-addr-info2",
>> >> +                        rtas_ibm_get_config_addr_info2);
>> >> +    spapr_rtas_register(RTAS_IBM_READ_SLOT_RESET_STATE2,
>> >> +                        "ibm,read-slot-reset-state2",
>> >> +                        rtas_ibm_read_slot_reset_state2);
>> >> +    spapr_rtas_register(RTAS_IBM_SET_SLOT_RESET,
>> >> +                        "ibm,set-slot-reset",
>> >> +                        rtas_ibm_set_slot_reset);
>> >> +    spapr_rtas_register(RTAS_IBM_CONFIGURE_PE,
>> >> +                        "ibm,configure-pe",
>> >> +                        rtas_ibm_configure_pe);
>> >> +    spapr_rtas_register(RTAS_IBM_SLOT_ERROR_DETAIL,
>> >> +                        "ibm,slot-error-detail",
>> >> +                        rtas_ibm_slot_error_detail);
>> >>  }
>> >>  
>> >>  static void spapr_pci_register_types(void)
>> >> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
>> >> index 876ecf0..bfde083 100644
>> >> --- a/include/hw/pci-host/spapr.h
>> >> +++ b/include/hw/pci-host/spapr.h
>> >> @@ -49,6 +49,7 @@ struct sPAPRPHBClass {
>> >>      PCIHostBridgeClass parent_class;
>> >>  
>> >>      void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
>> >> +    int (*eeh_handler)(sPAPRPHBState *sphb, int req, int opt);
>> >>  };
>> >>  
>> >>  typedef struct spapr_pci_msi {
>> >> @@ -109,6 +110,12 @@ struct sPAPRPHBVFIOState {
>> >>  
>> >>  #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>> >>  
>> >> +/* EEH related requests */
>> >> +#define RTAS_EEH_REQ_SET_OPTION      0
>> >> +#define RTAS_EEH_REQ_GET_STATE       1
>> >> +#define RTAS_EEH_REQ_RESET           2
>> >> +#define RTAS_EEH_REQ_CONFIGURE       3
>> >
>> >AFAICT these are internal constants only, not defined by PAPR, used to
>> >multiplex the eeh_handler callback.
>> >
>> >We have to cope with the multiplexed calls defined by PAPR, but we
>> >should avoid adding multiplexers of our own.  I think these functions
>> >should each be separate callbacks in the PHB class.
>> >
>> 
>> Ok. I'm going to split them into following callbacks and all of them will
>> have RTAS_OUT_* as their return values:
>> 
>>     eeh_set_option();
>>     eeh_get_state();
>>     eeh_reset();
>>     eeh_configure();
>> 
>> David, thank you for your time reviewing this.
>> 
>> Thanks,
>> Gavin
>> 
>> >>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>> >>  {
>> >>      return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
>> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> >> index 642cdc3..1cf5e89 100644
>> >> --- a/include/hw/ppc/spapr.h
>> >> +++ b/include/hw/ppc/spapr.h
>> >> @@ -338,6 +338,39 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
>> >>  int spapr_allocate_irq(int hint, bool lsi);
>> >>  int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>> >>  
>> >> +/* ibm,set-eeh-option */
>> >> +#define RTAS_EEH_DISABLE                 0
>> >> +#define RTAS_EEH_ENABLE                  1
>> >> +#define RTAS_EEH_THAW_IO                 2
>> >> +#define RTAS_EEH_THAW_DMA                3
>> >> +
>> >> +/* ibm,get-config-addr-info2 */
>> >> +#define RTAS_GET_PE_ADDR                 0
>> >> +#define RTAS_GET_PE_MODE                 1
>> >> +#define RTAS_PE_MODE_NONE                0
>> >> +#define RTAS_PE_MODE_NOT_SHARED          1
>> >> +#define RTAS_PE_MODE_SHARED              2
>> >> +
>> >> +/* ibm,read-slot-reset-state2 */
>> >> +#define RTAS_EEH_PE_STATE_NORMAL         0
>> >> +#define RTAS_EEH_PE_STATE_RESET          1
>> >> +#define RTAS_EEH_PE_STATE_STOPPED_IO_DMA 2
>> >> +#define RTAS_EEH_PE_STATE_STOPPED_DMA    4
>> >> +#define RTAS_EEH_PE_STATE_UNAVAIL        5
>> >> +#define RTAS_EEH_NOT_SUPPORT             0
>> >> +#define RTAS_EEH_SUPPORT                 1
>> >> +#define RTAS_EEH_PE_UNAVAIL_INFO         1000
>> >> +#define RTAS_EEH_PE_RECOVER_INFO         0
>> >> +
>> >> +/* ibm,set-slot-reset */
>> >> +#define RTAS_SLOT_RESET_DEACTIVATE       0
>> >> +#define RTAS_SLOT_RESET_HOT              1
>> >> +#define RTAS_SLOT_RESET_FUNDAMENTAL      3
>> >> +
>> >> +/* ibm,slot-error-detail */
>> >> +#define RTAS_SLOT_TEMP_ERR_LOG           1
>> >> +#define RTAS_SLOT_PERM_ERR_LOG           2
>> >> +
>> >>  /* RTAS return codes */
>> >>  #define RTAS_OUT_SUCCESS            0
>> >>  #define RTAS_OUT_NO_ERRORS_FOUND    1
>> >> @@ -382,8 +415,14 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>> >>  #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
>> >>  #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
>> >>  #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
>> >> -
>> >> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
>> >> +#define RTAS_IBM_SET_EEH_OPTION                 (RTAS_TOKEN_BASE + 0x20)
>> >> +#define RTAS_IBM_GET_CONFIG_ADDR_INFO2          (RTAS_TOKEN_BASE + 0x21)
>> >> +#define RTAS_IBM_READ_SLOT_RESET_STATE2         (RTAS_TOKEN_BASE + 0x22)
>> >> +#define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
>> >> +#define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
>> >> +#define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
>> >> +
>> >> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
>> >>  
>> >>  /* RTAS ibm,get-system-parameter token values */
>> >>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>> >
>> 
>> 
>
>-- 
>David Gibson			| I'll have my music baroque, and my code
>david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
>				| _way_ _around_!
>http://www.ozlabs.org/~dgibson
diff mbox

Patch

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 6deeb19..3fac5a9 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -406,6 +406,297 @@  static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
     rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
 }
 
+static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
+                                    sPAPREnvironment *spapr,
+                                    uint32_t token, uint32_t nargs,
+                                    target_ulong args, uint32_t nret,
+                                    target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint32_t addr, option;
+    uint64_t buid;
+    int ret;
+
+    if ((nargs != 4) || (nret != 1)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    addr = rtas_ld(args, 0);
+    option = rtas_ld(args, 3);
+
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_handler) {
+        goto param_error_exit;
+    }
+
+    switch (option) {
+    case RTAS_EEH_ENABLE:
+        if (!find_dev(spapr, buid, addr)) {
+            goto param_error_exit;
+        }
+        break;
+    case RTAS_EEH_DISABLE:
+    case RTAS_EEH_THAW_IO:
+    case RTAS_EEH_THAW_DMA:
+        break;
+    default:
+        goto param_error_exit;
+    }
+
+    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_SET_OPTION, option);
+    if (ret < 0) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
+                                           sPAPREnvironment *spapr,
+                                           uint32_t token, uint32_t nargs,
+                                           target_ulong args, uint32_t nret,
+                                           target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    PCIDevice *pdev;
+    uint32_t addr, option;
+    uint64_t buid;
+
+    if ((nargs != 4) || (nret != 2)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_handler) {
+        goto param_error_exit;
+    }
+
+    addr = rtas_ld(args, 0);
+    option = rtas_ld(args, 3);
+    if (option != RTAS_GET_PE_ADDR && option != RTAS_GET_PE_MODE) {
+        goto param_error_exit;
+    }
+
+    pdev = find_dev(spapr, buid, addr);
+    if (!pdev) {
+        goto param_error_exit;
+    }
+
+    /*
+     * For now, we always have bus level PE whose address
+     * has format "00BBSS00". The guest OS might regard
+     * PE address 0 as invalid. We avoid that simply by
+     * extending it with one.
+     */
+    if (option == RTAS_GET_PE_ADDR) {
+        rtas_st(rets, 1, (pci_bus_num(pdev->bus) << 16) + 1);
+    } else {
+        rtas_st(rets, 1, RTAS_PE_MODE_SHARED);
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
+                                            sPAPREnvironment *spapr,
+                                            uint32_t token, uint32_t nargs,
+                                            target_ulong args, uint32_t nret,
+                                            target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint64_t buid;
+    int ret;
+
+    if ((nargs != 3) || (nret != 4 && nret != 5)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_handler) {
+        goto param_error_exit;
+    }
+
+    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_GET_STATE, 0);
+    if (ret < 0) {
+        goto param_error_exit;
+    }
+
+    rtas_st(rets, 1, ret);
+    rtas_st(rets, 2, RTAS_EEH_SUPPORT);
+    rtas_st(rets, 3, RTAS_EEH_PE_UNAVAIL_INFO);
+    if (nret >= 5) {
+        rtas_st(rets, 4, RTAS_EEH_PE_RECOVER_INFO);
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
+                                    sPAPREnvironment *spapr,
+                                    uint32_t token, uint32_t nargs,
+                                    target_ulong args, uint32_t nret,
+                                    target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint32_t option;
+    uint64_t buid;
+    int ret;
+
+    if ((nargs != 4) || (nret != 1)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    option = rtas_ld(args, 3);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_handler) {
+        goto param_error_exit;
+    }
+
+    switch (option) {
+    case RTAS_SLOT_RESET_DEACTIVATE:
+    case RTAS_SLOT_RESET_HOT:
+    case RTAS_SLOT_RESET_FUNDAMENTAL:
+        break;
+    default:
+        goto param_error_exit;
+    }
+
+    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_RESET, option);
+    if (ret < 0) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args, uint32_t nret,
+                                  target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint64_t buid;
+    int ret;
+
+    if ((nargs != 3) || (nret != 1)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_handler) {
+        goto param_error_exit;
+    }
+
+    ret = spc->eeh_handler(sphb, RTAS_EEH_REQ_CONFIGURE, 0);
+    if (ret < 0) {
+        goto param_error_exit;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+/* To support it later */
+static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
+                                       sPAPREnvironment *spapr,
+                                       uint32_t token, uint32_t nargs,
+                                       target_ulong args, uint32_t nret,
+                                       target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    int option;
+    uint64_t buid;
+
+    if ((nargs != 8) || (nret != 1)) {
+        goto no_error_log;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto no_error_log;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_handler) {
+        goto no_error_log;
+    }
+
+    option = rtas_ld(args, 7);
+    switch (option) {
+    case RTAS_SLOT_TEMP_ERR_LOG:
+    case RTAS_SLOT_PERM_ERR_LOG:
+        break;
+    default:
+        goto no_error_log;
+    }
+
+    /* We don't get error log yet */
+    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
+    return;
+
+no_error_log:
+    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
+}
+
 static int pci_spapr_swizzle(int slot, int pin)
 {
     return (slot + pin) % PCI_NUM_PINS;
@@ -964,6 +1255,25 @@  void spapr_pci_rtas_init(void)
         spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi",
                             rtas_ibm_change_msi);
     }
+
+    spapr_rtas_register(RTAS_IBM_SET_EEH_OPTION,
+                        "ibm,set-eeh-option",
+                        rtas_ibm_set_eeh_option);
+    spapr_rtas_register(RTAS_IBM_GET_CONFIG_ADDR_INFO2,
+                        "ibm,get-config-addr-info2",
+                        rtas_ibm_get_config_addr_info2);
+    spapr_rtas_register(RTAS_IBM_READ_SLOT_RESET_STATE2,
+                        "ibm,read-slot-reset-state2",
+                        rtas_ibm_read_slot_reset_state2);
+    spapr_rtas_register(RTAS_IBM_SET_SLOT_RESET,
+                        "ibm,set-slot-reset",
+                        rtas_ibm_set_slot_reset);
+    spapr_rtas_register(RTAS_IBM_CONFIGURE_PE,
+                        "ibm,configure-pe",
+                        rtas_ibm_configure_pe);
+    spapr_rtas_register(RTAS_IBM_SLOT_ERROR_DETAIL,
+                        "ibm,slot-error-detail",
+                        rtas_ibm_slot_error_detail);
 }
 
 static void spapr_pci_register_types(void)
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 876ecf0..bfde083 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -49,6 +49,7 @@  struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
 
     void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
+    int (*eeh_handler)(sPAPRPHBState *sphb, int req, int opt);
 };
 
 typedef struct spapr_pci_msi {
@@ -109,6 +110,12 @@  struct sPAPRPHBVFIOState {
 
 #define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
 
+/* EEH related requests */
+#define RTAS_EEH_REQ_SET_OPTION      0
+#define RTAS_EEH_REQ_GET_STATE       1
+#define RTAS_EEH_REQ_RESET           2
+#define RTAS_EEH_REQ_CONFIGURE       3
+
 static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
 {
     return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 642cdc3..1cf5e89 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -338,6 +338,39 @@  target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
 int spapr_allocate_irq(int hint, bool lsi);
 int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 
+/* ibm,set-eeh-option */
+#define RTAS_EEH_DISABLE                 0
+#define RTAS_EEH_ENABLE                  1
+#define RTAS_EEH_THAW_IO                 2
+#define RTAS_EEH_THAW_DMA                3
+
+/* ibm,get-config-addr-info2 */
+#define RTAS_GET_PE_ADDR                 0
+#define RTAS_GET_PE_MODE                 1
+#define RTAS_PE_MODE_NONE                0
+#define RTAS_PE_MODE_NOT_SHARED          1
+#define RTAS_PE_MODE_SHARED              2
+
+/* ibm,read-slot-reset-state2 */
+#define RTAS_EEH_PE_STATE_NORMAL         0
+#define RTAS_EEH_PE_STATE_RESET          1
+#define RTAS_EEH_PE_STATE_STOPPED_IO_DMA 2
+#define RTAS_EEH_PE_STATE_STOPPED_DMA    4
+#define RTAS_EEH_PE_STATE_UNAVAIL        5
+#define RTAS_EEH_NOT_SUPPORT             0
+#define RTAS_EEH_SUPPORT                 1
+#define RTAS_EEH_PE_UNAVAIL_INFO         1000
+#define RTAS_EEH_PE_RECOVER_INFO         0
+
+/* ibm,set-slot-reset */
+#define RTAS_SLOT_RESET_DEACTIVATE       0
+#define RTAS_SLOT_RESET_HOT              1
+#define RTAS_SLOT_RESET_FUNDAMENTAL      3
+
+/* ibm,slot-error-detail */
+#define RTAS_SLOT_TEMP_ERR_LOG           1
+#define RTAS_SLOT_PERM_ERR_LOG           2
+
 /* RTAS return codes */
 #define RTAS_OUT_SUCCESS            0
 #define RTAS_OUT_NO_ERRORS_FOUND    1
@@ -382,8 +415,14 @@  int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
-
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_SET_EEH_OPTION                 (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_GET_CONFIG_ADDR_INFO2          (RTAS_TOKEN_BASE + 0x21)
+#define RTAS_IBM_READ_SLOT_RESET_STATE2         (RTAS_TOKEN_BASE + 0x22)
+#define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
+#define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
+#define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
+
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20