Re: [PATCHv3 08/14] Add get_fw_dev_path callback for pci bus.

[Gleb Natapov <gleb@redhat.com>]

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.