From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54614) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRs3D-0007m4-Qt for qemu-devel@nongnu.org; Tue, 18 Aug 2015 21:20:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZRs39-0007E5-RK for qemu-devel@nongnu.org; Tue, 18 Aug 2015 21:20:23 -0400 Date: Tue, 18 Aug 2015 18:20:25 -0700 From: David Gibson Message-ID: <20150819012025.GL3157@voom> 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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="NqNl6FRZtoRUn5bW" Content-Disposition: inline In-Reply-To: <20150819002604.GA9458@gwshan> 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, Thomas Huth , qemu-ppc@nongnu.org, qemu-devel@nongnu.org --NqNl6FRZtoRUn5bW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 19, 2015 at 10:26:04AM +1000, 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. > >>=20 > >> 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(-) > >>=20 > >> 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(pa= ram_buf, 1); > >> + mask =3D ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(pa= ram_buf, 3); > >> + buid =3D ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(pa= ram_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 that = and > use rtas_ld() directly. I agree with David that we don't have to maintain > another function, which is rarely used. I'm not necessarily opposed to an rtas_ldq() or similar, but I'd prefer to see it done as a separate cleanup patch. > >> + 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(pa= ram_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 syste= ms? If > some one tries to build this source file for non-powerpc systems, it will= throw > error and force users to check, which isn't bad actually. Using a VFIO device with a TCG guest is possible, at least theoretically. So this needs to be fixed. > >> #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(sPAPRPHBS= tate *sphb) > >> return RTAS_OUT_SUCCESS; > >> } > >> =20 > >> +static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb, > >> + uint32_t func, uint64_t ad= dr, > >> + uint64_t mask, bool is_64b= its) > >> +{ > >> + 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! >=20 > >> + op.err.func =3D func; > >> + break; > >> + default: > >> + ret =3D RTAS_OUT_PARAM_ERROR; > >> + goto out; > >> + } > >> + > >> + if (vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupi= d, > >> + VFIO_EEH_PE_OP, &op) < 0) { > >> + ret =3D RTAS_OUT_HW_ERROR; > >> + goto out; > >> + } > >> + > >> + ret =3D RTAS_OUT_SUCCESS; > >> +out: > >> + return ret; > >> +} > >> + > >> static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data) > >> { > >> DeviceClass *dc =3D DEVICE_CLASS(klass); > >> @@ -262,6 +317,7 @@ static void spapr_phb_vfio_class_init(ObjectClass = *klass, void *data) > >> spc->eeh_get_state =3D spapr_phb_vfio_eeh_get_state; > >> spc->eeh_reset =3D spapr_phb_vfio_eeh_reset; > >> spc->eeh_configure =3D spapr_phb_vfio_eeh_configure; > >> + spc->eeh_inject_error =3D spapr_phb_vfio_eeh_inject_error; > >> } > >> =20 > >> static const TypeInfo spapr_phb_vfio_info =3D { > >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > >> index 8405056..5645f43 100644 > >> --- a/hw/ppc/spapr_rtas.c > >> +++ b/hw/ppc/spapr_rtas.c > >> @@ -632,6 +632,54 @@ out: > >> rtas_st(rets, 1, ret); > >> } > >> =20 > >> +static void rtas_ibm_errinjct(PowerPCCPU *cpu, > >> + sPAPRMachineState *spapr, > >> + uint32_t token, uint32_t nargs, > >> + target_ulong args, uint32_t nret, > >> + target_ulong rets) > >> +{ > >> + target_ulong param_buf; > >> + uint32_t type, open_token; > >> + int32_t ret; > >> + > >> + /* Sanity check on number of arguments */ > >> + if ((nargs !=3D 3) || (nret !=3D 1)) { > > > >Less paranthesis, please? > > >=20 > Sure. >=20 > >> + ret =3D RTAS_OUT_PARAM_ERROR; > >> + goto out; > >> + } > >> + > >> + /* Check if we have opened token */ > >> + open_token =3D rtas_ld(args, 1); > >> + if (!spapr->is_errinjct_opened || > >> + spapr->errinjct_token !=3D open_token) { > >> + ret =3D RTAS_OUT_CLOSE_ERROR; > >> + goto out; > >> + } > >> + > >> + /* The parameter buffer should be 1KB aligned */ > >> + param_buf =3D rtas_ld(args, 2); > >> + if (param_buf & 0x3ff) { > >> + ret =3D RTAS_OUT_PARAM_ERROR; > >> + goto out; > >> + } > >> + > >> + /* Check the error type */ > >> + type =3D rtas_ld(args, 0); > >> + switch (type) { > >> + case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR: > >> + ret =3D spapr_rtas_errinjct_ioa(spapr, param_buf, false); > >> + break; > >> + case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64: > >> + ret =3D spapr_rtas_errinjct_ioa(spapr, param_buf, true); > >> + break; > >> + default: > >> + ret =3D RTAS_OUT_PARAM_ERROR; > >> + } > >> + > >> +out: > >> + rtas_st(rets, 0, ret); > >> +} > >> + > >> static void rtas_ibm_close_errinjct(PowerPCCPU *cpu, > >> sPAPRMachineState *spapr, > >> uint32_t token, uint32_t nargs, > >> @@ -723,6 +771,33 @@ int spapr_rtas_device_tree_setup(void *fdt, hwadd= r 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_str= ings[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 use= d by > userland utility "errinjct" like below. So I think qemu needs pass BE tok= ens > and the utility also needs do conversion if necessary. >=20 > ei_funcs[i].rtas_token =3D *(int *)tmp_ptr; /* tmp_ptr is pointing t= o data tream > * read from /rtas/ibm,errinjct-tokens */ Yes, values you put into the device tree should definitely be BE. > >> + offset +=3D sizeof(int); > >> + } > >> + > >> + fdt_offset =3D fdt_path_offset(fdt, "/rtas"); > >> + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,errinjct-tokens", > >> + errinjct_tokens, offset); > >> + if (ret < 0) { > >> + fprintf(stderr, "Couldn't add ibm,errinjct-tokens\n"); > >> + return ret; > >> + } >=20 > Thanks, > Gavin >=20 --=20 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 --NqNl6FRZtoRUn5bW Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV09nZAAoJEGw4ysog2bOS2XEQALXU+GP3gjCHBbzza95EqZwq wGf5t9OsYtQrjzymVorGpSeoHCLSTu4jDuG9oz8+acbZpGn6HfBM82GVsewStoWe Ur6XYEyz4642vLr/UJQWvBU0Mq35mo6vA4mkA6EZaj41N7dCD3v8mQ7+JQyF6QgI 0irJmZOL1Uwk4qJ3lE5dF1gB6sPa+3xpNvC+xOgpgt7dolVzWpV8GM3l3ksDFtve F3LyhbXMwigfE6ex2rdaqFmU0dvvFA/vY/QRYZcQIrYv1zotUzOk/pCbwhmPgABd moxqlDH9Oyiv6ebEvuAavIt227eV5gdTE6GFp4NGx+p1hRRyLhrczUlaU/qFmrkd Kd9tvWblPfivnjeP/IOAMLVGHgdAaqbivBZTHjv+q8mugT1RIJiNqnBnehxFsyKn T6jG1kZ+jxIgb+sGIGc/1BcqaIicEN6xwMkE8o0FjBXPnP341MptM3urr3RNNALn EgfZ+wZRIbJixOYpVC3u8hLFb5yRColXixVTHQdQi/EDlPMRlqGP+JVF7s0BVomY 2wJF4s3qKJ0u2R0FwxU05DWPhb9DsDWIwF1M78n2yLIBmRpHnIqsNXYHr4aOfSgl dSdoWwVb/flJB2CLXskgIxyITuKMKSh6XdroE1lDfqyLDlaKGk4dW4E+4WrxnKyM ABP+X8REGUDPLUoWNdX8 =heGM -----END PGP SIGNATURE----- --NqNl6FRZtoRUn5bW--