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