| Message ID | f8b4c010a407716b93cd4aa2f397adb4dac64027.1293015085.git.yamahata@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
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
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,
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
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 >
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
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);
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(-)