From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52346) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZS5bO-00043D-FX for qemu-devel@nongnu.org; Wed, 19 Aug 2015 11:48:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZS5bJ-00067Z-RP for qemu-devel@nongnu.org; Wed, 19 Aug 2015 11:48:34 -0400 Message-ID: <55D4A544.4040800@redhat.com> Date: Wed, 19 Aug 2015 08:48:20 -0700 From: Thomas Huth MIME-Version: 1.0 References: <1439862430-14996-1-git-send-email-gwshan@linux.vnet.ibm.com> <1439862430-14996-5-git-send-email-gwshan@linux.vnet.ibm.com> <55D373CB.7040905@redhat.com> <20150819002604.GA9458@gwshan> In-Reply-To: <20150819002604.GA9458@gwshan> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 4/4] sPAPR: Support RTAS call ibm, errinjct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gavin Shan Cc: aik@ozlabs.ru, peter.maydell@linaro.org, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, david@gibson.dropbear.id.au On 18/08/15 17:26, Gavin Shan wrote: > On Tue, Aug 18, 2015 at 11:04:59AM -0700, Thomas Huth wrote: >> On 17/08/15 18:47, Gavin Shan wrote: >>> The patch supports RTAS call "ibm,errinjct" to allow injecting >>> EEH errors to VFIO PCI devices. The implementation is similiar >>> to EEH support for VFIO PCI devices: The RTAS request is captured >>> by QEMU and routed to sPAPRPHBClass::eeh_inject_error() where the >>> request is translated to VFIO container IOCTL command to be handled >>> by the host. >>> >>> Signed-off-by: Gavin Shan >>> --- >>> hw/ppc/spapr_pci.c | 36 +++++++++++++++++++++ >>> hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++ >>> hw/ppc/spapr_rtas.c | 77 +++++++++++++++++++++++++++++++++++= ++++++++++ >>> include/hw/pci-host/spapr.h | 2 ++ >>> include/hw/ppc/spapr.h | 9 +++++- >>> 5 files changed, 179 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>> index 9d41060..f6223ce 100644 >>> --- a/hw/ppc/spapr_pci.c >>> +++ b/hw/ppc/spapr_pci.c >>> @@ -682,6 +682,42 @@ param_error_exit: >>> rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); >>> } >>> =20 >>> +int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr, >>> + target_ulong param_buf, >>> + bool is_64bits) >>> +{ >>> + sPAPRPHBState *sphb; >>> + sPAPRPHBClass *spc; >>> + uint64_t buid, addr, mask; >>> + uint32_t func; >>> + >>> + if (is_64bits) { >>> + addr =3D ((uint64_t)rtas_ld(param_buf, 0) << 32) | rtas_ld(p= aram_buf, 1); >>> + mask =3D ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(p= aram_buf, 3); >>> + buid =3D ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(p= aram_buf, 6); >> >> You might want to consider to introduce a helper function (e.g >> "ras_ld64"?) that loads the two 32 bit values and combines them. >> >=20 > In v1, I had rtas_ldq() for 64-bits values. David suggested to drop tha= t and > use rtas_ld() directly. I agree with David that we don't have to mainta= in > another function, which is rarely used. There are also other spots in the code that load a 64-bit value that way, so they could be reworked, too... Anyway, if you and David don't like this idea, simply never mind, it's not that important. >>> + func =3D rtas_ld(param_buf, 7); >>> + } else { >>> + addr =3D rtas_ld(param_buf, 0); >>> + mask =3D rtas_ld(param_buf, 1); >>> + buid =3D ((uint64_t)rtas_ld(param_buf, 3) << 32) | rtas_ld(p= aram_buf, 4); >>> + func =3D rtas_ld(param_buf, 5); >>> + } >>> + >>> + /* Find PHB */ >>> + sphb =3D spapr_pci_find_phb(spapr, buid); >>> + if (!sphb) { >>> + return RTAS_OUT_PARAM_ERROR; >>> + } >>> + >>> + spc =3D SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); >>> + if (!spc->eeh_inject_error) { >>> + return RTAS_OUT_PARAM_ERROR; >>> + } >>> + >>> + /* Handle the request */ >>> + return spc->eeh_inject_error(sphb, func, addr, mask, is_64bits); >>> +} >>> + >>> static int pci_spapr_swizzle(int slot, int pin) >>> { >>> return (slot + pin) % PCI_NUM_PINS; >>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >>> index cca45ed..a3674ee 100644 >>> --- a/hw/ppc/spapr_pci_vfio.c >>> +++ b/hw/ppc/spapr_pci_vfio.c >>> @@ -17,6 +17,8 @@ >>> * along with this program; if not, see . >>> */ >>> =20 >>> +#include >> >> This does not work when building on non-powerpc systems. I think you >> have to use something like this instead: >> >> #include "asm-powerpc/eeh.h" >> >=20 > The question is how hw/ppc/spapr_pci_vfio.c is built on non-powerpc sys= tems? If > some one tries to build this source file for non-powerpc systems, it wi= ll throw > error and force users to check, which isn't bad actually. Simply try to compile qemu-softmmu-pp64 on your x86 laptop (with TCG)! The spapr_pci_vfio.c file is also compiled there, and if you use "", you break the build! >>> #include "hw/ppc/spapr.h" >>> #include "hw/pci-host/spapr.h" >>> #include "hw/pci/msix.h" >>> @@ -250,6 +252,59 @@ static int spapr_phb_vfio_eeh_configure(sPAPRPHB= State *sphb) >>> return RTAS_OUT_SUCCESS; >>> } >>> =20 >>> +static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb, >>> + uint32_t func, uint64_t a= ddr, >>> + uint64_t mask, bool is_64= bits) >>> +{ >>> + sPAPRPHBVFIOState *svphb =3D SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >>> + struct vfio_eeh_pe_op op =3D { >>> + .op =3D VFIO_EEH_PE_INJECT_ERR, >>> + .argsz =3D sizeof(op) >>> + }; >>> + int ret =3D RTAS_OUT_SUCCESS; >>> + >>> + op.err.type =3D is_64bits ? EEH_ERR_TYPE_64 : EEH_ERR_TYPE_32; >>> + op.err.addr =3D addr; >>> + op.err.mask =3D mask; >>> + >>> + switch (func) { >>> + case EEH_ERR_FUNC_LD_MEM_ADDR: >>> + case EEH_ERR_FUNC_LD_MEM_DATA: >>> + case EEH_ERR_FUNC_LD_IO_ADDR: >>> + case EEH_ERR_FUNC_LD_IO_DATA: >>> + case EEH_ERR_FUNC_LD_CFG_ADDR: >>> + case EEH_ERR_FUNC_LD_CFG_DATA: >>> + case EEH_ERR_FUNC_ST_MEM_ADDR: >>> + case EEH_ERR_FUNC_ST_MEM_DATA: >>> + case EEH_ERR_FUNC_ST_IO_ADDR: >>> + case EEH_ERR_FUNC_ST_IO_DATA: >>> + case EEH_ERR_FUNC_ST_CFG_ADDR: >>> + case EEH_ERR_FUNC_ST_CFG_DATA: >>> + case EEH_ERR_FUNC_DMA_RD_ADDR: >>> + case EEH_ERR_FUNC_DMA_RD_DATA: >>> + case EEH_ERR_FUNC_DMA_RD_MASTER: >>> + case EEH_ERR_FUNC_DMA_RD_TARGET: >>> + case EEH_ERR_FUNC_DMA_WR_ADDR: >>> + case EEH_ERR_FUNC_DMA_WR_DATA: >>> + case EEH_ERR_FUNC_DMA_WR_MASTER: >> >> EEH_ERR_FUNC_DMA_WR_TARGET is missing in the list ... I assume this is >> on purpose? >> >=20 > Good catch! Ok, so if you want to test for all the defined cases from eeh.h, wouldn't it be easier to simply check if (func > EEH_ERR_FUNC_MAX) { ret =3D RTAS_OUT_PARAM_ERROR; goto out; } ? [...] >>> @@ -723,6 +771,33 @@ int spapr_rtas_device_tree_setup(void *fdt, hwad= dr rtas_addr, >>> int i; >>> uint32_t lrdr_capacity[5]; >>> MachineState *machine =3D MACHINE(qdev_get_machine()); >>> + char errinjct_tokens[1024]; >>> + int fdt_offset, offset; >>> + const int tokens[] =3D { >>> + RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR, >>> + RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64 >>> + }; >>> + const char *token_strings[] =3D { >>> + "ioa-bus-error", >>> + "ioa-bus-error-64" >>> + }; >>> + >>> + /* ibm,errinjct-tokens */ >>> + offset =3D 0; >>> + for (i =3D 0; i < ARRAY_SIZE(tokens); i++) { >>> + offset +=3D sprintf(errinjct_tokens + offset, "%s", token_st= rings[i]); >>> + errinjct_tokens[offset++] =3D '\0'; >>> + *(int *)(&errinjct_tokens[offset]) =3D tokens[i]; >> >> You can also get rid of some paranthesis here. Also I am not sure, but= I >> think you have to take care of the endianess here? =3D=3D> Use stl_be_= p instead? >=20 > Good question! Currently, the property (/rtas/ibm,errinjct-tokens) is u= sed by > userland utility "errinjct" like below. So I think qemu needs pass BE t= okens > and the utility also needs do conversion if necessary. Right, otherwise cross-endian setup won't work. Thomas