From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50985) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0g3o-0006zZ-EZ for qemu-devel@nongnu.org; Mon, 15 Dec 2014 19:32:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0g3f-0003fl-B4 for qemu-devel@nongnu.org; Mon, 15 Dec 2014 19:32:20 -0500 Received: from e28smtp09.in.ibm.com ([122.248.162.9]:45338) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0g3e-0003ew-N3 for qemu-devel@nongnu.org; Mon, 15 Dec 2014 19:32:11 -0500 Received: from /spool/local by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 16 Dec 2014 06:02:04 +0530 Date: Tue, 16 Dec 2014 11:31:59 +1100 From: Gavin Shan Message-ID: <20141216003158.GA27388@shangw> References: <1418602508-30845-1-git-send-email-gwshan@linux.vnet.ibm.com> <1418602508-30845-3-git-send-email-gwshan@linux.vnet.ibm.com> <548EF5A1.2040105@suse.de> <20141215230820.GA1113@shangw> <548F6AFF.1000000@suse.de> <20141215232916.GA18136@shangw> <548F7814.7030605@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <548F7814.7030605@suse.de> Subject: Re: [Qemu-devel] [PATCH v13 2/3] sPAPR: Implement EEH RTAS calls Reply-To: Gavin Shan List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: aik@ozlabs.ru, Gavin Shan , qemu-devel@nongnu.org, alex.williamson@redhat.com, qemu-ppc@nongnu.org On Tue, Dec 16, 2014 at 01:08:52AM +0100, Alexander Graf wrote: > > >On 16.12.14 00:29, Gavin Shan wrote: >> On Tue, Dec 16, 2014 at 12:13:03AM +0100, Alexander Graf wrote: >>> On 16.12.14 00:08, Gavin Shan wrote: >>>> On Mon, Dec 15, 2014 at 03:52:17PM +0100, Alexander Graf wrote: >>>>> On 15.12.14 01:15, 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 >>>>>> --- >>>>>> 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); >>>>> >>>>> What happens when you try to cast NULL? Could a guest process invoke a >>>>> host assert() through this and abort the whole VM? >>>>> >>>> >>>> Yes, it would cause core dump. I had one experiment to force assigning >>>> NULL to "sphb" before doing the cast, the whole VM is aborted. So I >>>> guess you're happy to have something as follows. If you're not suggesting >>>> something else, I'll update the code as follows in next version: >>>> >>>> sPAPRPHBState *sphb = spapr_pci_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); >>> >>> Yes, I think this is a lot safer. And yes, the other patch looks sane to me. >>> >> >> Thank you for your time reviewing this. Will update in next version. >> >>>> >>>>>> + >>>>>> + 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); >>>>>> + 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: >>>>> >>>>> So these don't use the addr hint? >>>>> >>>> >>>> No, there're no address as argument of this RTAS call "ibm,set-eeh-option". >>>> The RTAS call has 4 arguments, all of them are 32-bits: BUID high part, BUID >>>> low part, PE address, option. The option could be one of: enable/disable EEH >>>> functionality, enable IO path, enable DMA path. >>> >>> Well, I'm just wondering that ENABLE wants to make sure there's a device >>> and the others don't. >>> >> >> Oh, I misunderstood your question. Yes, you're correct. All options >> except ENABLE will have address check in rtas_handle_eeh_request() >> where we check on "BUID" since each PHB and PE have one-to-one >> relationship. >> >> ENABLE and other options are using different address: enable >> uses config address of one specific device, but other options use PE >> address. From guest's sides, it enables EEH capability on basis >> of PCI device and left options are supported on basis of PE. Each >> PE could contain one or multiple PCI devices. >> >> DISABLE option isn't used until now. > >So would DISABLE also take effect on devfn basis or would it ignore the >first parameter? > >If it behaves the same as ENABLE (which would be logical), please move >it into the same case group. > DISABLE takes effect on PE basis. We don't have devfn for the case and code needn't changes. Here's the concise procedures if DISABLE needs to be involved: - ENABLE on basis of PCI device - RTAS call "ibm,get-config-addr-info2" to get the PE address - DISABLE on basis of PE, with PE address as argument. If you don't have anything else, I would like to change the code and send new version to you. Thanks again for your time. Thanks, Gavin