From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39712) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzlgI-000302-L9 for qemu-devel@nongnu.org; Wed, 25 Jun 2014 07:48:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WzlgA-0004BA-5v for qemu-devel@nongnu.org; Wed, 25 Jun 2014 07:48:02 -0400 Received: from cantor2.suse.de ([195.135.220.15]:45969 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wzlg9-0004B3-Ra for qemu-devel@nongnu.org; Wed, 25 Jun 2014 07:47:54 -0400 Message-ID: <53AAB6E8.7090703@suse.de> Date: Wed, 25 Jun 2014 13:47:52 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1402365785-31620-1-git-send-email-gwshan@linux.vnet.ibm.com> <1402365785-31620-2-git-send-email-gwshan@linux.vnet.ibm.com> <53A98E8B.7010302@suse.de> <20140624233829.GA5366@shangw> In-Reply-To: <20140624233829.GA5366@shangw> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v10 1/3] sPAPR: Implement EEH RTAS calls List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gavin Shan Cc: aik@ozlabs.ru, alex.williamson@redhat.com, qiudayu@linux.vnet.ibm.com, qemu-devel@nongnu.org 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 >>> --- >>> 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