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 <gws...@linux.vnet.ibm.com>
>> ---
>> 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
