Re: [Qemu-devel] [PATCH RESEND v2 3/3] sPAPR: Support RTAS call ibm, errinjct

[Gavin Shan]

On Mon, Aug 03, 2015 at 01:01:50PM +1000, David Gibson wrote:
>On Mon, Aug 03, 2015 at 09:23:20AM +1000, 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          | 42 ++++++++++++++++++++++
>>  hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++
>>  hw/ppc/spapr_rtas.c         | 85 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/pci-host/spapr.h |  2 ++
>>  include/hw/ppc/spapr.h      | 28 ++++++++++++++-
>>  5 files changed, 212 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index cfd3b7b..fb03c3a 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -682,6 +682,48 @@ 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;
>> +    int ret;
>> +
>> +    if (is_64bits) {
>> +        addr = rtas_ldq(param_buf, 0);
>> +        mask = rtas_ldq(param_buf, 1);
>> +        buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(param_buf, 
>> 6);
>> +        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 */
>> +    ret = spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
>> +    if (ret < 0) {
>> +        return RTAS_OUT_HW_ERROR;
>
>Your eeh_inject_error callback below returns rtas error codes, which
>here you ignore and always return RTAS_OUT_HW_ERROR.
>

Yeah, I'll fix in next revision.

>> +    }
>> +
>> +    return RTAS_OUT_SUCCESS;
>> +}
>> +
>>  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>
>> +
>>  #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:
>> +        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 0a9c904..d6894ee 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -637,6 +637,53 @@ 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)) {
>> +        ret = RTAS_OUT_PARAM_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    /* Check if we have opened token */
>> +    open_token = rtas_ld(args, 1);
>> +    if (spapr->errinjct_token != open_token) {
>> +        ret = RTAS_OUT_TOKEN_OPENED;
>
>In the open function this error code indicates that the token is
>already opened, here it could indicate that it's not opened (or that
>you have the wrong one).
>

Yes, the correct errcode here should be 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,
>> @@ -728,6 +775,42 @@ 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 char *tokens[] = {
>> +        "fatal",
>> +        "recovered-random-event",
>> +        "recovered-special-event",
>> +        "corrupted-page",
>> +        "corrupted-slb",
>> +        "translator-failure",
>> +        "ioa-bus-error",
>> +        "ioa-bus-error-64",
>> +        "platform-specific",
>> +        "corrupted-dcache-start",
>> +        "corrupted-dcache-end",
>> +        "corrupted-icache-start",
>> +        "corrupted-icache-end",
>> +        "corrupted-tlb-start",
>> +        "corrupted-tlb-end"
>
>You list all these tokens despite the fact you only support two of them.
>
>This also silently depends on the fact that this list appears int the
>same order as your #defined token values, which is a non-obvious
>dependency I'd prefer to avoid.
>

Ok. I'll introduce two arrays as below. Then I don't need expose
those options that are not supported yet and make the dependencies
obvious:

    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", tokens[i]);
>> +        errinjct_tokens[offset++] = '\0';
>> +        *(int *)(&errinjct_tokens[offset]) = i+1;
>> +        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;
>> +    }
>>  
>>      ret = fdt_add_mem_rsv(fdt, rtas_addr, rtas_size);
>>      if (ret < 0) {
>> @@ -823,6 +906,8 @@ static void core_rtas_register_types(void)
>>                          rtas_ibm_configure_connector);
>>      spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct",
>>                          rtas_ibm_open_errinjct);
>> +    spapr_rtas_register(RTAS_IBM_ERRINJCT, "ibm,errinjct",
>> +                        rtas_ibm_errinjct);
>>      spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct",
>>                          rtas_ibm_close_errinjct);
>>  }
>> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
>> index 5322b56..069300d 100644
>> --- a/include/hw/pci-host/spapr.h
>> +++ b/include/hw/pci-host/spapr.h
>> @@ -53,6 +53,8 @@ struct sPAPRPHBClass {
>>      int (*eeh_get_state)(sPAPRPHBState *sphb, int *state);
>>      int (*eeh_reset)(sPAPRPHBState *sphb, int option);
>>      int (*eeh_configure)(sPAPRPHBState *sphb);
>> +    int (*eeh_inject_error)(sPAPRPHBState *sphb, uint32_t func,
>> +                            uint64_t addr, uint64_t mask, bool is_64bits);
>>  };
>>  
>>  typedef struct spapr_pci_msi {
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 30d9854..e3135e3 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -408,6 +408,24 @@ int spapr_allocate_irq_block(int num, bool lsi, bool 
>> msi);
>>  #define RTAS_SLOT_TEMP_ERR_LOG           1
>>  #define RTAS_SLOT_PERM_ERR_LOG           2
>>  
>> +/* ibm,errinjct */
>> +#define RTAS_ERRINJCT_TYPE_FATAL                1
>> +#define RTAS_ERRINJCT_TYPE_RANDOM_EVENT         2
>> +#define RTAS_ERRINJCT_TYPE_SPECIAL_EVENT        3
>> +#define RTAS_ERRINJCT_TYPE_CORRUPTED_PAGE       4
>> +#define RTAS_ERRINJCT_TYPE_CORRUPTED_SLB        5
>> +#define RTAS_ERRINJCT_TYPE_TRANSLATOR_FAILURE   6
>> +#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR        7
>> +#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64      8
>> +#define RTAS_ERRINJCT_TYPE_PLATFORM_SPECIFIC    9
>> +#define RTAS_ERRINJCT_TYPE_DCACHE_START         10
>> +#define RTAS_ERRINJCT_TYPE_DCACHE_END           11
>> +#define RTAS_ERRINJCT_TYPE_ICACHE_START         12
>> +#define RTAS_ERRINJCT_TYPE_ICACHE_END           13
>> +#define RTAS_ERRINJCT_TYPE_TLB_START            14
>> +#define RTAS_ERRINJCT_TYPE_TLB_END              15
>> +#define RTAS_ERRINJCT_TYPE_UPSTREAM_IO_ERROR    16
>> +

I'll drop those options that aren't supported by this patchset.

>>  /* RTAS return codes */
>>  #define RTAS_OUT_SUCCESS            0
>>  #define RTAS_OUT_NO_ERRORS_FOUND    1
>> @@ -462,8 +480,9 @@ int spapr_allocate_irq_block(int num, bool lsi, bool 
>> msi);
>>  #define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
>>  #define RTAS_IBM_OPEN_ERRINJCT                  (RTAS_TOKEN_BASE + 0x26)
>>  #define RTAS_IBM_CLOSE_ERRINJCT                 (RTAS_TOKEN_BASE + 0x27)
>> +#define RTAS_IBM_ERRINJCT                       (RTAS_TOKEN_BASE + 0x28)
>>  
>> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x28)
>> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x29)
>>  
>>  /* RTAS ibm,get-system-parameter token values */
>>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>> @@ -499,6 +518,11 @@ static inline uint32_t rtas_ld(target_ulong phys, int n)
>>      return ldl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 
>> 4*n));
>>  }
>>  
>> +static inline uint64_t rtas_ldq(target_ulong phys, int n)
>> +{
>> +    return ldq_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 
>> 8*n));
>> +}
>> +
>
>I don't much like this.  The rtas_ld() function is really designed to
>be used for direct rtas parameters, nothing else.  This is only used
>for another buffer.  Further, the index used here is different from
>rtas_ld() because it's counted in 8-byte rather than 4-byte
>increments, which is potentially confusing.
>
>It might make more sense to do the conversion from a guest physical
>addr to a qemu memory pointer in the top level rtas function, and just
>pass the pointer through to the other layers.
>

Ok. I'll drop rtas_ldq() and use rtas_ld() instead. Note rtas_ldq() was
used to grab the 64-bits address and mask, which are part of the PCI
error injection paramters in spapr_rtas_errinjct_ioa(). After replacing
rtas_ldq() with rtas_ld(), the code will be something like below:

    uint64_t addr, mask;

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

>>  static inline void rtas_st(target_ulong phys, int n, uint32_t val)
>>  {
>>      stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
>> @@ -595,6 +619,8 @@ int spapr_dma_dt(void *fdt, int node_off, const char 
>> *propname,
>>  int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
>>                        sPAPRTCETable *tcet);
>>  void spapr_pci_switch_vga(bool big_endian);
>> +int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr,
>> +                            target_ulong param_buf, bool is_64bits);
>>  void spapr_hotplug_req_add_event(sPAPRDRConnector *drc);
>>  void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc);
>>  

Thanks,
Gavin