Re: [Qemu-devel] [PATCH v1 1/3] sPAPR: Implement PCI error injection RTAS calls

[Alexander Graf <agraf@suse.de>]

On 23.06.14 04:22, Gavin Shan wrote:
> The patch implements PCI error injection RTAS calls for sPAPR
> platform, which are defined PAPR spec as follows. Those RTAS
> calls are expected to be invoked in strict sequence of
> "ibm,open-errinjct", "ibm,errinjct", "ibm,close-errinjct".
>
> - "ibm,open-errinjct": Apply for token to do error injection
> - "ibm,close-errinjct": Free the token assigned previously
> - "ibm,errinjct": Do error injection
>
> The patch defines constants used by error injection RTAS calls
> and adds callback sPAPRPHBClass::format_errinjct_cmd, which is
> going to be used like this way:
>
> - Error injection RTAS calls are received in spapr_rtas.c. If that's
>    "ibm,open-errinjct" or "ibm,close-errinjct", we handle it there
>    with the help of static counter "sPAPREnvironment::errinjct_token_cnt".
>    If the  RTAS call is "ibm,errinjct", sanity check is done there and
>    just returns failure if the error type isn't PCI related. Otherwise,
>    the input parameters are figured out and route the request to
>    sPAPRPHBClass::format_errinjct_cmd.
> - sPAPRPHBClass::format_errinjct_cmd translates the address (BUID, PE
>    number) to IOMMU group ID and then return the formated string back to
>    spapr_rtas.c where the string is writen to firmware sysfs file
>    "/sys/firmware/opal/errinjct" to do error injection.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>   hw/ppc/spapr_rtas.c         | 198 ++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/pci-host/spapr.h |  11 +++
>   include/hw/ppc/spapr.h      |  35 ++++++++
>   3 files changed, 244 insertions(+)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index a7233e4..94fb866 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -32,6 +32,7 @@
>   
>   #include "hw/ppc/spapr.h"
>   #include "hw/ppc/spapr_vio.h"
> +#include "hw/pci-host/spapr.h"
>   
>   #include <libfdt.h>
>   
> @@ -268,6 +269,196 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>       rtas_st(rets, 0, ret);
>   }
>   
> +static sPAPRErrInjct *rtas_find_errinjct_token(sPAPREnvironment *spapr,
> +                                               uint32_t token_num)
> +{
> +    sPAPRErrInjct *open_token;
> +
> +    QLIST_FOREACH(open_token, &spapr->errinjct_open_tokens, list) {
> +        if (open_token->token == token_num) {
> +            return open_token;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
> +                                   sPAPREnvironment *spapr,
> +                                   uint32_t token, uint32_t nargs,
> +                                   target_ulong args, uint32_t nret,
> +                                   target_ulong rets)
> +{
> +    sPAPRErrInjct *open_token;
> +    int32_t ret = RTAS_OUT_HW_ERROR;
> +
> +    /* Sanity check on number of arguments */
> +    if ((nargs != 0) || (nret != 2)) {
> +        goto out;
> +    }
> +
> +    /* Allocate token */
> +    open_token = g_malloc(sizeof(*open_token));

With this hypercall the guest can OOM us by simply allocating useless 
tokens.

> +    if (!open_token) {
> +        goto out;
> +    }
> +    open_token->token = spapr->errinjct_open_token_cnt++;
> +    QLIST_INSERT_HEAD(&spapr->errinjct_open_tokens, open_token, list);
> +
> +    /* Success */
> +    rtas_st(rets, 0, open_token->token);
> +    ret = RTAS_OUT_SUCCESS;
> +out:
> +    rtas_st(rets, 1, ret);
> +}
> +
> +static char *rtas_do_errinjct_ioa(sPAPREnvironment *spapr,
> +                                sPAPRErrInjctIOA *ioa)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *info;
> +
> +    /* Sanity check on argument */
> +    if (!spapr || !ioa) {
> +        return NULL;
> +    }
> +
> +    /* Invalid option ? */
> +    if (ioa->func > RTAS_ERRINJCT_IOA_DMA_WR_MEM_TARGET) {
> +        return NULL;
> +    }
> +
> +    /* Find PHB */
> +    sphb = spapr_find_phb(spapr, ioa->buid);
> +    if (!sphb) {
> +        return NULL;
> +    }
> +
> +    /* PHB doesn't support error injection ? */
> +    info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!info->format_errinjct_cmd) {
> +        return NULL;
> +    }
> +
> +    return info->format_errinjct_cmd(sphb, ioa);
> +}
> +
> +static void rtas_ibm_errinjct(PowerPCCPU *cpu,
> +                              sPAPREnvironment *spapr,
> +                              uint32_t token, uint32_t nargs,
> +                              target_ulong args, uint32_t nret,
> +                              target_ulong rets)
> +{
> +    sPAPRErrInjctIOA ioa;
> +    sPAPRErrInjct *open_token;
> +    uint32_t token_num, ei_token;
> +    target_ulong param_buf;
> +    char *pbuf;
> +    int fd;
> +    int32_t ret = RTAS_OUT_PARAM_ERROR;
> +
> +    /* Sanity check on number of arguments */
> +    if ((nargs != 3) || (nret != 1)) {
> +        goto out;
> +    }
> +
> +    /* Check if we have opened token */
> +    token_num = rtas_ld(args, 1);
> +    open_token = rtas_find_errinjct_token(spapr, token_num);
> +    if (!open_token) {
> +        goto out;
> +    }
> +
> +    /* Argument buffer should be aligned with 1KB */
> +    param_buf = rtas_ld(args, 2);
> +    if (param_buf & 0x3ff) {
> +        goto out;
> +    }
> +
> +    /* We only support IOA error for now */
> +    ei_token = rtas_ld(args, 0);
> +    switch (ei_token) {
> +    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR:
> +        ioa.ei_token = RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR;
> +        ioa.addr = rtas_ld(param_buf, 0);
> +        ioa.mask = rtas_ld(param_buf, 1);
> +        ioa.cfg_addr = rtas_ld(param_buf, 2);
> +        ioa.buid = ((uint64_t)rtas_ld(param_buf, 3) << 32) | rtas_ld(param_buf, 4);
> +        ioa.func = rtas_ld(param_buf, 5);
> +
> +        break;
> +    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64:
> +        ioa.ei_token = RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64;
> +        ioa.addr = ((uint64_t)rtas_ld(param_buf, 0) << 32) | rtas_ld(param_buf, 1);
> +        ioa.mask = ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(param_buf, 3);
> +        ioa.cfg_addr = rtas_ld(param_buf, 4);
> +        ioa.buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(param_buf, 6);
> +        ioa.func = rtas_ld(param_buf, 7);
> +
> +        break;
> +    default:
> +        goto out;
> +    }
> +
> +    /* We have nothing written to sysfs */
> +    pbuf = rtas_do_errinjct_ioa(spapr, &ioa);
> +    if (!pbuf) {
> +        goto out;
> +    }
> +
> +    /* Open firmware sysfs file */
> +    fd = qemu_open("/sys/firmware/opal/errinjct", O_WRONLY);

Device emulation code shouldn't even remotely have an idea what host 
it's running on. Also semantically there are a few issues with this approach

   1) QEMU is usually running with user privileges, so it doesn't have 
access to the file above
   2) QEMU's channel to hardware devices is via normal kernel API. For 
physical devices that's VFIO. No side channels please.
   3) Ownership of the question whether a PE is in error mode is 
responsibility of the PHB. In the emulated case, the PHB would have to 
set itself into a mode where it behaves as if it's blocked.


Alex