* [Qemu-devel] [PATCH v10 0/3] EEH Support for VFIO PCI Device
@ 2014-06-10 2:03 Gavin Shan
2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls Gavin Shan
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Gavin Shan @ 2014-06-10 2:03 UTC (permalink / raw)
To: qemu-devel; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu
The series of patches adds support EEH for VFIO PCI devices on sPAPR platform.
It requires corresponding host kernel support. Also, it is based on top of
Alexey's VFIO-for-sPAPR git repository.
QEMU: git://github.com/aik/qemu.git (branch: vfio)
Kernel: git://github.com/aik/linux.git (branch: vfio)
Kernel: http://linuxppc.10917.n7.nabble.com/PATCH-v10-0-3-EEH-Support-for-VFIO-PCI-Device-td83042.html
The implementations notes are below. Please comment.
* 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
=========
v8 -> v9:
* Update kernel header (vfio.h) according to changes applied to kerenl.
* Rename rtas_finish_eeh_request() to rtas_handle_eeh_request().
* vfio_pci_container_ioctl() moved to Alexey's VFIO patchset.
v9 -> v10:
* Make sure we have zero struct vfio_eeh_pe_op::flags
Gavin Shan (3):
sPAPR: Implement EEH RTAS calls
headers: Update kernel header
sPAPR: Implement sPAPRPHBClass::eeh_handler
hw/ppc/spapr_pci.c | 248 ++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_pci_vfio.c | 56 ++++++++++
include/hw/pci-host/spapr.h | 7 ++
include/hw/ppc/spapr.h | 33 ++++++
linux-headers/linux/vfio.h | 34 ++++++
5 files changed, 378 insertions(+)
--
1.8.3.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls 2014-06-10 2:03 [Qemu-devel] [PATCH v10 0/3] EEH Support for VFIO PCI Device Gavin Shan @ 2014-06-10 2:03 ` Gavin Shan 2014-06-24 14:43 ` Alexander Graf 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 2/3] headers: Update kernel header Gavin Shan 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler Gavin Shan 2 siblings, 1 reply; 11+ messages in thread From: Gavin Shan @ 2014-06-10 2:03 UTC (permalink / raw) To: qemu-devel; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu 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: 1. RTAS calls are received in spapr_pci.c, sanity check is done there. 2. RTAS handlers handle what they can. If there is something it cannot handle and sPAPRPHBClass::eeh_handler callback is defined, it is called. 3. 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. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- hw/ppc/spapr_pci.c | 248 ++++++++++++++++++++++++++++++++++++++++++++ include/hw/pci-host/spapr.h | 7 ++ include/hw/ppc/spapr.h | 33 ++++++ 3 files changed, 288 insertions(+) diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 131434b..bfea488 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -422,6 +422,241 @@ 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(sPAPRPHBState *sphb, + uint32_t req, uint32_t opt, + target_ulong rets) +{ + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); + int ret; + + ret = info->eeh_handler(sphb, req, opt); + if (ret >= 0) { + rtas_st(rets, 0, RTAS_OUT_SUCCESS); + } else { + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); + } + + return ret; +} + +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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); + + if (!sphb || !info->eeh_handler) { + goto param_error_exit; + } + + if ((nargs != 4) || (nret != 1)) { + goto param_error_exit; + } + + 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; + } + + rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_SET_OPTION, option, rets); + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); + PCIDevice *pdev; + + if (!sphb || !info->eeh_handler) { + goto param_error_exit; + } + + if ((nargs != 4) || (nret != 2)) { + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); + int32_t ret; + + if (!sphb || !info->eeh_handler) { + goto param_error_exit; + } + + if ((nargs != 3) || (nret != 4 && nret != 5)) { + goto param_error_exit; + } + + ret = rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_GET_STATE, 0, rets); + if (ret >= 0) { + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); + + if (!sphb || !info->eeh_handler) { + goto param_error_exit; + } + + if ((nargs != 4) || (nret != 1)) { + goto param_error_exit; + } + + option = rtas_ld(args, 3); + if (option != RTAS_SLOT_RESET_DEACTIVATE && + option != RTAS_SLOT_RESET_HOT && + option != RTAS_SLOT_RESET_FUNDAMENTAL) { + goto param_error_exit; + } + + rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_RESET, option, rets); + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); + + if (!sphb || !info->eeh_handler) { + goto param_error_exit; + } + + if ((nargs != 3) || (nret != 1)) { + goto param_error_exit; + } + + rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_CONFIGURE, 0, rets); + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); + + if (!sphb || !info->eeh_handler) { + goto param_error_exit; + } + + if ((nargs != 8) || (nret != 1)) { + goto param_error_exit; + } + + option = rtas_ld(args, 7); + if (option != RTAS_SLOT_TEMP_ERR_LOG && + option != RTAS_SLOT_PERM_ERR_LOG) { + goto param_error_exit; + } + + rtas_st(rets, 0, RTAS_OUT_SUCCESS); + 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; @@ -939,6 +1174,19 @@ void spapr_pci_rtas_init(void) rtas_ibm_query_interrupt_source_number); spapr_rtas_register("ibm,change-msi", rtas_ibm_change_msi); } + + spapr_rtas_register("ibm,set-eeh-option", + rtas_ibm_set_eeh_option); + spapr_rtas_register("ibm,get-config-addr-info2", + rtas_ibm_get_config_addr_info2); + spapr_rtas_register("ibm,read-slot-reset-state2", + rtas_ibm_read_slot_reset_state2); + spapr_rtas_register("ibm,set-slot-reset", + rtas_ibm_set_slot_reset); + spapr_rtas_register("ibm,configure-pe", + rtas_ibm_configure_pe); + spapr_rtas_register("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 caeecfa..f487f23 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -51,6 +51,7 @@ struct sPAPRPHBClass { PCIHostBridgeClass parent_class; void (*finish_realize)(sPAPRPHBState *sphb, Error **errp); + int (*eeh_handler)(sPAPRPHBState *sphb, int req, int opt); }; struct sPAPRPHBState { @@ -104,6 +105,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 6faae5d..ea84de2 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -354,6 +354,39 @@ static inline int spapr_allocate_lsi(int hint) return spapr_allocate_irq(hint, true); } +/* 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 -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls Gavin Shan @ 2014-06-24 14:43 ` Alexander Graf 2014-06-24 23:38 ` Gavin Shan 0 siblings, 1 reply; 11+ messages in thread From: Alexander Graf @ 2014-06-24 14:43 UTC (permalink / raw) To: Gavin Shan, qemu-devel; +Cc: aik, alex.williamson, qiudayu On 10.06.14 04:03, 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: > > 1. RTAS calls are received in spapr_pci.c, sanity check is done > there. > 2. RTAS handlers handle what they can. If there is something it > cannot handle and sPAPRPHBClass::eeh_handler callback is defined, > it is called. > 3. 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. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > hw/ppc/spapr_pci.c | 248 ++++++++++++++++++++++++++++++++++++++++++++ > include/hw/pci-host/spapr.h | 7 ++ > include/hw/ppc/spapr.h | 33 ++++++ > 3 files changed, 288 insertions(+) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 131434b..bfea488 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -422,6 +422,241 @@ 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(sPAPRPHBState *sphb, > + uint32_t req, uint32_t opt, > + target_ulong rets) > +{ > + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > + int ret; > + > + ret = info->eeh_handler(sphb, req, opt); > + if (ret >= 0) { > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > + } else { > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + } > + > + return ret; I think you're better off failing the eeh_handler() call in here when the function is not implemented. That way all callers don't have to explicitly check for it. I also find it a lot less confusing if the return value doesn't get set inside this helper. > +} > + > +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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); > + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); > + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > + > + if (!sphb || !info->eeh_handler) { > + goto param_error_exit; > + } > + > + if ((nargs != 4) || (nret != 1)) { > + goto param_error_exit; > + } > + > + 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; > + } > + > + rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_SET_OPTION, option, rets); > + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); > + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); > + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > + PCIDevice *pdev; > + > + if (!sphb || !info->eeh_handler) { > + goto param_error_exit; > + } > + > + if ((nargs != 4) || (nret != 2)) { > + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); > + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); > + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > + int32_t ret; > + > + if (!sphb || !info->eeh_handler) { > + goto param_error_exit; > + } > + > + if ((nargs != 3) || (nret != 4 && nret != 5)) { > + goto param_error_exit; > + } > + > + ret = rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_GET_STATE, 0, rets); > + if (ret >= 0) { > + 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); > + } > + } It's really awkward to not see an rtas_st(0) here. > + > + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); > + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); > + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > + > + if (!sphb || !info->eeh_handler) { > + goto param_error_exit; > + } > + > + if ((nargs != 4) || (nret != 1)) { > + goto param_error_exit; > + } > + > + option = rtas_ld(args, 3); > + if (option != RTAS_SLOT_RESET_DEACTIVATE && > + option != RTAS_SLOT_RESET_HOT && > + option != RTAS_SLOT_RESET_FUNDAMENTAL) { > + goto param_error_exit; > + } Please make this a switch(). > + > + rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_RESET, option, rets); > + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); > + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); > + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > + > + if (!sphb || !info->eeh_handler) { > + goto param_error_exit; > + } > + > + if ((nargs != 3) || (nret != 1)) { > + goto param_error_exit; > + } > + > + rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_CONFIGURE, 0, rets); > + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); > + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); > + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > + > + if (!sphb || !info->eeh_handler) { > + goto param_error_exit; > + } > + > + if ((nargs != 8) || (nret != 1)) { > + goto param_error_exit; > + } > + > + option = rtas_ld(args, 7); > + if (option != RTAS_SLOT_TEMP_ERR_LOG && > + option != RTAS_SLOT_PERM_ERR_LOG) { > + goto param_error_exit; > + } switch() again. Alex > + > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > + 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; > @@ -939,6 +1174,19 @@ void spapr_pci_rtas_init(void) > rtas_ibm_query_interrupt_source_number); > spapr_rtas_register("ibm,change-msi", rtas_ibm_change_msi); > } > + > + spapr_rtas_register("ibm,set-eeh-option", > + rtas_ibm_set_eeh_option); > + spapr_rtas_register("ibm,get-config-addr-info2", > + rtas_ibm_get_config_addr_info2); > + spapr_rtas_register("ibm,read-slot-reset-state2", > + rtas_ibm_read_slot_reset_state2); > + spapr_rtas_register("ibm,set-slot-reset", > + rtas_ibm_set_slot_reset); > + spapr_rtas_register("ibm,configure-pe", > + rtas_ibm_configure_pe); > + spapr_rtas_register("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 caeecfa..f487f23 100644 > --- a/include/hw/pci-host/spapr.h > +++ b/include/hw/pci-host/spapr.h > @@ -51,6 +51,7 @@ struct sPAPRPHBClass { > PCIHostBridgeClass parent_class; > > void (*finish_realize)(sPAPRPHBState *sphb, Error **errp); > + int (*eeh_handler)(sPAPRPHBState *sphb, int req, int opt); > }; > > struct sPAPRPHBState { > @@ -104,6 +105,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 6faae5d..ea84de2 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -354,6 +354,39 @@ static inline int spapr_allocate_lsi(int hint) > return spapr_allocate_irq(hint, true); > } > > +/* 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls 2014-06-24 14:43 ` Alexander Graf @ 2014-06-24 23:38 ` Gavin Shan 2014-06-25 11:47 ` Alexander Graf 0 siblings, 1 reply; 11+ messages in thread From: Gavin Shan @ 2014-06-24 23:38 UTC (permalink / raw) To: Alexander Graf; +Cc: aik, Gavin Shan, qemu-devel, alex.williamson, qiudayu On Tue, Jun 24, 2014 at 04:43:23PM +0200, Alexander Graf wrote: > >On 10.06.14 04:03, 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: >> >>1. RTAS calls are received in spapr_pci.c, sanity check is done >> there. >>2. RTAS handlers handle what they can. If there is something it >> cannot handle and sPAPRPHBClass::eeh_handler callback is defined, >> it is called. >>3. 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. >> >>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>--- >> hw/ppc/spapr_pci.c | 248 ++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/pci-host/spapr.h | 7 ++ >> include/hw/ppc/spapr.h | 33 ++++++ >> 3 files changed, 288 insertions(+) >> >>diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>index 131434b..bfea488 100644 >>--- a/hw/ppc/spapr_pci.c >>+++ b/hw/ppc/spapr_pci.c >>@@ -422,6 +422,241 @@ 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(sPAPRPHBState *sphb, >>+ uint32_t req, uint32_t opt, >>+ target_ulong rets) >>+{ >>+ sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>+ int ret; >>+ >>+ ret = info->eeh_handler(sphb, req, opt); >>+ if (ret >= 0) { >>+ rtas_st(rets, 0, RTAS_OUT_SUCCESS); >>+ } else { >>+ rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); >>+ } >>+ >>+ return ret; > >I think you're better off failing the eeh_handler() call in here when >the function is not implemented. That way all callers don't have to >explicitly check for it. > Yep, it makes the logic of the function complete. I'll fix in v11. >I also find it a lot less confusing if the return value doesn't get >set inside this helper. > Yeah, I agree and will fix in v11. >>+} >>+ >>+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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>+ sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>+ sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>+ >>+ if (!sphb || !info->eeh_handler) { >>+ goto param_error_exit; >>+ } >>+ >>+ if ((nargs != 4) || (nret != 1)) { >>+ goto param_error_exit; >>+ } >>+ >>+ 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; >>+ } >>+ >>+ rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_SET_OPTION, option, rets); >>+ 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>+ sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>+ sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>+ PCIDevice *pdev; >>+ >>+ if (!sphb || !info->eeh_handler) { >>+ goto param_error_exit; >>+ } >>+ >>+ if ((nargs != 4) || (nret != 2)) { >>+ 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>+ sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>+ sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>+ int32_t ret; >>+ >>+ if (!sphb || !info->eeh_handler) { >>+ goto param_error_exit; >>+ } >>+ >>+ if ((nargs != 3) || (nret != 4 && nret != 5)) { >>+ goto param_error_exit; >>+ } >>+ >>+ ret = rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_GET_STATE, 0, rets); >>+ if (ret >= 0) { >>+ 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); >>+ } >>+ } > >It's really awkward to not see an rtas_st(0) here. > Yeah. As you mentioned above, "rtas_st(0)" would make the function complete (from view of logic). Thanks and I'll fix in v11. >>+ >>+ 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>+ sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>+ sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>+ >>+ if (!sphb || !info->eeh_handler) { >>+ goto param_error_exit; >>+ } >>+ >>+ if ((nargs != 4) || (nret != 1)) { >>+ goto param_error_exit; >>+ } >>+ >>+ option = rtas_ld(args, 3); >>+ if (option != RTAS_SLOT_RESET_DEACTIVATE && >>+ option != RTAS_SLOT_RESET_HOT && >>+ option != RTAS_SLOT_RESET_FUNDAMENTAL) { >>+ goto param_error_exit; >>+ } > >Please make this a switch(). > Sure, but may I ask "switch()" could improve the code readability or save a bit .text space, or it's easy to be extended in future? :-) >>+ >>+ rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_RESET, option, rets); >>+ 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>+ sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>+ sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>+ >>+ if (!sphb || !info->eeh_handler) { >>+ goto param_error_exit; >>+ } >>+ >>+ if ((nargs != 3) || (nret != 1)) { >>+ goto param_error_exit; >>+ } >>+ >>+ rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_CONFIGURE, 0, rets); >>+ 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>+ sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>+ sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>+ >>+ if (!sphb || !info->eeh_handler) { >>+ goto param_error_exit; >>+ } >>+ >>+ if ((nargs != 8) || (nret != 1)) { >>+ goto param_error_exit; >>+ } >>+ >>+ option = rtas_ld(args, 7); >>+ if (option != RTAS_SLOT_TEMP_ERR_LOG && >>+ option != RTAS_SLOT_PERM_ERR_LOG) { >>+ goto param_error_exit; >>+ } > >switch() again. > Ok, but same question as above. Thanks, Gavin > >Alex > >>+ >>+ rtas_st(rets, 0, RTAS_OUT_SUCCESS); >>+ 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; >>@@ -939,6 +1174,19 @@ void spapr_pci_rtas_init(void) >> rtas_ibm_query_interrupt_source_number); >> spapr_rtas_register("ibm,change-msi", rtas_ibm_change_msi); >> } >>+ >>+ spapr_rtas_register("ibm,set-eeh-option", >>+ rtas_ibm_set_eeh_option); >>+ spapr_rtas_register("ibm,get-config-addr-info2", >>+ rtas_ibm_get_config_addr_info2); >>+ spapr_rtas_register("ibm,read-slot-reset-state2", >>+ rtas_ibm_read_slot_reset_state2); >>+ spapr_rtas_register("ibm,set-slot-reset", >>+ rtas_ibm_set_slot_reset); >>+ spapr_rtas_register("ibm,configure-pe", >>+ rtas_ibm_configure_pe); >>+ spapr_rtas_register("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 caeecfa..f487f23 100644 >>--- a/include/hw/pci-host/spapr.h >>+++ b/include/hw/pci-host/spapr.h >>@@ -51,6 +51,7 @@ struct sPAPRPHBClass { >> PCIHostBridgeClass parent_class; >> void (*finish_realize)(sPAPRPHBState *sphb, Error **errp); >>+ int (*eeh_handler)(sPAPRPHBState *sphb, int req, int opt); >> }; >> struct sPAPRPHBState { >>@@ -104,6 +105,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 6faae5d..ea84de2 100644 >>--- a/include/hw/ppc/spapr.h >>+++ b/include/hw/ppc/spapr.h >>@@ -354,6 +354,39 @@ static inline int spapr_allocate_lsi(int hint) >> return spapr_allocate_irq(hint, true); >> } >>+/* 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls 2014-06-24 23:38 ` Gavin Shan @ 2014-06-25 11:47 ` Alexander Graf 0 siblings, 0 replies; 11+ messages in thread From: Alexander Graf @ 2014-06-25 11:47 UTC (permalink / raw) To: Gavin Shan; +Cc: aik, alex.williamson, qiudayu, qemu-devel On 25.06.14 01:38, Gavin Shan wrote: > On Tue, Jun 24, 2014 at 04:43:23PM +0200, Alexander Graf wrote: >> On 10.06.14 04:03, 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: >>> >>> 1. RTAS calls are received in spapr_pci.c, sanity check is done >>> there. >>> 2. RTAS handlers handle what they can. If there is something it >>> cannot handle and sPAPRPHBClass::eeh_handler callback is defined, >>> it is called. >>> 3. 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. >>> >>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>> --- >>> hw/ppc/spapr_pci.c | 248 ++++++++++++++++++++++++++++++++++++++++++++ >>> include/hw/pci-host/spapr.h | 7 ++ >>> include/hw/ppc/spapr.h | 33 ++++++ >>> 3 files changed, 288 insertions(+) >>> >>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>> index 131434b..bfea488 100644 >>> --- a/hw/ppc/spapr_pci.c >>> +++ b/hw/ppc/spapr_pci.c >>> @@ -422,6 +422,241 @@ 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(sPAPRPHBState *sphb, >>> + uint32_t req, uint32_t opt, >>> + target_ulong rets) >>> +{ >>> + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>> + int ret; >>> + >>> + ret = info->eeh_handler(sphb, req, opt); >>> + if (ret >= 0) { >>> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >>> + } else { >>> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); >>> + } >>> + >>> + return ret; >> I think you're better off failing the eeh_handler() call in here when >> the function is not implemented. That way all callers don't have to >> explicitly check for it. >> > Yep, it makes the logic of the function complete. I'll fix in v11. > >> I also find it a lot less confusing if the return value doesn't get >> set inside this helper. >> > Yeah, I agree and will fix in v11. > >>> +} >>> + >>> +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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>> + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>> + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>> + >>> + if (!sphb || !info->eeh_handler) { >>> + goto param_error_exit; >>> + } >>> + >>> + if ((nargs != 4) || (nret != 1)) { >>> + goto param_error_exit; >>> + } >>> + >>> + 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; >>> + } >>> + >>> + rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_SET_OPTION, option, rets); >>> + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>> + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>> + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>> + PCIDevice *pdev; >>> + >>> + if (!sphb || !info->eeh_handler) { >>> + goto param_error_exit; >>> + } >>> + >>> + if ((nargs != 4) || (nret != 2)) { >>> + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>> + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>> + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>> + int32_t ret; >>> + >>> + if (!sphb || !info->eeh_handler) { >>> + goto param_error_exit; >>> + } >>> + >>> + if ((nargs != 3) || (nret != 4 && nret != 5)) { >>> + goto param_error_exit; >>> + } >>> + >>> + ret = rtas_handle_eeh_request(sphb, RTAS_EEH_REQ_GET_STATE, 0, rets); >>> + if (ret >= 0) { >>> + 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); >>> + } >>> + } >> It's really awkward to not see an rtas_st(0) here. >> > Yeah. As you mentioned above, "rtas_st(0)" would make the function > complete (from view of logic). Thanks and I'll fix in v11. > >>> + >>> + 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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >>> + sPAPRPHBState *sphb = spapr_find_phb(spapr, buid); >>> + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>> + >>> + if (!sphb || !info->eeh_handler) { >>> + goto param_error_exit; >>> + } >>> + >>> + if ((nargs != 4) || (nret != 1)) { >>> + goto param_error_exit; >>> + } >>> + >>> + option = rtas_ld(args, 3); >>> + if (option != RTAS_SLOT_RESET_DEACTIVATE && >>> + option != RTAS_SLOT_RESET_HOT && >>> + option != RTAS_SLOT_RESET_FUNDAMENTAL) { >>> + goto param_error_exit; >>> + } >> Please make this a switch(). >> > Sure, but may I ask "switch()" could improve the code readability > or save a bit .text space, or it's easy to be extended in future? :-) It's mostly just readability :). It's a lot easier to understand a switch() at a glimpse than ANDed option != foo statements. The resulting assembly code should be pretty much identical. Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v10 2/3] headers: Update kernel header 2014-06-10 2:03 [Qemu-devel] [PATCH v10 0/3] EEH Support for VFIO PCI Device Gavin Shan 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls Gavin Shan @ 2014-06-10 2:03 ` Gavin Shan 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler Gavin Shan 2 siblings, 0 replies; 11+ messages in thread From: Gavin Shan @ 2014-06-10 2:03 UTC (permalink / raw) To: qemu-devel; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu This updates kernel header (vfio.h) for EEH support on VFIO PCI devices. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- linux-headers/linux/vfio.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h index f0aa97d..6dfaf16 100644 --- a/linux-headers/linux/vfio.h +++ b/linux-headers/linux/vfio.h @@ -30,6 +30,9 @@ */ #define VFIO_DMA_CC_IOMMU 4 +/* Check if EEH is supported */ +#define VFIO_EEH 5 + /* * The IOCTL interface is designed for extensibility by embedding the * structure length (argsz) and flags into structures passed between @@ -490,6 +493,37 @@ struct vfio_iommu_spapr_tce_reset { }; #define VFIO_IOMMU_SPAPR_TCE_RESET _IO(VFIO_TYPE, VFIO_BASE + 20) +/* + * EEH PE operation struct provides ways to: + * - enable/disable EEH functionality; + * - unfreeze IO/DMA for frozen PE; + * - read PE state; + * - reset PE; + * - configure PE. + */ +struct vfio_eeh_pe_op { + __u32 argsz; + __u32 flags; + __u32 op; +}; + +#define VFIO_EEH_PE_DISABLE 0 /* Disable EEH functionality */ +#define VFIO_EEH_PE_ENABLE 1 /* Enable EEH functionality */ +#define VFIO_EEH_PE_UNFREEZE_IO 2 /* Enable IO for frozen PE */ +#define VFIO_EEH_PE_UNFREEZE_DMA 3 /* Enable DMA for frozen PE */ +#define VFIO_EEH_PE_GET_STATE 4 /* PE state retrieval */ +#define VFIO_EEH_PE_STATE_NORMAL 0 /* PE in functional state */ +#define VFIO_EEH_PE_STATE_RESET 1 /* PE reset in progress */ +#define VFIO_EEH_PE_STATE_STOPPED 2 /* Stopped DMA and IO */ +#define VFIO_EEH_PE_STATE_STOPPED_DMA 4 /* Stopped DMA only */ +#define VFIO_EEH_PE_STATE_UNAVAIL 5 /* State unavailable */ +#define VFIO_EEH_PE_RESET_DEACTIVATE 5 /* Deassert PE reset */ +#define VFIO_EEH_PE_RESET_HOT 6 /* Assert hot reset */ +#define VFIO_EEH_PE_RESET_FUNDAMENTAL 7 /* Assert fundamental reset */ +#define VFIO_EEH_PE_CONFIGURE 8 /* PE configuration */ + +#define VFIO_EEH_PE_OP _IO(VFIO_TYPE, VFIO_BASE + 21) + /* ***************************************************************** */ #endif /* VFIO_H */ -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler 2014-06-10 2:03 [Qemu-devel] [PATCH v10 0/3] EEH Support for VFIO PCI Device Gavin Shan 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls Gavin Shan 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 2/3] headers: Update kernel header Gavin Shan @ 2014-06-10 2:03 ` Gavin Shan 2014-06-11 20:26 ` Alex Williamson 2 siblings, 1 reply; 11+ messages in thread From: Gavin Shan @ 2014-06-10 2:03 UTC (permalink / raw) To: qemu-devel; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu 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 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c index 592d6a4..9750cf0 100644 --- a/hw/ppc/spapr_pci_vfio.c +++ b/hw/ppc/spapr_pci_vfio.c @@ -85,6 +85,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), .flags = 0 }; + 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 */ @@ -98,6 +153,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 = { -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler Gavin Shan @ 2014-06-11 20:26 ` Alex Williamson 2014-06-12 0:02 ` Gavin Shan 0 siblings, 1 reply; 11+ messages in thread From: Alex Williamson @ 2014-06-11 20:26 UTC (permalink / raw) To: Gavin Shan; +Cc: aik, qiudayu, qemu-devel, agraf On Tue, 2014-06-10 at 12:03 +1000, 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 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > index 592d6a4..9750cf0 100644 > --- a/hw/ppc/spapr_pci_vfio.c > +++ b/hw/ppc/spapr_pci_vfio.c > @@ -85,6 +85,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), .flags = 0 }; FWIW, flags = 0 isn't actually necessary. I'm sure someone here can quote the C spec, but it's my understanding that if any field of a structure is initialized, the remaining fields are zero initialized. vfio.c has a mix of initializations depending on whether using an explicit value for flags adds to the code clarity. > + 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 */ > @@ -98,6 +153,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 = { ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler 2014-06-11 20:26 ` Alex Williamson @ 2014-06-12 0:02 ` Gavin Shan 2014-06-12 1:37 ` Alex Williamson 0 siblings, 1 reply; 11+ messages in thread From: Gavin Shan @ 2014-06-12 0:02 UTC (permalink / raw) To: Alex Williamson; +Cc: qemu-devel, aik, agraf, Gavin Shan, qiudayu On Wed, Jun 11, 2014 at 02:26:51PM -0600, Alex Williamson wrote: >On Tue, 2014-06-10 at 12:03 +1000, 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 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> index 592d6a4..9750cf0 100644 >> --- a/hw/ppc/spapr_pci_vfio.c >> +++ b/hw/ppc/spapr_pci_vfio.c >> @@ -85,6 +85,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), .flags = 0 }; > >FWIW, flags = 0 isn't actually necessary. I'm sure someone here can >quote the C spec, but it's my understanding that if any field of a >structure is initialized, the remaining fields are zero initialized. >vfio.c has a mix of initializations depending on whether using an >explicit value for flags adds to the code clarity. > Yes, but it's not harmful. Please let me know if you want me to remove it :-) I had a very quick experiment on x86 and Power Linux with following tiny program and the result is just what you think: With "struct test foo" in func2(): func2: foo.a=0xffffffff, foo.b=0xffffffff with "static struct test foo" in func2(). Here's the explaining about this: section 2.4.2.3 of http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members func2: foo.a=0x00000000, foo.b=0x00000000 with "struct test foo = { .a = 0 }" in func2(). func2: foo.a=0x00000000, foo.b=0x00000000 With "struct test foo = { 0 }" in func2(): func2: foo.a=0x00000000, foo.b=0x00000000 --- #include <stdio.h> struct test { int a; int b; }; static func1(void) { int var[1000]; int i; for (i = 0; i < 1000; i++) var[i] = 0xffffffff; } static func2(void) { struct test foo; printf("%s: foo.a=0x%08x, foo.b=0x%08x\n", __func__, foo.a, foo.b); } int main(int argc, char **argv) { func1(); func2(); return 0; } Thanks, Gavin >> + 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 */ >> @@ -98,6 +153,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 = { > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler 2014-06-12 0:02 ` Gavin Shan @ 2014-06-12 1:37 ` Alex Williamson 2014-06-16 1:24 ` Gavin Shan 0 siblings, 1 reply; 11+ messages in thread From: Alex Williamson @ 2014-06-12 1:37 UTC (permalink / raw) To: Gavin Shan; +Cc: aik, qiudayu, qemu-devel, agraf On Thu, 2014-06-12 at 10:02 +1000, Gavin Shan wrote: > On Wed, Jun 11, 2014 at 02:26:51PM -0600, Alex Williamson wrote: > >On Tue, 2014-06-10 at 12:03 +1000, 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 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 56 insertions(+) > >> > >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > >> index 592d6a4..9750cf0 100644 > >> --- a/hw/ppc/spapr_pci_vfio.c > >> +++ b/hw/ppc/spapr_pci_vfio.c > >> @@ -85,6 +85,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), .flags = 0 }; > > > >FWIW, flags = 0 isn't actually necessary. I'm sure someone here can > >quote the C spec, but it's my understanding that if any field of a > >structure is initialized, the remaining fields are zero initialized. > >vfio.c has a mix of initializations depending on whether using an > >explicit value for flags adds to the code clarity. > > > > Yes, but it's not harmful. Please let me know if you want me to remove > it :-) It's ok, explicit initialization doesn't hurt anything here. The series looks ok to me, but it depends on the header update, so it needs to wait for that to happen in the kernel. I provided my ack for the other series, but let me know if I need to push the vfio changes through my tree. Thanks, Alex > I had a very quick experiment on x86 > and Power Linux with following tiny program and the result is just > what you think: > > With "struct test foo" in func2(): > func2: foo.a=0xffffffff, foo.b=0xffffffff > with "static struct test foo" in func2(). Here's the explaining about > this: section 2.4.2.3 of http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members > func2: foo.a=0x00000000, foo.b=0x00000000 > with "struct test foo = { .a = 0 }" in func2(). > func2: foo.a=0x00000000, foo.b=0x00000000 > With "struct test foo = { 0 }" in func2(): > func2: foo.a=0x00000000, foo.b=0x00000000 > > --- > > #include <stdio.h> > > struct test { > int a; > int b; > }; > > static func1(void) > { > int var[1000]; > int i; > > for (i = 0; i < 1000; i++) > var[i] = 0xffffffff; > } > > static func2(void) > { > struct test foo; > > printf("%s: foo.a=0x%08x, foo.b=0x%08x\n", > __func__, foo.a, foo.b); > } > > int main(int argc, char **argv) > { > func1(); > func2(); > > return 0; > } > > Thanks, > Gavin > > >> + 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 */ > >> @@ -98,6 +153,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 = { > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler 2014-06-12 1:37 ` Alex Williamson @ 2014-06-16 1:24 ` Gavin Shan 0 siblings, 0 replies; 11+ messages in thread From: Gavin Shan @ 2014-06-16 1:24 UTC (permalink / raw) To: Alex Williamson; +Cc: qemu-devel, aik, agraf, Gavin Shan, qiudayu On Wed, Jun 11, 2014 at 07:37:48PM -0600, Alex Williamson wrote: >On Thu, 2014-06-12 at 10:02 +1000, Gavin Shan wrote: >> On Wed, Jun 11, 2014 at 02:26:51PM -0600, Alex Williamson wrote: >> >On Tue, 2014-06-10 at 12:03 +1000, 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 +++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 56 insertions(+) >> >> >> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> >> index 592d6a4..9750cf0 100644 >> >> --- a/hw/ppc/spapr_pci_vfio.c >> >> +++ b/hw/ppc/spapr_pci_vfio.c >> >> @@ -85,6 +85,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), .flags = 0 }; >> > >> >FWIW, flags = 0 isn't actually necessary. I'm sure someone here can >> >quote the C spec, but it's my understanding that if any field of a >> >structure is initialized, the remaining fields are zero initialized. >> >vfio.c has a mix of initializations depending on whether using an >> >explicit value for flags adds to the code clarity. >> > >> >> Yes, but it's not harmful. Please let me know if you want me to remove >> it :-) > >It's ok, explicit initialization doesn't hurt anything here. The series >looks ok to me, but it depends on the header update, so it needs to wait >for that to happen in the kernel. I provided my ack for the other >series, but let me know if I need to push the vfio changes through my >tree. Thanks, > Thanks, Alex. The kernel part should be merged firstly. All the stuff (kernel & QEMU part) depends on Alexey's VFIO stuff. So lets wait until Alexey's VFIO stuff gets merged. That time, I guess I probably have to rebase and send out a new revision (with your ack of course). Thanks, Gavin >> I had a very quick experiment on x86 >> and Power Linux with following tiny program and the result is just >> what you think: >> >> With "struct test foo" in func2(): >> func2: foo.a=0xffffffff, foo.b=0xffffffff >> with "static struct test foo" in func2(). Here's the explaining about >> this: section 2.4.2.3 of http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members >> func2: foo.a=0x00000000, foo.b=0x00000000 >> with "struct test foo = { .a = 0 }" in func2(). >> func2: foo.a=0x00000000, foo.b=0x00000000 >> With "struct test foo = { 0 }" in func2(): >> func2: foo.a=0x00000000, foo.b=0x00000000 >> >> --- >> >> #include <stdio.h> >> >> struct test { >> int a; >> int b; >> }; >> >> static func1(void) >> { >> int var[1000]; >> int i; >> >> for (i = 0; i < 1000; i++) >> var[i] = 0xffffffff; >> } >> >> static func2(void) >> { >> struct test foo; >> >> printf("%s: foo.a=0x%08x, foo.b=0x%08x\n", >> __func__, foo.a, foo.b); >> } >> >> int main(int argc, char **argv) >> { >> func1(); >> func2(); >> >> return 0; >> } >> >> Thanks, >> Gavin >> >> >> + 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 */ >> >> @@ -98,6 +153,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 = { >> > >> > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-06-25 11:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-10 2:03 [Qemu-devel] [PATCH v10 0/3] EEH Support for VFIO PCI Device Gavin Shan 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls Gavin Shan 2014-06-24 14:43 ` Alexander Graf 2014-06-24 23:38 ` Gavin Shan 2014-06-25 11:47 ` Alexander Graf 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 2/3] headers: Update kernel header Gavin Shan 2014-06-10 2:03 ` [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler Gavin Shan 2014-06-11 20:26 ` Alex Williamson 2014-06-12 0:02 ` Gavin Shan 2014-06-12 1:37 ` Alex Williamson 2014-06-16 1:24 ` Gavin Shan
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).