[Qemu-devel] Re: [PATCH v7 3/6] pcie/aer: glue aer error injection into qemu monitor

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Nov 02, 2010 at 06:32:49PM +0900, Isaku Yamahata wrote:
> introduce pcie_aer_inject_error command.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> Changes v6 -> v7:
> - check return value.
> 
> Changes v3 -> v4:
> - s/PCIE_AER/PCIEAER/g for structure names.
> - compilation adjustment.
> 
> Changes v2 -> v3:
> - compilation adjustment.
> ---
>  hmp-commands.hx |   23 ++++++++++++++
>  hw/pcie_aer.c   |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sysemu.h        |    5 +++
>  3 files changed, 115 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 81999aa..02eae7d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -835,6 +835,29 @@ Hot remove PCI device.
>  ETEXI
>  
>      {
> +        .name       = "pcie_aer_inject_error",
> +        .args_type  = "advisory_non_fatal:-a,is_correctable:-c,"

is_correctable->correctable?

> +	              "pci_addr:s,error_status:i,"
> +	              "tlph0:i?,tlph1:i?,tlph2:i?,tlph3:i?,"
> +	              "hpfx0:i?,hpfx1:i?,hpfx2:i?,hpfx3:i?",

Do the field names have to be so cryptic?

> +        .params     = "[-a] [-c] [[<domain>:]<bus>:]<slot>.<func> "
> +	              "<error status:32bit> "

Only 1 bit is valid though, right?
So I would say let's pass symbolic name, not raw bit.

> +	              "[<tlp header:(32bit x 4)>] "
> +	              "[<tlp header prefix:(32bit x 4)>]",

Maybe -advisory -correctable?

> +        .help       = "inject pcie aer error "
> +	               "(use -a for advisory non fatal error) "
> +	               "(use -c for correctrable error)",
> +        .user_print  = pcie_aer_inject_error_print,
> +        .mhandler.cmd_new = do_pcie_aer_inejct_error,
> +    },
> +
> +STEXI
> +@item pcie_aer_inject_error
> +@findex pcie_aer_inject_error
> +Inject PCIe AER error
> +ETEXI
> +
> +    {
>          .name       = "host_net_add",
>          .args_type  = "device:s,opts:s?",
>          .params     = "tap|user|socket|vde|dump [options]",
> diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
> index 84c3457..30e39fd 100644
> --- a/hw/pcie_aer.c
> +++ b/hw/pcie_aer.c
> @@ -19,6 +19,8 @@
>   */
>  
>  #include "sysemu.h"
> +#include "qemu-objects.h"
> +#include "monitor.h"
>  #include "pci_bridge.h"
>  #include "pcie.h"
>  #include "msix.h"
> @@ -823,3 +825,88 @@ const VMStateDescription vmstate_pcie_aer_log = {
>      }
>  };
>  
> +void pcie_aer_inject_error_print(Monitor *mon, const QObject *data)
> +{
> +    QDict *qdict;
> +    int devfn;
> +    assert(qobject_type(data) == QTYPE_QDICT);
> +    qdict = qobject_to_qdict(data);
> +
> +    devfn = (int)qdict_get_int(qdict, "devfn");
> +    monitor_printf(mon, "OK domain: %x, bus: %x devfn: %x.%x\n",
> +                   (int) qdict_get_int(qdict, "domain"),
> +                   (int) qdict_get_int(qdict, "bus"),
> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
> +}
> +
> +int do_pcie_aer_inejct_error(Monitor *mon,
> +                             const QDict *qdict, QObject **ret_data)
> +{
> +    const char *pci_addr = qdict_get_str(qdict, "pci_addr");
> +    int dom;
> +    int bus;
> +    unsigned int slot;
> +    unsigned int func;
> +    PCIDevice *dev;
> +    PCIEAERErr err;
> +    int ret;
> +
> +    /* Ideally qdev device path should be used.
> +     * However at the moment there is no reliable way to determine
> +     * wheher a given qdev is pci device or not.
> +     * so pci_addr is used.
> +     */

This does not look like such a big deal, and tying interface
to such implementation detail seems wrong to me.

> +    if (pci_parse_devaddr(pci_addr, &dom, &bus, &slot, &func)) {
> +        monitor_printf(mon, "invalid pci address %s\n", pci_addr);
> +        return -1;
> +    }
> +    dev = pci_find_device(pci_find_root_bus(dom), bus, slot, func);
> +    if (!dev) {
> +        monitor_printf(mon, "device is not found. 0x%x:0x%x.0x%x\n",
> +                       bus, slot, func);
> +        return -1;
> +    }
> +    if (!pci_is_express(dev)) {
> +        monitor_printf(mon, "the device doesn't support pci express. "
> +                       "0x%x:0x%x.0x%x\n",
> +                       bus, slot, func);
> +        return -1;
> +    }
> +
> +    err.status = qdict_get_int(qdict, "error_status");
> +    err.source_id = (pci_bus_num(dev->bus) << 8) | dev->devfn;
> +
> +    err.flags = 0;
> +    if (qdict_get_int(qdict, "is_correctable")) {
> +        err.flags |= PCIE_AER_ERR_IS_CORRECTABLE;
> +    }
> +    if (qdict_get_int(qdict, "advisory_non_fatal")) {
> +        err.flags |= PCIE_AER_ERR_MAYBE_ADVISORY;
> +    }
> +    if (qdict_haskey(qdict, "tlph0")) {
> +        err.flags |= PCIE_AER_ERR_HEADER_VALID;
> +    }
> +    if (qdict_haskey(qdict, "hpfx0")) {
> +        err.flags |= PCIE_AER_ERR_TLP_PRESENT;
> +    }

So tlph0 is for header and hpfx0 is for TLP_PRESENT?
Not the other way around?

> +
> +    err.header[0] = qdict_get_try_int(qdict, "tlph0", 0);
> +    err.header[1] = qdict_get_try_int(qdict, "tlph1", 0);
> +    err.header[2] = qdict_get_try_int(qdict, "tlph2", 0);
> +    err.header[3] = qdict_get_try_int(qdict, "tlph3", 0);
> +
> +    err.prefix[0] = qdict_get_try_int(qdict, "hpfx0", 0);
> +    err.prefix[1] = qdict_get_try_int(qdict, "hpfx1", 0);
> +    err.prefix[2] = qdict_get_try_int(qdict, "hpfx2", 0);
> +    err.prefix[3] = qdict_get_try_int(qdict, "hpfx3", 0);
> +
> +    ret = pcie_aer_inject_error(dev, &err);
> +    *ret_data = qobject_from_jsonf("{ 'domain': %d, 'bus': %d, 'devfn': %d "
> +                                   "'ret': %d}",
> +                                   pci_find_domain(dev->bus),
> +                                   pci_bus_num(dev->bus), dev->devfn,
> +                                   ret);
> +    assert(*ret_data);
> +
> +    return 0;
> +}
> diff --git a/sysemu.h b/sysemu.h
> index b81a70e..99c7909 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -151,6 +151,11 @@ void pci_device_hot_add(Monitor *mon, const QDict *qdict);
>  void drive_hot_add(Monitor *mon, const QDict *qdict);
>  void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
>  
> +/* pcie aer error injection */
> +void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
> +int do_pcie_aer_inejct_error(Monitor *mon,
> +                             const QDict *qdict, QObject **ret_data);
> +
>  /* serial ports */
>  
>  #define MAX_SERIAL_PORTS 4
> -- 
> 1.7.1.1