| Message ID | 1289409261-5418-9-git-send-email-gleb@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: > > Signed-off-by: Gleb Natapov <gleb@redhat.com> Good stuff. We should also consider using this for CLI and monitor. Some comments below. > --- > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 54 insertions(+), 0 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 92aaa85..ab0399c 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -63,12 +63,14 @@ struct PCIBus { > > static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); > static char *pcibus_get_dev_path(DeviceState *dev); > +static char *pcibus_get_fw_dev_path(DeviceState *dev); > > static struct BusInfo pci_bus_info = { > .name = "PCI", > .size = sizeof(PCIBus), > .print_dev = pcibus_dev_print, > .get_dev_path = pcibus_get_dev_path, > + .get_fw_dev_path = pcibus_get_fw_dev_path, > .props = (Property[]) { > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > @@ -2135,6 +2137,58 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent) > } > } > > +static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len) > +{ > + PCIDevice *d = (PCIDevice *)dev; > + const char *name = NULL; > + > +#define E(R, N) case R: name = N; break; Straight from underhanded C contest :). Does this macro really buy us all that much? Split this switch to an inline function, we'll get case 0x0600: return "host"; and then we won't need the macro. > + > + /* names taken from pci binding for open firmware */ > + switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) { > + E(0x0001, "display"); E(0x0100, "scsi"); E(0x0101, "ide"); > + E(0x0102, "fdc"); E(0x0103, "ipi"); E(0x0104, "raid"); > + E(0x0200, "ethernet"); E(0x0201, "token-ring"); E(0x0202, "fddi"); > + E(0x0203, "atm"); E(0x0300 ... 0x03ff, "display"); E(0x0400, "video"); > + E(0x0401, "sound"); E(0x0500, "memory"); E(0x0501, "flash"); > + E(0x0600, "host"); E(0x0601, "isa"); E(0x0602, "eisa"); > + E(0x0603, "mca"); E(0x0604, "pci"); E(0x0605, "pcmcia"); > + E(0x0606, "nubus"); E(0x0607, "cardbus"); E(0x0700, "serial"); > + E(0x0701, "parallel"); E(0x0800, "interrupt-controller"); > + E(0x0801, "dma-controller"); E(0x0802, "timer"); > + E(0x0803, "rtc"); E(0x0900, "keyboard"); E(0x0901, "pen"); > + E(0x0902, "mouse"); E(0x0a00 ... 0x0aff, "dock"); > + E(0x0b00 ... 0x0bff, "cpu"); E(0x0c00, "fireware"); > + E(0x0c01, "access-bus"); E(0x0c02, "ssa"); E(0x0c03, "usb"); > + E(0x0c04, "fibre-channel"); Please use constants from hw/pci_ids.h > + } > +#undef E > + > + if (name) { > + pstrcpy(buf, len, name); > + } else { > + snprintf(buf, len, "pci%04x,%04x", > + pci_get_word(d->config + PCI_VENDOR_ID), > + pci_get_word(d->config + PCI_DEVICE_ID)); > + } > + > + return buf; > +} > + > +static char *pcibus_get_fw_dev_path(DeviceState *dev) > +{ > + PCIDevice *d = (PCIDevice *)dev; > + char path[50], name[33]; > + int off; > + > + off = snprintf(path, sizeof(path), "%s@%x", > + pci_dev_fw_name(dev, name, sizeof name), > + PCI_SLOT(d->devfn)); > + if (PCI_FUNC(d->devfn)) > + snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn)); > + return strdup(path); > +} > + > static char *pcibus_get_dev_path(DeviceState *dev) > { > PCIDevice *d = (PCIDevice *)dev; > -- > 1.7.1
On Wed, Nov 10, 2010 at 07:34:12PM +0200, Michael S. Tsirkin wrote: > On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: > > > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > > Good stuff. We should also consider using this for > CLI and monitor. Some comments below. > > > --- > > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 files changed, 54 insertions(+), 0 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index 92aaa85..ab0399c 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -63,12 +63,14 @@ struct PCIBus { > > > > static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); > > static char *pcibus_get_dev_path(DeviceState *dev); > > +static char *pcibus_get_fw_dev_path(DeviceState *dev); > > > > static struct BusInfo pci_bus_info = { > > .name = "PCI", > > .size = sizeof(PCIBus), > > .print_dev = pcibus_dev_print, > > .get_dev_path = pcibus_get_dev_path, > > + .get_fw_dev_path = pcibus_get_fw_dev_path, > > .props = (Property[]) { > > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > > @@ -2135,6 +2137,58 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent) > > } > > } > > > > +static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len) > > +{ > > + PCIDevice *d = (PCIDevice *)dev; > > + const char *name = NULL; > > + > > +#define E(R, N) case R: name = N; break; > > Straight from underhanded C contest :). > Does this macro really buy us all that much? Easy typing. > Split this switch to an inline function, we'll get > case 0x0600: return "host"; > and then we won't need the macro. > > > + > > + /* names taken from pci binding for open firmware */ > > + switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) { > > + E(0x0001, "display"); E(0x0100, "scsi"); E(0x0101, "ide"); > > + E(0x0102, "fdc"); E(0x0103, "ipi"); E(0x0104, "raid"); > > + E(0x0200, "ethernet"); E(0x0201, "token-ring"); E(0x0202, "fddi"); > > + E(0x0203, "atm"); E(0x0300 ... 0x03ff, "display"); E(0x0400, "video"); > > + E(0x0401, "sound"); E(0x0500, "memory"); E(0x0501, "flash"); > > + E(0x0600, "host"); E(0x0601, "isa"); E(0x0602, "eisa"); > > + E(0x0603, "mca"); E(0x0604, "pci"); E(0x0605, "pcmcia"); > > + E(0x0606, "nubus"); E(0x0607, "cardbus"); E(0x0700, "serial"); > > + E(0x0701, "parallel"); E(0x0800, "interrupt-controller"); > > + E(0x0801, "dma-controller"); E(0x0802, "timer"); > > + E(0x0803, "rtc"); E(0x0900, "keyboard"); E(0x0901, "pen"); > > + E(0x0902, "mouse"); E(0x0a00 ... 0x0aff, "dock"); > > + E(0x0b00 ... 0x0bff, "cpu"); E(0x0c00, "fireware"); > > + E(0x0c01, "access-bus"); E(0x0c02, "ssa"); E(0x0c03, "usb"); > > + E(0x0c04, "fibre-channel"); > > Please use constants from > hw/pci_ids.h OK. But this will make macro even more handy :) Are all classes defined there? > > > > + } > > +#undef E > > + > > + if (name) { > > + pstrcpy(buf, len, name); > > + } else { > > + snprintf(buf, len, "pci%04x,%04x", > > + pci_get_word(d->config + PCI_VENDOR_ID), > > + pci_get_word(d->config + PCI_DEVICE_ID)); > > + } > > + > > + return buf; > > +} > > + > > +static char *pcibus_get_fw_dev_path(DeviceState *dev) > > +{ > > + PCIDevice *d = (PCIDevice *)dev; > > + char path[50], name[33]; > > + int off; > > + > > + off = snprintf(path, sizeof(path), "%s@%x", > > + pci_dev_fw_name(dev, name, sizeof name), > > + PCI_SLOT(d->devfn)); > > + if (PCI_FUNC(d->devfn)) > > + snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn)); > > + return strdup(path); > > +} > > + > > static char *pcibus_get_dev_path(DeviceState *dev) > > { > > PCIDevice *d = (PCIDevice *)dev; > > -- > > 1.7.1 -- Gleb.
On Wed, Nov 10, 2010 at 08:02:12PM +0200, Gleb Natapov wrote: > On Wed, Nov 10, 2010 at 07:34:12PM +0200, Michael S. Tsirkin wrote: > > On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: > > > > > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > > > > Good stuff. We should also consider using this for > > CLI and monitor. Some comments below. > > > > > --- > > > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 files changed, 54 insertions(+), 0 deletions(-) > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > index 92aaa85..ab0399c 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -63,12 +63,14 @@ struct PCIBus { > > > > > > static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); > > > static char *pcibus_get_dev_path(DeviceState *dev); > > > +static char *pcibus_get_fw_dev_path(DeviceState *dev); > > > > > > static struct BusInfo pci_bus_info = { > > > .name = "PCI", > > > .size = sizeof(PCIBus), > > > .print_dev = pcibus_dev_print, > > > .get_dev_path = pcibus_get_dev_path, > > > + .get_fw_dev_path = pcibus_get_fw_dev_path, > > > .props = (Property[]) { > > > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > > > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > > > @@ -2135,6 +2137,58 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent) > > > } > > > } > > > > > > +static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len) > > > +{ > > > + PCIDevice *d = (PCIDevice *)dev; > > > + const char *name = NULL; > > > + > > > +#define E(R, N) case R: name = N; break; > > > > Straight from underhanded C contest :). > > Does this macro really buy us all that much? > Easy typing. I am serious about underhanded C. Macros like this one are their tool of choice. > > Split this switch to an inline function, we'll get > > case 0x0600: return "host"; > > and then we won't need the macro. > > > > > + > > > + /* names taken from pci binding for open firmware */ > > > + switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) { > > > + E(0x0001, "display"); E(0x0100, "scsi"); E(0x0101, "ide"); > > > + E(0x0102, "fdc"); E(0x0103, "ipi"); E(0x0104, "raid"); > > > + E(0x0200, "ethernet"); E(0x0201, "token-ring"); E(0x0202, "fddi"); > > > + E(0x0203, "atm"); E(0x0300 ... 0x03ff, "display"); E(0x0400, "video"); > > > + E(0x0401, "sound"); E(0x0500, "memory"); E(0x0501, "flash"); > > > + E(0x0600, "host"); E(0x0601, "isa"); E(0x0602, "eisa"); > > > + E(0x0603, "mca"); E(0x0604, "pci"); E(0x0605, "pcmcia"); > > > + E(0x0606, "nubus"); E(0x0607, "cardbus"); E(0x0700, "serial"); > > > + E(0x0701, "parallel"); E(0x0800, "interrupt-controller"); > > > + E(0x0801, "dma-controller"); E(0x0802, "timer"); > > > + E(0x0803, "rtc"); E(0x0900, "keyboard"); E(0x0901, "pen"); > > > + E(0x0902, "mouse"); E(0x0a00 ... 0x0aff, "dock"); > > > + E(0x0b00 ... 0x0bff, "cpu"); E(0x0c00, "fireware"); > > > + E(0x0c01, "access-bus"); E(0x0c02, "ssa"); E(0x0c03, "usb"); > > > + E(0x0c04, "fibre-channel"); > > > > Please use constants from > > hw/pci_ids.h > OK. But this will make macro even more handy :) Are all classes defined there? Not in qemu, but the linux pci_ids has them all I think. So just add stuff from there to qemu. > > > > > > > + } > > > +#undef E > > > + > > > + if (name) { > > > + pstrcpy(buf, len, name); > > > + } else { > > > + snprintf(buf, len, "pci%04x,%04x", > > > + pci_get_word(d->config + PCI_VENDOR_ID), > > > + pci_get_word(d->config + PCI_DEVICE_ID)); > > > + } > > > + > > > + return buf; > > > +} > > > + > > > +static char *pcibus_get_fw_dev_path(DeviceState *dev) > > > +{ > > > + PCIDevice *d = (PCIDevice *)dev; > > > + char path[50], name[33]; > > > + int off; > > > + > > > + off = snprintf(path, sizeof(path), "%s@%x", > > > + pci_dev_fw_name(dev, name, sizeof name), > > > + PCI_SLOT(d->devfn)); > > > + if (PCI_FUNC(d->devfn)) > > > + snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn)); > > > + return strdup(path); > > > +} > > > + > > > static char *pcibus_get_dev_path(DeviceState *dev) > > > { > > > PCIDevice *d = (PCIDevice *)dev; > > > -- > > > 1.7.1 > > -- > Gleb.
On Wed, Nov 10, 2010 at 6:21 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Nov 10, 2010 at 08:02:12PM +0200, Gleb Natapov wrote: >> On Wed, Nov 10, 2010 at 07:34:12PM +0200, Michael S. Tsirkin wrote: >> > On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: >> > > >> > > Signed-off-by: Gleb Natapov <gleb@redhat.com> >> > >> > Good stuff. We should also consider using this for >> > CLI and monitor. Some comments below. >> > >> > > --- >> > > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> > > 1 files changed, 54 insertions(+), 0 deletions(-) >> > > >> > > diff --git a/hw/pci.c b/hw/pci.c >> > > index 92aaa85..ab0399c 100644 >> > > --- a/hw/pci.c >> > > +++ b/hw/pci.c >> > > @@ -63,12 +63,14 @@ struct PCIBus { >> > > >> > > static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); >> > > static char *pcibus_get_dev_path(DeviceState *dev); >> > > +static char *pcibus_get_fw_dev_path(DeviceState *dev); >> > > >> > > static struct BusInfo pci_bus_info = { >> > > .name = "PCI", >> > > .size = sizeof(PCIBus), >> > > .print_dev = pcibus_dev_print, >> > > .get_dev_path = pcibus_get_dev_path, >> > > + .get_fw_dev_path = pcibus_get_fw_dev_path, >> > > .props = (Property[]) { >> > > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), >> > > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), >> > > @@ -2135,6 +2137,58 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent) >> > > } >> > > } >> > > >> > > +static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len) >> > > +{ >> > > + PCIDevice *d = (PCIDevice *)dev; >> > > + const char *name = NULL; >> > > + >> > > +#define E(R, N) case R: name = N; break; >> > >> > Straight from underhanded C contest :). >> > Does this macro really buy us all that much? >> Easy typing. > > I am serious about underhanded C. Macros like this one are their tool > of choice. > >> > Split this switch to an inline function, we'll get >> > case 0x0600: return "host"; >> > and then we won't need the macro. >> > >> > > + >> > > + /* names taken from pci binding for open firmware */ >> > > + switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) { >> > > + E(0x0001, "display"); E(0x0100, "scsi"); E(0x0101, "ide"); >> > > + E(0x0102, "fdc"); E(0x0103, "ipi"); E(0x0104, "raid"); >> > > + E(0x0200, "ethernet"); E(0x0201, "token-ring"); E(0x0202, "fddi"); >> > > + E(0x0203, "atm"); E(0x0300 ... 0x03ff, "display"); E(0x0400, "video"); >> > > + E(0x0401, "sound"); E(0x0500, "memory"); E(0x0501, "flash"); >> > > + E(0x0600, "host"); E(0x0601, "isa"); E(0x0602, "eisa"); >> > > + E(0x0603, "mca"); E(0x0604, "pci"); E(0x0605, "pcmcia"); >> > > + E(0x0606, "nubus"); E(0x0607, "cardbus"); E(0x0700, "serial"); >> > > + E(0x0701, "parallel"); E(0x0800, "interrupt-controller"); >> > > + E(0x0801, "dma-controller"); E(0x0802, "timer"); >> > > + E(0x0803, "rtc"); E(0x0900, "keyboard"); E(0x0901, "pen"); >> > > + E(0x0902, "mouse"); E(0x0a00 ... 0x0aff, "dock"); >> > > + E(0x0b00 ... 0x0bff, "cpu"); E(0x0c00, "fireware"); >> > > + E(0x0c01, "access-bus"); E(0x0c02, "ssa"); E(0x0c03, "usb"); >> > > + E(0x0c04, "fibre-channel"); >> > >> > Please use constants from >> > hw/pci_ids.h >> OK. But this will make macro even more handy :) Are all classes defined there? > > Not in qemu, but the linux pci_ids has them all I think. > So just add stuff from there to qemu. The PCI database in OpenBIOS may also be handy: http://tracker.coreboot.org/trac/openbios/browser/trunk/openbios-devel/drivers/pci_database.c
On Wed, Nov 10, 2010 at 08:21:55PM +0200, Michael S. Tsirkin wrote: > On Wed, Nov 10, 2010 at 08:02:12PM +0200, Gleb Natapov wrote: > > On Wed, Nov 10, 2010 at 07:34:12PM +0200, Michael S. Tsirkin wrote: > > > On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: > > > > > > > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > > > > > > Good stuff. We should also consider using this for > > > CLI and monitor. Some comments below. > > > > > > > --- > > > > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 files changed, 54 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > > index 92aaa85..ab0399c 100644 > > > > --- a/hw/pci.c > > > > +++ b/hw/pci.c > > > > @@ -63,12 +63,14 @@ struct PCIBus { > > > > > > > > static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); > > > > static char *pcibus_get_dev_path(DeviceState *dev); > > > > +static char *pcibus_get_fw_dev_path(DeviceState *dev); > > > > > > > > static struct BusInfo pci_bus_info = { > > > > .name = "PCI", > > > > .size = sizeof(PCIBus), > > > > .print_dev = pcibus_dev_print, > > > > .get_dev_path = pcibus_get_dev_path, > > > > + .get_fw_dev_path = pcibus_get_fw_dev_path, > > > > .props = (Property[]) { > > > > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > > > > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > > > > @@ -2135,6 +2137,58 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent) > > > > } > > > > } > > > > > > > > +static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len) > > > > +{ > > > > + PCIDevice *d = (PCIDevice *)dev; > > > > + const char *name = NULL; > > > > + > > > > +#define E(R, N) case R: name = N; break; > > > > > > Straight from underhanded C contest :). > > > Does this macro really buy us all that much? > > Easy typing. > > I am serious about underhanded C. Macros like this one are their tool > of choice. > Oh, come on. It is not even try to obfuscate anything. There are plenty of places in kernel where we define macro to simplify table creation. This is not a new trick. > > > Split this switch to an inline function, we'll get > > > case 0x0600: return "host"; > > > and then we won't need the macro. > > > > > > > + > > > > + /* names taken from pci binding for open firmware */ > > > > + switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) { > > > > + E(0x0001, "display"); E(0x0100, "scsi"); E(0x0101, "ide"); > > > > + E(0x0102, "fdc"); E(0x0103, "ipi"); E(0x0104, "raid"); > > > > + E(0x0200, "ethernet"); E(0x0201, "token-ring"); E(0x0202, "fddi"); > > > > + E(0x0203, "atm"); E(0x0300 ... 0x03ff, "display"); E(0x0400, "video"); > > > > + E(0x0401, "sound"); E(0x0500, "memory"); E(0x0501, "flash"); > > > > + E(0x0600, "host"); E(0x0601, "isa"); E(0x0602, "eisa"); > > > > + E(0x0603, "mca"); E(0x0604, "pci"); E(0x0605, "pcmcia"); > > > > + E(0x0606, "nubus"); E(0x0607, "cardbus"); E(0x0700, "serial"); > > > > + E(0x0701, "parallel"); E(0x0800, "interrupt-controller"); > > > > + E(0x0801, "dma-controller"); E(0x0802, "timer"); > > > > + E(0x0803, "rtc"); E(0x0900, "keyboard"); E(0x0901, "pen"); > > > > + E(0x0902, "mouse"); E(0x0a00 ... 0x0aff, "dock"); > > > > + E(0x0b00 ... 0x0bff, "cpu"); E(0x0c00, "fireware"); > > > > + E(0x0c01, "access-bus"); E(0x0c02, "ssa"); E(0x0c03, "usb"); > > > > + E(0x0c04, "fibre-channel"); > > > > > > Please use constants from > > > hw/pci_ids.h > > OK. But this will make macro even more handy :) Are all classes defined there? > > Not in qemu, but the linux pci_ids has them all I think. > So just add stuff from there to qemu. > OK. > > > > > > > > > > + } > > > > +#undef E > > > > + > > > > + if (name) { > > > > + pstrcpy(buf, len, name); > > > > + } else { > > > > + snprintf(buf, len, "pci%04x,%04x", > > > > + pci_get_word(d->config + PCI_VENDOR_ID), > > > > + pci_get_word(d->config + PCI_DEVICE_ID)); > > > > + } > > > > + > > > > + return buf; > > > > +} > > > > + > > > > +static char *pcibus_get_fw_dev_path(DeviceState *dev) > > > > +{ > > > > + PCIDevice *d = (PCIDevice *)dev; > > > > + char path[50], name[33]; > > > > + int off; > > > > + > > > > + off = snprintf(path, sizeof(path), "%s@%x", > > > > + pci_dev_fw_name(dev, name, sizeof name), > > > > + PCI_SLOT(d->devfn)); > > > > + if (PCI_FUNC(d->devfn)) > > > > + snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn)); > > > > + return strdup(path); > > > > +} > > > > + > > > > static char *pcibus_get_dev_path(DeviceState *dev) > > > > { > > > > PCIDevice *d = (PCIDevice *)dev; > > > > -- > > > > 1.7.1 > > > > -- > > Gleb. -- Gleb.
On 11/10/10 18:34, Michael S. Tsirkin wrote: > On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: >> >> Signed-off-by: Gleb Natapov<gleb@redhat.com> > > Good stuff. We should also consider using this for > CLI and monitor. Some comments below. Oh, we already have a table to map pci classes to descriptions for 'info pci'. I'd strongly suggest to just add the fw names to that table instead of creating a second one ... cheers, Gerd
On Thu, Nov 11, 2010 at 11:07:01AM +0100, Gerd Hoffmann wrote: > On 11/10/10 18:34, Michael S. Tsirkin wrote: > >On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: > >> > >>Signed-off-by: Gleb Natapov<gleb@redhat.com> > > > >Good stuff. We should also consider using this for > >CLI and monitor. Some comments below. > > Oh, we already have a table to map pci classes to descriptions for > 'info pci'. I'd strongly suggest to just add the fw names to that > table instead of creating a second one ... > Do you mean pci_class_descriptions? For some classes open firmware spec defines single name for all subclasses. For instance all 0Axx should be called "dock" no matter what xx is. This can't be encoded in a simple table. -- Gleb.
>> Oh, we already have a table to map pci classes to descriptions for >> 'info pci'. I'd strongly suggest to just add the fw names to that >> table instead of creating a second one ... >> > Do you mean pci_class_descriptions? Exactly. > For some classes open firmware spec > defines single name for all subclasses. For instance all 0Axx should be > called "dock" no matter what xx is. This can't be encoded in a simple > table. Well, actually it is a list not a table. Should be easy to extend, like adding a mask, so you can have { .class = 0xa000, .mask = 0xff00, .fw_name = "dock", .desc = "whatever" } to match 0xA0xx ... cheers, Gerd
On Thu, Nov 11, 2010 at 11:07:01AM +0100, Gerd Hoffmann wrote: > On 11/10/10 18:34, Michael S. Tsirkin wrote: > >On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: > >> > >>Signed-off-by: Gleb Natapov<gleb@redhat.com> > > > >Good stuff. We should also consider using this for > >CLI and monitor. Some comments below. > > Oh, we already have a table to map pci classes to descriptions for > 'info pci'. You remind me, that one must be fixed to use names from pci_ids.h > I'd strongly suggest to just add the fw names to that > table instead of creating a second one ... > > cheers, > Gerd No, I mean the path. We are currently passing domain:bus:slot.function to point at a device and we know it's broken for nested bridges and kind of broken for multiple domains. It's a bug that we must fix. Class names aren't that interesting to me, and I'd be just as happy with packing these into 2 tables as a single one personally, it all seems to be a matter of style.
On Thu, Nov 11, 2010 at 05:05:11PM +0200, Michael S. Tsirkin wrote: > On Thu, Nov 11, 2010 at 11:07:01AM +0100, Gerd Hoffmann wrote: > > On 11/10/10 18:34, Michael S. Tsirkin wrote: > > >On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: > > >> > > >>Signed-off-by: Gleb Natapov<gleb@redhat.com> > > > > > >Good stuff. We should also consider using this for > > >CLI and monitor. Some comments below. > > > > Oh, we already have a table to map pci classes to descriptions for > > 'info pci'. > > You remind me, that one must be fixed to use names from > pci_ids.h > For now I used numbers like other elements of the table. If I would do the conversion I would use convenience marcros to not have names like PCI_CLASS_NETWORK_TOKEN_RING all over the table :) -- Gleb.
On Thu, Nov 11, 2010 at 06:07:53PM +0200, Gleb Natapov wrote: > On Thu, Nov 11, 2010 at 05:05:11PM +0200, Michael S. Tsirkin wrote: > > On Thu, Nov 11, 2010 at 11:07:01AM +0100, Gerd Hoffmann wrote: > > > On 11/10/10 18:34, Michael S. Tsirkin wrote: > > > >On Wed, Nov 10, 2010 at 07:14:15PM +0200, Gleb Natapov wrote: > > > >> > > > >>Signed-off-by: Gleb Natapov<gleb@redhat.com> > > > > > > > >Good stuff. We should also consider using this for > > > >CLI and monitor. Some comments below. > > > > > > Oh, we already have a table to map pci classes to descriptions for > > > 'info pci'. > > > > You remind me, that one must be fixed to use names from > > pci_ids.h > > > For now I used numbers like other elements of the table. Fair enough. > If I > would do the conversion I would use convenience marcros to not have > names like PCI_CLASS_NETWORK_TOKEN_RING all over the table :) > > -- > Gleb. I'd prefer to keep it simple :)
diff --git a/hw/pci.c b/hw/pci.c index 92aaa85..ab0399c 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -63,12 +63,14 @@ struct PCIBus { static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent); static char *pcibus_get_dev_path(DeviceState *dev); +static char *pcibus_get_fw_dev_path(DeviceState *dev); static struct BusInfo pci_bus_info = { .name = "PCI", .size = sizeof(PCIBus), .print_dev = pcibus_dev_print, .get_dev_path = pcibus_get_dev_path, + .get_fw_dev_path = pcibus_get_fw_dev_path, .props = (Property[]) { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), DEFINE_PROP_STRING("romfile", PCIDevice, romfile), @@ -2135,6 +2137,58 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent) } } +static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len) +{ + PCIDevice *d = (PCIDevice *)dev; + const char *name = NULL; + +#define E(R, N) case R: name = N; break; + + /* names taken from pci binding for open firmware */ + switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) { + E(0x0001, "display"); E(0x0100, "scsi"); E(0x0101, "ide"); + E(0x0102, "fdc"); E(0x0103, "ipi"); E(0x0104, "raid"); + E(0x0200, "ethernet"); E(0x0201, "token-ring"); E(0x0202, "fddi"); + E(0x0203, "atm"); E(0x0300 ... 0x03ff, "display"); E(0x0400, "video"); + E(0x0401, "sound"); E(0x0500, "memory"); E(0x0501, "flash"); + E(0x0600, "host"); E(0x0601, "isa"); E(0x0602, "eisa"); + E(0x0603, "mca"); E(0x0604, "pci"); E(0x0605, "pcmcia"); + E(0x0606, "nubus"); E(0x0607, "cardbus"); E(0x0700, "serial"); + E(0x0701, "parallel"); E(0x0800, "interrupt-controller"); + E(0x0801, "dma-controller"); E(0x0802, "timer"); + E(0x0803, "rtc"); E(0x0900, "keyboard"); E(0x0901, "pen"); + E(0x0902, "mouse"); E(0x0a00 ... 0x0aff, "dock"); + E(0x0b00 ... 0x0bff, "cpu"); E(0x0c00, "fireware"); + E(0x0c01, "access-bus"); E(0x0c02, "ssa"); E(0x0c03, "usb"); + E(0x0c04, "fibre-channel"); + } +#undef E + + if (name) { + pstrcpy(buf, len, name); + } else { + snprintf(buf, len, "pci%04x,%04x", + pci_get_word(d->config + PCI_VENDOR_ID), + pci_get_word(d->config + PCI_DEVICE_ID)); + } + + return buf; +} + +static char *pcibus_get_fw_dev_path(DeviceState *dev) +{ + PCIDevice *d = (PCIDevice *)dev; + char path[50], name[33]; + int off; + + off = snprintf(path, sizeof(path), "%s@%x", + pci_dev_fw_name(dev, name, sizeof name), + PCI_SLOT(d->devfn)); + if (PCI_FUNC(d->devfn)) + snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn)); + return strdup(path); +} + static char *pcibus_get_dev_path(DeviceState *dev) { PCIDevice *d = (PCIDevice *)dev;
Signed-off-by: Gleb Natapov <gleb@redhat.com> --- hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 54 insertions(+), 0 deletions(-)