[Qemu-devel] [PATCH 0/3] pcie/aer: glue inject aer error into hmp

[Qemu-devel] [PATCH 0/3] pcie/aer: glue inject aer error into hmp

[Isaku Yamahata]

This patch series introduces hmp command to inject aer error.
Now fw device path is used to specify pci function.

Isaku Yamahata (3):
  build, pci: remove QMP dependency on core PCI code
  pci: introduce a parser for fw device path to pci device
  pcie/aer: glue aer error injection into qemu monitor

 Makefile.objs   |    4 +-
 Makefile.target |    2 +
 hmp-commands.hx |   28 +++++++
 hw/pci-stub.c   |   50 ++++++++++++
 hw/pci.c        |  128 ++++++++++++++++++++++++++++++++
 hw/pci.h        |    2 +
 hw/pcie_aer.c   |  222 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sysemu.h        |    5 +
 8 files changed, 438 insertions(+), 3 deletions(-)
 create mode 100644 hw/pci-stub.c

[Qemu-devel] [PATCH 1/3] build, pci: remove QMP dependency on core PCI code

[Isaku Yamahata]

by introducing pci-stub.c, eliminate QMP dependency on core PCI code
rquired by query-pci command.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 Makefile.objs   |    4 +---
 Makefile.target |    2 ++
 hw/pci-stub.c   |   37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 3 deletions(-)
 create mode 100644 hw/pci-stub.c

diff --git a/Makefile.objs b/Makefile.objs
index d6b3d60..c3e52c5 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -169,9 +169,7 @@ hw-obj-y =
 hw-obj-y += vl.o loader.o
 hw-obj-$(CONFIG_VIRTIO) += virtio.o virtio-console.o
 hw-obj-y += fw_cfg.o
-# FIXME: Core PCI code and its direct dependencies are required by the
-# QMP query-pci command.
-hw-obj-y += pci.o pci_bridge.o
+hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o
 hw-obj-$(CONFIG_PCI) += msix.o msi.o
 hw-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
 hw-obj-$(CONFIG_PCI) += ioh3420.o xio3130_upstream.o xio3130_downstream.o
diff --git a/Makefile.target b/Makefile.target
index d08f5dd..38582d4 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -1,6 +1,7 @@
 # -*- Mode: makefile -*-
 
 GENERATED_HEADERS = config-target.h
+CONFIG_NO_PCI = $(if $(subst n,,$(CONFIG_PCI)),n,y)
 CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)
 
 include ../config-host.mak
@@ -188,6 +189,7 @@ ifdef CONFIG_SOFTMMU
 obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
+obj-$(CONFIG_NO_PCI) += pci-stub.o
 obj-$(CONFIG_VIRTIO) += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o
 obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
 obj-y += vhost_net.o
diff --git a/hw/pci-stub.c b/hw/pci-stub.c
new file mode 100644
index 0000000..674591d
--- /dev/null
+++ b/hw/pci-stub.c
@@ -0,0 +1,37 @@
+/*
+ * PCI stubs for plathome that doesn't support pci bus.
+ *
+ * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "monitor.h"
+#include "pci.h"
+
+static void pci_error_message(Monitor *mon)
+{
+    monitor_printf(mon, "PCI devices not supported\n");
+}
+
+void do_pci_info(Monitor *mon, QObject **ret_data)
+{
+    pci_error_message(mon);
+}
+
+void do_pci_info_print(Monitor *mon, const QObject *data)
+{
+    pci_error_message(mon);
+}
-- 
1.7.1.1

[Qemu-devel] [PATCH 2/3] pci: introduce a parser for fw device path to pci device

[Isaku Yamahata]

Introduce a function to parse fw device path to pci device.
the format is
/pci@{<ioport>, <mmio>}/[<fw_name>]@<slot>,<func>/.../[<fw_name>]@<slot>,<func>

<ioport> = "i"<ioport addr in hex>
<mmio> = <mmio addr in hex>
<slot> = slot number in hex
<func> = func number in hex

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci.h |    2 +
 2 files changed, 130 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index eb21848..a52a323 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -2027,3 +2027,131 @@ static char *pcibus_get_dev_path(DeviceState *dev)
     return strdup(path);
 }
 
+/*
+ * Parse format and get PCIDevice
+ * return 0 on success
+ *       <0 on error: format is invalid or device isn't found.
+ *
+ * Format:
+ * /pci@{<ioport>, <mmio>}/[<fw_name>]@<slot>,<func>/...
+ *                     .../[<fw_name>]@<slot>,<func>
+ *
+ * <ioport> = "i"<ioport addr in hex>
+ * <mmio> = <mmio addr in hex>
+ * <slot> = slot number in hex
+ * <func> = func number in hex
+ *
+ */
+int pci_parse_fw_dev_path(const char *path, PCIDevice **pdev)
+{
+    const char *p = path;
+    char *e;
+    size_t len;
+    PCIBus *bus;
+    struct PCIHostBus *host;
+
+    if (*p != '/') {
+        return -EINVAL;
+    }
+    e = strchr(p + 1, '/');
+    if (e == NULL) {
+        return -EINVAL;
+    }
+    len = e - p;
+    p = e + 1;
+
+    bus = NULL;
+    QLIST_FOREACH(host, &host_buses, next) {
+        DeviceState *qdev = host->bus->qbus.parent;
+        if (qdev) {
+            char *devpath = qdev_get_fw_dev_path(qdev);
+
+            if (len == strlen(devpath) && !strncmp(devpath, path, len)) {
+                bus = host->bus;
+                qemu_free(devpath);
+                break;
+            }
+            qemu_free(devpath);
+        } else {
+            /* This pci bus doesn't have host-to-pci bridge device.
+             * Check only if the path is pci ignoring other parameters. */
+#define PCI_FW_PATH     "/pci@"
+            if (strncmp(path, PCI_FW_PATH, strlen(PCI_FW_PATH))) {
+                return -EINVAL;
+            }
+            bus = host->bus;
+            break;
+        }
+    }
+
+    for (;;) {
+        char *at;
+        char *comma;
+        unsigned long slot;
+        unsigned long func;
+        PCIDevice *dev;
+        PCIBus *child_bus;
+
+        if (!bus) {
+            return -ENODEV;
+        }
+        if (*p == '\0') {
+            return -EINVAL;
+        }
+
+        at = strchr(p, '@');
+        if (at == NULL) {
+            return -EINVAL;
+        }
+        slot = strtoul(at + 1, &e, 16);
+        if (e == at + 1 || *e != ',') {
+            return -EINVAL;
+        }
+        if (slot >= PCI_SLOT_MAX) {
+            return -EINVAL;
+        }
+
+        comma = e;
+        func = strtoul(comma + 1, &e, 16);
+        if (e == comma + 1 || (*e != '/' && *e != '\0')) {
+            return -EINVAL;
+        }
+        if (func >= PCI_FUNC_MAX) {
+            return -EINVAL;
+        }
+
+        len = e - p;
+        dev = bus->devices[PCI_DEVFN(slot, func)];
+        if (!dev) {
+            return -ENODEV;
+        }
+        if (at != p) {
+            /* fw_name is specified. */
+            char *fw_dev_path = pcibus_get_fw_dev_path(&dev->qdev);
+            if (strncmp(p, fw_dev_path, len)) {
+                qemu_free(fw_dev_path);
+                return -EINVAL;
+            }
+            qemu_free(fw_dev_path);
+        }
+
+        if (*e == '\0') {
+            *pdev = dev;
+            return 0;
+        }
+
+        /*
+         * descending down pci-to-pci bridge.
+         * At the moment, there is no way to safely determine if the given
+         * pci device is really pci-to-pci device.
+         */
+        p = e;
+        QLIST_FOREACH(child_bus, &bus->child, sibling) {
+            if (child_bus->parent_dev == dev) {
+                bus = child_bus;
+                continue;
+            }
+        }
+        bus = NULL;
+    }
+}
diff --git a/hw/pci.h b/hw/pci.h
index 6e80b08..96f8d52 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -16,6 +16,7 @@
 #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
 #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
 #define PCI_FUNC(devfn)         ((devfn) & 0x07)
+#define PCI_SLOT_MAX            32
 #define PCI_FUNC_MAX            8
 
 /* Class, Vendor and Device IDs from Linux's pci_ids.h */
@@ -258,6 +259,7 @@ int pci_parse_devaddr(const char *addr, int *domp, int *busp,
                       unsigned int *slotp, unsigned int *funcp);
 int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
                      unsigned *slotp);
+int pci_parse_fw_dev_path(const char *path, PCIDevice **pdev);
 
 void do_pci_info_print(Monitor *mon, const QObject *data);
 void do_pci_info(Monitor *mon, QObject **ret_data);
-- 
1.7.1.1

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

[Isaku Yamahata]

introduce pcie_aer_inject_error command.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
Changes v9 -> v10:
- use fw device path
- error path
- pci-stub.c for CONFIG_PCI=n

Changes v8 -> v9:
- revise error code

Changes v7 -> v8:
- use domain:slot.func:slot.func...:slot.func instead of domain:bus:slot.func
- allow symbolic aer error name in addition to 32bit value

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 |   28 +++++++
 hw/pci-stub.c   |   13 +++
 hw/pcie_aer.c   |  222 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sysemu.h        |    5 +
 4 files changed, 268 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index dd3db36..a5dec9e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -873,6 +873,34 @@ Hot remove PCI device.
 ETEXI
 
     {
+        .name       = "pcie_aer_inject_error",
+        .args_type  = "advisory_non_fatal:-a,correctable:-c,"
+	              "pci_fw_dev_path:s,error_status:s,"
+	              "header0:i?,header1:i?,header2:i?,header3:i?,"
+	              "prefix0:i?,prefix1:i?,prefix2:i?,prefix3:i?",
+        .params     = "[-a] [-c] <pci_fw_dev_path> "
+                      "<error_status> [<tlp header> [<tlp header prefix>]]",
+        .help       = "inject pcie aer error\n\t\t\t"
+	              " -a for advisory non fatal error\n\t\t\t"
+	              " -c for correctable error\n\t\t\t"
+                      "<pci_fw_dev_path> = fw device path to pci device"
+	              "\n\t\t\t/pci@<ioport or mmio(hex)>/"
+                      "[<fw_name>]@<slot(hex)>,<func(hex)>/.../"
+                      "[<fw_name>]@<slot(hex)>,<func(hex)>\n\t\t\t"
+                      "<error_status> = error string or 32bit\n\t\t\t"
+                      "<tlb header> = 32bit x 4\n\t\t\t"
+                      "<tlb header prefix> = 32bit x 4",
+        .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/pci-stub.c b/hw/pci-stub.c
index 674591d..c5a0aa8 100644
--- a/hw/pci-stub.c
+++ b/hw/pci-stub.c
@@ -18,6 +18,7 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "sysemu.h"
 #include "monitor.h"
 #include "pci.h"
 
@@ -35,3 +36,15 @@ void do_pci_info_print(Monitor *mon, const QObject *data)
 {
     pci_error_message(mon);
 }
+
+int do_pcie_aer_inejct_error(Monitor *mon,
+                             const QDict *qdict, QObject **ret_data)
+{
+    pci_error_message(mon);
+    return -ENOSYS;
+}
+
+void pcie_aer_inject_error_print(Monitor *mon, const QObject *data)
+{
+    pci_error_message(mon);
+}
diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
index cb97a95..091680e 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"
@@ -806,3 +808,223 @@ const VMStateDescription vmstate_pcie_aer_log = {
         VMSTATE_END_OF_LIST()
     }
 };
+
+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));
+}
+
+typedef struct PCIEAERErrorName {
+    const char *name;
+    uint32_t val;
+    bool correctable;
+} PCIEAERErrorName;
+
+/*
+ * AER error name -> value convertion table
+ * This naming scheme is same to linux aer-injection tool.
+ */
+static const struct PCIEAERErrorName pcie_aer_error_list[] = {
+    {
+        .name = "TRAIN",
+        .val = PCI_ERR_UNC_TRAIN,
+        .correctable = false,
+    }, {
+        .name = "DLP",
+        .val = PCI_ERR_UNC_DLP,
+        .correctable = false,
+    }, {
+        .name = "SDN",
+        .val = PCI_ERR_UNC_SDN,
+        .correctable = false,
+    }, {
+        .name = "POISON_TLP",
+        .val = PCI_ERR_UNC_POISON_TLP,
+        .correctable = false,
+    }, {
+        .name = "FCP",
+        .val = PCI_ERR_UNC_FCP,
+        .correctable = false,
+    }, {
+        .name = "COMP_TIME",
+        .val = PCI_ERR_UNC_COMP_TIME,
+        .correctable = false,
+    }, {
+        .name = "COMP_ABORT",
+        .val = PCI_ERR_UNC_COMP_ABORT,
+        .correctable = false,
+    }, {
+        .name = "UNX_COMP",
+        .val = PCI_ERR_UNC_UNX_COMP,
+        .correctable = false,
+    }, {
+        .name = "RX_OVER",
+        .val = PCI_ERR_UNC_RX_OVER,
+        .correctable = false,
+    }, {
+        .name = "MALF_TLP",
+        .val = PCI_ERR_UNC_MALF_TLP,
+        .correctable = false,
+    }, {
+        .name = "ECRC",
+        .val = PCI_ERR_UNC_ECRC,
+        .correctable = false,
+    }, {
+        .name = "UNSUP",
+        .val = PCI_ERR_UNC_UNSUP,
+        .correctable = false,
+    }, {
+        .name = "ACSV",
+        .val = PCI_ERR_UNC_ACSV,
+        .correctable = false,
+    }, {
+        .name = "INTN",
+        .val = PCI_ERR_UNC_INTN,
+        .correctable = false,
+    }, {
+        .name = "MCBTLP",
+        .val = PCI_ERR_UNC_MCBTLP,
+        .correctable = false,
+    }, {
+        .name = "ATOP_EBLOCKED",
+        .val = PCI_ERR_UNC_ATOP_EBLOCKED,
+        .correctable = false,
+    }, {
+        .name = "TLP_PRF_BLOCKED",
+        .val = PCI_ERR_UNC_TLP_PRF_BLOCKED,
+        .correctable = false,
+    }, {
+        .name = "RCVR",
+        .val = PCI_ERR_COR_RCVR,
+        .correctable = true,
+    }, {
+        .name = "BAD_TLP",
+        .val = PCI_ERR_COR_BAD_TLP,
+        .correctable = true,
+    }, {
+        .name = "BAD_DLLP",
+        .val = PCI_ERR_COR_BAD_DLLP,
+        .correctable = true,
+    }, {
+        .name = "REP_ROLL",
+        .val = PCI_ERR_COR_REP_ROLL,
+        .correctable = true,
+    }, {
+        .name = "REP_TIMER",
+        .val = PCI_ERR_COR_REP_TIMER,
+        .correctable = true,
+    }, {
+        .name = "ADV_NONFATAL",
+        .val = PCI_ERR_COR_ADV_NONFATAL,
+        .correctable = true,
+    }, {
+        .name = "INTERNAL",
+        .val = PCI_ERR_COR_INTERNAL,
+        .correctable = true,
+    }, {
+        .name = "HL_OVERFLOW",
+        .val = PCI_ERR_COR_HL_OVERFLOW,
+        .correctable = true,
+    },
+};
+
+static int pcie_aer_parse_error_string(const char *error_name,
+                                       uint32_t *status, bool *correctable)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(pcie_aer_error_list); i++) {
+        const  PCIEAERErrorName *e = &pcie_aer_error_list[i];
+        if (strcmp(error_name, e->name)) {
+            continue;
+        }
+
+        *status = e->val;
+        *correctable = e->correctable;
+        return 0;
+    }
+    return -EINVAL;
+}
+
+int do_pcie_aer_inejct_error(Monitor *mon,
+                             const QDict *qdict, QObject **ret_data)
+{
+    const char *pci_fw_dev_path = qdict_get_str(qdict, "pci_fw_dev_path");
+    const char *error_name;
+    uint32_t error_status;
+    bool correctable;
+    PCIDevice *dev;
+    PCIEAERErr err;
+    int ret;
+
+    ret = pci_parse_fw_dev_path(pci_fw_dev_path, &dev);
+    if (ret < 0) {
+        monitor_printf(mon,
+                       "pci device path is invalid or device not found. %s\n",
+                       pci_fw_dev_path);
+        return ret;
+    }
+    if (!pci_is_express(dev)) {
+        monitor_printf(mon, "the device doesn't support pci express. %s\n",
+                       pci_fw_dev_path);
+        return -ENOSYS;
+    }
+
+    error_name = qdict_get_str(qdict, "error_status");
+    if (pcie_aer_parse_error_string(error_name, &error_status, &correctable)) {
+        char *e = NULL;
+        error_status = strtoul(error_name, &e, 0);
+        correctable = !!qdict_get_int(qdict, "correctable");
+        if (!e || *e != '\0') {
+            monitor_printf(mon, "invalid error status value. \"%s\"",
+                           error_name);
+            return -EINVAL;
+        }
+    }
+    err.source_id = (pci_bus_num(dev->bus) << 8) | dev->devfn;
+
+    err.flags = 0;
+    if (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, "header0")) {
+        err.flags |= PCIE_AER_ERR_HEADER_VALID;
+    }
+    if (qdict_haskey(qdict, "prefix0")) {
+        err.flags |= PCIE_AER_ERR_TLP_PREFIX_PRESENT;
+    }
+
+    err.header[0] = qdict_get_try_int(qdict, "header0", 0);
+    err.header[1] = qdict_get_try_int(qdict, "header1", 0);
+    err.header[2] = qdict_get_try_int(qdict, "header2", 0);
+    err.header[3] = qdict_get_try_int(qdict, "header3", 0);
+
+    err.prefix[0] = qdict_get_try_int(qdict, "prefix0", 0);
+    err.prefix[1] = qdict_get_try_int(qdict, "prefix1", 0);
+    err.prefix[2] = qdict_get_try_int(qdict, "prefix2", 0);
+    err.prefix[3] = qdict_get_try_int(qdict, "prefix3", 0);
+
+    ret = pcie_aer_inject_error(dev, &err);
+    *ret_data = qobject_from_jsonf("{'fw_dev_path': %s, "
+                                   "'domain': %d, 'bus': %d, 'devfn': %d, "
+                                   "'ret': %d}",
+                                   pci_fw_dev_path,
+                                   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 38a20a3..4182aac 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -157,6 +157,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

[Qemu-devel] Re: [PATCH 2/3] pci: introduce a parser for fw device path to pci device

[Michael S. Tsirkin]

On Wed, Dec 22, 2010 at 07:54:49PM +0900, Isaku Yamahata wrote:
> Introduce a function to parse fw device path to pci device.
> the format is
> /pci@{<ioport>, <mmio>}/[<fw_name>]@<slot>,<func>/.../[<fw_name>]@<slot>,<func>
> 
> <ioport> = "i"<ioport addr in hex>
> <mmio> = <mmio addr in hex>
> <slot> = slot number in hex
> <func> = func number in hex
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

What concerns me the most here is the use of io addresses,
not sure it's the right thing for the command interface.

Why do we need to support full path at all?  Can we use the id of the
parent bus for this?  Supplying a bus id for the device seems like a
natural way to describe a tree, with minimal need for parsing.

> ---
>  hw/pci.c |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci.h |    2 +
>  2 files changed, 130 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index eb21848..a52a323 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -2027,3 +2027,131 @@ static char *pcibus_get_dev_path(DeviceState *dev)
>      return strdup(path);
>  }
>  
> +/*
> + * Parse format and get PCIDevice
> + * return 0 on success
> + *       <0 on error: format is invalid or device isn't found.
> + *
> + * Format:
> + * /pci@{<ioport>, <mmio>}/[<fw_name>]@<slot>,<func>/...
> + *                     .../[<fw_name>]@<slot>,<func>
> + *
> + * <ioport> = "i"<ioport addr in hex>
> + * <mmio> = <mmio addr in hex>
> + * <slot> = slot number in hex
> + * <func> = func number in hex
> + *
> + */
> +int pci_parse_fw_dev_path(const char *path, PCIDevice **pdev)
> +{
> +    const char *p = path;
> +    char *e;
> +    size_t len;
> +    PCIBus *bus;
> +    struct PCIHostBus *host;
> +
> +    if (*p != '/') {
> +        return -EINVAL;
> +    }
> +    e = strchr(p + 1, '/');
> +    if (e == NULL) {
> +        return -EINVAL;
> +    }
> +    len = e - p;
> +    p = e + 1;
> +
> +    bus = NULL;
> +    QLIST_FOREACH(host, &host_buses, next) {
> +        DeviceState *qdev = host->bus->qbus.parent;
> +        if (qdev) {
> +            char *devpath = qdev_get_fw_dev_path(qdev);
> +
> +            if (len == strlen(devpath) && !strncmp(devpath, path, len)) {
> +                bus = host->bus;
> +                qemu_free(devpath);
> +                break;
> +            }
> +            qemu_free(devpath);
> +        } else {
> +            /* This pci bus doesn't have host-to-pci bridge device.
> +             * Check only if the path is pci ignoring other parameters. */
> +#define PCI_FW_PATH     "/pci@"
> +            if (strncmp(path, PCI_FW_PATH, strlen(PCI_FW_PATH))) {
> +                return -EINVAL;
> +            }
> +            bus = host->bus;
> +            break;
> +        }
> +    }
> +
> +    for (;;) {
> +        char *at;
> +        char *comma;
> +        unsigned long slot;
> +        unsigned long func;
> +        PCIDevice *dev;
> +        PCIBus *child_bus;
> +
> +        if (!bus) {
> +            return -ENODEV;
> +        }
> +        if (*p == '\0') {
> +            return -EINVAL;
> +        }
> +
> +        at = strchr(p, '@');
> +        if (at == NULL) {
> +            return -EINVAL;
> +        }
> +        slot = strtoul(at + 1, &e, 16);
> +        if (e == at + 1 || *e != ',') {
> +            return -EINVAL;
> +        }
> +        if (slot >= PCI_SLOT_MAX) {
> +            return -EINVAL;
> +        }
> +
> +        comma = e;
> +        func = strtoul(comma + 1, &e, 16);
> +        if (e == comma + 1 || (*e != '/' && *e != '\0')) {
> +            return -EINVAL;
> +        }
> +        if (func >= PCI_FUNC_MAX) {
> +            return -EINVAL;
> +        }
> +
> +        len = e - p;
> +        dev = bus->devices[PCI_DEVFN(slot, func)];
> +        if (!dev) {
> +            return -ENODEV;
> +        }
> +        if (at != p) {
> +            /* fw_name is specified. */
> +            char *fw_dev_path = pcibus_get_fw_dev_path(&dev->qdev);
> +            if (strncmp(p, fw_dev_path, len)) {
> +                qemu_free(fw_dev_path);
> +                return -EINVAL;
> +            }
> +            qemu_free(fw_dev_path);
> +        }
> +
> +        if (*e == '\0') {
> +            *pdev = dev;
> +            return 0;
> +        }
> +
> +        /*
> +         * descending down pci-to-pci bridge.
> +         * At the moment, there is no way to safely determine if the given
> +         * pci device is really pci-to-pci device.
> +         */
> +        p = e;
> +        QLIST_FOREACH(child_bus, &bus->child, sibling) {
> +            if (child_bus->parent_dev == dev) {
> +                bus = child_bus;
> +                continue;
> +            }
> +        }
> +        bus = NULL;
> +    }
> +}
> diff --git a/hw/pci.h b/hw/pci.h
> index 6e80b08..96f8d52 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -16,6 +16,7 @@
>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>  #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>  #define PCI_FUNC(devfn)         ((devfn) & 0x07)
> +#define PCI_SLOT_MAX            32
>  #define PCI_FUNC_MAX            8
>  
>  /* Class, Vendor and Device IDs from Linux's pci_ids.h */
> @@ -258,6 +259,7 @@ int pci_parse_devaddr(const char *addr, int *domp, int *busp,
>                        unsigned int *slotp, unsigned int *funcp);
>  int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
>                       unsigned *slotp);
> +int pci_parse_fw_dev_path(const char *path, PCIDevice **pdev);
>  
>  void do_pci_info_print(Monitor *mon, const QObject *data);
>  void do_pci_info(Monitor *mon, QObject **ret_data);
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH 1/3] build, pci: remove QMP dependency on core PCI code

[Michael S. Tsirkin]

On Wed, Dec 22, 2010 at 07:54:48PM +0900, Isaku Yamahata wrote:
> by introducing pci-stub.c, eliminate QMP dependency on core PCI code
> rquired by query-pci command.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Yay! Applied.

> ---
>  Makefile.objs   |    4 +---
>  Makefile.target |    2 ++
>  hw/pci-stub.c   |   37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+), 3 deletions(-)
>  create mode 100644 hw/pci-stub.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index d6b3d60..c3e52c5 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -169,9 +169,7 @@ hw-obj-y =
>  hw-obj-y += vl.o loader.o
>  hw-obj-$(CONFIG_VIRTIO) += virtio.o virtio-console.o
>  hw-obj-y += fw_cfg.o
> -# FIXME: Core PCI code and its direct dependencies are required by the
> -# QMP query-pci command.
> -hw-obj-y += pci.o pci_bridge.o
> +hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o
>  hw-obj-$(CONFIG_PCI) += msix.o msi.o
>  hw-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
>  hw-obj-$(CONFIG_PCI) += ioh3420.o xio3130_upstream.o xio3130_downstream.o
> diff --git a/Makefile.target b/Makefile.target
> index d08f5dd..38582d4 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -1,6 +1,7 @@
>  # -*- Mode: makefile -*-
>  
>  GENERATED_HEADERS = config-target.h
> +CONFIG_NO_PCI = $(if $(subst n,,$(CONFIG_PCI)),n,y)
>  CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)
>  
>  include ../config-host.mak
> @@ -188,6 +189,7 @@ ifdef CONFIG_SOFTMMU
>  obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o
>  # virtio has to be here due to weird dependency between PCI and virtio-net.
>  # need to fix this properly
> +obj-$(CONFIG_NO_PCI) += pci-stub.o
>  obj-$(CONFIG_VIRTIO) += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o
>  obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>  obj-y += vhost_net.o
> diff --git a/hw/pci-stub.c b/hw/pci-stub.c
> new file mode 100644
> index 0000000..674591d
> --- /dev/null
> +++ b/hw/pci-stub.c
> @@ -0,0 +1,37 @@
> +/*
> + * PCI stubs for plathome that doesn't support pci bus.
> + *
> + * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "monitor.h"
> +#include "pci.h"
> +
> +static void pci_error_message(Monitor *mon)
> +{
> +    monitor_printf(mon, "PCI devices not supported\n");
> +}
> +
> +void do_pci_info(Monitor *mon, QObject **ret_data)
> +{
> +    pci_error_message(mon);
> +}
> +
> +void do_pci_info_print(Monitor *mon, const QObject *data)
> +{
> +    pci_error_message(mon);
> +}
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH 2/3] pci: introduce a parser for fw device path to pci device

[Isaku Yamahata]

On Wed, Dec 22, 2010 at 01:04:43PM +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 22, 2010 at 07:54:49PM +0900, Isaku Yamahata wrote:
> > Introduce a function to parse fw device path to pci device.
> > the format is
> > /pci@{<ioport>, <mmio>}/[<fw_name>]@<slot>,<func>/.../[<fw_name>]@<slot>,<func>
> > 
> > <ioport> = "i"<ioport addr in hex>
> > <mmio> = <mmio addr in hex>
> > <slot> = slot number in hex
> > <func> = func number in hex
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> What concerns me the most here is the use of io addresses,
> not sure it's the right thing for the command interface.
>
> Why do we need to support full path at all?  Can we use the id of the
> parent bus for this?  Supplying a bus id for the device seems like a
> natural way to describe a tree, with minimal need for parsing.

The ids of most devices are set NULL currently. So the id is useless
right now unfortunately. Maybe how to assign ids to all of qdevs
systematically would be difficult.

To be honest, I don't have strong opinion for format of pci topology.
So far we discussed the following candidates.
what format do you prefer?

- domain:<bus>:<slot>.<func>
  guest assigns bus number, so this can't be used for qemu internal use.
- domain:00:<slot>.<func>:...:<slot>.<func>
- fw device path
- id
  Unfortunately id is NULL for most devices right now. So id doesn't work.
- any other?

thanks,
-- 
yamahata

[Qemu-devel] Re: [PATCH 2/3] pci: introduce a parser for fw device path to pci device

[Michael S. Tsirkin]

On Wed, Dec 22, 2010 at 08:36:40PM +0900, Isaku Yamahata wrote:
> On Wed, Dec 22, 2010 at 01:04:43PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Dec 22, 2010 at 07:54:49PM +0900, Isaku Yamahata wrote:
> > > Introduce a function to parse fw device path to pci device.
> > > the format is
> > > /pci@{<ioport>, <mmio>}/[<fw_name>]@<slot>,<func>/.../[<fw_name>]@<slot>,<func>
> > > 
> > > <ioport> = "i"<ioport addr in hex>
> > > <mmio> = <mmio addr in hex>
> > > <slot> = slot number in hex
> > > <func> = func number in hex
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > What concerns me the most here is the use of io addresses,
> > not sure it's the right thing for the command interface.
> >
> > Why do we need to support full path at all?  Can we use the id of the
> > parent bus for this?  Supplying a bus id for the device seems like a
> > natural way to describe a tree, with minimal need for parsing.
> 
> The ids of most devices are set NULL currently. So the id is useless
> right now unfortunately.

It's up to the user to assign ids. If one doesn't one won't be able
to activate hotplug/aer, which does not seem like a serious limitation.

> Maybe how to assign ids to all of qdevs
> systematically would be difficult.

I don't think it's urgent. No id -> can't use some of the
functionality. No big deal IMO.

> To be honest, I don't have strong opinion for format of pci topology.
> So far we discussed the following candidates.
> what format do you prefer?
> 
> - domain:<bus>:<slot>.<func>
>   guest assigns bus number, so this can't be used for qemu internal use.
> - domain:00:<slot>.<func>:...:<slot>.<func>
> - fw device path
> - id
>   Unfortunately id is NULL for most devices right now. So id doesn't work.
> - any other?
> 
> thanks,
> -- 
> yamahata

[Qemu-devel] Re: [PATCH 2/3] pci: introduce a parser for fw device path to pci device

[Isaku Yamahata]

On Wed, Dec 22, 2010 at 02:03:55PM +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 22, 2010 at 08:36:40PM +0900, Isaku Yamahata wrote:
> > On Wed, Dec 22, 2010 at 01:04:43PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Dec 22, 2010 at 07:54:49PM +0900, Isaku Yamahata wrote:
> > > > Introduce a function to parse fw device path to pci device.
> > > > the format is
> > > > /pci@{<ioport>, <mmio>}/[<fw_name>]@<slot>,<func>/.../[<fw_name>]@<slot>,<func>
> > > > 
> > > > <ioport> = "i"<ioport addr in hex>
> > > > <mmio> = <mmio addr in hex>
> > > > <slot> = slot number in hex
> > > > <func> = func number in hex
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > 
> > > What concerns me the most here is the use of io addresses,
> > > not sure it's the right thing for the command interface.
> > >
> > > Why do we need to support full path at all?  Can we use the id of the
> > > parent bus for this?  Supplying a bus id for the device seems like a
> > > natural way to describe a tree, with minimal need for parsing.
> > 
> > The ids of most devices are set NULL currently. So the id is useless
> > right now unfortunately.
> 
> It's up to the user to assign ids. If one doesn't one won't be able
> to activate hotplug/aer, which does not seem like a serious limitation.
> 
> > Maybe how to assign ids to all of qdevs
> > systematically would be difficult.
> 
> I don't think it's urgent. No id -> can't use some of the
> functionality. No big deal IMO.

Hmm, it's big deal for me.
How about trying id first, if failed, then trying fw device path
as fall back?

> 
> > To be honest, I don't have strong opinion for format of pci topology.
> > So far we discussed the following candidates.
> > what format do you prefer?
> > 
> > - domain:<bus>:<slot>.<func>
> >   guest assigns bus number, so this can't be used for qemu internal use.
> > - domain:00:<slot>.<func>:...:<slot>.<func>
> > - fw device path
> > - id
> >   Unfortunately id is NULL for most devices right now. So id doesn't work.
> > - any other?
> > 
> > thanks,
> > -- 
> > yamahata
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 2/3] pci: introduce a parser for fw device path to pci device

[Michael S. Tsirkin]

On Fri, Dec 24, 2010 at 10:57:56AM +0900, Isaku Yamahata wrote:
> On Wed, Dec 22, 2010 at 02:03:55PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Dec 22, 2010 at 08:36:40PM +0900, Isaku Yamahata wrote:
> > > On Wed, Dec 22, 2010 at 01:04:43PM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Dec 22, 2010 at 07:54:49PM +0900, Isaku Yamahata wrote:
> > > > > Introduce a function to parse fw device path to pci device.
> > > > > the format is
> > > > > /pci@{<ioport>, <mmio>}/[<fw_name>]@<slot>,<func>/.../[<fw_name>]@<slot>,<func>
> > > > > 
> > > > > <ioport> = "i"<ioport addr in hex>
> > > > > <mmio> = <mmio addr in hex>
> > > > > <slot> = slot number in hex
> > > > > <func> = func number in hex
> > > > > 
> > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > 
> > > > What concerns me the most here is the use of io addresses,
> > > > not sure it's the right thing for the command interface.
> > > >
> > > > Why do we need to support full path at all?  Can we use the id of the
> > > > parent bus for this?  Supplying a bus id for the device seems like a
> > > > natural way to describe a tree, with minimal need for parsing.
> > > 
> > > The ids of most devices are set NULL currently. So the id is useless
> > > right now unfortunately.
> > 
> > It's up to the user to assign ids. If one doesn't one won't be able
> > to activate hotplug/aer, which does not seem like a serious limitation.
> > 
> > > Maybe how to assign ids to all of qdevs
> > > systematically would be difficult.
> > 
> > I don't think it's urgent. No id -> can't use some of the
> > functionality. No big deal IMO.
> 
> Hmm, it's big deal for me.

OK, but why? Another push in qdev/machine description direction can only
be a good thing, no?

> How about trying id first, if failed, then trying fw device path
> as fall back?

Well what bothered me with the proposed format in cli didn't go away.
Specifically the use of the io address as device name really looks strange,
and the mmio address is guest assigned, isn't it?
Also, for QMP, it seems we should be using arrays and attributes,
instead of asking everyone to build up/parse a path.

> > 
> > > To be honest, I don't have strong opinion for format of pci topology.
> > > So far we discussed the following candidates.
> > > what format do you prefer?
> > > 
> > > - domain:<bus>:<slot>.<func>
> > >   guest assigns bus number, so this can't be used for qemu internal use.
> > > - domain:00:<slot>.<func>:...:<slot>.<func>
> > > - fw device path
> > > - id
> > >   Unfortunately id is NULL for most devices right now. So id doesn't work.
> > > - any other?
> > > 
> > > thanks,
> > > -- 
> > > yamahata
> > 
> 
> -- 
> yamahata