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.
> >>
> >> Signed-off-by: Gavin Shan <gws...@linux.vnet.ibm.com>
> >> ---
> >> 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);
> >> }
> >>
> >> +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 = ((uint64_t)rtas_ld(param_buf, 0) << 32) |
> >> rtas_ld(param_buf, 1);
> >> + mask = ((uint64_t)rtas_ld(param_buf, 2) << 32) |
> >> rtas_ld(param_buf, 3);
> >> + buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) |
> >> rtas_ld(param_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.
> >
>
> 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 = rtas_ld(param_buf, 7);
> >> + } else {
> >> + addr = rtas_ld(param_buf, 0);
> >> + mask = rtas_ld(param_buf, 1);
> >> + buid = ((uint64_t)rtas_ld(param_buf, 3) << 32) |
> >> rtas_ld(param_buf, 4);
> >> + func = rtas_ld(param_buf, 5);
> >> + }
> >> +
> >> + /* Find PHB */
> >> + sphb = spapr_pci_find_phb(spapr, buid);
> >> + if (!sphb) {
> >> + return RTAS_OUT_PARAM_ERROR;
> >> + }
> >> +
> >> + spc = 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 <http://www.gnu.org/licenses/>.
> >> */
> >>
> >> +#include <asm/eeh.h>
> >
> >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"
> >
>
> The question is how hw/ppc/spapr_pci_vfio.c is built on non-powerpc systems?
> 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(sPAPRPHBState
> >> *sphb)
> >> return RTAS_OUT_SUCCESS;
> >> }
> >>
> >> +static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb,
> >> + uint32_t func, uint64_t addr,
> >> + uint64_t mask, bool is_64bits)
> >> +{
> >> + sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> >> + struct vfio_eeh_pe_op op = {
> >> + .op = VFIO_EEH_PE_INJECT_ERR,
> >> + .argsz = sizeof(op)
> >> + };
> >> + int ret = RTAS_OUT_SUCCESS;
> >> +
> >> + op.err.type = is_64bits ? EEH_ERR_TYPE_64 : EEH_ERR_TYPE_32;
> >> + op.err.addr = addr;
> >> + op.err.mask = 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?
> >
>
> Good catch!
>
> >> + op.err.func = func;
> >> + break;
> >> + default:
> >> + ret = RTAS_OUT_PARAM_ERROR;
> >> + goto out;
> >> + }
> >> +
> >> + if (vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> >> + VFIO_EEH_PE_OP, &op) < 0) {
> >> + ret = RTAS_OUT_HW_ERROR;
> >> + goto out;
> >> + }
> >> +
> >> + ret = RTAS_OUT_SUCCESS;
> >> +out:
> >> + return ret;
> >> +}
> >> +
> >> static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
> >> {
> >> DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -262,6 +317,7 @@ static void spapr_phb_vfio_class_init(ObjectClass
> >> *klass, void *data)
> >> spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
> >> spc->eeh_reset = spapr_phb_vfio_eeh_reset;
> >> spc->eeh_configure = spapr_phb_vfio_eeh_configure;
> >> + spc->eeh_inject_error = spapr_phb_vfio_eeh_inject_error;
> >> }
> >>
> >> static const TypeInfo spapr_phb_vfio_info = {
> >> 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);
> >> }
> >>
> >> +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 != 3) || (nret != 1)) {
> >
> >Less paranthesis, please?
> >
>
> Sure.
>
> >> + ret = RTAS_OUT_PARAM_ERROR;
> >> + goto out;
> >> + }
> >> +
> >> + /* Check if we have opened token */
> >> + open_token = rtas_ld(args, 1);
> >> + if (!spapr->is_errinjct_opened ||
> >> + spapr->errinjct_token != open_token) {
> >> + ret = RTAS_OUT_CLOSE_ERROR;
> >> + goto out;
> >> + }
> >> +
> >> + /* The parameter buffer should be 1KB aligned */
> >> + param_buf = rtas_ld(args, 2);
> >> + if (param_buf & 0x3ff) {
> >> + ret = RTAS_OUT_PARAM_ERROR;
> >> + goto out;
> >> + }
> >> +
> >> + /* Check the error type */
> >> + type = rtas_ld(args, 0);
> >> + switch (type) {
> >> + case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR:
> >> + ret = spapr_rtas_errinjct_ioa(spapr, param_buf, false);
> >> + break;
> >> + case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64:
> >> + ret = spapr_rtas_errinjct_ioa(spapr, param_buf, true);
> >> + break;
> >> + default:
> >> + ret = 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, hwaddr
> >> rtas_addr,
> >> int i;
> >> uint32_t lrdr_capacity[5];
> >> MachineState *machine = MACHINE(qdev_get_machine());
> >> + char errinjct_tokens[1024];
> >> + int fdt_offset, offset;
> >> + const int tokens[] = {
> >> + RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR,
> >> + RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64
> >> + };
> >> + const char *token_strings[] = {
> >> + "ioa-bus-error",
> >> + "ioa-bus-error-64"
> >> + };
> >> +
> >> + /* ibm,errinjct-tokens */
> >> + offset = 0;
> >> + for (i = 0; i < ARRAY_SIZE(tokens); i++) {
> >> + offset += sprintf(errinjct_tokens + offset, "%s",
> >> token_strings[i]);
> >> + errinjct_tokens[offset++] = '\0';
> >> + *(int *)(&errinjct_tokens[offset]) = 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? ==> Use stl_be_p instead?
> >
>
> Good question! Currently, the property (/rtas/ibm,errinjct-tokens) is used by
> userland utility "errinjct" like below. So I think qemu needs pass BE tokens
> and the utility also needs do conversion if necessary.
>
> ei_funcs[i].rtas_token = *(int *)tmp_ptr; /* tmp_ptr is pointing to data
> tream
> * read from
> /rtas/ibm,errinjct-tokens */
Yes, values you put into the device tree should definitely be BE.
> >> + offset += sizeof(int);
> >> + }
> >> +
> >> + fdt_offset = fdt_path_offset(fdt, "/rtas");
> >> + ret = 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;
> >> + }
>
> 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
pgpSjqfoVdCoM.pgp
Description: PGP signature
