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

[Thomas Huth]

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.

> +        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"

>  #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?

> +        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?

> +        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?

> +        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;
> +    }

 Thomas