On Tue, Dec 23, 2014 at 03:22:06PM +1100, David Gibson wrote: >On Mon, Dec 15, 2014 at 11:15:07AM +1100, Gavin Shan wrote: >> The emulation for EEH RTAS requests from guest isn't covered >> by QEMU yet and the patch implements them. >> >> The patch defines constants used by EEH RTAS calls and adds >> callback sPAPRPHBClass::eeh_handler, which is going to be used >> this way: >> >> * RTAS calls are received in spapr_pci.c, sanity check is done >> there. >> * RTAS handlers handle what they can. If there is something it >> cannot handle and sPAPRPHBClass::eeh_handler callback is defined, >> it is called. >> * sPAPRPHBClass::eeh_handler is only implemented for VFIO now. It >> does ioctl() to the IOMMU container fd to complete the call. Error >> codes from that ioctl() are transferred back to the guest. >> >> [aik: defined RTAS tokens for EEH RTAS calls] >> Signed-off-by: Gavin Shan <gws...@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_pci.c | 246 >> ++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/pci-host/spapr.h | 7 ++ >> include/hw/ppc/spapr.h | 43 +++++++- >> 3 files changed, 294 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 3d70efe..3bb1971 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -406,6 +406,233 @@ 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 = spapr_pci_find_phb(spapr, buid); >> + sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >> + >> + if (!sphb || !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 = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); > >You're dereferencing RTAS parameters here before you've checked the >number of parameters, which isn't safe. Similar problem in the other >entry points as well. >
Yep, I'll fix it in next version. Thanks for review and pointing it out. >> + int ret; >> + >> + 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 (!spapr_pci_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); >> + return; >> + } > >The fall through here means that any failure in >rtas_handle_eeh_request will be reported as RTAS_OUT_PARAM_ERROR, >which doesn't sound like it would always be the right error code. >Similar in the other entry points. > Yes, Varied error code to indicate different failure cases will be better. I'll check PAPR spec again and return more precise error code in next version. 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