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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

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 <gwshan@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.

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

>>  #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 */

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