* [Qemu-devel] [PATCH v15 0/2] EEH Support for VFIO Devices
@ 2015-01-05 0:26 Gavin Shan
2015-01-05 0:26 ` [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls Gavin Shan
2015-01-05 0:26 ` [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler Gavin Shan
0 siblings, 2 replies; 11+ messages in thread
From: Gavin Shan @ 2015-01-05 0:26 UTC (permalink / raw)
To: qemu-devel; +Cc: aik, agraf, Gavin Shan, alex.williamson, qemu-ppc, david
The series of patches adds support EEH for VFIO PCI devices on sPAPR platform.
It requires corresponding host kernel support, which was merged during 3.17
merge window. This patchset has been rebased to Alex Graf's QEMU repository:
git://github.com/agraf/qemu.git (branch: ppc-next)
The implementations notes are below. Please consider for merging!
* 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.
Changelog
=========
v12 -> v13:
* Rebase to Alex Graf's QEMU repository ("ppc-next" branch).
* Drop the patch for header file (vfio.h) changes, which was merged
to QEMU repository by commit a9fd1654 ("linux-headers: update to 3.17-rc7").
* Retested on Emulex adapter and EEH errors are recovered successfully.
v13 -> v14:
* Check if sPAPRPHBState instance is valid before converting it to the
corresponding class as pointed by Alex Graf.
v14 -> v15:
* Dropped unrelated patch making find_phb()/find_dev() public.
* Checking RTAS parameter number before accessing RTAS parameter buffer for
more safety.
* Return hardware error from RTAS call "ibm,set-eeh-option" and "ibm,set-slot-reset"
for some cases according to PAPR spec.
Gavin Shan (2):
sPAPR: Implement EEH RTAS calls
sPAPR: Implement sPAPRPHBClass::eeh_handler
hw/ppc/spapr_pci.c | 275 ++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_pci_vfio.c | 56 +++++++++
hw/vfio/common.c | 1 +
include/hw/pci-host/spapr.h | 7 ++
include/hw/ppc/spapr.h | 43 ++++++-
5 files changed, 380 insertions(+), 2 deletions(-)
--
1.8.3.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls 2015-01-05 0:26 [Qemu-devel] [PATCH v15 0/2] EEH Support for VFIO Devices Gavin Shan @ 2015-01-05 0:26 ` Gavin Shan 2015-01-14 1:39 ` David Gibson 2015-01-05 0:26 ` [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler Gavin Shan 1 sibling, 1 reply; 11+ messages in thread From: Gavin Shan @ 2015-01-05 0:26 UTC (permalink / raw) To: qemu-devel; +Cc: aik, agraf, Gavin Shan, alex.williamson, qemu-ppc, david 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 | 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; + } + + 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: + 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; + } + +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; + } + +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; + } + + 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 -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls 2015-01-05 0:26 ` [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls Gavin Shan @ 2015-01-14 1:39 ` David Gibson 2015-01-15 0:14 ` Gavin Shan 0 siblings, 1 reply; 11+ messages in thread From: David Gibson @ 2015-01-14 1:39 UTC (permalink / raw) To: Gavin Shan; +Cc: aik, alex.williamson, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 15679 bytes --] 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 <gwshan@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. 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. > + } > + > + 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? > + 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. > + > +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. > +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. > + } > + > + 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 [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls 2015-01-14 1:39 ` David Gibson @ 2015-01-15 0:14 ` Gavin Shan 2015-01-15 1:33 ` David Gibson 0 siblings, 1 reply; 11+ messages in thread From: Gavin Shan @ 2015-01-15 0:14 UTC (permalink / raw) To: David Gibson Cc: qemu-devel, aik, agraf, Gavin Shan, alex.williamson, qemu-ppc 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 <gwshan@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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls 2015-01-15 0:14 ` Gavin Shan @ 2015-01-15 1:33 ` David Gibson 2015-01-15 3:21 ` Gavin Shan 0 siblings, 1 reply; 11+ messages in thread From: David Gibson @ 2015-01-15 1:33 UTC (permalink / raw) To: Gavin Shan; +Cc: aik, alex.williamson, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 4136 bytes --] On Thu, Jan 15, 2015 at 11:14:36AM +1100, Gavin Shan wrote: > 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 <gwshan@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. Um.. I don't quite see why that makes returning RTAS erorr values difficult. But I think it's made irrelevant by your suggestion later. > >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. Yes, I think that might be the best idea, rtas_handle_eeh_request() seems like a bit of an odd multiplexer at the moment. [snip] > >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". Heh, ok. I think you still want to make the fall-through more obvious, it's easy to miss. -- 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 [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls 2015-01-15 1:33 ` David Gibson @ 2015-01-15 3:21 ` Gavin Shan 0 siblings, 0 replies; 11+ messages in thread From: Gavin Shan @ 2015-01-15 3:21 UTC (permalink / raw) To: David Gibson Cc: qemu-devel, aik, agraf, Gavin Shan, alex.williamson, qemu-ppc On Thu, Jan 15, 2015 at 12:33:59PM +1100, David Gibson wrote: >On Thu, Jan 15, 2015 at 11:14:36AM +1100, Gavin Shan wrote: >> 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 <gwshan@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. > >Um.. I don't quite see why that makes returning RTAS erorr values >difficult. But I think it's made irrelevant by your suggestion later. > Yeah, it's irrelevant after rtas_handle_eeh_request() is dropped. >> >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. > >Yes, I think that might be the best idea, rtas_handle_eeh_request() >seems like a bit of an odd multiplexer at the moment. > Thanks for confirm. I'll change code accordingly in next revision. >[snip] >> >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". > >Heh, ok. I think you still want to make the fall-through more >obvious, it's easy to miss. > Yes, I still need make it more obvious like this: int ret; ret = info->eeh_handler(); #if RTAS_CALL_SUPPORTS_HW_ERROR_AS_RETURN_VALUE if (ret < 0) { rtas_st(rets, 0, RTAS_OUT_HW_ERROR); return; } #else if (ret < 0) { goto param_error_exit; } #endif /* Handle the result from info->eeh_handler() */ rtas_st(rets, 0, RTAS_OUT_SUCCESS); return; param_error_exit: rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); Thanks, Gavin >-- >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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler 2015-01-05 0:26 [Qemu-devel] [PATCH v15 0/2] EEH Support for VFIO Devices Gavin Shan 2015-01-05 0:26 ` [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls Gavin Shan @ 2015-01-05 0:26 ` Gavin Shan 2015-01-14 1:41 ` David Gibson 1 sibling, 1 reply; 11+ messages in thread From: Gavin Shan @ 2015-01-05 0:26 UTC (permalink / raw) To: qemu-devel; +Cc: aik, agraf, Gavin Shan, alex.williamson, qemu-ppc, david The patch implements sPAPRPHBClass::eeh_handler so that the EEH RTAS requests can be routed to VFIO for further handling. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ hw/vfio/common.c | 1 + 2 files changed, 57 insertions(+) diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c index 144912b..73652a9 100644 --- a/hw/ppc/spapr_pci_vfio.c +++ b/hw/ppc/spapr_pci_vfio.c @@ -71,6 +71,61 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) spapr_tce_get_iommu(tcet)); } +static int spapr_phb_vfio_eeh_handler(sPAPRPHBState *sphb, int req, int opt) +{ + sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); + struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; + int cmd; + + switch (req) { + case RTAS_EEH_REQ_SET_OPTION: + switch (opt) { + case RTAS_EEH_DISABLE: + cmd = VFIO_EEH_PE_DISABLE; + break; + case RTAS_EEH_ENABLE: + cmd = VFIO_EEH_PE_ENABLE; + break; + case RTAS_EEH_THAW_IO: + cmd = VFIO_EEH_PE_UNFREEZE_IO; + break; + case RTAS_EEH_THAW_DMA: + cmd = VFIO_EEH_PE_UNFREEZE_DMA; + break; + default: + return -EINVAL; + } + break; + case RTAS_EEH_REQ_GET_STATE: + cmd = VFIO_EEH_PE_GET_STATE; + break; + case RTAS_EEH_REQ_RESET: + switch (opt) { + case RTAS_SLOT_RESET_DEACTIVATE: + cmd = VFIO_EEH_PE_RESET_DEACTIVATE; + break; + case RTAS_SLOT_RESET_HOT: + cmd = VFIO_EEH_PE_RESET_HOT; + break; + case RTAS_SLOT_RESET_FUNDAMENTAL: + cmd = VFIO_EEH_PE_RESET_FUNDAMENTAL; + break; + default: + return -EINVAL; + } + break; + case RTAS_EEH_REQ_CONFIGURE: + cmd = VFIO_EEH_PE_CONFIGURE; + break; + default: + return -EINVAL; + } + + op.op = cmd; + return vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, + VFIO_EEH_PE_OP, &op); +} + static void spapr_phb_vfio_reset(DeviceState *qdev) { /* Do nothing */ @@ -84,6 +139,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data) dc->props = spapr_phb_vfio_properties; dc->reset = spapr_phb_vfio_reset; spc->finish_realize = spapr_phb_vfio_finish_realize; + spc->eeh_handler = spapr_phb_vfio_eeh_handler; } static const TypeInfo spapr_phb_vfio_info = { diff --git a/hw/vfio/common.c b/hw/vfio/common.c index cf483ff..8a10c8b 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -948,6 +948,7 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid, switch (req) { case VFIO_CHECK_EXTENSION: case VFIO_IOMMU_SPAPR_TCE_GET_INFO: + case VFIO_EEH_PE_OP: break; default: /* Return an error on unknown requests */ -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler 2015-01-05 0:26 ` [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler Gavin Shan @ 2015-01-14 1:41 ` David Gibson 2015-01-15 0:25 ` Gavin Shan 2015-02-04 13:42 ` Alexander Graf 0 siblings, 2 replies; 11+ messages in thread From: David Gibson @ 2015-01-14 1:41 UTC (permalink / raw) To: Gavin Shan; +Cc: aik, alex.williamson, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 3880 bytes --] On Mon, Jan 05, 2015 at 11:26:28AM +1100, Gavin Shan wrote: > The patch implements sPAPRPHBClass::eeh_handler so that the > EEH RTAS requests can be routed to VFIO for further handling. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ > hw/vfio/common.c | 1 + > 2 files changed, 57 insertions(+) > > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > index 144912b..73652a9 100644 > --- a/hw/ppc/spapr_pci_vfio.c > +++ b/hw/ppc/spapr_pci_vfio.c > @@ -71,6 +71,61 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) > spapr_tce_get_iommu(tcet)); > } > > +static int spapr_phb_vfio_eeh_handler(sPAPRPHBState *sphb, int req, int opt) > +{ > + sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); > + struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; This is a local variable, which means it won't be initialized. You never memset() it and it's not obvious that all fields get initialized, which makes it dangerous to pass to an ioctl(). > + int cmd; > + > + switch (req) { > + case RTAS_EEH_REQ_SET_OPTION: > + switch (opt) { > + case RTAS_EEH_DISABLE: > + cmd = VFIO_EEH_PE_DISABLE; > + break; > + case RTAS_EEH_ENABLE: > + cmd = VFIO_EEH_PE_ENABLE; > + break; > + case RTAS_EEH_THAW_IO: > + cmd = VFIO_EEH_PE_UNFREEZE_IO; > + break; > + case RTAS_EEH_THAW_DMA: > + cmd = VFIO_EEH_PE_UNFREEZE_DMA; > + break; > + default: > + return -EINVAL; > + } > + break; > + case RTAS_EEH_REQ_GET_STATE: > + cmd = VFIO_EEH_PE_GET_STATE; > + break; > + case RTAS_EEH_REQ_RESET: > + switch (opt) { > + case RTAS_SLOT_RESET_DEACTIVATE: > + cmd = VFIO_EEH_PE_RESET_DEACTIVATE; > + break; > + case RTAS_SLOT_RESET_HOT: > + cmd = VFIO_EEH_PE_RESET_HOT; > + break; > + case RTAS_SLOT_RESET_FUNDAMENTAL: > + cmd = VFIO_EEH_PE_RESET_FUNDAMENTAL; > + break; > + default: > + return -EINVAL; > + } > + break; > + case RTAS_EEH_REQ_CONFIGURE: > + cmd = VFIO_EEH_PE_CONFIGURE; > + break; > + default: > + return -EINVAL; > + } > + > + op.op = cmd; > + return vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, > + VFIO_EEH_PE_OP, &op); Don't you need some sort of translation from the errnos the ioctl() returns into RTAS error codes? > +} > + > static void spapr_phb_vfio_reset(DeviceState *qdev) > { > /* Do nothing */ > @@ -84,6 +139,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data) > dc->props = spapr_phb_vfio_properties; > dc->reset = spapr_phb_vfio_reset; > spc->finish_realize = spapr_phb_vfio_finish_realize; > + spc->eeh_handler = spapr_phb_vfio_eeh_handler; > } > > static const TypeInfo spapr_phb_vfio_info = { > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index cf483ff..8a10c8b 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -948,6 +948,7 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid, > switch (req) { > case VFIO_CHECK_EXTENSION: > case VFIO_IOMMU_SPAPR_TCE_GET_INFO: > + case VFIO_EEH_PE_OP: > break; > default: > /* Return an error on unknown requests */ -- 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 [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler 2015-01-14 1:41 ` David Gibson @ 2015-01-15 0:25 ` Gavin Shan 2015-02-04 13:42 ` Alexander Graf 1 sibling, 0 replies; 11+ messages in thread From: Gavin Shan @ 2015-01-15 0:25 UTC (permalink / raw) To: David Gibson Cc: qemu-devel, aik, agraf, Gavin Shan, alex.williamson, qemu-ppc On Wed, Jan 14, 2015 at 12:41:59PM +1100, David Gibson wrote: >On Mon, Jan 05, 2015 at 11:26:28AM +1100, Gavin Shan wrote: >> The patch implements sPAPRPHBClass::eeh_handler so that the >> EEH RTAS requests can be routed to VFIO for further handling. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/vfio/common.c | 1 + >> 2 files changed, 57 insertions(+) >> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> index 144912b..73652a9 100644 >> --- a/hw/ppc/spapr_pci_vfio.c >> +++ b/hw/ppc/spapr_pci_vfio.c >> @@ -71,6 +71,61 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) >> spapr_tce_get_iommu(tcet)); >> } >> >> +static int spapr_phb_vfio_eeh_handler(sPAPRPHBState *sphb, int req, int opt) >> +{ >> + sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >> + struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; > >This is a local variable, which means it won't be initialized. You >never memset() it and it's not obvious that all fields get >initialized, which makes it dangerous to pass to an ioctl(). > Yeah, we might get uncertain "flags" passed to ioctl(). I'll change the code like this if you agree: struct vfio_eeh_pe_op op; memset(&op, 0, sizeof(op)); op.argsz = sizeof(op); >> + int cmd; >> + >> + switch (req) { >> + case RTAS_EEH_REQ_SET_OPTION: >> + switch (opt) { >> + case RTAS_EEH_DISABLE: >> + cmd = VFIO_EEH_PE_DISABLE; >> + break; >> + case RTAS_EEH_ENABLE: >> + cmd = VFIO_EEH_PE_ENABLE; >> + break; >> + case RTAS_EEH_THAW_IO: >> + cmd = VFIO_EEH_PE_UNFREEZE_IO; >> + break; >> + case RTAS_EEH_THAW_DMA: >> + cmd = VFIO_EEH_PE_UNFREEZE_DMA; >> + break; >> + default: >> + return -EINVAL; >> + } >> + break; >> + case RTAS_EEH_REQ_GET_STATE: >> + cmd = VFIO_EEH_PE_GET_STATE; >> + break; >> + case RTAS_EEH_REQ_RESET: >> + switch (opt) { >> + case RTAS_SLOT_RESET_DEACTIVATE: >> + cmd = VFIO_EEH_PE_RESET_DEACTIVATE; >> + break; >> + case RTAS_SLOT_RESET_HOT: >> + cmd = VFIO_EEH_PE_RESET_HOT; >> + break; >> + case RTAS_SLOT_RESET_FUNDAMENTAL: >> + cmd = VFIO_EEH_PE_RESET_FUNDAMENTAL; >> + break; >> + default: >> + return -EINVAL; >> + } >> + break; >> + case RTAS_EEH_REQ_CONFIGURE: >> + cmd = VFIO_EEH_PE_CONFIGURE; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + op.op = cmd; >> + return vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> + VFIO_EEH_PE_OP, &op); > >Don't you need some sort of translation from the errnos the ioctl() >returns into RTAS error codes? > Currently, errors from ioctl() is simply translated to RTAS_OUT_HW_ERROR if the RTAS call supports it. Otherwise, RTAS_OUT_PARAM_ERROR will be returned. Thanks, Gavin >> +} >> + >> static void spapr_phb_vfio_reset(DeviceState *qdev) >> { >> /* Do nothing */ >> @@ -84,6 +139,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data) >> dc->props = spapr_phb_vfio_properties; >> dc->reset = spapr_phb_vfio_reset; >> spc->finish_realize = spapr_phb_vfio_finish_realize; >> + spc->eeh_handler = spapr_phb_vfio_eeh_handler; >> } >> >> static const TypeInfo spapr_phb_vfio_info = { >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index cf483ff..8a10c8b 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -948,6 +948,7 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid, >> switch (req) { >> case VFIO_CHECK_EXTENSION: >> case VFIO_IOMMU_SPAPR_TCE_GET_INFO: >> + case VFIO_EEH_PE_OP: >> break; >> default: >> /* Return an error on unknown requests */ > >-- >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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler 2015-01-14 1:41 ` David Gibson 2015-01-15 0:25 ` Gavin Shan @ 2015-02-04 13:42 ` Alexander Graf 2015-02-04 13:44 ` David Gibson 1 sibling, 1 reply; 11+ messages in thread From: Alexander Graf @ 2015-02-04 13:42 UTC (permalink / raw) To: David Gibson, Gavin Shan; +Cc: aik, alex.williamson, qemu-ppc, qemu-devel On 14.01.15 02:41, David Gibson wrote: > On Mon, Jan 05, 2015 at 11:26:28AM +1100, Gavin Shan wrote: >> The patch implements sPAPRPHBClass::eeh_handler so that the >> EEH RTAS requests can be routed to VFIO for further handling. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/vfio/common.c | 1 + >> 2 files changed, 57 insertions(+) >> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> index 144912b..73652a9 100644 >> --- a/hw/ppc/spapr_pci_vfio.c >> +++ b/hw/ppc/spapr_pci_vfio.c >> @@ -71,6 +71,61 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) >> spapr_tce_get_iommu(tcet)); >> } >> >> +static int spapr_phb_vfio_eeh_handler(sPAPRPHBState *sphb, int req, int opt) >> +{ >> + sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >> + struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; > > This is a local variable, which means it won't be initialized. You > never memset() it and it's not obvious that all fields get > initialized, which makes it dangerous to pass to an ioctl(). As far as I understand C, in the construct above all unmentioned fields actually do get initialized to 0. Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler 2015-02-04 13:42 ` Alexander Graf @ 2015-02-04 13:44 ` David Gibson 0 siblings, 0 replies; 11+ messages in thread From: David Gibson @ 2015-02-04 13:44 UTC (permalink / raw) To: Alexander Graf; +Cc: aik, alex.williamson, qemu-ppc, Gavin Shan, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1695 bytes --] On Wed, Feb 04, 2015 at 02:42:00PM +0100, Alexander Graf wrote: > > > On 14.01.15 02:41, David Gibson wrote: > > On Mon, Jan 05, 2015 at 11:26:28AM +1100, Gavin Shan wrote: > >> The patch implements sPAPRPHBClass::eeh_handler so that the > >> EEH RTAS requests can be routed to VFIO for further handling. > >> > >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > >> --- > >> hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> hw/vfio/common.c | 1 + > >> 2 files changed, 57 insertions(+) > >> > >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > >> index 144912b..73652a9 100644 > >> --- a/hw/ppc/spapr_pci_vfio.c > >> +++ b/hw/ppc/spapr_pci_vfio.c > >> @@ -71,6 +71,61 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) > >> spapr_tce_get_iommu(tcet)); > >> } > >> > >> +static int spapr_phb_vfio_eeh_handler(sPAPRPHBState *sphb, int req, int opt) > >> +{ > >> + sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); > >> + struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; > > > > This is a local variable, which means it won't be initialized. You > > never memset() it and it's not obvious that all fields get > > initialized, which makes it dangerous to pass to an ioctl(). > > As far as I understand C, in the construct above all unmentioned fields > actually do get initialized to 0. Sorry, my mistake. -- 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 [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-02-04 13:44 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-05 0:26 [Qemu-devel] [PATCH v15 0/2] EEH Support for VFIO Devices Gavin Shan 2015-01-05 0:26 ` [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls Gavin Shan 2015-01-14 1:39 ` David Gibson 2015-01-15 0:14 ` Gavin Shan 2015-01-15 1:33 ` David Gibson 2015-01-15 3:21 ` Gavin Shan 2015-01-05 0:26 ` [Qemu-devel] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler Gavin Shan 2015-01-14 1:41 ` David Gibson 2015-01-15 0:25 ` Gavin Shan 2015-02-04 13:42 ` Alexander Graf 2015-02-04 13:44 ` David Gibson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).