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