Patchwork [PATCHv3,08/14] Add get_fw_dev_path callback for pci bus.

login
register
mail settings
Submitter Gleb Natapov
Date Nov. 10, 2010, 5:14 p.m.
Message ID <1289409261-5418-9-git-send-email-gleb@redhat.com>
Download mbox | patch
Permalink /patch/70659/
State New
Headers show

Comments

Gleb Natapov - Nov. 10, 2010, 5:14 p.m.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 hw/pci.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)
Michael S. Tsirkin - Nov. 10, 2010, 5:34 p.m.
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
Gleb Natapov - Nov. 10, 2010, 6:02 p.m.
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.
Michael S. Tsirkin - Nov. 10, 2010, 6:21 p.m.
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.
Blue Swirl - Nov. 10, 2010, 6:25 p.m.
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
Gleb Natapov - Nov. 10, 2010, 6:33 p.m.
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.
Gerd Hoffmann - Nov. 11, 2010, 10:07 a.m.
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
Gleb Natapov - Nov. 11, 2010, 10:17 a.m.
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.
Gerd Hoffmann - Nov. 11, 2010, 11 a.m.
>> 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
Michael S. Tsirkin - Nov. 11, 2010, 3:05 p.m.
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.
Gleb Natapov - Nov. 11, 2010, 4:07 p.m.
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.
Michael S. Tsirkin - Nov. 11, 2010, 5:14 p.m.
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 :)

Patch

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;