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

[Gavin Shan]

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 <address@hidden>
>>>> ---
>>>>  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