Re: [Qemu-devel] [PATCH v5 4/4] sPAPR: Support RTAS call ibm, errinjct

[David Gibson]

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 <address@hidden>
> >> ---
> >>  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

pgpQl7_VfbfZH.pgp
Description: PGP signature