On Wed, Aug 19, 2015 at 08:48:20AM -0700, Thomas Huth wrote:
>On 18/08/15 17:26, Gavin Shan wrote:
>> 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.
>
>There are also other spots in the code that load a 64-bit value that
>way, so they could be reworked, too...
>Anyway, if you and David don't like this idea, simply never mind, it's
>not that important.
>
Ok. I'll pick rtas_ldq() that was dropped in v2 in separate patch and try
to replace rtas_ld() with the new function.
>>>> + 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.
>
>Simply try to compile qemu-softmmu-pp64 on your x86 laptop (with TCG)!
>The spapr_pci_vfio.c file is also compiled there, and if you use
>"<asm/eeh.h>", you break the build!
>
Yes, Thanks for the details!
>>>> #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!
>
>Ok, so if you want to test for all the defined cases from eeh.h,
>wouldn't it be easier to simply check
>
> if (func > EEH_ERR_FUNC_MAX) {
> ret = RTAS_OUT_PARAM_ERROR;
> goto out;
> }
>
>?
>
Yes, absolutely.
>[...]
>>>> @@ -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.
>
>Right, otherwise cross-endian setup won't work.
>
Thanks,
Gavin
