[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
> /address@hidden<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 <address@hidden>

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:
> + * /address@hidden<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