On Wed, Jan 14, 2015 at 12:39:35PM +1100, David Gibson wrote: >On Mon, Jan 05, 2015 at 11:26:27AM +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 <gws...@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_pci.c | 275 >> ++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/pci-host/spapr.h | 7 ++ >> include/hw/ppc/spapr.h | 43 ++++++- >> 3 files changed, 323 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 21b95b3..a150074 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -406,6 +406,262 @@ static void >> rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu, >> rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */ >> } >> >> +static int rtas_handle_eeh_request(sPAPREnvironment *spapr, >> + uint64_t buid, uint32_t req, uint32_t >> opt) >> +{ >> + sPAPRPHBState *sphb = find_phb(spapr, buid); >> + sPAPRPHBClass *info; >> + >> + if (!sphb) { >> + return -ENODEV; > >I think it would make more sense to return RTAS error codes here, >rather than errnos. At present all the callers seem to ignore the >exact value of this return value. > >But it's not really correct to return RTAS_OUT_HW_ERROR for a bad >BUID, which is what this will do now. >
It makes sense: RTAS_OUT_PARAM_ERROR, instead of RTAS_OUT_HW_ERROR should be returned for invalid sPAPRPHBState and sPAPRPHBClass (as below). It's a bit hard to have RTAS_OUT_* as the function's return value because RTAS_OUT_PARAM_ERROR should be returned for some RTAS calls even info->eeh_handler() returns negative value. >Also several of the callers have already done a find_phb() by the time >they call this. Perhaps it would make more sense for this function to >take a sPAPRPHBState * instead of the buid. > It's not sure that find_phb() called by callers() before calling this function. We did call find_dev() before calling this function for some cases. How about changing the code like this way: Drop rtas_handle_eeh_request() and put the logic into its callers, which would give more flexibility for the callers to return proper values. >> + } >> + >> + info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >> + if (!info->eeh_handler) { >> + return -ENOENT; >> + } >> + >> + return info->eeh_handler(sphb, req, opt); >> +} >> + >> +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) >> +{ >> + 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); >> + 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: > >So, currently these are all implemented as no-ops. Is that correct? > Currently, each PHB is binding to one PE. rtas_handle_eeh_request() checks sPAPRPHBClass::eeh_handler accordingly, which is enough. >> + break; >> + default: >> + goto param_error_exit; >> + } >> + >> + ret = rtas_handle_eeh_request(spapr, buid, >> + RTAS_EEH_REQ_SET_OPTION, option); >> + if (ret >= 0) { >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> + } else { >> + rtas_st(rets, 0, RTAS_OUT_HW_ERROR); >> + } >> + >> + 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) >> +{ >> + uint32_t addr, option; >> + uint64_t buid; >> + sPAPRPHBState *sphb; >> + sPAPRPHBClass *info; >> + PCIDevice *pdev; >> + >> + 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; >> + } >> + >> + info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >> + if (!info->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. >> + */ >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> + 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); >> + } >> + >> + 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) >> +{ >> + 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); >> + ret = rtas_handle_eeh_request(spapr, buid, RTAS_EEH_REQ_GET_STATE, 0); >> + if (ret >= 0) { >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> + 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); >> + } >> + >> + return; >> + } > >The ret < 0 case isn't handled here. It will fall through to >param_error_exit, which is non-obvious, and it also seems unlikely >that a parameter error is the only possible thing that can go wrong. > Yeah, PAPR spec states the return value RTAS_OUT_SUCCESS or RTAS_OUT_PARAMETER_ERROR. There is no RTAS_OUT_HW_ERROR for this RTAS call "ibm,read-slot-reset-state2". >> + >> +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) >> +{ >> + 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); >> + switch (option) { >> + case RTAS_SLOT_RESET_DEACTIVATE: >> + case RTAS_SLOT_RESET_HOT: >> + case RTAS_SLOT_RESET_FUNDAMENTAL: >> + break; >> + default: >> + goto param_error_exit; >> + } >> + >> + ret = rtas_handle_eeh_request(spapr, buid, RTAS_EEH_REQ_RESET, option); >> + if (ret >= 0) { >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> + } else { >> + rtas_st(rets, 0, RTAS_OUT_HW_ERROR); >> + } >> + >> + 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) >> +{ >> + 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); >> + ret = rtas_handle_eeh_request(spapr, buid, RTAS_EEH_REQ_CONFIGURE, 0); >> + if (ret >= 0) { >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> + return; >> + } > >Again ret < 0 case isn't handled. > As as above, PAPR spec suggests return value RTAS_OUT_SUCCESS or RTAS_OUT_PARAM_ERROR for this RTAS call "ibm,configure-pe". There is no RTAS_OUT_HW_ERROR from this call. >> +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) >> +{ >> + int option; >> + uint64_t buid; >> + sPAPRPHBState *sphb; >> + sPAPRPHBClass *info; >> + >> + if ((nargs != 8) || (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; >> + } >> + >> + info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >> + if (!info->eeh_handler) { >> + goto param_error_exit; > >PARAM_ERROR doesn't sound like the right thing when the PHB doesn't >suppor the operation. I don't have access to PAPR to look for a more >appropriate error code, though. > PAPR suggests following error code. It even doesn't have RTAS_OUT_PARAM_OUT as its return value. I'll change the code to return "1" in next revision if you agree. 1: No Error Log Returned 0: Success -1: Hardware Error (cannot create log) Thanks, Gavin >> + } >> + >> + option = rtas_ld(args, 7); >> + switch (option) { >> + case RTAS_SLOT_TEMP_ERR_LOG: >> + case RTAS_SLOT_PERM_ERR_LOG: >> + break; >> + default: >> + goto param_error_exit; >> + } >> + >> + rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND); >> + return; >> + >> +param_error_exit: >> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); >> +} >> + >> static int pci_spapr_swizzle(int slot, int pin) >> { >> return (slot + pin) % PCI_NUM_PINS; >> @@ -958,6 +1214,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 4ea2a0d..ffe0faa 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 { >> @@ -107,6 +108,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 716bff4..89ec13c 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