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

[Thomas Huth]

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.

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

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

?

[...]
>>> @@ -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.

 Thomas